NetworkHandler: Add ui_proxy_config_service

chromeos::NetworkHandler: Add ui_proxy_config_service

This introduces NetworkHandler::InitializePrefServices which
passes PrefService for local (device) state and the logged in user. This will get called twice, once on startup
and once when the primary user logs in.

NetworkHandler::InitializePrefServices currently only initializes
an owned instance of UIProxyConfigService, which can then be used
by any code utilizing NetworkHandler. Currently this is the (old)
options UI, but it will soon include the networkingPrivate
extension API code in order to support the new MD Settings UI.

BUG=658015
Committed: https://crrev.com/5205952ad0b7f9df469a620b3bea96f81817015e
Cr-Commit-Position: refs/heads/master@{#429528}

[stevenjb, 2016-11-01 22:29:44]

Description was changed from

==========
NetworkHandler: Add ui_proxy_network_config_service

This introduces NetworkHandler::InitializePrefServices which
passes PrefService for local (device) state and the primary
(logged in) user. This will get called twice, once on startup
and once when the primary user logs in.

NetworkHandler::InitializePrefServices currently only initializes
an owned instance of UIProxyConfigService, which can then be used
by any code utilizing NetworkHandler. Currently this is the (old)
options UI, but it will soon include the networkingPrivate
extension API code in order to support the new MD Settings UI.

BUG=658015
==========

to

==========
chromeos::NetworkHandler: Add ui_proxy_network_config_service

This introduces NetworkHandler::InitializePrefServices which
passes PrefService for local (device) state and the logged in user. This will
get called twice, once on startup
and once when the primary user logs in.

NetworkHandler::InitializePrefServices currently only initializes
an owned instance of UIProxyConfigService, which can then be used
by any code utilizing NetworkHandler. Currently this is the (old)
options UI, but it will soon include the networkingPrivate
extension API code in order to support the new MD Settings UI.

BUG=658015
==========

[stevenjb, 2016-11-01 22:29:50]

stevenjb@chromium.org changed reviewers:
+ emaxx@chromium.org, jamescook@chromium.org, michaelpg@chromium.org,
mnissler@chromium.org

[stevenjb, 2016-11-01 22:29:51]

jamescook@, emaxx@ for re-factoring review.

michaelpg@ FYI and optional review.

mnissler@ FYI and in case you have any concerns about this change.

[James Cook, 2016-11-01 23:02:04]

Just FYI - we'll need to come up with some way in mustash to deal with
//chromeos/network using prefs (probably a pref service) but that's a problem
for another day.

https://codereview.chromium.org/2446893008/diff/80001/chrome/browser/chromeos...
File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right):

https://codereview.chromium.org/2446893008/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/chrome_browser_main_chromeos.cc:695:
network_pref_state_observer_.reset(new NetworkPrefStateObserver);
nit: Does this have to be here, or can it live up near the other networking
stuff (portal detection)?

nit: MakeUnique for consistency with rest of file

https://codereview.chromium.org/2446893008/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/chrome_browser_main_chromeos.cc:806:
network_pref_state_observer_.reset();
Could this be reset a few lines earlier, like above 802, so that the services
unroll in the opposite order to their creation?

https://codereview.chromium.org/2446893008/diff/80001/chrome/browser/chromeos...
File chrome/browser/chromeos/net/network_pref_state_observer.h (right):

https://codereview.chromium.org/2446893008/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/net/network_pref_state_observer.h:19: 
nit: no blank line

https://codereview.chromium.org/2446893008/diff/80001/chrome/browser/chromeos...
File chrome/browser/chromeos/net/network_pref_state_observer_unittest.cc
(right):

