Adding a new ManagedNetworkConfigurationHandler.

- Adding a new ManagedNetworkConfigurationHandler.
- Adapting the NetworkingPrivate extension API to use the new handler if it's available.
- Changing explicit ownership of NetworkConfigurationHandler to singleton.
- Adding gspencer@ and me as owners.

The new handler will be responsible to
- enforce network policies
- handle shared vs. non-shared network configuration

Compared to the NetworkConfigurationHandler, this handler shouldn't use Shill properties but ONC wherever possible.

TBR=zelidrag@chromium.org (for gypi change)
BUG=157696, 148353
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=184933

[pneubeck (no reviews), 2013-02-09 02:50:40]

@Greg, just FYI. I still have to cleaned up further.

[Greg Spencer (Chromium), 2013-02-19 22:58:26]

https://codereview.chromium.org/12211103/diff/3001/chrome/browser/chromeos/ex...
File chrome/browser/chromeos/extensions/networking_private_api.cc (right):

https://codereview.chromium.org/12211103/diff/3001/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/networking_private_api.cc:162:
onc::network_config::kGUID, service_path);
So, when we switched to ManagedNetworkConfigurationHandler, I wanted to switch
to using GUIDs here, but I don't see a change in the code that would translate
the service path into a GUID before sending it to JS.  Was that going to be
another phase?

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

https://codereview.chromium.org/12211103/diff/3001/chrome/browser/chromeos/ne...
chrome/browser/chromeos/net/managed_network_configuration_handler.cc:130:
DCHECK(guid == newGuid);  // TODO(pneubeck): Handle the other case, too.
Can you say what the "other case" is in the comment?  Is it where guid !=
newGuid? And what's the plan there?

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

https://codereview.chromium.org/12211103/diff/3001/chrome/browser/chromeos/ne...
chrome/browser/chromeos/net/managed_network_configuration_handler.h:99: //
Initially configures a yet unconfigured network with the given user
"a yet" => "an"

[pneubeck (no reviews), 2013-02-22 10:22:31]

https://codereview.chromium.org/12211103/diff/3001/chrome/browser/chromeos/ex...
File chrome/browser/chromeos/extensions/networking_private_api.cc (right):

https://codereview.chromium.org/12211103/diff/3001/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/networking_private_api.cc:162:
onc::network_config::kGUID, service_path);
I thought, when we checked the different calls (create, remove, modify), we
found that using the service_path would be the least pain?
The main "problem" was that the GUID doesn't exist for unconfigured networks.
The ManagedNetworkConfigurationHandler can still obtain the GUID of a configured
network by asking the NetworkStateHandler.

I sent you a document that describes what we discussed two weeks ago. Hopefully
it's understandable :-)

On 2013/02/19 22:58:26, Greg Spencer (Chromium) wrote:
> So, when we switched to ManagedNetworkConfigurationHandler, I wanted to switch
> to using GUIDs here, but I don't see a change in the code that would translate
> the service path into a GUID before sending it to JS.  Was that going to be
> another phase?

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

https://codereview.chromium.org/12211103/diff/3001/chrome/browser/chromeos/ne...
chrome/browser/chromeos/net/managed_network_configuration_handler.cc:130:
DCHECK(guid == newGuid);  // TODO(pneubeck): Handle the other case, too.
On 2013/02/19 22:58:26, Greg Spencer (Chromium) wrote:
> Can you say what the "other case" is in the comment?  Is it where guid !=
> newGuid? And what's the plan there?

Actually, I think, the ManagedNetworkConfigurationHandler alone should manage
the GUIDs of configurations. Thus, there is no other case and the returned GUID,
if any, is ignored.

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

https://codereview.chromium.org/12211103/diff/3001/chrome/browser/chromeos/ne...
chrome/browser/chromeos/net/managed_network_configuration_handler.h:99: //
Initially configures a yet unconfigured network with the given user
On 2013/02/19 22:58:26, Greg Spencer (Chromium) wrote:
> "a yet" => "an"

