Added "Use an autoconfiguration URL" checkbox in proxy tab

chrome/browser/chromeos/proxy_cros_settings_parser.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/chromeos/proxy_cros_settings_parser.h"
 
 #include "base/string_util.h"
 #include "base/values.h"
 #include "chrome/browser/chromeos/proxy_config_service_impl.h"
 #include "chrome/browser/profiles/profile.h"
@@ skipping 11 matching lines @@
 const char kProxyHttpPort[]       = "cros.session.proxy.httpport";
 const char kProxyHttpsUrl[]       = "cros.session.proxy.httpsurl";
 const char kProxyHttpsPort[]      = "cros.session.proxy.httpsport";
 const char kProxyType[]           = "cros.session.proxy.type";
 const char kProxySingle[]         = "cros.session.proxy.single";
 const char kProxyFtpUrl[]         = "cros.session.proxy.ftpurl";
 const char kProxyFtpPort[]        = "cros.session.proxy.ftpport";
 const char kProxySocks[]          = "cros.session.proxy.socks";
 const char kProxySocksPort[]      = "cros.session.proxy.socksport";
 const char kProxyIgnoreList[]     = "cros.session.proxy.ignorelist";
+const char kProxyUsePacUrl[]      = "cros.session.proxy.usepacurl";

[pneubeck (no reviews), 2013/04/09 09:48:19]

I don't like this forwarding of every UI element's value to C++ but rather keep
as many details locally in JS as possible.

What you have in the UI is a two level nesting of conditionals:
  'type (direct/manual/auto)'
  if 'type == auto' -> 'use pac URL'
  if 'use pac URL' -> 'pac URL'

but in C++ there is only one conditional: 'type (direct/manual/PAC/WPAD)'.

I'd very much prefer, if you did the mapping in JS and not in C++.
This would in particular allow to verify the URL (at least check for the input
field to be non-empty) at first within JS before sending pref changes to C++.

[sathish.kuppuswamy, 2013/04/16 16:46:57]

Hi pneubeck,
   Here(.js) you want me to check the URL field is empty or not, when usePacUrl
checkbox is checked and send a preference commit call to save the same to
backend with this call
Preferences.setURLPref(pref,Value,true);  ?
 I didn't dig deep into working of preferences. I just made sure this works
similar to other checkbox(use same proxy) in proxy tab .
  

 
 const char* const kProxySettings[] = {
   kProxyPacUrl,
   kProxySingleHttp,
   kProxySingleHttpPort,
   kProxyHttpUrl,
   kProxyHttpPort,
   kProxyHttpsUrl,
   kProxyHttpsPort,
   kProxyType,
   kProxySingle,
   kProxyFtpUrl,
   kProxyFtpPort,
   kProxySocks,
   kProxySocksPort,
   kProxyIgnoreList,
+  kProxyUsePacUrl

[Dan Beam, 2013/04/06 01:45:14]

nit: ,

 };
 
 // We have to explicitly export this because the arraysize macro doesn't like
 // extern arrays as their size is not known on compile time.
 const size_t kProxySettingsCount = arraysize(kProxySettings);
 
 namespace {
 
 base::Value* CreateServerHostValue(
     const ProxyConfigServiceImpl::ProxyConfig::ManualProxy& proxy) {
@@ skipping 66 matching lines @@
   }
 
   chromeos::ProxyConfigServiceImpl* config_service =
       profile->GetProxyConfigTracker();
   // Retrieve proxy config.
   chromeos::ProxyConfigServiceImpl::ProxyConfig config;
   config_service->UIGetProxyConfig(&config);
 
   if (path == kProxyPacUrl) {
     std::string val;
     if (in_value->GetAsString(&val)) {

[pneubeck (no reviews), 2013/04/09 09:48:19]

Why not check for the checkbox's value?

[sathish.kuppuswamy, 2013/04/16 16:46:57]

Hi pneubeck,
    Its been checked below (path== kProxyUsePacUrl). Removing this above
condition prevented setting PAC URL to proxy Config, which in turn made to fall
always to autodetect. 

       GURL url(val);
       if (url.is_valid())
         config_service->UISetProxyConfigToPACScript(url);
       else
         config_service->UISetProxyConfigToAutoDetect();
     }
   } else if (path == kProxySingleHttp) {
     std::string val;
     if (in_value->GetAsString(&val)) {
       config_service->UISetProxyConfigToSingleProxy(CreateProxyServerFromHost(
@@ skipping 80 matching lines @@
   } else if (path == kProxySingle) {
     bool val;
     if (in_value->GetAsBoolean(&val)) {
       if (val)
         config_service->UISetProxyConfigToSingleProxy(
             config.single_proxy.server);
       else
         config_service->UISetProxyConfigToProxyPerScheme("http",
             config.http_proxy.server);
     }
+  } else if (path == kProxyUsePacUrl) {
+    bool use_pac_url;
+    if (in_value->GetAsBoolean(&use_pac_url)) {
+      if (use_pac_url && config.automatic_proxy.pac_url.is_valid()) {
+        config_service->UISetProxyConfigToPACScript(
+               config.automatic_proxy.pac_url);
+      } else {
+        config_service->UISetProxyConfigToAutoDetect();
+      }
+    }
   } else if (path == kProxyFtpUrl) {
     std::string val;
     if (in_value->GetAsString(&val)) {
       config_service->UISetProxyConfigToProxyPerScheme("ftp",
           CreateProxyServerFromHost(
               val, config.ftp_proxy, net::ProxyServer::SCHEME_HTTP));
     }
   } else if (path == kProxyFtpPort) {
     int val;
     if (in_value->GetAsInteger(&val)) {
@@ skipping 85 matching lines @@
         controlled_by = "other";
         break;
       default:
         if (!config.user_modifiable)
           controlled_by = "shared";
         break;
     }
   } else if (path == kProxySingle) {
     data = base::Value::CreateBooleanValue(config.mode ==
         chromeos::ProxyConfigServiceImpl::ProxyConfig::MODE_SINGLE_PROXY);
+  } else if (path == kProxyUsePacUrl) {
+     data = base::Value::CreateBooleanValue(config.mode ==
+          chromeos::ProxyConfigServiceImpl::ProxyConfig::MODE_PAC_SCRIPT);
   } else if (path == kProxyFtpUrl) {
     data = CreateServerHostValue(config.ftp_proxy);
   } else if (path == kProxySocks) {
     data = CreateServerHostValue(config.socks_proxy);
   } else if (path == kProxyHttpPort) {
     data = CreateServerPortValue(config.http_proxy);
   } else if (path == kProxyHttpsPort) {
     data = CreateServerPortValue(config.https_proxy);
   } else if (path == kProxyFtpPort) {
     data = CreateServerPortValue(config.ftp_proxy);
@@ skipping 22 matching lines @@
   } else {
     dict->SetBoolean("disabled", false);
   }
   *out_value = dict;
   return true;
 }
 
 }  // namespace proxy_cros_settings_parser
 
 }  // namespace chromeos