Introduce networking.onc as an alias for networkingPrivate

Introduces alias networking.onc for networkingPrivate API.

Adds networking.onc permission, which guards the introduced
networking.onc alias - for now, the permission is bound to dev channel
and available to the same whitelist as networkingPrivate API.

To avoid duplicating whitelist for the two permissions, the whitelist
is moved to behavior features and the behavior feature is added as a
dependency for both of them.

Refactors networking private API tests to enable parameterizing the
networking API to be used in the test and adds tests that exercise
alias - networking.onc code path.
Also groups some sub tests (which were all run in a separate browser
test) together in order to reduce total overhead of starting browser
for browser tests.

This is the first step towards renaming networkingPrivate API to
networking.onc (which will be made available to kiosk sessions).

BUG=672186
Committed: https://crrev.com/975d838eca97d3cac1578861c9df42fe46e2a9d5
Cr-Commit-Position: refs/heads/master@{#438574}

[tbarzic, 2016-11-02 20:40:34]

The CQ bit was checked by tbarzic@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-02 20:41:14]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tbarzic, 2016-11-29 19:09:21]

The CQ bit was checked by tbarzic@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-29 19:09:55]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-29 19:33:34]

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

[commit-bot: I haz the power, 2016-11-29 19:33:35]

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

[tbarzic, 2016-11-29 19:37:40]

The CQ bit was checked by tbarzic@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-29 19:38:30]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-29 20:34:52]

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

[commit-bot: I haz the power, 2016-11-29 20:34:53]

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

[tbarzic, 2016-11-29 23:58:46]

The CQ bit was checked by tbarzic@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-29 23:59:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tbarzic, 2016-11-30 01:08:24]

Description was changed from

==========
Rename networkingPrivate API to networkConfig

Renames networkingPrivate API to networkConfig, and introduces
networkingPrivate API alias for created networkConfig API name.

BUG=651488
==========

to

==========
Rename networkingPrivate API to networkConfig

Renames networkingPrivate API to networkConfig, and keeps
networkingPrivate API an an alias for created networkConfig API.

BUG=651488
==========

[commit-bot: I haz the power, 2016-11-30 02:01:40]

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

[commit-bot: I haz the power, 2016-11-30 02:01:42]

Dry run: Try jobs failed on following builders:
  blimp_linux_dbg on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)

[tbarzic, 2016-12-06 22:32:26]

The CQ bit was checked by tbarzic@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-06 22:34:39]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-07 00:49:27]

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

[commit-bot: I haz the power, 2016-12-07 00:49:29]

Dry run: This issue passed the CQ dry run.

[tbarzic, 2016-12-07 18:43:57]

tbarzic@chromium.org changed reviewers:
+ rdevlin.cronin@chromium.org, stevenjb@chromium.org

[tbarzic, 2016-12-07 18:43:59]

Looks scary, but it's mostly mechanical - renaming networking_private ->
network_config

more interesting parts are in:
* extensions/common 
* chrome/common/extensions/permissions/chrome_permission_message_rules.cc


Note: I'm missing histogram updates, I'll add that before submitting

[stevenjb, 2016-12-07 18:55:13]

On 2016/12/07 18:43:59, tbarzic wrote:
> Looks scary, but it's mostly mechanical - renaming networking_private ->
> network_config
> 
> more interesting parts are in:
> * extensions/common 
> * chrome/common/extensions/permissions/chrome_permission_message_rules.cc
> 
> 
> Note: I'm missing histogram updates, I'll add that before submitting

Could we create a separate issue to track this? I remember discussing this, but
I don't recall if there was an issue or doc where that discussion was logged?

I am still concerned that this change may cause some confusion down the road, so
I would like to document the transition as clearly as possible.

Also, did you consider initially creating a network_config -> networking_private
alias instead of vice versa? That way we could test and use the alias in advance
of renaming / moving all of these files.

[Devlin, 2016-12-07 19:39:11]

