Allow ProxyConfigService to report "no configuration set"

net/proxy/proxy_service.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 "net/proxy/proxy_service.h"
 
 #include <algorithm>
 
 #include "base/compiler_specific.h"
 #include "base/logging.h"
-#include "base/values.h"
 #include "base/message_loop.h"
 #include "base/string_util.h"
+#include "base/values.h"
 #include "googleurl/src/gurl.h"
+#include "net/base/net_errors.h"
 #include "net/base/net_log.h"
-#include "net/base/net_errors.h"
 #include "net/base/net_util.h"
 #include "net/proxy/init_proxy_resolver.h"
 #include "net/proxy/multi_threaded_proxy_resolver.h"
 #include "net/proxy/proxy_config_service_fixed.h"
+#include "net/proxy/proxy_resolver.h"
+#include "net/proxy/proxy_resolver_js_bindings.h"
+#include "net/proxy/proxy_resolver_v8.h"
 #include "net/proxy/proxy_script_fetcher.h"
+#include "net/proxy/sync_host_resolver_bridge.h"
+#include "net/url_request/url_request_context.h"
+
 #if defined(OS_WIN)
 #include "net/proxy/proxy_config_service_win.h"
 #include "net/proxy/proxy_resolver_winhttp.h"
 #elif defined(OS_MACOSX)
 #include "net/proxy/proxy_config_service_mac.h"
 #include "net/proxy/proxy_resolver_mac.h"
 #elif defined(OS_LINUX) && !defined(OS_CHROMEOS)
 #include "net/proxy/proxy_config_service_linux.h"
 #endif
