Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1367413002:
Add user field to cr-policy-indicator (Closed)
|
Created:
5 years, 2 months ago by michaelpg Modified:
5 years, 2 months ago CC:
chromium-reviews, michaelpg+watch-md-settings_chromium.org, dbeam+watch-settings_chromium.org, stevenjb+watch-md-settings_chromium.org, oshima+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd user field to cr-policy-indicator
Add a field for user id for when an indicator is not directly related
to a pref.
BUG=521791
Committed: https://crrev.com/6ea79ace485b9f94cfcefabf7a820f45b56e76a2
Cr-Commit-Position: refs/heads/master@{#351215}
Patch Set 1 #Patch Set 2 : #
Total comments: 16
Patch Set 3 : Rebase on 1376553002 #Patch Set 4 : feedback #Patch Set 5 : (fix rebase) #
Depends on Patchset: Dependent Patchsets: Messages
Total messages: 31 (12 generated)
michaelpg@chromium.org changed reviewers: + stevenjb@chromium.org
CrOS settings have rights too!
https://codereview.chromium.org/1367413002/diff/20001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/v1_0/policy/cr_policy_indicator.js (right): https://codereview.chromium.org/1367413002/diff/20001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/v1_0/policy/cr_policy_indicator.js:34: pref: {type: Object, value: null}, Why are we reversing this? I thought that initializing to null resulted in additional change events? https://codereview.chromium.org/1367413002/diff/20001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/v1_0/policy/cr_policy_indicator.js:34: pref: {type: Object, value: null}, Why are we reversing this? I thought that initializing to null resulted in additional change events? https://codereview.chromium.org/1367413002/diff/20001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/v1_0/policy/cr_policy_indicator.js:38: * correspond to a pref (Chrome OS only). nit: Explicitly say that this is only used when |pref| is undefined (or null... if null is better let's discuss at today's standup)
https://codereview.chromium.org/1367413002/diff/20001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/v1_0/policy/cr_policy_indicator.js (right): https://codereview.chromium.org/1367413002/diff/20001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/v1_0/policy/cr_policy_indicator.js:34: pref: {type: Object, value: null}, On 2015/09/28 16:25:12, stevenjb wrote: > Why are we reversing this? The function for the computed annotation isn't called until all properties in the function are !== undefined, so we need a default value other than undefined. https://www.polymer-project.org/1.0/docs/devguide/properties.html#computed-pr... > I thought that initializing to null resulted in > additional change events? In this case the additional work would be very slight, although I agree it's not entirely optimal. https://codereview.chromium.org/1367413002/diff/20001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/v1_0/policy/cr_policy_indicator.js:38: * correspond to a pref (Chrome OS only). On 2015/09/28 16:25:12, stevenjb wrote: > nit: Explicitly say that this is only used when |pref| is undefined (or null... > if null is better let's discuss at today's standup) Will do. Can you think of a better way to achieve this CL, though? I'm not sure I like having a user and a pref property and having one of them always ignored. Maybe if we take a single object which "implements" a common interface like { policySource, policySourceName, policyEnforcement } ?
https://codereview.chromium.org/1367413002/diff/20001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/v1_0/policy/cr_policy_indicator.js (right): https://codereview.chromium.org/1367413002/diff/20001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/v1_0/policy/cr_policy_indicator.js:34: pref: {type: Object, value: null}, On 2015/09/28 16:54:00, michaelpg wrote: > On 2015/09/28 16:25:12, stevenjb wrote: > > Why are we reversing this? > > The function for the computed annotation isn't called until all properties in > the function are !== undefined, so we need a default value other than undefined. > > https://www.polymer-project.org/1.0/docs/devguide/properties.html#computed-pr... > > > I thought that initializing to null resulted in > > additional change events? > > In this case the additional work would be very slight, although I agree it's not > entirely optimal. Ugh, that's right. I do realize it's a minor optimization - just thinking in terms of patterns and precedent. Since this is somewhat subtle, let's explicitly document optional parameters like this, e.g. * Optional preference object associated with the indicator. Initialized to null so that computed functions will get called if this is never set. https://codereview.chromium.org/1367413002/diff/20001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/v1_0/policy/cr_policy_indicator.js:38: * correspond to a pref (Chrome OS only). On 2015/09/28 16:54:00, michaelpg wrote: > On 2015/09/28 16:25:12, stevenjb wrote: > > nit: Explicitly say that this is only used when |pref| is undefined (or > null... > > if null is better let's discuss at today's standup) > > Will do. Can you think of a better way to achieve this CL, though? I'm not sure > I like having a user and a pref property and having one of them always ignored. > > Maybe if we take a single object which "implements" a common interface like { > policySource, policySourceName, policyEnforcement } ? I may want to do some re-factoring to support networking indicators which aren't associated with a pref, at which point I can clean this up a bit. For now, I think using a similar 'Optional...' comment to what I suggested above would be clear enough. https://codereview.chromium.org/1367413002/diff/20001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/v1_0/policy/cr_policy_indicator.js:38: * correspond to a pref (Chrome OS only). On 2015/09/28 16:54:00, michaelpg wrote: > On 2015/09/28 16:25:12, stevenjb wrote: > > nit: Explicitly say that this is only used when |pref| is undefined (or > null... > > if null is better let's discuss at today's standup) > > Will do. Can you think of a better way to achieve this CL, though? I'm not sure > I like having a user and a pref property and having one of them always ignored. > > Maybe if we take a single object which "implements" a common interface like { > policySource, policySourceName, policyEnforcement } ? I may want to do some re-factoring to support networking indicators which aren't associated with a pref, at which point I can clean this up a bit. For now, I think using a similar 'Optional...' comment to what I suggested above would be clear enough.
dbeam@chromium.org changed reviewers: + dbeam@chromium.org
https://codereview.chromium.org/1367413002/diff/20001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/v1_0/policy/cr_policy_indicator.js (right): https://codereview.chromium.org/1367413002/diff/20001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/v1_0/policy/cr_policy_indicator.js:21: }; i would do things like this separately, as this makes your CL 98% noise https://codereview.chromium.org/1367413002/diff/20001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/v1_0/policy/cr_policy_indicator.js:40: user: {type: String, value: ''}, can this be private? https://codereview.chromium.org/1367413002/diff/20001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/v1_0/policy/cr_policy_indicator.js:140: if (pref.value == pref.recommendedValue) what if !pref?
https://codereview.chromium.org/1367413002/diff/20001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/v1_0/policy/cr_policy_indicator.js (right): https://codereview.chromium.org/1367413002/diff/20001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/v1_0/policy/cr_policy_indicator.js:21: }; On 2015/09/28 17:31:37, Dan Beam wrote: > i would do things like this separately, as this makes your CL 98% noise Very good point. I've uploaded the Type changes at https://codereview.chromium.org/1376553002. I re-uploaded my original CL rebased on those changes as Patch Set 3. Patch Set 4 then applies the feedback. https://codereview.chromium.org/1367413002/diff/20001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/v1_0/policy/cr_policy_indicator.js:34: pref: {type: Object, value: null}, On 2015/09/28 17:20:12, stevenjb wrote: > On 2015/09/28 16:54:00, michaelpg wrote: > > On 2015/09/28 16:25:12, stevenjb wrote: > > > Why are we reversing this? > > > > The function for the computed annotation isn't called until all properties in > > the function are !== undefined, so we need a default value other than > undefined. > > > > > https://www.polymer-project.org/1.0/docs/devguide/properties.html#computed-pr... > > > > > I thought that initializing to null resulted in > > > additional change events? > > > > In this case the additional work would be very slight, although I agree it's > not > > entirely optimal. > > Ugh, that's right. I do realize it's a minor optimization - just thinking in > terms of patterns and precedent. > > Since this is somewhat subtle, let's explicitly document optional parameters > like this, e.g. > * Optional preference object associated with the indicator. Initialized to null > so that computed functions will get called if this is never set. Done. https://codereview.chromium.org/1367413002/diff/20001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/v1_0/policy/cr_policy_indicator.js:38: * correspond to a pref (Chrome OS only). On 2015/09/28 17:20:12, stevenjb wrote: > On 2015/09/28 16:54:00, michaelpg wrote: > > On 2015/09/28 16:25:12, stevenjb wrote: > > > nit: Explicitly say that this is only used when |pref| is undefined (or > > null... > > > if null is better let's discuss at today's standup) > > > > Will do. Can you think of a better way to achieve this CL, though? I'm not > sure > > I like having a user and a pref property and having one of them always > ignored. > > > > Maybe if we take a single object which "implements" a common interface like { > > policySource, policySourceName, policyEnforcement } ? > > I may want to do some re-factoring to support networking indicators which aren't > associated with a pref, at which point I can clean this up a bit. > > For now, I think using a similar 'Optional...' comment to what I suggested above > would be clear enough. Done. https://codereview.chromium.org/1367413002/diff/20001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/v1_0/policy/cr_policy_indicator.js:40: user: {type: String, value: ''}, On 2015/09/28 17:31:37, Dan Beam wrote: > can this be private? No, this has to be provided by the host. Perhaps it's not apparent that the user controlling the setting is not the user associated with the browser context -- could be the owner, primary user, or supervisor. Does renaming to controllingUser help? https://codereview.chromium.org/1367413002/diff/20001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/v1_0/policy/cr_policy_indicator.js:140: if (pref.value == pref.recommendedValue) On 2015/09/28 17:31:37, Dan Beam wrote: > what if !pref? good catch, done
lgtm
so where is .controllingUser set?
On 2015/09/28 20:10:47, Dan Beam wrote: > so where is .controllingUser set? Same as .pref: it's set by the host when it creates the custom element. <policy-indicator controlling-user="[[owner]]"></policy-indicator> <policy-indicator pref="[[pref]]"></policy-indicator>
lgtm i guess, then
The CQ bit was checked by michaelpg@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1367413002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1367413002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for
ui/webui/resources/cr_elements/v1_0/policy/cr_policy_indicator.js:
While running git apply --index -3 -p1;
error: patch failed:
ui/webui/resources/cr_elements/v1_0/policy/cr_policy_indicator.js:113
error: repository lacks the necessary blob to fall back on 3-way merge.
error: ui/webui/resources/cr_elements/v1_0/policy/cr_policy_indicator.js:
patch does not apply
Patch: ui/webui/resources/cr_elements/v1_0/policy/cr_policy_indicator.js
Index: ui/webui/resources/cr_elements/v1_0/policy/cr_policy_indicator.js
diff --git a/ui/webui/resources/cr_elements/v1_0/policy/cr_policy_indicator.js
b/ui/webui/resources/cr_elements/v1_0/policy/cr_policy_indicator.js
index
96e5a94da9c1d09aa7cb289904fc398c4cdad689..d9248c52fabf4d7b0720e72e3481d9ed3b3567f4
100644
--- a/ui/webui/resources/cr_elements/v1_0/policy/cr_policy_indicator.js
+++ b/ui/webui/resources/cr_elements/v1_0/policy/cr_policy_indicator.js
@@ -28,10 +28,19 @@ Polymer({
properties: {
/**
- * The preference object to show an indicator for.
- * @type {chrome.settingsPrivate.PrefObject|undefined}
+ * Optional preference object associated with the indicator. Initialized to
+ * null so that computed functions will get called if this is never set.
+ * @type {?chrome.settingsPrivate.PrefObject}
*/
- pref: {type: Object},
+ pref: {type: Object, value: null},
+
+ /**
+ * Optional email of the user controlling the setting when the setting does
+ * not correspond to a pref (Chrome OS only). Only used when pref is null.
+ * Initialized to '' so that computed functions will get called if this is
+ * never set.
+ */
+ controllingUser: {type: String, value: ''},
/**
* Which indicator type to show (or NONE).
@@ -113,11 +122,13 @@ Polymer({
/**
* @param {CrPolicyIndicator.Type} type The type of indicator.
* @param {?chrome.settingsPrivate.PrefObject} pref
+ * @param {string} controllingUser The user controlling the setting, if
|pref|
+ * is null.
* @return {string} The tooltip text for |type|.
* @private
*/
- getTooltipText_: function(type, pref) {
- var name = pref.policySourceName || '';
+ getTooltipText_: function(type, pref, controllingUser) {
+ var name = pref ? pref.policySourceName : controllingUser;
switch (type) {
case CrPolicyIndicator.Type.PRIMARY_USER:
@@ -130,7 +141,7 @@ Polymer({
case CrPolicyIndicator.Type.EXTENSION:
return this.i18n_('controlledSettingExtension', name);
case CrPolicyIndicator.Type.RECOMMENDED:
- if (pref.value == pref.recommendedValue)
+ if (pref && pref.value == pref.recommendedValue)
return this.i18n_('controlledSettingRecommendedMatches');
return this.i18n_('controlledSettingRecommendedDiffers');
}
The CQ bit was unchecked by commit-bot@chromium.org
The CQ bit was checked by michaelpg@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1367413002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1367413002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for
ui/webui/resources/cr_elements/v1_0/policy/cr_policy_indicator.js:
While running git apply --index -3 -p1;
error: patch failed:
ui/webui/resources/cr_elements/v1_0/policy/cr_policy_indicator.js:113
error: repository lacks the necessary blob to fall back on 3-way merge.
error: ui/webui/resources/cr_elements/v1_0/policy/cr_policy_indicator.js:
patch does not apply
Patch: ui/webui/resources/cr_elements/v1_0/policy/cr_policy_indicator.js
Index: ui/webui/resources/cr_elements/v1_0/policy/cr_policy_indicator.js
diff --git a/ui/webui/resources/cr_elements/v1_0/policy/cr_policy_indicator.js
b/ui/webui/resources/cr_elements/v1_0/policy/cr_policy_indicator.js
index
96e5a94da9c1d09aa7cb289904fc398c4cdad689..d9248c52fabf4d7b0720e72e3481d9ed3b3567f4
100644
--- a/ui/webui/resources/cr_elements/v1_0/policy/cr_policy_indicator.js
+++ b/ui/webui/resources/cr_elements/v1_0/policy/cr_policy_indicator.js
@@ -28,10 +28,19 @@ Polymer({
properties: {
/**
- * The preference object to show an indicator for.
- * @type {chrome.settingsPrivate.PrefObject|undefined}
+ * Optional preference object associated with the indicator. Initialized to
+ * null so that computed functions will get called if this is never set.
+ * @type {?chrome.settingsPrivate.PrefObject}
*/
- pref: {type: Object},
+ pref: {type: Object, value: null},
+
+ /**
+ * Optional email of the user controlling the setting when the setting does
+ * not correspond to a pref (Chrome OS only). Only used when pref is null.
+ * Initialized to '' so that computed functions will get called if this is
+ * never set.
+ */
+ controllingUser: {type: String, value: ''},
/**
* Which indicator type to show (or NONE).
@@ -113,11 +122,13 @@ Polymer({
/**
* @param {CrPolicyIndicator.Type} type The type of indicator.
* @param {?chrome.settingsPrivate.PrefObject} pref
+ * @param {string} controllingUser The user controlling the setting, if
|pref|
+ * is null.
* @return {string} The tooltip text for |type|.
* @private
*/
- getTooltipText_: function(type, pref) {
- var name = pref.policySourceName || '';
+ getTooltipText_: function(type, pref, controllingUser) {
+ var name = pref ? pref.policySourceName : controllingUser;
switch (type) {
case CrPolicyIndicator.Type.PRIMARY_USER:
@@ -130,7 +141,7 @@ Polymer({
case CrPolicyIndicator.Type.EXTENSION:
return this.i18n_('controlledSettingExtension', name);
case CrPolicyIndicator.Type.RECOMMENDED:
- if (pref.value == pref.recommendedValue)
+ if (pref && pref.value == pref.recommendedValue)
return this.i18n_('controlledSettingRecommendedMatches');
return this.i18n_('controlledSettingRecommendedDiffers');
}
The CQ bit was unchecked by commit-bot@chromium.org
The CQ bit was checked by michaelpg@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1367413002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1367413002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for
ui/webui/resources/cr_elements/v1_0/policy/cr_policy_indicator.js:
While running git apply --index -3 -p1;
error: patch failed:
ui/webui/resources/cr_elements/v1_0/policy/cr_policy_indicator.js:113
error: repository lacks the necessary blob to fall back on 3-way merge.
error: ui/webui/resources/cr_elements/v1_0/policy/cr_policy_indicator.js:
patch does not apply
Patch: ui/webui/resources/cr_elements/v1_0/policy/cr_policy_indicator.js
Index: ui/webui/resources/cr_elements/v1_0/policy/cr_policy_indicator.js
diff --git a/ui/webui/resources/cr_elements/v1_0/policy/cr_policy_indicator.js
b/ui/webui/resources/cr_elements/v1_0/policy/cr_policy_indicator.js
index
96e5a94da9c1d09aa7cb289904fc398c4cdad689..d9248c52fabf4d7b0720e72e3481d9ed3b3567f4
100644
--- a/ui/webui/resources/cr_elements/v1_0/policy/cr_policy_indicator.js
+++ b/ui/webui/resources/cr_elements/v1_0/policy/cr_policy_indicator.js
@@ -28,10 +28,19 @@ Polymer({
properties: {
/**
- * The preference object to show an indicator for.
- * @type {chrome.settingsPrivate.PrefObject|undefined}
+ * Optional preference object associated with the indicator. Initialized to
+ * null so that computed functions will get called if this is never set.
+ * @type {?chrome.settingsPrivate.PrefObject}
*/
- pref: {type: Object},
+ pref: {type: Object, value: null},
+
+ /**
+ * Optional email of the user controlling the setting when the setting does
+ * not correspond to a pref (Chrome OS only). Only used when pref is null.
+ * Initialized to '' so that computed functions will get called if this is
+ * never set.
+ */
+ controllingUser: {type: String, value: ''},
/**
* Which indicator type to show (or NONE).
@@ -113,11 +122,13 @@ Polymer({
/**
* @param {CrPolicyIndicator.Type} type The type of indicator.
* @param {?chrome.settingsPrivate.PrefObject} pref
+ * @param {string} controllingUser The user controlling the setting, if
|pref|
+ * is null.
* @return {string} The tooltip text for |type|.
* @private
*/
- getTooltipText_: function(type, pref) {
- var name = pref.policySourceName || '';
+ getTooltipText_: function(type, pref, controllingUser) {
+ var name = pref ? pref.policySourceName : controllingUser;
switch (type) {
case CrPolicyIndicator.Type.PRIMARY_USER:
@@ -130,7 +141,7 @@ Polymer({
case CrPolicyIndicator.Type.EXTENSION:
return this.i18n_('controlledSettingExtension', name);
case CrPolicyIndicator.Type.RECOMMENDED:
- if (pref.value == pref.recommendedValue)
+ if (pref && pref.value == pref.recommendedValue)
return this.i18n_('controlledSettingRecommendedMatches');
return this.i18n_('controlledSettingRecommendedDiffers');
}
The CQ bit was unchecked by commit-bot@chromium.org
The CQ bit was checked by michaelpg@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1367413002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1367413002/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/6ea79ace485b9f94cfcefabf7a820f45b56e76a2 Cr-Commit-Position: refs/heads/master@{#351215} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
