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..6cf0ded2f5d27838cf9e6a202c32175afbf7d0e5 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"

[kuan, 2011/11/10 14:27:25]

now tt u've added notification_service.h, u can remove notification_details.h
and notification_source.h 'cos they're included in the former.

[pastarmovj, 2011/11/10 16:08:26]

Isn't the policy to include everything that we use even if it is indirectly
included? The IWYU model?

 
 namespace chromeos {
 
 CoreChromeOSOptionsHandler::CoreChromeOSOptionsHandler()
-    : handling_change_(false) {
+    : handling_change_(false),
+      initialized_(false),
+      pointer_factory_(this) {
 }
 
-CoreChromeOSOptionsHandler::~CoreChromeOSOptionsHandler() {}
+CoreChromeOSOptionsHandler::~CoreChromeOSOptionsHandler() {
+  if (initialized_) {
+    PrefProxyConfigTracker* proxy_config =

[kuan, 2011/11/10 14:27:25]

maybe use proxy_tracker?

[pastarmovj, 2011/11/10 16:08:26]

Done.

+        Profile::FromWebUI(web_ui_)->GetProxyConfigTracker();
+    proxy_config->RemoveNotificationCallback(
+        base::Bind(&CoreChromeOSOptionsHandler::NotifyNetworkChanged,
+                   pointer_factory_.GetWeakPtr()));
+  }
+}
 
 void CoreChromeOSOptionsHandler::Initialize() {
+  initialized_ = true;
   proxy_prefs_.reset(PrefSetObserver::CreateProxyPrefSetObserver(
     Profile::FromWebUI(web_ui_)->GetPrefs(), this));
+  // Observe the PrefProxyConfigTracker for changes from the UI.

[kuan, 2011/11/10 14:27:25]

s/PrefProxyConfigTracker/chromeos::ProxyConfigServiceImpl/

[pastarmovj, 2011/11/10 16:08:26]

Done.

+  PrefProxyConfigTracker* proxy_config =

[kuan, 2011/11/10 14:27:25]

s/proxy_config/proxy_tracker/

[pastarmovj, 2011/11/10 16:08:26]

Done.

+      Profile::FromWebUI(web_ui_)->GetProxyConfigTracker();
+  proxy_config->AddNotificationCallback(
+      base::Bind(&CoreChromeOSOptionsHandler::NotifyNetworkChanged,
+                 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::NotifyNetworkChanged() {

[kuan, 2011/11/10 14:27:25]

i'm still uncomfortable with the fn name, 'cos it sounds like network has
changed, which hasn't.  maybe change to something to the effect of
NotifyViewingProxyChanged or NotifyChangeInProxyToBeViewed?

[pastarmovj, 2011/11/10 16:08:26]

True the meaning has changed and given that it actually also notifies changes to
prefs from the ui it can be called even more generic 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