https://codereview.chromium.org/2446893008/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/net/network_pref_state_observer_unittest.cc:39:
namespace test {
optional nit: I don't usually put the tests themselves in namespace test, just
test utilities.

https://codereview.chromium.org/2446893008/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/net/network_pref_state_observer_unittest.cc:68:
base::RunLoop().RunUntilIdle();
It reads kinda weird to do this RunUntilIdle here. Is it needed for
DBusThreadManager::Initialize()? If so, move it up there.

https://codereview.chromium.org/2446893008/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/net/network_pref_state_observer_unittest.cc:96:
protected:
not needed. or make the above helper methods public.

https://codereview.chromium.org/2446893008/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/net/network_pref_state_observer_unittest.cc:141: }
Thanks for writing a nice test.

https://codereview.chromium.org/2446893008/diff/80001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.h
(right):

https://codereview.chromium.org/2446893008/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.h:9:
#include <memory>
Why memory but not <string>?

https://codereview.chromium.org/2446893008/diff/80001/chromeos/network/networ...
File chromeos/network/network_handler.cc (right):

https://codereview.chromium.org/2446893008/diff/80001/chromeos/network/networ...
chromeos/network/network_handler.cc:119: new UIProxyConfigService(profile_prefs,
device_prefs));
It's confusing to me that InitializePrefServices() and UIProxyConfigService()
take their parameters in the opposite order. They should either match the order,
or have an InitParams, or have a setter method.

https://codereview.chromium.org/2446893008/diff/80001/chromeos/network/proxy/...
File chromeos/network/proxy/ui_proxy_config_service.cc (right):

