Migrate ProxyConfigServiceImpl to NetworkStateHandler and NetworkProfileHandler.

Migrate ProxyConfigServiceImpl to NetworkStateHandler and NetworkProfileHandler.

- UI part of ProxyConfigServiceImpl is moved to UIProxyConfig and UIProxyConfigService.
- ProxyConfigServiceImpl uses NetworkStateHandler instead of NetworkLibrary except for the legacy migration of device proxy settings.
- UI code continues to use NetworkLibrary for now.

BUG=234982
TBR=willchan@chromium.org (for trivial but reviewed change in net/proxy/proxy_config.*)

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

[pneubeck (no reviews), 2013-05-07 09:03:35]

The tests I manually ran mostly worked. The only error that I found seems not to
be caused by this change (property changes to ProxyConfig didn't trigger
notifications, investigating further).

@Mattias, please review in particular *proxy_config*

@Steven, please review at least chromeos/ but better also
proxy_config_service_impl*

Thanks!

[Mattias Nissler (ping if slow), 2013-05-08 12:46:36]

https://codereview.chromium.org/14846004/diff/13001/chrome/browser/chromeos/p...
File chrome/browser/chromeos/proxy_config_service_impl.cc (right):

https://codereview.chromium.org/14846004/diff/13001/chrome/browser/chromeos/p...
chrome/browser/chromeos/proxy_config_service_impl.cc:255:
ProxyConfigServiceImpl::ProxyConfig::ToPrefProxyConfig() {
nit: indent by 4

https://codereview.chromium.org/14846004/diff/13001/chrome/browser/chromeos/p...
chrome/browser/chromeos/proxy_config_service_impl.cc:701: LOG_IF(ERROR, network
&& network->path() != default_network_)
Should this be a DLOG_IF?

https://codereview.chromium.org/14846004/diff/13001/chrome/browser/chromeos/p...
File chrome/browser/chromeos/proxy_config_service_impl.h (right):

https://codereview.chromium.org/14846004/diff/13001/chrome/browser/chromeos/p...
chrome/browser/chromeos/proxy_config_service_impl.h:157: // Called from UI to
make the current default network the one to be displayed
Why the change in wording to default? The previous name was more obvious to me.

https://codereview.chromium.org/14846004/diff/13001/chromeos/network/network_...
File chromeos/network/network_state.cc (right):

https://codereview.chromium.org/14846004/diff/13001/chromeos/network/network_...
chromeos/network/network_state.cc:89:
ConvertListValueToStringVector(*dns_servers, &dns_servers_);
Hm, this returns true now also for broken dns_servers lists. Is that
intentional?

[pneubeck (no reviews), 2013-05-08 18:32:14]

https://codereview.chromium.org/14846004/diff/13001/chrome/browser/chromeos/p...
File chrome/browser/chromeos/proxy_config_service_impl.cc (right):

https://codereview.chromium.org/14846004/diff/13001/chrome/browser/chromeos/p...
chrome/browser/chromeos/proxy_config_service_impl.cc:255:
ProxyConfigServiceImpl::ProxyConfig::ToPrefProxyConfig() {
On 2013/05/08 12:46:36, Mattias Nissler wrote:
> nit: indent by 4

Done.

https://codereview.chromium.org/14846004/diff/13001/chrome/browser/chromeos/p...
chrome/browser/chromeos/proxy_config_service_impl.cc:701: LOG_IF(ERROR, network
&& network->path() != default_network_)
On 2013/05/08 12:46:36, Mattias Nissler wrote:
> Should this be a DLOG_IF?

Hm. It would be interesting to know about this case even if we don't expect it.
For the case, that you just didn't like the length of it, I reduced the
verbosity.

However, I can also make it a DLOG_IF if you think it's more appropriate.

https://codereview.chromium.org/14846004/diff/13001/chrome/browser/chromeos/p...
File chrome/browser/chromeos/proxy_config_service_impl.h (right):

https://codereview.chromium.org/14846004/diff/13001/chrome/browser/chromeos/p...
chrome/browser/chromeos/proxy_config_service_impl.h:157: // Called from UI to
make the current default network the one to be displayed
On 2013/05/08 12:46:36, Mattias Nissler wrote:
> Why the change in wording to default? The previous name was more obvious to
me.

The wording was changed in many places. I think the rational was: there can be
several networks active, like Ethernet+VPN. But only one has the "default
route".

I don't really care, as this code should go away better sooner than later...

https://codereview.chromium.org/14846004/diff/13001/chromeos/network/network_...
File chromeos/network/network_state.cc (right):

https://codereview.chromium.org/14846004/diff/13001/chromeos/network/network_...
chromeos/network/network_state.cc:89:
ConvertListValueToStringVector(*dns_servers, &dns_servers_);
On 2013/05/08 12:46:36, Mattias Nissler wrote:
> Hm, this returns true now also for broken dns_servers lists. Is that
> intentional?

The description for this function in the header changed as well.
As this function is used, it should return true, once it potentially changed the
state.

So, because of dns_servers_.clear() return true is necessary.

[stevenjb, 2013-05-08 20:32:09]

https://codereview.chromium.org/14846004/diff/22039/chrome/browser/chromeos/p...
File chrome/browser/chromeos/proxy_config_service_impl.cc (right):

https://codereview.chromium.org/14846004/diff/22039/chrome/browser/chromeos/p...
chrome/browser/chromeos/proxy_config_service_impl.cc:116: }
Mixing NetworkLibrary and NetworkStateHandler is prone to all sorts of bad edge
cases; we should avoid it.

Here it looks like we only need unique_id and ui_data.

So far I have avoided adding UIData to NetworkState, but I don't have any
particular objections to adding it if it makes code like this easier (otherwise
we would have to request it asynchronously which might be a pain).

The unique_id logic could easily be added to
NetworkState::InitialPropertiesReceived(). I suspect that we can just store it
in guid_, but we can add another member if that is not the case (or we are
unsure).

https://codereview.chromium.org/14846004/diff/22039/chrome/browser/chromeos/p...
chrome/browser/chromeos/proxy_config_service_impl.cc:471: current_ui_network_);
I don't see anything here that can't use NetworkState.

