Add 'setProperties' to InternetOptionsHandler

Add 'setProperties' to InternetOptionsHandler

Replaces simple custom setters with setProperties call, matching
the networkingPrivate API.

BUG=430113
Committed: https://crrev.com/bc4af0395bb3c948c7704a83bc989ca770d1d7f9
Cr-Commit-Position: refs/heads/master@{#304780}

[stevenjb, 2014-11-04 19:32:17]

stevenjb@chromium.org changed reviewers:
+ michaelpg@chromium.org, pneubeck@chromium.org

[stevenjb, 2014-11-04 19:32:18]

https://codereview.chromium.org/694533007/diff/1/chromeos/network/onc/onc_val...
File chromeos/network/onc/onc_validator.cc (right):

https://codereview.chromium.org/694533007/diff/1/chromeos/network/onc/onc_val...
chromeos/network/onc/onc_validator.cc:413: }
This doesn't seem to me like it should be a "warning" if error_on_missing_field_
is false, but the warning spam this generates is fairly minimal, so I am also OK
reverting this change if you prefer.

[pneubeck (no reviews), 2014-11-11 16:09:31]

It seems to me that only 'Type' should have missed in the ONC dictionary that
was passed to MNCH::SetProperties. If that's the case (or at least the number of
properties is _very_ small), then you shouldn't need
TranslateNetworkConfigurationStateToONC and GetConfigurationStateProperties.

Could you add an API test or an MNCH unit test that tests this part of
SetProperties?

https://codereview.chromium.org/694533007/diff/20001/chrome/browser/resources...
File chrome/browser/resources/options/chromeos/onc_data.js (right):

https://codereview.chromium.org/694533007/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/chromeos/onc_data.js:183: * Returns the comple
ONC dictionary.
comple ?
maybe 'complete'

https://codereview.chromium.org/694533007/diff/20001/chromeos/dbus/fake_shill...
File chromeos/dbus/fake_shill_service_client.cc (right):

https://codereview.chromium.org/694533007/diff/20001/chromeos/dbus/fake_shill...
chromeos/dbus/fake_shill_service_client.cc:380:
properties->SetWithoutPathExpansion(
SetStringWithoutPathExpansion would be more compact and doesn't require a 'new'

https://codereview.chromium.org/694533007/diff/20001/chromeos/network/managed...
File chromeos/network/managed_network_configuration_handler_impl.cc (right):

https://codereview.chromium.org/694533007/diff/20001/chromeos/network/managed...
chromeos/network/managed_network_configuration_handler_impl.cc:272:
network_util::TranslateNetworkConfigurationStateToONC(state));
I think this is solution is not ideal yet.
We need to merge only the bare minimum that makes the dictionary parseable.
Apart from 'Type' nothing even comes to my mind. If you add something like
EAPMethod, then the UI cannot unset that value anymore because it's always
filled by this code.

Alternatively, the UI could also set the type itself.

https://codereview.chromium.org/694533007/diff/20001/chromeos/network/network...
File chromeos/network/network_state.cc (right):

https://codereview.chromium.org/694533007/diff/20001/chromeos/network/network...
chromeos/network/network_state.cc:178: 
maybe add a comment that the remainder of this function adds all
non-configuration (i.e. state) properties.

https://codereview.chromium.org/694533007/diff/20001/chromeos/network/network...
chromeos/network/network_state.cc:209:
ManagedState::GetStateProperties(dictionary);
this seems not intuitive.
The function description explicitly says that state properties are not included.
I think you have to rename or split the ManagedState::GetStateProperties
function as well.

https://codereview.chromium.org/694533007/diff/20001/chromeos/network/network...
File chromeos/network/network_state.h (right):

https://codereview.chromium.org/694533007/diff/20001/chromeos/network/network...
chromeos/network/network_state.h:48: // i.e. no visible state properties.
what are 'visible state properties' ?
maybe drop the 'visible', or if 'visible' an example then maybe change to
'i.e. does not set state properties like 'visible'.

