Add forgetNetwork to networkingPrivate

Add forgetNetwork to networkingPrivate

BUG=433984
For:
extensions/common/api/networking_private.json
third_party/closure_compiler/externs/chrome_extensions.js
TBR=jamescook@chromium.org,dbeam@chromium.org

Committed: https://crrev.com/b0caacc752c1c62da71a465c721446ab64139ec7
Cr-Commit-Position: refs/heads/master@{#322204}

[stevenjb, 2015-03-07 01:24:55]

stevenjb@chromium.org changed reviewers:
+ mef@chromium.org, pneubeck@chromium.org, zentaro@chromium.org

[stevenjb, 2015-03-07 01:32:44]

Patchset #3 (id:40001) has been deleted

[pneubeck (no reviews), 2015-03-16 10:14:58]

The cabling of the MNCH::RemoveConfiguration to the API l-g-t-m.

However, I have some concern about the current implementation of that function.
I'm fine with going forward even with the current behavior, but we should have
agreement on the direction / final behavior of the API.

https://codereview.chromium.org/987963002/diff/60001/chrome/browser/extension...
File
chrome/browser/extensions/api/networking_private/networking_private_apitest.cc
(right):

https://codereview.chromium.org/987963002/diff/60001/chrome/browser/extension...
chrome/browser/extensions/api/networking_private/networking_private_apitest.cc:341:
EXPECT_TRUE(RunNetworkingSubtest("createNetwork")) << message_;
wrong test, should call 'forgetNetwork'

https://codereview.chromium.org/987963002/diff/60001/chrome/browser/extension...
chrome/browser/extensions/api/networking_private/networking_private_apitest.cc:434:
EXPECT_FALSE(RunNetworkingSubtest("createNetwork")) << message_;
wrong test, should call 'forgetNetwork'

https://codereview.chromium.org/987963002/diff/60001/extensions/browser/api/n...
File extensions/browser/api/networking_private/networking_private_chromeos.cc
(right):

https://codereview.chromium.org/987963002/diff/60001/extensions/browser/api/n...
extensions/browser/api/networking_private/networking_private_chromeos.cc:14:
#include "chromeos/network/network_activation_handler.h"
is this related?

https://codereview.chromium.org/987963002/diff/60001/extensions/common/api/ne...
File extensions/common/api/networking_private.json (right):

https://codereview.chromium.org/987963002/diff/60001/extensions/common/api/ne...
extensions/common/api/networking_private.json:191: "description": "Forgets a
network configuration, clearing any configured properties.",
Re "Clearing any configured properties.":
The current implementation of MNCH:: / NCH::RemoveConfiguration is that _all_
configurations, both user's and shared, are removed (using that
ProfileEntryDeleter).

I think this is not a good API to expose. All other API functions use GUID as a
_configuration_ identifier and allow control on the granularity of
configurations (e.g. modify the properties of a specific config).

The current behavior of forgetNetwork is like a hammer and simply removes all
configs.

I'd suggest to move towards a finer grained 'removes the configuration with GUID
networkGuid'

In which case we would either remove the user's _or_ the shared config, but not
automatically both.

Wdyt?

If really necessary, we could also expose the GUID of the shared config even if
it's currently hidden/overwritten by the user's config, e.g. by adding a
property with that GUID, or having additional network list entries.

https://codereview.chromium.org/987963002/diff/60001/extensions/common/api/ne...
extensions/common/api/networking_private.json:203: "description": "A callback
function that indicates that network has been forgotten."
the callback itself doesn't indicate success. only if there is no
runtime.lastError set.

Maybe more neutral (and less helpful :-) ): "Will be called after the operation
completed."

[stevenjb, 2015-03-20 16:52:26]

stevenjb@chromium.org changed reviewers:
+ isherman@chromium.org

[stevenjb, 2015-03-20 16:52:27]

+isherman@ for histograms.xml

PTAL

https://codereview.chromium.org/987963002/diff/60001/chrome/browser/extension...
File
chrome/browser/extensions/api/networking_private/networking_private_apitest.cc
(right):