https://codereview.chromium.org/14846004/diff/22039/chrome/browser/chromeos/p...
chrome/browser/chromeos/proxy_config_service_impl.cc:656:
network->SetProxyConfig(value);
We should switch to using NetworkConfigurationHandler for this at the same time.

https://codereview.chromium.org/14846004/diff/22039/chrome/browser/ui/webui/c...
File chrome/browser/ui/webui/chromeos/login/network_state_informer.cc (right):

https://codereview.chromium.org/14846004/diff/22039/chrome/browser/ui/webui/c...
chrome/browser/ui/webui/chromeos/login/network_state_informer.cc:216:
DCHECK(network_state);
Ugh. We need to convert this class too asap, mixing the two is not ideal.

It is possible for NetworkStateHandler and NetworkLibrary to be out of sync. We
should return false in that case instead of a DCHECK.

https://codereview.chromium.org/14846004/diff/22039/chrome/browser/ui/webui/c...
chrome/browser/ui/webui/chromeos/login/network_state_informer.cc:219: if
(!ProxyConfigServiceImpl::ParseProxyConfig(*network_state,
I find this mixing of * and & confusing. It seems like ParseProxyConfig() should
take a const*.

That said, this looks like the only place outside of ProxyConfigServiceImpl that
we call ParseProxyConfig. Maybe pass network->service_path() for now instead of
doing the NetworkState lookup here, then make ParseProxyConfig a public wrapper
that does the lookup and calls a local method.

[pneubeck (no reviews), 2013-05-10 10:58:52]

https://codereview.chromium.org/14846004/diff/22039/chrome/browser/chromeos/p...
File chrome/browser/chromeos/proxy_config_service_impl.cc (right):

https://codereview.chromium.org/14846004/diff/22039/chrome/browser/chromeos/p...
chrome/browser/chromeos/proxy_config_service_impl.cc:116: }
On 2013/05/08 20:32:09, stevenjb (chromium) wrote:
> Mixing NetworkLibrary and NetworkStateHandler is prone to all sorts of bad
edge
> cases; we should avoid it.
> 
> Here it looks like we only need unique_id and ui_data.
> 
> So far I have avoided adding UIData to NetworkState, but I don't have any
> particular objections to adding it if it makes code like this easier
(otherwise
> we would have to request it asynchronously which might be a pain).
> 
> The unique_id logic could easily be added to
> NetworkState::InitialPropertiesReceived(). I suspect that we can just store it
> in guid_, but we can add another member if that is not the case (or we are
> unsure).

I removed the dependency of NetworkPropertyUIData on NetworkUIData. And
unique_id in the context of policy is actually guid.

We cannot remove the network_library->FindOncForNetwork() call for now, as
policies are still applied by NetworkLibrary.

I could switch over to the new code for policy application, but that would
require to replace all calls to network_library->FindOncForNetwork in the old UI
code by some call to ManagedNetworkConfigHandler. It's not much to change, but
maybe we don't want to have that dependency?

I thought about that step previously. But I'm not sure if that's worth it.

https://codereview.chromium.org/14846004/diff/22039/chrome/browser/chromeos/p...
chrome/browser/chromeos/proxy_config_service_impl.cc:471: current_ui_network_);
On 2013/05/08 20:32:09, stevenjb (chromium) wrote:
> I don't see anything here that can't use NetworkState.

I tried to not change the UI part too much.

