NetworkHandler: Add ui_proxy_config_service

chromeos/network/proxy/ui_proxy_config_service.h

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef CHROMEOS_NETWORK_PROXY_UI_PROXY_CONFIG_SERVICE_H_

[michaelpg, 2016/11/02 05:34:51]

what's the deal with the title? what is being added here?

[stevenjb, 2016/11/02 18:03:41]

I don't understand this question.

[michaelpg, 2016/11/02 23:39:09]

sorry, I commented in a better place about changing the CL title but still
published this draft. It's accurate now :-)

 #define CHROMEOS_NETWORK_PROXY_UI_PROXY_CONFIG_SERVICE_H_
 
 #include <string>
 
 #include "base/macros.h"
 #include "chromeos/chromeos_export.h"
 #include "chromeos/network/proxy/ui_proxy_config.h"
+#include "components/prefs/pref_change_registrar.h"
 
 class PrefService;
 
 namespace chromeos {
 
 class NetworkState;
 
-// This class is only accessed from the UI via Profile::GetProxyConfigTracker to
-// allow the user to read and modify the proxy configuration via
-// GetProxyConfig and SetProxyConfig.
-//
-// Before reading/setting a proxy config, a network has to be selected using
-// either SetCurrentNetwork (any remembered network) or
-// MakeActiveNetworkCurrent.
+// This class provides an interface to the UI for getting and setting a proxy
+// configuration. Primarily this class caches the "current" requested config
+// and populates UIProxyConfig for convenient UI consumption.
+// NOTE: This class must be rebuilt when the logged in profile changes.
 class CHROMEOS_EXPORT UIProxyConfigService {
  public:
-  UIProxyConfigService();
+  // |local_state_prefs| must not be null. |logged_in_profile_prefs| can be
+  // null if there is no logged in user, in which case only the local state
+  // (device) prefs will be used. See note above.
+  UIProxyConfigService(PrefService* local_state_prefs,
+                       PrefService* logged_in_profile_prefs);
   ~UIProxyConfigService();
 
-  // After this call, proxy settings are read from |profile_prefs| and
-  // |local_state_prefs|. In case of usage for the sign-in screen,
-  // |profile_prefs| must be NULL because sign-in screen should depend only on
-  // shared settings.
-  void SetPrefs(PrefService* profile_prefs, PrefService* local_state_prefs);
+  // Called when ::proxy_config::prefs change or to force an update.
+  void UpdateFromPrefs(const std::string& guid);
 
-  // Called by UI to set the network with GUID |current_guid| to be
-  // displayed or edited.  Subsequent Set*/Get* methods will use this
-  // network, until this method is called again.
-  void SetCurrentNetworkGuid(const std::string& current_guid);
-
-  void UpdateFromPrefs();
-
-  // Called from UI to retrieve the stored proxy configuration, which is either
-  // the last proxy config of the current network or the one last set by
-  // SetProxyConfig.
-  void GetProxyConfig(UIProxyConfig* config) const;
+  // Called from UI to retrieve the active proxy configuration, using
+  // |network_guid| for the network state if no global proxy configuration
+  // supersedes it.
+  void GetProxyConfig(const std::string& network_guid, UIProxyConfig* config);

[michaelpg, 2016/11/02 05:34:51]

The comment makes sense, but between the naming, and the similarities with
SetProxyConfig, it could be easy for a caller to think this returns the proxy
config specific to the specified network...

Maybe something like GetEffectiveProxyConfig (like how prefs work)?

It's also kind of odd that an object's Get* method can change it, but I guess
it's only changing cached stuff. Still... "PopulateUIProxyConfig"?

Do we use "mutable" in chromium much? would it make sense to make this function
const and just make the "cache" members mutable? is that a thing we do?

[stevenjb, 2016/11/02 18:03:42]

I absolutely agree that this class is a bit of a mess and really needs to go
away, but that will require even more re-factoring.

For now I will try to improve the comment but I don't think changing the name
will make this less confusing.

We tend to avoid mutable. I am personally not a fan. Again, more significant
re-factoring is in order.

 
   // Called from UI to update proxy configuration for different modes. Stores
-  // and persists |config| to shill for the current network.
-  void SetProxyConfig(const UIProxyConfig& config);
+  // and persists |config| to shill for the network matching |network_guid|.
+  void SetProxyConfig(const std::string& network_guid,
+                      const UIProxyConfig& config);
 
  private:
   // Determines effective proxy config based on prefs from config tracker,
   // |network| and if user is using shared proxies.  The effective config is
   // stored in |current_ui_config_| but not activated on network stack, and
   // hence, not picked up by observers.
   void DetermineEffectiveConfig(const NetworkState& network);
 
-  // GUID of network whose proxy configuration is being displayed or
-  // edited via UI.
+  void OnPreferenceChanged(const std::string& pref_name);
+
+  // GUID of network used for current_ui_config_.
   std::string current_ui_network_guid_;
 
   // Proxy configuration of |current_ui_network_|.

[michaelpg, 2016/11/02 05:34:51]

|current_ui_network_guid_| (or just normal words)

[stevenjb, 2016/11/02 18:03:41]

Done.

   UIProxyConfig current_ui_config_;
 
-  // Not owned.
-  PrefService* profile_prefs_;
+  PrefService* logged_in_profile_prefs_;  // unowned

[michaelpg, 2016/11/02 05:34:51]

s/profile/user while you're here?

[stevenjb, 2016/11/02 18:03:41]

Well, they are associated with Profile, not User, so I think I will leave this
as-is.

+  PrefChangeRegistrar logged_in_profile_registrar_;
 
-  // Not owned.
-  PrefService* local_state_prefs_;
+  PrefService* local_state_prefs_;  // unowned
+  PrefChangeRegistrar local_state_registrar_;
 
   DISALLOW_COPY_AND_ASSIGN(UIProxyConfigService);
 };
 
 }  // namespace chromeos
 
 #endif  // CHROMEOS_NETWORK_PROXY_UI_PROXY_CONFIG_SERVICE_H_