https://codereview.chromium.org/694533007/diff/20001/chromeos/network/onc/onc...
File chromeos/network/onc/onc_validator.cc (right):

https://codereview.chromium.org/694533007/diff/20001/chromeos/network/onc/onc...
chromeos/network/onc/onc_validator.cc:409: error_or_warning_found_ = true;
the header describes this behavior. please update that as well, if you keep
this.

I'm fine with and without changing from WARNING to VLOG.

[stevenjb, 2014-11-12 01:23:29]

PTAL

I modified the setProperties test so that it fails with the existing
SetProperties code but passes with the changes.

https://codereview.chromium.org/694533007/diff/20001/chrome/browser/resources...
File chrome/browser/resources/options/chromeos/onc_data.js (right):

https://codereview.chromium.org/694533007/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/chromeos/onc_data.js:183: * Returns the comple
ONC dictionary.
On 2014/11/11 16:09:31, pneubeck wrote:
> comple ?
> maybe 'complete'

Complete. Done.

https://codereview.chromium.org/694533007/diff/20001/chromeos/dbus/fake_shill...
File chromeos/dbus/fake_shill_service_client.cc (right):

https://codereview.chromium.org/694533007/diff/20001/chromeos/dbus/fake_shill...
chromeos/dbus/fake_shill_service_client.cc:380:
properties->SetWithoutPathExpansion(
On 2014/11/11 16:09:31, pneubeck wrote:
> SetStringWithoutPathExpansion would be more compact and doesn't require a
'new'

This function predates those. Replaced them all.

https://codereview.chromium.org/694533007/diff/20001/chromeos/network/managed...
File chromeos/network/managed_network_configuration_handler_impl.cc (right):

https://codereview.chromium.org/694533007/diff/20001/chromeos/network/managed...
chromeos/network/managed_network_configuration_handler_impl.cc:272:
network_util::TranslateNetworkConfigurationStateToONC(state));
On 2014/11/11 16:09:31, pneubeck wrote:
> I think this is solution is not ideal yet.
> We need to merge only the bare minimum that makes the dictionary parseable.
> Apart from 'Type' nothing even comes to my mind. If you add something like
> EAPMethod, then the UI cannot unset that value anymore because it's always
> filled by this code.
> 
> Alternatively, the UI could also set the type itself.
Hmm. I don't like the idea of forcing the UI to specify a particular subset just
so that the ONC validation code can run correctly. We use this method in
settings e.g. just to set the Priority or APN.

This should already be a nearly minimal subset. Type is definitely needed.
Security at least produces warnings if missing.

Since we merge in the user settings, any of them could be overridden. To 'unset'
a property we set it to 'null', so the merge will still override any properties
that are explicitly 'unset'.

If this really bothers you I'll do the explicit conversion of just 'Type' (which
I know we need, I learned that the hard way), but the validation code is still a
bit opaque to me, so if you could at least confirm that we don't need anything
else I would appreciate it.

https://codereview.chromium.org/694533007/diff/20001/chromeos/network/network...
File chromeos/network/network_state.cc (right):

https://codereview.chromium.org/694533007/diff/20001/chromeos/network/network...
chromeos/network/network_state.cc:209:
ManagedState::GetStateProperties(dictionary);
On 2014/11/11 16:09:31, pneubeck wrote:
> this seems not intuitive.
> The function description explicitly says that state properties are not
included.
> I think you have to rename or split the ManagedState::GetStateProperties
> function as well.
Separated out GetNetworkStateProperties and GetVisibleStateProperties helpers to
make this a bit more clear.

https://codereview.chromium.org/694533007/diff/20001/chromeos/network/network...
File chromeos/network/network_state.h (right):

https://codereview.chromium.org/694533007/diff/20001/chromeos/network/network...
chromeos/network/network_state.h:48: // i.e. no visible state properties.
On 2014/11/11 16:09:31, pneubeck wrote:
> what are 'visible state properties' ?
> maybe drop the 'visible', or if 'visible' an example then maybe change to
> 'i.e. does not set state properties like 'visible'.