Done.

https://codereview.chromium.org/12211103/diff/18001/chrome/browser/chromeos/n...
File chrome/browser/chromeos/net/managed_network_configuration_handler.h
(right):

https://codereview.chromium.org/12211103/diff/18001/chrome/browser/chromeos/n...
chrome/browser/chromeos/net/managed_network_configuration_handler.h:36: //
Network configurations are referred to by identifiers. Users of this class
It may be better to actually expose that the identifiers are the service_paths,
so that the ConfigurationHandler and the StateHandler can be used in
combination.

Users of the JS api would still be agnostic about which ID we're providing. 

Thoughts?

[Greg Spencer (Chromium), 2013-02-23 00:11:29]

LGTM

https://codereview.chromium.org/12211103/diff/3001/chrome/browser/chromeos/ex...
File chrome/browser/chromeos/extensions/networking_private_api.cc (right):

https://codereview.chromium.org/12211103/diff/3001/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/networking_private_api.cc:162:
onc::network_config::kGUID, service_path);
On 2013/02/22 10:22:31, pneubeck wrote:
> I thought, when we checked the different calls (create, remove, modify), we
> found that using the service_path would be the least pain?
> The main "problem" was that the GUID doesn't exist for unconfigured networks.
> The ManagedNetworkConfigurationHandler can still obtain the GUID of a
configured
> network by asking the NetworkStateHandler.
> 
> I sent you a document that describes what we discussed two weeks ago.
Hopefully
> it's understandable :-)

Yes, I remember that, but I thought there was one case where it wouldn't work,
but I guess that's where we generate a GUID for the unconfigured networks.  In
any case, your diagram makes sense.  Let's use service path like you have here.

[pneubeck (no reviews), 2013-02-25 14:48:06]

I added us as OWNERS and changed the ManagedNetworkCH class to expose the
service paths (changed the comments and variable names), because its users may
reuse the service path to talk to the NetworkStateHandler.


On 2013/02/23 00:11:29, Greg Spencer (Chromium) wrote:
> LGTM
> 
>
https://codereview.chromium.org/12211103/diff/3001/chrome/browser/chromeos/ex...
> File chrome/browser/chromeos/extensions/networking_private_api.cc (right):
> 
>
https://codereview.chromium.org/12211103/diff/3001/chrome/browser/chromeos/ex...
> chrome/browser/chromeos/extensions/networking_private_api.cc:162:
> onc::network_config::kGUID, service_path);
> On 2013/02/22 10:22:31, pneubeck wrote:
> > I thought, when we checked the different calls (create, remove, modify), we
> > found that using the service_path would be the least pain?
> > The main "problem" was that the GUID doesn't exist for unconfigured
networks.
> > The ManagedNetworkConfigurationHandler can still obtain the GUID of a
> configured
> > network by asking the NetworkStateHandler.
> > 
> > I sent you a document that describes what we discussed two weeks ago.
> Hopefully
> > it's understandable :-)
> 
> Yes, I remember that, but I thought there was one case where it wouldn't work,
> but I guess that's where we generate a GUID for the unconfigured networks.  In
> any case, your diagram makes sense.  Let's use service path like you have
here.

[pneubeck (no reviews), 2013-02-25 14:51:00]

Hi Steven,

please owner review
  chrome/browser/chromeos/
  chromeos/chromeos_switches.*

Most of this CL is again only data forwarding...

Thanks!
Philipp

[pneubeck (no reviews), 2013-02-26 09:25:21]

Hi Steven,

please owner review
  chrome/browser/chromeos/
  chromeos/chromeos_switches.*

Most of this CL is again only data forwarding...

Thanks!
Philipp

[pneubeck (no reviews), 2013-02-26 09:25:41]

Hi Steven,

please owner review
  chrome/browser/chromeos/
  chromeos/chromeos_switches.*

Most of this CL is again only data forwarding...

Thanks!
Philipp

[stevenjb, 2013-02-26 17:31:24]