https://codereview.chromium.org/2446893008/diff/80001/chromeos/network/proxy/...
chromeos/network/proxy/ui_proxy_config_service.cc:71:
local_state_prefs_(local_state_prefs) {
nit: DCHECK(local_state_prefs) if it cannot be null

https://codereview.chromium.org/2446893008/diff/80001/chromeos/network/proxy/...
File chromeos/network/proxy/ui_proxy_config_service.h (right):

https://codereview.chromium.org/2446893008/diff/80001/chromeos/network/proxy/...
chromeos/network/proxy/ui_proxy_config_service.h:21: // This class provdes an
interface to the UI for getting and setting a proxy
nit: provides

https://codereview.chromium.org/2446893008/diff/80001/chromeos/network/proxy/...
chromeos/network/proxy/ui_proxy_config_service.h:28:
UIProxyConfigService(PrefService* logged_in_profile_prefs,
nit: document what it means if one (or both) are null

[stevenjb, 2016-11-01 23:55:11]

Agreed that we need to decide what to do about prefs in mustash soon. We use
PrefService elsewhere in src/chromeos (timezone/ and audio/) and in many
components (not sure if any such components are used in ash though).

https://codereview.chromium.org/2446893008/diff/80001/chrome/browser/chromeos...
File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right):

https://codereview.chromium.org/2446893008/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/chrome_browser_main_chromeos.cc:695:
network_pref_state_observer_.reset(new NetworkPrefStateObserver);
On 2016/11/01 23:02:03, James Cook wrote:
> nit: Does this have to be here, or can it live up near the other networking
> stuff (portal detection)?
> 
> nit: MakeUnique for consistency with rest of file

Moved near portal detection which should be fine.
Changed to MakeUnique.

https://codereview.chromium.org/2446893008/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/chrome_browser_main_chromeos.cc:806:
network_pref_state_observer_.reset();
On 2016/11/01 23:02:03, James Cook wrote:
> Could this be reset a few lines earlier, like above 802, so that the services
> unroll in the opposite order to their creation?

Moved.

https://codereview.chromium.org/2446893008/diff/80001/chrome/browser/chromeos...
File chrome/browser/chromeos/net/network_pref_state_observer.h (right):

https://codereview.chromium.org/2446893008/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/net/network_pref_state_observer.h:19: 
On 2016/11/01 23:02:03, James Cook wrote:
> nit: no blank line

Done.

https://codereview.chromium.org/2446893008/diff/80001/chrome/browser/chromeos...
File chrome/browser/chromeos/net/network_pref_state_observer_unittest.cc
(right):

https://codereview.chromium.org/2446893008/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/net/network_pref_state_observer_unittest.cc:39:
namespace test {
On 2016/11/01 23:02:04, James Cook wrote:
> optional nit: I don't usually put the tests themselves in namespace test, just
> test utilities.

Done.

https://codereview.chromium.org/2446893008/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/net/network_pref_state_observer_unittest.cc:68:
base::RunLoop().RunUntilIdle();
On 2016/11/01 23:02:04, James Cook wrote:
> It reads kinda weird to do this RunUntilIdle here. Is it needed for
> DBusThreadManager::Initialize()? If so, move it up there.

Removed.

https://codereview.chromium.org/2446893008/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/net/network_pref_state_observer_unittest.cc:96:
protected:
On 2016/11/01 23:02:04, James Cook wrote:
> not needed. or make the above helper methods public.

Done.

https://codereview.chromium.org/2446893008/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/net/network_pref_state_observer_unittest.cc:141: }
On 2016/11/01 23:02:04, James Cook wrote:
> Thanks for writing a nice test.

Acknowledged.

https://codereview.chromium.org/2446893008/diff/80001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.h
(right):

https://codereview.chromium.org/2446893008/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.h:9:
#include <memory>
On 2016/11/01 23:02:04, James Cook wrote:
> Why memory but not <string>?

removed

https://codereview.chromium.org/2446893008/diff/80001/chromeos/network/networ...
File chromeos/network/network_handler.cc (right):

https://codereview.chromium.org/2446893008/diff/80001/chromeos/network/networ...
chromeos/network/network_handler.cc:119: new UIProxyConfigService(profile_prefs,
device_prefs));
On 2016/11/01 23:02:04, James Cook wrote:
> It's confusing to me that InitializePrefServices() and UIProxyConfigService()
> take their parameters in the opposite order. They should either match the
order,
> or have an InitParams, or have a setter method.

Agreed it's confusing. Was trying to minimize churn, and at one point there was
more than one instance... fixed.

https://codereview.chromium.org/2446893008/diff/80001/chromeos/network/proxy/...
File chromeos/network/proxy/ui_proxy_config_service.cc (right):

https://codereview.chromium.org/2446893008/diff/80001/chromeos/network/proxy/...
chromeos/network/proxy/ui_proxy_config_service.cc:71:
local_state_prefs_(local_state_prefs) {
On 2016/11/01 23:02:04, James Cook wrote:
> nit: DCHECK(local_state_prefs) if it cannot be null

Done.

https://codereview.chromium.org/2446893008/diff/80001/chromeos/network/proxy/...
File chromeos/network/proxy/ui_proxy_config_service.h (right):

https://codereview.chromium.org/2446893008/diff/80001/chromeos/network/proxy/...
chromeos/network/proxy/ui_proxy_config_service.h:21: // This class provdes an
interface to the UI for getting and setting a proxy
On 2016/11/01 23:02:04, James Cook wrote:
> nit: provides

Done.

https://codereview.chromium.org/2446893008/diff/80001/chromeos/network/proxy/...
chromeos/network/proxy/ui_proxy_config_service.h:28:
UIProxyConfigService(PrefService* logged_in_profile_prefs,
On 2016/11/01 23:02:04, James Cook wrote:
> nit: document what it means if one (or both) are null

Done.

[James Cook, 2016-11-02 01:57:33]

LGTM. I noticed a couple cleanup things -- sorry I missed them last time around.

https://codereview.chromium.org/2446893008/diff/100001/chrome/browser/chromeo...
File chrome/browser/chromeos/net/network_pref_state_observer_unittest.cc
(right):

https://codereview.chromium.org/2446893008/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/net/network_pref_state_observer_unittest.cc:19: #include
"chromeos/dbus/shill_service_client.h"
Do you still need this include?

https://codereview.chromium.org/2446893008/diff/100001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.h
(right):

https://codereview.chromium.org/2446893008/diff/100001/chrome/browser/ui/webu...
chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.h:10:
#include "base/compiler_specific.h"
While you're in the file, not needed, but probably needs macros.h

https://codereview.chromium.org/2446893008/diff/100001/chrome/browser/ui/webu...
chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.h:71:
std::string network_guid_;
#include <string> for new usage of string

https://codereview.chromium.org/2446893008/diff/100001/chrome/browser/ui/webu...
chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.h:72: };
DISALLOW_COPY_AND_ASSIGN while you're here

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

I don't know how much of this class is new or recently changed. So if the part
I'm confused by is legacy code, don't worry about my review too much (at least
for the purposes of landing your changes).

I think overall, it would be nice if UIProxyConfigService *looked* more like
what it *is*. Its purpose is to populate a config object from proxy settings,
and update proxy settings from a config object. (It's a proxy for proxy settings
UI.) It doesn't need a complicated interface. It just so happens to rely on a
cache for the first part, and observes prefs in order to rebuild that cache, but
these are implementation details.

But we expose those cache internals, externally. See UpdateFromPrefs, the
comment about rebuilding this class when the profile changes, etc. This makes it
look a lot messier than "a proxy for settings UI". We're actually an object that
needs a lot of attention, considering all the caller wants is to calculate the
effective config or set a new config (IIUC). Seeing this for the first time,
even with the comments it took me a while to understand that although this looks
more complicated, it's really just meant as a getter/setter that uses a cache.

If anything, I would want to be able to observe UIProxyConfigService, not the
other way around (having to tell UIProxyConfigService when proxy prefs change in
a way that might update the UI).

https://codereview.chromium.org/2446893008/diff/100001/chromeos/network/netwo...
File chromeos/network/network_handler.h (right):

https://codereview.chromium.org/2446893008/diff/100001/chromeos/network/netwo...
chromeos/network/network_handler.h:77: UIProxyConfigService*
ui_proxy_config_service();
update CL title/description

https://codereview.chromium.org/2446893008/diff/100001/chromeos/network/proxy...
File chromeos/network/proxy/ui_proxy_config_service.cc (right):

https://codereview.chromium.org/2446893008/diff/100001/chromeos/network/proxy...
chromeos/network/proxy/ui_proxy_config_service.cc:69: PrefService*
logged_in_profile_prefs)
nit: Can you order the pref service args to match the order used by
proxy_config::GetProxyConfigForNetwork? Or if it's common to pass both
PrefServices around, use whatever the most common ordering is.

https://codereview.chromium.org/2446893008/diff/100001/chromeos/network/proxy...
chromeos/network/proxy/ui_proxy_config_service.cc:70: :
logged_in_profile_prefs_(logged_in_profile_prefs),
order according to args

https://codereview.chromium.org/2446893008/diff/100001/chromeos/network/proxy...
chromeos/network/proxy/ui_proxy_config_service.cc:125: *config =
current_ui_config_;
Is this a copy? So current_ui_config_ is just used as a cache to copy from?

https://codereview.chromium.org/2446893008/diff/100001/chromeos/network/proxy...
File chromeos/network/proxy/ui_proxy_config_service.h (right):

https://codereview.chromium.org/2446893008/diff/100001/chromeos/network/proxy...
chromeos/network/proxy/ui_proxy_config_service.h:5: #ifndef
CHROMEOS_NETWORK_PROXY_UI_PROXY_CONFIG_SERVICE_H_
what's the deal with the title? what is being added here?

https://codereview.chromium.org/2446893008/diff/100001/chromeos/network/proxy...
chromeos/network/proxy/ui_proxy_config_service.h:40: void GetProxyConfig(const
std::string& network_guid, UIProxyConfig* config);
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?

https://codereview.chromium.org/2446893008/diff/100001/chromeos/network/proxy...
chromeos/network/proxy/ui_proxy_config_service.h:59: // Proxy configuration of
|current_ui_network_|.
|current_ui_network_guid_| (or just normal words)

https://codereview.chromium.org/2446893008/diff/100001/chromeos/network/proxy...
chromeos/network/proxy/ui_proxy_config_service.h:62: PrefService*
logged_in_profile_prefs_;  // unowned
s/profile/user while you're here?

[michaelpg, 2016-11-02 05:44:06]

What needs to be done for mustash? PrefService is, well, already a service,
right? Is it a question of how the PrefService is created/owned? Currently this
is done by the browser process for local state, and the individual profiles for
user prefs. AppShell similarly creates the PrefService for local state, and
we're deciding how to handle the user pref store. So it seems pretty well
componentized already; the PrefService can be created with the PrefStore types
the owner is going to need.

Is it about separating that ownership from the "browser process"? Or creating a
mojo endpoint?

Sorry for the interrogation, and we can move this elsewhere, I'm just trying to
wrap my head around what's changing for mustash and at the same time
investigating prefs for AppShell.

On 2016/11/01 23:55:11, stevenjb wrote:
> Agreed that we need to decide what to do about prefs in mustash soon. We use
> PrefService elsewhere in src/chromeos (timezone/ and audio/) and in many
> components (not sure if any such components are used in ash though).

On 2016/11/01 23:02:04, James Cook wrote:
> Just FYI - we'll need to come up with some way in mustash to deal with
> //chromeos/network using prefs (probably a pref service) but that's a problem
> for another day.

[stevenjb, 2016-11-02 17:21:09]

Description was changed from

==========
chromeos::NetworkHandler: Add ui_proxy_network_config_service

This introduces NetworkHandler::InitializePrefServices which
passes PrefService for local (device) state and the logged in user. This will
get called twice, once on startup
and once when the primary user logs in.

NetworkHandler::InitializePrefServices currently only initializes
an owned instance of UIProxyConfigService, which can then be used
by any code utilizing NetworkHandler. Currently this is the (old)
options UI, but it will soon include the networkingPrivate
extension API code in order to support the new MD Settings UI.

BUG=658015
==========

to

==========
chromeos::NetworkHandler: Add ui_proxy_config_service

This introduces NetworkHandler::InitializePrefServices which
passes PrefService for local (device) state and the logged in user. This will
get called twice, once on startup
and once when the primary user logs in.

NetworkHandler::InitializePrefServices currently only initializes
an owned instance of UIProxyConfigService, which can then be used
by any code utilizing NetworkHandler. Currently this is the (old)
options UI, but it will soon include the networkingPrivate
extension API code in order to support the new MD Settings UI.

BUG=658015
==========

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

PTAL

michaelpg@ - Thanks for taking the time to look at this closely. The proxy
config code has been around for a very long time, has evolved through two
Settings re-factors, and is complicated in subtle ways. I have tried to do an
incremental re-factor here, keeping the risk of breakage to a minimum. In the
process I have improved my understanding of what the code does so hopefully now
I will be able to successfully clean this up once we abandon the old options
code. In the meanwhile I am hoping that this CL will allow us to leverage the
same code that the old options UI is using in the networkingPrivate extension
API so that the new Settings code can use it (see next CL).

https://codereview.chromium.org/2446893008/diff/100001/chrome/browser/chromeo...
File chrome/browser/chromeos/net/network_pref_state_observer_unittest.cc
(right):

https://codereview.chromium.org/2446893008/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/net/network_pref_state_observer_unittest.cc:19: #include
"chromeos/dbus/shill_service_client.h"
On 2016/11/02 01:57:33, James Cook wrote:
> Do you still need this include?

Good catch. Removed.

https://codereview.chromium.org/2446893008/diff/100001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.h
(right):

https://codereview.chromium.org/2446893008/diff/100001/chrome/browser/ui/webu...
chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.h:10:
#include "base/compiler_specific.h"
On 2016/11/02 01:57:33, James Cook wrote:
> While you're in the file, not needed, but probably needs macros.h

Done.

https://codereview.chromium.org/2446893008/diff/100001/chrome/browser/ui/webu...
chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.h:71:
std::string network_guid_;
On 2016/11/02 01:57:33, James Cook wrote:
> #include <string> for new usage of string

Done.

https://codereview.chromium.org/2446893008/diff/100001/chrome/browser/ui/webu...
chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.h:72: };
On 2016/11/02 01:57:33, James Cook wrote:
> DISALLOW_COPY_AND_ASSIGN while you're here

