arc: update proxy settings from UI and ONC policy.

chrome/browser/chromeos/arc/arc_settings_service.cc

Index: chrome/browser/chromeos/arc/arc_settings_service.cc
diff --git a/chrome/browser/chromeos/arc/arc_settings_service.cc b/chrome/browser/chromeos/arc/arc_settings_service.cc
index 7d2e4f4a0599fe255810ab0dcc4500f186ef062b..e4d0e65a84d63ac7069e2f01ede17d59e67112b7 100644
--- a/chrome/browser/chromeos/arc/arc_settings_service.cc
+++ b/chrome/browser/chromeos/arc/arc_settings_service.cc
@@ -10,15 +10,23 @@
 #include "base/json/json_writer.h"
 #include "base/strings/stringprintf.h"
 #include "base/values.h"
+#include "chrome/browser/browser_process.h"
 #include "chrome/browser/chromeos/arc/arc_auth_service.h"
+#include "chrome/browser/chromeos/net/proxy_config_handler.h"
+#include "chrome/browser/chromeos/proxy_config_service_impl.h"
 #include "chrome/browser/chromeos/settings/cros_settings.h"
 #include "chrome/browser/profiles/profile_manager.h"
 #include "chrome/common/pref_names.h"
+#include "chromeos/network/network_handler.h"
+#include "chromeos/network/network_state.h"
+#include "chromeos/network/network_state_handler.h"
+#include "chromeos/network/network_state_handler_observer.h"
 #include "chromeos/settings/cros_settings_names.h"
 #include "chromeos/settings/timezone_settings.h"
 #include "components/arc/intent_helper/font_size_util.h"
 #include "components/prefs/pref_change_registrar.h"
 #include "components/prefs/pref_service.h"
+#include "components/proxy_config/pref_proxy_config_tracker_impl.h"
 #include "components/proxy_config/proxy_config_dictionary.h"
 #include "components/proxy_config/proxy_config_pref_names.h"
 #include "device/bluetooth/bluetooth_adapter.h"