https://codereview.chromium.org/987963002/diff/60001/chrome/browser/extension...
chrome/browser/extensions/api/networking_private/networking_private_apitest.cc:341:
EXPECT_TRUE(RunNetworkingSubtest("createNetwork")) << message_;
On 2015/03/16 10:14:57, pneubeck wrote:
> wrong test, should call 'forgetNetwork'

Oops. Done.

https://codereview.chromium.org/987963002/diff/60001/chrome/browser/extension...
chrome/browser/extensions/api/networking_private/networking_private_apitest.cc:434:
EXPECT_FALSE(RunNetworkingSubtest("createNetwork")) << message_;
On 2015/03/16 10:14:57, pneubeck wrote:
> wrong test, should call 'forgetNetwork'

Done.

https://codereview.chromium.org/987963002/diff/60001/extensions/browser/api/n...
File extensions/browser/api/networking_private/networking_private_chromeos.cc
(right):

https://codereview.chromium.org/987963002/diff/60001/extensions/browser/api/n...
extensions/browser/api/networking_private/networking_private_chromeos.cc:14:
#include "chromeos/network/network_activation_handler.h"
On 2015/03/16 10:14:57, pneubeck wrote:
> is this related?

No, wrong upstream.

https://codereview.chromium.org/987963002/diff/60001/extensions/common/api/ne...
File extensions/common/api/networking_private.json (right):

https://codereview.chromium.org/987963002/diff/60001/extensions/common/api/ne...
extensions/common/api/networking_private.json:191: "description": "Forgets a
network configuration, clearing any configured properties.",
On 2015/03/16 10:14:58, pneubeck wrote:
> Re "Clearing any configured properties.":
> The current implementation of MNCH:: / NCH::RemoveConfiguration is that _all_
> configurations, both user's and shared, are removed (using that
> ProfileEntryDeleter).
> 
> I think this is not a good API to expose. All other API functions use GUID as
a
> _configuration_ identifier and allow control on the granularity of
> configurations (e.g. modify the properties of a specific config).
> 
> The current behavior of forgetNetwork is like a hammer and simply removes all
> configs.
> 
> I'd suggest to move towards a finer grained 'removes the configuration with
GUID
> networkGuid'
> 
> In which case we would either remove the user's _or_ the shared config, but
not
> automatically both.
> 
> Wdyt?
> 
> If really necessary, we could also expose the GUID of the shared config even
if
> it's currently hidden/overwritten by the user's config, e.g. by adding a
> property with that GUID, or having additional network list entries.

What you suggest is reasonable, but I do not think it is worth the effort at
this time. We made a UI decision not to make that distinction and I don't expect
that to change any time soon.

The change you suggest will be easier once we migrate all the NetworkHandler
code to use GUID instead of service path.

I will clarify the description in the API for now. If we want to change the
behavior in the future we can add an optional 'source' parameter with the
default behavior being 'all'. The behavior will change slightly (only matching
guids, not identifiers will be removed), but I don't expect that to cause any
problems.

https://codereview.chromium.org/987963002/diff/60001/extensions/common/api/ne...
extensions/common/api/networking_private.json:203: "description": "A callback
function that indicates that network has been forgotten."
On 2015/03/16 10:14:58, pneubeck wrote:
> the callback itself doesn't indicate success. only if there is no
> runtime.lastError set.
> 
> Maybe more neutral (and less helpful :-) ): "Will be called after the
operation
> completed."

Done.

[Ilya Sherman, 2015-03-20 19:47:21]

histograms.xml lgtm

[pneubeck (no reviews), 2015-03-23 14:15:21]

I'm not totally convinced of the behavior of forgetNetwork.
As the difference is not critical enough to justify a long argument, feel free
to make the call.

lgtm

https://codereview.chromium.org/987963002/diff/60001/extensions/common/api/ne...
File extensions/common/api/networking_private.json (right):

