Extract common per-network proxy configuration functions.

Extract common per-network proxy configuration functions.

- Extracted functions to get and set per-network proxy configuration from ProxyConfigServiceImpl, UIProxyConfigService and NetworkStateInformer, and moved them to a common place (chrome/browser/chromeos/net/proxy_config_handler.h).

- The previous SetProxyConfig call to NetworkLibrary was extracted and instead ShillServiceClient is called directly.

- Now uses NetworkStateHandler and NetworkState instead of NetworkLibrary to get the current proxy config.

- This removed any dependency of proxy UI on NetworkLibrary.

BUG=234982
R=mnissler@chromium.org, nkostylev@chromium.org, stevenjb@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=205214

[pneubeck (no reviews), 2013-06-06 15:49:23]

See also the shared doc for the "roadmap".

[pneubeck (no reviews), 2013-06-06 15:52:34]

@Mattias, FYI

[stevenjb, 2013-06-06 18:21:12]

One suggestion, which I am fine with skipping for now. Otherwise LGTM.

https://codereview.chromium.org/16398005/diff/3001/chrome/browser/chromeos/ne...
File chrome/browser/chromeos/net/proxy_config_handler.cc (right):

https://codereview.chromium.org/16398005/diff/3001/chrome/browser/chromeos/ne...
chrome/browser/chromeos/net/proxy_config_handler.cc:23: LOG(ERROR) << "Could not
clear or set ProxyConfig: " << error_message;
Could you use network_handler::ShillErrorCallbackFunction instead?
You can use network_handler::ErrorCallback() for |error_callback| since the
function will handle any logging.

https://codereview.chromium.org/16398005/diff/3001/chrome/browser/chromeos/ne...
File chrome/browser/chromeos/net/proxy_config_handler.h (right):

https://codereview.chromium.org/16398005/diff/3001/chrome/browser/chromeos/ne...
chrome/browser/chromeos/net/proxy_config_handler.h:17:
scoped_ptr<ProxyConfigDictionary> GetProxyConfigOfNetwork(
nit: s/Of/For
Also, should these have success / error callbacks like the
NetworkConfigurationHandler functions? It seems like it would be more
consistent, and something we should handle in the future, even if they are
currently NULL.

https://codereview.chromium.org/16398005/diff/3001/chrome/browser/chromeos/ne...
chrome/browser/chromeos/net/proxy_config_handler.h:21: const NetworkState&
network);
Here too

[Mattias Nissler (ping if slow), 2013-06-07 09:10:14]

Didn't look very thoroughly, but what I saw looks good :)

One thing to improve is the commit message, it's entirely unobvious what you
mean by "central place" to somebody just reading the commit message.

https://codereview.chromium.org/16398005/diff/9001/chrome/browser/chromeos/ne...
File chrome/browser/chromeos/net/proxy_config_handler.cc (right):