Clarified.

https://codereview.chromium.org/694533007/diff/20001/chromeos/network/onc/onc...
File chromeos/network/onc/onc_validator.cc (right):

https://codereview.chromium.org/694533007/diff/20001/chromeos/network/onc/onc...
chromeos/network/onc/onc_validator.cc:409: error_or_warning_found_ = true;
On 2014/11/11 16:09:31, pneubeck wrote:
> the header describes this behavior. please update that as well, if you keep
> this.
> 
> I'm fine with and without changing from WARNING to VLOG.

Done.

[pneubeck (no reviews), 2014-11-13 17:38:57]

please rebase on my CLs whether that helps.
I think, merging with NetworkState's properties shouldn't be necessary anymore?

https://codereview.chromium.org/694533007/diff/20001/chromeos/network/managed...
File chromeos/network/managed_network_configuration_handler_impl.cc (right):

https://codereview.chromium.org/694533007/diff/20001/chromeos/network/managed...
chromeos/network/managed_network_configuration_handler_impl.cc:272:
network_util::TranslateNetworkConfigurationStateToONC(state));
Just looked at the validator, and I actually think that NOTREACHED that you ran
into is wrong.
The validator should never crash because of the dictionary that it verifies and
also should verify and repair as much as it can.

I uploaded a CL, afterwards should not have to merge anything.

https://codereview.chromium.org/694533007/diff/60001/chrome/test/data/extensi...
File chrome/test/data/extensions/api_test/networking/test.js (right):

https://codereview.chromium.org/694533007/diff/60001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/networking/test.js:383: var new_properties
= {};
wouldn't
var new_properties = {
  Priority: 1,
  WiFi: {
    AutoConnect: true
  }
};

be much easier to read an the javascript-way?
(alternative with the keys in quotes)

https://codereview.chromium.org/694533007/diff/60001/chromeos/network/onc/onc...
File chromeos/network/onc/onc_validator.cc (right):

https://codereview.chromium.org/694533007/diff/60001/chromeos/network/onc/onc...
chromeos/network/onc/onc_validator.cc:409: error_or_warning_found_ = true;
This will break a few unit tests.
I uploaded a CL to change this separately.

[stevenjb, 2014-11-13 22:25:34]

On 2014/11/13 17:38:57, pneubeck wrote:
> please rebase on my CLs whether that helps.
> I think, merging with NetworkState's properties shouldn't be necessary
anymore?
> 
>
https://codereview.chromium.org/694533007/diff/20001/chromeos/network/managed...
> File chromeos/network/managed_network_configuration_handler_impl.cc (right):
> 
>
https://codereview.chromium.org/694533007/diff/20001/chromeos/network/managed...
> chromeos/network/managed_network_configuration_handler_impl.cc:272:
> network_util::TranslateNetworkConfigurationStateToONC(state));
> Just looked at the validator, and I actually think that NOTREACHED that you
ran
> into is wrong.
> The validator should never crash because of the dictionary that it verifies
and
> also should verify and repair as much as it can.
> 
> I uploaded a CL, afterwards should not have to merge anything.
> 
>
https://codereview.chromium.org/694533007/diff/60001/chrome/test/data/extensi...
> File chrome/test/data/extensions/api_test/networking/test.js (right):
> 
>
https://codereview.chromium.org/694533007/diff/60001/chrome/test/data/extensi...
> chrome/test/data/extensions/api_test/networking/test.js:383: var
new_properties
> = {};
> wouldn't
> var new_properties = {
>   Priority: 1,
>   WiFi: {
>     AutoConnect: true
>   }
> };
> 
> be much easier to read an the javascript-way?
> (alternative with the keys in quotes)
> 
>
https://codereview.chromium.org/694533007/diff/60001/chromeos/network/onc/onc...
> File chromeos/network/onc/onc_validator.cc (right):
> 
>
https://codereview.chromium.org/694533007/diff/60001/chromeos/network/onc/onc...
> chromeos/network/onc/onc_validator.cc:409: error_or_warning_found_ = true;
> This will break a few unit tests.
> I uploaded a CL to change this separately.