Done.

https://codereview.chromium.org/2446893008/diff/100001/chromeos/network/netwo...
File chromeos/network/network_handler.h (right):

https://codereview.chromium.org/2446893008/diff/100001/chromeos/network/netwo...
chromeos/network/network_handler.h:77: UIProxyConfigService*
ui_proxy_config_service();
On 2016/11/02 05:34:51, michaelpg wrote:
> update CL title/description

Done.

https://codereview.chromium.org/2446893008/diff/100001/chromeos/network/proxy...
File chromeos/network/proxy/ui_proxy_config_service.cc (right):

https://codereview.chromium.org/2446893008/diff/100001/chromeos/network/proxy...
chromeos/network/proxy/ui_proxy_config_service.cc:69: PrefService*
logged_in_profile_prefs)
On 2016/11/02 05:34:51, michaelpg wrote:
> nit: Can you order the pref service args to match the order used by
> proxy_config::GetProxyConfigForNetwork? Or if it's common to pass both
> PrefServices around, use whatever the most common ordering is.

Argh. I tried to change some code to local, user since local is always set, but
there's a bunch of other code that is user, local and I don't want to try to
change all of it, so I will make all of this code user, local. Sigh.

https://codereview.chromium.org/2446893008/diff/100001/chromeos/network/proxy...
chromeos/network/proxy/ui_proxy_config_service.cc:70: :
logged_in_profile_prefs_(logged_in_profile_prefs),
On 2016/11/02 05:34:51, michaelpg wrote:
> order according to args