https://codereview.chromium.org/987963002/diff/60001/extensions/common/api/ne...
extensions/common/api/networking_private.json:191: "description": "Forgets a
network configuration, clearing any configured properties.",
On 2015/03/20 16:52:27, stevenjb wrote:
> On 2015/03/16 10:14:58, pneubeck wrote:
> > Re "Clearing any configured properties.":
> > The current implementation of MNCH:: / NCH::RemoveConfiguration is that
_all_
> > configurations, both user's and shared, are removed (using that
> > ProfileEntryDeleter).
> > 
> > I think this is not a good API to expose. All other API functions use GUID
as
> a
> > _configuration_ identifier and allow control on the granularity of
> > configurations (e.g. modify the properties of a specific config).
> > 
> > The current behavior of forgetNetwork is like a hammer and simply removes
all
> > configs.
> > 
> > I'd suggest to move towards a finer grained 'removes the configuration with
> GUID
> > networkGuid'
> > 
> > In which case we would either remove the user's _or_ the shared config, but
> not
> > automatically both.
> > 
> > Wdyt?
> > 
> > If really necessary, we could also expose the GUID of the shared config even
> if
> > it's currently hidden/overwritten by the user's config, e.g. by adding a
> > property with that GUID, or having additional network list entries.
> 
> What you suggest is reasonable, but I do not think it is worth the effort at
> this time. We made a UI decision not to make that distinction and I don't
expect
> that to change any time soon.
I can believe that it was a UI decision to show at most one configuration per
SSID+SecurityType.
I very much doubt however, that it was a well-thought-out decision to remove all
network configurations at a time from the 'preferred networks' screen.
E.g. if a user indeed ends-up in the case that there is a shared and a private
configuration for the same network and he wants to 'repair' the current
configuration by removing it, he will accidentally remove the shared
configuration that he's very likely unaware of and will potentially break
connectivity on the login-screen and for other users.

Btw. the 'remove all configurations' behavior can be easily polyfilled in JS:
After the removal, get the updated network list and check for a 'similar'
configuration and remove it as well.

> The change you suggest will be easier once we migrate all the NetworkHandler
> code to use GUID instead of service path.
> 
> I will clarify the description in the API for now. If we want to change the
> behavior in the future we can add an optional 'source' parameter with the
> default behavior being 'all'. The behavior will change slightly (only matching
> guids, not identifiers will be removed), but I don't expect that to cause any
> problems.

Sure, that would work as well. IMO, the default behavior (without providing the
optional 'source' parameter) would be irritating though.
If you want to go this route, pls invert the default:
 - initially make the additional argument mandatory and only support 'all' for
now
 - later add support for specific sources.

https://codereview.chromium.org/987963002/diff/80001/chrome/test/data/extensi...
File chrome/test/data/extensions/api_test/networking_private/chromeos/test.js
(right):

https://codereview.chromium.org/987963002/diff/80001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/networking_private/chromeos/test.js:153:
console.log(JSON.stringify(networks));
debugging left-over?

https://codereview.chromium.org/987963002/diff/80001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/networking_private/chromeos/test.js:154:
assertEq(kNumNetworks - 1, networks.length);
would be nice to ensure that the right network was removed.

https://codereview.chromium.org/987963002/diff/80001/extensions/common/api/ne...
File extensions/common/api/networking_private.json (right):

https://codereview.chromium.org/987963002/diff/80001/extensions/common/api/ne...
extensions/common/api/networking_private.json:193: "description": "Forgets a
network configuration, clearing any configured properties of networks with
identifiers matching those associated with 'networkGuid'. If no configuration
matching 'networkGuid' exists then an error will be set and the operation will
fail.",
reads a bit complicated.
Please re-read as a native speaker and if necessary re-phrase.

[stevenjb, 2015-03-23 16:11:02]

https://codereview.chromium.org/987963002/diff/60001/extensions/common/api/ne...
File extensions/common/api/networking_private.json (right):