OK, thanks for making those changes, I'll go ahead and remove the NetworkState
property merge, rebase once your CL lands, and re-test.

[stevenjb, 2014-11-13 22:25:41]

https://codereview.chromium.org/694533007/diff/60001/chrome/test/data/extensi...
File chrome/test/data/extensions/api_test/networking/test.js (right):

https://codereview.chromium.org/694533007/diff/60001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/networking/test.js:383: var new_properties
= {};
On 2014/11/13 17:38:56, pneubeck wrote:
> wouldn't
> var new_properties = {
>   Priority: 1,
>   WiFi: {
>     AutoConnect: true
>   }
> };
> 
> be much easier to read an the javascript-way?
> (alternative with the keys in quotes)

Yeah, probably. What can i say, I'm still a C++ programmer at heart.

https://codereview.chromium.org/694533007/diff/60001/chromeos/network/onc/onc...
File chromeos/network/onc/onc_validator.cc (right):

https://codereview.chromium.org/694533007/diff/60001/chromeos/network/onc/onc...
chromeos/network/onc/onc_validator.cc:409: error_or_warning_found_ = true;
On 2014/11/13 17:38:56, pneubeck wrote:
> This will break a few unit tests.
> I uploaded a CL to change this separately.

Thanks! I'll remove these changes from this CL.

[pneubeck (no reviews), 2014-11-14 08:17:03]

https://codereview.chromium.org/694533007/diff/60001/chrome/test/data/extensi...
File chrome/test/data/extensions/api_test/networking/test.js (right):

https://codereview.chromium.org/694533007/diff/60001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/networking/test.js:383: var new_properties
= {};
On 2014/11/13 22:25:41, stevenjb wrote:
> On 2014/11/13 17:38:56, pneubeck wrote:
> > wouldn't
> > var new_properties = {
> >   Priority: 1,
> >   WiFi: {
> >     AutoConnect: true
> >   }
> > };
> > 
> > be much easier to read an the javascript-way?
> > (alternative with the keys in quotes)
> 
> Yeah, probably. What can i say, I'm still a C++ programmer at heart.

:-)

[stevenjb, 2014-11-14 18:20:00]

Patchset #5 (id:80001) has been deleted

[stevenjb, 2014-11-14 18:20:10]

PTAL

[stevenjb, 2014-11-14 21:51:56]

So we do need 'Type' still to translate sub dictionaries, so I simplified MNCH
to just set Type.

[pneubeck (no reviews), 2014-11-18 13:21:39]

lgtm with few comments.

https://codereview.chromium.org/694533007/diff/120001/chrome/browser/resource...
File chrome/browser/resources/options/chromeos/internet_detail.js (right):

https://codereview.chromium.org/694533007/diff/120001/chrome/browser/resource...
chrome/browser/resources/options/chromeos/internet_detail.js:1081:
oncData.setManagedProperty('Priority', priority);
this shouldn't create a managed dictionary.
SetProperties expects an un-managed ONC dictionary and this function name
indicates the opposite.

https://codereview.chromium.org/694533007/diff/120001/chromeos/network/manage...
File chromeos/network/managed_network_configuration_handler_impl.cc (right):