Done.

https://codereview.chromium.org/2446893008/diff/100001/chromeos/network/proxy...
chromeos/network/proxy/ui_proxy_config_service.cc:125: *config =
current_ui_config_;
On 2016/11/02 05:34:51, michaelpg wrote:
> Is this a copy? So current_ui_config_ is just used as a cache to copy from?

Correct. The old options code will call into this once for each individual proxy
property (there are 15, see proxy_cros_settings_parser.h). The new code will set
the entire dictionary at which point we should be able to abandon this class
entirely...

https://codereview.chromium.org/2446893008/diff/100001/chromeos/network/proxy...
File chromeos/network/proxy/ui_proxy_config_service.h (right):

https://codereview.chromium.org/2446893008/diff/100001/chromeos/network/proxy...
chromeos/network/proxy/ui_proxy_config_service.h:5: #ifndef
CHROMEOS_NETWORK_PROXY_UI_PROXY_CONFIG_SERVICE_H_
On 2016/11/02 05:34:51, michaelpg wrote:
> what's the deal with the title? what is being added here?

I don't understand this question.

https://codereview.chromium.org/2446893008/diff/100001/chromeos/network/proxy...
chromeos/network/proxy/ui_proxy_config_service.h:40: void GetProxyConfig(const
std::string& network_guid, UIProxyConfig* config);
On 2016/11/02 05:34:51, michaelpg wrote:
> 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?

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.

