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..6150c865102ed9ea36c5a59a72c9b37543fea36a 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
@@ -17,8 +17,37 @@
 #include "content/public/browser/notification_details.h"
 #include "content/public/browser/notification_source.h"
 

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

merge the two include blocks.

[pastarmovj, 2011/11/09 17:51:53]

Done. Facepalm for me :)

+#include "chrome/browser/chromeos/proxy_config_service_impl.h"
+#include "chrome/browser/chromeos/proxy_cros_settings_parser.h"
+#include "chrome/browser/profiles/profile.h"
+#include "chrome/common/chrome_notification_types.h"
+#include "content/public/browser/notification_service.h"
+
 namespace chromeos {
 
+namespace {
+
+const char kProxyPrefsPrefix[] = "cros.session.proxy";
+
+const char* const kProxySettings[] = {
+  kProxyPacUrl,
+  kProxySingleHttp,
+  kProxySingleHttpPort,
+  kProxyHttpUrl,
+  kProxyHttpPort,
+  kProxyHttpsUrl,
+  kProxyHttpsPort,
+  kProxyType,
+  kProxySingle,
+  kProxyFtpUrl,
+  kProxyFtpPort,
+  kProxySocks,
+  kProxySocksPort,
+  kProxyIgnoreList,
+};
+

[kuan, 2011/11/08 14:45:25]

u may want to comment here and proxy_cros_settings_parser.h/cc that
kProxySettings must include all settings declared in
proxy_cros_settings_parser.h, so that future changes to settings will be
consistent throughout. 

[pastarmovj, 2011/11/09 17:51:53]

I moved that to proxy_cros_settings_parser.h.

+}
+
 CoreChromeOSOptionsHandler::CoreChromeOSOptionsHandler()
     : handling_change_(false) {
 }
@@ -26,11 +55,22 @@ CoreChromeOSOptionsHandler::CoreChromeOSOptionsHandler()
 CoreChromeOSOptionsHandler::~CoreChromeOSOptionsHandler() {}
 
 void CoreChromeOSOptionsHandler::Initialize() {
+  proxy_registrar_clients_ = 0;
   proxy_prefs_.reset(PrefSetObserver::CreateProxyPrefSetObserver(
     Profile::FromWebUI(web_ui_)->GetPrefs(), this));
 }
 
-Value* CoreChromeOSOptionsHandler::FetchPref(const std::string& pref_name) {
+base::Value* CoreChromeOSOptionsHandler::FetchPref(
+    const std::string& pref_name) {
+  if (pref_name.find(kProxyPrefsPrefix) == 0) {

[kuan, 2011/11/08 14:45:25]

is StartWith enough?
in any case, maybe place this fn in a separate fn like IsProxyPref() and call it
everywhere else?

[pastarmovj, 2011/11/09 17:51:53]

It is enough and I actually made a nice helper function for that.

+    base::Value *value = NULL;
+    ProxyCrosSettingsParser::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 +79,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 +87,41 @@ 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 (pref_name.find(kProxyPrefsPrefix) == 0) {
+    // We already listen for all events that concern proxies.

[kuan, 2011/11/08 14:45:25]

i was confused by ur comment and the code below.  u only want to execute the
code below if u haven't registered for notification right?  if yes, maybe
rephrase the comment to something like "if we're not yet listening for events
...., do so now."
shouldn't u be incrementing proxy_registrar_clients_ for ObserverPref of every
proxy setting even if proxy_registrar_ is not empty?  'cos u decrement
proxy_registrar_clients for StopObservingPref of every proxy setting.

[pastarmovj, 2011/11/09 17:51:53]

You were right the comment was outdated and wrong. Fixed.

+    if (proxy_registrar_.IsEmpty()) {

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

why not just listen in any case?

[pastarmovj, 2011/11/09 17:51:53]

Done so.

+      proxy_registrar_clients_++;

[kuan, 2011/11/08 14:45:25]

should pre-increment?

[pastarmovj, 2011/11/09 17:51:53]

Removed altogether.

+      proxy_registrar_.Add(this, chrome::NOTIFICATION_CURRENT_NETWORK_CHANGED,
+                           content::NotificationService::AllSources());
+    }
+    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 (pref_name.find(kProxyPrefsPrefix) == 0) {
+    ProxyCrosSettingsParser::SetProxyPrefValue(Profile::FromWebUI(web_ui_),
+                                               pref_name, value);
+    ProcessUserMetric(value, metric);

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

I don't like the duplication here...

[kuan, 2011/11/08 14:45:25]

well, it's not exact duplication 'cos u miss out the resetting handling_change_
to false like for other settings.

[pastarmovj, 2011/11/09 17:51:53]

I agree the one line repetition is really not such a big deal I could reverse
the order and possibly get rid of it but why convoluting the logic just for
that?

+    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;
 
@@ -76,10 +130,19 @@ void CoreChromeOSOptionsHandler::SetPref(const std::string& pref_name,
 
 void CoreChromeOSOptionsHandler::StopObservingPref(const std::string& path) {
   // Unregister this instance from observing prefs of chrome os settings.
-  if (CrosSettings::IsCrosSettings(path))
+  if (CrosSettings::IsCrosSettings(path)) {
     CrosSettings::Get()->RemoveSettingsObserver(path.c_str(), this);
-  else  // Call base class to handle regular preferences.
+  } else if (path.find(kProxyPrefsPrefix) == 0) {

[kuan, 2011/11/08 14:45:25]

wouldn't kProxyPRefsPrefix return true for the first "if
CrosSettings::IsCrosSettings"?  then ur path.find will never execute.

[pastarmovj, 2011/11/09 17:51:53]

True good catch!

+    // We listen for all events that concern proxies.
+    if (--proxy_registrar_clients_ <= 0) {
+      proxy_registrar_clients_ = 0;  // Safeguard the value to not go below 0.
+      proxy_registrar_.RemoveAll();
+    }
+    return;
+  } else {
+    // Call base class to handle regular preferences.
     ::CoreOptionsHandler::StopObservingPref(path);
+  }
 }
 
 void CoreChromeOSOptionsHandler::Observe(
@@ -93,6 +156,10 @@ void CoreChromeOSOptionsHandler::Observe(
     NotifySettingsChanged(content::Details<std::string>(details).ptr());
     return;
   }
+  if (type == chrome::NOTIFICATION_CURRENT_NETWORK_CHANGED) {
+    NotifyNetworkChanged();

[kuan, 2011/11/08 14:45:25]

like i mentioned in chrome_notification_types.h, NotifyNetworkChanged is
probably not the correct function name.

[pastarmovj, 2011/11/09 17:51:53]

Is removed now in favor of a callback.

+    return;
+  }
   // Special handling for preferences kUseSharedProxies and kProxy, the latter
   // controls the former and decides if it's managed by policy/extension.
   if (type == chrome::NOTIFICATION_PREF_CHANGED) {
@@ -112,7 +179,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 +191,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 +200,26 @@ void CoreChromeOSOptionsHandler::NotifySettingsChanged(
     delete value;
 }
 
+void CoreChromeOSOptionsHandler::NotifyNetworkChanged() {
+  DCHECK(web_ui_);
+  for (size_t i = 0; i < arraysize(kProxySettings); ++i) {
+    base::Value* value = NULL;
+    ProxyCrosSettingsParser::GetProxyPrefValue(
+        Profile::FromWebUI(web_ui_),kProxySettings[i], &value);
+    DCHECK(value);

[kuan, 2011/11/08 14:45:25]

DCHECK(value) is useless for chrome on chromeos, 'cos it's always run in
non-debug mode, so u're gonna crash below if value is null.

[pastarmovj, 2011/11/09 17:51:53]

This is not really true. If you build chrome from external sources and with the
proper flags you get debug builds of chrome on ChromeOS. Crashing will happen
anyway but in debug we'll get the nice stack trace too.

+    for (PreferenceCallbackMap::const_iterator iter =
+      pref_callback_map_.find(kProxySettings[i]);

[kuan, 2011/11/08 14:45:25]

indentation

[pastarmovj, 2011/11/09 17:51:53]

I stole that from the function above but now it's changed to my preferred
syntax. Initialize the iterator outside of the for decl.

+      iter != pref_callback_map_.end(); ++iter) {

[kuan, 2011/11/08 14:45:25]

indentation

[pastarmovj, 2011/11/09 17:51:53]

Done.

+      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