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

chrome/browser/chromeos/proxy_config_service_impl.cc

Index: chrome/browser/chromeos/proxy_config_service_impl.cc
===================================================================
--- chrome/browser/chromeos/proxy_config_service_impl.cc	(revision 103881)
+++ chrome/browser/chromeos/proxy_config_service_impl.cc	(working copy)
@@ -11,9 +11,13 @@
 #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"
@@ -29,10 +33,20 @@
   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 "";
@@ -40,9 +54,7 @@
 
 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;
 }
 
@@ -51,25 +63,28 @@
   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";
@@ -102,18 +117,10 @@
 
 }  // 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() {}
 
@@ -153,31 +160,53 @@
   }
 }
 
-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*
@@ -273,6 +302,8 @@
 
 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) {
@@ -487,65 +518,57 @@
   *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;
@@ -712,6 +735,7 @@
   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);
   }
 }
@@ -821,7 +845,7 @@
   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() &&
@@ -833,6 +857,43 @@
   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_)
@@ -847,6 +908,12 @@
   }
 }
 
+void ProxyConfigServiceImpl::ResetUICache() {
+  current_ui_network_.clear();
+  current_ui_network_name_.clear();
+  current_ui_config_ = ProxyConfig();
+}
+
 void ProxyConfigServiceImpl::CheckCurrentlyOnIOThread() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
 }