Using the new Network*Handlers in networkingPrivate Extension API.

Changes the networkingPrivate Extension API to use
the NetworkStateHandler and ManagedNetworkConfigurationHandler instead of talking to the Shill*Client classes directly.

This removes nearly all dependencies to Shill (except the VerifyAndSign functions) from the API implementation.

BUG=147614, 157696
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=187202

[Greg Spencer (Chromium), 2013-03-02 00:09:10]

https://codereview.chromium.org/12319145/diff/5001/chrome/browser/chromeos/ex...
File chrome/browser/chromeos/extensions/networking_private_api.cc (left):

https://codereview.chromium.org/12319145/diff/5001/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/networking_private_api.cc:57: class
ResultList : public base::RefCounted<ResultList> {
It feels nice to get rid of this complexity finally. :-)

https://codereview.chromium.org/12319145/diff/5001/chrome/browser/extensions/...
File chrome/browser/extensions/api/test/test_api.cc (right):

https://codereview.chromium.org/12319145/diff/5001/chrome/browser/extensions/...
chrome/browser/extensions/api/test/test_api.cc:73: LOG(ERROR) << message;
Are you sure we want this?  Seems like someone put it into VLOG(1) for a reason.
 Probably to reduce log spam when running tests.  Also, if you're going to send
it to LOG(ERROR), then there's no need to send it to VLOG(1) too.

Or maybe this is just leftover debugging output that you forgot to remove?

https://codereview.chromium.org/12319145/diff/5001/chrome/test/data/extension...
File chrome/test/data/extensions/api_test/networking/test.js (right):

https://codereview.chromium.org/12319145/diff/5001/chrome/test/data/extension...
chrome/test/data/extensions/api_test/networking/test.js:31: var expectedState =
expectedStates.pop();
I like this: more flexible.

[pneubeck (no reviews), 2013-03-04 10:44:37]

Removed the debugging cruft.

The command line switch is now used only to toggle the usage of the handlers in
the Tray.
The extension API always uses the new handlers.

https://codereview.chromium.org/12319145/diff/5001/chrome/browser/extensions/...
File chrome/browser/extensions/api/test/test_api.cc (right):

https://codereview.chromium.org/12319145/diff/5001/chrome/browser/extensions/...
chrome/browser/extensions/api/test/test_api.cc:73: LOG(ERROR) << message;
Yes. that's a leftover. Somehow it's not possible to enable the VLOG in this
case. I have to investigate that at some time, as this is really really
annoying.

On 2013/03/02 00:09:11, Greg Spencer (Chromium) wrote:
> Are you sure we want this?  Seems like someone put it into VLOG(1) for a
reason.
>  Probably to reduce log spam when running tests.  Also, if you're going to
send
> it to LOG(ERROR), then there's no need to send it to VLOG(1) too.
> 
> Or maybe this is just leftover debugging output that you forgot to remove?

https://codereview.chromium.org/12319145/diff/5001/chrome/renderer/resources/...
File chrome/renderer/resources/extensions/apitest.js (right):

https://codereview.chromium.org/12319145/diff/5001/chrome/renderer/resources/...
chrome/renderer/resources/extensions/apitest.js:92: chrome.test.log("[FAIL] " +
testName(currentTest) + ": " + message);
also a leftover for debuggin...

[pneubeck (no reviews), 2013-03-04 11:37:45]

Hi Steven,

