Use setProperties for IP Config.

Use setProperties for IP Config.

This changes the settings UI to use networkingPrivate.setProperties for setting IP config related properties.

It is dependent on the ONC changes in https://codereview.chromium.org/749013003/.

BUG=430113
For components/wifi/fake_wifi_service.cc (test expecations):
TBR=mef@chromium.org

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

[stevenjb, 2014-11-06 00:21:04]

Patchset #2 (id:20001) has been deleted

[stevenjb, 2014-11-06 00:21:11]

Patchset #2 (id:40001) has been deleted

[stevenjb, 2014-11-06 00:23:00]

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

[stevenjb, 2014-11-06 00:23:01]

This CL is a follow-up to https://codereview.chromium.org/694533007/ which
should be reviewed first.

[stevenjb, 2014-11-19 01:04:34]

pneubeck@, PTAL when you have a chance. In particular there are some changes to
NCH that allow clearing of null/undefined properties using
networkingPrivate.setProperties without adding an explicit clearProperties call.
This may need to be discussed in and of itself, or inform other discussions
regarding setProperties.

[pneubeck (no reviews), 2014-11-19 20:05:51]

I took a quick look.
I have to think a bit more about how this makes sense with ONC and the merging
of user settings with the policy, in particular with values recommended by
policy.

https://codereview.chromium.org/708563005/diff/100001/chrome/browser/resource...
File chrome/browser/resources/options/chromeos/onc_data.js (right):

https://codereview.chromium.org/708563005/diff/100001/chrome/browser/resource...
chrome/browser/resources/options/chromeos/onc_data.js:94: if (effective !==
undefined)
does this change anything? I'm asking, because in JS you never know ;-)

https://codereview.chromium.org/708563005/diff/100001/chrome/browser/resource...
chrome/browser/resources/options/chromeos/onc_data.js:116: if (auth !==
undefined)
same here. does it change anything?

https://codereview.chromium.org/708563005/diff/100001/chromeos/network/onc/on...
File chromeos/network/onc/onc_validator.cc (right):

https://codereview.chromium.org/708563005/diff/100001/chromeos/network/onc/on...
chromeos/network/onc/onc_validator.cc:84: return
make_scoped_ptr(base::Value::CreateNullValue());
In general I don't like weaking the validation of the policy.

It might be worth it to allow this only depending on a new Validator flag? E.g.
we could add an allow_null_values boolean to the c'tor.

Alternatively, we could reuse the 'managed' flag.

Note also, that some code assumes that the type is already verified and would
crash on the wrong type.

[stevenjb, 2014-11-19 21:26:42]

https://codereview.chromium.org/708563005/diff/100001/chrome/browser/resource...
File chrome/browser/resources/options/chromeos/onc_data.js (right):

https://codereview.chromium.org/708563005/diff/100001/chrome/browser/resource...
chrome/browser/resources/options/chromeos/onc_data.js:94: if (effective !==
undefined)
On 2014/11/19 20:05:50, pneubeck wrote:
> does this change anything? I'm asking, because in JS you never know ;-)

Yes, I'll add a comment. null == undefined returns true, but null === undefined
returns false. We need to distinguish between null (an entry exists but is set
to null) and undefined (no entry exists) now.

https://codereview.chromium.org/708563005/diff/100001/chrome/browser/resource...
chrome/browser/resources/options/chromeos/onc_data.js:116: if (auth !==
undefined)
On 2014/11/19 20:05:50, pneubeck wrote:
> same here. does it change anything?

ditto

https://codereview.chromium.org/708563005/diff/100001/chromeos/network/onc/on...
File chromeos/network/onc/onc_validator.cc (right):

https://codereview.chromium.org/708563005/diff/100001/chromeos/network/onc/on...
chromeos/network/onc/onc_validator.cc:84: return
make_scoped_ptr(base::Value::CreateNullValue());
On 2014/11/19 20:05:50, pneubeck wrote:
> In general I don't like weaking the validation of the policy.
> 
> It might be worth it to allow this only depending on a new Validator flag?
E.g.
> we could add an allow_null_values boolean to the c'tor.
> 
> Alternatively, we could reuse the 'managed' flag.
> 
> Note also, that some code assumes that the type is already verified and would
> crash on the wrong type.

