Disable UI in chrome://settings/internet and details popup for policy-managed networks.

Disable UI in chrome://settings/internet and details popup for policy-managed networks.

BUG=chromium-os:23124
TEST=Network configuration continues to work, options are locked down for policy-configured networks.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=112507

[Mattias Nissler (ping if slow), 2011-11-28 20:00:57]

I'd appreciate if both of you could look at this, Steven for the functional
review, James for the HTML/JS.

[James Hawkins, 2011-11-28 21:38:06]

http://codereview.chromium.org/8726007/diff/3001/chrome/browser/resources/opt...
File chrome/browser/resources/options/chromeos/internet_detail_ip_config_list.js
(right):

http://codereview.chromium.org/8726007/diff/3001/chrome/browser/resources/opt...
chrome/browser/resources/options/chromeos/internet_detail_ip_config_list.js:77:
* @param {Event} e The end event. * @private
Wha? I think this change was unintentional.

http://codereview.chromium.org/8726007/diff/3001/chrome/browser/resources/opt...
File chrome/browser/resources/options/chromeos/internet_options.js (right):

http://codereview.chromium.org/8726007/diff/3001/chrome/browser/resources/opt...
chrome/browser/resources/options/chromeos/internet_options.js:270: * Updates the
policy indicator visibility. Space is only allocate for the
s/allocate/allocated/

http://codereview.chromium.org/8726007/diff/3001/chrome/browser/ui/webui/opti...
File chrome/browser/ui/webui/options/chromeos/internet_options_handler.h
(right):

http://codereview.chromium.org/8726007/diff/3001/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/chromeos/internet_options_handler.h:132: //
Stores a dictionary under |key| in |settings| that is suitable be sent to
"suitable to be sent"?

[James Hawkins, 2011-11-28 21:38:16]

LGTM with those nits.

[stevenjb, 2011-11-29 07:23:19]

I apologize if my comments don't make a lot of sense in the larger vision. I'm
catching up a bit and juggling several other fires, but I do think its important
we do this right.

http://codereview.chromium.org/8726007/diff/3001/chrome/browser/chromeos/cros...
File chrome/browser/chromeos/cros/network_library.cc (right):

http://codereview.chromium.org/8726007/diff/3001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/network_library.cc:362: // Initialize to defaults.
This comment confused me, since setting controller_ seems to be the entire
purpose of this function if property_key is NULL.

http://codereview.chromium.org/8726007/diff/3001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/network_library.cc:5114: AddStubNetwork(vpn4,
PROFILE_USER);
Make sure that these changes don't break the unit tests. You might want to put
them in a separate CL.

http://codereview.chromium.org/8726007/diff/3001/chrome/browser/chromeos/cros...
File chrome/browser/chromeos/cros/network_library.h (right):

http://codereview.chromium.org/8726007/diff/3001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/network_library.h:397: };
I think this would be better in its own file. network_library is already too
large, and I think that separating this would make it more clear that this is a
separate class, not really tied directly to NetworkLibrary.

http://codereview.chromium.org/8726007/diff/3001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/network_library.h:712: void
GetUIDataForIPConfig(NetworkPropertyUIData* ui_data) const;
These last three seem unnecessary. Why not just call
ui_data->UpdateFromNetwork(network)?

http://codereview.chromium.org/8726007/diff/3001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/network_library.h:771: bool IsManaged(const char*
prop);
This appears to be unimplemented?

http://codereview.chromium.org/8726007/diff/3001/chrome/browser/ui/webui/opti...
File chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc
(right):

http://codereview.chromium.org/8726007/diff/3001/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc:891:
chromeos::NetworkPropertyUIData property_ui_data;
nit: declare this closer to where it is used?

http://codereview.chromium.org/8726007/diff/3001/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc:951:
network->GetUIDataForAutoConnect(&property_ui_data);
I found this re-use of a single property_ui_data confusing. It took me a while
to understand how NetworkPropertyUIData was being used. It seems like I'd rather
see something like this:

SetValueDictionary(&dictionary, "autoConnect",
Value::CreateBooleanValue(network->auto_connect()),
chromeos::NetworkPropertyUIData(network, kUIDataPropertyAutoConnect)

http://codereview.chromium.org/8726007/diff/3001/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc:1380:
value_dict->Set("value", value);
value_dict takes ownership of |value|, correct? I think that is worth
commenting, I recall that causing me much confusion a while back.
(I just saw that is in the header, but I would briefly mention it here also).

[Mattias Nissler (ping if slow), 2011-11-29 14:48:47]

I now split out the ui_data handling into a separate CL:
http://codereview.chromium.org/8728030/

This one is now rebased on top of the former, which I'd like to discuss first,
so this is on hold for now.

http://codereview.chromium.org/8726007/diff/3001/chrome/browser/chromeos/cros...
File chrome/browser/chromeos/cros/network_library.cc (right):

http://codereview.chromium.org/8726007/diff/3001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/network_library.cc:362: // Initialize to defaults.
On 2011/11/29 07:23:19, Steven Bennetts wrote:
> This comment confused me, since setting controller_ seems to be the entire
> purpose of this function if property_key is NULL. 

You are right. I clarified the comment.

http://codereview.chromium.org/8726007/diff/3001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/network_library.cc:5114: AddStubNetwork(vpn4,
PROFILE_USER);
On 2011/11/29 07:23:19, Steven Bennetts wrote:
> Make sure that these changes don't break the unit tests. You might want to put
> them in a separate CL.

Done.

http://codereview.chromium.org/8726007/diff/3001/chrome/browser/chromeos/cros...
File chrome/browser/chromeos/cros/network_library.h (right):

http://codereview.chromium.org/8726007/diff/3001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/network_library.h:397: };
On 2011/11/29 07:23:19, Steven Bennetts wrote:
> I think this would be better in its own file. network_library is already too
> large, and I think that separating this would make it more clear that this is
a
> separate class, not really tied directly to NetworkLibrary.

Done.

http://codereview.chromium.org/8726007/diff/3001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/network_library.h:712: void
GetUIDataForIPConfig(NetworkPropertyUIData* ui_data) const;
On 2011/11/29 07:23:19, Steven Bennetts wrote:
> These last three seem unnecessary. Why not just call
> ui_data->UpdateFromNetwork(network)?

Obsolete in the new version.

http://codereview.chromium.org/8726007/diff/3001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/network_library.h:771: bool IsManaged(const char*
prop);
On 2011/11/29 07:23:19, Steven Bennetts wrote:
> This appears to be unimplemented?

Good catch, this is a leftover. Removed.

http://codereview.chromium.org/8726007/diff/3001/chrome/browser/resources/opt...
File chrome/browser/resources/options/chromeos/internet_detail_ip_config_list.js
(right):

http://codereview.chromium.org/8726007/diff/3001/chrome/browser/resources/opt...
chrome/browser/resources/options/chromeos/internet_detail_ip_config_list.js:77:
* @param {Event} e The end event. * @private
On 2011/11/28 21:38:06, James Hawkins wrote:
> Wha? I think this change was unintentional.

Sorry. Reverted.

http://codereview.chromium.org/8726007/diff/3001/chrome/browser/resources/opt...
File chrome/browser/resources/options/chromeos/internet_options.js (right):

http://codereview.chromium.org/8726007/diff/3001/chrome/browser/resources/opt...
chrome/browser/resources/options/chromeos/internet_options.js:270: * Updates the
policy indicator visibility. Space is only allocate for the
On 2011/11/28 21:38:06, James Hawkins wrote:
> s/allocate/allocated/

Done.

http://codereview.chromium.org/8726007/diff/3001/chrome/browser/ui/webui/opti...
File chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc
(right):