On 2016/12/07 18:55:13, stevenjb wrote:
> On 2016/12/07 18:43:59, tbarzic wrote:
> > Looks scary, but it's mostly mechanical - renaming networking_private ->
> > network_config
> > 
> > more interesting parts are in:
> > * extensions/common 
> > * chrome/common/extensions/permissions/chrome_permission_message_rules.cc
> > 
> > 
> > Note: I'm missing histogram updates, I'll add that before submitting
> 
> Could we create a separate issue to track this? I remember discussing this,
but
> I don't recall if there was an issue or doc where that discussion was logged?
> 
> I am still concerned that this change may cause some confusion down the road,
so
> I would like to document the transition as clearly as possible.
> 
> Also, did you consider initially creating a network_config ->
networking_private
> alias instead of vice versa? That way we could test and use the alias in
advance
> of renaming / moving all of these files.

+1; for now I'd much prefer we have networkingPrivate be the source.

[tbarzic, 2016-12-07 20:37:23]

Description was changed from

==========
Rename networkingPrivate API to networkConfig

Renames networkingPrivate API to networkConfig, and keeps
networkingPrivate API an an alias for created networkConfig API.

BUG=651488
==========

to

==========
Introduces alias networkConfig for networkingPrivate API.

Adds networkConfig permission, which guards the introduced
networkConfig alias - for now, the permission is bound to dev channel
and available to the same whitelist as networkingPrivate API.

To avoid duplicating whitelist for the two permissions, the whitelist
is moved to behavior features and the behavior feature is added as a
dependency for both of them.

This is the first step towards renaming networkingPrivate API to
networkConfig (which will be made available to kiosk sessions).

BUG=672186
==========

[tbarzic, 2016-12-07 21:24:25]

OK. Updated the set-up so networkConfig is an alias. Please take another look.

[stevenjb, 2016-12-07 21:35:31]

https://codereview.chromium.org/2470193002/diff/180001/chrome/common/extensio...
File chrome/common/extensions/permissions/chrome_permission_message_rules.cc
(right):

https://codereview.chromium.org/2470193002/diff/180001/chrome/common/extensio...
chrome/common/extensions/permissions/chrome_permission_message_rules.cc:527:
{}},
I'm not familiar with how these rules work, but it seems odd that
kNetworkingPrivate exists twice (lines 524 and 526). Maybe explain this?

https://codereview.chromium.org/2470193002/diff/180001/extensions/common/api/...
File extensions/common/api/_behavior_features.json (right):

https://codereview.chromium.org/2470193002/diff/180001/extensions/common/api/...
extensions/common/api/_behavior_features.json:94: "whitelisted_for_network_api":
{
network_config_api ?

https://codereview.chromium.org/2470193002/diff/180001/extensions/common/api/...
File extensions/common/api/_permission_features.json (right):

https://codereview.chromium.org/2470193002/diff/180001/extensions/common/api/...
extensions/common/api/_permission_features.json:317: "dependencies":
["behavior:whitelisted_for_network_api"],
I think having both of these us 'whitelisted_for_network_config_api' is
reasonable.

https://codereview.chromium.org/2470193002/diff/180001/extensions/common/feat...
File extensions/common/features/behavior_feature.h (right):

https://codereview.chromium.org/2470193002/diff/180001/extensions/common/feat...
extensions/common/features/behavior_feature.h:19: extern const char
kWhitelistedForNetworkApi[];
Here too. Because we (unfortunately) also have a "networking.config" API, I
think we should be explicit and use "NetworkConfigApi", with a comment somewhere
(maybe in the .json file?) that we are currently aliasing networkConfig ->
networkingPrivate, but eventually plan to migrate everything to networkConfig
and deprecate networkingPrivate.

[tbarzic, 2016-12-07 22:32:54]

https://codereview.chromium.org/2470193002/diff/180001/chrome/common/extensio...
File chrome/common/extensions/permissions/chrome_permission_message_rules.cc
(right):

https://codereview.chromium.org/2470193002/diff/180001/chrome/common/extensio...
chrome/common/extensions/permissions/chrome_permission_message_rules.cc:527:
{}},
On 2016/12/07 21:35:31, stevenjb wrote:
> I'm not familiar with how these rules work, but it seems odd that
> kNetworkingPrivate exists twice (lines 524 and 526). Maybe explain this?

Done.