I don't think I like using the 'managed' flag. I can look into ways to indicate
in the signature when a null value is valid (and am open to recommendations).

[stevenjb, 2014-11-20 01:00:52]

https://codereview.chromium.org/708563005/diff/100001/chrome/browser/resource...
File chrome/browser/resources/options/chromeos/onc_data.js (right):

https://codereview.chromium.org/708563005/diff/100001/chrome/browser/resource...
chrome/browser/resources/options/chromeos/onc_data.js:116: if (auth !==
undefined)
On 2014/11/19 21:26:42, stevenjb wrote:
> On 2014/11/19 20:05:50, pneubeck wrote:
> > same here. does it change anything?
> 
> ditto

Actually, this change wasn't necessary.

[stevenjb, 2014-11-24 22:47:06]

Patchset #8 (id:180001) has been deleted

[stevenjb, 2014-11-24 22:55:36]

OK, I re-factored this to use empty values instead of null in ONC and updated
the ONC spec accordingly.

PTAL

[pneubeck (no reviews), 2014-11-25 21:13:34]

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

https://codereview.chromium.org/708563005/diff/240001/chrome/browser/resource...
chrome/browser/resources/options/chromeos/internet_detail.js:394: * @return
{Object} The IP Config ONC object.
nit: IP Config -> IPConfig