http://codereview.chromium.org/8726007/diff/3001/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc:891:
chromeos::NetworkPropertyUIData property_ui_data;
On 2011/11/29 07:23:19, Steven Bennetts wrote:
> nit: declare this closer to where it is used?

Done.

http://codereview.chromium.org/8726007/diff/3001/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc:951:
network->GetUIDataForAutoConnect(&property_ui_data);
On 2011/11/29 07:23:19, Steven Bennetts wrote:
> I found this re-use of a single property_ui_data confusing. It took me a while
> to understand how NetworkPropertyUIData was being used. It seems like I'd
rather
> see something like this:
> 
> SetValueDictionary(&dictionary, "autoConnect",
> Value::CreateBooleanValue(network->auto_connect()),
> chromeos::NetworkPropertyUIData(network, kUIDataPropertyAutoConnect)
> 

Done.

http://codereview.chromium.org/8726007/diff/3001/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc:1380:
value_dict->Set("value", value);
On 2011/11/29 07:23:19, Steven Bennetts wrote:
> value_dict takes ownership of |value|, correct? I think that is worth
> commenting, I recall that causing me much confusion a while back.
> (I just saw that is in the header, but I would briefly mention it here also).

Yes. I guess I already got used to it :) Added a comment for clarification.

http://codereview.chromium.org/8726007/diff/3001/chrome/browser/ui/webui/opti...
File chrome/browser/ui/webui/options/chromeos/internet_options_handler.h
(right):

http://codereview.chromium.org/8726007/diff/3001/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/chromeos/internet_options_handler.h:132: //
Stores a dictionary under |key| in |settings| that is suitable be sent to
On 2011/11/28 21:38:06, James Hawkins wrote:
> "suitable to be sent"?

Done.

[stevenjb, 2011-11-29 18:15:38]

http://codereview.chromium.org/8726007/diff/5002/chrome/browser/ui/webui/opti...
File chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc
(right):

http://codereview.chromium.org/8726007/diff/5002/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc:1386:
switch (ui_data.controller()) {
As discussed, I think this will be better with the logic in
NetworkPropertyUIData, so this would look like:
if (ui_data.managed())
...
else if (ui_data.recommended())
...

[Mattias Nissler (ping if slow), 2011-11-29 21:29:04]

Ready for another round.

http://codereview.chromium.org/8726007/diff/5002/chrome/browser/ui/webui/opti...
File chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc
(right):

http://codereview.chromium.org/8726007/diff/5002/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc:1386:
switch (ui_data.controller()) {
On 2011/11/29 18:15:39, Steven Bennetts wrote:
> As discussed, I think this will be better with the logic in
> NetworkPropertyUIData, so this would look like:
> if (ui_data.managed())
> ...
> else if (ui_data.recommended())
> ...

Done.

[Mattias Nissler (ping if slow), 2011-12-01 13:19:52]

On 2011/11/29 21:29:04, Mattias Nissler wrote:
> Ready for another round.
> 
>
http://codereview.chromium.org/8726007/diff/5002/chrome/browser/ui/webui/opti...
> File chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc
> (right):
> 
>
http://codereview.chromium.org/8726007/diff/5002/chrome/browser/ui/webui/opti...
> chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc:1386:
> switch (ui_data.controller()) {
> On 2011/11/29 18:15:39, Steven Bennetts wrote:
> > As discussed, I think this will be better with the logic in
> > NetworkPropertyUIData, so this would look like:
> > if (ui_data.managed())
> > ...
> > else if (ui_data.recommended())
> > ...
> 
> Done.

Steven: friendly ping.

James: Only made slight changes since your LGTM, so I assume it's still good,
please speak up if not.

[stevenjb, 2011-12-01 16:55:02]

Sorry, thought I'd done this one. LGTM.

[commit-bot: I haz the power, 2011-12-01 18:32:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mnissler@chromium.org/8726007/8009

[commit-bot: I haz the power, 2011-12-01 20:03:48]

Change committed as 112507