-#include "net/proxy/proxy_resolver.h"
-#include "net/proxy/proxy_resolver_js_bindings.h"
-#include "net/proxy/proxy_resolver_v8.h"
-#include "net/proxy/sync_host_resolver_bridge.h"
-#include "net/url_request/url_request_context.h"
 
 using base::TimeDelta;
 using base::TimeTicks;
 
 namespace net {
 
 namespace {
 
 const size_t kMaxNumNetLogEntries = 100;
 const size_t kDefaultNumPacThreads = 4;
@@ skipping 36 matching lines @@
 // things. It is entirely possible that the value is insuficient for other
 // setups.
 const int64 kNumMillisToStallAfterNetworkChanges = 2000;
 
 // Config getter that always returns direct settings.
 class ProxyConfigServiceDirect : public ProxyConfigService {
  public:
   // ProxyConfigService implementation:
   virtual void AddObserver(Observer* observer) {}
   virtual void RemoveObserver(Observer* observer) {}
-  virtual bool GetLatestProxyConfig(ProxyConfig* config) {
+  virtual ConfigAvailability GetLatestProxyConfig(ProxyConfig* config) {
     *config = ProxyConfig::CreateDirect();
-    return true;
+    return CONFIG_VALID;
   }
 };
 
 // Proxy resolver that fails every time.
 class ProxyResolverNull : public ProxyResolver {
  public:
   ProxyResolverNull() : ProxyResolver(false /*expects_pac_bytes*/) {}
 
   // ProxyResolver implementation:
   virtual int GetProxyForURL(const GURL& url,
@@ skipping 515 matching lines @@
     return;
   }
 
   // Otherwise we need to first fetch the configuration.
   current_state_ = STATE_WAITING_FOR_PROXY_CONFIG;
 
   // Retrieve the current proxy configuration from the ProxyConfigService.
   // If a configuration is not available yet, we will get called back later
   // by our ProxyConfigService::Observer once it changes.
   ProxyConfig config;
-  bool has_config = config_service_->GetLatestProxyConfig(&config);
-  if (has_config)
-    OnProxyConfigChanged(config);
+  ProxyConfigService::ConfigAvailability availability =
+      config_service_->GetLatestProxyConfig(&config);
+  if (availability != ProxyConfigService::CONFIG_PENDING)
+    OnProxyConfigChanged(config, availability);
 }
 
 void ProxyService::OnInitProxyResolverComplete(int result) {
   DCHECK_EQ(STATE_WAITING_FOR_INIT_PROXY_RESOLVER, current_state_);
   DCHECK(init_proxy_resolver_.get());
   DCHECK(fetched_config_.HasAutomaticSettings());
   init_proxy_resolver_.reset();
 
   if (result != OK) {
     VLOG(1) << "Failed configuring with PAC script, falling-back to manual "
@@ skipping 143 matching lines @@
 
 // static
 ProxyConfigService* ProxyService::CreateSystemProxyConfigService(
     MessageLoop* io_loop, MessageLoop* file_loop) {
 #if defined(OS_WIN)
   return new ProxyConfigServiceWin();
 #elif defined(OS_MACOSX)
   return new ProxyConfigServiceMac(io_loop);
 #elif defined(OS_CHROMEOS)
   NOTREACHED() << "ProxyConfigService for ChromeOS should be created in "
-               << "chrome_url_request_context.cc::CreateProxyConfigService.";
+               << "profile_io_data.cc::CreateProxyConfigService.";
   return NULL;
 #elif defined(OS_LINUX)
-  ProxyConfigServiceLinux* linux_config_service
-      = new ProxyConfigServiceLinux();
+  ProxyConfigServiceLinux* linux_config_service =
+      new ProxyConfigServiceLinux();
 
   // Assume we got called from the UI loop, which runs the default
   // glib main loop, so the current thread is where we should be
   // running gconf calls from.
   MessageLoop* glib_default_loop = MessageLoopForUI::current();
 
   // The file loop should be a MessageLoopForIO on Linux.
   DCHECK_EQ(MessageLoop::TYPE_IO, file_loop->type());
 
   // Synchronously fetch the current proxy config (since we are
   // running on glib_default_loop). Additionally register for
   // notifications (delivered in either |glib_default_loop| or
   // |file_loop|) to keep us updated when the proxy config changes.
   linux_config_service->SetupAndFetchInitialConfig(glib_default_loop, io_loop,
       static_cast<MessageLoopForIO*>(file_loop));
 
   return linux_config_service;
 #else
   LOG(WARNING) << "Failed to choose a system proxy settings fetcher "
                   "for this platform.";
   return new ProxyConfigServiceNull();
 #endif
 }
 
-void ProxyService::OnProxyConfigChanged(const ProxyConfig& config) {
+void ProxyService::OnProxyConfigChanged(
+    const ProxyConfig& config,
+    ProxyConfigService::ConfigAvailability availability) {
+  // Retrieve the current proxy configuration from the ProxyConfigService.
+  // If a configuration is not available yet, we will get called back later
+  // by our ProxyConfigService::Observer once it changes.
+  ProxyConfig effective_config;
+  switch (availability) {
+    case ProxyConfigService::CONFIG_PENDING:

[eroman, 2011/03/16 01:30:00]

I don't think this case makes sense. We should instead DCHECK() that
|availability| != CONFIG_PENDING.

[Mattias Nissler (ping if slow), 2011/03/16 17:40:00]

You're right in that it doesn't make sense to send the notification in the first
place. However, there's no harm in handling that condition gracefully. Put a
NOTREACHED().

+      // Will get called back later through ProxyConfigService::Observer.
+      return;
+    case ProxyConfigService::CONFIG_VALID:
+      effective_config = config;
+      break;
+    case ProxyConfigService::CONFIG_UNSET:
+      effective_config = ProxyConfig::CreateAutoDetect();

[eroman, 2011/03/16 01:30:00]

I think we should default to Direct() rather than AutoDetect(). That said, I
wander if it should be valid to reach CONFIG_UNSET here.

CONFIG_UNSET seems like a helper value for creating a chain of services. I don't
think it should not be valid for the leaf nodes to emit this.

(That way the proxyconfigservice is fully in control of the configuration
fallback chain, rather than having the ProxyService decide to use AutoDetect or
something here).

(Also note that the "defaults" are somewhat platform-specific, which is why it
is better to let the system ProxyConfigService make this call. For instance on
Windows it is very common to default to auto-detect so that default would make
sense. However on Linux that would be weird, and you would want to default to
direct).

[Mattias Nissler (ping if slow), 2011/03/16 17:40:00]

OK, I agree that for now we should make the system services not return
CONFIG_UNSET. I still think that it makes sense to put a reasonable default
here, since there is no harm in having it and ProxyConfigService implementors
can still decide what's best for them.

+      break;
+  }
+
   // Emit the proxy settings change to the NetLog stream.
   if (net_log_) {
     scoped_refptr<NetLog::EventParameters> params(
-        new ProxyConfigChangedNetLogParam(fetched_config_, config));
+        new ProxyConfigChangedNetLogParam(fetched_config_, effective_config));
     net_log_->AddEntry(net::NetLog::TYPE_PROXY_CONFIG_CHANGED,
                        base::TimeTicks::Now(),
                        NetLog::Source(),
                        NetLog::PHASE_NONE,
                        params);
   }
 
   // Set the new configuration as the most recently fetched one.
-  fetched_config_ = config;
+  fetched_config_ = effective_config;
   fetched_config_.set_id(1);  // Needed for a later DCHECK of is_valid().
 
   InitializeUsingLastFetchedConfig();
 }
 
 void ProxyService::InitializeUsingLastFetchedConfig() {
   ResetProxyConfig(false);
 
   DCHECK(fetched_config_.is_valid());
 
@@ skipping 94 matching lines @@
     OnCompletion(result_);
   }
 }
 
 void SyncProxyServiceHelper::OnCompletion(int rv) {
   result_ = rv;
   event_.Signal();
 }
 
 }  // namespace net