redesign and reimplement proxy config service and tracker, revise proxy ui on cros

chrome/browser/chromeos/proxy_config_service_impl.cc

 // 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 "chrome/browser/chromeos/proxy_config_service_impl.h"
 
 #include <ostream>
 
 #include "base/logging.h"
 #include "base/string_util.h"
 #include "base/task.h"
 #include "chrome/browser/chromeos/cros/cros_library.h"
 #include "chrome/browser/chromeos/cros_settings_names.h"
+#include "chrome/browser/net/pref_proxy_config_service.h"
 #include "chrome/browser/policy/proto/chrome_device_policy.pb.h"
+#include "chrome/browser/prefs/pref_service.h"
 #include "chrome/browser/prefs/proxy_config_dictionary.h"
 #include "chrome/browser/prefs/proxy_prefs.h"
+#include "chrome/browser/profiles/profile.h"
+#include "chrome/common/pref_names.h"
 #include "content/browser/browser_thread.h"
 #include "content/common/json_value_serializer.h"
 #include "grit/generated_resources.h"
 #include "ui/base/l10n/l10n_util.h"
 
 namespace em = enterprise_management;
 
 namespace chromeos {
 
 namespace {
 
 const char* SourceToString(ProxyConfigServiceImpl::ProxyConfig::Source source) {
   switch (source) {
     case ProxyConfigServiceImpl::ProxyConfig::SOURCE_NONE:
       return "SOURCE_NONE";
+    case ProxyConfigServiceImpl::ProxyConfig::SOURCE_NONE_DISABLED:
+      return "SOURCE_NONE_DISABLED";
     case ProxyConfigServiceImpl::ProxyConfig::SOURCE_POLICY:
       return "SOURCE_POLICY";
-    case ProxyConfigServiceImpl::ProxyConfig::SOURCE_OWNER:
-      return "SOURCE_OWNER";
+    case ProxyConfigServiceImpl::ProxyConfig::SOURCE_EXTENSION:
+      return "SOURCE_EXTENSION";
+    case ProxyConfigServiceImpl::ProxyConfig::SOURCE_RECOMMENDED:
+      return "SOURCE_RECOMMENDED";
+    case ProxyConfigServiceImpl::ProxyConfig::SOURCE_RECOMMENDED_DISABLED:
+      return "SOURCE_RECOMMENDED_DISABLED";
+    case ProxyConfigServiceImpl::ProxyConfig::SOURCE_NETWORK:
+      return "SOURCE_NETWORK";
+    case ProxyConfigServiceImpl::ProxyConfig::SOURCE_NETWORK_DISABLED:
+      return "SOURCE_NETWORK_DISABLED";
   }
   NOTREACHED() << "Unrecognized source type";
   return "";
 }
 
 std::ostream& operator<<(std::ostream& out,
     const ProxyConfigServiceImpl::ProxyConfig::ManualProxy& proxy) {

[Mattias Nissler (ping if slow), 2011/10/05 10:18:36]

indentation: parameters should line up (break after opening parentheses).

[kuan, 2011/10/07 00:30:41]

i can't line them up, 'cos they won't fit, i believe the style guideline is to
indent 4 spaces when it can't fit?

On 2011/10/05 10:18:36, Mattias Nissler wrote:

-  out << "  " << SourceToString(proxy.source) << "\n"
-      << "  server: " << (proxy.server.is_valid() ? proxy.server.ToURI() : "")
-      << "\n";
+  out << (proxy.server.is_valid() ? proxy.server.ToURI() : "") << "\n";
   return out;
 }
 
 std::ostream& operator<<(std::ostream& out,
     const ProxyConfigServiceImpl::ProxyConfig& config) {

[Mattias Nissler (ping if slow), 2011/10/05 10:18:36]

same here.

[kuan, 2011/10/07 00:30:41]

Done.

   switch (config.mode) {
     case ProxyConfigServiceImpl::ProxyConfig::MODE_DIRECT:
       out << "Direct connection:\n  "
-          << SourceToString(config.automatic_proxy.source) << "\n";
+          << SourceToString(config.source) << "\n";
       break;
     case ProxyConfigServiceImpl::ProxyConfig::MODE_AUTO_DETECT:
       out << "Auto detection:\n  "
-          << SourceToString(config.automatic_proxy.source) << "\n";
+          << SourceToString(config.source) << "\n";
       break;
     case ProxyConfigServiceImpl::ProxyConfig::MODE_PAC_SCRIPT:
       out << "Custom PAC script:\n  "
-          << SourceToString(config.automatic_proxy.source)
+          << SourceToString(config.source)
           << "\n  PAC: " << config.automatic_proxy.pac_url << "\n";
       break;
     case ProxyConfigServiceImpl::ProxyConfig::MODE_SINGLE_PROXY:
-      out << "Single proxy:\n" << config.single_proxy;
+      out << "Single proxy:\n"
+          << SourceToString(config.source) << "\n  " << config.single_proxy;
       break;
     case ProxyConfigServiceImpl::ProxyConfig::MODE_PROXY_PER_SCHEME:
-      out << "HTTP proxy: " << config.http_proxy;
-      out << "HTTPS proxy: " << config.https_proxy;
-      out << "FTP proxy: " << config.ftp_proxy;
-      out << "SOCKS proxy: " << config.socks_proxy;
+      out << "Manual proxies:\n"
+          << SourceToString(config.source) << "\n"
+          << "  HTTP:  " << config.http_proxy
+          << "  HTTPS: " << config.https_proxy
+          << "  FTP: " << config.ftp_proxy
+          << "  SOCKS: " << config.socks_proxy;
       break;
     default:
       NOTREACHED() << "Unrecognized proxy config mode";
       break;
   }
   if (config.mode == ProxyConfigServiceImpl::ProxyConfig::MODE_SINGLE_PROXY ||
       config.mode ==
           ProxyConfigServiceImpl::ProxyConfig::MODE_PROXY_PER_SCHEME) {
     out << "Bypass list: ";
     if (config.bypass_rules.rules().empty()) {
       out << "[None]";
     } else {
       const net::ProxyBypassRules& bypass_rules = config.bypass_rules;
       net::ProxyBypassRules::RuleList::const_iterator it;
       for (it = bypass_rules.rules().begin();
            it != bypass_rules.rules().end(); ++it) {
         out << "\n    " << (*it)->ToString();
       }
     }
   }
   return out;
 }
 
 std::string ProxyConfigToString(

[Mattias Nissler (ping if slow), 2011/10/05 10:18:36]

This and the functions above appear to be dead code. Remove? I guess you used it
for debug logging, but IMHO it's quite a lot to be maintained in tree. At the
very least, make #ifdef it out and put a comment why it's here.

[kuan, 2011/10/07 00:30:41]

i need it for debugging, so i make #if defined.

On 2011/10/05 10:18:36, Mattias Nissler wrote:

     const ProxyConfigServiceImpl::ProxyConfig& proxy_config) {
   std::ostringstream stream;
   stream << proxy_config;
   return stream.str();
 }
 
 }  // namespace
 
-//---------- ProxyConfigServiceImpl::ProxyConfig::Setting methods --------------
-
-bool ProxyConfigServiceImpl::ProxyConfig::Setting::CanBeWrittenByUser(
-    bool user_is_owner) {
-  // Setting can only be written by user if user is owner and setting is not
-  // from policy.
-  return user_is_owner && source != ProxyConfig::SOURCE_POLICY;
-}
-
 //----------- ProxyConfigServiceImpl::ProxyConfig: public methods --------------
 
-ProxyConfigServiceImpl::ProxyConfig::ProxyConfig() : mode(MODE_DIRECT) {}
+ProxyConfigServiceImpl::ProxyConfig::ProxyConfig()
+    : mode(MODE_DIRECT), source(SOURCE_NONE) {}
 
 ProxyConfigServiceImpl::ProxyConfig::~ProxyConfig() {}
 
 void ProxyConfigServiceImpl::ProxyConfig::ToNetProxyConfig(
     net::ProxyConfig* net_config) {
   switch (mode) {
     case MODE_DIRECT:
       *net_config = net::ProxyConfig::CreateDirect();
       break;
     case MODE_AUTO_DETECT:
@@ skipping 19 matching lines @@
       net_config->proxy_rules().proxy_for_ftp = ftp_proxy.server;
       net_config->proxy_rules().fallback_proxy = socks_proxy.server;
       net_config->proxy_rules().bypass_rules = bypass_rules;
       break;
     default:
       NOTREACHED() << "Unrecognized proxy config mode";
       break;
   }
 }
 
-bool ProxyConfigServiceImpl::ProxyConfig::CanBeWrittenByUser(
-    bool user_is_owner, const std::string& scheme) {
-  // Setting can only be written by user if user is owner and setting is not
-  // from policy.
-  Setting* setting = NULL;
-  switch (mode) {
-    case MODE_DIRECT:
-    case MODE_AUTO_DETECT:
-    case MODE_PAC_SCRIPT:
-      setting = &automatic_proxy;
-      break;
-    case MODE_SINGLE_PROXY:
-      setting = &single_proxy;
-      break;
-    case MODE_PROXY_PER_SCHEME:
-      setting = MapSchemeToProxy(scheme);
-      break;
+bool ProxyConfigServiceImpl::ProxyConfig::FromNetProxyConfig(
+    const net::ProxyConfig& net_config) {
+  const net::ProxyConfig::ProxyRules& rules = net_config.proxy_rules();
+  switch (rules.type) {
+    case net::ProxyConfig::ProxyRules::TYPE_NO_RULES:
+      if (!net_config.HasAutomaticSettings()) {
+        mode = ProxyConfig::MODE_DIRECT;
+      } else if (net_config.auto_detect()) {
+        mode = ProxyConfig::MODE_AUTO_DETECT;
+      } else if (net_config.has_pac_url()) {
+        mode = ProxyConfig::MODE_PAC_SCRIPT;
+        automatic_proxy.pac_url = net_config.pac_url();
+      } else {
+        return false;
+      }
+      return true;
+    case net::ProxyConfig::ProxyRules::TYPE_SINGLE_PROXY:
+      if (!rules.single_proxy.is_valid())
+        return false;
+      mode = MODE_SINGLE_PROXY;
+      single_proxy.server = rules.single_proxy;
+      bypass_rules = rules.bypass_rules;
+      return true;
+    case net::ProxyConfig::ProxyRules::TYPE_PROXY_PER_SCHEME:
+      // Make sure we have valid server for at least one of the protocols.
+      if (!rules.proxy_for_http.is_valid() &&
+          !rules.proxy_for_https.is_valid() &&
+          !rules.proxy_for_ftp.is_valid() &&
+          !rules.fallback_proxy.is_valid()) {
+        return false;
+      }
+      mode = MODE_PROXY_PER_SCHEME;
+      if (rules.proxy_for_http.is_valid())
+        http_proxy.server = rules.proxy_for_http;
+      if (rules.proxy_for_https.is_valid())
+        https_proxy.server = rules.proxy_for_https;
+      if (rules.proxy_for_ftp.is_valid())
+        ftp_proxy.server = rules.proxy_for_ftp;
+      if (rules.fallback_proxy.is_valid())
+        socks_proxy.server = rules.fallback_proxy;
+      bypass_rules = rules.bypass_rules;
+      return true;
     default:
+      NOTREACHED() << "Unrecognized proxy config mode";
       break;
   }
-  if (!setting) {
-    NOTREACHED() << "Unrecognized proxy config mode";
-    return false;
-  }
-  return setting->CanBeWrittenByUser(user_is_owner);
+  return false;
 }
 
 ProxyConfigServiceImpl::ProxyConfig::ManualProxy*
     ProxyConfigServiceImpl::ProxyConfig::MapSchemeToProxy(
         const std::string& scheme) {
   if (scheme == "http")
     return &http_proxy;
   if (scheme == "https")
     return &https_proxy;
   if (scheme == "ftp")
@@ skipping 75 matching lines @@
   JSONStringValueSerializer serializer(input);
   scoped_ptr<Value> value(serializer.Deserialize(NULL, NULL));
   if (!value.get() || value->GetType() != Value::TYPE_DICTIONARY)
     return false;
   DictionaryValue* proxy_dict = static_cast<DictionaryValue*>(value.get());
   return FromPrefProxyConfig(proxy_dict);
 }
 
 bool ProxyConfigServiceImpl::ProxyConfig::Equals(
     const ProxyConfig& other) const {
+  // Intentionally ignore source which is only used for ui purposes and has no
+  // impact on proxy backend.
   if (mode != other.mode)
     return false;
   switch (mode) {
     case MODE_DIRECT:
     case MODE_AUTO_DETECT:
       return true;
     case MODE_PAC_SCRIPT:
       return automatic_proxy.pac_url == other.automatic_proxy.pac_url;
     case MODE_SINGLE_PROXY:
       return single_proxy.server == other.single_proxy.server &&
@@ skipping 194 matching lines @@
   }
 }
 
 void ProxyConfigServiceImpl::UIGetProxyConfig(ProxyConfig* config) {
   // Should be called from UI thread.
   CheckCurrentlyOnUIThread();
   // Simply returns the copy on the UI thread.
   *config = current_ui_config_;
 }
 
-bool ProxyConfigServiceImpl::UISetCurrentNetwork(
-    const std::string& current_network) {
+void ProxyConfigServiceImpl::UISetCurrentNetworkWithProfile(
+    const std::string& current_network, Profile* profile) {

[Mattias Nissler (ping if slow), 2011/10/05 10:18:36]

nit: second parameter on next line.

[kuan, 2011/10/07 00:30:41]

Done.

   // Should be called from UI thread.
   CheckCurrentlyOnUIThread();
-  if (current_ui_network_ == current_network)
-    return false;
   Network* network = CrosLibrary::Get()->GetNetworkLibrary()->FindNetworkByPath(
       current_network);
   if (!network) {
+    ResetUICache();
     LOG(ERROR) << "can't find requested network " << current_network;
-    return false;
+    return;
   }
   current_ui_network_ = current_network;
-  current_ui_config_ = ProxyConfig();
+  DetermineUIConfig(profile, network);
   SetCurrentNetworkName(network);
-  if (!network->proxy_config().empty())
-    current_ui_config_.DeserializeForNetwork(network->proxy_config());
   VLOG(1) << "current ui network: "
           << (current_ui_network_name_.empty() ?
               current_ui_network_ : current_ui_network_name_)
-          << ", proxy mode: " << current_ui_config_.mode;
-  return true;
+          << ", proxy mode: " << current_ui_config_.mode
+          << ", " << SourceToString(current_ui_config_.source)
+          << ", modifiable:" << current_ui_config_.IsUserModifiable();
 }
 
-bool ProxyConfigServiceImpl::UIMakeActiveNetworkCurrent() {
+void ProxyConfigServiceImpl::UIMakeActiveNetworkCurrentWithProfile(
+    Profile* profile) {
   // Should be called from UI thread.
   CheckCurrentlyOnUIThread();
-  if (current_ui_network_ == active_network_)
-    return false;
   Network* network = NULL;
   if (!testing_) {
     network = CrosLibrary::Get()->GetNetworkLibrary()->FindNetworkByPath(
       active_network_);
     if (!network) {
+      ResetUICache();
       LOG(ERROR) << "can't find requested network " << active_network_;
-      return false;
+      return;
     }
   }
   current_ui_network_ = active_network_;
-  current_ui_config_ = active_config_;
+  DetermineUIConfig(profile, network);
   SetCurrentNetworkName(network);
   VLOG(1) << "current ui network: "
           << (current_ui_network_name_.empty() ?
               current_ui_network_ : current_ui_network_name_)
-          << ", proxy mode: " << current_ui_config_.mode;
-  return true;
+          << ", proxy mode: " << current_ui_config_.mode
+          << ", " << SourceToString(current_ui_config_.source)
+          << ", modifiable:" << current_ui_config_.IsUserModifiable();
 }
 
 void ProxyConfigServiceImpl::UISetUseSharedProxies(bool use_shared) {
   // Should be called from UI thread.
   CheckCurrentlyOnUIThread();
 
-  // Reset all UI-related variables so that the next opening of proxy
-  // configuration dialog of any network will trigger javascript reloading of
-  // (possibly) new proxy settings.
-  current_ui_network_.clear();
-  current_ui_network_name_.clear();
-  current_ui_config_ = ProxyConfig();
-
   if (use_shared_proxies_ == use_shared) {
     VLOG(1) << "same use_shared_proxies = " << use_shared_proxies_;
     return;
   }
   use_shared_proxies_ = use_shared;
   VLOG(1) << "new use_shared_proxies = " << use_shared_proxies_;
   if (active_network_.empty())
     return;
   Network* network = CrosLibrary::Get()->GetNetworkLibrary()->FindNetworkByPath(
       active_network_);
@@ skipping 146 matching lines @@
     IOSetProxyConfig(active_config_, net::ProxyConfigService::CONFIG_VALID);
     return;
   }
   if (current_ui_network_.empty())
     return;
   // Update config to flimflam.
   std::string value;
   if (current_ui_config_.SerializeForNetwork(&value)) {
     VLOG(1) << "set proxy (mode=" << current_ui_config_.mode
             << ") for " << current_ui_network_;
+    current_ui_config_.source = ProxyConfig::SOURCE_NETWORK;
     SetProxyConfigForNetwork(current_ui_network_, value, false);
   }
 }
 
 void ProxyConfigServiceImpl::IOSetProxyConfig(
     const ProxyConfig& new_config,
     net::ProxyConfigService::ConfigAvailability new_availability) {
   if (!BrowserThread::CurrentlyOn(BrowserThread::IO) && can_post_task_) {
     // Posts a task to IO thread with the new config, so it can update
     // |cached_config_|.
@@ skipping 89 matching lines @@
       DetermineConfigFromNetwork(network);
   }
 }
 
 void ProxyConfigServiceImpl::DetermineConfigFromNetwork(
     const Network* network) {
   active_config_ = ProxyConfig();  // Default is DIRECT mode (i.e. no proxy).
   net::ProxyConfigService::ConfigAvailability available =
       net::ProxyConfigService::CONFIG_UNSET;
   // If network is shared but user doesn't use shared proxies, use direct mode.
-  if (network->profile_type() == PROFILE_SHARED && !use_shared_proxies_) {
+  if (IgnoreProxy(network)) {
     VLOG(1) << "shared network and !use_shared_proxies, using direct";
     available = net::ProxyConfigService::CONFIG_VALID;
   } else if (!network->proxy_config().empty() &&
              active_config_.DeserializeForNetwork(network->proxy_config())) {
     // Network is private or shared with user using shared proxies.
     VLOG(1) << "using network proxy: " << network->proxy_config();
     available = net::ProxyConfigService::CONFIG_VALID;
   }
   IOSetProxyConfig(active_config_, available);
 }
 
+void ProxyConfigServiceImpl::DetermineUIConfig(Profile* profile,
+    const Network* network) {

[Mattias Nissler (ping if slow), 2011/10/05 10:18:36]

second parameter fits on previous line.

[kuan, 2011/10/07 00:30:41]

it doesn't fit, i align it with 1st param.

On 2011/10/05 10:18:36, Mattias Nissler wrote:

+  if (!network) {
+    if (testing_)
+      current_ui_config_ = active_config_;
+    return;
+  }
+  net::ProxyConfig pref_config;
+  PrefProxyConfigTracker::ConfigState state =
+      PrefProxyConfigTracker::CONFIG_UNSET;
+  if (profile)
+    state = profile->GetProxyConfigTracker()->UIGetProxyConfig(&pref_config);
+  if (state == PrefProxyConfigTracker::CONFIG_PRESENT) {  // Mandatory proxy.
+    current_ui_config_.FromNetProxyConfig(pref_config);
+    const PrefService::Preference* pref = profile->GetPrefs()->FindPreference(
+        prefs::kProxy);
+    current_ui_config_.source = pref && pref->IsManaged() ?
+        ProxyConfig::SOURCE_POLICY :
+        (pref && pref->IsExtensionControlled() ?
+         ProxyConfig::SOURCE_EXTENSION : ProxyConfig::SOURCE_NONE_DISABLED);

[stevenjb, 2011/10/05 00:22:57]

nit: Maybe use an if / else if / else here instead of a double trinary?

[Mattias Nissler (ping if slow), 2011/10/05 10:18:36]

You split the logic between PrefProxyConfigTracker and here, which is fragile.
Can we instead have PrefProxyConfigTracker return a more detailed state that
also indicates the configuration source?

[kuan, 2011/10/07 00:30:41]

Done.

+  } else if (!network->proxy_config().empty()) {  // Network proxy.
+    current_ui_config_.DeserializeForNetwork(network->proxy_config());
+    current_ui_config_.source = IgnoreProxy(network) ?
+        ProxyConfig::SOURCE_NETWORK_DISABLED : ProxyConfig::SOURCE_NETWORK;

[stevenjb, 2011/10/05 00:22:57]

I really think
  source = SOURCE_NETWORK
  is_disabled = IgnoreProxy(network)
 throughout this function would be more readable

[Mattias Nissler (ping if slow), 2011/10/05 10:18:36]

+1

[kuan, 2011/10/07 00:30:41]

Done.

[kuan, 2011/10/07 00:30:41]

Done.

+  } else if (state == PrefProxyConfigTracker::CONFIG_FALLBACK) {
+    // Recommended proxy.
+    current_ui_config_.FromNetProxyConfig(pref_config);
+    current_ui_config_.source = IgnoreProxy(network) ?
+        ProxyConfig::SOURCE_RECOMMENDED_DISABLED :
+        ProxyConfig::SOURCE_RECOMMENDED;
+  } else {  // No proxy.
+    current_ui_config_ = ProxyConfig();
+    current_ui_config_.source = IgnoreProxy(network) ?
+        ProxyConfig::SOURCE_NONE_DISABLED : ProxyConfig::SOURCE_NONE;
+  }
+}
+
 void ProxyConfigServiceImpl::SetCurrentNetworkName(const Network* network) {
   if (!network) {
     if (testing_)
       current_ui_network_name_ = "test";
     return;
   }
   if (network->name().empty() && network->type() == chromeos::TYPE_ETHERNET) {
     current_ui_network_name_ = l10n_util::GetStringUTF8(
         IDS_STATUSBAR_NETWORK_DEVICE_ETHERNET);
   } else {
     current_ui_network_name_ = network->name();
   }
 }
 
+void ProxyConfigServiceImpl::ResetUICache() {
+  current_ui_network_.clear();
+  current_ui_network_name_.clear();
+  current_ui_config_ = ProxyConfig();
+}
+
 void ProxyConfigServiceImpl::CheckCurrentlyOnIOThread() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
 }
 
 void ProxyConfigServiceImpl::CheckCurrentlyOnUIThread() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 }
 
 }  // namespace chromeos