https://codereview.chromium.org/2446893008/diff/100001/chromeos/network/proxy...
chromeos/network/proxy/ui_proxy_config_service.h:59: // Proxy configuration of
|current_ui_network_|.
On 2016/11/02 05:34:51, michaelpg wrote:
> |current_ui_network_guid_| (or just normal words)

Done.

https://codereview.chromium.org/2446893008/diff/100001/chromeos/network/proxy...
chromeos/network/proxy/ui_proxy_config_service.h:62: PrefService*
logged_in_profile_prefs_;  // unowned
On 2016/11/02 05:34:51, michaelpg wrote:
> s/profile/user while you're here?

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

[stevenjb, 2016-11-02 18:10:17]

Note: In PS#8 I reduced some churn that I introduced while gaining a better
understanding of the code, specifically some variable renaming in
UIProxyConfigService.

[stevenjb, 2016-11-02 18:10:31]

The CQ bit was checked by stevenjb@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-02 18:10:57]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[emaxx, 2016-11-02 18:18:05]

I'm not familiar with the changed code pieces, so only did a high-level review.

LGTM with a few nits.

https://codereview.chromium.org/2446893008/diff/140001/chrome/browser/chromeo...
File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right):

https://codereview.chromium.org/2446893008/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/chrome_browser_main_chromeos.cc:656: // Initialize a
observer to update NetworkHandler's pref based services.
nit: s/a observer/an observer/

https://codereview.chromium.org/2446893008/diff/140001/chrome/browser/chromeo...
File chrome/browser/chromeos/net/network_pref_state_observer.cc (right):

https://codereview.chromium.org/2446893008/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/net/network_pref_state_observer.cc:16: // Initialize
NetworkHandlwer with device prefs only.
nit: s/NetworkHandlwer/NetworkHandler/