lgtm with nits

https://codereview.chromium.org/12211103/diff/20006/chrome/browser/chromeos/n...
File chrome/browser/chromeos/net/managed_network_configuration_handler.cc
(right):

https://codereview.chromium.org/12211103/diff/20006/chrome/browser/chromeos/n...
chrome/browser/chromeos/net/managed_network_configuration_handler.cc:47: const
network_handler::ErrorCallback& error_callback) {
Can we add a network_event_log event here?

https://codereview.chromium.org/12211103/diff/20006/chrome/browser/chromeos/n...
chrome/browser/chromeos/net/managed_network_configuration_handler.cc:97: //
TODO(pneubeck): Merging!
Clarify?

https://codereview.chromium.org/12211103/diff/20006/chrome/browser/chromeos/n...
chrome/browser/chromeos/net/managed_network_configuration_handler.cc:126: //
TODO(pneubeck): Merging!
ditto. I assume you mean 'merge properties'?

https://codereview.chromium.org/12211103/diff/20006/chrome/browser/chromeos/n...
chrome/browser/chromeos/net/managed_network_configuration_handler.cc:176: //
TODO(pneubeck): Merging!
ditto

https://codereview.chromium.org/12211103/diff/20006/chrome/browser/chromeos/n...
File chrome/browser/chromeos/net/managed_network_configuration_handler.h
(right):

https://codereview.chromium.org/12211103/diff/20006/chrome/browser/chromeos/n...
chrome/browser/chromeos/net/managed_network_configuration_handler.h:96: 
nit: one blank line

[gauravsh, 2013-02-26 21:25:03]

FYI

https://codereview.chromium.org/12211103/diff/20006/chrome/browser/chromeos/n...
File chrome/browser/chromeos/net/managed_network_configuration_handler.cc
(right):

https://codereview.chromium.org/12211103/diff/20006/chrome/browser/chromeos/n...
chrome/browser/chromeos/net/managed_network_configuration_handler.cc:1: //
Copyright (c) 2012 The Chromium Authors. All rights reserved.
NIT: 2013

https://codereview.chromium.org/12211103/diff/20006/chrome/browser/chromeos/n...
chrome/browser/chromeos/net/managed_network_configuration_handler.cc:35: const
char kErrorMessage[] = "errorMessage";
You should add a comment that these are error names and message meant to be used
with error callbacks.

https://codereview.chromium.org/12211103/diff/20006/chrome/browser/chromeos/n...
File chrome/browser/chromeos/net/managed_network_configuration_handler.h
(right):

https://codereview.chromium.org/12211103/diff/20006/chrome/browser/chromeos/n...
chrome/browser/chromeos/net/managed_network_configuration_handler.h:1: //
Copyright (c) 2012 The Chromium Authors. All rights reserved.
NIT: 2013

https://codereview.chromium.org/12211103/diff/20006/chrome/browser/chromeos/n...
chrome/browser/chromeos/net/managed_network_configuration_handler.h:33: // This
is a singleton and it's lifetime is managed by by the Chrome startup
NIT: s/it's/its/, s/by by/by/

https://codereview.chromium.org/12211103/diff/20006/chrome/chrome_browser_chr...
File chrome/chrome_browser_chromeos.gypi (right):

https://codereview.chromium.org/12211103/diff/20006/chrome/chrome_browser_chr...
chrome/chrome_browser_chromeos.gypi:524:
'browser/chromeos/net/managed_network_configuration_handler.cc',
Can this be a part of chromeos/network? Looking at the #includes, doesn't look
like the ManagedNetworkConfigurationHandler depends on anything under
chrome/browser/chromeos.

[pneubeck (no reviews), 2013-02-27 09:49:25]

https://codereview.chromium.org/12211103/diff/20006/chrome/browser/chromeos/n...
File chrome/browser/chromeos/net/managed_network_configuration_handler.cc
(right):

https://codereview.chromium.org/12211103/diff/20006/chrome/browser/chromeos/n...
chrome/browser/chromeos/net/managed_network_configuration_handler.cc:1: //
Copyright (c) 2012 The Chromium Authors. All rights reserved.
On 2013/02/26 21:25:03, gauravsh wrote:
> NIT: 2013

Done.

https://codereview.chromium.org/12211103/diff/20006/chrome/browser/chromeos/n...
chrome/browser/chromeos/net/managed_network_configuration_handler.cc:35: const
char kErrorMessage[] = "errorMessage";
On 2013/02/26 21:25:03, gauravsh wrote:
> You should add a comment that these are error names and message meant to be
used
> with error callbacks.

Done.

https://codereview.chromium.org/12211103/diff/20006/chrome/browser/chromeos/n...
chrome/browser/chromeos/net/managed_network_configuration_handler.cc:47: const
network_handler::ErrorCallback& error_callback) {
On 2013/02/26 17:31:24, stevenjb (chromium) wrote:
> Can we add a network_event_log event here?

Done.

https://codereview.chromium.org/12211103/diff/20006/chrome/browser/chromeos/n...
chrome/browser/chromeos/net/managed_network_configuration_handler.cc:97: //
TODO(pneubeck): Merging!
On 2013/02/26 17:31:24, stevenjb (chromium) wrote:
> Clarify?

Done.

https://codereview.chromium.org/12211103/diff/20006/chrome/browser/chromeos/n...
chrome/browser/chromeos/net/managed_network_configuration_handler.cc:126: //
TODO(pneubeck): Merging!
On 2013/02/26 17:31:24, stevenjb (chromium) wrote:
> ditto. I assume you mean 'merge properties'?

Done.

https://codereview.chromium.org/12211103/diff/20006/chrome/browser/chromeos/n...
chrome/browser/chromeos/net/managed_network_configuration_handler.cc:176: //
TODO(pneubeck): Merging!
On 2013/02/26 17:31:24, stevenjb (chromium) wrote:
> ditto

Done.

https://codereview.chromium.org/12211103/diff/20006/chrome/browser/chromeos/n...
File chrome/browser/chromeos/net/managed_network_configuration_handler.h
(right):

https://codereview.chromium.org/12211103/diff/20006/chrome/browser/chromeos/n...
chrome/browser/chromeos/net/managed_network_configuration_handler.h:1: //
Copyright (c) 2012 The Chromium Authors. All rights reserved.
On 2013/02/26 21:25:03, gauravsh wrote:
> NIT: 2013

Done.

https://codereview.chromium.org/12211103/diff/20006/chrome/browser/chromeos/n...
chrome/browser/chromeos/net/managed_network_configuration_handler.h:33: // This
is a singleton and it's lifetime is managed by by the Chrome startup
On 2013/02/26 21:25:03, gauravsh wrote:
> NIT: s/it's/its/, s/by by/by/

Done.

https://codereview.chromium.org/12211103/diff/20006/chrome/browser/chromeos/n...
chrome/browser/chromeos/net/managed_network_configuration_handler.h:96: 
On 2013/02/26 17:31:24, stevenjb (chromium) wrote:
> nit: one blank line

Done.

https://codereview.chromium.org/12211103/diff/20006/chrome/chrome_browser_chr...
File chrome/chrome_browser_chromeos.gypi (right):

https://codereview.chromium.org/12211103/diff/20006/chrome/chrome_browser_chr...
chrome/chrome_browser_chromeos.gypi:524:
'browser/chromeos/net/managed_network_configuration_handler.cc',
On 2013/02/26 21:25:03, gauravsh wrote:
> Can this be a part of chromeos/network? Looking at the #includes, doesn't look
> like the ManagedNetworkConfigurationHandler depends on anything under
> chrome/browser/chromeos.

It will when I added the policy application.

[commit-bot: I haz the power, 2013-02-27 09:55:02]

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

[commit-bot: I haz the power, 2013-02-27 12:19:29]

Change committed as 184933