https://codereview.chromium.org/2470193002/diff/180001/extensions/common/api/...
File extensions/common/api/_behavior_features.json (right):

https://codereview.chromium.org/2470193002/diff/180001/extensions/common/api/...
extensions/common/api/_behavior_features.json:94: "whitelisted_for_network_api":
{
On 2016/12/07 21:35:31, stevenjb wrote:
> network_config_api ?

Done. (though, might change depending on the API on which we eventually land)

https://codereview.chromium.org/2470193002/diff/180001/extensions/common/api/...
File extensions/common/api/_permission_features.json (right):

https://codereview.chromium.org/2470193002/diff/180001/extensions/common/api/...
extensions/common/api/_permission_features.json:317: "dependencies":
["behavior:whitelisted_for_network_api"],
On 2016/12/07 21:35:31, stevenjb wrote:
> I think having both of these us 'whitelisted_for_network_config_api' is
> reasonable.

Done.

https://codereview.chromium.org/2470193002/diff/180001/extensions/common/feat...
File extensions/common/features/behavior_feature.h (right):

https://codereview.chromium.org/2470193002/diff/180001/extensions/common/feat...
extensions/common/features/behavior_feature.h:19: extern const char
kWhitelistedForNetworkApi[];
On 2016/12/07 21:35:31, stevenjb wrote:
> Here too. Because we (unfortunately) also have a "networking.config" API, I
> think we should be explicit and use "NetworkConfigApi", with a comment
somewhere
> (maybe in the .json file?) that we are currently aliasing networkConfig ->
> networkingPrivate, but eventually plan to migrate everything to networkConfig
> and deprecate networkingPrivate.

Done.

[Devlin, 2016-12-09 18:23:20]

we should test this, since I don't believe we currently have any e2e tests for
aliasing.

https://codereview.chromium.org/2470193002/diff/220001/extensions/common/api/...
File extensions/common/api/_api_features.json (right):

https://codereview.chromium.org/2470193002/diff/220001/extensions/common/api/...
extensions/common/api/_api_features.json:308: "channel": "stable",
Do we need this?  It seems like for webui we should be able to do one sweep, so
only one of networking.onc and networkingPrivate should be available.

[tbarzic, 2016-12-09 18:51:32]

yes, I'm aware we have to test the alias - I'm working on it and plan to upload
tests during the day today :)

https://codereview.chromium.org/2470193002/diff/220001/extensions/common/api/...
File extensions/common/api/_api_features.json (right):

https://codereview.chromium.org/2470193002/diff/220001/extensions/common/api/...
extensions/common/api/_api_features.json:308: "channel": "stable",
On 2016/12/09 18:23:20, Devlin (in MON - PST plus 3) wrote:
> Do we need this?  It seems like for webui we should be able to do one sweep,
so
> only one of networking.onc and networkingPrivate should be available.

Sure, I can add this when we switch web UI to networking.onc

[tbarzic, 2016-12-09 20:24:01]

Description was changed from

==========
Introduces alias networkConfig for networkingPrivate API.

Adds networkConfig permission, which guards the introduced
networkConfig alias - for now, the permission is bound to dev channel
and available to the same whitelist as networkingPrivate API.

To avoid duplicating whitelist for the two permissions, the whitelist
is moved to behavior features and the behavior feature is added as a
dependency for both of them.

This is the first step towards renaming networkingPrivate API to
networkConfig (which will be made available to kiosk sessions).

BUG=672186
==========

to

==========
Introduces alias networking.onc for networkingPrivate API.

Adds networking.onc permission, which guards the introduced
networking.onc alias - for now, the permission is bound to dev channel
and available to the same whitelist as networkingPrivate API.

To avoid duplicating whitelist for the two permissions, the whitelist
is moved to behavior features and the behavior feature is added as a
dependency for both of them.

Refactors networking private API tests to enable parameterizing the
networking API to be used in the test and adds tests that exercise
alias - networking.onc code path.
Also groups some sub tests (which were all run in a separate browser
test) together in order to reduce total overhead of starting browser
for browser tests.

This is the first step towards renaming networkingPrivate API to
networking.onc (which will be made available to kiosk sessions).

BUG=672186
==========