https://codereview.chromium.org/16398005/diff/9001/chrome/browser/chromeos/ne...
chrome/browser/chromeos/net/proxy_config_handler.cc:32:
scoped_ptr<ProxyConfigDictionary> GetProxyConfigOfNetwork(
nit: ForNetwork is probably more accurate, as proxy config is technically not a
characteristic of the network.

https://codereview.chromium.org/16398005/diff/9001/chrome/browser/chromeos/ne...
chrome/browser/chromeos/net/proxy_config_handler.cc:40: void
SetProxyConfigOfNetwork(const ProxyConfigDictionary& proxy_config,
ditto

[pneubeck (no reviews), 2013-06-07 09:46:09]

https://codereview.chromium.org/16398005/diff/3001/chrome/browser/chromeos/ne...
File chrome/browser/chromeos/net/proxy_config_handler.cc (right):

https://codereview.chromium.org/16398005/diff/3001/chrome/browser/chromeos/ne...
chrome/browser/chromeos/net/proxy_config_handler.cc:23: LOG(ERROR) << "Could not
clear or set ProxyConfig: " << error_message;
On 2013/06/06 18:21:12, stevenjb (chromium) wrote:
> Could you use network_handler::ShillErrorCallbackFunction instead?
> You can use network_handler::ErrorCallback() for |error_callback| since the
> function will handle any logging.

Done.

https://codereview.chromium.org/16398005/diff/3001/chrome/browser/chromeos/ne...
File chrome/browser/chromeos/net/proxy_config_handler.h (right):

https://codereview.chromium.org/16398005/diff/3001/chrome/browser/chromeos/ne...
chrome/browser/chromeos/net/proxy_config_handler.h:17:
scoped_ptr<ProxyConfigDictionary> GetProxyConfigOfNetwork(
On 2013/06/06 18:21:12, stevenjb (chromium) wrote:
> nit: s/Of/For
> Also, should these have success / error callbacks like the
> NetworkConfigurationHandler functions? It seems like it would be more
> consistent, and something we should handle in the future, even if they are
> currently NULL.

I agree, that we should go in that direction.
However, as these callbacks wouldn't be used, I'd like to postpone that change
and avoid yet another bunch of calls in the kind of:
 handler->Set...(
        ...
        chromeos::network_handler::ErrorCallback());

These are already all over the place.

https://codereview.chromium.org/16398005/diff/3001/chrome/browser/chromeos/ne...
chrome/browser/chromeos/net/proxy_config_handler.h:21: const NetworkState&
network);
On 2013/06/06 18:21:12, stevenjb (chromium) wrote:
> Here too

Done.

https://codereview.chromium.org/16398005/diff/9001/chrome/browser/chromeos/ne...
File chrome/browser/chromeos/net/proxy_config_handler.cc (right):

https://codereview.chromium.org/16398005/diff/9001/chrome/browser/chromeos/ne...
chrome/browser/chromeos/net/proxy_config_handler.cc:32:
scoped_ptr<ProxyConfigDictionary> GetProxyConfigOfNetwork(
On 2013/06/07 09:10:14, Mattias Nissler wrote:
> nit: ForNetwork is probably more accurate, as proxy config is technically not
a
> characteristic of the network.

Done.

https://codereview.chromium.org/16398005/diff/9001/chrome/browser/chromeos/ne...
chrome/browser/chromeos/net/proxy_config_handler.cc:40: void
SetProxyConfigOfNetwork(const ProxyConfigDictionary& proxy_config,
On 2013/06/07 09:10:14, Mattias Nissler wrote:
> ditto

Done.

[commit-bot: I haz the power, 2013-06-07 09:46:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pneubeck@chromium.org/16398005/17001

[commit-bot: I haz the power, 2013-06-07 09:46:31]

Failed to apply patch for chrome/browser/chromeos/ui_proxy_config_service.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/chromeos/ui_proxy_config_service.cc
  Hunk #1 FAILED at 7.
  Hunk #2 FAILED at 35.
  Hunk #3 succeeded at 93 (offset 19 lines).
  Hunk #4 FAILED at 105.
  Hunk #5 succeeded at 204 (offset 70 lines).
  Hunk #6 succeeded at 232 (offset 70 lines).
  3 out of 6 hunks FAILED -- saving rejects to file
chrome/browser/chromeos/ui_proxy_config_service.cc.rej

Patch:       chrome/browser/chromeos/ui_proxy_config_service.cc
Index: chrome/browser/chromeos/ui_proxy_config_service.cc
diff --git a/chrome/browser/chromeos/ui_proxy_config_service.cc
b/chrome/browser/chromeos/ui_proxy_config_service.cc
index
a613596d3d20cf1a32609d7d6b6410a28818f7f0..0f6988a65c2a2c769d7d302e78c8ee4267007526
100644
--- a/chrome/browser/chromeos/ui_proxy_config_service.cc
+++ b/chrome/browser/chromeos/ui_proxy_config_service.cc
@@ -7,10 +7,10 @@
 #include "base/logging.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/values.h"
-#include "chrome/browser/chromeos/cros/cros_library.h"
-#include "chrome/browser/chromeos/cros/network_library.h"
+#include "chrome/browser/chromeos/net/proxy_config_handler.h"
 #include "chrome/browser/chromeos/proxy_config_service_impl.h"
-#include "chromeos/network/onc/onc_utils.h"
+#include "chromeos/network/network_state.h"
+#include "chromeos/network/network_state_handler.h"
 #include "grit/generated_resources.h"
 #include "net/proxy/proxy_config.h"
 
@@ -35,27 +35,21 @@ const char* ModeToString(UIProxyConfig::Mode mode) {
   return "";
 }
 
-bool ParseProxyConfig(const std::string& pref_proxy_config,
-                      net::ProxyConfig* proxy_config) {
-  if (pref_proxy_config.empty())
+// Writes the proxy config of |network| to |proxy_config|.  Returns false if no
+// proxy was configured for this network.
+bool GetProxyConfig(const NetworkState& network,
+                    net::ProxyConfig* proxy_config) {
+  scoped_ptr<ProxyConfigDictionary> proxy_dict =
+      proxy_config::GetProxyConfigForNetwork(network);
+  if (!proxy_dict)
     return false;
-
-  scoped_ptr<base::DictionaryValue> proxy_config_dict(
-      chromeos::onc::ReadDictionaryFromJson(pref_proxy_config));
-  if (!proxy_config_dict) {
-    LOG(WARNING) << "Failed to parse proxy config.";
-    return false;
-  }
-
-  ProxyConfigDictionary proxy_config_dict_wrapper(proxy_config_dict.get());
-  return PrefProxyConfigTrackerImpl::PrefConfigToNetConfig(
-      proxy_config_dict_wrapper,
-      proxy_config);
+  return PrefProxyConfigTrackerImpl::PrefConfigToNetConfig(*proxy_dict,
+                                                           proxy_config);
 }
 
 // Returns true if proxy settings of |network| are editable.
-bool IsNetworkProxySettingsEditable(const Network& network) {
-  onc::ONCSource source = network.ui_data().onc_source();
+bool IsNetworkProxySettingsEditable(const NetworkState& network) {
+  onc::ONCSource source = network.onc_source();
   return source != onc::ONC_SOURCE_DEVICE_POLICY &&
       source != onc::ONC_SOURCE_USER_POLICY;
 }
@@ -74,9 +68,9 @@ void UIProxyConfigService::SetPrefs(PrefService* pref_service)
{
 
 void UIProxyConfigService::SetCurrentNetwork(
     const std::string& current_network) {
-  Network* network = NULL;
+  const NetworkState* network = NULL;
   if (!current_network.empty()) {
-    network = CrosLibrary::Get()->GetNetworkLibrary()->FindNetworkByPath(
+    network = NetworkHandler::Get()->network_state_handler()->GetNetworkState(
         current_network);
     LOG_IF(ERROR, !network)
         << "Can't find requested network " << current_network;
@@ -105,24 +99,28 @@ void UIProxyConfigService::SetProxyConfig(const
UIProxyConfig& config) {
   if (current_ui_network_.empty())
     return;
 
-  // Update config to shill.
-  std::string value;
-  if (!current_ui_config_.SerializeForNetwork(&value))
-    return;
-
-  VLOG(1) << "Set proxy for " << current_ui_network_ << " to " << value;
-  current_ui_config_.state = ProxyPrefs::CONFIG_SYSTEM;
-
-  Network* network =
CrosLibrary::Get()->GetNetworkLibrary()->FindNetworkByPath(
-      current_ui_network_);
+  const NetworkState* network =
+      NetworkHandler::Get()->network_state_handler()->
+      GetNetworkState(current_ui_network_);
   if (!network) {
     LOG(ERROR) << "Can't find requested network " << current_ui_network_;
     return;
   }
-  network->SetProxyConfig(value);
+
+  // Store config for this network.
+  scoped_ptr<base::DictionaryValue> proxy_config_value(
+      config.ToPrefProxyConfig());
+  ProxyConfigDictionary proxy_config_dict(proxy_config_value.get());
+
+  VLOG(1) << "Set proxy for " << current_ui_network_
+          << " to " << *proxy_config_value;
+
+  proxy_config::SetProxyConfigForNetwork(proxy_config_dict, *network);
+  current_ui_config_.state = ProxyPrefs::CONFIG_SYSTEM;
 }
 
-void UIProxyConfigService::DetermineEffectiveConfig(const Network& network) {
+void UIProxyConfigService::DetermineEffectiveConfig(
+    const NetworkState& network) {
   DCHECK(pref_service_);
 
   // Get prefs proxy config if available.
@@ -134,7 +132,7 @@ void UIProxyConfigService::DetermineEffectiveConfig(const
Network& network) {
   net::ProxyConfig network_config;
   net::ProxyConfigService::ConfigAvailability network_availability =
       net::ProxyConfigService::CONFIG_UNSET;
-  if (ParseProxyConfig(network.proxy_config(), &network_config)) {
+  if (chromeos::GetProxyConfig(network, &network_config)) {
     // Network is private or shared with user using shared proxies.
     VLOG(1) << this << ": using network proxy: " << network.proxy_config();
     network_availability = net::ProxyConfigService::CONFIG_VALID;
@@ -162,7 +160,7 @@ void UIProxyConfigService::DetermineEffectiveConfig(const
Network& network) {
     current_ui_config_.user_modifiable =
         !ProxyConfigServiceImpl::IgnoreProxy(pref_service_,
                                              network.profile_path(),
-                                             network.ui_data().onc_source());
+                                             network.onc_source());
   }
 }

[pneubeck (no reviews), 2013-06-07 11:30:59]

Hi Nikita,
please take a look at the two remaining files:
 .../ui/webui/options/preferences_browsertest.cc
 .../ui/webui/chromeos/login/network_state_informer.cc

Thanks!

[Nikita (slow), 2013-06-10 09:07:51]

lgtm

[commit-bot: I haz the power, 2013-06-10 09:40:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pneubeck@chromium.org/16398005/38001

[commit-bot: I haz the power, 2013-06-10 09:52:38]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[Mattias Nissler (ping if slow), 2013-06-10 12:30:51]

LGTM

[commit-bot: I haz the power, 2013-06-10 12:31:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pneubeck@chromium.org/16398005/38001

[pneubeck (no reviews), 2013-06-10 15:10:03]

Committed patchset #4 manually as r205214 (presubmit successful).