https://codereview.chromium.org/2446893008/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/net/network_pref_state_observer.cc:20:
chrome::NOTIFICATION_LOGIN_USER_PROFILE_PREPARED,
Isn't the usage of NotificationType discouraged in the new code?
I don't know, though, whether there's now a cleaner way to observe this event.

https://codereview.chromium.org/2446893008/diff/140001/chrome/browser/chromeo...
File chrome/browser/chromeos/net/network_pref_state_observer.h (right):

https://codereview.chromium.org/2446893008/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/net/network_pref_state_observer.h:26: const
content::NotificationSource& source,
nit: #include "content/public/browser/notification_source.h"

https://codereview.chromium.org/2446893008/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/net/network_pref_state_observer.h:27: const
content::NotificationDetails& details) override;
nit: #include "content/public/browser/notification_details.h"

https://codereview.chromium.org/2446893008/diff/140001/chromeos/network/proxy...
File chromeos/network/proxy/ui_proxy_config_service.h (right):

https://codereview.chromium.org/2446893008/diff/140001/chromeos/network/proxy...
chromeos/network/proxy/ui_proxy_config_service.h:24: // NOTE: This class must be
rebuilt when the logged in profile changes.
nit: Not sure, but probably the following would sound a bit better:
"The instance of this class must be re-created when ..."

[commit-bot: I haz the power, 2016-11-02 19:22:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-02 19:22:05]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[stevenjb, 2016-11-02 19:47:34]

https://codereview.chromium.org/2446893008/diff/140001/chrome/browser/chromeo...
File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right):

https://codereview.chromium.org/2446893008/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/chrome_browser_main_chromeos.cc:656: // Initialize a
observer to update NetworkHandler's pref based services.
On 2016/11/02 18:18:04, emaxx wrote:
> nit: s/a observer/an observer/

Done.

https://codereview.chromium.org/2446893008/diff/140001/chrome/browser/chromeo...
File chrome/browser/chromeos/net/network_pref_state_observer.cc (right):

https://codereview.chromium.org/2446893008/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/net/network_pref_state_observer.cc:16: // Initialize
NetworkHandlwer with device prefs only.
On 2016/11/02 18:18:05, emaxx wrote:
> nit: s/NetworkHandlwer/NetworkHandler/

Done.

https://codereview.chromium.org/2446893008/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/net/network_pref_state_observer.cc:20:
chrome::NOTIFICATION_LOGIN_USER_PROFILE_PREPARED,
On 2016/11/02 18:18:05, emaxx wrote:
> Isn't the usage of NotificationType discouraged in the new code?
> I don't know, though, whether there's now a cleaner way to observe this event.

I am not aware of any alternative, and we rely on this pattern elsewhere.
ExistingUserController::OnProfilePrepared (which normally gets called when
NOTIFICATION_LOGIN_USER_PROFILE_PREPARED is sent) does not trigger any events
and ExistingUserController does not have any observers.

https://codereview.chromium.org/2446893008/diff/140001/chrome/browser/chromeo...
File chrome/browser/chromeos/net/network_pref_state_observer.h (right):

https://codereview.chromium.org/2446893008/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/net/network_pref_state_observer.h:26: const
content::NotificationSource& source,
On 2016/11/02 18:18:05, emaxx wrote:
> nit: #include "content/public/browser/notification_source.h"

Not actually necessary. This is overriding a NotificationObserver method,
therefore NotificationSource must already be forward declared.

https://codereview.chromium.org/2446893008/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/net/network_pref_state_observer.h:27: const
content::NotificationDetails& details) override;
On 2016/11/02 18:18:05, emaxx wrote:
> nit: #include "content/public/browser/notification_details.h"

Ditto.

https://codereview.chromium.org/2446893008/diff/140001/chromeos/network/proxy...
File chromeos/network/proxy/ui_proxy_config_service.h (right):

