Make CrOS proxy configuration write prefs directly.

chrome/browser/net/pref_proxy_config_service.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/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"
 #include "chrome/browser/prefs/proxy_config_dictionary.h"
 #include "chrome/common/notification_details.h"
 #include "chrome/common/notification_source.h"
 #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() {
   DCHECK(pref_service_ == NULL);
 }
 
 bool PrefProxyConfigTracker::GetProxyConfig(net::ProxyConfig* config) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
   if (valid_)
     *config = pref_config_;
   return valid_;
 }
 
 void PrefProxyConfigTracker::DetachFromPrefService() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   // Stop notifications.
   proxy_prefs_observer_.reset();
   pref_service_ = NULL;
+  fallback_proxy_prefs_observer_.reset();
+  fallback_pref_service_ = NULL;
 }
 
 void PrefProxyConfigTracker::AddObserver(
     PrefProxyConfigTracker::Observer* observer) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
   observers_.AddObserver(observer);
 }
 
 void PrefProxyConfigTracker::RemoveObserver(
     PrefProxyConfigTracker::Observer* observer) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
   observers_.RemoveObserver(observer);
 }
 
 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(
         BrowserThread::IO, FROM_HERE,
         NewRunnableMethod(this,
                           &PrefProxyConfigTracker::InstallProxyConfig,
                           new_config, valid));
   } else {
     NOTREACHED() << "Unexpected notification of type " << type.value;
   }
 }
 
 void PrefProxyConfigTracker::InstallProxyConfig(const net::ProxyConfig& config,
                                                 bool valid) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
   if (valid_ != valid || (valid && !pref_config_.Equals(config))) {
     valid_ = valid;
     if (valid_)
       pref_config_ = config;
     FOR_EACH_OBSERVER(Observer, observers_, OnPrefProxyConfigChanged());
   }
 }
 
 bool PrefProxyConfigTracker::ReadPrefConfig(net::ProxyConfig* config) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   // 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)) {
     // Fall back to system settings if the mode preference is invalid.
     return false;
   }
 
   switch (mode) {
     case ProxyPrefs::MODE_SYSTEM:
       // Use system settings.
@@ skipping 46 matching lines @@
 PrefProxyConfigService::PrefProxyConfigService(
     PrefProxyConfigTracker* tracker,
     net::ProxyConfigService* base_service)
     : base_service_(base_service),
       pref_config_tracker_(tracker),
       registered_observers_(false) {
 }
 
 PrefProxyConfigService::~PrefProxyConfigService() {
   if (registered_observers_) {
-    base_service_->RemoveObserver(this);
+    if (base_service_.get())
+      base_service_->RemoveObserver(this);
     pref_config_tracker_->RemoveObserver(this);
   }
 }
 
 void PrefProxyConfigService::AddObserver(
     net::ProxyConfigService::Observer* observer) {
   RegisterObservers();
   observers_.AddObserver(observer);
 }
 
 void PrefProxyConfigService::RemoveObserver(
     net::ProxyConfigService::Observer* observer) {
   observers_.RemoveObserver(observer);
 }
 
 bool PrefProxyConfigService::GetLatestProxyConfig(net::ProxyConfig* config) {
   RegisterObservers();
   net::ProxyConfig pref_config;
   if (pref_config_tracker_->GetProxyConfig(&pref_config)) {
     *config = pref_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(
     const net::ProxyConfig& config) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
 
   // Check whether there is a proxy configuration defined by preferences. In
   // this case that proxy configuration takes precedence and the change event
   // from the delegate proxy service can be disregarded.
   net::ProxyConfig pref_config;
@@ skipping 17 matching lines @@
   net::ProxyConfig new_config;
   if (GetLatestProxyConfig(&new_config)) {
     FOR_EACH_OBSERVER(net::ProxyConfigService::Observer, observers_,
                       OnProxyConfigChanged(new_config));
   }
 }
 
 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;
   }
 }
 
 // static
 void PrefProxyConfigService::RegisterPrefs(PrefService* pref_service) {
   DictionaryValue* default_settings = ProxyConfigDictionary::CreateSystem();
   pref_service->RegisterDictionaryPref(prefs::kProxy, default_settings);
 }