Refactor proxy handling for ChromeOS to not go through the CrosSettings interface.

chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.cc

Index: chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.cc
diff --git a/chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.cc b/chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.cc
index 831a5b122bbaa98ec27ad0bb551daf88df943a2d..243fb2f6f27ab22c91047333a9a0d10c3216d103 100644
--- a/chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.cc
+++ b/chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.cc
@@ -6,31 +6,62 @@
 
 #include <string>
 
+#include "base/bind.h"
 #include "base/string_number_conversions.h"
 #include "base/string_util.h"
 #include "chrome/browser/chromeos/cros_settings.h"
+#include "chrome/browser/chromeos/proxy_config_service_impl.h"
+#include "chrome/browser/chromeos/proxy_cros_settings_parser.h"
 #include "chrome/browser/prefs/pref_set_observer.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/common/chrome_notification_types.h"
 #include "chrome/common/pref_names.h"
 #include "content/browser/user_metrics.h"
 #include "content/public/browser/notification_details.h"
+#include "content/public/browser/notification_service.h"
 #include "content/public/browser/notification_source.h"
 
 namespace chromeos {
 
 CoreChromeOSOptionsHandler::CoreChromeOSOptionsHandler()
-    : handling_change_(false) {
+    : handling_change_(false),
+      initialized_(false),
+      pointer_factory_(this) {
 }
 
-CoreChromeOSOptionsHandler::~CoreChromeOSOptionsHandler() {}
+CoreChromeOSOptionsHandler::~CoreChromeOSOptionsHandler() {
+  if (initialized_) {

[Mattias Nissler (ping if slow), 2011/11/11 12:10:46]

the way this is handled in ProxyConfigServiceImpl, you can even Remove the
callback without checking whether you added it before. Also, have you verified
that callback comparisons actually do the right thing?

[pastarmovj, 2011/11/11 15:17:31]

You are right I cleaned up a bit.

The Equals operator in the Callback class checks if the object pointer and the
function pointer coincide and the WeakPointerFactory is still only wrapper for
this so as long as this is the same this check should return true.

+    PrefProxyConfigTracker* proxy_tracker =
+        Profile::FromWebUI(web_ui_)->GetProxyConfigTracker();
+    proxy_tracker->RemoveNotificationCallback(
+        base::Bind(&CoreChromeOSOptionsHandler::NotifyProxyPrefsChanged,
+                   pointer_factory_.GetWeakPtr()));
+  }
+}
 
 void CoreChromeOSOptionsHandler::Initialize() {
+  initialized_ = true;

[Mattias Nissler (ping if slow), 2011/11/11 12:10:46]

maybe move this to the end of the function?

[pastarmovj, 2011/11/11 15:17:31]

Given no returns here the only change we won't do the register is if we crash
which will sort of invalidate the destructor too ;) but sure semantically it is
not initialized at this point yet.

   proxy_prefs_.reset(PrefSetObserver::CreateProxyPrefSetObserver(
     Profile::FromWebUI(web_ui_)->GetPrefs(), this));
+  // Observe the chromeos::ProxyConfigServiceImpl for changes from the UI.
+  PrefProxyConfigTracker* proxy_tracker =
+      Profile::FromWebUI(web_ui_)->GetProxyConfigTracker();
+  proxy_tracker->AddNotificationCallback(
+      base::Bind(&CoreChromeOSOptionsHandler::NotifyProxyPrefsChanged,
+                 pointer_factory_.GetWeakPtr()));
 }
 
-Value* CoreChromeOSOptionsHandler::FetchPref(const std::string& pref_name) {
+base::Value* CoreChromeOSOptionsHandler::FetchPref(
+    const std::string& pref_name) {
+  if (proxy_cros_settings_parser::IsProxyPref(pref_name)) {
+    base::Value *value = NULL;
+    proxy_cros_settings_parser::GetProxyPrefValue(Profile::FromWebUI(web_ui_),
+                                                  pref_name, &value);
+    if (!value)
+      return base::Value::CreateNullValue();
+
+    return value;
+  }
   if (!CrosSettings::IsCrosSettings(pref_name)) {
     // Specially handle kUseSharedProxies because kProxy controls it to
     // determine if it's managed by policy/extension.
@@ -39,7 +70,7 @@ Value* CoreChromeOSOptionsHandler::FetchPref(const std::string& pref_name) {
       const PrefService::Preference* pref =
           pref_service->FindPreference(prefs::kUseSharedProxies);
       if (!pref)
-        return Value::CreateNullValue();
+        return base::Value::CreateNullValue();
       const PrefService::Preference* controlling_pref =
           pref_service->FindPreference(prefs::kProxy);
       return CreateValueForPref(pref, controlling_pref);
@@ -47,27 +78,36 @@ Value* CoreChromeOSOptionsHandler::FetchPref(const std::string& pref_name) {
     return ::CoreOptionsHandler::FetchPref(pref_name);
   }
 
-  Value* pref_value = NULL;
+  base::Value* pref_value = NULL;
   CrosSettings::Get()->Get(pref_name, &pref_value);
-  return pref_value ? pref_value : Value::CreateNullValue();
+  return pref_value ? pref_value : base::Value::CreateNullValue();
 }
 
 void CoreChromeOSOptionsHandler::ObservePref(const std::string& pref_name) {
+  if (proxy_cros_settings_parser::IsProxyPref(pref_name)) {
+    // We observe those all the time.
+    return;
+  }
   if (!CrosSettings::IsCrosSettings(pref_name))
     return ::CoreOptionsHandler::ObservePref(pref_name);
-
   // TODO(xiyuan): Change this when CrosSettings supports observers.
   CrosSettings::Get()->AddSettingsObserver(pref_name.c_str(), this);
 }
 
 void CoreChromeOSOptionsHandler::SetPref(const std::string& pref_name,
-                                         const Value* value,
+                                         const base::Value* value,
                                          const std::string& metric) {
+  if (proxy_cros_settings_parser::IsProxyPref(pref_name)) {
+    proxy_cros_settings_parser::SetProxyPrefValue(Profile::FromWebUI(web_ui_),
+                                                  pref_name, value);
+    ProcessUserMetric(value, metric);
+    return;
+  }
   if (!CrosSettings::IsCrosSettings(pref_name))
     return ::CoreOptionsHandler::SetPref(pref_name, value, metric);
   handling_change_ = true;
   // CrosSettings takes ownership of its value so we need to copy it.
-  Value* pref_value = value->DeepCopy();
+  base::Value* pref_value = value->DeepCopy();
   CrosSettings::Get()->Set(pref_name, pref_value);
   handling_change_ = false;
 
@@ -75,6 +115,8 @@ void CoreChromeOSOptionsHandler::SetPref(const std::string& pref_name,
 }
 
 void CoreChromeOSOptionsHandler::StopObservingPref(const std::string& path) {
+  if (proxy_cros_settings_parser::IsProxyPref(path))
+    return;  // We unregister those in the destructor.
   // Unregister this instance from observing prefs of chrome os settings.
   if (CrosSettings::IsCrosSettings(path))
     CrosSettings::Get()->RemoveSettingsObserver(path.c_str(), this);
@@ -112,7 +154,7 @@ void CoreChromeOSOptionsHandler::NotifySettingsChanged(
     const std::string* setting_name) {
   DCHECK(web_ui_);
   DCHECK(CrosSettings::Get()->IsCrosSettings(*setting_name));
-  Value* value = NULL;
+  base::Value* value = NULL;
   if (!CrosSettings::Get()->Get(*setting_name, &value)) {
     NOTREACHED();
     if (value)
@@ -124,7 +166,7 @@ void CoreChromeOSOptionsHandler::NotifySettingsChanged(
       iter != pref_callback_map_.end(); ++iter) {
     const std::wstring& callback_function = iter->second;
     ListValue result_value;
-    result_value.Append(Value::CreateStringValue(setting_name->c_str()));
+    result_value.Append(base::Value::CreateStringValue(setting_name->c_str()));
     result_value.Append(value->DeepCopy());
     web_ui_->CallJavascriptFunction(WideToASCII(callback_function),
                                     result_value);
@@ -133,4 +175,26 @@ void CoreChromeOSOptionsHandler::NotifySettingsChanged(
     delete value;
 }
 
+void CoreChromeOSOptionsHandler::NotifyProxyPrefsChanged() {
+  DCHECK(web_ui_);
+  for (size_t i = 0; i < kProxySettingsCount; ++i) {
+    base::Value* value = NULL;
+    proxy_cros_settings_parser::GetProxyPrefValue(
+        Profile::FromWebUI(web_ui_), kProxySettings[i], &value);
+    DCHECK(value);
+    PreferenceCallbackMap::const_iterator iter =
+        pref_callback_map_.find(kProxySettings[i]);
+    for (; iter != pref_callback_map_.end(); ++iter) {
+      const std::wstring& callback_function = iter->second;
+      ListValue result_value;
+      result_value.Append(base::Value::CreateStringValue(kProxySettings[i]));
+      result_value.Append(value->DeepCopy());
+      web_ui_->CallJavascriptFunction(WideToASCII(callback_function),
+                                      result_value);
+    }
+    if (value)
+      delete value;
+  }
+}
+
 }  // namespace chromeos