https://codereview.chromium.org/708563005/diff/240001/chrome/browser/resource...
chrome/browser/resources/options/chromeos/internet_detail.js:1328: if
(savedIpAddress) {
I'm not sure why this block is changing? I didn't see a change to SavedIPConfig.

https://codereview.chromium.org/708563005/diff/240001/chrome/test/data/extens...
File chrome/test/data/extensions/api_test/networking/test.js (right):

https://codereview.chromium.org/708563005/diff/240001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/networking/test.js:403:
console.log('Result: ' + JSON.stringify(result));
nit: was for debugging? maybe remove

https://codereview.chromium.org/708563005/diff/240001/chromeos/network/networ...
File chromeos/network/network_configuration_handler.cc (right):

https://codereview.chromium.org/708563005/diff/240001/chromeos/network/networ...
chromeos/network/network_configuration_handler.cc:478: // Trigger an IP config
refresh for set properties after calling
I don't fully understand this rational. Is the goal here to prevent Refresh()
calls to Shill because they cause a re-connect (it's documented that it will
cause a renewal of the DHCP lease) of the network even if the IPConfig didn't
actually change?

Why is it not necessary to prevent a Refresh if the same property was set again?
Or do we just not care that the DHCP lease is then (unnecessarily) renewed?

At least this should be documented here. I had to read a chunk of code to guess
that.

Overall, I'm super astonished by this complexity... to clear a value or,
actually, to transition to automatic ip-configuration.

https://codereview.chromium.org/708563005/diff/240001/chromeos/network/networ...
chromeos/network/network_configuration_handler.cc:481: // trigger
nit: remove linebreak
btw. you might avoid this corner case if the GetProperties would be called
before the split between Set and Clear.

https://codereview.chromium.org/708563005/diff/240001/chromeos/network/networ...
File chromeos/network/network_configuration_handler.h (right):

https://codereview.chromium.org/708563005/diff/240001/chromeos/network/networ...
chromeos/network/network_configuration_handler.h:185: // requested so that only
set properties are cleared. On succes this informs
typo: succes -> success

https://codereview.chromium.org/708563005/diff/240001/components/onc/docs/onc...
File components/onc/docs/onc_spec.html (right):

https://codereview.chromium.org/708563005/diff/240001/components/onc/docs/onc...
components/onc/docs/onc_spec.html:272: unspecifed or empty, the network will be
configured to use DHCP values.
This reads very very contradicting. The dictionary name reads 'StaticIPConfig'
but it allows for Automatic (DHCP) configuration.
The 'IPConfig' type specifies that 'IPAddress' is a required field and that it
must be an IP address, i.e. non-empty. But this sentence allows for it to be
empty or unset.

Everywhere else in this document, if there were multiple alternative
configurations with different sets of parameters, a 'type' field was used and
depending on the type subdictionaries were required.
Analogously, this would mean in this case:

"IPConfigType" field of type string, value must be one of {"Automatic",
"Static"}, optional, defaults to "Automatic"

"StaticIPConfig" field of type IPConfig, required if IPConfigType==Static

https://codereview.chromium.org/708563005/diff/240001/components/onc/docs/onc...
components/onc/docs/onc_spec.html:316: <dt class="field">NameServers</dt>
I think you wanted to change the NameServers field in IPConfig and not the
toplevel one?

[stevenjb, 2014-11-25 21:59:47]

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

https://codereview.chromium.org/708563005/diff/240001/chrome/browser/resource...
chrome/browser/resources/options/chromeos/internet_detail.js:394: * @return
{Object} The IP Config ONC object.
On 2014/11/25 21:13:34, pneubeck wrote:
> nit: IP Config -> IPConfig

Done.

https://codereview.chromium.org/708563005/diff/240001/chrome/browser/resource...
chrome/browser/resources/options/chromeos/internet_detail.js:1328: if
(savedIpAddress) {
On 2014/11/25 21:13:34, pneubeck wrote:
> I'm not sure why this block is changing? I didn't see a change to
SavedIPConfig.

I wanted to be consistent with the changes for Static. In practice this should
never happen, but it seems to me that it also doesn't make sense to keep track
of saved gateway/netmask if there is no saved  IP Address. Updated toe comment
to match.

https://codereview.chromium.org/708563005/diff/240001/chrome/test/data/extens...
File chrome/test/data/extensions/api_test/networking/test.js (right):

https://codereview.chromium.org/708563005/diff/240001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/networking/test.js:403:
console.log('Result: ' + JSON.stringify(result));
On 2014/11/25 21:13:34, pneubeck wrote:
> nit: was for debugging? maybe remove

Done.

https://codereview.chromium.org/708563005/diff/240001/chromeos/network/networ...
File chromeos/network/network_configuration_handler.cc (right):

https://codereview.chromium.org/708563005/diff/240001/chromeos/network/networ...
chromeos/network/network_configuration_handler.cc:478: // Trigger an IP config
refresh for set properties after calling
On 2014/11/25 21:13:34, pneubeck wrote:
> I don't fully understand this rational. Is the goal here to prevent Refresh()
> calls to Shill because they cause a re-connect (it's documented that it will
> cause a renewal of the DHCP lease) of the network even if the IPConfig didn't
> actually change?
> 
> Why is it not necessary to prevent a Refresh if the same property was set
again?
> Or do we just not care that the DHCP lease is then (unnecessarily) renewed?
> 
> At least this should be documented here. I had to read a chunk of code to
guess
> that.
> 
> Overall, I'm super astonished by this complexity... to clear a value or,
> actually, to transition to automatic ip-configuration.

I'm not a fan of the complexity either, but my recollection / understanding is
that changing or clearing IP config properties does not automatically trigger a
Refresh in Shill. If we want a Refresh (which, for every existing case we do if
we change an IPConfig property), we need to explicitly request one. I believe
this is because we used to set each property individually, before Shill
supported passing properties to set in a dictionary, so Shill wouldn't know when
the configuration was complete.

Since requesting a refresh may interrupt connectivity, we don't want to send the
request unless one of the properties *may* have changed. (It does not seem
necessary to check whether or not the value actually changed, we should make the
UI smarter about that in the future).

I will try to clarify the comment.

https://codereview.chromium.org/708563005/diff/240001/chromeos/network/networ...
chromeos/network/network_configuration_handler.cc:481: // trigger
On 2014/11/25 21:13:34, pneubeck wrote:
> nit: remove linebreak
> btw. you might avoid this corner case if the GetProperties would be called
> before the split between Set and Clear.

I'm not sure what you are suggesting, but it sounds like either additional
complexity (which I choose to avoid) or a separate Clear implementation for Set
(which I also choose to avoid).

https://codereview.chromium.org/708563005/diff/240001/chromeos/network/networ...
File chromeos/network/network_configuration_handler.h (right):

https://codereview.chromium.org/708563005/diff/240001/chromeos/network/networ...
chromeos/network/network_configuration_handler.h:185: // requested so that only
set properties are cleared. On succes this informs
On 2014/11/25 21:13:34, pneubeck wrote:
> typo: succes -> success

Done.

https://codereview.chromium.org/708563005/diff/240001/components/onc/docs/onc...
File components/onc/docs/onc_spec.html (right):

https://codereview.chromium.org/708563005/diff/240001/components/onc/docs/onc...
components/onc/docs/onc_spec.html:272: unspecifed or empty, the network will be
configured to use DHCP values.
On 2014/11/25 21:13:34, pneubeck wrote:
> This reads very very contradicting. The dictionary name reads 'StaticIPConfig'
> but it allows for Automatic (DHCP) configuration.
> The 'IPConfig' type specifies that 'IPAddress' is a required field and that it
> must be an IP address, i.e. non-empty. But this sentence allows for it to be
> empty or unset.
> 
> Everywhere else in this document, if there were multiple alternative
> configurations with different sets of parameters, a 'type' field was used and
> depending on the type subdictionaries were required.
> Analogously, this would mean in this case:
> 
> "IPConfigType" field of type string, value must be one of {"Automatic",
> "Static"}, optional, defaults to "Automatic"
> 
> "StaticIPConfig" field of type IPConfig, required if IPConfigType==Static

I was trying to keep this reasonably simple, but do want to do this in a way
that makes sense to you also. I'm not sure I entirely follow your suggestion. I
think it might be easiest if you were to update the spec the way you would like
it and email it to me (or whatever), and I will implement it that way? Thanks!

https://codereview.chromium.org/708563005/diff/240001/components/onc/docs/onc...
components/onc/docs/onc_spec.html:316: <dt class="field">NameServers</dt>
On 2014/11/25 21:13:34, pneubeck wrote:
> I think you wanted to change the NameServers field in IPConfig and not the
> toplevel one?

Yes, you're right. We'll want to make sure we can configure NameServers
automatically independently of the IPAddress/Gateway/Prefix parameters in the
updated spec.

[pneubeck (no reviews), 2014-11-27 18:55:01]

I think you didn't upload the latest patchset.
A part from that I'm ok with everything except the ONC change as discussed in
line.

https://codereview.chromium.org/708563005/diff/240001/components/onc/docs/onc...
File components/onc/docs/onc_spec.html (right):

https://codereview.chromium.org/708563005/diff/240001/components/onc/docs/onc...
components/onc/docs/onc_spec.html:272: unspecifed or empty, the network will be
configured to use DHCP values.
On 2014/11/25 21:59:47, stevenjb wrote:
> On 2014/11/25 21:13:34, pneubeck wrote:
> > This reads very very contradicting. The dictionary name reads
'StaticIPConfig'
> > but it allows for Automatic (DHCP) configuration.
> > The 'IPConfig' type specifies that 'IPAddress' is a required field and that
it
> > must be an IP address, i.e. non-empty. But this sentence allows for it to be
> > empty or unset.
> > 
> > Everywhere else in this document, if there were multiple alternative
> > configurations with different sets of parameters, a 'type' field was used
and
> > depending on the type subdictionaries were required.
> > Analogously, this would mean in this case:
> > 
> > "IPConfigType" field of type string, value must be one of {"Automatic",
> > "Static"}, optional, defaults to "Automatic"
> > 
> > "StaticIPConfig" field of type IPConfig, required if IPConfigType==Static
> 
> I was trying to keep this reasonably simple, but do want to do this in a way
> that makes sense to you also. I'm not sure I entirely follow your suggestion.
I
> think it might be easiest if you were to update the spec the way you would
like
> it and email it to me (or whatever), and I will implement it that way? Thanks!

I started a CL only about the ONC change of IPConfig which I can finish
tomorrow.
Maybe that can save you some work.

[stevenjb, 2014-12-01 18:21:10]

Sorry, I should have added a comment that I was waiting to upload a new
patchset until we resolve how to handle automatic IP config. Thanks for the
CL, I will review that next.


On Thu, Nov 27, 2014 at 10:55 AM, <pneubeck@chromium.org> wrote:

> I think you didn't upload the latest patchset.
> A part from that I'm ok with everything except the ONC change as discussed
> in
> line.
>
>
> https://codereview.chromium.org/708563005/diff/240001/
> components/onc/docs/onc_spec.html
> File components/onc/docs/onc_spec.html (right):
>
> https://codereview.chromium.org/708563005/diff/240001/
> components/onc/docs/onc_spec.html#newcode272
> components/onc/docs/onc_spec.html:272: unspecifed or empty, the network
> will be configured to use DHCP values.
> On 2014/11/25 21:59:47, stevenjb wrote:
>
>> On 2014/11/25 21:13:34, pneubeck wrote:
>> > This reads very very contradicting. The dictionary name reads
>>
> 'StaticIPConfig'
>
>> > but it allows for Automatic (DHCP) configuration.
>> > The 'IPConfig' type specifies that 'IPAddress' is a required field
>>
> and that it
>
>> > must be an IP address, i.e. non-empty. But this sentence allows for
>>
> it to be
>
>> > empty or unset.
>> >
>> > Everywhere else in this document, if there were multiple alternative
>> > configurations with different sets of parameters, a 'type' field was
>>
> used and
>
>> > depending on the type subdictionaries were required.
>> > Analogously, this would mean in this case:
>> >
>> > "IPConfigType" field of type string, value must be one of
>>
> {"Automatic",
>
>> > "Static"}, optional, defaults to "Automatic"
>> >
>> > "StaticIPConfig" field of type IPConfig, required if
>>
> IPConfigType==Static
>
>  I was trying to keep this reasonably simple, but do want to do this in
>>
> a way
>
>> that makes sense to you also. I'm not sure I entirely follow your
>>
> suggestion. I
>
>> think it might be easiest if you were to update the spec the way you
>>
> would like
>
>> it and email it to me (or whatever), and I will implement it that way?
>>
> Thanks!
>
> I started a CL only about the ONC change of IPConfig which I can finish
> tomorrow.
> Maybe that can save you some work.
>
> https://codereview.chromium.org/708563005/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[stevenjb, 2014-12-05 01:21:13]

Patchset #11 (id:260001) has been deleted

[stevenjb, 2014-12-05 01:29:33]

Patchset #12 (id:300001) has been deleted

[stevenjb, 2014-12-05 01:59:13]

PTAL, this is rebased with the IPConfigType CL.

https://codereview.chromium.org/708563005/diff/280001/chromeos/network/onc/on...
File chromeos/network/onc/onc_merger.cc (right):

https://codereview.chromium.org/708563005/diff/280001/chromeos/network/onc/on...
chromeos/network/onc/onc_merger.cc:374: return nullptr;
Note: without this change unrecognized/deprecated values (e.g. top level
NameServers which I was using for a while) that get stored in UIData will cause
a crash.

https://codereview.chromium.org/708563005/diff/280001/chromeos/network/onc/on...
File chromeos/network/onc/onc_translator_onc_to_shill.cc (right):

https://codereview.chromium.org/708563005/diff/280001/chromeos/network/onc/on...
chromeos/network/onc/onc_translator_onc_to_shill.cc:202: ::onc::wifi::kSecurity,
&security)) {
Without this we were getting warning spam when setting a property like
Passphrase without setting Security.

[pneubeck (no reviews), 2014-12-05 09:47:46]

overall looks mostly good to me with the caveat that I have the impression that
we should have even split Static/Automatic IPAddresses from Static/Automatic
Nameservers in ONC.
In principal we can change that even later, as long as we don't persist such ONC
anywhere yet.

I'm also still concerned about the complexity required for clearing values and
for notifying Shill about IPConfig changes.

https://codereview.chromium.org/708563005/diff/280001/chromeos/network/onc/on...
File chromeos/network/onc/onc_merger.cc (right):

https://codereview.chromium.org/708563005/diff/280001/chromeos/network/onc/on...
chromeos/network/onc/onc_merger.cc:374: return nullptr;
On 2014/12/05 01:59:13, stevenjb wrote:
> Note: without this change unrecognized/deprecated values (e.g. top level
> NameServers which I was using for a while) that get stored in UIData will
cause
> a crash.

Acknowledged.

https://codereview.chromium.org/708563005/diff/280001/chromeos/network/onc/on...
File chromeos/network/onc/onc_translator_onc_to_shill.cc (right):

https://codereview.chromium.org/708563005/diff/280001/chromeos/network/onc/on...
chromeos/network/onc/onc_translator_onc_to_shill.cc:202: ::onc::wifi::kSecurity,
&security)) {
On 2014/12/05 01:59:13, stevenjb wrote:
> Without this we were getting warning spam when setting a property like
> Passphrase without setting Security.

Acknowledged.

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

https://codereview.chromium.org/708563005/diff/320001/chrome/browser/resource...
chrome/browser/resources/options/chromeos/internet_detail.js:421:
userNameServers = GoogleNameServers;
just be aware that this will not create a copy and any modification of
'userNameServers' will be a modification of GoogleNameServers.
E.g. as does the sort() below.

https://codereview.chromium.org/708563005/diff/320001/chrome/browser/resource...
chrome/browser/resources/options/chromeos/internet_detail.js:1325: // Override
the "automatic" values with the real saved DHCP values,
I find it a bit hard to find out what .value and .automatic is exactly used for.
This comment says that the 'automatic' values will be overwritten, but the code
also sets '.value'.
Could you add a comment, maybe at the declaration of
inet{Address,Netmask,Gateway}, that explains what each member will be for?

https://codereview.chromium.org/708563005/diff/320001/chrome/browser/resource...
chrome/browser/resources/options/chromeos/internet_detail.js:1381:
onc.hasValue('StaticIPConfig.NameServers')) {
these hasValue checks make me wonder whether you would have preferred to have
ipaddress and nameservers being more separated? so that each can be checked with
a type value?

https://codereview.chromium.org/708563005/diff/320001/chrome/test/data/extens...
File chrome/test/data/extensions/api_test/networking/test.js (right):

https://codereview.chromium.org/708563005/diff/320001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/networking/test.js:393: IPConfigType:
'Static',
should this have been 'DHCP' to test that case also?

https://codereview.chromium.org/708563005/diff/320001/chromeos/dbus/fake_shil...
File chromeos/dbus/fake_shill_manager_client.cc (right):

https://codereview.chromium.org/708563005/diff/320001/chromeos/dbus/fake_shil...
chromeos/dbus/fake_shill_manager_client.cc:599: shill::kPrefixlenProperty, 1);
how is this change relevant?

