Some JS cleanup, including proper ownership of ONC properties.

Some JS cleanup, including proper ownership of ONC properties.

This is in preparation for eliminating the remaining non ONC properties
from the 'data' object passed from Chrome.

BUG=279351
Committed: https://crrev.com/5ca466f14cd4b221b5e1340641fd6793621881bd
Cr-Commit-Position: refs/heads/master@{#293895}

[stevenjb, 2014-09-05 23:32:29]

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

[armansito, 2014-09-06 00:38:57]

lgtm

https://codereview.chromium.org/547703004/diff/1/chrome/browser/resources/opt...
File chrome/browser/resources/options/chromeos/internet_detail.js (right):

https://codereview.chromium.org/547703004/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/chromeos/internet_detail.js:47:
DetailsInternetPage.getInstance().updateControls_();
nit: It feels a bit awkward that this top-level function is calling what looks
like a private function (viz "_"). Should the function be called updateControls
like before?

[pneubeck (no reviews), 2014-09-08 09:18:00]

I didn't do a diff of the moved code. The structural changes lgtm at a high
level.

https://codereview.chromium.org/547703004/diff/1/chrome/browser/resources/opt...
File chrome/browser/resources/options/chromeos/internet_detail.js (right):

https://codereview.chromium.org/547703004/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/chromeos/internet_detail.js:1033: if
('showBuyButton' in update)
not sure whether that has the intended effect:

if showBuyButton _was_ true
and changes to not existent, then you will not hide the element anymore.

same for below.

https://codereview.chromium.org/547703004/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/chromeos/internet_detail.js:1040:
$('details-internet-login').hidden = true;
this seems to have been unreliable already in the old code?
i.e. if 'details-internet-login' was already hidden and should now be visible,
it will not appear.

IIUC, this seems to be a common problem in this function: there are several
guarded assignments to 'hidden'...

https://codereview.chromium.org/547703004/diff/1/chrome/browser/resources/opt...
File chrome/browser/resources/options/chromeos/onc_data.js (right):

https://codereview.chromium.org/547703004/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/chromeos/onc_data.js:136: * Updates the
properties of |data_| from the properties in |update|.
not sure whether that has the intended effect.
IIUC, usually |this.data_| and |update| are nested dictionaries, in which case
your updating will happen only on the toplevel.

_If_ this is intended, please document it and then this seems also not an ONC
specific function anymore but InternetDetails.js specific.


I guess, this update is only there to keep user modifications?
If so, I think a more reliable solution would be to keep the user modifications
separately and to completely drop the old |onc| and replace it by the newly
received |update|.
You could then reapply the user modifications to |update|, if necessary.

[stevenjb, 2014-09-08 19:43:12]

https://codereview.chromium.org/547703004/diff/1/chrome/browser/resources/opt...
File chrome/browser/resources/options/chromeos/internet_detail.js (right):

https://codereview.chromium.org/547703004/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/chromeos/internet_detail.js:47:
DetailsInternetPage.getInstance().updateControls_();
On 2014/09/06 00:38:56, armansito wrote:
> nit: It feels a bit awkward that this top-level function is calling what looks
> like a private function (viz "_"). Should the function be called
updateControls
> like before?
Yeah, I think you're right. I will make updateControls "public".
Done.

https://codereview.chromium.org/547703004/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/chromeos/internet_detail.js:1033: if
('showBuyButton' in update)
On 2014/09/08 09:18:00, pneubeck wrote:
> not sure whether that has the intended effect:
> 
> if showBuyButton _was_ true
> and changes to not existent, then you will not hide the element anymore.
> 
> same for below.

Ugh. So, this variable isn't even used, I'll remove it, but the logic for the
others is not what one would expect - not present does in fact mean 'false'.
Since we want to remove that logic from the C++ code anyway, I'm not going to
fix that, I'll just restore the JS logic.

https://codereview.chromium.org/547703004/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/chromeos/internet_detail.js:1040:
$('details-internet-login').hidden = true;
On 2014/09/08 09:18:00, pneubeck wrote:
> this seems to have been unreliable already in the old code?
> i.e. if 'details-internet-login' was already hidden and should now be visible,
> it will not appear.
> 
> IIUC, this seems to be a common problem in this function: there are several
> guarded assignments to 'hidden'...

Yeah, I fixed this one here and in showDetailedInfo.

https://codereview.chromium.org/547703004/diff/1/chrome/browser/resources/opt...
File chrome/browser/resources/options/chromeos/onc_data.js (right):

https://codereview.chromium.org/547703004/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/chromeos/onc_data.js:136: * Updates the
properties of |data_| from the properties in |update|.
On 2014/09/08 09:18:00, pneubeck wrote:
> not sure whether that has the intended effect.
> IIUC, usually |this.data_| and |update| are nested dictionaries, in which case
> your updating will happen only on the toplevel.
> 
> _If_ this is intended, please document it and then this seems also not an ONC
> specific function anymore but InternetDetails.js specific.
> 
> 
> I guess, this update is only there to keep user modifications?
> If so, I think a more reliable solution would be to keep the user
modifications
> separately and to completely drop the old |onc| and replace it by the newly
> received |update|.
> You could then reapply the user modifications to |update|, if necessary.

Eventually this should go away, we really shouldn't support partial ONC updates.
We shouldn't have user modifications that are not supported round trip (and if
we do we should keep them separately as you say).