@@ -30,11 +38,44 @@ using ::chromeos::system::TimezoneSettings;
 
 namespace {
 
-bool GetHttpProxyServer(const ProxyConfigDictionary& proxy_config_dict,
+std::unique_ptr<ProxyConfigDictionary> GetProxyConfig(
+    const chromeos::NetworkState* network) {
+  Profile* profile = ProfileManager::GetActiveUserProfile();
+
+  // Apply Pref Proxy configuration if available.
+  net::ProxyConfig pref_proxy_config;
+  ProxyPrefs::ConfigState pref_state =
+      PrefProxyConfigTrackerImpl::ReadPrefConfig(profile->GetPrefs(),
+                                                 &pref_proxy_config);
+  if (PrefProxyConfigTrackerImpl::PrefPrecedes(pref_state)) {
+    const PrefService::Preference* const pref =
+        profile->GetPrefs()->FindPreference(proxy_config::prefs::kProxy);
+    const base::DictionaryValue* proxy_config_value;
+    bool value_exists = pref->GetValue()->GetAsDictionary(&proxy_config_value);
+    DCHECK(value_exists);
+
+    return base::MakeUnique<ProxyConfigDictionary>(proxy_config_value);
+  }
+
+  // Apply network proxy configuration.
+  onc::ONCSource onc_source;
+  std::unique_ptr<ProxyConfigDictionary> proxy_config =
+      chromeos::proxy_config::GetProxyConfigForNetwork(
+          profile->GetPrefs(), g_browser_process->local_state(), *network,
+          &onc_source);
+  if (chromeos::ProxyConfigServiceImpl::IgnoreProxy(
+          profile->GetPrefs(), network->profile_path(), onc_source)) {

[stevenjb, 2016/09/02 16:20:55]

Can we put this logic into proxy_config_service_impl.cc? It is similar to 
ProxyConfigServiceImpl::DetermineEffectiveConfigFromDefaultNetwork(). Combining
them might be difficult so I don't think we need to do that, but I would like to
keep the logic in the same place so that if we add it somewhere else we don't
create a third copy of effectively the same logic.

e.g.
proxy_config = chromeos::proxy_config::GetDefaultProxyConfigDictionary(...);
if (proxy_config)
  return proxy_config;

Also, this way we don't need know the implementation details of NetworkState in
this class, GetDefaultProxyConfigDictionary can grab the default network itself.

[Polina Bondarenko, 2016/09/05 20:08:45]

Moved all logic to ProxyConfigServiceImpl and removed GetProxyConfig at all.

+    return base::MakeUnique<ProxyConfigDictionary>(
+        ProxyConfigDictionary::CreateDirect());
+  }

[stevenjb, 2016/09/02 16:20:55]

nit: I would invert this to make the positive results conditional and the
default an empty config.

[Polina Bondarenko, 2016/09/05 20:08:45]

Done.

+  return proxy_config;
+}
+
+bool GetHttpProxyServer(const ProxyConfigDictionary* proxy_config_dict,
                         std::string* host,
                         int* port) {
   std::string proxy_rules_string;
-  if (!proxy_config_dict.GetProxyServer(&proxy_rules_string))
+  if (!proxy_config_dict->GetProxyServer(&proxy_rules_string))
     return false;
 
   net::ProxyConfig::ProxyRules proxy_rules;
@@ -65,7 +106,8 @@ namespace arc {
 class ArcSettingsServiceImpl
     : public chromeos::system::TimezoneSettings::Observer,
       public device::BluetoothAdapter::Observer,
-      public ArcAuthService::Observer {
+      public ArcAuthService::Observer,
+      public chromeos::NetworkStateHandlerObserver {
  public:
   explicit ArcSettingsServiceImpl(ArcBridgeService* arc_bridge_service);
   ~ArcSettingsServiceImpl() override;
@@ -84,6 +126,9 @@ class ArcSettingsServiceImpl
   // ArcAuthService::Observer:
   void OnInitialStart() override;
 
+  // NetworkStateHandlerObserver:
+  void DefaultNetworkChanged(const chromeos::NetworkState* network) override;
+
  private:
   // Registers to observe changes for Chrome settings we care about.
   void StartObservingSettingsChanges();
@@ -99,7 +144,6 @@ class ArcSettingsServiceImpl
   void SyncInitialSettings() const;
   void SyncFontSize() const;
   void SyncLocale() const;
-  void SyncProxySettings() const;
   void SyncReportingConsent() const;
   void SyncSpokenFeedbackEnabled() const;
   void SyncTimeZone() const;
@@ -124,6 +168,9 @@ class ArcSettingsServiceImpl
   void SendSettingsBroadcast(const std::string& action,
                              const base::DictionaryValue& extras) const;
 
+  // Sends proxy settings broadcast to the delegate.
+  void SendProxySettingsBroadcast(const ProxyConfigDictionary* proxy_config);
+
   // Manages pref observation registration.
   PrefChangeRegistrar registrar_;
 
@@ -133,6 +180,9 @@ class ArcSettingsServiceImpl
 
   scoped_refptr<device::BluetoothAdapter> bluetooth_adapter_;
 
+  // Do not send the same proxy configuration.
+  std::unique_ptr<base::DictionaryValue> last_proxy_extras_;
+
   // WeakPtrFactory to use for callback for getting the bluetooth adapter.
   base::WeakPtrFactory<ArcSettingsServiceImpl> weak_factory_;
 
@@ -168,7 +218,6 @@ void ArcSettingsServiceImpl::StartObservingSettingsChanges() {
   AddPrefToObserve(prefs::kWebKitMinimumFontSize);
   AddPrefToObserve(prefs::kAccessibilitySpokenFeedbackEnabled);
   AddPrefToObserve(prefs::kUse24HourClock);
-  AddPrefToObserve(proxy_config::prefs::kProxy);
   AddPrefToObserve(prefs::kArcBackupRestoreEnabled);
 
   reporting_consent_subscription_ = CrosSettings::Get()->AddSettingsObserver(
@@ -183,6 +232,17 @@ void ArcSettingsServiceImpl::StartObservingSettingsChanges() {
         base::Bind(&ArcSettingsServiceImpl::OnBluetoothAdapterInitialized,
                    weak_factory_.GetWeakPtr()));
   }
+
+  chromeos::NetworkHandler::Get()->network_state_handler()->AddObserver(
+      this, FROM_HERE);
+  const chromeos::NetworkState* network = chromeos::NetworkHandler::Get()
+                                              ->network_state_handler()
+                                              ->DefaultNetwork();
+  if (network) {
+    std::unique_ptr<ProxyConfigDictionary> proxy_config =
+        GetProxyConfig(network);
+    SendProxySettingsBroadcast(proxy_config.get());
+  }

[stevenjb, 2016/09/02 16:20:55]

This could be:
proxy_config = GetProxyConfig();
if (proxy_config)
  SendProxySettingsBroadcast(proxy_config.get());

Which would probably be better, since proxy_config might be based on the prefs,
not the default network.

[Polina Bondarenko, 2016/09/05 20:08:45]

Done.

 }
 
 void ArcSettingsServiceImpl::OnBluetoothAdapterInitialized(
@@ -201,7 +261,6 @@ void ArcSettingsServiceImpl::OnInitialStart() {
 void ArcSettingsServiceImpl::SyncRuntimeSettings() const {
   SyncFontSize();
   SyncLocale();
-  SyncProxySettings();
   SyncReportingConsent();
   SyncSpokenFeedbackEnabled();
   SyncTimeZone();
@@ -225,6 +284,8 @@ void ArcSettingsServiceImpl::StopObservingSettingsChanges() {
   reporting_consent_subscription_.reset();
 
   TimezoneSettings::GetInstance()->RemoveObserver(this);
+  chromeos::NetworkHandler::Get()->network_state_handler()->RemoveObserver(
+      this, FROM_HERE);
 }
 
 void ArcSettingsServiceImpl::AddPrefToObserve(const std::string& pref_name) {
@@ -250,8 +311,6 @@ void ArcSettingsServiceImpl::OnPrefChanged(const std::string& pref_name) const {
     SyncFontSize();
   } else if (pref_name == prefs::kUse24HourClock) {
     SyncUse24HourClock();
-  } else if (pref_name == proxy_config::prefs::kProxy) {
-    SyncProxySettings();
   } else {
     LOG(ERROR) << "Unknown pref changed.";
   }
@@ -348,17 +407,10 @@ void ArcSettingsServiceImpl::SyncUse24HourClock() const {
                         extras);
 }
 
-void ArcSettingsServiceImpl::SyncProxySettings() const {
-  const PrefService::Preference* const pref =
-      registrar_.prefs()->FindPreference(proxy_config::prefs::kProxy);
-  const base::DictionaryValue* proxy_config_value;
-  bool value_exists = pref->GetValue()->GetAsDictionary(&proxy_config_value);
-  DCHECK(value_exists);
-
-  ProxyConfigDictionary proxy_config_dict(proxy_config_value);
-
+void ArcSettingsServiceImpl::SendProxySettingsBroadcast(
+    const ProxyConfigDictionary* proxy_config_dict) {
   ProxyPrefs::ProxyMode mode;
-  if (!proxy_config_dict.GetMode(&mode))
+  if (!proxy_config_dict || !proxy_config_dict->GetMode(&mode))
     mode = ProxyPrefs::MODE_DIRECT;
 
   base::DictionaryValue extras;
@@ -375,7 +427,7 @@ void ArcSettingsServiceImpl::SyncProxySettings() const {
       break;
     case ProxyPrefs::MODE_PAC_SCRIPT: {
       std::string pac_url;
-      if (!proxy_config_dict.GetPacUrl(&pac_url)) {
+      if (!proxy_config_dict->GetPacUrl(&pac_url)) {
         LOG(ERROR) << "No pac URL for pac_script proxy mode.";
         return;
       }
@@ -393,7 +445,7 @@ void ArcSettingsServiceImpl::SyncProxySettings() const {
       extras.SetInteger("port", port);
 
       std::string bypass_list;
-      if (proxy_config_dict.GetBypassList(&bypass_list) &&
+      if (proxy_config_dict->GetBypassList(&bypass_list) &&
           !bypass_list.empty()) {
         extras.SetString("bypassList", bypass_list);
       }
@@ -404,7 +456,11 @@ void ArcSettingsServiceImpl::SyncProxySettings() const {
       return;
   }
 
-  SendSettingsBroadcast("org.chromium.arc.intent_helper.SET_PROXY", extras);
+  if (!last_proxy_extras_ || !last_proxy_extras_->Equals(&extras)) {
+    last_proxy_extras_.reset();
+    last_proxy_extras_ = base::DictionaryValue::From(extras.CreateDeepCopy());
+    SendSettingsBroadcast("org.chromium.arc.intent_helper.SET_PROXY", extras);
+  }
 }
 
 void ArcSettingsServiceImpl::SyncBackupEnabled() const {
@@ -438,6 +494,12 @@ void ArcSettingsServiceImpl::SendSettingsBroadcast(
   }
 }
 
+void ArcSettingsServiceImpl::DefaultNetworkChanged(
+    const chromeos::NetworkState* network) {
+  std::unique_ptr<ProxyConfigDictionary> proxy_config = GetProxyConfig(network);
+  SendProxySettingsBroadcast(proxy_config.get());

[stevenjb, 2016/09/02 16:20:55]

Do we want to cache the proxy config and only broadcast it if it changes?

Also, we should observe changes to proxy_config::prefs::kProxy also.

[Polina Bondarenko, 2016/09/05 20:08:45]

Yes, added check of the source to reduce the amount of unnecessary calls. 
Fixed, +ONC policy changes.

+}
+
 ArcSettingsService::ArcSettingsService(ArcBridgeService* bridge_service)
     : ArcService(bridge_service) {
   arc_bridge_service()->intent_helper()->AddObserver(this);