[tbarzic, 2016-12-09 20:26:50]

The CQ bit was checked by tbarzic@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-09 20:27:30]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tbarzic, 2016-12-09 22:29:04]

added tests :)

[commit-bot: I haz the power, 2016-12-09 23:16:53]

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

[commit-bot: I haz the power, 2016-12-09 23:16:54]

Dry run: This issue passed the CQ dry run.

[stevenjb, 2016-12-09 23:29:04]

https://codereview.chromium.org/2470193002/diff/280001/chrome/browser/extensi...
File
chrome/browser/extensions/api/networking_private/networking_private_chromeos_apitest.cc
(right):

https://codereview.chromium.org/2470193002/diff/280001/chrome/browser/extensi...
chrome/browser/extensions/api/networking_private/networking_private_chromeos_apitest.cc:530:
EXPECT_TRUE(RunNetworkingTest("networkingPrivate", {"startActivate"}))
This is a lot of changes. Why not leave RunNetworkingSubtest as-is, and add
something like RunNetworkingSubtestWithAlias? I'm fine with a little bit of
re-factoring here, but this seems excessive (and will take me a while to
review...)

https://codereview.chromium.org/2470193002/diff/280001/chrome/test/data/exten...
File chrome/test/data/extensions/api_test/networking_private/chromeos/test.js
(right):

https://codereview.chromium.org/2470193002/diff/280001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/networking_private/chromeos/test.js:883: //
checked - in case of the failure it will contain components of the APi
s/the failure/failure/

https://codereview.chromium.org/2470193002/diff/280001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/networking_private/chromeos/test.js:902:
console.error(config.customArg);
Debugging?

[tbarzic, 2016-12-10 02:04:32]

The CQ bit was checked by tbarzic@chromium.org to run a CQ dry run

[tbarzic, 2016-12-10 02:04:49]

https://codereview.chromium.org/2470193002/diff/280001/chrome/browser/extensi...
File
chrome/browser/extensions/api/networking_private/networking_private_chromeos_apitest.cc
(right):

https://codereview.chromium.org/2470193002/diff/280001/chrome/browser/extensi...
chrome/browser/extensions/api/networking_private/networking_private_chromeos_apitest.cc:530:
EXPECT_TRUE(RunNetworkingTest("networkingPrivate", {"startActivate"}))
On 2016/12/09 23:29:04, stevenjb wrote:
> This is a lot of changes. Why not leave RunNetworkingSubtest as-is, and add
> something like RunNetworkingSubtestWithAlias? I'm fine with a little bit of
> re-factoring here, but this seems excessive (and will take me a while to
> review...)

OK, I reduced the scope of the test changes here.

https://codereview.chromium.org/2470193002/diff/280001/chrome/test/data/exten...
File chrome/test/data/extensions/api_test/networking_private/chromeos/test.js
(right):

https://codereview.chromium.org/2470193002/diff/280001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/networking_private/chromeos/test.js:883: //
checked - in case of the failure it will contain components of the APi
On 2016/12/09 23:29:04, stevenjb wrote:
> s/the failure/failure/

Done.

https://codereview.chromium.org/2470193002/diff/280001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/networking_private/chromeos/test.js:902:
console.error(config.customArg);
On 2016/12/09 23:29:04, stevenjb wrote:
> Debugging?

Done.

[commit-bot: I haz the power, 2016-12-10 02:05:23]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-10 03:57:33]

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

[commit-bot: I haz the power, 2016-12-10 03:57:34]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[stevenjb, 2016-12-12 17:46:32]

chrome/ lgtm with a suggestion.

https://codereview.chromium.org/2470193002/diff/300001/chrome/browser/extensi...
File
chrome/browser/extensions/api/networking_private/networking_private_chromeos_apitest.cc
(right):

https://codereview.chromium.org/2470193002/diff/300001/chrome/browser/extensi...
chrome/browser/extensions/api/networking_private/networking_private_chromeos_apitest.cc:870:
}
I'm not really sure how valuable it is to test every api call with the alias,
and duplicating test code is not great. This is fine as is, but personally I
would take a subset of simpler tests and put them in a separate section to test
the alias, but I don't feel too strongly.

