This adds the setProperties and getState functions to the networking API

This adds the setProperties and getState functions to the networking API
for JavaScript.

(TBR'ing kalman because it's a private API, and because I may need to merge it to R26)

TBR=kalman@chromium.org
BUG=chromium:180633
TEST=ran new browser tests.


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

[Greg Spencer (Chromium), 2013-03-06 22:45:09]

Phillipp: please review.

Steven: OWNERS review, please.

[stevenjb, 2013-03-07 00:43:22]

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

https://codereview.chromium.org/12541007/diff/2001/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/networking_private_api.cc:82: error_ =
"Error.InvalidParameter";
Should this be "InvalidParameter"? A network might have disconnected and be no
longer available.

https://codereview.chromium.org/12541007/diff/2001/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/networking_private_api.cc:98:
~NetworkingPrivateSetPropertiesFunction() {
No WS at beginning of line here I think

https://codereview.chromium.org/12541007/diff/2001/chrome/browser/chromeos/ex...
File chrome/browser/chromeos/extensions/networking_private_api.h (right):

https://codereview.chromium.org/12541007/diff/2001/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/networking_private_api.h:41: class
NetworkingPrivateGetStateFunction : public AsyncExtensionFunction {
NetworkingPrivateGetPropertiesFunction?

https://codereview.chromium.org/12541007/diff/2001/chromeos/dbus/shill_manage...
File chromeos/dbus/shill_manager_client_stub.cc (right):

https://codereview.chromium.org/12541007/diff/2001/chromeos/dbus/shill_manage...
chromeos/dbus/shill_manager_client_stub.cc:164: // property matching magic here.
nit: move this commend below the next line (closer to the relevant code)

https://codereview.chromium.org/12541007/diff/2001/chromeos/dbus/shill_manage...
chromeos/dbus/shill_manager_client_stub.cc:193: // Now set all the properties.
Shouldn't we just set all of the new properties instead? I believe that will
have the same result as merging.

[pneubeck (no reviews), 2013-03-07 13:06:18]

lgtm

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

https://codereview.chromium.org/12541007/diff/2001/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/networking_private_api.cc:87:
state->FillDictionary(result_dict.get());
|result_dict| has to be translated to ONC, since "FillDictionary" doesn't do
that anymore.
See line 155 on the right.

https://codereview.chromium.org/12541007/diff/2001/chrome/common/extensions/a...
File chrome/common/extensions/api/networking_private.json (right):

https://codereview.chromium.org/12541007/diff/2001/chrome/common/extensions/a...
chrome/common/extensions/api/networking_private.json:70: "description": "Gets
the cached read-only properties of the network with id networkGuid.",
Mention here and at getVisibleNetworks, that both provide the same kind of
state, in contrast to getProperties

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

https://codereview.chromium.org/12541007/diff/2001/chrome/test/data/extension...
chrome/test/data/extensions/api_test/networking/test.js:190:
"Cellular.ActivationState": "",
After adding the translation, this dict should look identical to the one from
the getVisibleNetworks test.

https://codereview.chromium.org/12541007/diff/2001/chromeos/dbus/shill_manage...
File chromeos/dbus/shill_manager_client_stub.cc (right):

https://codereview.chromium.org/12541007/diff/2001/chromeos/dbus/shill_manage...
chromeos/dbus/shill_manager_client_stub.cc:164: // property matching magic here.
we can do that for now, but it might be necessary to change that... sadly...

https://codereview.chromium.org/12541007/diff/2001/chromeos/dbus/shill_manage...
chromeos/dbus/shill_manager_client_stub.cc:193: // Now set all the properties.
On 2013/03/07 00:43:22, stevenjb (chromium) wrote:
> Shouldn't we just set all of the new properties instead? I believe that will
> have the same result as merging.
+1

[Greg Spencer (Chromium), 2013-03-07 22:01:23]

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

https://codereview.chromium.org/12541007/diff/2001/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/networking_private_api.cc:82: error_ =
"Error.InvalidParameter";
On 2013/03/07 00:43:22, stevenjb (chromium) wrote:
> Should this be "InvalidParameter"? A network might have disconnected and be no
> longer available.

Well, my thought was that the service path was no longer valid, but if it can be
still valid and still get back NULL, then what should I return (i.e. how can I
distinguish between an invalid service path and a disconnected network)?

https://codereview.chromium.org/12541007/diff/2001/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/networking_private_api.cc:87:
state->FillDictionary(result_dict.get());
On 2013/03/07 13:06:18, pneubeck wrote:
> |result_dict| has to be translated to ONC, since "FillDictionary" doesn't do
> that anymore.
> See line 155 on the right.

Ahh, no wonder my results were off...

https://codereview.chromium.org/12541007/diff/2001/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/networking_private_api.cc:98:
~NetworkingPrivateSetPropertiesFunction() {
On 2013/03/07 00:43:22, stevenjb (chromium) wrote:
> No WS at beginning of line here I think

OK. :-)  I don't think there's a lot of precedent for this, so I was kind of
winging it.  The whitespace felt to me like it was a continuation more than not
having it.

https://codereview.chromium.org/12541007/diff/2001/chrome/browser/chromeos/ex...
File chrome/browser/chromeos/extensions/networking_private_api.h (right):

https://codereview.chromium.org/12541007/diff/2001/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/networking_private_api.h:41: class
NetworkingPrivateGetStateFunction : public AsyncExtensionFunction {
On 2013/03/07 00:43:22, stevenjb (chromium) wrote:
> NetworkingPrivateGetPropertiesFunction?

No, the comment is wrong.  Fixed.

https://codereview.chromium.org/12541007/diff/2001/chrome/common/extensions/a...
File chrome/common/extensions/api/networking_private.json (right):

https://codereview.chromium.org/12541007/diff/2001/chrome/common/extensions/a...
chrome/common/extensions/api/networking_private.json:70: "description": "Gets
the cached read-only properties of the network with id networkGuid.",
On 2013/03/07 13:06:18, pneubeck wrote:
> Mention here and at getVisibleNetworks, that both provide the same kind of
> state, in contrast to getProperties

Good point.  Done.

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

https://codereview.chromium.org/12541007/diff/2001/chrome/test/data/extension...
chrome/test/data/extensions/api_test/networking/test.js:190:
"Cellular.ActivationState": "",
On 2013/03/07 13:06:18, pneubeck wrote:
> After adding the translation, this dict should look identical to the one from
> the getVisibleNetworks test.

Right.  I was wondering why they were off, and now I know.

https://codereview.chromium.org/12541007/diff/2001/chromeos/dbus/shill_manage...
File chromeos/dbus/shill_manager_client_stub.cc (right):

https://codereview.chromium.org/12541007/diff/2001/chromeos/dbus/shill_manage...
chromeos/dbus/shill_manager_client_stub.cc:164: // property matching magic here.
On 2013/03/07 13:06:18, pneubeck wrote:
> we can do that for now, but it might be necessary to change that... sadly...

Yeah, I know.  I hope we don't have to.

https://codereview.chromium.org/12541007/diff/2001/chromeos/dbus/shill_manage...
chromeos/dbus/shill_manager_client_stub.cc:193: // Now set all the properties.
On 2013/03/07 00:43:22, stevenjb (chromium) wrote:
> Shouldn't we just set all of the new properties instead? I believe that will
> have the same result as merging.

Well, but (as I understand it) merging is recursive, and setting properties
isn't.  For instance, setting only "Security" in the WiFi property will mean
that all the existing WiFi properties are removed except "Security".  I was
trying to match Shill's behavior as much as possible, and since Shill only
merges when it does ConfigureService, that's what I was doing.

[pneubeck (no reviews), 2013-03-08 08:30:35]

still lgtm

https://codereview.chromium.org/12541007/diff/2001/chromeos/dbus/shill_manage...
File chromeos/dbus/shill_manager_client_stub.cc (right):

https://codereview.chromium.org/12541007/diff/2001/chromeos/dbus/shill_manage...
chromeos/dbus/shill_manager_client_stub.cc:193: // Now set all the properties.
Makes sense. Your solution is better for Dictionary-valued properties. I'd guess
it's also more similar to what Shill does, although I don't know its behavior
with certainty.

On 2013/03/07 22:01:23, Greg Spencer (Chromium) wrote:
> On 2013/03/07 00:43:22, stevenjb (chromium) wrote:
> > Shouldn't we just set all of the new properties instead? I believe that will
> > have the same result as merging.
> 
> Well, but (as I understand it) merging is recursive, and setting properties
> isn't.  For instance, setting only "Security" in the WiFi property will mean
> that all the existing WiFi properties are removed except "Security".  I was
> trying to match Shill's behavior as much as possible, and since Shill only
> merges when it does ConfigureService, that's what I was doing.

[stevenjb, 2013-03-08 19:04:16]

LGTM

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

https://codereview.chromium.org/12541007/diff/2001/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/networking_private_api.cc:82: error_ =
"Error.InvalidParameter";
On 2013/03/07 22:01:23, Greg Spencer (Chromium) wrote:
> On 2013/03/07 00:43:22, stevenjb (chromium) wrote:
> > Should this be "InvalidParameter"? A network might have disconnected and be
no
> > longer available.
> 
> Well, my thought was that the service path was no longer valid, but if it can
be
> still valid and still get back NULL, then what should I return (i.e. how can I
> distinguish between an invalid service path and a disconnected network)?

I guess I was thinking that the service could be "no longer available", which
isn't exactly the same as "invalid", but I guess it is impractical to
differentiate that (we'd have to track services that were available) and no real
practical difference between the two anyway.

https://codereview.chromium.org/12541007/diff/2001/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/networking_private_api.cc:98:
~NetworkingPrivateSetPropertiesFunction() {
On 2013/03/07 22:01:23, Greg Spencer (Chromium) wrote:
> On 2013/03/07 00:43:22, stevenjb (chromium) wrote:
> > No WS at beginning of line here I think
> 
> OK. :-)  I don't think there's a lot of precedent for this, so I was kind of
> winging it.  The whitespace felt to me like it was a continuation more than
not
> having it.

There's not a ton of precedent, but it's what I have seen and used before, and
is consistent with the Google Emacs formatting plugin :)

[Greg Spencer (Chromium), 2013-03-12 17:16:18]

Ben, I need an OWNERS review for the extension API changes.

[commit-bot: I haz the power, 2013-03-13 18:54:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gspencer@chromium.org/12541007/25001

[commit-bot: I haz the power, 2013-03-13 23:31:36]

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=linux_clan...

[commit-bot: I haz the power, 2013-03-14 21:15:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gspencer@chromium.org/12541007/25001

[commit-bot: I haz the power, 2013-03-14 21:31:20]

Change committed as 188199