please review
  /chromeos/**
  /chrome/browser/chromeos/chrome_browser_main_chromeos.cc

sorry for the seesaw with the command line flag, but conditionally enabling the
new handlers for the API makes testing rather problematic.
Since there are no real users of the API yet, we also cannot break anything
anyways.

[stevenjb, 2013-03-04 18:57:58]

https://codereview.chromium.org/12319145/diff/4036/chrome/browser/chromeos/ch...
File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (left):

https://codereview.chromium.org/12319145/diff/4036/chrome/browser/chromeos/ch...
chrome/browser/chromeos/chrome_browser_main_chromeos.cc:287:
chromeos::switches::kEnableNewNetworkConfigurationHandlers)) {
If we're changing the meaning of this, we should change the name, to something
like "use-new-network-configuration-handlers" (similar to what I did for the
status area)

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

https://codereview.chromium.org/12319145/diff/4036/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/networking_private_api.cc:111: type_filter !=
onc::network_type::kAllTypes)
nit: switch test order (slightly more readable) and add {}. Also, personally in
this case I think 'continue' is less clear than putting the push_back inside the
(inverted) if().

https://codereview.chromium.org/12319145/diff/4036/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/networking_private_api.cc:114:
result.push_back(linked_ptr<api::NetworkProperties>(
Converting a scoped_ptr to a linked_ptr is confusing. I would suggest having
CreateNetworkProperties either return a raw ptr (it's local), then you can use a
ScopedVector here and line 66 will be unaffected, or have
CreateNetworkProperties that a api::NetworkProperties* instead of creating the
instance.

https://codereview.chromium.org/12319145/diff/4036/chromeos/network/network_s...
File chromeos/network/network_state.cc (right):

https://codereview.chromium.org/12319145/diff/4036/chromeos/network/network_s...
chromeos/network/network_state.cc:77: signal_strength());
Alternately, the above code doesn't actually depend on ONC, and could be named
something like NetworkState::GetDictionary(dict), which would pretty much be the
inverse of PropertyChanged(). Having a few extra properties in the dictionary
won't cost much and it might be useful elsewhere.

https://codereview.chromium.org/12319145/diff/4036/chromeos/network/network_s...
File chromeos/network/network_state.h (right):

https://codereview.chromium.org/12319145/diff/4036/chromeos/network/network_s...
chromeos/network/network_state.h:48: scoped_ptr<base::DictionaryValue>
TranslateToONC() const;
I don't like having this here. I would rather we have a namespaced global
method:

bool onc::TranslateNetworkState(const NetworkState*, DictionaryValue* onc)

(note: I also prefer passing the dictionary to fill in rather than passing
around scoped_ptr, but that's just a personal style preference).

If we don't have and don't want NetworkState dependencies in the ONC code
itself, I am fine with adding network_state_onc.cc to this directory, but I
don't want NetworkState itself to depend on ONC. Does that make sense?

[pneubeck (no reviews), 2013-03-05 13:11:55]

https://codereview.chromium.org/12319145/diff/4036/chrome/browser/chromeos/ch...
File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (left):

https://codereview.chromium.org/12319145/diff/4036/chrome/browser/chromeos/ch...
chrome/browser/chromeos/chrome_browser_main_chromeos.cc:287:
chromeos::switches::kEnableNewNetworkConfigurationHandlers)) {
On 2013/03/04 18:57:58, stevenjb (chromium) wrote:
> If we're changing the meaning of this, we should change the name, to something
> like "use-new-network-configuration-handlers" (similar to what I did for the
> status area)

Done.

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

https://codereview.chromium.org/12319145/diff/4036/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/networking_private_api.cc:111: type_filter !=
onc::network_type::kAllTypes)
On 2013/03/04 18:57:58, stevenjb (chromium) wrote:
> nit: switch test order (slightly more readable) and add {}. Also, personally
in
> this case I think 'continue' is less clear than putting the push_back inside
the
> (inverted) if().

Done.

https://codereview.chromium.org/12319145/diff/4036/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/networking_private_api.cc:114:
result.push_back(linked_ptr<api::NetworkProperties>(
The linked_ptr is required by the call to
  api::GetVisibleNetworks::Results::Create
below.

As the whole point of the generated Results classes is typesafety and
simplicity, which isn't at all given in this case (we only use the untyped
additional_properties), I removed them and directly create the results_. That
safes some useless copies and simplifies the code.

On 2013/03/04 18:57:58, stevenjb (chromium) wrote:
> Converting a scoped_ptr to a linked_ptr is confusing. I would suggest having
> CreateNetworkProperties either return a raw ptr (it's local), then you can use
a
> ScopedVector here and line 66 will be unaffected, or have
> CreateNetworkProperties that a api::NetworkProperties* instead of creating the
> instance.

https://codereview.chromium.org/12319145/diff/4036/chromeos/network/network_s...
File chromeos/network/network_state.cc (right):

https://codereview.chromium.org/12319145/diff/4036/chromeos/network/network_s...
chromeos/network/network_state.cc:77: signal_strength());
Added such an inverse function. Changed the order of the Set* calls to match the
implementation of PropertyChanged.

Done.

On 2013/03/04 18:57:58, stevenjb (chromium) wrote:
> Alternately, the above code doesn't actually depend on ONC, and could be named
> something like NetworkState::GetDictionary(dict), which would pretty much be
the
> inverse of PropertyChanged(). Having a few extra properties in the dictionary
> won't cost much and it might be useful elsewhere.

https://codereview.chromium.org/12319145/diff/4036/chromeos/network/network_s...
File chromeos/network/network_state.h (right):

https://codereview.chromium.org/12319145/diff/4036/chromeos/network/network_s...
chromeos/network/network_state.h:48: scoped_ptr<base::DictionaryValue>
TranslateToONC() const;
On 2013/03/04 18:57:58, stevenjb (chromium) wrote:
> I don't like having this here. I would rather we have a namespaced global
> method:
> 
> bool onc::TranslateNetworkState(const NetworkState*, DictionaryValue* onc)
> 
> (note: I also prefer passing the dictionary to fill in rather than passing
> around scoped_ptr, but that's just a personal style preference).
> 
> If we don't have and don't want NetworkState dependencies in the ONC code
> itself, I am fine with adding network_state_onc.cc to this directory, but I
> don't want NetworkState itself to depend on ONC. Does that make sense?

Done.

[stevenjb, 2013-03-05 17:59:18]

This looks better, thanks.

https://codereview.chromium.org/12319145/diff/18001/chrome/browser/chromeos/e...
File chrome/browser/chromeos/extensions/networking_private_api.cc (right):

https://codereview.chromium.org/12319145/diff/18001/chrome/browser/chromeos/e...
chrome/browser/chromeos/extensions/networking_private_api.cc:80:
NetworkStateHandler::Get();
One line?

https://codereview.chromium.org/12319145/diff/18001/chrome/browser/chromeos/e...
chrome/browser/chromeos/extensions/networking_private_api.cc:101: 
nit: remove blank line

https://codereview.chromium.org/12319145/diff/18001/chrome/browser/chromeos/s...
File chrome/browser/chromeos/system/ash_system_tray_delegate.cc (right):

https://codereview.chromium.org/12319145/diff/18001/chrome/browser/chromeos/s...
chrome/browser/chromeos/system/ash_system_tray_delegate.cc:747:
ash::switches::kAshEnableNewNetworkStatusArea)) {
Please use the new flag here; it needs to match the flag in tray_network.cc (and
we haven't really tested this path yet).

https://codereview.chromium.org/12319145/diff/18001/chromeos/dbus/shill_servi...
File chromeos/dbus/shill_service_client_stub.cc (right):

https://codereview.chromium.org/12319145/diff/18001/chromeos/dbus/shill_servi...
chromeos/dbus/shill_service_client_stub.cc:346:
base::StringValue(flimflam::kRoamingStateHome));
These changes are fine, but don't rely on them for tests. Instead, call
ClearServices() then set up the expectations for that test. See unittest changes
in https://codereview.chromium.org/12387065/

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

https://codereview.chromium.org/12319145/diff/18001/chromeos/network/network_...
chromeos/network/network_state.cc:54: void
NetworkState::FillDictionary(base::DictionaryValue* dictionary) const {
nit: let's put this (and the header definition) just below PropertyChanged(),
and comment in the header that changing one should change the other.

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

https://codereview.chromium.org/12319145/diff/18001/chromeos/network/network_...
chromeos/network/network_state.h:8: #include "base/memory/scoped_ptr.h"
Not needed

[gauravsh, 2013-03-05 21:04:56]

https://codereview.chromium.org/12319145/diff/18001/chrome/browser/chromeos/e...
File chrome/browser/chromeos/extensions/networking_private_api.cc (right):

https://codereview.chromium.org/12319145/diff/18001/chrome/browser/chromeos/e...
chrome/browser/chromeos/extensions/networking_private_api.cc:54:
SetResult(network_properties);
SetResult will take ownership of |network_properties|?

https://codereview.chromium.org/12319145/diff/18001/chrome/browser/chromeos/e...
chrome/browser/chromeos/extensions/networking_private_api.cc:82:
state_handler->GetNetworkList(&network_states);
|state_handler| is used only once, do you really need a separate variable for
it?

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

https://codereview.chromium.org/12319145/diff/18001/chromeos/network/network_...
chromeos/network/network_state.cc:58: signal_strength());
NIT: indent off?

https://codereview.chromium.org/12319145/diff/18001/chromeos/network/network_...
chromeos/network/network_state.cc:60: connection_state());
NIT: indent off? (and for the lines below as well).

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

https://codereview.chromium.org/12319145/diff/18001/chromeos/network/network_...
chromeos/network/network_state.h:49: // to PropertyChanged.
How often do you anticipate this being used? Since we have accessors for all the
properties, does this helper needs to be a method in the class?

https://codereview.chromium.org/12319145/diff/18001/chromeos/network/network_...
chromeos/network/network_state.h:50: void FillDictionary(base::DictionaryValue*
dictionary) const;
I'd prefer a less generic name - GetPropertyDictionary(), for example.

[pneubeck (no reviews), 2013-03-07 15:53:02]

Addressed all comments. Tests should now independent of the stubs default.

https://codereview.chromium.org/12319145/diff/18001/chrome/browser/chromeos/e...
File chrome/browser/chromeos/extensions/networking_private_api.cc (right):

https://codereview.chromium.org/12319145/diff/18001/chrome/browser/chromeos/e...
chrome/browser/chromeos/extensions/networking_private_api.cc:54:
SetResult(network_properties);
On 2013/03/05 21:04:56, gauravsh wrote:
> SetResult will take ownership of |network_properties|?

yes.

https://codereview.chromium.org/12319145/diff/18001/chrome/browser/chromeos/e...
chrome/browser/chromeos/extensions/networking_private_api.cc:80:
NetworkStateHandler::Get();
On 2013/03/05 17:59:18, stevenjb (chromium) wrote:
> One line?

Done.

https://codereview.chromium.org/12319145/diff/18001/chrome/browser/chromeos/e...
chrome/browser/chromeos/extensions/networking_private_api.cc:82:
state_handler->GetNetworkList(&network_states);
On 2013/03/05 21:04:56, gauravsh wrote:
> |state_handler| is used only once, do you really need a separate variable for
> it?

Done.

https://codereview.chromium.org/12319145/diff/18001/chrome/browser/chromeos/e...
chrome/browser/chromeos/extensions/networking_private_api.cc:101: 
On 2013/03/05 17:59:18, stevenjb (chromium) wrote:
> nit: remove blank line

Done.

https://codereview.chromium.org/12319145/diff/18001/chrome/browser/chromeos/s...
File chrome/browser/chromeos/system/ash_system_tray_delegate.cc (right):

https://codereview.chromium.org/12319145/diff/18001/chrome/browser/chromeos/s...
chrome/browser/chromeos/system/ash_system_tray_delegate.cc:747:
ash::switches::kAshEnableNewNetworkStatusArea)) {
On 2013/03/05 17:59:18, stevenjb (chromium) wrote:
> Please use the new flag here; it needs to match the flag in tray_network.cc
(and
> we haven't really tested this path yet).

Done.

https://codereview.chromium.org/12319145/diff/18001/chromeos/dbus/shill_servi...
File chromeos/dbus/shill_service_client_stub.cc (right):

https://codereview.chromium.org/12319145/diff/18001/chromeos/dbus/shill_servi...
chromeos/dbus/shill_service_client_stub.cc:346:
base::StringValue(flimflam::kRoamingStateHome));
On 2013/03/05 17:59:18, stevenjb (chromium) wrote:
> These changes are fine, but don't rely on them for tests. Instead, call
> ClearServices() then set up the expectations for that test. See unittest
changes
> in https://codereview.chromium.org/12387065/

Done after that CL was committed.

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

https://codereview.chromium.org/12319145/diff/18001/chromeos/network/network_...
chromeos/network/network_state.cc:54: void
NetworkState::FillDictionary(base::DictionaryValue* dictionary) const {
On 2013/03/05 17:59:18, stevenjb (chromium) wrote:
> nit: let's put this (and the header definition) just below PropertyChanged(),
> and comment in the header that changing one should change the other.

Done.

https://codereview.chromium.org/12319145/diff/18001/chromeos/network/network_...
chromeos/network/network_state.cc:58: signal_strength());
On 2013/03/05 21:04:56, gauravsh wrote:
> NIT: indent off?

Done.

https://codereview.chromium.org/12319145/diff/18001/chromeos/network/network_...
chromeos/network/network_state.cc:60: connection_state());
On 2013/03/05 21:04:56, gauravsh wrote:
> NIT: indent off? (and for the lines below as well).

Done.

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

https://codereview.chromium.org/12319145/diff/18001/chromeos/network/network_...
chromeos/network/network_state.h:8: #include "base/memory/scoped_ptr.h"
On 2013/03/05 17:59:18, stevenjb (chromium) wrote:
> Not needed

Done.

https://codereview.chromium.org/12319145/diff/18001/chromeos/network/network_...
chromeos/network/network_state.h:49: // to PropertyChanged.
By having this function in network_state.cc, we reduce the number of places with
references to Shill state constants and it's easier to keep the two directions
of translation in sync.

With this change, the only places that use these constants are the ONC
translator and NetworkState. I'd prefer to not add another such dependency.

On 2013/03/05 21:04:56, gauravsh wrote:
> How often do you anticipate this being used? Since we have accessors for all
the
> properties, does this helper needs to be a method in the class?

https://codereview.chromium.org/12319145/diff/18001/chromeos/network/network_...
chromeos/network/network_state.h:50: void FillDictionary(base::DictionaryValue*
dictionary) const;
Dictionary is already encoded in the type. Chose GetProperties.

On 2013/03/05 21:04:56, gauravsh wrote:
> I'd prefer a less generic name - GetPropertyDictionary(), for example.

[stevenjb, 2013-03-07 18:29:54]

LGTM w/nits

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

https://codereview.chromium.org/12319145/diff/50001/chromeos/network/network_...
chromeos/network/network_state.h:27: // If you change this method, update
GetProperties too.
nit: We should probably describe this function too (my bad)
Also, technically non-sentences like "ManagedState overrides", "// Accessors",
and the other comments below should not end with a '.'

[pneubeck (no reviews), 2013-03-07 19:26:36]

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

https://codereview.chromium.org/12319145/diff/50001/chromeos/network/network_...
chromeos/network/network_state.h:27: // If you change this method, update
GetProperties too.
This function is already documented in the base class. I don't think we want to
repeat the description? This comment is only implementation related.

I'm rather passionless about the comments format. I think, I looked at
http://src.chromium.org/viewvc/chrome/trunk/src/chromeos/network/network_stat...
for consistency, but that in turn was influenced by our last review ;-)

On 2013/03/07 18:29:54, stevenjb (chromium) wrote:
> nit: We should probably describe this function too (my bad)
> Also, technically non-sentences like "ManagedState overrides", "// Accessors",
> and the other comments below should not end with a '.'

[commit-bot: I haz the power, 2013-03-08 19:11:15]

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

[commit-bot: I haz the power, 2013-03-08 19:14:13]

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

[commit-bot: I haz the power, 2013-03-08 19:17:25]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...

[commit-bot: I haz the power, 2013-03-10 08:26:13]

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

[commit-bot: I haz the power, 2013-03-10 09:40:04]

Change committed as 187202