[tbarzic, 2016-12-12 22:35:44]

The CQ bit was checked by tbarzic@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-12 22:37:07]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tbarzic, 2016-12-12 23:00:39]

The CQ bit was checked by tbarzic@chromium.org to run a CQ dry run

[tbarzic, 2016-12-12 23:01:49]

https://codereview.chromium.org/2470193002/diff/300001/chrome/browser/extensi...
File
chrome/browser/extensions/api/networking_private/networking_private_chromeos_apitest.cc
(right):

https://codereview.chromium.org/2470193002/diff/300001/chrome/browser/extensi...
chrome/browser/extensions/api/networking_private/networking_private_chromeos_apitest.cc:870:
}
On 2016/12/12 17:46:32, stevenjb wrote:
> I'm not really sure how valuable it is to test every api call with the alias,
> and duplicating test code is not great. This is fine as is, but personally I
> would take a subset of simpler tests and put them in a separate section to
test
> the alias, but I don't feel too strongly.

yeah, good point.
I think additional problem was that tests testing the alias when the app has
only alias permission (networking.onc) was missing - I added that to the most
recent version.

[commit-bot: I haz the power, 2016-12-12 23:01:49]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Devlin, 2016-12-12 23:25:47]

looks pretty good, just one request and one question.

https://codereview.chromium.org/2470193002/diff/380001/chrome/test/data/exten...
File chrome/test/data/extensions/api_test/networking_private/alias/test.js
(right):

https://codereview.chromium.org/2470193002/diff/380001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/networking_private/alias/test.js:40: }
Can we add a test that this extension cannot access networkingPrivate?

https://codereview.chromium.org/2470193002/diff/380001/chrome/test/data/exten...
File chrome/test/data/extensions/api_test/networking_private/chromeos/test.js
(right):

https://codereview.chromium.org/2470193002/diff/380001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/networking_private/chromeos/test.js:14: //
Shorthand for using the networkingPrivate API.
Why these changes?

[tbarzic, 2016-12-12 23:55:42]

https://codereview.chromium.org/2470193002/diff/380001/chrome/test/data/exten...
File chrome/test/data/extensions/api_test/networking_private/alias/test.js
(right):

https://codereview.chromium.org/2470193002/diff/380001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/networking_private/alias/test.js:40: }
On 2016/12/12 23:25:47, Devlin wrote:
> Can we add a test that this extension cannot access networkingPrivate?

Done.

https://codereview.chromium.org/2470193002/diff/380001/chrome/test/data/exten...
File chrome/test/data/extensions/api_test/networking_private/chromeos/test.js
(right):

https://codereview.chromium.org/2470193002/diff/380001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/networking_private/chromeos/test.js:14: //
Shorthand for using the networkingPrivate API.
On 2016/12/12 23:25:47, Devlin wrote:
> Why these changes?

I initially planned to use this tests for both networkingPrivate and
networking.onc, but backtracked - this is leftover from that approach - I'll
revert this.

[Devlin, 2016-12-13 15:54:01]

lgtm

https://codereview.chromium.org/2470193002/diff/400001/chrome/browser/extensi...
File
chrome/browser/extensions/api/networking_private/networking_private_chromeos_apitest.cc
(left):

https://codereview.chromium.org/2470193002/diff/400001/chrome/browser/extensi...
chrome/browser/extensions/api/networking_private/networking_private_chromeos_apitest.cc:562:
// TODO(stevenjb): Find a better way to set this up on Chrome OS.
nit: Is this change still necessary?

https://codereview.chromium.org/2470193002/diff/400001/chrome/browser/extensi...
File
chrome/browser/extensions/api/networking_private/networking_private_chromeos_apitest.cc
(right):

https://codereview.chromium.org/2470193002/diff/400001/chrome/browser/extensi...
chrome/browser/extensions/api/networking_private/networking_private_chromeos_apitest.cc:171:
bool RunNetworkingSubtest(const std::string& test) {
nit: is this change still necessary?

https://codereview.chromium.org/2470193002/diff/400001/extensions/common/perm...
File extensions/common/permissions/extensions_api_permissions.cc (right):

https://codereview.chromium.org/2470193002/diff/400001/extensions/common/perm...
extensions/common/permissions/extensions_api_permissions.cc:68:
APIPermissionInfo::kFlagCannotBeOptional},
Why not optional?