https://codereview.chromium.org/14846004/diff/22039/chrome/browser/chromeos/p...
chrome/browser/chromeos/proxy_config_service_impl.cc:656:
network->SetProxyConfig(value);
On 2013/05/08 20:32:09, stevenjb (chromium) wrote:
> We should switch to using NetworkConfigurationHandler for this at the same
time.

What means "at the same time"?
The problem with using the new config handler is, that NetworkLibrary will
likely get out of sync, and I don't know what the implications would be.

A possible solution coulde be:
Move storage of ProxyConfig out of Shill and use the config handler in parallel
to NetworkLibrary only for:
  - ProxyConfig
  - policy application (see larger comment above)
Remove all references to ProxyConfig from NetworkLibrary.

https://codereview.chromium.org/14846004/diff/22039/chrome/browser/ui/webui/c...
File chrome/browser/ui/webui/chromeos/login/network_state_informer.cc (right):

https://codereview.chromium.org/14846004/diff/22039/chrome/browser/ui/webui/c...
chrome/browser/ui/webui/chromeos/login/network_state_informer.cc:216:
DCHECK(network_state);
On 2013/05/08 20:32:09, stevenjb (chromium) wrote:
> Ugh. We need to convert this class too asap, mixing the two is not ideal.
> 
> It is possible for NetworkStateHandler and NetworkLibrary to be out of sync.
We
> should return false in that case instead of a DCHECK.

Gaurav does that already in another CL.
I'll rebase onto his.

