Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
427903004:
Support Managed NetworkState format dictionaries for controlled settings. (Closed)
|
Created:
6 years, 4 months ago by stevenjb Modified:
6 years, 4 months ago CC:
chromium-reviews, dbeam+watch-options_chromium.org, nkostylev+watch_chromium.org, arv+watch_chromium.org, oshima+watch_chromium.org, stevenjb+watch_chromium.org, davemoore+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Project:
chromium Visibility:
Public. |
DescriptionSupport Managed NetworkState format dictionaries for controlled settings.
BUG=279351
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=288145
Patch Set 1 #Patch Set 2 : . #Patch Set 3 : . #
Total comments: 57
Patch Set 4 : Feedback #
Total comments: 4
Patch Set 5 : Fix Managed Properties Dict #
Total comments: 14
Patch Set 6 : Don't use NetworkPropertyUIData to generate Managed dictionaries #
Total comments: 17
Patch Set 7 : ONC fixes #
Total comments: 14
Patch Set 8 : More ONC fixes #
Total comments: 14
Patch Set 9 : More feedback #Patch Set 10 : Set SharedSetting correctly #
Total comments: 6
Patch Set 11 : Rebase + changed to efective test for Editable #
Messages
Total messages: 29 (0 generated)
I kept this small(ish) because some of the changes are subtle, so there is a fair bit of code to support getManagedPropeties style dictionaries, even though in this CL it is only used for AutoConnect. In a follow up CLs I will: 1. Start aggressively converting properties to use the 'managed' format in the C++ and JS code. 2. Switch to using MNCH::GetManagedProperties instead of GetProperties in the C++ code. 3. Call getManagedProperties from the JS code directly. And then we'll see where we are at... https://codereview.chromium.org/427903004/diff/40001/chrome/browser/resources... File chrome/browser/resources/options/chromeos/internet_detail.html (left): https://codereview.chromium.org/427903004/diff/40001/chrome/browser/resources... chrome/browser/resources/options/chromeos/internet_detail.html:132: </tr> These are unused; they are logically always hidden. https://codereview.chromium.org/427903004/diff/40001/chrome/browser/resources... File chrome/browser/resources/options/chromeos/internet_detail.js (left): https://codereview.chromium.org/427903004/diff/40001/chrome/browser/resources... chrome/browser/resources/options/chromeos/internet_detail.js:453: (this.wireless && !this.showStaticIPConfig) || this.vpn); We were setting showStaticIPConfig = true for wifi networks (this.wireless) so this is just type == VPN. I think maybe 'wireless' use to include cellular and/or wimax? https://codereview.chromium.org/427903004/diff/40001/chrome/browser/resources... chrome/browser/resources/options/chromeos/internet_detail.js:455: this.showStaticIPConfig); Always true, so removed elements with that class. https://codereview.chromium.org/427903004/diff/40001/chrome/browser/resources... File chrome/browser/resources/options/chromeos/internet_detail.js (right): https://codereview.chromium.org/427903004/diff/40001/chrome/browser/resources... chrome/browser/resources/options/chromeos/internet_detail.js:1237: var servingOperator; Removed locally.
the policy parts are a bit tricky. I reviewed only the policy, autoconnect and eap.identity change so far. https://codereview.chromium.org/427903004/diff/40001/chrome/browser/resources... File chrome/browser/resources/options/chromeos/internet_detail.js (right): https://codereview.chromium.org/427903004/diff/40001/chrome/browser/resources... chrome/browser/resources/options/chromeos/internet_detail.js:19: * ONC Managed Property sub-dictionary, e.g. getActiveValue(data, 'Name'). How about: Helper function to read the 'active' value of a property from an ONC with managed information, e.g. getActive(network.WiFi, 'AutoConnect') . https://codereview.chromium.org/427903004/diff/40001/chrome/browser/resources... chrome/browser/resources/options/chromeos/internet_detail.js:24: function getActiveValue(data, key) { The @param section doesn't mention data or key, but property. Seems that you originally had getActiveValue(property). Why is the single argument not sufficient? Seems that it could provide exactly the same functionality because you never check anything on the |data| argument. https://codereview.chromium.org/427903004/diff/40001/chrome/browser/resources... chrome/browser/resources/options/chromeos/internet_detail.js:32: if ('Effective' in property) { i would drop this block. active should be always set, no need to look at the effective value except if you anticipate deviations between the value in Shill and the enforce policy value. In that case it depends on what value we prefer to show... https://codereview.chromium.org/427903004/diff/40001/chrome/browser/resources... chrome/browser/resources/options/chromeos/internet_detail.js:41: * Helper function to get a property from a dictionary in an ONC Managed again i'd drop 'a dictionary in' https://codereview.chromium.org/427903004/diff/40001/chrome/browser/resources... chrome/browser/resources/options/chromeos/internet_detail.js:47: function getActiveDictionaryValue(data, dict, key) { hm. again could be one argument less get..(data, key) { if (typeof data != 'object') return undefined; return getActiveValue(data[key]); } https://codereview.chromium.org/427903004/diff/40001/chrome/browser/resources... chrome/browser/resources/options/chromeos/internet_detail.js:469: createManagedEvent_: function(name, propData) { nit: replace propData['abc'] by propData.abc https://codereview.chromium.org/427903004/diff/40001/chrome/browser/resources... chrome/browser/resources/options/chromeos/internet_detail.js:474: if (effective != 'Unmanaged') var policy; if (effective == UserPolicy || effective == DevicePolicy) policy = effective; var is_editable = propData.UserEditable || propData.DeviceEditable; if (policy && !is_editable) ..controlledBy = 'policy' else if (policy && is_editable) { ..controlledBy = 'recommended' ..recommendedValue = propData[policy]; } ..value = propData.Active; https://codereview.chromium.org/427903004/diff/40001/chrome/browser/resources... chrome/browser/resources/options/chromeos/internet_detail.js:480: if ('Active' in propData) 'Active' should always be present. https://codereview.chromium.org/427903004/diff/40001/chrome/browser/resources... chrome/browser/resources/options/chromeos/internet_detail.js:1124: var fieldEntryName = field + '-entry'; Please consider this very thoroughly. I usually prefer the searchability of element ids instead of saving some code. Is there maybe another solution that does not have to construct ids? E.g. in the next function the parent is hidden instead. (fine to punt) https://codereview.chromium.org/427903004/diff/40001/chrome/browser/resources... chrome/browser/resources/options/chromeos/internet_detail.js:1200: setOrHideEntry('wifi-eap-identity', identity); could actually be setOrHideParent in this case. https://codereview.chromium.org/427903004/diff/40001/chrome/browser/resources... chrome/browser/resources/options/chromeos/internet_detail.js:1351: var prop = indicators[i].hasAttribute('managed') ? 'managed' : 'data'; prop -> attributeName or managedType ? "prop" indicates that it's the value of something with the name |propName|. confusing. https://codereview.chromium.org/427903004/diff/40001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc (right): https://codereview.chromium.org/427903004/diff/40001/chrome/browser/ui/webui/... chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc:316: // Creates a ManagedState style dictionary. Note(stevenjb): This is bridge code here and in the commit message: ManagedState -> ManagedProperties ? ManagedState sounds like chromeos/network/managed_state?? https://codereview.chromium.org/427903004/diff/40001/chrome/browser/ui/webui/... chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc:318: void SetManagedValueDictionary(base::DictionaryValue* settings, nit: should be the last argument https://codereview.chromium.org/427903004/diff/40001/chrome/browser/ui/webui/... chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc:319: const char* key, const std::string& https://codereview.chromium.org/427903004/diff/40001/chrome/browser/ui/webui/... chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc:320: base::Value* value, scoped_ptr (it's actually called with .release() ) https://codereview.chromium.org/427903004/diff/40001/chrome/browser/ui/webui/... chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc:328: if (ui_data.onc_source() == ::onc::ONC_SOURCE_DEVICE_POLICY) I fear that this is not fully right. Looking at NetworkPropertyUIData, - onc_source() is by default the source of the policy - if this particular property is 'recommended', onc_source() is NONE instead. - default_value() is only set if the property is 'recommended' - if the property is not recommended it was assumed that the policy is enforced anyways, and the actual value == policy value Thus, this function should probably look like: void SetManagedValueDictionary(OncSource onc_source, bool is_private, .... ) { DCHECK(value); ... dict->Set(::onc::kAugmentationActiveSetting, value); // Fine, as value comes directly from Shill // We don't have a better source for the user's setting than the actual value from Shill std::string effective = is_private ? UserSetting : SharedSetting; dict->Set(effective, value->DeepCopy()); if (onc_source == ...NONE) { // No policy to consider. } else { std::string policy_key = onc_source == USER_POLICY ? ::onc::kAugmentationUserPolicy : ...DevicePolicy; if (ui_data.onc_source() == ...NONE) { // The policy 'recommends' this property, thus the value is editable by the user. dict->SetBoolean(onc_source == USER_POLICY ? kAugmentationUserEditable : kAugmentationDeviceEditable, true); // ui_data.default_value contains the original policy value, even if // the user changed it. If the policy didn't set any value, then default_value is NULL. if (ui_data.default_value()) dict->Set(policy_key, ui_data.default_value()->DeepCopy()); // If the recommended value equals the actual value from Shill, we don't know // whether the user has set this property explicitly or not. Assume that the // user didn't modify it. if (value->Equals(ui_data.default_value())) effective = policy_key; } else { // Don't set the default Editable value of false. // The actual value should equal the enforce policy value. Set it here for completeness. dict->Set(policy_key, value->DeepCopy()); } } dict->SetString(::onc::kAugmentationEffectiveSetting, effective); } https://codereview.chromium.org/427903004/diff/40001/chrome/browser/ui/webui/... chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc:333: effective = ::onc::kAugmentationUnmanaged; could potentially also be kAugmentationUserSetting or ..SharedSettting . On this side it might be ok to keep this, but on the .js side you will have to handle either value at some time. https://codereview.chromium.org/427903004/diff/40001/chrome/browser/ui/webui/... chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc:1345: if (!auto_connect_ui_data.IsManaged() && don't use this IsManaged, as it's referring to the properties state only (which could be recommended -> IsManaged == false). Use onc_source != ..NONE instead. https://codereview.chromium.org/427903004/diff/40001/chrome/browser/ui/webui/... chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc:1347: ::onc::ONCSource source = network->IsPrivate() Please add a comment (which I should have done in the old code as well): // This modifies auto_connect_ui_data, so that the autoconnect value appears as set to 'false' by policy and not editable by the user. https://codereview.chromium.org/427903004/diff/40001/chrome/browser/ui/webui/... chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc:1349: auto_connect_ui_data.SetOncSourceAndValue(source, auto_connect_value.get()); set the value to 'false' (although it should be the same if there was no bug in the policy application). If you want to, add a DCHECK to ensure this.
https://codereview.chromium.org/427903004/diff/40001/chrome/browser/resources... File chrome/browser/resources/options/chromeos/internet_detail.js (left): https://codereview.chromium.org/427903004/diff/40001/chrome/browser/resources... chrome/browser/resources/options/chromeos/internet_detail.js:453: (this.wireless && !this.showStaticIPConfig) || this.vpn); On 2014/07/29 23:21:11, stevenjb wrote: > We were setting showStaticIPConfig = true for wifi networks (this.wireless) so > this is just type == VPN. I think maybe 'wireless' use to include cellular > and/or wimax? > Acknowledged. https://codereview.chromium.org/427903004/diff/40001/chrome/browser/resources... chrome/browser/resources/options/chromeos/internet_detail.js:455: this.showStaticIPConfig); On 2014/07/29 23:21:11, stevenjb wrote: > Always true, so removed elements with that class. Acknowledged. https://codereview.chromium.org/427903004/diff/40001/chrome/browser/resources... File chrome/browser/resources/options/chromeos/internet_detail.js (right): https://codereview.chromium.org/427903004/diff/40001/chrome/browser/resources... chrome/browser/resources/options/chromeos/internet_detail.js:19: * ONC Managed Property sub-dictionary, e.g. getActiveValue(data, 'Name'). On 2014/07/30 20:12:45, pneubeck wrote: > How about: > Helper function to read the 'active' value of a property from an ONC with > managed information, e.g. getActive(network.WiFi, 'AutoConnect') . Done. https://codereview.chromium.org/427903004/diff/40001/chrome/browser/resources... chrome/browser/resources/options/chromeos/internet_detail.js:24: function getActiveValue(data, key) { On 2014/07/30 20:12:46, pneubeck wrote: > The @param section doesn't mention data or key, but property. > Seems that you originally had getActiveValue(property). > Why is the single argument not sufficient? Seems that it could provide exactly > the same functionality because you never check anything on the |data| argument. I went with this format because I wanted to keep open the possibility of adding ONC validation at some point, e.g. verifying that key is an actual ONC property. Fixed the @param comments. https://codereview.chromium.org/427903004/diff/40001/chrome/browser/resources... chrome/browser/resources/options/chromeos/internet_detail.js:32: if ('Effective' in property) { On 2014/07/30 20:12:45, pneubeck wrote: > i would drop this block. > > active should be always set, no need to look at the effective value except if > you anticipate deviations between the value in Shill and the enforce policy > value. In that case it depends on what value we prefer to show... As discussed in chat, 'Active' may not be set. https://codereview.chromium.org/427903004/diff/40001/chrome/browser/resources... chrome/browser/resources/options/chromeos/internet_detail.js:41: * Helper function to get a property from a dictionary in an ONC Managed On 2014/07/30 20:12:45, pneubeck wrote: > again i'd drop 'a dictionary in' Simplified comment. https://codereview.chromium.org/427903004/diff/40001/chrome/browser/resources... chrome/browser/resources/options/chromeos/internet_detail.js:47: function getActiveDictionaryValue(data, dict, key) { On 2014/07/30 20:12:45, pneubeck wrote: > hm. again could be one argument less > get..(data, key) { > if (typeof data != 'object') > return undefined; > return getActiveValue(data[key]); > } Same reason. https://codereview.chromium.org/427903004/diff/40001/chrome/browser/resources... chrome/browser/resources/options/chromeos/internet_detail.js:469: createManagedEvent_: function(name, propData) { On 2014/07/30 20:12:45, pneubeck wrote: > nit: replace propData['abc'] by propData.abc So, I kept you out of a long email thread that wanted all of the ONC property values to use javaScriptCase instead of PascalCase since that is the convention in JS. Eventually I was able to make the case that these are just property dictionaries, and it is a quirk of JS that it treats Objects and Dictionaries the same. In order to make that more clear, I have intentionally used [] instead of '.' as I switch to ONC properties. https://codereview.chromium.org/427903004/diff/40001/chrome/browser/resources... chrome/browser/resources/options/chromeos/internet_detail.js:474: if (effective != 'Unmanaged') On 2014/07/30 20:12:45, pneubeck wrote: > var policy; > if (effective == UserPolicy || effective == DevicePolicy) > policy = effective; > var is_editable = propData.UserEditable || propData.DeviceEditable; > > if (policy && !is_editable) > ..controlledBy = 'policy' > else if (policy && is_editable) { > ..controlledBy = 'recommended' > ..recommendedValue = propData[policy]; > } > ..value = propData.Active; I wasn't sure whether there might be other policy types added, but I guess if there are we will need to change this anyway. I also didn't see any sign of {User|Device}Editable being used in the existing code and was wary of adding that at this stage, but perhaps I missed it? If this would be new, I would rather add it in a followup. https://codereview.chromium.org/427903004/diff/40001/chrome/browser/resources... chrome/browser/resources/options/chromeos/internet_detail.js:480: if ('Active' in propData) On 2014/07/30 20:12:45, pneubeck wrote: > 'Active' should always be present. See previous comment. https://codereview.chromium.org/427903004/diff/40001/chrome/browser/resources... chrome/browser/resources/options/chromeos/internet_detail.js:1124: var fieldEntryName = field + '-entry'; On 2014/07/30 20:12:45, pneubeck wrote: > Please consider this very thoroughly. > I usually prefer the searchability of element ids instead of saving some code. > Is there maybe another solution that does not have to construct ids? > E.g. in the next function the parent is hidden instead. > > (fine to punt) I generally agree with you on the importance of searchability. It turns out though that we can just use setOrHideParent in all cases. https://codereview.chromium.org/427903004/diff/40001/chrome/browser/resources... chrome/browser/resources/options/chromeos/internet_detail.js:1200: setOrHideEntry('wifi-eap-identity', identity); On 2014/07/30 20:12:46, pneubeck wrote: > could actually be setOrHideParent in this case. So, this should actually be wimax-eap-identity, oops, but yes, it looks like all of these can use setOrHideParent. https://codereview.chromium.org/427903004/diff/40001/chrome/browser/resources... chrome/browser/resources/options/chromeos/internet_detail.js:1237: var servingOperator; On 2014/07/29 23:21:11, stevenjb wrote: > Removed locally. Done. https://codereview.chromium.org/427903004/diff/40001/chrome/browser/resources... chrome/browser/resources/options/chromeos/internet_detail.js:1351: var prop = indicators[i].hasAttribute('managed') ? 'managed' : 'data'; On 2014/07/30 20:12:45, pneubeck wrote: > prop -> attributeName or managedType ? > > "prop" indicates that it's the value of something with the name |propName|. > confusing. Done. https://codereview.chromium.org/427903004/diff/40001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc (right): https://codereview.chromium.org/427903004/diff/40001/chrome/browser/ui/webui/... chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc:316: // Creates a ManagedState style dictionary. Note(stevenjb): This is bridge code On 2014/07/30 20:12:46, pneubeck wrote: > here and in the commit message: > ManagedState -> ManagedProperties ? > > ManagedState sounds like chromeos/network/managed_state?? Done. https://codereview.chromium.org/427903004/diff/40001/chrome/browser/ui/webui/... chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc:318: void SetManagedValueDictionary(base::DictionaryValue* settings, On 2014/07/30 20:12:46, pneubeck wrote: > nit: should be the last argument Fixed here and for SetValueDictionary. https://codereview.chromium.org/427903004/diff/40001/chrome/browser/ui/webui/... chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc:319: const char* key, On 2014/07/30 20:12:46, pneubeck wrote: > const std::string& Why? https://codereview.chromium.org/427903004/diff/40001/chrome/browser/ui/webui/... chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc:320: base::Value* value, On 2014/07/30 20:12:46, pneubeck wrote: > scoped_ptr (it's actually called with .release() ) I'm leaving this consistent with SetValueDictionary. I don't agree that adding scoped_ptr overhead to this file local helper function is more readable. https://codereview.chromium.org/427903004/diff/40001/chrome/browser/ui/webui/... chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc:328: if (ui_data.onc_source() == ::onc::ONC_SOURCE_DEVICE_POLICY) On 2014/07/30 20:12:46, pneubeck wrote: > I fear that this is not fully right. > Looking at NetworkPropertyUIData, > - onc_source() is by default the source of the policy > - if this particular property is 'recommended', onc_source() is NONE instead. > - default_value() is only set if the property is 'recommended' > - if the property is not recommended it was assumed that the policy is > enforced anyways, and the actual value == policy value > > Thus, this function should probably look like: > > void SetManagedValueDictionary(OncSource onc_source, bool is_private, .... ) { > DCHECK(value); > ... > dict->Set(::onc::kAugmentationActiveSetting, value); // Fine, as value comes > directly from Shill > > // We don't have a better source for the user's setting than the actual value > from Shill > std::string effective = is_private ? UserSetting : SharedSetting; > dict->Set(effective, value->DeepCopy()); > > if (onc_source == ...NONE) { > // No policy to consider. > } else { > std::string policy_key = onc_source == USER_POLICY ? > ::onc::kAugmentationUserPolicy : ...DevicePolicy; > if (ui_data.onc_source() == ...NONE) { > // The policy 'recommends' this property, thus the value is editable by > the user. > dict->SetBoolean(onc_source == USER_POLICY ? kAugmentationUserEditable : > kAugmentationDeviceEditable, true); > // ui_data.default_value contains the original policy value, even if > // the user changed it. If the policy didn't set any value, then > default_value is NULL. > if (ui_data.default_value()) > dict->Set(policy_key, ui_data.default_value()->DeepCopy()); > // If the recommended value equals the actual value from Shill, we don't > know > // whether the user has set this property explicitly or not. Assume that > the > // user didn't modify it. > if (value->Equals(ui_data.default_value())) > effective = policy_key; > } else { > // Don't set the default Editable value of false. > // The actual value should equal the enforce policy value. Set it here > for completeness. > dict->Set(policy_key, value->DeepCopy()); > } > } > dict->SetString(::onc::kAugmentationEffectiveSetting, effective); > } So, this is an intermediate helper function, and eventually all SetValueDictionary calls will be replaced with it (in places where we can't immediately/easily use the results of GetManagedProperties directly). What is the source of 'is_private' in these cases? I explicitly mangled the AutoConnect code to modify ui_data t avoid passing that argument. I don't see any reference to UserSetting or SharedSetting in the existing SetValueDictionary or SetAutoconnectValueDictionary. I really don't want to add functionality I don't fully understand here, I would much rather do that in a followup. (Again, unless I am missing something?) https://codereview.chromium.org/427903004/diff/40001/chrome/browser/ui/webui/... chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc:333: effective = ::onc::kAugmentationUnmanaged; On 2014/07/30 20:12:46, pneubeck wrote: > could potentially also be kAugmentationUserSetting or ..SharedSettting . > On this side it might be ok to keep this, but on the .js side you will have to > handle either value at some time. Acknowledged. https://codereview.chromium.org/427903004/diff/40001/chrome/browser/ui/webui/... chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc:1345: if (!auto_connect_ui_data.IsManaged() && On 2014/07/30 20:12:46, pneubeck wrote: > don't use this IsManaged, as it's referring to the properties state only (which > could be recommended -> IsManaged == false). > Use onc_source != ..NONE instead. Done. https://codereview.chromium.org/427903004/diff/40001/chrome/browser/ui/webui/... chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc:1347: ::onc::ONCSource source = network->IsPrivate() On 2014/07/30 20:12:46, pneubeck wrote: > Please add a comment (which I should have done in the old code as well): > > // This modifies auto_connect_ui_data, so that the autoconnect value appears as > set to 'false' by policy and not editable by the user. Done. https://codereview.chromium.org/427903004/diff/40001/chrome/browser/ui/webui/... chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc:1349: auto_connect_ui_data.SetOncSourceAndValue(source, auto_connect_value.get()); On 2014/07/30 20:12:46, pneubeck wrote: > set the value to 'false' (although it should be the same if there was no bug in > the policy application). > If you want to, add a DCHECK to ensure this. Done.
PTAL
https://codereview.chromium.org/427903004/diff/60001/chrome/browser/chromeos/... File chrome/browser/chromeos/options/network_property_ui_data.h (right): https://codereview.chromium.org/427903004/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/options/network_property_ui_data.h:41: // a temporary helper method for the internet_options_handler.cc migraiton. nit: s/migraiton/migration/
https://codereview.chromium.org/427903004/diff/40001/chrome/browser/resources... File chrome/browser/resources/options/chromeos/internet_detail.js (right): https://codereview.chromium.org/427903004/diff/40001/chrome/browser/resources... chrome/browser/resources/options/chromeos/internet_detail.js:474: if (effective != 'Unmanaged') On 2014/07/30 21:57:47, stevenjb wrote: > On 2014/07/30 20:12:45, pneubeck wrote: > > var policy; > > if (effective == UserPolicy || effective == DevicePolicy) > > policy = effective; > > var is_editable = propData.UserEditable || propData.DeviceEditable; > > > > if (policy && !is_editable) > > ..controlledBy = 'policy' > > else if (policy && is_editable) { > > ..controlledBy = 'recommended' > > ..recommendedValue = propData[policy]; > > } > > ..value = propData.Active; > > I wasn't sure whether there might be other policy types added, but I guess if > there are we will need to change this anyway. > > I also didn't see any sign of {User|Device}Editable being used in the existing > code and was wary of adding that at this stage, but perhaps I missed it? If this > would be new, I would rather add it in a followup. You should see the *Editable fields already if you apply a policy with recommended settings. E.g. our OpenVPN has an editable hostname. Google-A has an editable EAP.Identity. Or you could set something up on managedchrome.com. https://codereview.chromium.org/427903004/diff/40001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc (right): https://codereview.chromium.org/427903004/diff/40001/chrome/browser/ui/webui/... chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc:319: const char* key, On 2014/07/30 21:57:48, stevenjb wrote: > On 2014/07/30 20:12:46, pneubeck wrote: > > const std::string& > Why? This CL has more important changes, that I will spare you any elaboration. Should have marked it as 'optional nit' https://codereview.chromium.org/427903004/diff/40001/chrome/browser/ui/webui/... chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc:320: base::Value* value, On 2014/07/30 21:57:48, stevenjb wrote: > On 2014/07/30 20:12:46, pneubeck wrote: > > scoped_ptr (it's actually called with .release() ) > I'm leaving this consistent with SetValueDictionary. I don't agree that adding > scoped_ptr overhead to this file local helper function is more readable. it's documenting ownership. Otherwise this should probably have a comment. https://codereview.chromium.org/427903004/diff/40001/chrome/browser/ui/webui/... chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc:328: if (ui_data.onc_source() == ::onc::ONC_SOURCE_DEVICE_POLICY) In SetValueDictionary, UserSetting / SharedSetting can't be mentioned as it's following the format of CoreOptionsHandler::CreateValueForPref . Maybe you meant that you didn't see it in current calls to getManagedProperties of the API. You're right, that it doesn't appear there yet, because we still don't store user settings separately from the Service properties in Shill. It should then be sufficient to have combinations of these fields. Without policy: Active, Effective (for now without UserSetting/SharedSetting) With user (device) policy additionally: UserPolicy (DevicePolicy), UserEditable (DeviceEditable) is_private: Whether the network is shared or not (stored in the user's profile or the default profile) onc_source: The source of the policy that controls the (whole) network, or NONE if not present. This is different from ui_data.onc_source() as explained somewhere else. The latter is NONE, if this particular property is recommended by policy. https://codereview.chromium.org/427903004/diff/40001/chrome/browser/ui/webui/... chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc:1345: if (!auto_connect_ui_data.IsManaged() && I meant the onc_source variable in this functions scope, not the member of auto_connect_ui_data. https://codereview.chromium.org/427903004/diff/60001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc (right): https://codereview.chromium.org/427903004/diff/60001/chrome/browser/ui/webui/... chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc:326: if (ui_data.onc_source() == ::onc::ONC_SOURCE_NONE) this is does not match the behavior of getManagedProperties. This case can occur in two cases: 1) No policy for this network -> 'return;' is fine. 2) There is a policy which recommends this property. -> UI should show a policy indicator. 'return;' is wrong.
PTAL https://codereview.chromium.org/427903004/diff/40001/chrome/browser/resources... File chrome/browser/resources/options/chromeos/internet_detail.js (right): https://codereview.chromium.org/427903004/diff/40001/chrome/browser/resources... chrome/browser/resources/options/chromeos/internet_detail.js:474: if (effective != 'Unmanaged') On 2014/07/31 20:29:36, pneubeck wrote: > On 2014/07/30 21:57:47, stevenjb wrote: > > On 2014/07/30 20:12:45, pneubeck wrote: > > > var policy; > > > if (effective == UserPolicy || effective == DevicePolicy) > > > policy = effective; > > > var is_editable = propData.UserEditable || propData.DeviceEditable; > > > > > > if (policy && !is_editable) > > > ..controlledBy = 'policy' > > > else if (policy && is_editable) { > > > ..controlledBy = 'recommended' > > > ..recommendedValue = propData[policy]; > > > } > > > ..value = propData.Active; > > > > I wasn't sure whether there might be other policy types added, but I guess if > > there are we will need to change this anyway. > > > > I also didn't see any sign of {User|Device}Editable being used in the existing > > code and was wary of adding that at this stage, but perhaps I missed it? If > this > > would be new, I would rather add it in a followup. > > You should see the *Editable fields already if you apply a policy with > recommended settings. > E.g. our OpenVPN has an editable hostname. Google-A has an editable > EAP.Identity. Or you could set something up on http://managedchrome.com. OK, I think I get it now, documenting the properties of each structure helped. The missing piece was that we need to set the *Editable property in the C++ code. https://codereview.chromium.org/427903004/diff/40001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc (right): https://codereview.chromium.org/427903004/diff/40001/chrome/browser/ui/webui/... chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc:320: base::Value* value, On 2014/07/31 20:29:36, pneubeck wrote: > On 2014/07/30 21:57:48, stevenjb wrote: > > On 2014/07/30 20:12:46, pneubeck wrote: > > > scoped_ptr (it's actually called with .release() ) > > I'm leaving this consistent with SetValueDictionary. I don't agree that adding > > scoped_ptr overhead to this file local helper function is more readable. > it's documenting ownership. > Otherwise this should probably have a comment. Commented. https://codereview.chromium.org/427903004/diff/40001/chrome/browser/ui/webui/... chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc:328: if (ui_data.onc_source() == ::onc::ONC_SOURCE_DEVICE_POLICY) On 2014/07/31 20:29:36, pneubeck wrote: > In SetValueDictionary, UserSetting / SharedSetting can't be mentioned as it's > following the format of CoreOptionsHandler::CreateValueForPref . > > Maybe you meant that you didn't see it in current calls to getManagedProperties > of the API. > You're right, that it doesn't appear there yet, because we still don't store > user settings separately from the Service properties in Shill. > It should then be sufficient to have combinations of these fields. > Without policy: > Active, Effective (for now without UserSetting/SharedSetting) > With user (device) policy additionally: > UserPolicy (DevicePolicy), UserEditable (DeviceEditable) > > is_private: Whether the network is shared or not (stored in the user's profile > or the default profile) > onc_source: The source of the policy that controls the (whole) network, or NONE > if not present. This is different from ui_data.onc_source() as explained > somewhere else. The latter is NONE, if this particular property is recommended > by policy. I think, now that I have written it all out in the doc I shared, that I understand this better and am at least closer to something that is at least partly correct. https://codereview.chromium.org/427903004/diff/40001/chrome/browser/ui/webui/... chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc:1345: if (!auto_connect_ui_data.IsManaged() && On 2014/07/31 20:29:36, pneubeck wrote: > I meant the onc_source variable in this functions scope, not the member of > auto_connect_ui_data. GGAHHHHHHH! I get it, I think, but.... I have to tell you that having a NetworkPropertyUIData class that has an onc_source_ member that needs to be accompanied by another onc_source argument seems... with all due respect.... bat shit crazy. https://codereview.chromium.org/427903004/diff/60001/chrome/browser/chromeos/... File chrome/browser/chromeos/options/network_property_ui_data.h (right): https://codereview.chromium.org/427903004/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/options/network_property_ui_data.h:41: // a temporary helper method for the internet_options_handler.cc migraiton. On 2014/07/31 19:15:46, armansito wrote: > nit: s/migraiton/migration/ Done. https://codereview.chromium.org/427903004/diff/60001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc (right): https://codereview.chromium.org/427903004/diff/60001/chrome/browser/ui/webui/... chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc:326: if (ui_data.onc_source() == ::onc::ONC_SOURCE_NONE) On 2014/07/31 20:29:36, pneubeck wrote: > this is does not match the behavior of getManagedProperties. > This case can occur in two cases: > 1) No policy for this network -> 'return;' is fine. > 2) There is a policy which recommends this property. -> UI should show a policy > indicator. 'return;' is wrong. OK, I looked at the NetworkPropertyUIData code, which is very confusing and sparsely documented. I think I understand it now.
https://codereview.chromium.org/427903004/diff/40001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc (right): https://codereview.chromium.org/427903004/diff/40001/chrome/browser/ui/webui/... chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc:1345: if (!auto_connect_ui_data.IsManaged() && On 2014/08/01 00:53:41, stevenjb wrote: > On 2014/07/31 20:29:36, pneubeck wrote: > > I meant the onc_source variable in this functions scope, not the member of > > auto_connect_ui_data. > > GGAHHHHHHH! I get it, I think, but.... I have to tell you that having a > NetworkPropertyUIData class that has an onc_source_ member that needs to be > accompanied by another onc_source argument seems... with all due respect.... bat > shit crazy. Maybe you guessed it already but this behavior of *UIData pre-dates my time at this company. Feel free to run a 'git blame' ;-) I fully agree that this is in a bad state. That's one of the reasons for migrating to the ONC/getManagedProperties stuff. Hopefully you don't find such crazyness there :-) https://codereview.chromium.org/427903004/diff/80001/chrome/browser/resources... File chrome/browser/resources/options/chromeos/internet_detail.js (right): https://codereview.chromium.org/427903004/diff/80001/chrome/browser/resources... chrome/browser/resources/options/chromeos/internet_detail.js:486: event.value.recommendedValue = propData[effective]; If the policy recommends a value and the user overwrites the property, then effective will be != {User,Device}Policy. But still the recommendedValue should be set. This was also wrong in my suggested code. Could be solved by setting event.value.recommendedValue outside this block: if (propData['UserEditable'] || propData['DeviceEditable'] ) event.value.recommendedValue = propData['UserPolicy'] || propData['DevicePolicy']; https://codereview.chromium.org/427903004/diff/80001/chrome/browser/resources... chrome/browser/resources/options/chromeos/internet_detail.js:900: if (data.type == 'Ethernet') { isn't this part of the already landed CL? https://codereview.chromium.org/427903004/diff/80001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc (right): https://codereview.chromium.org/427903004/diff/80001/chrome/browser/ui/webui/... chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc:329: ::onc::ONCSource recommended_source, optional suggestion: I find the name 'recommended_source' irritating. In the current implementation of policy, there is always at most one source of policy, either user or device policy. This naming suggests that there could be another source. This also implies that if ui_data.onc_source is != NONE, then ui_data.onc_source() == recommended_source. I would suggest just calling it onc_source and documenting it as 'source of the policy of the network, if present' https://codereview.chromium.org/427903004/diff/80001/chrome/browser/ui/webui/... chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc:337: return; // Unmanaged The comment is inaccurate. This case only means that there is no recommended value, but the property is still user editable. Thus the early return is wrong and should instead also set the Editable=true. https://codereview.chromium.org/427903004/diff/80001/chrome/browser/ui/webui/... chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc:339: dict->Set(::onc::kAugmentationUserEditable, optional nit: Using UserEditable will have the same effect (for now) as DeviceEditable. The originally intended semantics however were, that UserEditable means that the user policy allows the user to edit. DeviceEditable means that the device policy allows users to edit. Thus the field would depend on the onc_source or is_private of the whole network. https://codereview.chromium.org/427903004/diff/80001/chrome/browser/ui/webui/... chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc:1361: if (auto_connect_ui_data.onc_source() != ::onc::ONC_SOURCE_NONE && This still has to be changed. Exact syntactic replacement in this line should be: s/auto_connect_ui_data.onc_source() !=/onc_source ==/ (note the negation from != to ==)
https://codereview.chromium.org/427903004/diff/80001/chrome/browser/resources... File chrome/browser/resources/options/chromeos/internet_detail.js (right): https://codereview.chromium.org/427903004/diff/80001/chrome/browser/resources... chrome/browser/resources/options/chromeos/internet_detail.js:486: event.value.recommendedValue = propData[effective]; On 2014/08/01 16:09:08, pneubeck wrote: > If the policy recommends a value and the user overwrites the property, then > effective will be != {User,Device}Policy. > But still the recommendedValue should be set. > > This was also wrong in my suggested code. > > Could be solved by setting event.value.recommendedValue outside this block: > > if (propData['UserEditable'] || propData['DeviceEditable'] ) > event.value.recommendedValue = propData['UserPolicy'] || > propData['DevicePolicy']; This is where re really need to document the behavior somewhere. If User policy is recommended and Device policy is enforced, or vice versa, is the field user editable or not? If both recommend a value which do we use? https://codereview.chromium.org/427903004/diff/80001/chrome/browser/resources... chrome/browser/resources/options/chromeos/internet_detail.js:900: if (data.type == 'Ethernet') { On 2014/08/01 16:09:08, pneubeck wrote: > isn't this part of the already landed CL? Yes, I forgot to set my 'upstream' and haven't rebased. https://codereview.chromium.org/427903004/diff/80001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc (right): https://codereview.chromium.org/427903004/diff/80001/chrome/browser/ui/webui/... chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc:329: ::onc::ONCSource recommended_source, On 2014/08/01 16:09:08, pneubeck wrote: > optional suggestion: > > I find the name 'recommended_source' irritating. > In the current implementation of policy, there is always at most one source of > policy, either user or device policy. > This naming suggests that there could be another source. > > This also implies that if ui_data.onc_source is != NONE, > then ui_data.onc_source() == recommended_source. > > I would suggest just calling it onc_source and documenting it as 'source of the > policy of the network, if present' I am trying to differentiate it from ui_data.onc_source. I really didn't want to change too much in this CL, but maybe I need to just not use NetworkPropertyUIData here? https://codereview.chromium.org/427903004/diff/80001/chrome/browser/ui/webui/... chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc:337: return; // Unmanaged On 2014/08/01 16:09:08, pneubeck wrote: > The comment is inaccurate. This case only means that there is no recommended > value, but the property is still user editable. Thus the early return is wrong > and should instead also set the Editable=true. Bah. Right. OK, I'm going to try do do this without NetworkPropertyUIData then, it's a disaster. https://codereview.chromium.org/427903004/diff/80001/chrome/browser/ui/webui/... chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc:339: dict->Set(::onc::kAugmentationUserEditable, On 2014/08/01 16:09:08, pneubeck wrote: > optional nit: > Using UserEditable will have the same effect (for now) as DeviceEditable. > > The originally intended semantics however were, that UserEditable means that the > user policy allows the user to edit. > > DeviceEditable means that the device policy allows users to edit. > > Thus the field would depend on the onc_source or is_private of the whole > network. Uh... yeah.... OK? Not sure I followed all that, see my previous comment about documenting the behavior :) https://codereview.chromium.org/427903004/diff/80001/chrome/browser/ui/webui/... chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc:1361: if (auto_connect_ui_data.onc_source() != ::onc::ONC_SOURCE_NONE && On 2014/08/01 16:09:08, pneubeck wrote: > This still has to be changed. > Exact syntactic replacement in this line should be: > > s/auto_connect_ui_data.onc_source() !=/onc_source ==/ > > (note the negation from != to ==) I -really- don't understand this. Let's see how this looks when I remove NetworkPropertyUIData here.
https://codereview.chromium.org/427903004/diff/80001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc (right): https://codereview.chromium.org/427903004/diff/80001/chrome/browser/ui/webui/... chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc:329: ::onc::ONCSource recommended_source, On 2014/08/01 17:07:35, stevenjb wrote: > On 2014/08/01 16:09:08, pneubeck wrote: > > optional suggestion: > > > > I find the name 'recommended_source' irritating. > > In the current implementation of policy, there is always at most one source of > > policy, either user or device policy. > > This naming suggests that there could be another source. > > > > This also implies that if ui_data.onc_source is != NONE, > > then ui_data.onc_source() == recommended_source. > > > > I would suggest just calling it onc_source and documenting it as 'source of > the > > policy of the network, if present' > > I am trying to differentiate it from ui_data.onc_source. I really didn't want to > change too much in this CL, but maybe I need to just not use > NetworkPropertyUIData here? If you look into NetowrkProperytUIData, the only functionality that you need is where it looks for the property in the next Recommended array. Maybe you can instead extract that function and use it directly: IsRecommended(property, onc_dict) or something like that. ?
OK, I removed the NetworkUIData dependency for the Managed dictionaries and re-factored the logic. Hopefully this is at least closer to being accurate. Unfortunately I have been prevented from testing this by breakage to the simple chrome workflow, but I expect at least one more round anyway. PTAL
https://codereview.chromium.org/427903004/diff/80001/chrome/browser/resources... File chrome/browser/resources/options/chromeos/internet_detail.js (right): https://codereview.chromium.org/427903004/diff/80001/chrome/browser/resources... chrome/browser/resources/options/chromeos/internet_detail.js:486: event.value.recommendedValue = propData[effective]; > If User policy is recommended and Device policy is enforced, or vice versa, is > the field user editable or not? If both recommend a value which do we use? > This case (mixing of user and device policy) is currently not possible. As described in your doc, User policy and Device policy currently can't affect the same instance of a network configuration (ProfileEntry (or Service) in Shill terms). They can configure the same SSID, but then they're affecting different Shill profiles and Shill only exposes one of the two configurations at a time, namely the user's profile and accordingly only the user policy. https://codereview.chromium.org/427903004/diff/120001/chrome/browser/resource... File chrome/browser/resources/options/chromeos/internet_detail.js (right): https://codereview.chromium.org/427903004/diff/120001/chrome/browser/resource... chrome/browser/resources/options/chromeos/internet_detail.js:476: if (activeValue != undefined) nit: could be changed to 'if (activeValue)' (dropping '!= undefined') to match style of line 473. https://codereview.chromium.org/427903004/diff/120001/chrome/browser/resource... chrome/browser/resources/options/chromeos/internet_detail.js:480: event.value.recommendedValue = effectiveValue; CoreOptionsHandler::CreateValueForPref does not set this for a mandatory policy but only for a 'recommended' (modifiable) policy. Whether the policy is recommended or mandatory is encoded in the User(Device)Editable fields that you use in the next block. Setting 'recommendValue' should therefore move into the next if-block. As mentioned in internet_options_handler.cc, 'effective' should not be set to UserPolicy if the user overwrote a recommended setting. In that case, you have to access propData['UserPolicy'] (or propData['DevicePolicy']) directly. https://codereview.chromium.org/427903004/diff/120001/chrome/browser/resource... chrome/browser/resources/options/chromeos/internet_detail.js:484: event.value.controlledBy = 'recommended'; CoreOptionsHandler::CreateValueForPref sets 'controlledBy' to 'recommended' only if the user didn't overwrite the property. We know for sure that the user overwrote it if the activeValue != recommendedValue. https://codereview.chromium.org/427903004/diff/120001/chrome/browser/ui/webui... File chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc (right): https://codereview.chromium.org/427903004/diff/120001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc:330: const base::Value* default_value, nit: drop a comment that this 'default_value' can either be a recommended or mandatory value from policy. https://codereview.chromium.org/427903004/diff/120001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc:348: dict->SetString(::onc::kAugmentationEffectiveSetting, effective); Please consider the following case: Policy recommends autoconnect to 'true'. User overwrites it to 'false'. |recommend| will be 'true'. |default_value| will be 'true' |value| will be 'false'. The dictionary entry "Effective" should be "UserSetting" in this case and not "UserPolicy" (or DevicePolicy). https://codereview.chromium.org/427903004/diff/120001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc:1360: const base::Value* auto_connect_default_value = NULL; nit: drop a comment that this 'default_value' can either be a recommended or mandatory value from policy. https://codereview.chromium.org/427903004/diff/120001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc:1365: if (auto_connect_onc_source != ::onc::ONC_SOURCE_NONE && This policy applies to all unmanaged networks (networks that are not configured by policy). Therefore the '!=' must be changed to '=='. The meaning of the policy is as user (device) policy: Disable the Autoconnect of all private (shared) user created network configurations. The purpose of this is, that networks from policy are the only autoconnecting networks which is what some admins want for e.g. public sessions.
PTAL https://codereview.chromium.org/427903004/diff/120001/chrome/browser/resource... File chrome/browser/resources/options/chromeos/internet_detail.js (right): https://codereview.chromium.org/427903004/diff/120001/chrome/browser/resource... chrome/browser/resources/options/chromeos/internet_detail.js:476: if (activeValue != undefined) On 2014/08/06 07:13:07, pneubeck2 wrote: > nit: could be changed to 'if (activeValue)' (dropping '!= undefined') to match > style of line 473. Actually I think 473 is incorrect. "False" or 0 could be valid properties for effective or active. https://codereview.chromium.org/427903004/diff/120001/chrome/browser/resource... chrome/browser/resources/options/chromeos/internet_detail.js:480: event.value.recommendedValue = effectiveValue; On 2014/08/06 07:13:07, pneubeck2 wrote: > CoreOptionsHandler::CreateValueForPref > does not set this for a mandatory policy but only for a 'recommended' > (modifiable) policy. > > Whether the policy is recommended or mandatory is encoded in the > User(Device)Editable fields that you use in the next block. > Setting 'recommendValue' should therefore move into the next if-block. > > > As mentioned in internet_options_handler.cc, 'effective' should not be set to > UserPolicy if the user overwrote a recommended setting. In that case, you have > to access propData['UserPolicy'] (or propData['DevicePolicy']) directly. Glancing at the code it didn't seem problematic to set 'recommendedValue' anyway, and might be useful in the future, but I will only set it for recommended for now to be consistent with CoreOptionsHandler. Sorry, I'm not sure I understand. Can this be explained with an example in the ONC doc I created? I *think* you are saying that I can not rely on 'effective' here and should explicitly set recommendedValue to propData['UserPolicy'] if propData['UserEditable'] is true? (Same for 'Device'). I will go ahead and assume that to be the case for now, it doesn't seem like that would be incorrect anyway. https://codereview.chromium.org/427903004/diff/120001/chrome/browser/resource... chrome/browser/resources/options/chromeos/internet_detail.js:484: event.value.controlledBy = 'recommended'; On 2014/08/06 07:13:07, pneubeck2 wrote: > CoreOptionsHandler::CreateValueForPref sets 'controlledBy' to 'recommended' only > if the user didn't overwrite the property. > We know for sure that the user overwrote it if the activeValue != > recommendedValue. Done. https://codereview.chromium.org/427903004/diff/120001/chrome/browser/ui/webui... File chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc (right): https://codereview.chromium.org/427903004/diff/120001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc:330: const base::Value* default_value, On 2014/08/06 07:13:07, pneubeck2 wrote: > nit: drop a comment that this 'default_value' can either be a recommended or > mandatory value from policy. Done. https://codereview.chromium.org/427903004/diff/120001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc:348: dict->SetString(::onc::kAugmentationEffectiveSetting, effective); On 2014/08/06 07:13:08, pneubeck2 wrote: > Please consider the following case: > > Policy recommends autoconnect to 'true'. > User overwrites it to 'false'. > > |recommend| will be 'true'. > |default_value| will be 'true' > |value| will be 'false'. > > The dictionary entry "Effective" should be "UserSetting" in this case and not > "UserPolicy" (or DevicePolicy). Could you add this as an example to the ONC doc when you have a chance? Thanks! https://codereview.chromium.org/427903004/diff/120001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc:348: dict->SetString(::onc::kAugmentationEffectiveSetting, effective); On 2014/08/06 07:13:08, pneubeck2 wrote: > Please consider the following case: > > Policy recommends autoconnect to 'true'. > User overwrites it to 'false'. > > |recommend| will be 'true'. > |default_value| will be 'true' > |value| will be 'false'. > > The dictionary entry "Effective" should be "UserSetting" in this case and not > "UserPolicy" (or DevicePolicy). Done. https://codereview.chromium.org/427903004/diff/120001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc:1360: const base::Value* auto_connect_default_value = NULL; On 2014/08/06 07:13:08, pneubeck2 wrote: > nit: drop a comment that this 'default_value' can either be a recommended or > mandatory value from policy. Done. https://codereview.chromium.org/427903004/diff/120001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc:1365: if (auto_connect_onc_source != ::onc::ONC_SOURCE_NONE && On 2014/08/06 07:13:08, pneubeck2 wrote: > This policy applies to all unmanaged networks (networks that are not configured > by policy). Therefore the '!=' must be changed to '=='. > > The meaning of the policy is as user (device) policy: > Disable the Autoconnect of all private (shared) user created network > configurations. > > The purpose of this is, that networks from policy are the only autoconnecting > networks which is what some admins want for e.g. public sessions. This took me a while to parse. By "this policy" I think you mean "onc::PolicyAllowsOnlyPolicyNetworksToAutoconnect", correct? We really need to move this logic to MNCH so we can eliminate this from here when we switch to calling GetManagedNetworks. https://codereview.chromium.org/427903004/diff/120001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc:1365: if (auto_connect_onc_source != ::onc::ONC_SOURCE_NONE && On 2014/08/06 07:13:08, pneubeck2 wrote: > This policy applies to all unmanaged networks (networks that are not configured > by policy). Therefore the '!=' must be changed to '=='. > > The meaning of the policy is as user (device) policy: > Disable the Autoconnect of all private (shared) user created network > configurations. > > The purpose of this is, that networks from policy are the only autoconnecting > networks which is what some admins want for e.g. public sessions. Done.
https://codereview.chromium.org/427903004/diff/120001/chrome/browser/resource... File chrome/browser/resources/options/chromeos/internet_detail.js (right): https://codereview.chromium.org/427903004/diff/120001/chrome/browser/resource... chrome/browser/resources/options/chromeos/internet_detail.js:476: if (activeValue != undefined) On 2014/08/06 16:29:43, stevenjb wrote: > On 2014/08/06 07:13:07, pneubeck2 wrote: > > nit: could be changed to 'if (activeValue)' (dropping '!= undefined') to match > > style of line 473. > > Actually I think 473 is incorrect. "False" or 0 could be valid properties for > effective or active. Agreed that explicit comparison to undefined is much safer. https://codereview.chromium.org/427903004/diff/140001/chrome/browser/resource... File chrome/browser/resources/options/chromeos/internet_detail.js (right): https://codereview.chromium.org/427903004/diff/140001/chrome/browser/resource... chrome/browser/resources/options/chromeos/internet_detail.js:478: if (userValue != activeValue) { As the comment in the next line says, we should check for an unchanged value, i.e. userValue == activeValue https://codereview.chromium.org/427903004/diff/140001/chrome/browser/resource... chrome/browser/resources/options/chromeos/internet_detail.js:479: // UserEditable and unchanged value indicates this is recommended. nit: ... indicates that the recommended value is in use. https://codereview.chromium.org/427903004/diff/140001/chrome/browser/resource... chrome/browser/resources/options/chromeos/internet_detail.js:481: event.value.recommendedValue = userValue; can be set independent of activeValue. just move it out of the inner if block, i.e. if (propData['UserEditable']) { if (userValue != activeValue) { ...controlledBy = ... } event.value.recommendedValue = userValue; } https://codereview.chromium.org/427903004/diff/140001/chrome/browser/resource... chrome/browser/resources/options/chromeos/internet_detail.js:485: if (deviceValue != activeValue) { ditto, wrong negation. https://codereview.chromium.org/427903004/diff/140001/chrome/browser/resource... chrome/browser/resources/options/chromeos/internet_detail.js:488: event.value.recommendedValue = deviceValue; ditto, should be out of the inner block. https://codereview.chromium.org/427903004/diff/140001/chrome/browser/resource... chrome/browser/resources/options/chromeos/internet_detail.js:491: // Otherwise this is a controlled property. nit: I wouldn't call it 'controlled' but 'Otherwise this property set to a mandatory value by policy.' https://codereview.chromium.org/427903004/diff/140001/chrome/browser/ui/webui... File chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc (right): https://codereview.chromium.org/427903004/diff/140001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc:352: dict->SetString(::onc::kAugmentationEffectiveSetting, Please compare this with case 2. that I added to your doc. In the .js you're comparing the actual value with the recommended value to decide whether the recommended value is used or not. You could move this comparison here and set the Effective accordingly to 'UserSetting'. On .js side it would be sufficient to then look for Effective= UserSetting.
PTAL https://codereview.chromium.org/427903004/diff/140001/chrome/browser/resource... File chrome/browser/resources/options/chromeos/internet_detail.js (right): https://codereview.chromium.org/427903004/diff/140001/chrome/browser/resource... chrome/browser/resources/options/chromeos/internet_detail.js:478: if (userValue != activeValue) { On 2014/08/06 17:37:35, pneubeck wrote: > As the comment in the next line says, we should check for an unchanged value, > i.e. userValue == activeValue Oops. Done. https://codereview.chromium.org/427903004/diff/140001/chrome/browser/resource... chrome/browser/resources/options/chromeos/internet_detail.js:479: // UserEditable and unchanged value indicates this is recommended. On 2014/08/06 17:37:35, pneubeck wrote: > nit: > > ... indicates that the recommended value is in use. Moved to one (hopefully more clear) above the if block. https://codereview.chromium.org/427903004/diff/140001/chrome/browser/resource... chrome/browser/resources/options/chromeos/internet_detail.js:481: event.value.recommendedValue = userValue; On 2014/08/06 17:37:35, pneubeck wrote: > can be set independent of activeValue. > just move it out of the inner if block, i.e. > > if (propData['UserEditable']) { > if (userValue != activeValue) { > ...controlledBy = ... > } > event.value.recommendedValue = userValue; > } Ah. Done. https://codereview.chromium.org/427903004/diff/140001/chrome/browser/resource... chrome/browser/resources/options/chromeos/internet_detail.js:485: if (deviceValue != activeValue) { On 2014/08/06 17:37:35, pneubeck wrote: > ditto, wrong negation. Done. https://codereview.chromium.org/427903004/diff/140001/chrome/browser/resource... chrome/browser/resources/options/chromeos/internet_detail.js:488: event.value.recommendedValue = deviceValue; On 2014/08/06 17:37:35, pneubeck wrote: > ditto, should be out of the inner block. Done. https://codereview.chromium.org/427903004/diff/140001/chrome/browser/resource... chrome/browser/resources/options/chromeos/internet_detail.js:491: // Otherwise this is a controlled property. On 2014/08/06 17:37:35, pneubeck wrote: > nit: I wouldn't call it 'controlled' but > 'Otherwise this property set to a mandatory value by policy.' Acknowledged. https://codereview.chromium.org/427903004/diff/140001/chrome/browser/ui/webui... File chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc (right): https://codereview.chromium.org/427903004/diff/140001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc:352: dict->SetString(::onc::kAugmentationEffectiveSetting, On 2014/08/06 17:37:35, pneubeck wrote: > Please compare this with case 2. that I added to your doc. > > In the .js you're comparing the actual value with the recommended value to > decide whether the recommended value is used or not. > > You could move this comparison here and set the Effective accordingly to > 'UserSetting'. > On .js side it would be sufficient to then look for Effective= UserSetting. OK, I think I get this, the example helps, thanks. Updated the JS as suggested.
lgtm with the few remaining comments. At some patchset I reviewed already the other parts. If there is a specific part that I should double check, please tell me. https://codereview.chromium.org/427903004/diff/160001/chrome/browser/resource... File chrome/browser/resources/options/chromeos/internet_detail.js (right): https://codereview.chromium.org/427903004/diff/160001/chrome/browser/resource... chrome/browser/resources/options/chromeos/internet_detail.js:479: // Otherwise if an effective policy is set then the set value is mandated maybe reword 'effective policy is set'. I'm not sure what that should mean. maybe, 'if the policy is effective' ? https://codereview.chromium.org/427903004/diff/160001/chrome/browser/resource... chrome/browser/resources/options/chromeos/internet_detail.js:482: var userValue = propData['UserPolicy']; nit: now this one can be inlined into the assignment to recommendedValue. https://codereview.chromium.org/427903004/diff/160001/chrome/browser/resource... chrome/browser/resources/options/chromeos/internet_detail.js:487: var deviceValue = propData['DevicePolicy']; nit: ditto, can be inlined into the assignment below https://codereview.chromium.org/427903004/diff/160001/chrome/browser/resource... chrome/browser/resources/options/chromeos/internet_detail.js:488: if (effective != 'UserSetting') copy&paste error: UserSetting -> DeviceSetting https://codereview.chromium.org/427903004/diff/160001/chrome/browser/ui/webui... File chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc (right): https://codereview.chromium.org/427903004/diff/160001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc:351: if (recommended && !value->Equals(default_value)) { nit: to get some more debugging support, maybe add DCHECKs for the pointer arguments that should be not null to the beginning of this function. I would expect, |value| and |settings| to be not NULL.
https://codereview.chromium.org/427903004/diff/160001/chrome/browser/resource... File chrome/browser/resources/options/chromeos/internet_detail.js (right): https://codereview.chromium.org/427903004/diff/160001/chrome/browser/resource... chrome/browser/resources/options/chromeos/internet_detail.js:479: // Otherwise if an effective policy is set then the set value is mandated On 2014/08/06 19:52:06, pneubeck wrote: > maybe reword 'effective policy is set'. I'm not sure what that should mean. > > maybe, 'if the policy is effective' ? Made that part more explicit. https://codereview.chromium.org/427903004/diff/160001/chrome/browser/resource... chrome/browser/resources/options/chromeos/internet_detail.js:482: var userValue = propData['UserPolicy']; On 2014/08/06 19:52:06, pneubeck wrote: > nit: now this one can be inlined into the assignment to recommendedValue. Done. https://codereview.chromium.org/427903004/diff/160001/chrome/browser/resource... chrome/browser/resources/options/chromeos/internet_detail.js:487: var deviceValue = propData['DevicePolicy']; On 2014/08/06 19:52:06, pneubeck wrote: > nit: ditto, can be inlined into the assignment below Done. https://codereview.chromium.org/427903004/diff/160001/chrome/browser/resource... chrome/browser/resources/options/chromeos/internet_detail.js:488: if (effective != 'UserSetting') On 2014/08/06 19:52:06, pneubeck wrote: > copy&paste error: > UserSetting -> DeviceSetting Shouldn't this still be 'UserSetting' if the effective setting was set by the user? https://codereview.chromium.org/427903004/diff/160001/chrome/browser/ui/webui... File chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc (right): https://codereview.chromium.org/427903004/diff/160001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc:351: if (recommended && !value->Equals(default_value)) { On 2014/08/06 19:52:06, pneubeck wrote: > nit: to get some more debugging support, maybe add DCHECKs for the pointer > arguments that should be not null to the beginning of this function. > > I would expect, |value| and |settings| to be not NULL. DCHECK(value) seems reasonable, I can see that mistake being made maybe, but |settings| seems redundant.
https://codereview.chromium.org/427903004/diff/160001/chrome/browser/resource... File chrome/browser/resources/options/chromeos/internet_detail.js (right): https://codereview.chromium.org/427903004/diff/160001/chrome/browser/resource... chrome/browser/resources/options/chromeos/internet_detail.js:488: if (effective != 'UserSetting') On 2014/08/06 20:22:52, stevenjb wrote: > On 2014/08/06 19:52:06, pneubeck wrote: > > copy&paste error: > > UserSetting -> DeviceSetting > Shouldn't this still be 'UserSetting' if the effective setting was set by the > user? > Damn it. I meant SharedSetting. But as you're always setting UserSetting in the C++ side, this should be ok for now. SharedSetting is the equivalent of UserSetting for shared networks. Just keep UserSetting for now.
https://codereview.chromium.org/427903004/diff/160001/chrome/browser/resource... File chrome/browser/resources/options/chromeos/internet_detail.js (right): https://codereview.chromium.org/427903004/diff/160001/chrome/browser/resource... chrome/browser/resources/options/chromeos/internet_detail.js:488: if (effective != 'UserSetting') On 2014/08/06 20:52:57, pneubeck wrote: > On 2014/08/06 20:22:52, stevenjb wrote: > > On 2014/08/06 19:52:06, pneubeck wrote: > > > copy&paste error: > > > UserSetting -> DeviceSetting > > Shouldn't this still be 'UserSetting' if the effective setting was set by the > > user? > > > > Damn it. I meant SharedSetting. > But as you're always setting UserSetting in the C++ side, this should be ok for > now. > SharedSetting is the equivalent of UserSetting for shared networks. > > Just keep UserSetting for now. I'd like to get this right for when we call this with the results of GetManagedProperties also. I will follow up in email.
ptal https://codereview.chromium.org/427903004/diff/160001/chrome/browser/resource... File chrome/browser/resources/options/chromeos/internet_detail.js (right): https://codereview.chromium.org/427903004/diff/160001/chrome/browser/resource... chrome/browser/resources/options/chromeos/internet_detail.js:488: if (effective != 'UserSetting') On 2014/08/06 21:20:28, stevenjb wrote: > On 2014/08/06 20:52:57, pneubeck wrote: > > On 2014/08/06 20:22:52, stevenjb wrote: > > > On 2014/08/06 19:52:06, pneubeck wrote: > > > > copy&paste error: > > > > UserSetting -> DeviceSetting > > > Shouldn't this still be 'UserSetting' if the effective setting was set by > the > > > user? > > > > > > > Damn it. I meant SharedSetting. > > But as you're always setting UserSetting in the C++ side, this should be ok > for > > now. > > SharedSetting is the equivalent of UserSetting for shared networks. > > > > Just keep UserSetting for now. > > I'd like to get this right for when we call this with the results of > GetManagedProperties also. I will follow up in email. OK, I changed the .cc code and checked for SharedSetting here. Question: Would these be more accurate as: if (effective == {User|Device}Policy)? i.e. would a recommended value ever have Effective set to anything else?
https://codereview.chromium.org/427903004/diff/160001/chrome/browser/resource... File chrome/browser/resources/options/chromeos/internet_detail.js (right): https://codereview.chromium.org/427903004/diff/160001/chrome/browser/resource... chrome/browser/resources/options/chromeos/internet_detail.js:488: if (effective != 'UserSetting') On 2014/08/06 21:43:19, stevenjb wrote: > On 2014/08/06 21:20:28, stevenjb wrote: > > On 2014/08/06 20:52:57, pneubeck wrote: > > > On 2014/08/06 20:22:52, stevenjb wrote: > > > > On 2014/08/06 19:52:06, pneubeck wrote: > > > > > copy&paste error: > > > > > UserSetting -> DeviceSetting > > > > Shouldn't this still be 'UserSetting' if the effective setting was set by > > the > > > > user? > > > > > > > > > > Damn it. I meant SharedSetting. > > > But as you're always setting UserSetting in the C++ side, this should be ok > > for > > > now. > > > SharedSetting is the equivalent of UserSetting for shared networks. > > > > > > Just keep UserSetting for now. > > > > I'd like to get this right for when we call this with the results of > > GetManagedProperties also. I will follow up in email. > > OK, I changed the .cc code and checked for SharedSetting here. > > Question: Would these be more accurate as: > if (effective == {User|Device}Policy)? i.e. would a recommended value ever > have Effective set to anything else? There is no other Effective value, but Effective could be unset if neither policy nor user set property. CoreOptionsHandler doesn't set the controlledBy to recommended in that case. So you could change it to 'effective == UserPolicy' and accordingly 'effective == DevicePolicy'.
https://codereview.chromium.org/427903004/diff/200001/chrome/browser/resource... File chrome/browser/resources/options/chromeos/internet_detail.js (right): https://codereview.chromium.org/427903004/diff/200001/chrome/browser/resource... chrome/browser/resources/options/chromeos/internet_detail.js:482: if (effective != 'UserSetting') So, just to confirm, this would be better as: if (effective == 'UserPolicy') https://codereview.chromium.org/427903004/diff/200001/chrome/browser/resource... chrome/browser/resources/options/chromeos/internet_detail.js:486: if (effective != 'SharedSetting') and this as (effective == 'DevicePolicy')
https://codereview.chromium.org/427903004/diff/200001/chrome/browser/resource... File chrome/browser/resources/options/chromeos/internet_detail.js (right): https://codereview.chromium.org/427903004/diff/200001/chrome/browser/resource... chrome/browser/resources/options/chromeos/internet_detail.js:482: if (effective != 'UserSetting') On 2014/08/07 17:39:30, stevenjb wrote: > So, just to confirm, this would be better as: > if (effective == 'UserPolicy') yes https://codereview.chromium.org/427903004/diff/200001/chrome/browser/resource... chrome/browser/resources/options/chromeos/internet_detail.js:486: if (effective != 'SharedSetting') On 2014/08/07 17:39:29, stevenjb wrote: > and this as (effective == 'DevicePolicy') yes
https://codereview.chromium.org/427903004/diff/200001/chrome/browser/resource... File chrome/browser/resources/options/chromeos/internet_detail.js (right): https://codereview.chromium.org/427903004/diff/200001/chrome/browser/resource... chrome/browser/resources/options/chromeos/internet_detail.js:482: if (effective != 'UserSetting') On 2014/08/07 17:41:05, pneubeck wrote: > On 2014/08/07 17:39:30, stevenjb wrote: > > So, just to confirm, this would be better as: > > if (effective == 'UserPolicy') > yes Done. https://codereview.chromium.org/427903004/diff/200001/chrome/browser/resource... chrome/browser/resources/options/chromeos/internet_detail.js:486: if (effective != 'SharedSetting') On 2014/08/07 17:41:05, pneubeck wrote: > On 2014/08/07 17:39:29, stevenjb wrote: > > and this as (effective == 'DevicePolicy') > > yes Done.
lgtm
The CQ bit was checked by stevenjb@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/stevenjb@chromium.org/427903004/220001
Message was sent while issue was closed.
Change committed as 288145 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