https://codereview.chromium.org/987963002/diff/60001/extensions/common/api/ne...
extensions/common/api/networking_private.json:191: "description": "Forgets a
network configuration, clearing any configured properties.",
On 2015/03/23 14:15:20, pneubeck wrote:
> On 2015/03/20 16:52:27, stevenjb wrote:
> > On 2015/03/16 10:14:58, pneubeck wrote:
> > > Re "Clearing any configured properties.":
> > > The current implementation of MNCH:: / NCH::RemoveConfiguration is that
> _all_
> > > configurations, both user's and shared, are removed (using that
> > > ProfileEntryDeleter).
> > > 
> > > I think this is not a good API to expose. All other API functions use GUID
> as
> > a
> > > _configuration_ identifier and allow control on the granularity of
> > > configurations (e.g. modify the properties of a specific config).
> > > 
> > > The current behavior of forgetNetwork is like a hammer and simply removes
> all
> > > configs.
> > > 
> > > I'd suggest to move towards a finer grained 'removes the configuration
with
> > GUID
> > > networkGuid'
> > > 
> > > In which case we would either remove the user's _or_ the shared config,
but
> > not
> > > automatically both.
> > > 
> > > Wdyt?
> > > 
> > > If really necessary, we could also expose the GUID of the shared config
even
> > if
> > > it's currently hidden/overwritten by the user's config, e.g. by adding a
> > > property with that GUID, or having additional network list entries.
> > 
> > What you suggest is reasonable, but I do not think it is worth the effort at
> > this time. We made a UI decision not to make that distinction and I don't
> expect
> > that to change any time soon.
> I can believe that it was a UI decision to show at most one configuration per
> SSID+SecurityType.
> I very much doubt however, that it was a well-thought-out decision to remove
all
> network configurations at a time from the 'preferred networks' screen.
> E.g. if a user indeed ends-up in the case that there is a shared and a private
> configuration for the same network and he wants to 'repair' the current
> configuration by removing it, he will accidentally remove the shared
> configuration that he's very likely unaware of and will potentially break
> connectivity on the login-screen and for other users.
> 
> Btw. the 'remove all configurations' behavior can be easily polyfilled in JS:
> After the removal, get the updated network list and check for a 'similar'
> configuration and remove it as well.
> 
> > The change you suggest will be easier once we migrate all the NetworkHandler
> > code to use GUID instead of service path.
> > 
> > I will clarify the description in the API for now. If we want to change the
> > behavior in the future we can add an optional 'source' parameter with the
> > default behavior being 'all'. The behavior will change slightly (only
matching
> > guids, not identifiers will be removed), but I don't expect that to cause
any
> > problems.
> 
> Sure, that would work as well. IMO, the default behavior (without providing
the
> optional 'source' parameter) would be irritating though.
> If you want to go this route, pls invert the default:
>  - initially make the additional argument mandatory and only support 'all' for
> now
>  - later add support for specific sources.

If we had already implemented NCH::RemoveConfiguration as you suggest, I would
have no problem with any of your suggestions. However, spending the time to
modify NCH and test it, only to implement the behavior as-is in the UI, is hard
for me to justify. Especially when adding the suggested behavior later will be
easy / straightforward. (The choice of default behavior won't actually be
important since we will also control the consumers).

(BTW, FWIW, IIRC, when we made the UI decision, we couldn't come up with a non
contrived use case where a user would want to forget a non-shared network that
was also shared, and leave the shared configuration intact).

https://codereview.chromium.org/987963002/diff/80001/chrome/test/data/extensi...
File chrome/test/data/extensions/api_test/networking_private/chromeos/test.js
(right):

https://codereview.chromium.org/987963002/diff/80001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/networking_private/chromeos/test.js:153:
console.log(JSON.stringify(networks));
On 2015/03/23 14:15:20, pneubeck wrote:
> debugging left-over?

Oops. Thanks. Done.

https://codereview.chromium.org/987963002/diff/80001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/networking_private/chromeos/test.js:154:
assertEq(kNumNetworks - 1, networks.length);
On 2015/03/23 14:15:20, pneubeck wrote:
> would be nice to ensure that the right network was removed.

Done.

