Make CrOS proxy configuration write prefs directly.

chrome/browser/net/pref_proxy_config_service.cc

Index: chrome/browser/net/pref_proxy_config_service.cc
diff --git a/chrome/browser/net/pref_proxy_config_service.cc b/chrome/browser/net/pref_proxy_config_service.cc
index e783f39a5086e47836392e835e68bbefd176918b..a06f6b1ebcc25d44479f77d99d4c847cf970ec8a 100644
--- a/chrome/browser/net/pref_proxy_config_service.cc
+++ b/chrome/browser/net/pref_proxy_config_service.cc
@@ -5,6 +5,7 @@
 #include "chrome/browser/net/pref_proxy_config_service.h"
 
 #include "base/values.h"
+#include "chrome/browser/browser_process.h"
 #include "chrome/browser/browser_thread.h"
 #include "chrome/browser/prefs/pref_service.h"
 #include "chrome/browser/prefs/pref_set_observer.h"
@@ -14,11 +15,21 @@
 #include "chrome/common/notification_type.h"
 #include "chrome/common/pref_names.h"
 
-PrefProxyConfigTracker::PrefProxyConfigTracker(PrefService* pref_service)
-    : pref_service_(pref_service) {
+PrefProxyConfigTracker::PrefProxyConfigTracker(
+    PrefService* pref_service,
+    PrefService* fallback_pref_service)
+    : pref_service_(pref_service),
+      fallback_pref_service_(fallback_pref_service) {
   valid_ = ReadPrefConfig(&pref_config_);
   proxy_prefs_observer_.reset(
       PrefSetObserver::CreateProxyPrefSetObserver(pref_service_, this));
+
+  // Also install an observer on |fallback|, if appropriate.

[battre, 2011/02/22 10:32:40]

nit: |fallback_pref_service_|

[Mattias Nissler (ping if slow), 2011/02/22 11:08:02]

Done.

+  if (fallback_pref_service_) {
+    fallback_proxy_prefs_observer_.reset(
+        PrefSetObserver::CreateProxyPrefSetObserver(fallback_pref_service_,
+                                                    this));
+  }
 }
 
 PrefProxyConfigTracker::~PrefProxyConfigTracker() {
@@ -37,6 +48,8 @@ void PrefProxyConfigTracker::DetachFromPrefService() {
   // Stop notifications.
   proxy_prefs_observer_.reset();
   pref_service_ = NULL;
+  fallback_proxy_prefs_observer_.reset();
+  fallback_pref_service_ = NULL;
 }
 
 void PrefProxyConfigTracker::AddObserver(
@@ -55,8 +68,9 @@ void PrefProxyConfigTracker::Observe(NotificationType type,
                                      const NotificationSource& source,
                                      const NotificationDetails& details) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  const PrefService* pref_source = Source<PrefService>(source).ptr();
   if (type == NotificationType::PREF_CHANGED &&
-      Source<PrefService>(source).ptr() == pref_service_) {
+      (pref_source == pref_service_ || pref_source == fallback_pref_service_)) {
     net::ProxyConfig new_config;
     bool valid = ReadPrefConfig(&new_config);
     BrowserThread::PostTask(
@@ -86,7 +100,28 @@ bool PrefProxyConfigTracker::ReadPrefConfig(net::ProxyConfig* config) {
   // Clear the configuration.
   *config = net::ProxyConfig();
 
-  ProxyConfigDictionary proxy_dict(pref_service_->GetDictionary(prefs::kProxy));
+  const PrefService::Preference* pref =
+      pref_service_->FindPreference(prefs::kProxy);
+  if (!pref) {
+    NOTREACHED();
+    return true;
+  }
+
+  const DictionaryValue* proxy_value =
+      pref_service_->GetDictionary(prefs::kProxy);
+  if (fallback_pref_service_ &&
+      pref->IsUserModifiable() &&
+      !pref->HasUserSetting()) {

[danno, 2011/02/21 21:45:20]

We discussed a strategy offline that was more like this:
1) Use user managed policy
2) if not set, use device managed policy
3) if not set, use user extension policy 
4) if not set, use what user has selected in the setting page (local state)
5) if not set, use user recommended policy (or maybe not?)
6) if not set, use device recommended policy
... or something to that effect. This code seems to implement something
different, more like:
1) Use user managed policy
2) if not set use user extension policy
3) if not set, use what user has selected in the setting page (local state)
4) use use recommended policy
can you explain why? will the other changes come in a later CL?

[Mattias Nissler (ping if slow), 2011/02/22 09:45:34]

I pondered this for quite a while. I came to the conclusion that for policy,
extensions, etc. it's probably better to not have device override user or vice
versa in any way. That is so these stakeholders get more flexibility on how the
overriding should work. In the case of policy, we want to make that decision at
the policy provider level (i.e. fall back to device proxy policy if there's no
user proxy policy or the like).

Things are a bit different for user settings though. We could decide not to do
the fallback here, but that would mean that when writing proxy prefs, we'd have
to put them not only into local state but the PrefServices of all Profiles we
know of. Even worse: If a profile is created later, we'd have to seed the proxy
settings there. That seemed a bad design choice to me, so I decided to go with
the idea of defaulting proxy user prefs (and only that layer in the pref store
hierarchy) to what's local state. This is what the code implements.

In a later CL, I indeed plan to modify our cloud policy providers so device
proxy policy and user policy are merged for the profile proxy configuration.

+    // Default to the fallback, if existing.
+    const PrefService::Preference* fallback_pref =
+        fallback_pref_service_->FindPreference(prefs::kProxy);
+    if (!fallback_pref)
+      NOTREACHED();
+    else if (fallback_pref->HasUserSetting())
+      proxy_value = fallback_pref_service_->GetMutableDictionary(prefs::kProxy);
+  }
+
+  ProxyConfigDictionary proxy_dict(proxy_value);
 
   ProxyPrefs::ProxyMode mode;
   if (!proxy_dict.GetMode(&mode)) {
@@ -153,7 +188,8 @@ PrefProxyConfigService::PrefProxyConfigService(
 
 PrefProxyConfigService::~PrefProxyConfigService() {
   if (registered_observers_) {
-    base_service_->RemoveObserver(this);
+    if (base_service_.get())
+      base_service_->RemoveObserver(this);
     pref_config_tracker_->RemoveObserver(this);
   }
 }
@@ -177,11 +213,17 @@ bool PrefProxyConfigService::GetLatestProxyConfig(net::ProxyConfig* config) {
     return true;
   }
 
-  return base_service_->GetLatestProxyConfig(config);
+  if (base_service_.get())
+    return base_service_->GetLatestProxyConfig(config);
+
+  // Fall back to direct configuration.
+  *config = net::ProxyConfig::CreateDirect();
+  return true;
 }
 
 void PrefProxyConfigService::OnLazyPoll() {
-  base_service_->OnLazyPoll();
+  if (base_service_.get())
+    base_service_->OnLazyPoll();
 }
 
 void PrefProxyConfigService::OnProxyConfigChanged(
@@ -219,7 +261,8 @@ void PrefProxyConfigService::OnPrefProxyConfigChanged() {
 void PrefProxyConfigService::RegisterObservers() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
   if (!registered_observers_) {
-    base_service_->AddObserver(this);
+    if (base_service_.get())
+      base_service_->AddObserver(this);
     pref_config_tracker_->AddObserver(this);
     registered_observers_ = true;
   }