Allow ProxyConfigService to report "no configuration set"

net/proxy/proxy_config_service_linux.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_config_service_linux.h"
 
 #include <errno.h>
 #include <fcntl.h>
 #if defined(USE_GCONF)
 #include <gconf/gconf-client.h>
 #endif
 #include <limits.h>
 #include <stdio.h>
 #include <stdlib.h>
 #include <sys/inotify.h>
 #include <unistd.h>
 
 #include <map>
 
 #include "base/environment.h"
 #include "base/file_path.h"
 #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"
 #include "net/proxy/proxy_config.h"
 #include "net/proxy/proxy_server.h"
 
 namespace net {
 
 namespace {
 
@@ skipping 910 matching lines @@
     return true;
   }
 
   if (mode == "auto") {
     // automatic proxy config
     std::string pac_url_str;
     if (gconf_getter_->GetString("/system/proxy/autoconfig_url",
                                  &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);
     return true;
   }
 
   if (mode != "manual") {
     // Mode is unrecognized.
     return false;
   }
@@ skipping 127 matching lines @@
   glib_default_loop_ = glib_default_loop;
   io_loop_ = io_loop;
 
   // If we are passed a NULL io_loop or file_loop, then we don't set up
   // proxy setting change notifications. This should not be the usual
   // case but is intended to simplify test setups.
   if (!io_loop_ || !file_loop)
     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.
 
   // Note: It would be nice to prioritize environment variables
   // and only fall back to gconf if env vars were unset. But
   // gnome-terminal "helpfully" sets http_proxy and no_proxy, and it
   // does so even if the proxy mode is set to auto, which would
   // mislead us.
 
@@ skipping 35 matching lines @@
 }
 
 void ProxyConfigServiceLinux::Delegate::AddObserver(Observer* observer) {
   observers_.AddObserver(observer);
 }
 
 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
 // on either the UI thread (GConf) or the file thread (KDE).
 void ProxyConfigServiceLinux::Delegate::OnCheckProxyConfigSettings() {
   MessageLoop* required_loop = gconf_getter_->GetNotificationLoop();
   DCHECK(!required_loop || MessageLoop::current() == required_loop);
   ProxyConfig new_config;
   bool valid = GetConfigFromGConf(&new_config);
   if (valid)
@@ skipping 13 matching lines @@
     // Update the thread-private copy in |reference_config_| as well.
     reference_config_ = new_config;
   }
 }
 
 void ProxyConfigServiceLinux::Delegate::SetNewProxyConfig(
     const ProxyConfig& new_config) {
   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() {
   if (!gconf_getter_.get())
     return;
   MessageLoop* shutdown_loop = gconf_getter_->GetNotificationLoop();
   if (!shutdown_loop || MessageLoop::current() == shutdown_loop) {
     // Already on the right thread, call directly.
     // This is the case for the unittests.
     OnDestroy();
@@ skipping 33 matching lines @@
 }
 
 void ProxyConfigServiceLinux::AddObserver(Observer* observer) {
   delegate_->AddObserver(observer);
 }
 
 void ProxyConfigServiceLinux::RemoveObserver(Observer* observer) {
   delegate_->RemoveObserver(observer);
 }
 
-bool ProxyConfigServiceLinux::GetLatestProxyConfig(ProxyConfig* config) {
+ProxyConfigService::ConfigAvailability
+    ProxyConfigServiceLinux::GetLatestProxyConfig(ProxyConfig* config) {
   return delegate_->GetLatestProxyConfig(config);
 }
 
 }  // namespace net