https://codereview.chromium.org/708563005/diff/320001/chromeos/network/networ...
File chromeos/network/network_configuration_handler.cc (right):

https://codereview.chromium.org/708563005/diff/320001/chromeos/network/networ...
chromeos/network/network_configuration_handler.cc:273: } else if
(value.IsType(base::Value::TYPE_DICTIONARY)) {
isn't this rather dangerous if you want to support partial updates? If ever an
empty dictionary ends up in a setProperties dictionary by mistake, the settings
will be removed.

the code that you changed in onc_to_shill translation sets NULL and not an empty
dict. Is this change then necessary at all?

https://codereview.chromium.org/708563005/diff/320001/chromeos/network/networ...
chromeos/network/network_configuration_handler.cc:276: if
(!value.GetAsDictionary(&dictionary) || dictionary->empty()) {
this check for !GetAsDictionary after IsType(Dictionary) reads a bit confusing.
Could you instead replace IsType by value.GetAsDictionary ?

https://codereview.chromium.org/708563005/diff/320001/chromeos/network/networ...
chromeos/network/network_configuration_handler.cc:290: ClearProperties(
is this behavior covered in NCH unit tests already?
considering the complexity of this, it better should be.

https://codereview.chromium.org/708563005/diff/320001/chromeos/network/onc/on...
File chromeos/network/onc/onc_translator_onc_to_shill.cc (right):

https://codereview.chromium.org/708563005/diff/320001/chromeos/network/onc/on...
chromeos/network/onc/onc_translator_onc_to_shill.cc:263: // empty
StaticIPConfigs object defined which will clear any existing
below you set it to Null and not to an empty Dict

https://codereview.chromium.org/708563005/diff/320001/chromeos/network/onc/on...
File chromeos/network/onc/onc_translator_shill_to_onc.cc (right):

https://codereview.chromium.org/708563005/diff/320001/chromeos/network/onc/on...
chromeos/network/onc/onc_translator_shill_to_onc.cc:489: // Set IPConfigType to
'Static' if a StaticIPConfig object exists.
ah, forgot that one.
You could also set it to DHCP in the else case.

https://codereview.chromium.org/708563005/diff/320001/components/onc/docs/onc...
File components/onc/docs/onc_spec.html (left):

https://codereview.chromium.org/708563005/diff/320001/components/onc/docs/onc...
components/onc/docs/onc_spec.html:333: <dt class="field">NameServers</dt>
please search for 'name.*server' in this file. I think there were some
references to this field.

[stevenjb, 2015-01-08 00:44:39]

PTAL

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

https://codereview.chromium.org/708563005/diff/320001/chrome/browser/resource...
chrome/browser/resources/options/chromeos/internet_detail.js:421:
userNameServers = GoogleNameServers;
On 2014/12/05 09:47:46, pneubeck wrote:
> just be aware that this will not create a copy and any modification of
> 'userNameServers' will be a modification of GoogleNameServers.
> E.g. as does the sort() below.

Ugh. I hate implicit reference languages. Switched to using slice() (it
shouldn't matter, since GoogleNameServers should be sorted, but seems better
practice).

https://codereview.chromium.org/708563005/diff/320001/chrome/browser/resource...
chrome/browser/resources/options/chromeos/internet_detail.js:1325: // Override
the "automatic" values with the real saved DHCP values,
On 2014/12/05 09:47:46, pneubeck wrote:
> I find it a bit hard to find out what .value and .automatic is exactly used
for.
> This comment says that the 'automatic' values will be overwritten, but the
code
> also sets '.value'.
> Could you add a comment, maybe at the declaration of
> inet{Address,Netmask,Gateway}, that explains what each member will be for?

Clarified the comment a bit and added a comment above the declarations.

https://codereview.chromium.org/708563005/diff/320001/chrome/browser/resource...
chrome/browser/resources/options/chromeos/internet_detail.js:1381:
onc.hasValue('StaticIPConfig.NameServers')) {
On 2014/12/05 09:47:46, pneubeck wrote:
> these hasValue checks make me wonder whether you would have preferred to have
> ipaddress and nameservers being more separated? so that each can be checked
with
> a type value?

Yes, but I'd also rather not mess with it at this point. Maybe later.

https://codereview.chromium.org/708563005/diff/320001/chrome/test/data/extens...
File chrome/test/data/extensions/api_test/networking/test.js (right):

https://codereview.chromium.org/708563005/diff/320001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/networking/test.js:393: IPConfigType:
'Static',
On 2014/12/05 09:47:46, pneubeck wrote:
> should this have been 'DHCP' to test that case also?

The above tests getting, this tests setting. The Static case seems more
interesting than the dynamic case. (We don't try to be exhaustive here. Someday
maybe.)

https://codereview.chromium.org/708563005/diff/320001/chromeos/dbus/fake_shil...
File chromeos/dbus/fake_shill_manager_client.cc (right):

https://codereview.chromium.org/708563005/diff/320001/chromeos/dbus/fake_shil...
chromeos/dbus/fake_shill_manager_client.cc:599: shill::kPrefixlenProperty, 1);
On 2014/12/05 09:47:46, pneubeck wrote:
> how is this change relevant?

0 is not valid. I rely on the fake implementation for most of my (non automated)
testing.

https://codereview.chromium.org/708563005/diff/320001/chromeos/network/networ...
File chromeos/network/network_configuration_handler.cc (right):

https://codereview.chromium.org/708563005/diff/320001/chromeos/network/networ...
chromeos/network/network_configuration_handler.cc:273: } else if
(value.IsType(base::Value::TYPE_DICTIONARY)) {
On 2014/12/05 09:47:46, pneubeck wrote:
> isn't this rather dangerous if you want to support partial updates? If ever an
> empty dictionary ends up in a setProperties dictionary by mistake, the
settings
> will be removed.
> 
> the code that you changed in onc_to_shill translation sets NULL and not an
empty
> dict. Is this change then necessary at all?

Removed (for now at least) since we are using empty dictionaries for IP config.

https://codereview.chromium.org/708563005/diff/320001/chromeos/network/networ...
chromeos/network/network_configuration_handler.cc:276: if
(!value.GetAsDictionary(&dictionary) || dictionary->empty()) {
On 2014/12/05 09:47:46, pneubeck wrote:
> this check for !GetAsDictionary after IsType(Dictionary) reads a bit
confusing.
> Could you instead replace IsType by value.GetAsDictionary ?

Acknowledged.

https://codereview.chromium.org/708563005/diff/320001/chromeos/network/networ...
chromeos/network/network_configuration_handler.cc:290: ClearProperties(
On 2014/12/05 09:47:46, pneubeck wrote:
> is this behavior covered in NCH unit tests already?
> considering the complexity of this, it better should be.

Removed for now.

https://codereview.chromium.org/708563005/diff/320001/chromeos/network/onc/on...
File chromeos/network/onc/onc_translator_onc_to_shill.cc (right):

https://codereview.chromium.org/708563005/diff/320001/chromeos/network/onc/on...
chromeos/network/onc/onc_translator_onc_to_shill.cc:263: // empty
StaticIPConfigs object defined which will clear any existing
On 2014/12/05 09:47:46, pneubeck wrote:
> below you set it to Null and not to an empty Dict

Addressed in other CL.

https://codereview.chromium.org/708563005/diff/320001/chromeos/network/onc/on...
File chromeos/network/onc/onc_translator_shill_to_onc.cc (right):

https://codereview.chromium.org/708563005/diff/320001/chromeos/network/onc/on...
chromeos/network/onc/onc_translator_shill_to_onc.cc:489: // Set IPConfigType to
'Static' if a StaticIPConfig object exists.
On 2014/12/05 09:47:46, pneubeck wrote:
> ah, forgot that one.
> You could also set it to DHCP in the else case.

Addressed in other CL.

https://codereview.chromium.org/708563005/diff/320001/components/onc/docs/onc...
File components/onc/docs/onc_spec.html (left):

https://codereview.chromium.org/708563005/diff/320001/components/onc/docs/onc...
components/onc/docs/onc_spec.html:333: <dt class="field">NameServers</dt>
On 2014/12/05 09:47:46, pneubeck wrote:
> please search for 'name.*server' in this file. I think there were some
> references to this field.

Addressed in other CL.

[pneubeck (no reviews), 2015-01-08 09:47:13]

thanks this is much more comprehensible now.
lgtm.

please update the commit message before committing.

https://codereview.chromium.org/708563005/diff/380001/chrome/browser/resource...
File chrome/browser/resources/options/chromeos/internet_detail.html (right):

https://codereview.chromium.org/708563005/diff/380001/chrome/browser/resource...
chrome/browser/resources/options/chromeos/internet_detail.html:412:
managed="IPConfigType"></span>
IPAddressConfigType

[stevenjb, 2015-01-08 17:09:58]

https://codereview.chromium.org/708563005/diff/380001/chrome/browser/resource...
File chrome/browser/resources/options/chromeos/internet_detail.html (right):

https://codereview.chromium.org/708563005/diff/380001/chrome/browser/resource...
chrome/browser/resources/options/chromeos/internet_detail.html:412:
managed="IPConfigType"></span>
On 2015/01/08 09:47:13, pneubeck wrote:
> IPAddressConfigType

Oops, thanks. Done. Also did a grep, no other missed instances.

[stevenjb, 2015-01-08 17:10:08]

The CQ bit was checked by stevenjb@chromium.org

[commit-bot: I haz the power, 2015-01-08 17:11:26]

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

[commit-bot: I haz the power, 2015-01-08 17:56:24]

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

[commit-bot: I haz the power, 2015-01-08 17:56:25]

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

[stevenjb, 2015-01-08 18:35:04]

The CQ bit was checked by stevenjb@chromium.org

[commit-bot: I haz the power, 2015-01-08 18:35:49]

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

[commit-bot: I haz the power, 2015-01-08 20:03:03]

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

[commit-bot: I haz the power, 2015-01-08 20:03:04]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[stevenjb, 2015-01-08 21:32:35]

The CQ bit was checked by stevenjb@chromium.org

[commit-bot: I haz the power, 2015-01-08 21:33:28]

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

[commit-bot: I haz the power, 2015-01-08 21:42:44]

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

[commit-bot: I haz the power, 2015-01-08 21:42:54]

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

[stevenjb, 2015-01-08 22:28:57]

The CQ bit was checked by stevenjb@chromium.org

[commit-bot: I haz the power, 2015-01-08 22:30:26]

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

[commit-bot: I haz the power, 2015-01-08 22:41:28]

Committed patchset #18 (id:440001)

[commit-bot: I haz the power, 2015-01-08 22:42:19]

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