Add getGlobalPolicy to networkingPrivate API.

Add getGlobalPolicy to networkingPrivate API.

This also updates the chrome://network test page (network_ui) to
show the global policy properties.

BUG=678786
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2620463003
Cr-Commit-Position: refs/heads/master@{#442398}
Committed: https://chromium.googlesource.com/chromium/src/+/8a48016a1de004b6fd1d4d98c11e8e03e975056d

[stevenjb, 2017-01-06 18:46:23]

Description was changed from

==========
Add getGlobalPolicy to networkingPrivate API.

This also updates the chrome://network test page (network_ui) to
show the global policy properties.

BUG=678786
==========

to

==========
Add getGlobalPolicy to networkingPrivate API.

This also updates the chrome://network test page (network_ui) to
show the global policy properties.

BUG=678786
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[stevenjb, 2017-01-06 21:21:36]

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

[stevenjb, 2017-01-06 21:21:37]

tbarzic@ - I know you are looking into slimming this API down, but we need this
addition for Settings, and it makes the most sense to me to make this part of
networkingPrivate/networking.onc since it is configuration related.

rdevlin.cronin@ for extensions/

[tbarzic, 2017-01-06 22:43:18]

yeah, I think this would make sense for networkingPrivate

https://codereview.chromium.org/2620463003/diff/1/chrome/browser/extensions/a...
File
chrome/browser/extensions/api/networking_private/networking_private_apitest.cc
(right):

https://codereview.chromium.org/2620463003/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/networking_private/networking_private_apitest.cc:188:
result->SetBooleanWithoutPathExpansion(
I'd add an extra property to the policy dict - to ensure API handles that case
well.

https://codereview.chromium.org/2620463003/diff/1/chrome/test/data/extensions...
File chrome/test/data/extensions/api_test/networking_private/test.js (right):

https://codereview.chromium.org/2620463003/diff/1/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/networking_private/test.js:133: return
JSON.stringify(result) == JSON.stringify({
assertEq() should work here as well

https://codereview.chromium.org/2620463003/diff/1/extensions/browser/api/netw...
File extensions/browser/api/networking_private/networking_private_api.cc
(right):

https://codereview.chromium.org/2620463003/diff/1/extensions/browser/api/netw...
extensions/browser/api/networking_private/networking_private_api.cc:793: //
private_api::GlobalPolicy is a subset of the global policy dictionary
I think explicitly setting |policy| properties might be better here
(mainly so there's no dependency on internals of GlobalPolicy::FromValue), but I
could live with this.

https://codereview.chromium.org/2620463003/diff/1/extensions/browser/api/netw...
extensions/browser/api/networking_private/networking_private_api.cc:796:
private_api::GlobalPolicy::FromValue(*policy_dict.get()));
nit: .get() should not be needed.

https://codereview.chromium.org/2620463003/diff/1/extensions/browser/api/netw...
extensions/browser/api/networking_private/networking_private_api.cc:797: return
RespondNow(OneArgument(policy->ToValue()));
suggestion:
RespondNow(ArgumentList(
    private_api::GetGlobalPolicy::Results::Create(*policy)));

https://codereview.chromium.org/2620463003/diff/1/extensions/browser/api/netw...
File extensions/browser/api/networking_private/networking_private_chromeos.cc
(right):

https://codereview.chromium.org/2620463003/diff/1/extensions/browser/api/netw...
extensions/browser/api/networking_private/networking_private_chromeos.cc:680:
GetManagedConfigurationHandler()->GetGlobalConfigFromPolicy(
Why not return |global_network_config directly| and use that dict to create API
method result (to avoid creating extra copy of the dict)?

https://codereview.chromium.org/2620463003/diff/1/extensions/browser/api/netw...
File extensions/browser/api/networking_private/networking_private_linux.cc
(right):

https://codereview.chromium.org/2620463003/diff/1/extensions/browser/api/netw...
extensions/browser/api/networking_private/networking_private_linux.cc:595:
callback.Run(base::MakeUnique<base::DictionaryValue>());
return base::MakeUnique...

https://codereview.chromium.org/2620463003/diff/1/extensions/common/api/netwo...
File extensions/common/api/networking_private.idl (right):

https://codereview.chromium.org/2620463003/diff/1/extensions/common/api/netwo...
extensions/common/api/networking_private.idl:810: // If true, only allow policy
networks to autoconnect. Defaults to false.
Since this describes state of the system, rather than a function arg, "only
allow" seems a bit misleading.
How about:
If true, only policy configured networks can auto-connect.

https://codereview.chromium.org/2620463003/diff/1/extensions/common/api/netwo...
extensions/common/api/networking_private.idl:814: // added. Defaults to false.
I think "configured" would be clearer than "added".

https://codereview.chromium.org/2620463003/diff/1/extensions/common/api/netwo...
extensions/common/api/networking_private.idl:1048: // Get the global policy
properties. These properties are not expected to
suggestion:
s/Get/Gets/
s/are not expected to change during a session/are expected to be constant within
a session/

[stevenjb, 2017-01-09 19:30:36]

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

[stevenjb, 2017-01-09 19:30:37]

PTAL

+isherman@chromium.org for histograms

https://codereview.chromium.org/2620463003/diff/1/chrome/browser/extensions/a...
File
chrome/browser/extensions/api/networking_private/networking_private_apitest.cc
(right):

https://codereview.chromium.org/2620463003/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/networking_private/networking_private_apitest.cc:188:
result->SetBooleanWithoutPathExpansion(
On 2017/01/06 22:43:17, tbarzic wrote:
> I'd add an extra property to the policy dict - to ensure API handles that case
> well.
Good call, but I will actually do that in the chromeos specific test. I am going
to make this just return an empty dictionary and remove the trivial api test for
consistency. We should really either make the rest of these delegate methods
trivial, or get assertEq working and actually test the return values for the
entire API.

https://codereview.chromium.org/2620463003/diff/1/chrome/test/data/extensions...
File chrome/test/data/extensions/api_test/networking_private/test.js (right):

https://codereview.chromium.org/2620463003/diff/1/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/networking_private/test.js:133: return
JSON.stringify(result) == JSON.stringify({
On 2017/01/06 22:43:17, tbarzic wrote:
> assertEq() should work here as well

It doesn't actually, I don't recall why. I'm going to actually remove this to be
consistent with other tests in this file, and simplify the test delegate also
(see previous comment). We have coverage in the chromeos specific test.

https://codereview.chromium.org/2620463003/diff/1/extensions/browser/api/netw...
File extensions/browser/api/networking_private/networking_private_api.cc
(right):

https://codereview.chromium.org/2620463003/diff/1/extensions/browser/api/netw...
extensions/browser/api/networking_private/networking_private_api.cc:793: //
private_api::GlobalPolicy is a subset of the global policy dictionary
On 2017/01/06 22:43:17, tbarzic wrote:
> I think explicitly setting |policy| properties might be better here
> (mainly so there's no dependency on internals of GlobalPolicy::FromValue), but
I
> could live with this.

That would effectively be duplicating the code in GlobalPolicy::FromValue, and
would have to be kept in sync. This way if we add a new policy, we can just add
it to the IDL.

https://codereview.chromium.org/2620463003/diff/1/extensions/browser/api/netw...
extensions/browser/api/networking_private/networking_private_api.cc:796:
private_api::GlobalPolicy::FromValue(*policy_dict.get()));
On 2017/01/06 22:43:17, tbarzic wrote:
> nit: .get() should not be needed.

Done.

https://codereview.chromium.org/2620463003/diff/1/extensions/browser/api/netw...
extensions/browser/api/networking_private/networking_private_api.cc:797: return
RespondNow(OneArgument(policy->ToValue()));
On 2017/01/06 22:43:17, tbarzic wrote:
> suggestion:
> RespondNow(ArgumentList(
>     private_api::GetGlobalPolicy::Results::Create(*policy)));

Done.

https://codereview.chromium.org/2620463003/diff/1/extensions/browser/api/netw...
File extensions/browser/api/networking_private/networking_private_chromeos.cc
(right):

https://codereview.chromium.org/2620463003/diff/1/extensions/browser/api/netw...
extensions/browser/api/networking_private/networking_private_chromeos.cc:680:
GetManagedConfigurationHandler()->GetGlobalConfigFromPolicy(
On 2017/01/06 22:43:18, tbarzic wrote:
> Why not return |global_network_config directly| and use that dict to create
API
> method result (to avoid creating extra copy of the dict)?

GetGlobalConfigFromPolicy returns a const*, so we have to do a deep copy anyway.
This way we don't have to handle null results in
NetworkingPrivateGetGlobalPolicyFunction::Run.

https://codereview.chromium.org/2620463003/diff/1/extensions/browser/api/netw...
File extensions/browser/api/networking_private/networking_private_linux.cc
(right):

https://codereview.chromium.org/2620463003/diff/1/extensions/browser/api/netw...
extensions/browser/api/networking_private/networking_private_linux.cc:595:
callback.Run(base::MakeUnique<base::DictionaryValue>());
On 2017/01/06 22:43:18, tbarzic wrote:
> return base::MakeUnique...

Oops, thanks. Done.

https://codereview.chromium.org/2620463003/diff/1/extensions/common/api/netwo...
File extensions/common/api/networking_private.idl (right):

https://codereview.chromium.org/2620463003/diff/1/extensions/common/api/netwo...
extensions/common/api/networking_private.idl:810: // If true, only allow policy
networks to autoconnect. Defaults to false.
On 2017/01/06 22:43:18, tbarzic wrote:
> Since this describes state of the system, rather than a function arg, "only
> allow" seems a bit misleading.
> How about:
> If true, only policy configured networks can auto-connect.

Done.

https://codereview.chromium.org/2620463003/diff/1/extensions/common/api/netwo...
extensions/common/api/networking_private.idl:814: // added. Defaults to false.
On 2017/01/06 22:43:18, tbarzic wrote:
> I think "configured" would be clearer than "added".

Done.

https://codereview.chromium.org/2620463003/diff/1/extensions/common/api/netwo...
extensions/common/api/networking_private.idl:1048: // Get the global policy
properties. These properties are not expected to
On 2017/01/06 22:43:18, tbarzic wrote:
> suggestion:
> s/Get/Gets/
> s/are not expected to change during a session/are expected to be constant
within
> a session/
Fixed Gets. 'Expected to be constant' sounds more like a code comment than an
API comment to me. *shrug*

[stevenjb, 2017-01-09 19:30:43]

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

[commit-bot: I haz the power, 2017-01-09 19:32:50]

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, 2017-01-09 20:03:36]

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

[commit-bot: I haz the power, 2017-01-09 20:03:37]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[tbarzic, 2017-01-09 20:24:30]

https://codereview.chromium.org/2620463003/diff/1/extensions/browser/api/netw...
File extensions/browser/api/networking_private/networking_private_chromeos.cc
(right):

https://codereview.chromium.org/2620463003/diff/1/extensions/browser/api/netw...
extensions/browser/api/networking_private/networking_private_chromeos.cc:680:
GetManagedConfigurationHandler()->GetGlobalConfigFromPolicy(
On 2017/01/09 19:30:37, stevenjb wrote:
> On 2017/01/06 22:43:18, tbarzic wrote:
> > Why not return |global_network_config directly| and use that dict to create
> API
> > method result (to avoid creating extra copy of the dict)?
> 
> GetGlobalConfigFromPolicy returns a const*, so we have to do a deep copy
anyway.
> This way we don't have to handle null results in
> NetworkingPrivateGetGlobalPolicyFunction::Run.

Wouldn't something like the following avoid this extra copy:
{
  const base::DictionaryValue* policy_value = GetGlobalPolicy();
  private_api::GlobalPolicy global_policy;
  if (policy_value) {
    if (!private_api::GlobalPolicy::Populate(*policy_value, &global_policy))
      LOG(ERROR) << "Failed to parse policy";
  }
  return RespondNow(ArgumentList(
      private_api::GetGlobalPolicy::Results::Create(global_policy)));
}

either way, up to you.

https://codereview.chromium.org/2620463003/diff/20001/extensions/browser/api/...
File
extensions/browser/api/networking_private/networking_private_service_client.cc
(right):

https://codereview.chromium.org/2620463003/diff/20001/extensions/browser/api/...
extensions/browser/api/networking_private/networking_private_service_client.cc:358:
callback.Run(base::MakeUnique<base::DictionaryValue>());
this should return unique_ptr, too :)

[Ilya Sherman, 2017-01-09 20:40:17]

histograms.xml lgtm

[stevenjb, 2017-01-09 20:43:34]

https://codereview.chromium.org/2620463003/diff/1/extensions/browser/api/netw...
File extensions/browser/api/networking_private/networking_private_chromeos.cc
(right):

https://codereview.chromium.org/2620463003/diff/1/extensions/browser/api/netw...
extensions/browser/api/networking_private/networking_private_chromeos.cc:680:
GetManagedConfigurationHandler()->GetGlobalConfigFromPolicy(
On 2017/01/09 20:24:30, tbarzic wrote:
> On 2017/01/09 19:30:37, stevenjb wrote:
> > On 2017/01/06 22:43:18, tbarzic wrote:
> > > Why not return |global_network_config directly| and use that dict to
create
> > API
> > > method result (to avoid creating extra copy of the dict)?
> > 
> > GetGlobalConfigFromPolicy returns a const*, so we have to do a deep copy
> anyway.
> > This way we don't have to handle null results in
> > NetworkingPrivateGetGlobalPolicyFunction::Run.
> 
> Wouldn't something like the following avoid this extra copy:
> {
>   const base::DictionaryValue* policy_value = GetGlobalPolicy();
>   private_api::GlobalPolicy global_policy;
>   if (policy_value) {
>     if (!private_api::GlobalPolicy::Populate(*policy_value, &global_policy))

We can't call RespondNow from the delegate, and I would prefer not to introduce
additional dependencies on the API details to the delegae (i.e. by returning
std::unique_ptr<GlobalPolicy>) even if it means an extra copy here (relatively
trivial all things considered).

https://codereview.chromium.org/2620463003/diff/20001/extensions/browser/api/...
File
extensions/browser/api/networking_private/networking_private_service_client.cc
(right):

https://codereview.chromium.org/2620463003/diff/20001/extensions/browser/api/...
extensions/browser/api/networking_private/networking_private_service_client.cc:358:
callback.Run(base::MakeUnique<base::DictionaryValue>());
On 2017/01/09 20:24:30, tbarzic wrote:
> this should return unique_ptr, too :)

Done.

[stevenjb, 2017-01-09 20:43:42]

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

[commit-bot: I haz the power, 2017-01-09 20:44:14]

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

[tbarzic, 2017-01-09 21:11:07]

https://codereview.chromium.org/2620463003/diff/1/extensions/browser/api/netw...
File extensions/browser/api/networking_private/networking_private_chromeos.cc
(right):

https://codereview.chromium.org/2620463003/diff/1/extensions/browser/api/netw...
extensions/browser/api/networking_private/networking_private_chromeos.cc:680:
GetManagedConfigurationHandler()->GetGlobalConfigFromPolicy(
On 2017/01/09 20:43:34, stevenjb wrote:
> On 2017/01/09 20:24:30, tbarzic wrote:
> > On 2017/01/09 19:30:37, stevenjb wrote:
> > > On 2017/01/06 22:43:18, tbarzic wrote:
> > > > Why not return |global_network_config directly| and use that dict to
> create
> > > API
> > > > method result (to avoid creating extra copy of the dict)?
> > > 
> > > GetGlobalConfigFromPolicy returns a const*, so we have to do a deep copy
> > anyway.
> > > This way we don't have to handle null results in
> > > NetworkingPrivateGetGlobalPolicyFunction::Run.
> > 
> > Wouldn't something like the following avoid this extra copy:
> > {
> >   const base::DictionaryValue* policy_value = GetGlobalPolicy();
> >   private_api::GlobalPolicy global_policy;
> >   if (policy_value) {
> >     if (!private_api::GlobalPolicy::Populate(*policy_value, &global_policy))
> 
> We can't call RespondNow from the delegate, and I would prefer not to
introduce
> additional dependencies on the API details to the delegae (i.e. by returning
> std::unique_ptr<GlobalPolicy>) even if it means an extra copy here (relatively
> trivial all things considered).

That snippet would be networking_private_api.cc code in case the delegate just
returned GetManagedConfigurationHandler()->GetGlobalConfigFromPolicy(); (and the
non Chrome OS delegate could just return nullptr).

[stevenjb, 2017-01-09 21:17:09]

On 2017/01/09 21:11:07, tbarzic wrote:
>
https://codereview.chromium.org/2620463003/diff/1/extensions/browser/api/netw...
> File extensions/browser/api/networking_private/networking_private_chromeos.cc
> (right):
> 
>
https://codereview.chromium.org/2620463003/diff/1/extensions/browser/api/netw...
> extensions/browser/api/networking_private/networking_private_chromeos.cc:680:
> GetManagedConfigurationHandler()->GetGlobalConfigFromPolicy(
> On 2017/01/09 20:43:34, stevenjb wrote:
> > On 2017/01/09 20:24:30, tbarzic wrote:
> > > On 2017/01/09 19:30:37, stevenjb wrote:
> > > > On 2017/01/06 22:43:18, tbarzic wrote:
> > > > > Why not return |global_network_config directly| and use that dict to
> > create
> > > > API
> > > > > method result (to avoid creating extra copy of the dict)?
> > > > 
> > > > GetGlobalConfigFromPolicy returns a const*, so we have to do a deep copy
> > > anyway.
> > > > This way we don't have to handle null results in
> > > > NetworkingPrivateGetGlobalPolicyFunction::Run.
> > > 
> > > Wouldn't something like the following avoid this extra copy:
> > > {
> > >   const base::DictionaryValue* policy_value = GetGlobalPolicy();
> > >   private_api::GlobalPolicy global_policy;
> > >   if (policy_value) {
> > >     if (!private_api::GlobalPolicy::Populate(*policy_value,
&global_policy))
> > 
> > We can't call RespondNow from the delegate, and I would prefer not to
> introduce
> > additional dependencies on the API details to the delegae (i.e. by returning
> > std::unique_ptr<GlobalPolicy>) even if it means an extra copy here
(relatively
> > trivial all things considered).
> 
> That snippet would be networking_private_api.cc code in case the delegate just
> returned GetManagedConfigurationHandler()->GetGlobalConfigFromPolicy(); (and
the
> non Chrome OS delegate could just return nullptr).

Yes, we could do it that way also. I still prefer this way (see my first reply).
Do you strongly prefer that, or are you still fine either way?

[tbarzic, 2017-01-09 21:24:32]

I'd prefer that approach (I don't think there's much difference in handling
nullptr policy in networking_private_api or here, in delegate); but I'm fine
with this..

LGTM

https://codereview.chromium.org/2620463003/diff/40001/extensions/browser/api/...
File extensions/browser/api/networking_private/networking_private_api.cc
(right):

https://codereview.chromium.org/2620463003/diff/40001/extensions/browser/api/...
extensions/browser/api/networking_private/networking_private_api.cc:796:
private_api::GlobalPolicy::FromValue(*policy_dict));
DCHECK policy is not null?

[stevenjb, 2017-01-09 21:48:49]

https://codereview.chromium.org/2620463003/diff/40001/extensions/browser/api/...
File extensions/browser/api/networking_private/networking_private_api.cc
(right):

https://codereview.chromium.org/2620463003/diff/40001/extensions/browser/api/...
extensions/browser/api/networking_private/networking_private_api.cc:796:
private_api::GlobalPolicy::FromValue(*policy_dict));
On 2017/01/09 21:24:32, tbarzic wrote:
> DCHECK policy is not null?

Done.

[stevenjb, 2017-01-09 21:48:54]

The CQ bit was checked by stevenjb@chromium.org

[stevenjb, 2017-01-09 21:48:54]

The patchset sent to the CQ was uploaded after l-g-t-m from
isherman@chromium.org, tbarzic@chromium.org
Link to the patchset: https://codereview.chromium.org/2620463003/#ps60001
(title: "Add dcheck")

[commit-bot: I haz the power, 2017-01-09 21:49:20]

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, 2017-01-09 23:26:08]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1483998534071750,
"parent_rev": "8819858e6747aae06f155f04909b458ee782538c", "commit_rev":
"8a48016a1de004b6fd1d4d98c11e8e03e975056d"}

[commit-bot: I haz the power, 2017-01-09 23:26:41]

Description was changed from

==========
Add getGlobalPolicy to networkingPrivate API.

This also updates the chrome://network test page (network_ui) to
show the global policy properties.

BUG=678786
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Add getGlobalPolicy to networkingPrivate API.

This also updates the chrome://network test page (network_ui) to
show the global policy properties.

BUG=678786
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2620463003
Cr-Commit-Position: refs/heads/master@{#442398}
Committed:
https://chromium.googlesource.com/chromium/src/+/8a48016a1de004b6fd1d4d98c11e...
==========

[commit-bot: I haz the power, 2017-01-09 23:26:43]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/8a48016a1de004b6fd1d4d98c11e...

[tbarzic, 2017-01-09 23:52:45]

On 2017/01/09 23:26:43, commit-bot: I haz the power wrote:
> Committed patchset #4 (id:60001) as
>
https://chromium.googlesource.com/chromium/src/+/8a48016a1de004b6fd1d4d98c11e...

wait, how did this get through the cq? Isn't a review from an owner for
extensions/common/api/networking_private.idl missing?

[stevenjb, 2017-01-10 00:10:44]

On 2017/01/09 23:52:45, tbarzic wrote:
> On 2017/01/09 23:26:43, commit-bot: I haz the power wrote:
> > Committed patchset #4 (id:60001) as
> >
>
https://chromium.googlesource.com/chromium/src/+/8a48016a1de004b6fd1d4d98c11e...
> 
> wait, how did this get through the cq? Isn't a review from an owner for
> extensions/common/api/networking_private.idl missing?

Um... good question. I missed that Devlin hadn't reviewed this one yet, sorry...
quick post humorous review, Devlin?

I filed crbug.com/679531 for the CQ mystery.

[Devlin, 2017-01-10 15:25:22]

https://codereview.chromium.org/2620463003/diff/60001/chrome/browser/resource...
File chrome/browser/resources/chromeos/network_ui/network_ui.js (right):

https://codereview.chromium.org/2620463003/diff/60001/chrome/browser/resource...
chrome/browser/resources/chromeos/network_ui/network_ui.js:321:
document.querySelector('#global-policy').textContent =
Why document.querySelector('#foo') instead of $('foo')?

https://codereview.chromium.org/2620463003/diff/60001/extensions/browser/api/...
File extensions/browser/api/networking_private/networking_private_delegate.h
(right):

https://codereview.chromium.org/2620463003/diff/60001/extensions/browser/api/...
extensions/browser/api/networking_private/networking_private_delegate.h:186:
virtual std::unique_ptr<base::DictionaryValue> GetGlobalPolicy() = 0;
It looks like this is only used in the new API function, which only needs it to
get a const&.  Can we return a const DictionaryValue* to avoid the copy?

https://codereview.chromium.org/2620463003/diff/60001/extensions/common/api/n...
File extensions/common/api/networking_private.idl (right):

https://codereview.chromium.org/2620463003/diff/60001/extensions/common/api/n...
extensions/common/api/networking_private.idl:810: // If true, only policy
networks may auto connect. Defaults to false.
"policy networks" sounds strange.  Is this something like "networks specified by
enterprise policy"?  Or something else?

https://codereview.chromium.org/2620463003/diff/60001/extensions/common/api/n...
extensions/common/api/networking_private.idl:811: boolean?
AllowOnlyPolicyNetworksToAutoconnect;
These should be javaScriptCased.  Admittedly, this whole file is inconsistent,
but may as well choose in the direction of what it should be.

https://codereview.chromium.org/2620463003/diff/60001/extensions/common/api/n...
extensions/common/api/networking_private.idl:813: // If true, only policy
networks may be connected to and no new networks may
ditto re policy networks
"may be connected to" is also a little strange.  Maybe "users can only connect
to ..."

https://codereview.chromium.org/2620463003/diff/60001/extensions/common/api/n...
extensions/common/api/networking_private.idl:815: boolean?
AllowOnlyPolicyNetworksToConnect;
for this and above, we call them "AllowOnlyPolicyNetworksTo*" - is it the
network that's doing *, or the user/profile/something else?  Should this instead
be something like "OnlyAllowConnectionsToPolicyNetworks",
"OnlyAllowAutoconnectToPolicyNetworks"?

[Devlin, 2017-01-10 15:26:01]

also, I only did a drive-by on resources/webui.  It might be worth adding one of
dbeam, dpapad, michaelpg et al if you wanted another pair of eyes on it.

[stevenjb, 2017-01-10 17:49:39]

https://codereview.chromium.org/2620463003/diff/60001/chrome/browser/resource...
File chrome/browser/resources/chromeos/network_ui/network_ui.js (right):

https://codereview.chromium.org/2620463003/diff/60001/chrome/browser/resource...
chrome/browser/resources/chromeos/network_ui/network_ui.js:321:
document.querySelector('#global-policy').textContent =
On 2017/01/10 15:25:21, Devlin wrote:
> Why document.querySelector('#foo') instead of $('foo')?

No particular reason, I just copied from somewhere that used
document.querySelector, and I forgot about $ (which we don't use much in Polymer
code).

https://codereview.chromium.org/2620463003/diff/60001/extensions/browser/api/...
File extensions/browser/api/networking_private/networking_private_delegate.h
(right):

https://codereview.chromium.org/2620463003/diff/60001/extensions/browser/api/...
extensions/browser/api/networking_private/networking_private_delegate.h:186:
virtual std::unique_ptr<base::DictionaryValue> GetGlobalPolicy() = 0;
On 2017/01/10 15:25:22, Devlin wrote:
> It looks like this is only used in the new API function, which only needs it
to
> get a const&.  Can we return a const DictionaryValue* to avoid the copy?

I guess my main objections to passing const DictionaryValue* is that it would be
inconsistent with the other delegate methods. Also, if we later merge properties
from other sources in the future, the delegate code becomes slightly awkward.

https://codereview.chromium.org/2620463003/diff/60001/extensions/common/api/n...
File extensions/common/api/networking_private.idl (right):

https://codereview.chromium.org/2620463003/diff/60001/extensions/common/api/n...
extensions/common/api/networking_private.idl:810: // If true, only policy
networks may auto connect. Defaults to false.
On 2017/01/10 15:25:22, Devlin wrote:
> "policy networks" sounds strange.  Is this something like "networks specified
by
> enterprise policy"?  Or something else?

It is  "networks specified by enterprise policy", and we use the term pretty
liberally in the code.

https://codereview.chromium.org/2620463003/diff/60001/extensions/common/api/n...
extensions/common/api/networking_private.idl:811: boolean?
AllowOnlyPolicyNetworksToAutoconnect;
On 2017/01/10 15:25:22, Devlin wrote:
> These should be javaScriptCased.  Admittedly, this whole file is inconsistent,
> but may as well choose in the direction of what it should be.

See comment at top of file.

https://codereview.chromium.org/2620463003/diff/60001/extensions/common/api/n...
extensions/common/api/networking_private.idl:813: // If true, only policy
networks may be connected to and no new networks may
On 2017/01/10 15:25:22, Devlin wrote:
> ditto re policy networks
> "may be connected to" is also a little strange.  Maybe "users can only connect
> to ..."

It's a permission so "may" is *technically* correct...

https://codereview.chromium.org/2620463003/diff/60001/extensions/common/api/n...
extensions/common/api/networking_private.idl:815: boolean?
AllowOnlyPolicyNetworksToConnect;
On 2017/01/10 15:25:22, Devlin wrote:
> for this and above, we call them "AllowOnlyPolicyNetworksTo*" - is it the
> network that's doing *, or the user/profile/something else?  Should this
instead
> be something like "OnlyAllowConnectionsToPolicyNetworks",
> "OnlyAllowAutoconnectToPolicyNetworks"?

The names are already defined by ONC / policy.

[Devlin, 2017-01-10 18:43:06]

https://codereview.chromium.org/2620463003/diff/60001/chrome/browser/resource...
File chrome/browser/resources/chromeos/network_ui/network_ui.js (right):

https://codereview.chromium.org/2620463003/diff/60001/chrome/browser/resource...
chrome/browser/resources/chromeos/network_ui/network_ui.js:321:
document.querySelector('#global-policy').textContent =
On 2017/01/10 17:49:39, stevenjb wrote:
> On 2017/01/10 15:25:21, Devlin wrote:
> > Why document.querySelector('#foo') instead of $('foo')?
> 
> No particular reason, I just copied from somewhere that used
> document.querySelector, and I forgot about $ (which we don't use much in
Polymer
> code).

AFAIK, we still use $ liberally.  It's also shorter, cleaner, and probably
faster (since we don't have to parse the selector string - we know it's an id
from the get-go).

https://codereview.chromium.org/2620463003/diff/60001/extensions/browser/api/...
File extensions/browser/api/networking_private/networking_private_delegate.h
(right):

https://codereview.chromium.org/2620463003/diff/60001/extensions/browser/api/...
extensions/browser/api/networking_private/networking_private_delegate.h:186:
virtual std::unique_ptr<base::DictionaryValue> GetGlobalPolicy() = 0;
On 2017/01/10 17:49:39, stevenjb wrote:
> On 2017/01/10 15:25:22, Devlin wrote:
> > It looks like this is only used in the new API function, which only needs it
> to
> > get a const&.  Can we return a const DictionaryValue* to avoid the copy?
> 
> I guess my main objections to passing const DictionaryValue* is that it would
be
> inconsistent with the other delegate methods. Also, if we later merge
properties
> from other sources in the future, the delegate code becomes slightly awkward.

Most other methods actually *do* create/modify the result, so it makes sense to
return a unique_ptr.  Here, we don't.  And if we later merge properties, then we
can adjust the signature again. :)

https://codereview.chromium.org/2620463003/diff/60001/extensions/common/api/n...
File extensions/common/api/networking_private.idl (right):

https://codereview.chromium.org/2620463003/diff/60001/extensions/common/api/n...
extensions/common/api/networking_private.idl:810: // If true, only policy
networks may auto connect. Defaults to false.
On 2017/01/10 17:49:39, stevenjb wrote:
> On 2017/01/10 15:25:22, Devlin wrote:
> > "policy networks" sounds strange.  Is this something like "networks
specified
> by
> > enterprise policy"?  Or something else?
> 
> It is  "networks specified by enterprise policy", and we use the term pretty
> liberally in the code.

But we don't use it in documentation, which this is.

https://developer.chrome.com/extensions/networkingPrivate has no instances of
"policy networks".

https://codereview.chromium.org/2620463003/diff/60001/extensions/common/api/n...
extensions/common/api/networking_private.idl:811: boolean?
AllowOnlyPolicyNetworksToAutoconnect;
On 2017/01/10 17:49:39, stevenjb wrote:
> On 2017/01/10 15:25:22, Devlin wrote:
> > These should be javaScriptCased.  Admittedly, this whole file is
inconsistent,
> > but may as well choose in the direction of what it should be.
> 
> See comment at top of file.

Didn't realize these were from ONC :/

https://codereview.chromium.org/2620463003/diff/60001/extensions/common/api/n...
extensions/common/api/networking_private.idl:813: // If true, only policy
networks may be connected to and no new networks may
On 2017/01/10 17:49:39, stevenjb wrote:
> On 2017/01/10 15:25:22, Devlin wrote:
> > ditto re policy networks
> > "may be connected to" is also a little strange.  Maybe "users can only
connect
> > to ..."
> 
> It's a permission so "may" is *technically* correct...

My nit was around the phrasing.  "Only policy networks may be connected to"
sounds more awkward to me than something like "Users may only connect to
networks specified by enterprise policy".

[stevenjb, 2017-01-10 18:58:29]

https://codereview.chromium.org/2620463003/diff/60001/chrome/browser/resource...
File chrome/browser/resources/chromeos/network_ui/network_ui.js (right):

https://codereview.chromium.org/2620463003/diff/60001/chrome/browser/resource...
chrome/browser/resources/chromeos/network_ui/network_ui.js:321:
document.querySelector('#global-policy').textContent =
On 2017/01/10 18:43:06, Devlin wrote:
> On 2017/01/10 17:49:39, stevenjb wrote:
> > On 2017/01/10 15:25:21, Devlin wrote:
> > > Why document.querySelector('#foo') instead of $('foo')?
> > 
> > No particular reason, I just copied from somewhere that used
> > document.querySelector, and I forgot about $ (which we don't use much in
> Polymer
> > code).
> 
> AFAIK, we still use $ liberally.  It's also shorter, cleaner, and probably
> faster (since we don't have to parse the selector string - we know it's an id
> from the get-go).

I didn't mean to suggest that $ wasn't. We also however use
querySelector('#foo') liberally, so I kind of took this as a nit.

https://codereview.chromium.org/2620463003/diff/60001/extensions/browser/api/...
File extensions/browser/api/networking_private/networking_private_delegate.h
(right):

https://codereview.chromium.org/2620463003/diff/60001/extensions/browser/api/...
extensions/browser/api/networking_private/networking_private_delegate.h:186:
virtual std::unique_ptr<base::DictionaryValue> GetGlobalPolicy() = 0;
On 2017/01/10 18:43:06, Devlin wrote:
> On 2017/01/10 17:49:39, stevenjb wrote:
> > On 2017/01/10 15:25:22, Devlin wrote:
> > > It looks like this is only used in the new API function, which only needs
it
> > to
> > > get a const&.  Can we return a const DictionaryValue* to avoid the copy?
> > 
> > I guess my main objections to passing const DictionaryValue* is that it
would
> be
> > inconsistent with the other delegate methods. Also, if we later merge
> properties
> > from other sources in the future, the delegate code becomes slightly
awkward.
> 
> Most other methods actually *do* create/modify the result, so it makes sense
to
> return a unique_ptr.  Here, we don't.  And if we later merge properties, then
we
> can adjust the signature again. :)

How strongly do you feel about this?

https://codereview.chromium.org/2620463003/diff/60001/extensions/common/api/n...
File extensions/common/api/networking_private.idl (right):

https://codereview.chromium.org/2620463003/diff/60001/extensions/common/api/n...
extensions/common/api/networking_private.idl:810: // If true, only policy
networks may auto connect. Defaults to false.
On 2017/01/10 18:43:06, Devlin wrote:
> On 2017/01/10 17:49:39, stevenjb wrote:
> > On 2017/01/10 15:25:22, Devlin wrote:
> > > "policy networks" sounds strange.  Is this something like "networks
> specified
> > by
> > > enterprise policy"?  Or something else?
> > 
> > It is  "networks specified by enterprise policy", and we use the term pretty
> > liberally in the code.
> 
> But we don't use it in documentation, which this is.
> 
> https://developer.chrome.com/extensions/networkingPrivate has no instances of
> "policy networks".

We also don't refer to "enterprise policy" here. Nothing imo is going to make
this especially clear to someone unfamilar with the term "policy networks".

https://codereview.chromium.org/2620463003/diff/60001/extensions/common/api/n...
extensions/common/api/networking_private.idl:811: boolean?
AllowOnlyPolicyNetworksToAutoconnect;
On 2017/01/10 18:43:06, Devlin wrote:
> On 2017/01/10 17:49:39, stevenjb wrote:
> > On 2017/01/10 15:25:22, Devlin wrote:
> > > These should be javaScriptCased.  Admittedly, this whole file is
> inconsistent,
> > > but may as well choose in the direction of what it should be.
> > 
> > See comment at top of file.
> 
> Didn't realize these were from ONC :/

Acknowledged.

https://codereview.chromium.org/2620463003/diff/60001/extensions/common/api/n...
extensions/common/api/networking_private.idl:813: // If true, only policy
networks may be connected to and no new networks may
On 2017/01/10 18:43:06, Devlin wrote:
> On 2017/01/10 17:49:39, stevenjb wrote:
> > On 2017/01/10 15:25:22, Devlin wrote:
> > > ditto re policy networks
> > > "may be connected to" is also a little strange.  Maybe "users can only
> connect
> > > to ..."
> > 
> > It's a permission so "may" is *technically* correct...
> 
> My nit was around the phrasing.  "Only policy networks may be connected to"
> sounds more awkward to me than something like "Users may only connect to
> networks specified by enterprise policy".

Acknowledged.

[Wez, 2017-02-07 02:28:31]

wez@chromium.org changed reviewers:
+ lazyboy@chromium.org, wez@chromium.org

[Wez, 2017-02-07 02:28:32]

Hallo lazyboy@chromium.org!
Due to a depot_tools patch which mistakenly removed the OWNERS check for
non-source files (see crbug.com/684270), the following files landed in this CL
and need a retrospective review from you:
	chrome/app/chromeos_strings.grdp
	chrome/browser/resources/chromeos/network_ui/network_ui.html
	extensions/common/api/networking_private.idl
	third_party/closure_compiler/externs/networking_private.js
	third_party/closure_compiler/interfaces/networking_private_interface.js
	tools/metrics/histograms/histograms.xml
Thanks,
Wez

[Devlin, 2017-02-07 02:44:27]

On 2017/02/07 02:28:32, Wez wrote:
> Hallo mailto:lazyboy@chromium.org!
> Due to a depot_tools patch which mistakenly removed the OWNERS check for
> non-source files (see crbug.com/684270), the following files landed in this CL
> and need a retrospective review from you:
> 	chrome/app/chromeos_strings.grdp
> 	chrome/browser/resources/chromeos/network_ui/network_ui.html
> 	extensions/common/api/networking_private.idl
> 	third_party/closure_compiler/externs/networking_private.js
> 	third_party/closure_compiler/interfaces/networking_private_interface.js
> 	tools/metrics/histograms/histograms.xml
> Thanks,
> Wez

Already reviewed by me (post-land).

[Wez, 2017-02-07 02:54:20]

On 2017/02/07 02:44:27, Devlin wrote:
[snip]
> Already reviewed by me (post-land).

Thanks; would you mind explicitly LGTMing, to help my script see that you
approved it?