https://codereview.chromium.org/694533007/diff/120001/chromeos/network/manage...
chromeos/network/managed_network_configuration_handler_impl.cc:229: void
ManagedNetworkConfigurationHandlerImpl::SetProperties(
since you seem to prefer SetProperties to do a merge of settings, we should
merge with the old user_settings.

I'll prepare a CL where we can discuss that.

https://codereview.chromium.org/694533007/diff/120001/chromeos/network/manage...
chromeos/network/managed_network_configuration_handler_impl.cc:270: // included
for ONC validation.
as you mentioned in your review-comment you need it (also or solely) for the
ONC->Shill translation later on.
please mention that in this comment.

[stevenjb, 2014-11-18 23:12:06]

PTAL since there is a non-trivial change and I wasn't 100% certain I understood
one of your comments. If you think this is fine as-is (with possible changes to
follow), go ahead and click the the CQ checkbox.

https://codereview.chromium.org/694533007/diff/120001/chrome/browser/resource...
File chrome/browser/resources/options/chromeos/internet_detail.js (right):

https://codereview.chromium.org/694533007/diff/120001/chrome/browser/resource...
chrome/browser/resources/options/chromeos/internet_detail.js:1081:
oncData.setManagedProperty('Priority', priority);
On 2014/11/18 13:21:38, pneubeck wrote:
> this shouldn't create a managed dictionary.
> SetProperties expects an un-managed ONC dictionary and this function name
> indicates the opposite.

Yes, that is poorly named, it explicitly only sets unmanaged properties. I
renamed it to just 'OncData.setProperty'.

https://codereview.chromium.org/694533007/diff/120001/chromeos/network/manage...
File chromeos/network/managed_network_configuration_handler_impl.cc (right):

https://codereview.chromium.org/694533007/diff/120001/chromeos/network/manage...
chromeos/network/managed_network_configuration_handler_impl.cc:229: void
ManagedNetworkConfigurationHandlerImpl::SetProperties(
On 2014/11/18 13:21:38, pneubeck wrote:
> since you seem to prefer SetProperties to do a merge of settings, we should
> merge with the old user_settings.
> 
> I'll prepare a CL where we can discuss that.

I'm not sure I understand what you mean.

If by "merge" you mean "send a partial set of properties to change" then yes,
that is what I prefer because:
a) It means the UI does not need to keep track of all current properties (which
has all sorts of edge cases if two settings apps are running).
b) Requiring a "complete" set of properties is misleading because some
properties like Passphrase are not provided to the UI and so must be optional.

We could certainly address those issues, but I am unclear what the benefit would
be.

Perhaps we should discuss in email before you put up a CL (it is unclear to me
what that CL would be?)

https://codereview.chromium.org/694533007/diff/120001/chromeos/network/manage...
chromeos/network/managed_network_configuration_handler_impl.cc:270: // included
for ONC validation.
On 2014/11/18 13:21:38, pneubeck wrote:
> as you mentioned in your review-comment you need it (also or solely) for the
> ONC->Shill translation later on.
> please mention that in this comment.

Sorry, I assumed it was the validation that was failing not the translation. I
will update the comment.

[pneubeck (no reviews), 2014-11-19 06:59:39]

lgtm

https://codereview.chromium.org/694533007/diff/120001/chromeos/network/manage...
File chromeos/network/managed_network_configuration_handler_impl.cc (right):

https://codereview.chromium.org/694533007/diff/120001/chromeos/network/manage...
chromeos/network/managed_network_configuration_handler_impl.cc:270: // included
for ONC validation.
On 2014/11/18 23:12:06, stevenjb wrote:
> On 2014/11/18 13:21:38, pneubeck wrote:
> > as you mentioned in your review-comment you need it (also or solely) for the
> > ONC->Shill translation later on.
> > please mention that in this comment.
> 
> Sorry, I assumed it was the validation that was failing not the translation. I
> will update the comment.

I guess, originally you ran into issues with the validation. But after the
validation fix, it's the translation :-)

[pneubeck (no reviews), 2014-11-19 06:59:46]

The CQ bit was checked by pneubeck@chromium.org

[commit-bot: I haz the power, 2014-11-19 07:00:18]

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

[commit-bot: I haz the power, 2014-11-19 08:09:47]

Committed patchset #7 (id:140001)

[commit-bot: I haz the power, 2014-11-19 08:10:33]

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