Track sources of proxy settings.

net/proxy/proxy_service.cc

Index: net/proxy/proxy_service.cc
diff --git a/net/proxy/proxy_service.cc b/net/proxy/proxy_service.cc
index 1c9203348ae908a1bcf36862bf4fd443b85011cb..d12e12efa3734b2d654aaafd4e9c8d8f92e54aa5 100644
--- a/net/proxy/proxy_service.cc
+++ b/net/proxy/proxy_service.cc
@@ -164,6 +164,7 @@ class ProxyConfigServiceDirect : public ProxyConfigService {
   virtual ConfigAvailability GetLatestProxyConfig(ProxyConfig* config)
       OVERRIDE {
     *config = ProxyConfig::CreateDirect();
+    config->set_source(PROXY_CONFIG_SOURCE_NONE);
     return CONFIG_VALID;
   }
 };
@@ -799,6 +800,7 @@ class ProxyService::PacRequest
         url_(url),
         resolve_job_(NULL),
         config_id_(ProxyConfig::kInvalidConfigID),
+        config_source_(PROXY_CONFIG_SOURCE_UNKNOWN),
         net_log_(net_log) {
     DCHECK(!user_callback.is_null());
   }
@@ -811,6 +813,7 @@ class ProxyService::PacRequest
     DCHECK(service_->config_.is_valid());
 
     config_id_ = service_->config_.id();
+    config_source_ = service_->config_.source();
 
     return resolver()->GetProxyForURL(
         url_, results_,
@@ -866,10 +869,12 @@ class ProxyService::PacRequest
     // Make a note in the results which configuration was in use at the
     // time of the resolve.
     results_->config_id_ = config_id_;
+    results_->config_source_ = config_source_;
 
     // Reset the state associated with in-progress-resolve.
     resolve_job_ = NULL;
     config_id_ = ProxyConfig::kInvalidConfigID;
+    config_source_ = PROXY_CONFIG_SOURCE_UNKNOWN;
 
     return service_->DidFinishResolvingProxy(results_, result_code, net_log_);
   }
@@ -911,6 +916,7 @@ class ProxyService::PacRequest
   GURL url_;
   ProxyResolver::RequestHandle resolve_job_;
   ProxyConfig::ID config_id_;  // The config id when the resolve was started.
+  ProxyConfigSource config_source_;  // The source of proxy settings.
   BoundNetLog net_log_;
 };
 
@@ -1112,7 +1118,9 @@ int ProxyService::TryToCompleteSynchronously(const GURL& url,
     return ERR_IO_PENDING;  // Must submit the request to the proxy resolver.
 
   // Use the manual proxy settings.
-  config_.proxy_rules().Apply(url, result);
+  bool did_use_proxy_rules = config_.proxy_rules().Apply(url, result);
+  result->config_source_ =
+      (did_use_proxy_rules ? config_.source() : PROXY_CONFIG_SOURCE_NONE);

[eroman, 2012/06/02 02:06:10]

I still don't agree with this layering.

This makes it seem like somehow Apply failed to apply the rules and we are
falling back. In doing this we lose track of the source where these setting
originated from.

However the fact that we ended up using direct was itself a "rule", not a
failure.

I think there are two concepts trying to be merged into one here.

  (1) There is the concept of the "source", which identifies where the
underlying settings originated from. This basically tracks the internals of the
ProxyConfigService.

  (2) There is the concept of whether we arrived at a DIRECT connection by way
of a bypass rule.


Basically what I am asking for here is to keep these two separate. How about
mirroring config_.source() directly to result->config_source_ so it preserves
the origin of the settings.

The second bit of information could be whether we applied a bypass rule. I think
the bypass case in ProxyConfig is the only one you actually need to care about,
since the higher level check would be listsize == 1 && isdirect &&
proxyconfig.was_from_bypass.

Lastly, if you keep the "was from bypass" bit as an output from Apply(), I think
it should either be a named out-parameter instead of return value to make it
clear that it does not reflect the success of the operation. Or it should be
directly set into |result| as part of Apply(), and not be an extra parameter.

Sorry for taking so long on this review, I have had a hard time understanding
this and expressing what my concerns are :) But I think we are pretty close now.

Feel free to grab me on IM or on the phone if we can't get this finalized
quickly!

   result->config_id_ = config_.id();
   return OK;
 }
@@ -1234,6 +1242,7 @@ void ProxyService::OnInitProxyResolverComplete(int result) {
   // TODO(eroman): Make this ID unique in the case where configuration changed
   //               due to ProxyScriptDeciderPoller.
   config_.set_id(fetched_config_.id());
+  config_.set_source(fetched_config_.source());
 
   // Resume any requests which we had to defer until the PAC script was
   // downloaded.