[tbarzic, 2016-12-13 17:40:02]

https://codereview.chromium.org/2470193002/diff/400001/chrome/browser/extensi...
File
chrome/browser/extensions/api/networking_private/networking_private_chromeos_apitest.cc
(left):

https://codereview.chromium.org/2470193002/diff/400001/chrome/browser/extensi...
chrome/browser/extensions/api/networking_private/networking_private_chromeos_apitest.cc:562:
// TODO(stevenjb): Find a better way to set this up on Chrome OS.
On 2016/12/13 15:54:01, Devlin wrote:
> nit: Is this change still necessary?

not really

https://codereview.chromium.org/2470193002/diff/400001/chrome/browser/extensi...
File
chrome/browser/extensions/api/networking_private/networking_private_chromeos_apitest.cc
(right):

https://codereview.chromium.org/2470193002/diff/400001/chrome/browser/extensi...
chrome/browser/extensions/api/networking_private/networking_private_chromeos_apitest.cc:171:
bool RunNetworkingSubtest(const std::string& test) {
On 2016/12/13 15:54:01, Devlin wrote:
> nit: is this change still necessary?

not strictly necessary, but it is little cleaner way of passing arguments to api
tests.

https://codereview.chromium.org/2470193002/diff/400001/extensions/common/perm...
File extensions/common/permissions/extensions_api_permissions.cc (right):

https://codereview.chromium.org/2470193002/diff/400001/extensions/common/perm...
extensions/common/permissions/extensions_api_permissions.cc:68:
APIPermissionInfo::kFlagCannotBeOptional},
On 2016/12/13 15:54:01, Devlin wrote:
> Why not optional?

because networkingPrivate is the same way.

[Devlin, 2016-12-13 17:44:29]

https://codereview.chromium.org/2470193002/diff/400001/extensions/common/perm...
File extensions/common/permissions/extensions_api_permissions.cc (right):

https://codereview.chromium.org/2470193002/diff/400001/extensions/common/perm...
extensions/common/permissions/extensions_api_permissions.cc:68:
APIPermissionInfo::kFlagCannotBeOptional},
On 2016/12/13 17:40:02, tbarzic wrote:
> On 2016/12/13 15:54:01, Devlin wrote:
> > Why not optional?
> 
> because networkingPrivate is the same way.

It's up for debate whether or not that should be, but having an optional private
permission is significantly weirder since it's designed to be used by component
extensions and webui primarily.  If this is going to be public, I'd say it
should be optional.

[tbarzic, 2016-12-13 17:52:28]

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

[tbarzic, 2016-12-13 17:52:29]

+isherman for histograms change

https://codereview.chromium.org/2470193002/diff/400001/extensions/common/perm...
File extensions/common/permissions/extensions_api_permissions.cc (right):

https://codereview.chromium.org/2470193002/diff/400001/extensions/common/perm...
extensions/common/permissions/extensions_api_permissions.cc:68:
APIPermissionInfo::kFlagCannotBeOptional},
On 2016/12/13 17:44:28, Devlin wrote:
> On 2016/12/13 17:40:02, tbarzic wrote:
> > On 2016/12/13 15:54:01, Devlin wrote:
> > > Why not optional?
> > 
> > because networkingPrivate is the same way.
> 
> It's up for debate whether or not that should be, but having an optional
private
> permission is significantly weirder since it's designed to be used by
component
> extensions and webui primarily.  If this is going to be public, I'd say it
> should be optional.

OK, Done.

[Ilya Sherman, 2016-12-13 21:14:41]

histograms.xml lgtm

[tbarzic, 2016-12-14 17:59:35]

The CQ bit was checked by tbarzic@chromium.org

[tbarzic, 2016-12-14 17:59:36]

The patchset sent to the CQ was uploaded after l-g-t-m from
stevenjb@chromium.org, rdevlin.cronin@chromium.org
Link to the patchset: https://codereview.chromium.org/2470193002/#ps460001
(title: "allow optional permission")

