Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1310373008:
Add cr_policy_indicator for settings controls (Closed)
|
Created:
5 years, 4 months ago by stevenjb Modified:
5 years, 3 months ago CC:
chromium-reviews, extensions-reviews_chromium.org, khorimoto+watch-md-settings_chromium.org, michaelpg+watch-md-settings_chromium.org, jlklein+watch-closure_chromium.org, jhawkins+watch-md-settings_chromium.org, orenb+watch-md-settings_chromium.org, jlklein+watch-md-settings_chromium.org, oshima+watch_chromium.org, vitalyp+closure_chromium.org, chromium-apps-reviews_chromium.org, dbeam+watch-closure_chromium.org, stevenjb+watch-md-settings_chromium.org, arv+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@md_settings_compiled_resources_3 Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd cr_policy_indicator for settings controls
This also integrates cr_policy_indicator with settings/checknox.
BUG=521791
For trivial change to idl:
TBR=kalman@chromium.org
Committed: https://crrev.com/7182593974f9788691c98bca3943d377b22894c3
Cr-Commit-Position: refs/heads/master@{#349459}
Patch Set 1 #Patch Set 2 : . #Patch Set 3 : Use iron-icons and simplify #Patch Set 4 : Elim unused tyep #
Total comments: 58
Patch Set 5 : Fix cros only, undo namespace change #Patch Set 6 : Fix prefs_util, feedback #Patch Set 7 : Rebase #Patch Set 8 : Rebase fix #Patch Set 9 : Revert unrelated changes #
Total comments: 30
Patch Set 10 : Feedback #
Total comments: 11
Patch Set 11 : Rebase + Feedback #
Total comments: 8
Patch Set 12 : Rebase + More Feedback #Patch Set 13 : Address missed feedback #Patch Set 14 : Better observer pattern for cr_policy_indicator #
Total comments: 12
Patch Set 15 : Comment fixes #Patch Set 16 : Rebase #Dependent Patchsets: Messages
Total messages: 35 (9 generated)
stevenjb@chromium.org changed reviewers: + dbeam@chromium.org, michaelpg@chromium.org
Hold off on this (I know you're eager to review it), I am going to switch to the MD icons and make a few other changes before the first iteration.
Patchset #3 (id:40001) has been deleted
Patchset #3 (id:60001) has been deleted
OK, This is good to go. PTAL when you have a chance.
https://codereview.chromium.org/1310373008/diff/100001/chrome/browser/extensi... File chrome/browser/extensions/api/settings_private/prefs_util.cc (right): https://codereview.chromium.org/1310373008/diff/100001/chrome/browser/extensi... chrome/browser/extensions/api/settings_private/prefs_util.cc:149: const base::Value* value = CrosSettings::Get()->GetPref(name); GetPref doesn't check the whitelist, so |value| could be null if the settings page uses a bad pref key, leading to a segfault. Do we care? (for non-CrOS prefs, GetPref returns a nullptr for an invalid key.) https://codereview.chromium.org/1310373008/diff/100001/chrome/browser/extensi... chrome/browser/extensions/api/settings_private/prefs_util.cc:190: // also restricted to the owner, and on non Chrome OS we hide these controls nit: "non-Chrome OS" https://codereview.chromium.org/1310373008/diff/100001/chrome/browser/extensi... chrome/browser/extensions/api/settings_private/prefs_util.cc:195: if (pref->IsManagedByCustodian()) I'm confused about the difference between "managed by custodian" and "supervisor-controlled". And when can a pref be managed (which implies admin policy) and also managed by custodian? https://codereview.chromium.org/1310373008/diff/100001/chrome/browser/extensi... chrome/browser/extensions/api/settings_private/prefs_util.cc:198: source = settings_api::PolicySource::POLICY_SOURCE_USER; When is a preference "managed", but not mandated by enterprise policy or a supervisor? https://codereview.chromium.org/1310373008/diff/100001/chrome/browser/extensi... chrome/browser/extensions/api/settings_private/prefs_util.cc:199: if (pref->IsRecommended()) { Another point of confusion :-) How can a pref be in the managed store and the recommended store at the same time? Seems to me if it's in the managed store, PrefValueStore::ControllingPrefStoreForPref will always return MANAGED_STORE, so PrefValueStore::PrefValueFromRecommendedStore will be false. https://codereview.chromium.org/1310373008/diff/100001/chrome/browser/extensi... chrome/browser/extensions/api/settings_private/prefs_util.cc:212: source = settings_api::PolicySource::POLICY_SOURCE_USER; What is a user policy? In this case the pref is controlled by an extension or the command-line store, right? https://codereview.chromium.org/1310373008/diff/100001/chrome/browser/extensi... chrome/browser/extensions/api/settings_private/prefs_util.cc:363: bool PrefsUtil::IsPrefSupervisorControlled(const std::string& pref_name) { Does PrefService::IsManagedByCustodian result in different behavior? https://codereview.chromium.org/1310373008/diff/100001/chrome/browser/extensi... chrome/browser/extensions/api/settings_private/prefs_util.cc:373: if (pref_name == prefs::kWakeOnWifiSsid) { #if defined(OS_CHROMEOS)? https://codereview.chromium.org/1310373008/diff/100001/chrome/browser/extensi... chrome/browser/extensions/api/settings_private/prefs_util.cc:390: IsPrefSupervisorControlled(pref_name) || Why isn't it enough to check pref->IsUserModifiable()? https://codereview.chromium.org/1310373008/diff/100001/chrome/browser/extensi... File chrome/browser/extensions/api/settings_private/prefs_util.h (right): https://codereview.chromium.org/1310373008/diff/100001/chrome/browser/extensi... chrome/browser/extensions/api/settings_private/prefs_util.h:67: // the owner, and |profile_| is not the owner profile. Chrome OS only. https://codereview.chromium.org/1310373008/diff/100001/chrome/browser/extensi... chrome/browser/extensions/api/settings_private/prefs_util.h:75: // the primary user, and |profile_| is not the primary profile. Chrome OS only, right? https://codereview.chromium.org/1310373008/diff/100001/chrome/browser/resourc... File chrome/browser/resources/settings/checkbox/checkbox.js (right): https://codereview.chromium.org/1310373008/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/checkbox/checkbox.js:86: if (change.path == 'pref.policySource' || else if https://codereview.chromium.org/1310373008/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/checkbox/checkbox.js:108: this.checkboxDisabled = this.disabled || this.isSettingDisabled_(this.pref); nit: instead of adding listeners like disabledChanged and pref.*, can you bind the HTML "disabled" attribute to a function which checks pref, pref.policyEnforcement and disabled https://codereview.chromium.org/1310373008/diff/100001/chrome/common/extensio... File chrome/common/extensions/api/settings_private.idl (right): https://codereview.chromium.org/1310373008/diff/100001/chrome/common/extensio... chrome/common/extensions/api/settings_private.idl:11: // Source of a restricted pref, either by policy or other source. Source of a restricted pref (e.g., policy).
https://codereview.chromium.org/1310373008/diff/100001/chrome/browser/extensi... File chrome/browser/extensions/api/settings_private/prefs_util.cc (right): https://codereview.chromium.org/1310373008/diff/100001/chrome/browser/extensi... chrome/browser/extensions/api/settings_private/prefs_util.cc:42: namespace settings_api = api::settings_private; can you do this in a separate CL? https://codereview.chromium.org/1310373008/diff/100001/chrome/browser/extensi... chrome/browser/extensions/api/settings_private/prefs_util.cc:390: IsPrefSupervisorControlled(pref_name) || On 2015/08/27 23:40:01, michaelpg wrote: > Why isn't it enough to check pref->IsUserModifiable()? +1 https://codereview.chromium.org/1310373008/diff/100001/chrome/browser/extensi... File chrome/browser/extensions/api/settings_private/prefs_util.h (right): https://codereview.chromium.org/1310373008/diff/100001/chrome/browser/extensi... chrome/browser/extensions/api/settings_private/prefs_util.h:67: // the owner, and |profile_| is not the owner profile. On 2015/08/27 23:40:01, michaelpg wrote: > Chrome OS only. can it be in if-defs, then? https://codereview.chromium.org/1310373008/diff/100001/chrome/browser/resourc... File chrome/browser/resources/settings/checkbox/checkbox.css (right): https://codereview.chromium.org/1310373008/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/checkbox/checkbox.css:11: #outerDiv { idsLikeThis or ids-like-this? https://codereview.chromium.org/1310373008/diff/100001/chrome/browser/resourc... File chrome/browser/resources/settings/checkbox/checkbox.js (right): https://codereview.chromium.org/1310373008/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/checkbox/checkbox.js:27: value: null nit: we are not IE, feel free to add trailing , so the blame doesn't change for the last line of an object every time a new property is added https://codereview.chromium.org/1310373008/diff/100001/chrome/browser/resourc... File chrome/browser/resources/settings/prefs/prefs_types.js (right): https://codereview.chromium.org/1310373008/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/prefs/prefs_types.js:12: * isInitialized: (boolean|undefined) indent off https://codereview.chromium.org/1310373008/diff/100001/ui/webui/resources/cr_... File ui/webui/resources/cr_elements/v1_0/policy/cr_policy_indicator.css (right): https://codereview.chromium.org/1310373008/diff/100001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/v1_0/policy/cr_policy_indicator.css:6: the use of overflow: hidden. */ nit: /* ... * ... */ ^ https://codereview.chromium.org/1310373008/diff/100001/ui/webui/resources/cr_... File ui/webui/resources/cr_elements/v1_0/policy/cr_policy_indicator.html (right): https://codereview.chromium.org/1310373008/diff/100001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/v1_0/policy/cr_policy_indicator.html:11: icon="[[getIcon_(indicatorType)]]"/> I think you need a full </iron-icon> instead of <iron-icon/> https://codereview.chromium.org/1310373008/diff/100001/ui/webui/resources/cr_... File ui/webui/resources/cr_elements/v1_0/policy/cr_policy_indicator.js (right): https://codereview.chromium.org/1310373008/diff/100001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/v1_0/policy/cr_policy_indicator.js:49: return; nit: combine ifs https://codereview.chromium.org/1310373008/diff/100001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/v1_0/policy/cr_policy_indicator.js:56: type = IndicatorType.PRIMARY; why PRIMARY vs PRIMARY_USER? https://codereview.chromium.org/1310373008/diff/100001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/v1_0/policy/cr_policy_indicator.js:60: type = IndicatorType.USER_POLICY; why USER vs USER_POLICY? https://codereview.chromium.org/1310373008/diff/100001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/v1_0/policy/cr_policy_indicator.js:62: type = IndicatorType.DEVICE_POLICY; why DEVICE vs DEVICE_POLICY? https://codereview.chromium.org/1310373008/diff/100001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/v1_0/policy/cr_policy_indicator.js:77: showIndicator_: function(type) { return type != IndicatorType.NONE; }, this sounds like a method to show an indicator, not whether an indicator should be shown https://codereview.chromium.org/1310373008/diff/100001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/v1_0/policy/cr_policy_indicator.js:97: } should there be an assertNotReached() here?
This still isn't full ready, but it should be close. Mostly I want to update the CL in case my machine catches fire while I'm gone. https://codereview.chromium.org/1310373008/diff/100001/chrome/browser/extensi... File chrome/browser/extensions/api/settings_private/prefs_util.cc (right): https://codereview.chromium.org/1310373008/diff/100001/chrome/browser/extensi... chrome/browser/extensions/api/settings_private/prefs_util.cc:42: namespace settings_api = api::settings_private; On 2015/08/28 00:20:22, Dan Beam wrote: > can you do this in a separate CL? I'll undo it. I know the churn sucks. It helped align some stuff, but in hindsight is was a bad call. https://codereview.chromium.org/1310373008/diff/100001/chrome/browser/extensi... chrome/browser/extensions/api/settings_private/prefs_util.cc:149: const base::Value* value = CrosSettings::Get()->GetPref(name); On 2015/08/27 23:40:01, michaelpg wrote: > GetPref doesn't check the whitelist, so |value| could be null if the settings > page uses a bad pref key, leading to a segfault. Do we care? (for non-CrOS > prefs, GetPref returns a nullptr for an invalid key.) We probably do care, I'll fix that while I'm in here. https://codereview.chromium.org/1310373008/diff/100001/chrome/browser/extensi... chrome/browser/extensions/api/settings_private/prefs_util.cc:190: // also restricted to the owner, and on non Chrome OS we hide these controls On 2015/08/27 23:40:01, michaelpg wrote: > nit: "non-Chrome OS" really? :P https://codereview.chromium.org/1310373008/diff/100001/chrome/browser/extensi... chrome/browser/extensions/api/settings_private/prefs_util.cc:195: if (pref->IsManagedByCustodian()) On 2015/08/27 23:40:01, michaelpg wrote: > I'm confused about the difference between "managed by custodian" and > "supervisor-controlled". > > And when can a pref be managed (which implies admin policy) and also managed by > custodian? I'm a bit confused too. I didn't change this logic, but I'm not sure where oren got this from. The comment says: IsManaged() says: "Returns true if the Preference is managed, i.e. set by an admin policy. Since managed prefs have the highest priority, this also indicates whether the pref is actually being controlled by the policy setting." IsManagedByCustondian() says: "Returns true if the Preference is controlled by the custodian of the supervised user. Since a supervised user is not expected to have an admin policy, this is the controlling pref if set." I don't see any other references to IsManagedByCustondian(); maybe it was in the original code and pulled? I'm going to eliminate that logic. In practice we don't currently differentiate between 'USER' and "DEVICE' anyway. https://codereview.chromium.org/1310373008/diff/100001/chrome/browser/extensi... chrome/browser/extensions/api/settings_private/prefs_util.cc:198: source = settings_api::PolicySource::POLICY_SOURCE_USER; On 2015/08/27 23:40:01, michaelpg wrote: > When is a preference "managed", but not mandated by enterprise policy or a > supervisor? I believe that desktop chrome allows for managed preferences, but I'm not sure how that works. https://codereview.chromium.org/1310373008/diff/100001/chrome/browser/extensi... chrome/browser/extensions/api/settings_private/prefs_util.cc:199: if (pref->IsRecommended()) { On 2015/08/27 23:40:01, michaelpg wrote: > Another point of confusion :-) How can a pref be in the managed store and the > recommended store at the same time? Seems to me if it's in the managed store, > PrefValueStore::ControllingPrefStoreForPref will always return MANAGED_STORE, so > PrefValueStore::PrefValueFromRecommendedStore will be false. Wow, the existing logic is just... wrong. Looking at the original source and matching the logic to it. https://codereview.chromium.org/1310373008/diff/100001/chrome/browser/extensi... chrome/browser/extensions/api/settings_private/prefs_util.cc:212: source = settings_api::PolicySource::POLICY_SOURCE_USER; On 2015/08/27 23:40:01, michaelpg wrote: > What is a user policy? In this case the pref is controlled by an extension or > the command-line store, right? Yeah, we appear to be conflating things here. The original code sets a 'disabled' property; I am going to add a 'readOnly' property instead. https://codereview.chromium.org/1310373008/diff/100001/chrome/browser/extensi... chrome/browser/extensions/api/settings_private/prefs_util.cc:363: bool PrefsUtil::IsPrefSupervisorControlled(const std::string& pref_name) { On 2015/08/27 23:40:01, michaelpg wrote: > Does PrefService::IsManagedByCustodian result in different behavior? This should also be 'readOnly' = true to match the current behavior. https://codereview.chromium.org/1310373008/diff/100001/chrome/browser/extensi... chrome/browser/extensions/api/settings_private/prefs_util.cc:373: if (pref_name == prefs::kWakeOnWifiSsid) { On 2015/08/27 23:40:01, michaelpg wrote: > #if defined(OS_CHROMEOS)? Done. https://codereview.chromium.org/1310373008/diff/100001/chrome/browser/extensi... chrome/browser/extensions/api/settings_private/prefs_util.cc:390: IsPrefSupervisorControlled(pref_name) || On 2015/08/28 00:20:21, Dan Beam wrote: > On 2015/08/27 23:40:01, michaelpg wrote: > > Why isn't it enough to check pref->IsUserModifiable()? > > +1 I think actually that should be sufficient. Eliminating the cruft. https://codereview.chromium.org/1310373008/diff/100001/chrome/browser/extensi... File chrome/browser/extensions/api/settings_private/prefs_util.h (right): https://codereview.chromium.org/1310373008/diff/100001/chrome/browser/extensi... chrome/browser/extensions/api/settings_private/prefs_util.h:67: // the owner, and |profile_| is not the owner profile. On 2015/08/28 00:20:22, Dan Beam wrote: > On 2015/08/27 23:40:01, michaelpg wrote: > > Chrome OS only. > > can it be in if-defs, then? Done. https://codereview.chromium.org/1310373008/diff/100001/chrome/browser/extensi... chrome/browser/extensions/api/settings_private/prefs_util.h:75: // the primary user, and |profile_| is not the primary profile. On 2015/08/27 23:40:01, michaelpg wrote: > Chrome OS only, right? Yes, done. https://codereview.chromium.org/1310373008/diff/100001/chrome/browser/resourc... File chrome/browser/resources/settings/checkbox/checkbox.css (right): https://codereview.chromium.org/1310373008/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/checkbox/checkbox.css:11: #outerDiv { On 2015/08/28 00:20:22, Dan Beam wrote: > idsLikeThis or ids-like-this? Done. https://codereview.chromium.org/1310373008/diff/100001/chrome/browser/resourc... File chrome/browser/resources/settings/checkbox/checkbox.js (right): https://codereview.chromium.org/1310373008/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/checkbox/checkbox.js:27: value: null On 2015/08/28 00:20:22, Dan Beam wrote: > nit: we are not IE, feel free to add trailing , so the blame doesn't change for > the last line of an object every time a new property is added I forget where it's OK and where it's not; I think maybe it's not OK in closure typedef'd objects? Anyway, done throughot. https://codereview.chromium.org/1310373008/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/checkbox/checkbox.js:86: if (change.path == 'pref.policySource' || On 2015/08/27 23:40:01, michaelpg wrote: > else if Done. https://codereview.chromium.org/1310373008/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/checkbox/checkbox.js:108: this.checkboxDisabled = this.disabled || this.isSettingDisabled_(this.pref); On 2015/08/27 23:40:01, michaelpg wrote: > nit: instead of adding listeners like disabledChanged and pref.*, can you bind > the HTML "disabled" attribute to a function which checks pref, > pref.policyEnforcement and disabled Probably... I'll look into that after I merge with you prefs.js CL. https://codereview.chromium.org/1310373008/diff/100001/chrome/browser/resourc... File chrome/browser/resources/settings/prefs/prefs_types.js (right): https://codereview.chromium.org/1310373008/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/prefs/prefs_types.js:12: * isInitialized: (boolean|undefined) On 2015/08/28 00:20:22, Dan Beam wrote: > indent off Done. https://codereview.chromium.org/1310373008/diff/100001/chrome/common/extensio... File chrome/common/extensions/api/settings_private.idl (right): https://codereview.chromium.org/1310373008/diff/100001/chrome/common/extensio... chrome/common/extensions/api/settings_private.idl:11: // Source of a restricted pref, either by policy or other source. On 2015/08/27 23:40:01, michaelpg wrote: > Source of a restricted pref (e.g., policy). Hmm... I like it as-is. https://codereview.chromium.org/1310373008/diff/100001/ui/webui/resources/cr_... File ui/webui/resources/cr_elements/v1_0/policy/cr_policy_indicator.css (right): https://codereview.chromium.org/1310373008/diff/100001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/v1_0/policy/cr_policy_indicator.css:6: the use of overflow: hidden. */ On 2015/08/28 00:20:22, Dan Beam wrote: > nit: > > /* ... > * ... */ > ^ Done. https://codereview.chromium.org/1310373008/diff/100001/ui/webui/resources/cr_... File ui/webui/resources/cr_elements/v1_0/policy/cr_policy_indicator.html (right): https://codereview.chromium.org/1310373008/diff/100001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/v1_0/policy/cr_policy_indicator.html:11: icon="[[getIcon_(indicatorType)]]"/> On 2015/08/28 00:20:22, Dan Beam wrote: > I think you need a full </iron-icon> instead of <iron-icon/> Done. https://codereview.chromium.org/1310373008/diff/100001/ui/webui/resources/cr_... File ui/webui/resources/cr_elements/v1_0/policy/cr_policy_indicator.js (right): https://codereview.chromium.org/1310373008/diff/100001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/v1_0/policy/cr_policy_indicator.js:49: return; On 2015/08/28 00:20:22, Dan Beam wrote: > nit: combine ifs Done. https://codereview.chromium.org/1310373008/diff/100001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/v1_0/policy/cr_policy_indicator.js:56: type = IndicatorType.PRIMARY; On 2015/08/28 00:20:22, Dan Beam wrote: > why PRIMARY vs PRIMARY_USER? Done. https://codereview.chromium.org/1310373008/diff/100001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/v1_0/policy/cr_policy_indicator.js:60: type = IndicatorType.USER_POLICY; On 2015/08/28 00:20:22, Dan Beam wrote: > why USER vs USER_POLICY? Yeah, I should fix that in the idl. Done. https://codereview.chromium.org/1310373008/diff/100001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/v1_0/policy/cr_policy_indicator.js:62: type = IndicatorType.DEVICE_POLICY; On 2015/08/28 00:20:22, Dan Beam wrote: > why DEVICE vs DEVICE_POLICY? Done. https://codereview.chromium.org/1310373008/diff/100001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/v1_0/policy/cr_policy_indicator.js:77: showIndicator_: function(type) { return type != IndicatorType.NONE; }, On 2015/08/28 00:20:22, Dan Beam wrote: > this sounds like a method to show an indicator, not whether an indicator should > be shown Done. https://codereview.chromium.org/1310373008/diff/100001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/v1_0/policy/cr_policy_indicator.js:97: } On 2015/08/28 00:20:22, Dan Beam wrote: > should there be an assertNotReached() here? Done (hadn't seen that in our JS before).
https://codereview.chromium.org/1310373008/diff/100001/chrome/browser/extensi... File chrome/browser/extensions/api/settings_private/prefs_util.cc (right): https://codereview.chromium.org/1310373008/diff/100001/chrome/browser/extensi... chrome/browser/extensions/api/settings_private/prefs_util.cc:42: namespace settings_api = api::settings_private; On 2015/08/28 23:18:07, stevenjb wrote: > On 2015/08/28 00:20:22, Dan Beam wrote: > > can you do this in a separate CL? > > I'll undo it. I know the churn sucks. It helped align some stuff, but in > hindsight is was a bad call. churn is not bad. big diffs are bad. because they make it hard to review
OK, this is working again with the prefs.js changes and is ready for review, PTAL.
https://codereview.chromium.org/1310373008/diff/100001/chrome/browser/resourc... File chrome/browser/resources/settings/checkbox/checkbox.js (right): https://codereview.chromium.org/1310373008/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/checkbox/checkbox.js:27: value: null On 2015/08/28 23:18:07, stevenjb wrote: > On 2015/08/28 00:20:22, Dan Beam wrote: > > nit: we are not IE, feel free to add trailing , so the blame doesn't change > for > > the last line of an object every time a new property is added > > I forget where it's OK and where it's not; I think maybe it's not OK in closure > typedef'd objects? Anyway, done throughot. also not OK in JSON https://codereview.chromium.org/1310373008/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/checkbox/checkbox.js:108: this.checkboxDisabled = this.disabled || this.isSettingDisabled_(this.pref); On 2015/08/28 23:18:07, stevenjb wrote: > On 2015/08/27 23:40:01, michaelpg wrote: > > nit: instead of adding listeners like disabledChanged and pref.*, can you bind > > the HTML "disabled" attribute to a function which checks pref, > > pref.policyEnforcement and disabled > > Probably... I'll look into that after I merge with you prefs.js CL. could you revisit this? I think having a computed attribute would make the code simpler. https://codereview.chromium.org/1310373008/diff/190001/chrome/browser/extensi... File chrome/browser/extensions/api/settings_private/prefs_util.cc (right): https://codereview.chromium.org/1310373008/diff/190001/chrome/browser/extensi... chrome/browser/extensions/api/settings_private/prefs_util.cc:172: if (IsCrosSetting(name)) { no need to check this on non-Chrome OS https://codereview.chromium.org/1310373008/diff/190001/chrome/browser/extensi... chrome/browser/extensions/api/settings_private/prefs_util.cc:186: if (IsPrefPrimaryUserControlled(name)) { shouldn't we check enterprise policy first, since that would have a higher precedence? https://codereview.chromium.org/1310373008/diff/190001/chrome/browser/extensi... chrome/browser/extensions/api/settings_private/prefs_util.cc:193: if (IsPrefOwnerControlled(name)) { this ordering also doesn't look right: for managed devices, IsPrefOwnerControlled is equivalent to IsPrivilegedCrosSetting. so this would always trigger for privileged preferences, and we'd set the policy source to Owner instead of Policy. https://codereview.chromium.org/1310373008/diff/190001/chrome/browser/extensi... chrome/browser/extensions/api/settings_private/prefs_util.cc:409: return true; nit: return IsSupervised(); or move this check to the beginning: if (!IsSupervised) return false; return pref_name == Guest || pref_name == AddPerson; https://codereview.chromium.org/1310373008/diff/190001/chrome/browser/extensi... File chrome/browser/extensions/api/settings_private/prefs_util.h (right): https://codereview.chromium.org/1310373008/diff/190001/chrome/browser/extensi... chrome/browser/extensions/api/settings_private/prefs_util.h:100: SetPrefResult SetCrosSettingsPref(const std::string& name, #if defined(OS_CHROMEOS) https://codereview.chromium.org/1310373008/diff/190001/chrome/browser/resourc... File chrome/browser/resources/settings/checkbox/checkbox.js (right): https://codereview.chromium.org/1310373008/diff/190001/chrome/browser/resourc... chrome/browser/resources/settings/checkbox/checkbox.js:43: /** Set to disable the element programatically. */ programatically... or via HTML, like any other property. https://codereview.chromium.org/1310373008/diff/190001/chrome/browser/resourc... chrome/browser/resources/settings/checkbox/checkbox.js:52: * false, otherwise may be set to false by a pref policy. if you're going to keep this, make it @private https://codereview.chromium.org/1310373008/diff/190001/chrome/browser/resourc... chrome/browser/resources/settings/checkbox/checkbox.js:107: this.set('pref.value', this.getNewValue_(this.checked)); this.set('pref.value', foo) does nothing if this.pref is not defined. I don't think it's bad to check "if (!this.pref)", but because in this case you're only doing it to be "safe", I would leave it out. https://codereview.chromium.org/1310373008/diff/190001/chrome/common/extensio... File chrome/common/extensions/api/settings_private.idl (right): https://codereview.chromium.org/1310373008/diff/190001/chrome/common/extensio... chrome/common/extensions/api/settings_private.idl:44: // The extension Id if policySource == EXTENSION. nit: ID https://codereview.chromium.org/1310373008/diff/190001/chrome/common/extensio... chrome/common/extensions/api/settings_private.idl:47: // True if the pref is not controlled by a policy or user, but it can not be opt nit: cannot https://codereview.chromium.org/1310373008/diff/190001/ui/webui/resources/cr_... File ui/webui/resources/cr_elements/v1_0/policy/cr_policy_indicator.js (right): https://codereview.chromium.org/1310373008/diff/190001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/v1_0/policy/cr_policy_indicator.js:35: indicatorType: {type: Object, value: IndicatorType.NONE}, type: String https://codereview.chromium.org/1310373008/diff/190001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/v1_0/policy/cr_policy_indicator.js:42: * @param {?{path: string, value: *}} change o rly?
Patchset #10 (id:210001) has been deleted
Patchset #10 (id:230001) has been deleted
stevenjb@chromium.org changed reviewers: + kalman@chromium.org
+kalman@ for settings_private.idl https://codereview.chromium.org/1310373008/diff/100001/chrome/browser/resourc... File chrome/browser/resources/settings/checkbox/checkbox.js (right): https://codereview.chromium.org/1310373008/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/checkbox/checkbox.js:108: this.checkboxDisabled = this.disabled || this.isSettingDisabled_(this.pref); On 2015/09/14 05:14:51, michaelpg wrote: > On 2015/08/28 23:18:07, stevenjb wrote: > > On 2015/08/27 23:40:01, michaelpg wrote: > > > nit: instead of adding listeners like disabledChanged and pref.*, can you > bind > > > the HTML "disabled" attribute to a function which checks pref, > > > pref.policyEnforcement and disabled > > > > Probably... I'll look into that after I merge with you prefs.js CL. > > could you revisit this? I think having a computed attribute would make the code > simpler. Done. https://codereview.chromium.org/1310373008/diff/190001/chrome/browser/extensi... File chrome/browser/extensions/api/settings_private/prefs_util.cc (right): https://codereview.chromium.org/1310373008/diff/190001/chrome/browser/extensi... chrome/browser/extensions/api/settings_private/prefs_util.cc:172: if (IsCrosSetting(name)) { On 2015/09/14 05:14:51, michaelpg wrote: > no need to check this on non-Chrome OS Maybe fix in a re-factor. https://codereview.chromium.org/1310373008/diff/190001/chrome/browser/extensi... chrome/browser/extensions/api/settings_private/prefs_util.cc:186: if (IsPrefPrimaryUserControlled(name)) { On 2015/09/14 05:14:51, michaelpg wrote: > shouldn't we check enterprise policy first, since that would have a higher > precedence? So, with the last iteration, I did think about the order, rather than using the existing (and I am pretty certain mostly arbitrary) order. * First off, it is unlikely that more than one of these will be true. * For secondary users, the owner or policy state is irrelevant if the setting is controlled by the primary user so we should show the primary user indicator first. * For owner controlled settings, the policy for the logged in user is also irrelevant. https://codereview.chromium.org/1310373008/diff/190001/chrome/browser/extensi... chrome/browser/extensions/api/settings_private/prefs_util.cc:193: if (IsPrefOwnerControlled(name)) { On 2015/09/14 05:14:51, michaelpg wrote: > this ordering also doesn't look right: for managed devices, > IsPrefOwnerControlled is equivalent to IsPrivilegedCrosSetting. so this would > always trigger for privileged preferences, and we'd set the policy source to > Owner instead of Policy. Which is correct; the policy for the logged in user isn't relevant for owner controlled settings. If it is controlled by a device policy that is arguably relevant, but in practice only for the owner. https://codereview.chromium.org/1310373008/diff/190001/chrome/browser/extensi... chrome/browser/extensions/api/settings_private/prefs_util.cc:409: return true; On 2015/09/14 05:14:51, michaelpg wrote: > nit: return IsSupervised(); > > or move this check to the beginning: > if (!IsSupervised) > return false; > return pref_name == Guest || pref_name == AddPerson; Did the opposite since IsSupervised() has some cost to it, but simplified. https://codereview.chromium.org/1310373008/diff/190001/chrome/browser/extensi... File chrome/browser/extensions/api/settings_private/prefs_util.h (right): https://codereview.chromium.org/1310373008/diff/190001/chrome/browser/extensi... chrome/browser/extensions/api/settings_private/prefs_util.h:100: SetPrefResult SetCrosSettingsPref(const std::string& name, On 2015/09/14 05:14:51, michaelpg wrote: > #if defined(OS_CHROMEOS) Currently IsCrosSetting, etc, are all implemented regardless. Personally I would re-factor this code to have a prefs_util_chromeos derived class that had all of the chromeos specific code, but for now I think it is fine as-is. https://codereview.chromium.org/1310373008/diff/190001/chrome/browser/resourc... File chrome/browser/resources/settings/checkbox/checkbox.js (right): https://codereview.chromium.org/1310373008/diff/190001/chrome/browser/resourc... chrome/browser/resources/settings/checkbox/checkbox.js:43: /** Set to disable the element programatically. */ On 2015/09/14 05:14:51, michaelpg wrote: > programatically... or via HTML, like any other property. Simplified not that checkboxDisabled_ is a method. https://codereview.chromium.org/1310373008/diff/190001/chrome/browser/resourc... chrome/browser/resources/settings/checkbox/checkbox.js:52: * false, otherwise may be set to false by a pref policy. On 2015/09/14 05:14:51, michaelpg wrote: > if you're going to keep this, make it @private Acknowledged. https://codereview.chromium.org/1310373008/diff/190001/chrome/browser/resourc... chrome/browser/resources/settings/checkbox/checkbox.js:107: this.set('pref.value', this.getNewValue_(this.checked)); On 2015/09/14 05:14:51, michaelpg wrote: > this.set('pref.value', foo) does nothing if this.pref is not defined. > > I don't think it's bad to check "if (!this.pref)", but because in this case > you're only doing it to be "safe", I would leave it out. I only changed this to match disabledChanged_, which is now gone, but done. https://codereview.chromium.org/1310373008/diff/190001/chrome/common/extensio... File chrome/common/extensions/api/settings_private.idl (right): https://codereview.chromium.org/1310373008/diff/190001/chrome/common/extensio... chrome/common/extensions/api/settings_private.idl:44: // The extension Id if policySource == EXTENSION. On 2015/09/14 05:14:51, michaelpg wrote: > nit: ID Done. https://codereview.chromium.org/1310373008/diff/190001/chrome/common/extensio... chrome/common/extensions/api/settings_private.idl:47: // True if the pref is not controlled by a policy or user, but it can not be On 2015/09/14 05:14:51, michaelpg wrote: > opt nit: cannot Acknowledged. https://codereview.chromium.org/1310373008/diff/190001/ui/webui/resources/cr_... File ui/webui/resources/cr_elements/v1_0/policy/cr_policy_indicator.js (right): https://codereview.chromium.org/1310373008/diff/190001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/v1_0/policy/cr_policy_indicator.js:35: indicatorType: {type: Object, value: IndicatorType.NONE}, On 2015/09/14 05:14:51, michaelpg wrote: > type: String Done. https://codereview.chromium.org/1310373008/diff/190001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/v1_0/policy/cr_policy_indicator.js:42: * @param {?{path: string, value: *}} change On 2015/09/14 05:14:51, michaelpg wrote: > o rly? ??
lgtm
lgtm https://codereview.chromium.org/1310373008/diff/250001/chrome/browser/extensi... File chrome/browser/extensions/api/settings_private/prefs_util.cc (right): https://codereview.chromium.org/1310373008/diff/250001/chrome/browser/extensi... chrome/browser/extensions/api/settings_private/prefs_util.cc:159: return nullptr; can we not rely on automagic scoped_ptr creation from nullptr here? https://codereview.chromium.org/1310373008/diff/250001/chrome/browser/resourc... File chrome/browser/resources/settings/prefs/prefs.js (right): https://codereview.chromium.org/1310373008/diff/250001/chrome/browser/resourc... chrome/browser/resources/settings/prefs/prefs.js:244: this.notifyPath('prefs.' + newPrefObj.key, newPrefObj); this will conflict with michaelpg@'s CL https://codereview.chromium.org/1310373008/diff/250001/ui/webui/resources/cr_... File ui/webui/resources/cr_elements/v1_0/policy/cr_policy_indicator.js (right): https://codereview.chromium.org/1310373008/diff/250001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/v1_0/policy/cr_policy_indicator.js:38: observers: ['prefPropertyChanged_(pref.*)'], nit: (pref.policySource, pref.policyEnforcement)
Patchset #11 (id:270001) has been deleted
https://codereview.chromium.org/1310373008/diff/250001/chrome/browser/extensi... File chrome/browser/extensions/api/settings_private/prefs_util.cc (right): https://codereview.chromium.org/1310373008/diff/250001/chrome/browser/extensi... chrome/browser/extensions/api/settings_private/prefs_util.cc:159: return nullptr; On 2015/09/16 01:31:44, Dan Beam wrote: > can we not rely on automagic scoped_ptr creation from nullptr here? I'm not sure I parsed that correctly, but: explicit scoped_ptr_impl(T* p) : data_(p) {} nullptr is a valid T*, no need for an explicit cast. https://codereview.chromium.org/1310373008/diff/250001/chrome/browser/resourc... File chrome/browser/resources/settings/prefs/prefs.js (right): https://codereview.chromium.org/1310373008/diff/250001/chrome/browser/resourc... chrome/browser/resources/settings/prefs/prefs.js:244: this.notifyPath('prefs.' + newPrefObj.key, newPrefObj); On 2015/09/16 01:31:44, Dan Beam wrote: > this will conflict with michaelpg@'s CL One of us will have to resolve the comment differences. I already discussed my confusion with the older comment. https://codereview.chromium.org/1310373008/diff/250001/ui/webui/resources/cr_... File ui/webui/resources/cr_elements/v1_0/policy/cr_policy_indicator.js (right): https://codereview.chromium.org/1310373008/diff/250001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/v1_0/policy/cr_policy_indicator.js:38: observers: ['prefPropertyChanged_(pref.*)'], On 2015/09/16 01:31:44, Dan Beam wrote: > nit: (pref.policySource, pref.policyEnforcement) Not sure that is any more efficient, but done.
https://codereview.chromium.org/1310373008/diff/190001/chrome/browser/extensi... File chrome/browser/extensions/api/settings_private/prefs_util.cc (right): https://codereview.chromium.org/1310373008/diff/190001/chrome/browser/extensi... chrome/browser/extensions/api/settings_private/prefs_util.cc:186: if (IsPrefPrimaryUserControlled(name)) { On 2015/09/14 23:05:30, stevenjb wrote: > On 2015/09/14 05:14:51, michaelpg wrote: > > shouldn't we check enterprise policy first, since that would have a higher > > precedence? > > So, with the last iteration, I did think about the order, rather than using the > existing (and I am pretty certain mostly arbitrary) order. > * First off, it is unlikely that more than one of these will be true. granted > * For secondary users, the owner or policy state is irrelevant if the setting is > controlled by the primary user so we should show the primary user indicator > first. Can there be different policies per user, or are policies always machine-wide? > * For owner controlled settings, the policy for the logged in user is also > irrelevant. https://codereview.chromium.org/1310373008/diff/190001/chrome/browser/extensi... chrome/browser/extensions/api/settings_private/prefs_util.cc:193: if (IsPrefOwnerControlled(name)) { On 2015/09/14 23:05:30, stevenjb wrote: > On 2015/09/14 05:14:51, michaelpg wrote: > > this ordering also doesn't look right: for managed devices, > > IsPrefOwnerControlled is equivalent to IsPrivilegedCrosSetting. so this would > > always trigger for privileged preferences, and we'd set the policy source to > > Owner instead of Policy. > > Which is correct; the policy for the logged in user isn't relevant for owner > controlled settings. If it is controlled by a device policy that is arguably > relevant, but in practice only for the owner. > If it is an enrolled device there *is no owner*, but my fear is that IsPrefOwnerControlled returns true anyway. https://codereview.chromium.org/1310373008/diff/190001/ui/webui/resources/cr_... File ui/webui/resources/cr_elements/v1_0/policy/cr_policy_indicator.js (right): https://codereview.chromium.org/1310373008/diff/190001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/v1_0/policy/cr_policy_indicator.js:42: * @param {?{path: string, value: *}} change On 2015/09/14 23:05:31, stevenjb wrote: > On 2015/09/14 05:14:51, michaelpg wrote: > > o rly? > > ?? I don't think change can ever be null: {!{path: string, value: *}} also, change includes a "base" property which you don't list -- I don't care if you leave it out, but you do include "value" which you also don't use. https://codereview.chromium.org/1310373008/diff/250001/chrome/browser/resourc... File chrome/browser/resources/settings/checkbox/checkbox.js (right): https://codereview.chromium.org/1310373008/diff/250001/chrome/browser/resourc... chrome/browser/resources/settings/checkbox/checkbox.js:73: * @param {?chrome.settingsPrivate.PrefObject} prefValue the type of value is {*} -- pref is the PrefObject https://codereview.chromium.org/1310373008/diff/250001/chrome/browser/resourc... chrome/browser/resources/settings/checkbox/checkbox.js:78: if (prefValue !== undefined) should be able to remove this upon rebase https://codereview.chromium.org/1310373008/diff/290001/chrome/browser/resourc... File chrome/browser/resources/settings/prefs/prefs.js (right): https://codereview.chromium.org/1310373008/diff/290001/chrome/browser/resourc... chrome/browser/resources/settings/prefs/prefs.js:244: this.notifyPath('prefs.' + newPrefObj.key, newPrefObj); You'll need to rebase this on https://codereview.chromium.org/1346833003/ (or rather, you can just remove this change and that CL does it for you).
https://codereview.chromium.org/1310373008/diff/290001/chrome/browser/extensi... File chrome/browser/extensions/api/settings_private/prefs_util.cc (right): https://codereview.chromium.org/1310373008/diff/290001/chrome/browser/extensi... chrome/browser/extensions/api/settings_private/prefs_util.cc:161: return nullptr; why not return pref_object.Pass(); or if (value) { pref_object->...; } ? https://codereview.chromium.org/1310373008/diff/290001/ui/webui/resources/cr_... File ui/webui/resources/cr_elements/v1_0/policy/cr_policy_indicator.js (right): https://codereview.chromium.org/1310373008/diff/290001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/v1_0/policy/cr_policy_indicator.js:40: 'prefPolicyChanged_(pref.policyEnforcement)' 'prefPolicyChanged_(pref.policySource, pref.policyEnforcement)'
PTAL https://codereview.chromium.org/1310373008/diff/190001/chrome/browser/extensi... File chrome/browser/extensions/api/settings_private/prefs_util.cc (right): https://codereview.chromium.org/1310373008/diff/190001/chrome/browser/extensi... chrome/browser/extensions/api/settings_private/prefs_util.cc:186: if (IsPrefPrimaryUserControlled(name)) { On 2015/09/16 18:58:52, michaelpg wrote: > On 2015/09/14 23:05:30, stevenjb wrote: > > On 2015/09/14 05:14:51, michaelpg wrote: > > > shouldn't we check enterprise policy first, since that would have a higher > > > precedence? > > > > So, with the last iteration, I did think about the order, rather than using > the > > existing (and I am pretty certain mostly arbitrary) order. > > * First off, it is unlikely that more than one of these will be true. > > granted > > > * For secondary users, the owner or policy state is irrelevant if the setting > is > > controlled by the primary user so we should show the primary user indicator > > first. > > Can there be different policies per user, or are policies always machine-wide? > > > * For owner controlled settings, the policy for the logged in user is also > > irrelevant. Acknowledged. https://codereview.chromium.org/1310373008/diff/190001/chrome/browser/extensi... chrome/browser/extensions/api/settings_private/prefs_util.cc:193: if (IsPrefOwnerControlled(name)) { On 2015/09/16 18:58:52, michaelpg wrote: > On 2015/09/14 23:05:30, stevenjb wrote: > > On 2015/09/14 05:14:51, michaelpg wrote: > > > this ordering also doesn't look right: for managed devices, > > > IsPrefOwnerControlled is equivalent to IsPrivilegedCrosSetting. so this > would > > > always trigger for privileged preferences, and we'd set the policy source to > > > Owner instead of Policy. > > > > Which is correct; the policy for the logged in user isn't relevant for owner > > controlled settings. If it is controlled by a device policy that is arguably > > relevant, but in practice only for the owner. > > > > If it is an enrolled device there *is no owner*, but my fear is that > IsPrefOwnerControlled returns true anyway. Yeah, the logic there is fragile at best. I'll go ahead and reverse the order. https://codereview.chromium.org/1310373008/diff/190001/ui/webui/resources/cr_... File ui/webui/resources/cr_elements/v1_0/policy/cr_policy_indicator.js (right): https://codereview.chromium.org/1310373008/diff/190001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/v1_0/policy/cr_policy_indicator.js:42: * @param {?{path: string, value: *}} change On 2015/09/16 18:58:52, michaelpg wrote: > On 2015/09/14 23:05:31, stevenjb wrote: > > On 2015/09/14 05:14:51, michaelpg wrote: > > > o rly? > > > > ?? > > I don't think change can ever be null: > > {!{path: string, value: *}} > > also, change includes a "base" property which you don't list -- I don't care if > you leave it out, but you do include "value" which you also don't use. This got removed entirely in response to Dan's feedback, but yeah, removing value would have made sense (I was using it at one point). https://codereview.chromium.org/1310373008/diff/290001/chrome/browser/extensi... File chrome/browser/extensions/api/settings_private/prefs_util.cc (right): https://codereview.chromium.org/1310373008/diff/290001/chrome/browser/extensi... chrome/browser/extensions/api/settings_private/prefs_util.cc:161: return nullptr; On 2015/09/16 21:02:19, Dan Beam wrote: > why not > > return pref_object.Pass(); > > or > > if (value) { > pref_object->...; > } > > ? Oh, yeah, I see, that is weird, we should return either nullptr or an empty pref_object if we don't set anything, not both. Examining the code, this should not logically happen, so changed to a DCHECK. https://codereview.chromium.org/1310373008/diff/290001/chrome/browser/resourc... File chrome/browser/resources/settings/prefs/prefs.js (right): https://codereview.chromium.org/1310373008/diff/290001/chrome/browser/resourc... chrome/browser/resources/settings/prefs/prefs.js:244: this.notifyPath('prefs.' + newPrefObj.key, newPrefObj); On 2015/09/16 18:58:52, michaelpg wrote: > You'll need to rebase this on https://codereview.chromium.org/1346833003/ (or > rather, you can just remove this change and that CL does it for you). Acknowledged. https://codereview.chromium.org/1310373008/diff/290001/ui/webui/resources/cr_... File ui/webui/resources/cr_elements/v1_0/policy/cr_policy_indicator.js (right): https://codereview.chromium.org/1310373008/diff/290001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/v1_0/policy/cr_policy_indicator.js:40: 'prefPolicyChanged_(pref.policyEnforcement)' On 2015/09/16 21:02:19, Dan Beam wrote: > 'prefPolicyChanged_(pref.policySource, pref.policyEnforcement)' Done.
https://codereview.chromium.org/1310373008/diff/250001/chrome/browser/resourc... File chrome/browser/resources/settings/checkbox/checkbox.js (right): https://codereview.chromium.org/1310373008/diff/250001/chrome/browser/resourc... chrome/browser/resources/settings/checkbox/checkbox.js:73: * @param {?chrome.settingsPrivate.PrefObject} prefValue On 2015/09/16 18:58:52, michaelpg wrote: > the type of value is {*} -- pref is the PrefObject Ah, right, pref.value. Done. https://codereview.chromium.org/1310373008/diff/250001/chrome/browser/resourc... chrome/browser/resources/settings/checkbox/checkbox.js:78: if (prefValue !== undefined) On 2015/09/16 18:58:52, michaelpg wrote: > should be able to remove this upon rebase Done.
lgtm https://codereview.chromium.org/1310373008/diff/290001/chrome/browser/extensi... File chrome/browser/extensions/api/settings_private/prefs_util.cc (right): https://codereview.chromium.org/1310373008/diff/290001/chrome/browser/extensi... chrome/browser/extensions/api/settings_private/prefs_util.cc:214: return pref_object.Pass(); I think this was fine
https://codereview.chromium.org/1310373008/diff/290001/chrome/browser/extensi... File chrome/browser/extensions/api/settings_private/prefs_util.cc (right): https://codereview.chromium.org/1310373008/diff/290001/chrome/browser/extensi... chrome/browser/extensions/api/settings_private/prefs_util.cc:214: return pref_object.Pass(); On 2015/09/16 21:46:23, Dan Beam wrote: > I think this was fine If IsCrosSetting() is true, we don't set |pref|, but we still want to check IsPrefOwnerControlled(name) below. This is... more than a little confusing. I'd really like to make it less confusing by pulling the cros specific logic (which applies to both IsCrosSetting() and non cros prefs) into a derived class, but for now I am about 98% certain that this is at least logically correct.
https://codereview.chromium.org/1310373008/diff/250001/ui/webui/resources/cr_... File ui/webui/resources/cr_elements/v1_0/policy/cr_policy_indicator.js (right): https://codereview.chromium.org/1310373008/diff/250001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/v1_0/policy/cr_policy_indicator.js:38: observers: ['prefPropertyChanged_(pref.*)'], On 2015/09/16 18:18:18, stevenjb wrote: > On 2015/09/16 01:31:44, Dan Beam wrote: > > nit: (pref.policySource, pref.policyEnforcement) > > Not sure that is any more efficient, but done. I just realized what you actually meant here; fixed more better now.
lgtm w/ a bunch of small things that probably don't matter https://codereview.chromium.org/1310373008/diff/350001/chrome/browser/extensi... File chrome/browser/extensions/api/settings_private/prefs_util.cc (right): https://codereview.chromium.org/1310373008/diff/350001/chrome/browser/extensi... chrome/browser/extensions/api/settings_private/prefs_util.cc:160: DCHECK(value); value shouldn't be NULL, but it could be, so a DCHECK isn't quite sufficient. maybe instead: if (value) { ...} else { NOTREACHED(); } https://codereview.chromium.org/1310373008/diff/350001/chrome/browser/extensi... chrome/browser/extensions/api/settings_private/prefs_util.cc:226: // situations apply, either badge is potentially relevent, so the order sp: relevant https://codereview.chromium.org/1310373008/diff/350001/chrome/browser/resourc... File chrome/browser/resources/settings/checkbox/checkbox.js (right): https://codereview.chromium.org/1310373008/diff/350001/chrome/browser/resourc... chrome/browser/resources/settings/checkbox/checkbox.js:71: * Polymer observer for pref.value nit: append "." https://codereview.chromium.org/1310373008/diff/350001/chrome/browser/resourc... chrome/browser/resources/settings/checkbox/checkbox.js:103: return disabled || (!!pref && !!pref -> pref (no need to convert null to boolean) https://codereview.chromium.org/1310373008/diff/350001/ui/webui/resources/cr_... File ui/webui/resources/cr_elements/v1_0/policy/cr_policy_indicator.css (right): https://codereview.chromium.org/1310373008/diff/350001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/v1_0/policy/cr_policy_indicator.css:6: * the use of overflow: hidden. */ nit: fix "the" alignment (and maybe move to be above the display: block line?) https://codereview.chromium.org/1310373008/diff/350001/ui/webui/resources/cr_... File ui/webui/resources/cr_elements/v1_0/policy/cr_policy_indicator.js (right): https://codereview.chromium.org/1310373008/diff/350001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/v1_0/policy/cr_policy_indicator.js:44: * @param {chrome.settingsPrivate.PolicySource} source can these be undefined?
https://codereview.chromium.org/1310373008/diff/350001/chrome/browser/extensi... File chrome/browser/extensions/api/settings_private/prefs_util.cc (right): https://codereview.chromium.org/1310373008/diff/350001/chrome/browser/extensi... chrome/browser/extensions/api/settings_private/prefs_util.cc:160: DCHECK(value); On 2015/09/17 02:19:25, michaelpg wrote: > value shouldn't be NULL, but it could be, so a DCHECK isn't quite sufficient. > maybe instead: > > if (value) { ...} else { NOTREACHED(); } GetPref() already has a NOTREACHED if it returns null, so we won't actually reach this, it is just documenting the behavior. https://codereview.chromium.org/1310373008/diff/350001/chrome/browser/extensi... chrome/browser/extensions/api/settings_private/prefs_util.cc:226: // situations apply, either badge is potentially relevent, so the order On 2015/09/17 02:19:25, michaelpg wrote: > sp: relevant Done. https://codereview.chromium.org/1310373008/diff/350001/chrome/browser/resourc... File chrome/browser/resources/settings/checkbox/checkbox.js (right): https://codereview.chromium.org/1310373008/diff/350001/chrome/browser/resourc... chrome/browser/resources/settings/checkbox/checkbox.js:71: * Polymer observer for pref.value On 2015/09/17 02:19:25, michaelpg wrote: > nit: append "." Done. https://codereview.chromium.org/1310373008/diff/350001/chrome/browser/resourc... chrome/browser/resources/settings/checkbox/checkbox.js:103: return disabled || (!!pref && On 2015/09/17 02:19:25, michaelpg wrote: > !!pref -> pref (no need to convert null to boolean) Actually, closure will complain without the !!. https://codereview.chromium.org/1310373008/diff/350001/ui/webui/resources/cr_... File ui/webui/resources/cr_elements/v1_0/policy/cr_policy_indicator.css (right): https://codereview.chromium.org/1310373008/diff/350001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/v1_0/policy/cr_policy_indicator.css:6: * the use of overflow: hidden. */ On 2015/09/17 02:19:25, michaelpg wrote: > nit: fix "the" alignment > > (and maybe move to be above the display: block line?) Fixed the alignment. https://codereview.chromium.org/1310373008/diff/350001/ui/webui/resources/cr_... File ui/webui/resources/cr_elements/v1_0/policy/cr_policy_indicator.js (right): https://codereview.chromium.org/1310373008/diff/350001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/v1_0/policy/cr_policy_indicator.js:44: * @param {chrome.settingsPrivate.PolicySource} source On 2015/09/17 02:19:25, michaelpg wrote: > can these be undefined? Nope: https://www.polymer-project.org/1.0/docs/devguide/properties.html#multi-prope...
The CQ bit was checked by stevenjb@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kalman@chromium.org, dbeam@chromium.org, michaelpg@chromium.org Link to the patchset: https://codereview.chromium.org/1310373008/#ps390001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1310373008/390001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1310373008/390001
Message was sent while issue was closed.
Committed patchset #16 (id:390001)
Message was sent while issue was closed.
Patchset 16 (id:??) landed as https://crrev.com/7182593974f9788691c98bca3943d377b22894c3 Cr-Commit-Position: refs/heads/master@{#349459} |