For now, it should be safe to do top level replacement, i.e. if we update an ONC
entry that has nested dictionaries, we will always update the entire dictionary.


I will add a comment to that effect.

[stevenjb, 2014-09-08 21:31:18]

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

[stevenjb, 2014-09-08 21:31:19]

PTAL

https://codereview.chromium.org/547703004/diff/1/chrome/browser/resources/opt...
File chrome/browser/resources/options/chromeos/internet_detail.js (right):

https://codereview.chromium.org/547703004/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/chromeos/internet_detail.js:1040:
$('details-internet-login').hidden = true;
On 2014/09/08 19:43:12, stevenjb wrote:
> On 2014/09/08 09:18:00, pneubeck wrote:
> > this seems to have been unreliable already in the old code?
> > i.e. if 'details-internet-login' was already hidden and should now be
visible,
> > it will not appear.
> > 
> > IIUC, this seems to be a common problem in this function: there are several
> > guarded assignments to 'hidden'...
> 
> Yeah, I fixed this one here and in showDetailedInfo.
> 
Actually, it turns out this overrides 'details-internet-login.hidden' set in
updateConnectionButtonVisibilty_() iff showActivateButton is true, so the
original logic was correct.

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

https://codereview.chromium.org/547703004/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/chromeos/internet_detail.js:693:
stringFromValue(defaultApn['AccessPointName']);
This was buggy; activeApn needs to be defined as a dictionary object first.
Also, we really shouldn't call onc.setmanagedProperty in line 701 unless we set
activeApn anyway.

[armansito, 2014-09-08 21:43:32]

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

https://codereview.chromium.org/547703004/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/chromeos/internet_detail.js:693:
stringFromValue(defaultApn['AccessPointName']);
On 2014/09/08 21:31:18, stevenjb wrote:
> This was buggy; activeApn needs to be defined as a dictionary object first.
> Also, we really shouldn't call onc.setmanagedProperty in line 701 unless we
set
> activeApn anyway.

Yes, good catch by vitalyp@.

[Vitaly Pavlenko, 2014-09-08 21:46:03]

lgtm

[pneubeck (no reviews), 2014-09-08 21:51:06]

lgtm with one comment

https://codereview.chromium.org/547703004/diff/1/chrome/browser/resources/opt...
File chrome/browser/resources/options/chromeos/internet_detail.js (right):

https://codereview.chromium.org/547703004/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/chromeos/internet_detail.js:693:
onc.setManagedProperty('Cellular.APN', activeApn);
maybe the original idea here was, that this property will be 'unset' in the else
case.

[stevenjb, 2014-09-08 23:25:16]

https://codereview.chromium.org/547703004/diff/1/chrome/browser/resources/opt...
File chrome/browser/resources/options/chromeos/internet_detail.js (right):

https://codereview.chromium.org/547703004/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/chromeos/internet_detail.js:693:
onc.setManagedProperty('Cellular.APN', activeApn);
On 2014/09/08 21:51:05, pneubeck wrote:
> maybe the original idea here was, that this property will be 'unset' in the
else
> case.

I thought of that, but I don't see any advantage to that behavior.
'Cellular.APN' is only ever relevant if 'Cellular.APNList' is non empty.

Eventually 'onc_' needs to be read-only so that the UI reflects whatever the
values actually get set to after and update gets sent.

[stevenjb, 2014-09-09 00:51:44]

The CQ bit was checked by stevenjb@chromium.org

[stevenjb, 2014-09-09 00:51:45]

The CQ bit was unchecked by stevenjb@chromium.org

[stevenjb, 2014-09-09 00:51:47]

The CQ bit was checked by stevenjb@chromium.org

[commit-bot: I haz the power, 2014-09-09 00:52:56]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/stevenjb@chromium.org/547703004/60001

[commit-bot: I haz the power, 2014-09-09 08:17:10]

Committed patchset #4 (id:60001) as 5aabef7e4824bc8e71bb3f3978774b0b0942eb6d

[commit-bot: I haz the power, 2014-09-10 03:51:58]

Patchset 4 (id:??) landed as
https://crrev.com/5ca466f14cd4b221b5e1340641fd6793621881bd
Cr-Commit-Position: refs/heads/master@{#293895}