[commit-bot: I haz the power, 2016-12-14 17:59:59]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-14 19:12:39]

CQ is committing da patch.

Bot data: {"patchset_id": 460001, "attempt_start_ts": 1481738375860980,
"parent_rev": "e7da7097d84f3b7ae9c4bfaccd193c283f01c8fa", "commit_rev":
"34c8e6107af9c42f1ce80c9b8ffc2ea243797008"}

[commit-bot: I haz the power, 2016-12-14 19:13:11]

Description was changed from

==========
Introduces alias networking.onc for networkingPrivate API.

Adds networking.onc permission, which guards the introduced
networking.onc alias - for now, the permission is bound to dev channel
and available to the same whitelist as networkingPrivate API.

To avoid duplicating whitelist for the two permissions, the whitelist
is moved to behavior features and the behavior feature is added as a
dependency for both of them.

Refactors networking private API tests to enable parameterizing the
networking API to be used in the test and adds tests that exercise
alias - networking.onc code path.
Also groups some sub tests (which were all run in a separate browser
test) together in order to reduce total overhead of starting browser
for browser tests.

This is the first step towards renaming networkingPrivate API to
networking.onc (which will be made available to kiosk sessions).

BUG=672186
==========

to

==========
Introduces alias networking.onc for networkingPrivate API.

Adds networking.onc permission, which guards the introduced
networking.onc alias - for now, the permission is bound to dev channel
and available to the same whitelist as networkingPrivate API.

To avoid duplicating whitelist for the two permissions, the whitelist
is moved to behavior features and the behavior feature is added as a
dependency for both of them.

Refactors networking private API tests to enable parameterizing the
networking API to be used in the test and adds tests that exercise
alias - networking.onc code path.
Also groups some sub tests (which were all run in a separate browser
test) together in order to reduce total overhead of starting browser
for browser tests.

This is the first step towards renaming networkingPrivate API to
networking.onc (which will be made available to kiosk sessions).

BUG=672186

Review-Url: https://codereview.chromium.org/2470193002
==========

[commit-bot: I haz the power, 2016-12-14 19:13:13]

Committed patchset #24 (id:460001)

[commit-bot: I haz the power, 2016-12-14 19:16:53]

Description was changed from

==========
Introduces alias networking.onc for networkingPrivate API.

Adds networking.onc permission, which guards the introduced
networking.onc alias - for now, the permission is bound to dev channel
and available to the same whitelist as networkingPrivate API.

To avoid duplicating whitelist for the two permissions, the whitelist
is moved to behavior features and the behavior feature is added as a
dependency for both of them.

Refactors networking private API tests to enable parameterizing the
networking API to be used in the test and adds tests that exercise
alias - networking.onc code path.
Also groups some sub tests (which were all run in a separate browser
test) together in order to reduce total overhead of starting browser
for browser tests.

This is the first step towards renaming networkingPrivate API to
networking.onc (which will be made available to kiosk sessions).

BUG=672186

Review-Url: https://codereview.chromium.org/2470193002
==========

to

==========
Introduces alias networking.onc for networkingPrivate API.

Adds networking.onc permission, which guards the introduced
networking.onc alias - for now, the permission is bound to dev channel
and available to the same whitelist as networkingPrivate API.

To avoid duplicating whitelist for the two permissions, the whitelist
is moved to behavior features and the behavior feature is added as a
dependency for both of them.

Refactors networking private API tests to enable parameterizing the
networking API to be used in the test and adds tests that exercise
alias - networking.onc code path.
Also groups some sub tests (which were all run in a separate browser
test) together in order to reduce total overhead of starting browser
for browser tests.

This is the first step towards renaming networkingPrivate API to
networking.onc (which will be made available to kiosk sessions).

BUG=672186

Committed: https://crrev.com/975d838eca97d3cac1578861c9df42fe46e2a9d5
Cr-Commit-Position: refs/heads/master@{#438574}
==========

[commit-bot: I haz the power, 2016-12-14 19:16:55]

Patchset 24 (id:??) landed as
https://crrev.com/975d838eca97d3cac1578861c9df42fe46e2a9d5
Cr-Commit-Position: refs/heads/master@{#438574}