https://codereview.chromium.org/2446893008/diff/140001/chromeos/network/proxy...
chromeos/network/proxy/ui_proxy_config_service.h:24: // NOTE: This class must be
rebuilt when the logged in profile changes.
On 2016/11/02 18:18:05, emaxx wrote:
> nit: Not sure, but probably the following would sound a bit better:
> "The instance of this class must be re-created when ..."

I think either is fine, leaving this.

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

lgtm

https://codereview.chromium.org/2446893008/diff/100001/chromeos/network/proxy...
File chromeos/network/proxy/ui_proxy_config_service.h (right):

https://codereview.chromium.org/2446893008/diff/100001/chromeos/network/proxy...
chromeos/network/proxy/ui_proxy_config_service.h:5: #ifndef
CHROMEOS_NETWORK_PROXY_UI_PROXY_CONFIG_SERVICE_H_
On 2016/11/02 18:03:41, stevenjb wrote:
> On 2016/11/02 05:34:51, michaelpg wrote:
> > what's the deal with the title? what is being added here?
> 
> I don't understand this question.

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

[stevenjb, 2016-11-03 00:35:25]

The CQ bit was checked by stevenjb@chromium.org

[stevenjb, 2016-11-03 00:35:26]

The patchset sent to the CQ was uploaded after l-g-t-m from
jamescook@chromium.org, emaxx@chromium.org
Link to the patchset: https://codereview.chromium.org/2446893008/#ps160001
(title: "Add NetworkHandler::ShutdownPrefServices")

[commit-bot: I haz the power, 2016-11-03 00:35:56]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-03 05:17:22]

Description was changed from

==========
chromeos::NetworkHandler: Add ui_proxy_config_service

This introduces NetworkHandler::InitializePrefServices which
passes PrefService for local (device) state and the logged in user. This will
get called twice, once on startup
and once when the primary user logs in.

NetworkHandler::InitializePrefServices currently only initializes
an owned instance of UIProxyConfigService, which can then be used
by any code utilizing NetworkHandler. Currently this is the (old)
options UI, but it will soon include the networkingPrivate
extension API code in order to support the new MD Settings UI.

BUG=658015
==========

to

==========
chromeos::NetworkHandler: Add ui_proxy_config_service

This introduces NetworkHandler::InitializePrefServices which
passes PrefService for local (device) state and the logged in user. This will
get called twice, once on startup
and once when the primary user logs in.

NetworkHandler::InitializePrefServices currently only initializes
an owned instance of UIProxyConfigService, which can then be used
by any code utilizing NetworkHandler. Currently this is the (old)
options UI, but it will soon include the networkingPrivate
extension API code in order to support the new MD Settings UI.

BUG=658015
==========

[commit-bot: I haz the power, 2016-11-03 05:17:23]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-11-03 05:19:07]

Description was changed from

==========
chromeos::NetworkHandler: Add ui_proxy_config_service

This introduces NetworkHandler::InitializePrefServices which
passes PrefService for local (device) state and the logged in user. This will
get called twice, once on startup
and once when the primary user logs in.

NetworkHandler::InitializePrefServices currently only initializes
an owned instance of UIProxyConfigService, which can then be used
by any code utilizing NetworkHandler. Currently this is the (old)
options UI, but it will soon include the networkingPrivate
extension API code in order to support the new MD Settings UI.

BUG=658015
==========

to

==========
chromeos::NetworkHandler: Add ui_proxy_config_service

This introduces NetworkHandler::InitializePrefServices which
passes PrefService for local (device) state and the logged in user. This will
get called twice, once on startup
and once when the primary user logs in.

NetworkHandler::InitializePrefServices currently only initializes
an owned instance of UIProxyConfigService, which can then be used
by any code utilizing NetworkHandler. Currently this is the (old)
options UI, but it will soon include the networkingPrivate
extension API code in order to support the new MD Settings UI.

BUG=658015

Committed: https://crrev.com/5205952ad0b7f9df469a620b3bea96f81817015e
Cr-Commit-Position: refs/heads/master@{#429528}
==========

[commit-bot: I haz the power, 2016-11-03 05:19:10]

Patchset 9 (id:??) landed as
https://crrev.com/5205952ad0b7f9df469a620b3bea96f81817015e
Cr-Commit-Position: refs/heads/master@{#429528}