https://codereview.chromium.org/987963002/diff/80001/extensions/common/api/ne...
File extensions/common/api/networking_private.json (right):

https://codereview.chromium.org/987963002/diff/80001/extensions/common/api/ne...
extensions/common/api/networking_private.json:193: "description": "Forgets a
network configuration, clearing any configured properties of networks with
identifiers matching those associated with 'networkGuid'. If no configuration
matching 'networkGuid' exists then an error will be set and the operation will
fail.",
On 2015/03/23 14:15:20, pneubeck wrote:
> reads a bit complicated.
> Please re-read as a native speaker and if necessary re-phrase.

Attempted to clarify.

[stevenjb, 2015-03-23 16:14:20]

The CQ bit was checked by stevenjb@chromium.org

[stevenjb, 2015-03-23 16:14:20]

The patchset sent to the CQ was uploaded after l-g-t-m from
isherman@chromium.org, pneubeck@chromium.org
Link to the patchset: https://codereview.chromium.org/987963002/#ps120001
(title: "Rebase")

[commit-bot: I haz the power, 2015-03-23 16:14:39]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/987963002/120001

[commit-bot: I haz the power, 2015-03-23 16:21:27]

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

[commit-bot: I haz the power, 2015-03-23 16:21:28]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[stevenjb, 2015-03-24 21:13:29]

The CQ bit was checked by stevenjb@chromium.org

[stevenjb, 2015-03-24 21:13:35]

The CQ bit was unchecked by stevenjb@chromium.org

[stevenjb, 2015-03-24 22:19:53]

The CQ bit was checked by stevenjb@chromium.org

[stevenjb, 2015-03-24 22:19:54]

The patchset sent to the CQ was uploaded after l-g-t-m from
isherman@chromium.org, pneubeck@chromium.org
Link to the patchset: https://codereview.chromium.org/987963002/#ps140001
(title: "Rebase")

[commit-bot: I haz the power, 2015-03-24 22:21:27]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/987963002/140001

[commit-bot: I haz the power, 2015-03-25 00:26:40]

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

[commit-bot: I haz the power, 2015-03-25 00:26:44]

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

[stevenjb, 2015-03-25 16:47:26]

The CQ bit was checked by stevenjb@chromium.org

[stevenjb, 2015-03-25 16:47:30]

The CQ bit was unchecked by stevenjb@chromium.org

[stevenjb, 2015-03-25 17:48:46]

The CQ bit was checked by stevenjb@chromium.org

[stevenjb, 2015-03-25 17:48:47]

The patchset sent to the CQ was uploaded after l-g-t-m from
isherman@chromium.org, pneubeck@chromium.org
Link to the patchset: https://codereview.chromium.org/987963002/#ps160001
(title: "Rebase")

[commit-bot: I haz the power, 2015-03-25 17:49:47]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/987963002/160001

[commit-bot: I haz the power, 2015-03-25 19:12:54]

Committed patchset #8 (id:160001)

[commit-bot: I haz the power, 2015-03-25 19:13:34]

Patchset 8 (id:??) landed as
https://crrev.com/b0caacc752c1c62da71a465c721446ab64139ec7
Cr-Commit-Position: refs/heads/master@{#322204}

[Dan Beam, 2015-03-25 23:02:29]

dbeam@chromium.org changed reviewers:
+ dbeam@chromium.org

[Dan Beam, 2015-03-25 23:02:30]

for future reference, putting TBR= in the CL description after you've uploaded
doesn't email the reviewers.

they need to be in the Reviewers: [________] box

https://codereview.chromium.org/987963002/diff/160001/third_party/closure_com...
File third_party/closure_compiler/externs/chrome_extensions.js (right):

https://codereview.chromium.org/987963002/diff/160001/third_party/closure_com...
third_party/closure_compiler/externs/chrome_extensions.js:8334:
chrome.networkingPrivate.forgetNetwork = function(guid, opt_callback) {};
this is a generated file, changes to it will be lost.

I'm adding a message to this effect now in bump_compiler_version