Proxy settings automation hooks and related proxy settings fixes.

chrome/browser/chromeos/proxy_cros_settings_provider.cc

-// Copyright (c) 2010 The Chromium Authors. All rights reserved.
+// Copyright (c) 2011 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_provider.h"
 
 #include "base/command_line.h"
 #include "base/string_util.h"
 #include "chrome/browser/browser_list.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/common/chrome_switches.h"
@@ skipping 38 matching lines @@
 
   if (path == kProxyPacUrl) {
     std::string val;
     if (in_value->GetAsString(&val)) {
       GURL url(val);
       config_service->UISetProxyConfigToPACScript(url);
     }
   } else if (path == kProxySingleHttp) {
     std::string val;
     if (in_value->GetAsString(&val)) {
-      std::string uri = val;
-      AppendPortIfValid(kProxySingleHttpPort, &uri);
-      config_service->UISetProxyConfigToSingleProxy(
-          net::ProxyServer::FromURI(uri, net::ProxyServer::SCHEME_HTTP));
+      config_service->UISetProxyConfigToSingleProxy(CreateProxyServerFromHost(
+          val, config.single_proxy, net::ProxyServer::SCHEME_HTTP));

[xiyuan, 2011/04/04 21:24:20]

You need AppendPortIfValid to get cached user input because underlying proxy
service does not remember invalid input.
e.g.
if user enters port first without a valid uri, then port will be lost and revert
to default port.

[dtu, 2011/04/04 22:48:57]

This was true previously, when this function was creating a URI string and using
FromURI to parse a ProxyServer from that string. But now it uses the ProxyServer
constructor directly and will happily accept arbitrary URIs.

[xiyuan, 2011/04/04 23:31:18]

It would be great that the backend proxy service would remeber arbitrary values.

Please make sure you test this case:
1. Go to proxy settings and enter an invalid host name (simulate user typo);
2. Enter a valid port number;
3. Click anywhere on settings page to trigger a blur event so that the change is
saved;
4. Refresh proxy settings page and see if the port number is correct.

If that works, then I am okay.

[dtu, 2011/04/05 00:04:11]

I have tried it and it works =)

[xiyuan, 2011/04/05 16:32:49]

Cool. Thanks for trying this out.

Then we don't need a local cache for the user typed values at all. Please remove
cache_ and its references. Or at least put a TODO for this.

[dtu, 2011/04/05 18:48:49]

Huh, I thought I had already done that. Done now.

     }
   } else if (path == kProxySingleHttpPort) {
-    std::string val;
-    if (in_value->GetAsString(&val)) {
-      std::string uri;
-      FormServerUriIfValid(kProxySingleHttp, val, &uri);
-      config_service->UISetProxyConfigToSingleProxy(
-          net::ProxyServer::FromURI(uri, net::ProxyServer::SCHEME_HTTP));
+    int val;
+    if (in_value->GetAsInteger(&val)) {
+      config_service->UISetProxyConfigToSingleProxy(CreateProxyServerFromPort(
+          val, config.single_proxy, net::ProxyServer::SCHEME_HTTP));

[xiyuan, 2011/04/04 21:24:20]

similar, you need FormServerUriIfValid.

We might want to get it a better name though. FormServerUriIfValid and
AppendPortIfValid is not intutive and does not tell you what they do.

[dtu, 2011/04/04 22:48:57]

See above.

     }
   } else if (path == kProxyHttpUrl) {
     std::string val;
     if (in_value->GetAsString(&val)) {
-      std::string uri = val;
-      AppendPortIfValid(kProxyHttpPort, &uri);
       config_service->UISetProxyConfigToProxyPerScheme("http",
-          net::ProxyServer::FromURI(uri, net::ProxyServer::SCHEME_HTTP));
+          CreateProxyServerFromHost(
+              val, config.http_proxy, net::ProxyServer::SCHEME_HTTP));
     }
   } else if (path == kProxyHttpPort) {
-    std::string val;
-    if (in_value->GetAsString(&val)) {
-      std::string uri;
-      FormServerUriIfValid(kProxyHttpUrl, val, &uri);
+    int val;
+    if (in_value->GetAsInteger(&val)) {
       config_service->UISetProxyConfigToProxyPerScheme("http",
-         net::ProxyServer::FromURI(uri, net::ProxyServer::SCHEME_HTTP));
+          CreateProxyServerFromPort(
+              val, config.http_proxy, net::ProxyServer::SCHEME_HTTP));
     }
   } else if (path == kProxyHttpsUrl) {
     std::string val;
     if (in_value->GetAsString(&val)) {
-      std::string uri = val;
-      AppendPortIfValid(kProxyHttpsPort, &uri);
       config_service->UISetProxyConfigToProxyPerScheme("https",
-          net::ProxyServer::FromURI(uri, net::ProxyServer::SCHEME_HTTP));
+          CreateProxyServerFromHost(
+              val, config.https_proxy, net::ProxyServer::SCHEME_HTTPS));
     }
   } else if (path == kProxyHttpsPort) {
-    std::string val;
-    if (in_value->GetAsString(&val)) {
-      std::string uri;
-      FormServerUriIfValid(kProxyHttpsUrl, val, &uri);
+    int val;
+    if (in_value->GetAsInteger(&val)) {
       config_service->UISetProxyConfigToProxyPerScheme("https",
-          net::ProxyServer::FromURI(uri, net::ProxyServer::SCHEME_HTTP));
+          CreateProxyServerFromPort(
+              val, config.https_proxy, net::ProxyServer::SCHEME_HTTPS));
     }
   } else if (path == kProxyType) {
     int val;
     if (in_value->GetAsInteger(&val)) {
       if (val == 3) {
         if (config.automatic_proxy.pac_url.is_valid())
           config_service->UISetProxyConfigToPACScript(
               config.automatic_proxy.pac_url);
         else
           config_service->UISetProxyConfigToAutoDetect();
@@ skipping 38 matching lines @@
       if (val)
         config_service->UISetProxyConfigToSingleProxy(
             config.single_proxy.server);
       else
         config_service->UISetProxyConfigToProxyPerScheme("http",
             config.http_proxy.server);
     }
   } else if (path == kProxyFtpUrl) {
     std::string val;
     if (in_value->GetAsString(&val)) {
-      std::string uri = val;
-      AppendPortIfValid(kProxyFtpPort, &uri);
       config_service->UISetProxyConfigToProxyPerScheme("ftp",
-          net::ProxyServer::FromURI(uri, net::ProxyServer::SCHEME_HTTP));
+          CreateProxyServerFromHost(
+              val, config.ftp_proxy, net::ProxyServer::SCHEME_HTTP));
     }
   } else if (path == kProxyFtpPort) {
-    std::string val;
-    if (in_value->GetAsString(&val)) {
-      std::string uri;
-      FormServerUriIfValid(kProxyFtpUrl, val, &uri);
+    int val;
+    if (in_value->GetAsInteger(&val)) {
       config_service->UISetProxyConfigToProxyPerScheme("ftp",
-          net::ProxyServer::FromURI(uri, net::ProxyServer::SCHEME_HTTP));
+          CreateProxyServerFromPort(
+              val, config.ftp_proxy, net::ProxyServer::SCHEME_HTTP));
     }
   } else if (path == kProxySocks) {
     std::string val;
     if (in_value->GetAsString(&val)) {
-      std::string uri = val;
-      AppendPortIfValid(kProxySocksPort, &uri);
       config_service->UISetProxyConfigToProxyPerScheme("socks",
-          net::ProxyServer::FromURI(uri,
-              StartsWithASCII(uri, "socks4://", false) ?
-                  net::ProxyServer::SCHEME_SOCKS4 :
-                  net::ProxyServer::SCHEME_SOCKS5));
+          CreateProxyServerFromHost(val, config.socks_proxy,
+                                    StartsWithASCII(val, "socks5://", false) ?
+                                        net::ProxyServer::SCHEME_SOCKS5 :
+                                        net::ProxyServer::SCHEME_SOCKS4));
     }
   } else if (path == kProxySocksPort) {
-    std::string val;
-    if (in_value->GetAsString(&val)) {
-      std::string uri;
-      FormServerUriIfValid(kProxySocks, val, &uri);
+    int val;
+    if (in_value->GetAsInteger(&val)) {
+      std::string host = config.socks_proxy.server.host_port_pair().host();
       config_service->UISetProxyConfigToProxyPerScheme("socks",
-          net::ProxyServer::FromURI(uri,
-              StartsWithASCII(uri, "socks4://", false) ?
-                  net::ProxyServer::SCHEME_SOCKS4 :
-                  net::ProxyServer::SCHEME_SOCKS5));
+          CreateProxyServerFromPort(val, config.socks_proxy,
+                                    StartsWithASCII(host, "socks5://", false) ?
+                                        net::ProxyServer::SCHEME_SOCKS5 :
+                                        net::ProxyServer::SCHEME_SOCKS4));
     }
   } else if (path == kProxyIgnoreList) {
     net::ProxyBypassRules bypass_rules;
     if (in_value->GetType() == Value::TYPE_LIST) {
       const ListValue* list_value = static_cast<const ListValue*>(in_value);
       for (size_t x = 0; x < list_value->GetSize(); x++) {
         std::string val;
         if (list_value->GetString(x, &val)) {
           bypass_rules.AddRuleFromString(val);
         }
@@ skipping 81 matching lines @@
   return ::StartsWithASCII(path, "cros.session.proxy", true);
 }
 
 //----------------- ProxyCrosSettingsProvider: private methods -----------------
 
 chromeos::ProxyConfigServiceImpl*
     ProxyCrosSettingsProvider::GetConfigService() const {
   return g_browser_process->chromeos_proxy_config_service_impl();
 }
 
-void ProxyCrosSettingsProvider::AppendPortIfValid(
-    const char* port_cache_key,
-    std::string* server_uri) {
-  std::string port;
-  if (!server_uri->empty() && cache_.GetString(port_cache_key, &port) &&
-      !port.empty()) {
-    *server_uri += ":" + port;
-  }
+net::ProxyServer ProxyCrosSettingsProvider::CreateProxyServerFromHost(
+    const std::string& host,
+    const ProxyConfigServiceImpl::ProxyConfig::ManualProxy& proxy,
+    net::ProxyServer::Scheme scheme) const {
+  uint16 port = proxy.server.host_port_pair().port();
+  if (!port)
+    port = net::ProxyServer::GetDefaultPortForScheme(scheme);
+  net::HostPortPair host_port_pair(host, port);
+  return net::ProxyServer(scheme, host_port_pair);
 }
 
-void ProxyCrosSettingsProvider::FormServerUriIfValid(
-    const char* host_cache_key,
-    const std::string& port_num, std::string* server_uri) {
-  if (cache_.GetString(host_cache_key, server_uri) && !server_uri->empty() &&
-      !port_num.empty())
-    *server_uri += ":" + port_num;
+net::ProxyServer ProxyCrosSettingsProvider::CreateProxyServerFromPort(
+    uint16 port,
+    const ProxyConfigServiceImpl::ProxyConfig::ManualProxy& proxy,
+    net::ProxyServer::Scheme scheme) const {
+  std::string host = proxy.server.host_port_pair().host();
+  net::HostPortPair host_port_pair(host, port);
+  return net::ProxyServer(scheme, host_port_pair);
 }
 
 Value* ProxyCrosSettingsProvider::CreateServerHostValue(
     const ProxyConfigServiceImpl::ProxyConfig::ManualProxy& proxy) const {
   return proxy.server.is_valid() ?
          Value::CreateStringValue(proxy.server.host_port_pair().host()) :
          NULL;
 }
 
 Value* ProxyCrosSettingsProvider::CreateServerPortValue(
     const ProxyConfigServiceImpl::ProxyConfig::ManualProxy& proxy) const {
   return proxy.server.is_valid() ?
          Value::CreateIntegerValue(proxy.server.host_port_pair().port()) :
          NULL;
 }
 
 void ProxyCrosSettingsProvider::SetCache(const std::string& key,
     const Value* value) {
   cache_.Set(key, value->DeepCopy());
 }
 
 }  // namespace chromeos