https://codereview.chromium.org/14846004/diff/22039/chrome/browser/ui/webui/c...
chrome/browser/ui/webui/chromeos/login/network_state_informer.cc:219: if
(!ProxyConfigServiceImpl::ParseProxyConfig(*network_state,
> That said, this looks like the only place outside of ProxyConfigServiceImpl
that
> we call ParseProxyConfig. Maybe pass network->service_path() for now instead
of
> doing the NetworkState lookup here, then make ParseProxyConfig a public
wrapper
> that does the lookup and calls a local method.

Actually, I'd even like to get rid of this weird dependency entirely. Did that
now.

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

https://codereview.chromium.org/14846004/diff/13001/chrome/browser/chromeos/p...
File chrome/browser/chromeos/proxy_config_service_impl.cc (right):

https://codereview.chromium.org/14846004/diff/13001/chrome/browser/chromeos/p...
chrome/browser/chromeos/proxy_config_service_impl.cc:701: LOG_IF(ERROR, network
&& network->path() != default_network_)
On 2013/05/08 18:32:14, pneubeck wrote:
> On 2013/05/08 12:46:36, Mattias Nissler wrote:
> > Should this be a DLOG_IF?
> 
> Hm. It would be interesting to know about this case even if we don't expect
it.
> For the case, that you just didn't like the length of it, I reduced the
> verbosity.

If this is in the "program logic error that cannot happen and is not
recoverable" bucket, it should be a CHECK. If it is "program logic error that
really cannot happen because of invariants", then you should put a DCHECK. If
this is "expected to happen in practice and we can continue safely" then the
LOG_IF is appropriate.

> 
> However, I can also make it a DLOG_IF if you think it's more appropriate.

https://codereview.chromium.org/14846004/diff/13001/chrome/browser/chromeos/p...
File chrome/browser/chromeos/proxy_config_service_impl.h (right):

https://codereview.chromium.org/14846004/diff/13001/chrome/browser/chromeos/p...
chrome/browser/chromeos/proxy_config_service_impl.h:157: // Called from UI to
make the current default network the one to be displayed
On 2013/05/08 18:32:14, pneubeck wrote:
> On 2013/05/08 12:46:36, Mattias Nissler wrote:
> > Why the change in wording to default? The previous name was more obvious to
> me.
> 
> The wording was changed in many places. I think the rational was: there can be
> several networks active, like Ethernet+VPN. But only one has the "default
> route".
> 
> I don't really care, as this code should go away better sooner than later...

If this is consistent with the networking code, I'm fine.

[pneubeck (no reviews), 2013-05-10 13:30:39]

Please wait with further reviews. I ran into issues with the unittest because of
this interleaved UI code.

I'll separate the UI code at first and ping you when it's reviewable again.

[pneubeck (no reviews), 2013-05-15 15:52:43]

PTAL

First tests are positive. I'll do a few others tomorrow.

[stevenjb, 2013-05-15 17:44:14]

I like the separation a lot better, thanks.
Just a few non-nit suggestions/questions/requests.

https://codereview.chromium.org/14846004/diff/73001/chrome/browser/chromeos/p...
File chrome/browser/chromeos/proxy_config_service_impl.cc (right):

https://codereview.chromium.org/14846004/diff/73001/chrome/browser/chromeos/p...
chrome/browser/chromeos/proxy_config_service_impl.cc:72:
NetworkStateHandler::Get()->RemoveObserver(this);
Test NetworkStateHandler::IsInitialized() here. (This will change to
NetworkHandler::IsInitialized(), but will serve as a reminder).
Shutdown order for any code associated with the UI can be fragile, and I'd
prefer to avoid an uninteresting crash.

https://codereview.chromium.org/14846004/diff/73001/chrome/browser/chromeos/p...
chrome/browser/chromeos/proxy_config_service_impl.cc:115: VLOG(1) <<
"DefaultNetworkChanged to " << new_network_path << ".";
nit:  '' around path (which might be empty)

https://codereview.chromium.org/14846004/diff/73001/chrome/browser/chromeos/p...
chrome/browser/chromeos/proxy_config_service_impl.cc:135:
!network_state->proxy_config().empty() ||
This is somewhat non-obvious. Can you add a comment explaining that this method
only copies device proxy configs to network services with empty proxy configs?

https://codereview.chromium.org/14846004/diff/73001/chrome/browser/chromeos/p...
chrome/browser/chromeos/proxy_config_service_impl.cc:148:
network->SetProxyConfig(device_config_);
Can we move this to NetworkLibrary, i.e.
CrosLibrary::Get()->GetNetworkLibrary()->SetProxyConfigForNetwork(network_state->path());

The reason is that I would like to make the Network class and its derived
classes private to NetworkLibrary soon, as part of the transition away from
NetworkLibrary.

https://codereview.chromium.org/14846004/diff/73001/chrome/browser/chromeos/u...
File chrome/browser/chromeos/ui_proxy_config.cc (right):

https://codereview.chromium.org/14846004/diff/73001/chrome/browser/chromeos/u...
chrome/browser/chromeos/ui_proxy_config.cc:19: user_modifiable(true) {}
nit: } on separate line outside of class declaration

https://codereview.chromium.org/14846004/diff/73001/chrome/browser/chromeos/u...
chrome/browser/chromeos/ui_proxy_config.cc:56: switch (rules.type) {
nit: if/else is generally more readable than complex switch statements like
this.

https://codereview.chromium.org/14846004/diff/73001/chrome/browser/chromeos/u...
chrome/browser/chromeos/ui_proxy_config.cc:103: switch (mode) {
nit: if/else would be better here also imho. Otherwise, it is acceptable (and I
think more readable) to omit {} for cases that do not require it.

https://codereview.chromium.org/14846004/diff/73001/chrome/browser/chromeos/u...
chrome/browser/chromeos/ui_proxy_config.cc:168: switch (rules.type) {
nit: if/else

https://codereview.chromium.org/14846004/diff/73001/chrome/browser/chromeos/u...
File chrome/browser/chromeos/ui_proxy_config.h (right):

https://codereview.chromium.org/14846004/diff/73001/chrome/browser/chromeos/u...
chrome/browser/chromeos/ui_proxy_config.h:25: // In contrary to other platforms
which simply use the systems' UI to allow
nit: s/In contrary to/Contrary to (or 'In contrast to')/

https://codereview.chromium.org/14846004/diff/73001/chrome/browser/chromeos/u...
chrome/browser/chromeos/ui_proxy_config.h:28: // net::ProxyConfig does not
suffice.  So we create an augmented analog to
nit: s/suffice/supply

https://codereview.chromium.org/14846004/diff/73001/chrome/browser/chromeos/u...
chrome/browser/chromeos/ui_proxy_config.h:37: struct UIProxyConfig {
This is complex enough that a class might be better, unless we're implicitly or
regularly copying it?

https://codereview.chromium.org/14846004/diff/73001/chrome/browser/chromeos/u...
chrome/browser/chromeos/ui_proxy_config.h:99: ProxyPrefs::ConfigState state;
Without the trailing _, these look like locals in the code. We should either:
* Make this a class
* name these something like config_mode and config_state
* move the complex methods to either a helper class or global namespaced
functions

https://codereview.chromium.org/14846004/diff/73001/chrome/browser/chromeos/u...
File chrome/browser/chromeos/ui_proxy_config_service.cc (right):

https://codereview.chromium.org/14846004/diff/73001/chrome/browser/chromeos/u...
chrome/browser/chromeos/ui_proxy_config_service.cc:24: // Helper comparator
functor for the find_if call in |findIfEqual|
I don't think "Equals" is generic enough for a helper, but a Callback specific
'CallbackEquals in base/callback_helpers.h seems useful (but should probably be
in a separate CL).
Here I would make this Callback specific (+ see next comment).

https://codereview.chromium.org/14846004/diff/73001/chrome/browser/chromeos/u...
chrome/browser/chromeos/ui_proxy_config_service.cc:44: }
This doesn't seem to add much clarity than just inlining below:
std::find_if(callbacks_.begin(), callbacks_.end(), CallbackEquals);

https://codereview.chromium.org/14846004/diff/73001/chrome/browser/chromeos/u...
chrome/browser/chromeos/ui_proxy_config_service.cc:47: switch (mode) {
FWIW, this (imho) is a good use of 'switch'.

https://codereview.chromium.org/14846004/diff/73001/chrome/browser/chromeos/u...
chrome/browser/chromeos/ui_proxy_config_service.cc:241: LOG(ERROR) <<
"PrefPrecedes";
Debugging? VLOG? (Here and below)

https://codereview.chromium.org/14846004/diff/73001/chrome/browser/chromeos/u...
File chrome/browser/chromeos/ui_proxy_config_service.h (right):

https://codereview.chromium.org/14846004/diff/73001/chrome/browser/chromeos/u...
chrome/browser/chromeos/ui_proxy_config_service.h:35: // changed by the UI.
Maybe describe when these are called?

https://codereview.chromium.org/14846004/diff/73001/chrome/browser/chromeos/u...
chrome/browser/chromeos/ui_proxy_config_service.h:46: void
MakeActiveNetworkCurrent();
We should use 'DefaultNetwork' here.

https://codereview.chromium.org/14846004/diff/73001/chrome/browser/ui/webui/c...
File chrome/browser/ui/webui/chromeos/login/network_state_informer.cc (right):

https://codereview.chromium.org/14846004/diff/73001/chrome/browser/ui/webui/c...
chrome/browser/ui/webui/chromeos/login/network_state_informer.cc:215:
DCHECK(network_state);
nit: Highly unlikely but not impossible, e.g. if this happens as a side effect
of NetworkLibrary receiving a Manger update before NetworkStateHandler processes
the same update. I'd rather handle this with a warning (i.e. return false) than
crash.

https://codereview.chromium.org/14846004/diff/73001/chromeos/network/managed_...
File chromeos/network/managed_state.h (right):

https://codereview.chromium.org/14846004/diff/73001/chromeos/network/managed_...
chromeos/network/managed_state.h:43: // Called by NetworkStateHandler when a
property was received. The return
I think that the original 'Returns false if' clause was more precise, how about:

Called by NetworkStateHandler when Shill indicates that a property has changed.
Returns false if the property value is unchanged for any of the following
reasons:
* The property was not recognized.
* The property was not parsed successfully.
* The |value| is equal to the existing property value; this avoids unnecessary
callbacks and logging. (Note: complex properties may not be compared, so this
may return true even though the value did not actually change).

https://codereview.chromium.org/14846004/diff/73001/chromeos/network/network_...
File chromeos/network/network_state.h (right):

https://codereview.chromium.org/14846004/diff/73001/chromeos/network/network_...
chromeos/network/network_state.h:102: base::DictionaryValue proxy_config_;
This will conflict with gaurav's CL:
https://codereview.chromium.org/14134007/diff/76001/chromeos/network/network_...

Also I don't like storing properties here that do not actually reflect the Shill
state, even temporarily. Plus I don't see where these are set?

Would it be possible to store these in a separate dictionary instead?

[pneubeck (no reviews), 2013-05-15 17:49:47]

Please review
  Yuri:  login related changes
  Nikita:
       core_chromeos_options_handler.cc
       proxy_settings_ui.cc
  jar@: net related changes
  Pawel: testing_automation_provider_chromeos.cc

Thanks!

[Paweł Hajdan Jr., 2013-05-15 18:07:33]

automation LGTM

[pneubeck (no reviews), 2013-05-15 20:24:48]

Addressed Steven's comments.

https://codereview.chromium.org/14846004/diff/73001/chrome/browser/chromeos/p...
File chrome/browser/chromeos/proxy_config_service_impl.cc (right):

https://codereview.chromium.org/14846004/diff/73001/chrome/browser/chromeos/p...
chrome/browser/chromeos/proxy_config_service_impl.cc:72:
NetworkStateHandler::Get()->RemoveObserver(this);
On 2013/05/15 17:44:14, stevenjb (chromium) wrote:
> Test NetworkStateHandler::IsInitialized() here. (This will change to
> NetworkHandler::IsInitialized(), but will serve as a reminder).
> Shutdown order for any code associated with the UI can be fragile, and I'd
> prefer to avoid an uninteresting crash.

Done.

https://codereview.chromium.org/14846004/diff/73001/chrome/browser/chromeos/p...
chrome/browser/chromeos/proxy_config_service_impl.cc:115: VLOG(1) <<
"DefaultNetworkChanged to " << new_network_path << ".";
On 2013/05/15 17:44:14, stevenjb (chromium) wrote:
> nit:  '' around path (which might be empty)

Done.

https://codereview.chromium.org/14846004/diff/73001/chrome/browser/chromeos/p...
chrome/browser/chromeos/proxy_config_service_impl.cc:135:
!network_state->proxy_config().empty() ||
On 2013/05/15 17:44:14, stevenjb (chromium) wrote:
> This is somewhat non-obvious. Can you add a comment explaining that this
method
> only copies device proxy configs to network services with empty proxy configs?

Done.

https://codereview.chromium.org/14846004/diff/73001/chrome/browser/chromeos/p...
chrome/browser/chromeos/proxy_config_service_impl.cc:148:
network->SetProxyConfig(device_config_);
On 2013/05/15 17:44:14, stevenjb (chromium) wrote:
> Can we move this to NetworkLibrary, i.e.
>
CrosLibrary::Get()->GetNetworkLibrary()->SetProxyConfigForNetwork(network_state->path());
> 
> The reason is that I would like to make the Network class and its derived
> classes private to NetworkLibrary soon, as part of the transition away from
> NetworkLibrary.

Actually I had liked to remove this migration code all together. See my reply
outside the code.

https://codereview.chromium.org/14846004/diff/73001/chrome/browser/chromeos/u...
File chrome/browser/chromeos/ui_proxy_config.cc (right):

https://codereview.chromium.org/14846004/diff/73001/chrome/browser/chromeos/u...
chrome/browser/chromeos/ui_proxy_config.cc:19: user_modifiable(true) {}
On 2013/05/15 17:44:14, stevenjb (chromium) wrote:
> nit: } on separate line outside of class declaration

Done.

https://codereview.chromium.org/14846004/diff/73001/chrome/browser/chromeos/u...
File chrome/browser/chromeos/ui_proxy_config.h (right):

https://codereview.chromium.org/14846004/diff/73001/chrome/browser/chromeos/u...
chrome/browser/chromeos/ui_proxy_config.h:25: // In contrary to other platforms
which simply use the systems' UI to allow
On 2013/05/15 17:44:14, stevenjb (chromium) wrote:
> nit: s/In contrary to/Contrary to (or 'In contrast to')/

Done.

https://codereview.chromium.org/14846004/diff/73001/chrome/browser/chromeos/u...
chrome/browser/chromeos/ui_proxy_config.h:28: // net::ProxyConfig does not
suffice.  So we create an augmented analog to
On 2013/05/15 17:44:14, stevenjb (chromium) wrote:
> nit: s/suffice/supply

Done.

https://codereview.chromium.org/14846004/diff/73001/chrome/browser/chromeos/u...
chrome/browser/chromeos/ui_proxy_config.h:37: struct UIProxyConfig {
On 2013/05/15 17:44:14, stevenjb (chromium) wrote:
> This is complex enough that a class might be better, unless we're implicitly
or
> regularly copying it?

Yes it's regularly copied.

I totally agree (also to the next comment) that this should be changed.

However, I'd like to postpone that change for a separate CL. There is still huge
potential to simplify the UI related part. And this struct is one of the first
candidates to be extremely reduced.
And this CL is already to huge.

https://codereview.chromium.org/14846004/diff/73001/chrome/browser/chromeos/u...
chrome/browser/chromeos/ui_proxy_config.h:99: ProxyPrefs::ConfigState state;
On 2013/05/15 17:44:14, stevenjb (chromium) wrote:
> Without the trailing _, these look like locals in the code. We should either:
> * Make this a class
> * name these something like config_mode and config_state
> * move the complex methods to either a helper class or global namespaced
> functions

I totally agree. So far this is only moved code with only the necessary changes.
See previous comment.

https://codereview.chromium.org/14846004/diff/73001/chrome/browser/chromeos/u...
File chrome/browser/chromeos/ui_proxy_config_service.cc (right):

https://codereview.chromium.org/14846004/diff/73001/chrome/browser/chromeos/u...
chrome/browser/chromeos/ui_proxy_config_service.cc:24: // Helper comparator
functor for the find_if call in |findIfEqual|
I don't like the complexity here about Callbacks at all (could be also
Observers) because the Callback mechanism is only used in exactly one place for
a bad hack (CoreChromeOSOptionsHandler).

I removed the question from the comment (which wasn't mine).

On 2013/05/15 17:44:14, stevenjb (chromium) wrote:
> I don't think "Equals" is generic enough for a helper, but a Callback specific
> 'CallbackEquals in base/callback_helpers.h seems useful (but should probably
be
> in a separate CL).
> Here I would make this Callback specific (+ see next comment).

https://codereview.chromium.org/14846004/diff/73001/chrome/browser/chromeos/u...
chrome/browser/chromeos/ui_proxy_config_service.cc:44: }
On 2013/05/15 17:44:14, stevenjb (chromium) wrote:
> This doesn't seem to add much clarity than just inlining below:
> std::find_if(callbacks_.begin(), callbacks_.end(), CallbackEquals);

Done.

https://codereview.chromium.org/14846004/diff/73001/chrome/browser/chromeos/u...
chrome/browser/chromeos/ui_proxy_config_service.cc:241: LOG(ERROR) <<
"PrefPrecedes";
On 2013/05/15 17:44:14, stevenjb (chromium) wrote:
> Debugging? VLOG? (Here and below)

Oops. Yes, debugging.

https://codereview.chromium.org/14846004/diff/73001/chrome/browser/chromeos/u...
File chrome/browser/chromeos/ui_proxy_config_service.h (right):

https://codereview.chromium.org/14846004/diff/73001/chrome/browser/chromeos/u...
chrome/browser/chromeos/ui_proxy_config_service.h:35: // changed by the UI.
On 2013/05/15 17:44:14, stevenjb (chromium) wrote:
> Maybe describe when these are called?
Done. Again, hopefully I can remove this callback stuff altogether in the next
CL.

https://codereview.chromium.org/14846004/diff/73001/chrome/browser/chromeos/u...
chrome/browser/chromeos/ui_proxy_config_service.h:46: void
MakeActiveNetworkCurrent();
On 2013/05/15 17:44:14, stevenjb (chromium) wrote:
> We should use 'DefaultNetwork' here.
I didn't touch it as it's still using network library.

https://codereview.chromium.org/14846004/diff/73001/chrome/browser/ui/webui/c...
File chrome/browser/ui/webui/chromeos/login/network_state_informer.cc (right):

https://codereview.chromium.org/14846004/diff/73001/chrome/browser/ui/webui/c...
chrome/browser/ui/webui/chromeos/login/network_state_informer.cc:215:
DCHECK(network_state);
On 2013/05/15 17:44:14, stevenjb (chromium) wrote:
> nit: Highly unlikely but not impossible, e.g. if this happens as a side effect
> of NetworkLibrary receiving a Manger update before NetworkStateHandler
processes
> the same update. I'd rather handle this with a warning (i.e. return false)
than
> crash.

This is still waiting on Gaurav's commit. I should all be NetworkStateHandler.

https://codereview.chromium.org/14846004/diff/73001/chromeos/network/managed_...
File chromeos/network/managed_state.h (right):

https://codereview.chromium.org/14846004/diff/73001/chromeos/network/managed_...
chromeos/network/managed_state.h:43: // Called by NetworkStateHandler when a
property was received. The return
On 2013/05/15 17:44:14, stevenjb (chromium) wrote:
> I think that the original 'Returns false if' clause was more precise, how
about:
> 
> Called by NetworkStateHandler when Shill indicates that a property has
changed.
> Returns false if the property value is unchanged for any of the following
> reasons:
> * The property was not recognized.
> * The property was not parsed successfully.
> * The |value| is equal to the existing property value; this avoids unnecessary
> callbacks and logging. (Note: complex properties may not be compared, so this
> may return true even though the value did not actually change).

Please double check my change to the return statements in the .cc file at first.

Maybe, you swapped the direction of the implication by mistake.
What I want to ensure with the comment is:

If the state changed then all observers are notified. That is the central
responsibility of NetworkStateHandler.

IMO, the details that you mention are unimportant to the one caller of this
function, which is only interested in state changes to trigger notifications; it
cannot even differentiate the parse errors.

Please, check the adapted comment which should cover all suggestions :-)

https://codereview.chromium.org/14846004/diff/73001/chromeos/network/network_...
File chromeos/network/network_state.h (right):

https://codereview.chromium.org/14846004/diff/73001/chromeos/network/network_...
chromeos/network/network_state.h:102: base::DictionaryValue proxy_config_;
On 2013/05/15 17:44:14, stevenjb (chromium) wrote:
> This will conflict with gaurav's CL:
>
https://codereview.chromium.org/14134007/diff/76001/chromeos/network/network_...
I know. I planned to overwrite his string.

> Also I don't like storing properties here that do not actually reflect the
Shill
> state, even temporarily. Plus I don't see where these are set?
> 
> Would it be possible to store these in a separate dictionary instead?

Sorry, the change in the .cc got lost during merge. PTAL.
Also consider, that at least the proxy_config, but also likely the onc_source,
will get removed again once the other handlers are ready.

[Nikita (slow), 2013-05-16 06:57:06]

On 2013/05/15 17:49:47, pneubeck wrote:
>   Nikita:
>        core_chromeos_options_handler.cc
>        proxy_settings_ui.cc

these 2 files lgtm

[stevenjb, 2013-05-16 17:04:34]

lgtm with added TODO comments.

https://codereview.chromium.org/14846004/diff/73001/chrome/browser/chromeos/p...
File chrome/browser/chromeos/proxy_config_service_impl.cc (right):

https://codereview.chromium.org/14846004/diff/73001/chrome/browser/chromeos/p...
chrome/browser/chromeos/proxy_config_service_impl.cc:148:
network->SetProxyConfig(device_config_);
On 2013/05/15 20:24:48, pneubeck wrote:
> On 2013/05/15 17:44:14, stevenjb (chromium) wrote:
> > Can we move this to NetworkLibrary, i.e.
> >
>
CrosLibrary::Get()->GetNetworkLibrary()->SetProxyConfigForNetwork(network_state->path());
> > 
> > The reason is that I would like to make the Network class and its derived
> > classes private to NetworkLibrary soon, as part of the transition away from
> > NetworkLibrary.
> 
> Actually I had liked to remove this migration code all together. See my reply
> outside the code.
If we eliminate
If we eliminate this, or plan to eliminate this, that is fine, but we should add
a comment to that effect (with an issue#) if we keep it for now.

https://codereview.chromium.org/14846004/diff/73001/chrome/browser/chromeos/u...
File chrome/browser/chromeos/ui_proxy_config.h (right):

https://codereview.chromium.org/14846004/diff/73001/chrome/browser/chromeos/u...
chrome/browser/chromeos/ui_proxy_config.h:37: struct UIProxyConfig {
On 2013/05/15 20:24:48, pneubeck wrote:
> On 2013/05/15 17:44:14, stevenjb (chromium) wrote:
> > This is complex enough that a class might be better, unless we're implicitly
> or
> > regularly copying it?
> 
> Yes it's regularly copied.
> 
> I totally agree (also to the next comment) that this should be changed.
> 
> However, I'd like to postpone that change for a separate CL. There is still
huge
> potential to simplify the UI related part. And this struct is one of the first
> candidates to be extremely reduced.
> And this CL is already to huge.
OK, that's fine, as long as we do follow up; the C++ code is a bit challenging
to follow.

https://codereview.chromium.org/14846004/diff/99001/chromeos/network/network_...
File chromeos/network/network_state.h (right):

https://codereview.chromium.org/14846004/diff/99001/chromeos/network/network_...
chromeos/network/network_state.h:101: int priority_;
Please add a TODO:remove comment for these with an issue#. I'm OK with storing
them for now for convenience, but we do want to make some effort to keep this
class from turning into another chromeos::Network... :)
(Out of curiosity, I checked, and sizeof(chromeos::WifiNetwork) = 504, and
sizeof(chromeos::NetworkState) = 200, so we're still a lot smaller, but getting
up there).

[pneubeck (no reviews), 2013-05-21 12:00:34]

@jar, ping.

[pneubeck (no reviews), 2013-05-21 12:06:41]

Hi Dominic,
PTAL at chrome/browser/prefs/proxy_prefs.*

Thanks!

[pneubeck (no reviews), 2013-05-21 12:06:59]

Hi Dominic,
PTAL at chrome/browser/prefs/proxy_prefs.*

Thanks!

[battre, 2013-05-21 12:15:42]

LGTM @ chrome/browser/prefs/proxy_prefs.*

[pneubeck (no reviews), 2013-05-23 08:43:40]

On 2013/05/21 12:15:42, battre wrote:
> LGTM @ chrome/browser/prefs/proxy_prefs.*

Dominic, could you please also take a look at
chrome/browser/net/pref_proxy_config_tracker_impl.*

and
net/proxy/proxy_config.*

jar@ doesn't react and the latter change seems trivial enough for a TBR after
someone having reviewed it.

[battre, 2013-05-23 08:50:14]

LGTM at chrome/browser/net/pref_proxy_config_tracker_impl.* and
net/proxy/proxy_config.*

https://codereview.chromium.org/14846004/diff/155003/chrome/browser/net/pref_...
File chrome/browser/net/pref_proxy_config_tracker_impl.cc (right):

https://codereview.chromium.org/14846004/diff/155003/chrome/browser/net/pref_...
chrome/browser/net/pref_proxy_config_tracker_impl.cc:217: const PrefService*
pref_service, net::ProxyConfig* config) {
nit: parameters in separate lines

[commit-bot: I haz the power, 2013-05-23 08:58:03]

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

[jar (doing other things), 2013-05-24 01:12:51]

Adding Eric to reviewer list, for the chrome/browser/net files relating to proxy
config calls.

[eroman, 2013-05-25 02:03:53]

lgtm for browser/net/* and net/*

[commit-bot: I haz the power, 2013-05-25 07:51:15]

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

[commit-bot: I haz the power, 2013-05-25 08:20:10]

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

[pneubeck (no reviews), 2013-05-27 13:36:50]

rlp@
please take a look at the last patch fixing profile_manager_unittest.cc.

Thanks!

[rpetterson, 2013-05-28 17:08:38]

LGTM assuming all the tests pass  . . .

https://codereview.chromium.org/14846004/diff/195001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_manager_unittest.cc (left):

https://codereview.chromium.org/14846004/diff/195001/chrome/browser/profiles/...
chrome/browser/profiles/profile_manager_unittest.cc:146: IOThread io_thread_;
I assume you've tested that these test still pass? My recollection is that the
io_thread_ was necessary for some profile keyed services to work, but this may
no longer be the case with recent changes to the browser_context_keyed_service
framework.

[commit-bot: I haz the power, 2013-06-04 12:52:38]

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

[commit-bot: I haz the power, 2013-06-04 19:29:37]

Change committed as 204016