Allow ProxyConfigService to report "no configuration set"

net/proxy/proxy_config_service_linux.cc

Index: net/proxy/proxy_config_service_linux.cc
diff --git a/net/proxy/proxy_config_service_linux.cc b/net/proxy/proxy_config_service_linux.cc
index 893a8e5d86e1788f0a3f32930d8f0a1be4d68e26..3d8893b3ebf2fa65c1baa8c4dff44bd402268666 100644
--- a/net/proxy/proxy_config_service_linux.cc
+++ b/net/proxy/proxy_config_service_linux.cc
@@ -1,4 +1,4 @@
-// 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.
 
@@ -22,13 +22,13 @@
 #include "base/file_util.h"
 #include "base/logging.h"
 #include "base/message_loop.h"
+#include "base/nix/xdg_util.h"
 #include "base/string_number_conversions.h"
 #include "base/string_tokenizer.h"
 #include "base/string_util.h"
 #include "base/task.h"
 #include "base/threading/thread_restrictions.h"
 #include "base/timer.h"
-#include "base/nix/xdg_util.h"
 #include "googleurl/src/url_canon.h"
 #include "net/base/net_errors.h"
 #include "net/http/http_util.h"
@@ -959,10 +959,10 @@ bool ProxyConfigServiceLinux::Delegate::GetConfigFromGConf(
                                  &pac_url_str)) {
       if (!pac_url_str.empty()) {
         GURL pac_url(pac_url_str);
-        if (!pac_url.is_valid())
-          return false;

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

This seems like it will change the behavior (before it would return false, now
you will return true on invalid URLs). Are you sure about this?

If you would like to change how this works please propose that in a separate
changelist.

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

I agree it's not a good idea to change behavior of the system services in this
CL.
Done.

[eroman, 2011/03/26 00:50:30]

I still see this change in the latest patchset...

-        config->set_pac_url(pac_url);
-        return true;
+        if (pac_url.is_valid()) {
+          config->set_pac_url(pac_url);
+          return true;
+        }
       }
     }
     config->set_auto_detect(true);
@@ -1110,7 +1110,7 @@ void ProxyConfigServiceLinux::Delegate::SetupAndFetchInitialConfig(
     VLOG(1) << "Monitoring of proxy setting changes is disabled";
 
   // Fetch and cache the current proxy config. The config is left in
-  // cached_config_, where GetLatestProxyConfig() running on the IO thread
+  // cached_config_, where GetLastestProxyConfig() running on the IO thread

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

typo: please restore earlier version.

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

Done.

[eroman, 2011/03/26 00:50:30]

I don't see this change int he latest patchset...

   // will expect to find it. This is safe to do because we return
   // before this ProxyConfigServiceLinux is passed on to
   // the ProxyService.
@@ -1166,22 +1166,26 @@ void ProxyConfigServiceLinux::Delegate::RemoveObserver(Observer* observer) {
   observers_.RemoveObserver(observer);
 }
 
-bool ProxyConfigServiceLinux::Delegate::GetLatestProxyConfig(
-    ProxyConfig* config) {
+ProxyConfigService::ConfigAvailability
+    ProxyConfigServiceLinux::Delegate::GetLatestProxyConfig(
+        ProxyConfig* config) {
   // This is called from the IO thread.
   DCHECK(!io_loop_ || MessageLoop::current() == io_loop_);
 
   // Simply return the last proxy configuration that glib_default_loop
   // notified us of.
-  *config = cached_config_.is_valid() ?
-      cached_config_ : ProxyConfig::CreateDirect();
+  if (cached_config_.is_valid()) {
+    *config = cached_config_;
+    return CONFIG_VALID;
+  }
 
-  // We return true to indicate that *config was filled in. It is always
-  // going to be available since we initialized eagerly on the UI thread.
+  // We return CONFIG_UNSET to indicate that the config has been read
+  // successfully but there was nothing configured. Since we initialize eagerly
+  // on the UI thread, we never need to return CONFIG_PENDING.
   // TODO(eroman): do lazy initialization instead, so we no longer need
   //               to construct ProxyConfigServiceLinux on the UI thread.
   //               In which case, we may return false here.
-  return true;
+  return CONFIG_UNSET;

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

I don't think the system config services should ever return CONFIG_UNSET.

Makes more sense for me that they return DIRECT, since that is the implicit
fallback scheme they assume.

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

I did this change for consolidating the defaulting into ProxyConfigService.
However, I don't feel strong about this, we can always change this later if the
need arises, so I reverted this change.
Done.

 }
 
 // Depending on the GConfSettingGetter in use, this method will be called
@@ -1215,7 +1219,12 @@ void ProxyConfigServiceLinux::Delegate::SetNewProxyConfig(
   DCHECK(MessageLoop::current() == io_loop_);
   VLOG(1) << "Proxy configuration changed";
   cached_config_ = new_config;
-  FOR_EACH_OBSERVER(Observer, observers_, OnProxyConfigChanged(new_config));
+  FOR_EACH_OBSERVER(
+      Observer, observers_,
+      OnProxyConfigChanged(cached_config_,
+                           cached_config_.is_valid() ?
+                               ProxyConfigService::CONFIG_VALID :
+                               ProxyConfigService::CONFIG_UNSET));

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

See earlier comment.

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

Done.

 }
 
 void ProxyConfigServiceLinux::Delegate::PostDestroyTask() {
@@ -1269,7 +1278,8 @@ void ProxyConfigServiceLinux::RemoveObserver(Observer* observer) {
   delegate_->RemoveObserver(observer);
 }
 
-bool ProxyConfigServiceLinux::GetLatestProxyConfig(ProxyConfig* config) {
+ProxyConfigService::ConfigAvailability
+    ProxyConfigServiceLinux::GetLatestProxyConfig(ProxyConfig* config) {
   return delegate_->GetLatestProxyConfig(config);
 }