Change ProxyRules to handle ProxyLists rather than just single ProxyServer instances.

net/proxy/proxy_config.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 "net/proxy/proxy_config.h"
 
 #include "base/logging.h"
 #include "base/string_util.h"
 #include "base/strings/string_tokenizer.h"
 #include "base/values.h"
 #include "net/proxy/proxy_info.h"
 
 namespace net {
 
 namespace {
 
 // If |proxy| is valid, sets it in |dict| under the key |name|.

[eroman, 2013/02/26 01:13:50]

"is valid" needs to be updated.

[marq_use_my_chromium_address, 2013/02/27 23:08:19]

Done.

 void AddProxyToValue(const char* name,

[eroman, 2013/02/26 01:13:50]

This should be renamed to AddProxyListToValue()

[marq_use_my_chromium_address, 2013/02/27 23:08:19]

Done.

-                     const ProxyServer& proxy,
+                     const ProxyList& proxy,
                      base::DictionaryValue* dict) {
-  if (proxy.is_valid())
-    dict->SetString(name, proxy.ToURI());
+  if (!proxy.IsEmpty())
+    dict->SetString(name, proxy.ToPacString());
 }
 
 }  // namespace
 
 ProxyConfig::ProxyRules::ProxyRules()
     : reverse_bypass(false),
-      type(TYPE_NO_RULES) {
+      type(TYPE_NO_RULES){

[eroman, 2013/02/26 01:13:50]

Why remove the space? That is counter to the style AFAIK.

[marq_use_my_chromium_address, 2013/02/27 23:08:19]

Done.

 }
 
 ProxyConfig::ProxyRules::~ProxyRules() {
 }
 
 void ProxyConfig::ProxyRules::Apply(const GURL& url, ProxyInfo* result) const {
   if (empty()) {
     result->UseDirect();
     return;
   }
 
   bool bypass_proxy = bypass_rules.Matches(url);
   if (reverse_bypass)
     bypass_proxy = !bypass_proxy;
   if (bypass_proxy) {
     result->UseDirectWithBypassedProxy();
     return;
   }
 
   switch (type) {
     case ProxyRules::TYPE_SINGLE_PROXY: {
-      result->UseProxyServer(single_proxy);
+      result->UsePacString(single_proxy.ToPacString());

[eroman, 2013/02/26 01:13:50]

Rather than round-tripping through the PAC string format, this should just set
the values directly.

Perhaps add a method like:
  result->UseProxyList(single_proxy);

[marq_use_my_chromium_address, 2013/02/27 23:08:19]

Done.

       return;
     }
     case ProxyRules::TYPE_PROXY_PER_SCHEME: {
-      const ProxyServer* entry = MapUrlSchemeToProxy(url.scheme());
+      const ProxyList* entry = MapUrlSchemeToProxy(url.scheme());
       if (entry) {
-        result->UseProxyServer(*entry);
+        result->UsePacString(entry->ToPacString());

[eroman, 2013/02/26 01:13:50]

See comment above.

[marq_use_my_chromium_address, 2013/02/27 23:08:19]

Done.

       } else {
         // We failed to find a matching proxy server for the current URL
         // scheme. Default to direct.
         result->UseDirect();
       }
       return;
     }
     default: {
       result->UseDirect();
       NOTREACHED();
       return;
     }
   }
 }
 
 void ProxyConfig::ProxyRules::ParseFromString(const std::string& proxy_rules) {
   // Reset.
   type = TYPE_NO_RULES;
-  single_proxy = ProxyServer();
-  proxy_for_http = ProxyServer();
-  proxy_for_https = ProxyServer();
-  proxy_for_ftp = ProxyServer();
-  fallback_proxy = ProxyServer();
+  single_proxy = ProxyList();
+  proxy_for_http = ProxyList();
+  proxy_for_https = ProxyList();
+  proxy_for_ftp = ProxyList();
+  fallback_proxy = ProxyList();
 
   base::StringTokenizer proxy_server_list(proxy_rules, ";");
   while (proxy_server_list.GetNext()) {
     base::StringTokenizer proxy_server_for_scheme(
         proxy_server_list.token_begin(), proxy_server_list.token_end(), "=");
 
     while (proxy_server_for_scheme.GetNext()) {
       std::string url_scheme = proxy_server_for_scheme.token();
 
       // If we fail to get the proxy server here, it means that
       // this is a regular proxy server configuration, i.e. proxies
       // are not configured per protocol.
       if (!proxy_server_for_scheme.GetNext()) {
         if (type == TYPE_PROXY_PER_SCHEME)
           continue;  // Unexpected.
-        single_proxy = ProxyServer::FromURI(url_scheme,
-                                            ProxyServer::SCHEME_HTTP);
+        single_proxy.SetSingleProxyServer(
+            ProxyServer::FromURI(url_scheme, ProxyServer::SCHEME_HTTP));
         type = TYPE_SINGLE_PROXY;
         return;
       }
 
       // Trim whitespace off the url scheme.
       TrimWhitespaceASCII(url_scheme, TRIM_ALL, &url_scheme);
 
       // Add it to the per-scheme mappings (if supported scheme).
       type = TYPE_PROXY_PER_SCHEME;
-      ProxyServer* entry = MapUrlSchemeToProxyNoFallback(url_scheme);
+      ProxyList* entry = MapUrlSchemeToProxyNoFallback(url_scheme);
       ProxyServer::Scheme default_scheme = ProxyServer::SCHEME_HTTP;
 
       // socks=XXX is inconsistent with the other formats, since "socks"
       // is not a URL scheme. Rather this means "for everything else, send
       // it to the SOCKS proxy server XXX".
       if (url_scheme == "socks") {
         DCHECK(!entry);
         entry = &fallback_proxy;
+        // Note that here 'socks' is understood to be SOCKS4, even though
+        // 'socks' maps to SOCKS5 in ProxyServer::GetSchemeFromURIInternal.

[eroman, 2013/02/26 01:13:50]

Yeah, icky carry-over from the windows interpretation of this string (which is
where the format comes from). It is convenient having a cross-platform
implementation for the purpose of command line flags, but maybe it doesn't make
sense and we should invent our own for that.

[marq_use_my_chromium_address, 2013/02/27 23:08:19]

Not my call; I just added the comment because it's non-obvious.

         default_scheme = ProxyServer::SCHEME_SOCKS4;
       }
 
       if (entry) {
-        *entry = ProxyServer::FromURI(proxy_server_for_scheme.token(),
-                                      default_scheme);
+        ProxyServer new_proxy = ProxyServer::FromURI(
+            proxy_server_for_scheme.token(), default_scheme);
+        // Don't have DIRECT as the first proxy in a list.

[eroman, 2013/02/26 01:13:50]

Why disallow this?

[marq_use_my_chromium_address, 2013/02/27 23:08:19]

I had assumed that this would create a wasteful situation where a ProxyInfo
would not return true from IsDirect, but would nonetheless function that way. On
closer reading of the code, that actually isn't the case, so I've removed this
condition.

+        if (!new_proxy.is_direct() || !entry->IsEmpty()) {
+          entry->AddProxyServer(new_proxy);
+        }
       }
     }
   }
 }
 
-const ProxyServer* ProxyConfig::ProxyRules::MapUrlSchemeToProxy(
+const ProxyList* ProxyConfig::ProxyRules::MapUrlSchemeToProxy(
     const std::string& url_scheme) const {
-  const ProxyServer* proxy_server =
+  const ProxyList* proxy_server_list =
       const_cast<ProxyRules*>(this)->MapUrlSchemeToProxyNoFallback(url_scheme);
-  if (proxy_server && proxy_server->is_valid())
-    return proxy_server;
-  if (fallback_proxy.is_valid())
+  if (proxy_server_list && !proxy_server_list->IsEmpty())
+    return proxy_server_list;
+  if (!fallback_proxy.IsEmpty())
     return &fallback_proxy;
   return NULL;  // No mapping for this scheme. Use direct.
 }
 
 bool ProxyConfig::ProxyRules::Equals(const ProxyRules& other) const {
   return type == other.type &&
-         single_proxy == other.single_proxy &&
-         proxy_for_http == other.proxy_for_http &&
-         proxy_for_https == other.proxy_for_https &&
-         proxy_for_ftp == other.proxy_for_ftp &&
-         fallback_proxy == other.fallback_proxy &&
+         single_proxy.Equals(other.single_proxy) &&
+         proxy_for_http.Equals(other.proxy_for_http) &&
+         proxy_for_https.Equals(other.proxy_for_https) &&
+         proxy_for_ftp.Equals(other.proxy_for_ftp) &&
          bypass_rules.Equals(other.bypass_rules) &&
          reverse_bypass == other.reverse_bypass;
 }
 
-ProxyServer* ProxyConfig::ProxyRules::MapUrlSchemeToProxyNoFallback(
+ProxyList* ProxyConfig::ProxyRules::MapUrlSchemeToProxyNoFallback(
     const std::string& scheme) {
   DCHECK_EQ(TYPE_PROXY_PER_SCHEME, type);
   if (scheme == "http")
     return &proxy_for_http;
   if (scheme == "https")
     return &proxy_for_https;
   if (scheme == "ftp")
     return &proxy_for_ftp;
   return NULL;  // No mapping for this scheme.
 }
@@ skipping 92 matching lines @@
     }
   }
 
   // Output the source.
   dict->SetString("source", ProxyConfigSourceToString(source_));
 
   return dict;
 }
 
 }  // namespace net