Move common cr-policy-indicator behavior into CrPolicyIndicatorBehavior

Move common cr-policy-indicator behavior into CrPolicyIndicatorBehavior

This will make it simpler to create new indicators or add indicators to
existing controls.

BUG=317936
R=stevenjb@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2696903005
Cr-Commit-Position: refs/heads/master@{#454069}
Committed: https://chromium.googlesource.com/chromium/src/+/e3d7d44eb069441b0c2c10a51128193d940dcc53

[michaelpg, 2017-02-15 00:02:55]

Description was changed from

==========
Move common cr-policy-indicator behavior into CrPolicyIndicatorBehavior

BUG=
==========

to

==========
Move common cr-policy-indicator behavior into CrPolicyIndicatorBehavior

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[michaelpg, 2017-02-15 00:03:35]

Description was changed from

==========
Move common cr-policy-indicator behavior into CrPolicyIndicatorBehavior

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Move common cr-policy-indicator behavior into CrPolicyIndicatorBehavior

This will make it simpler to create new indicators or add indicators to
existing controls.

BUG=317936
R=stevenjb@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[michaelpg, 2017-02-15 00:03:36]

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

[michaelpg, 2017-02-15 00:16:53]

PTAL.

[stevenjb, 2017-02-15 00:39:08]

Thanks for the cleanup! If you could do me a favor and rebase this on
https://codereview.chromium.org/2624003003/ (or wait for it to land) and fix
and/or add tests, that would be great!

(Note, I have a second CL to add network indicator tests, which I will rebase on
this once it lands, so no need to add those).

https://codereview.chromium.org/2696903005/diff/40001/ui/webui/resources/cr_e...
File ui/webui/resources/cr_elements/policy/cr_policy_network_indicator.js
(right):

https://codereview.chromium.org/2696903005/diff/40001/ui/webui/resources/cr_e...
ui/webui/resources/cr_elements/policy/cr_policy_network_indicator.js:80: if
(this.indicatorType == CrPolicyIndicatorType.RECOMMENDED &&
We should either use |indicatorType| throughout or not declare it as a parameter
above.

[michaelpg, 2017-02-21 23:59:01]

stevenjb: I found a (IMO) better way to work in the behavior that removes some
duplication by between Settings controls and their indicators. Also added tests.

The additional lines are mostly moved code and new tests, hope it's not too
annoying to re-review.

https://codereview.chromium.org/2696903005/diff/40001/ui/webui/resources/cr_e...
File ui/webui/resources/cr_elements/policy/cr_policy_network_indicator.js
(right):

https://codereview.chromium.org/2696903005/diff/40001/ui/webui/resources/cr_e...
ui/webui/resources/cr_elements/policy/cr_policy_network_indicator.js:80: if
(this.indicatorType == CrPolicyIndicatorType.RECOMMENDED &&
On 2017/02/15 00:39:08, stevenjb wrote:
> We should either use |indicatorType| throughout or not declare it as a
parameter
> above.

Done.

[stevenjb (google-dont-use), 2017-02-22 01:01:09]

stevenjb@google.com changed reviewers:
+ stevenjb@google.com

[stevenjb (google-dont-use), 2017-02-22 01:01:11]

Thanks for the cleanup. A few comments/questions.

https://codereview.chromium.org/2696903005/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/controls/controlled_button.html (right):

https://codereview.chromium.org/2696903005/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/controls/controlled_button.html:45: <template
is="dom-if" if="[[hasPolicy(pref.*)]]" restamp>
Should this be [[indicatorVisible]]?

https://codereview.chromium.org/2696903005/diff/60001/ui/webui/resources/cr_e...
File ui/webui/resources/cr_elements/policy/cr_policy_pref_indicator.js (right):

https://codereview.chromium.org/2696903005/diff/60001/ui/webui/resources/cr_e...
ui/webui/resources/cr_elements/policy/cr_policy_pref_indicator.js:25: *
@override
Huh. Didn't realize we can do this. It's a little odd... but fine I guess...

https://codereview.chromium.org/2696903005/diff/60001/ui/webui/resources/cr_e...
ui/webui/resources/cr_elements/policy/cr_policy_pref_indicator.js:30: computed:
'getIndicatorType_(pref.controlledBy, pref.enforcement)',
maybe getIndicatorTypeForPref_?

[michaelpg, 2017-02-22 03:20:53]

Patchset #5 (id:80001) has been deleted

[michaelpg, 2017-02-22 20:59:10]

Patchset #6 (id:120001) has been deleted

[michaelpg, 2017-02-22 21:00:01]

https://codereview.chromium.org/2696903005/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/controls/controlled_button.html (right):

https://codereview.chromium.org/2696903005/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/controls/controlled_button.html:45: <template
is="dom-if" if="[[hasPolicy(pref.*)]]" restamp>
On 2017/02/22 01:01:10, stevenjb (google-dont-use) wrote:
> Should this be [[indicatorVisible]]?

It amounts to the same thing, just from the other direction: an indicator is
visible iff its pref is policy-controlled.

hasPolicy() lets us wrap the indicator in a dom-if. It's an extra function, but
it lets us remove CrPolicyIndicatorBehavior from the control (since the
indicator already has that behavior anyway and makes all the same calculations).

https://codereview.chromium.org/2696903005/diff/60001/ui/webui/resources/cr_e...
File ui/webui/resources/cr_elements/policy/cr_policy_pref_indicator.js (right):

https://codereview.chromium.org/2696903005/diff/60001/ui/webui/resources/cr_e...
ui/webui/resources/cr_elements/policy/cr_policy_pref_indicator.js:25: *
@override
On 2017/02/22 01:01:10, stevenjb (google-dont-use) wrote:
> Huh. Didn't realize we can do this. It's a little odd... but fine I guess...

Sometimes it would be nice to have real inheritance... :-(

https://codereview.chromium.org/2696903005/diff/60001/ui/webui/resources/cr_e...
ui/webui/resources/cr_elements/policy/cr_policy_pref_indicator.js:30: computed:
'getIndicatorType_(pref.controlledBy, pref.enforcement)',
On 2017/02/22 01:01:10, stevenjb (google-dont-use) wrote:
> maybe getIndicatorTypeForPref_?

Done.

[stevenjb, 2017-02-23 18:21:39]

https://codereview.chromium.org/2696903005/diff/100001/ui/webui/resources/cr_...
File ui/webui/resources/cr_elements/policy/cr_policy_pref_behavior.js (right):

https://codereview.chromium.org/2696903005/diff/100001/ui/webui/resources/cr_...
ui/webui/resources/cr_elements/policy/cr_policy_pref_behavior.js:24: hasPolicy:
function() {
This is why I was confused with the controlled-button change. I didn't
immediately assume that hasPolicy() > isPrefPolicyControlled()  (I assumed the
opposite). Can we come up with a better name? Depending on how/where else this
is used, maybe hasPrefPolicyIndicator()?

[stevenjb, 2017-02-23 19:31:44]

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

[stevenjb, 2017-02-23 19:31:45]

+dschuyler FYI

[michaelpg, 2017-02-28 01:16:29]

https://codereview.chromium.org/2696903005/diff/100001/ui/webui/resources/cr_...
File ui/webui/resources/cr_elements/policy/cr_policy_pref_behavior.js (right):

https://codereview.chromium.org/2696903005/diff/100001/ui/webui/resources/cr_...
ui/webui/resources/cr_elements/policy/cr_policy_pref_behavior.js:24: hasPolicy:
function() {
On 2017/02/23 18:21:39, stevenjb wrote:
> This is why I was confused with the controlled-button change. I didn't
> immediately assume that hasPolicy() > isPrefPolicyControlled()  (I assumed the
> opposite). Can we come up with a better name? Depending on how/where else this
> is used, maybe hasPrefPolicyIndicator()?

Done (only used by controlled-button to show the indicator)

[stevenjb, 2017-02-28 01:26:08]

lgtm

[dschuyler, 2017-02-28 01:31:38]

On 2017/02/28 01:26:08, stevenjb wrote:
> lgtm

CL 2696903005 and CL 2708013003 appear to be in the same space (controlled by
indicators). Does it make sense to land one before the other particularly?

[stevenjb, 2017-02-28 03:04:02]

On 2017/02/28 01:31:38, dschuyler wrote:
> On 2017/02/28 01:26:08, stevenjb wrote:
> > lgtm
> 
> CL 2696903005 and CL 2708013003 appear to be in the same space (controlled by
> indicators). Does it make sense to land one before the other particularly?

I think this CL should land first, thanks.

[michaelpg, 2017-02-28 05:42:55]

On 2017/02/28 03:04:02, stevenjb wrote:
> On 2017/02/28 01:31:38, dschuyler wrote:
> > On 2017/02/28 01:26:08, stevenjb wrote:
> > > lgtm
> > 
> > CL 2696903005 and CL 2708013003 appear to be in the same space (controlled
by
> > indicators). Does it make sense to land one before the other particularly?
> 
> I think this CL should land first, thanks.

I agree (but I'm biased). dschuyler, if that SGTY, would you like to review this
CL or let stevenjb's review stand?

[dschuyler, 2017-02-28 19:42:40]

https://codereview.chromium.org/2696903005/diff/140001/chrome/browser/resourc...
File chrome/browser/resources/settings/controls/controlled_button.html (right):

https://codereview.chromium.org/2696903005/diff/140001/chrome/browser/resourc...
chrome/browser/resources/settings/controls/controlled_button.html:45: <template
is="dom-if" if="[[hasPrefPolicyIndicator(pref.*)]]" restamp>
Let's make this more general by dropping the "Policy" part, e.g.
hasPrefIndicator or hasControlledIndicator.

https://codereview.chromium.org/2696903005/diff/140001/ui/webui/resources/cr_...
File ui/webui/resources/cr_elements/policy/cr_policy_pref_behavior.js (right):

https://codereview.chromium.org/2696903005/diff/140001/ui/webui/resources/cr_...
ui/webui/resources/cr_elements/policy/cr_policy_pref_behavior.js:14:
isPrefPolicyControlled: function() {
How about changing this to isPrefEnforced? That seems more specific to what it's
doing internally (and it drops the "Policy" part).

https://codereview.chromium.org/2696903005/diff/140001/ui/webui/resources/cr_...
ui/webui/resources/cr_elements/policy/cr_policy_pref_behavior.js:24:
hasPrefPolicyIndicator: function() {
How about hasPrefIndicator since RECOMMENDED isn't a policy (afaik).

[stevenjb, 2017-02-28 20:04:37]

https://codereview.chromium.org/2696903005/diff/140001/chrome/browser/resourc...
File chrome/browser/resources/settings/controls/controlled_button.html (right):

https://codereview.chromium.org/2696903005/diff/140001/chrome/browser/resourc...
chrome/browser/resources/settings/controls/controlled_button.html:45: <template
is="dom-if" if="[[hasPrefPolicyIndicator(pref.*)]]" restamp>
On 2017/02/28 19:42:40, dschuyler wrote:
> Let's make this more general by dropping the "Policy" part, e.g.
> hasPrefIndicator or hasControlledIndicator. 

May I suggest that if we do any significant renaming we do it in a separate CL
once the current in-progress CLs have landed? 

Currently the indicators are in a "policy" directory and are all prefixed with
cr-policy. Using "policy" to also describe indicators that may come form an
extension may not be ideal, but if we decide to drop "policy" we should do so
consistently and in an isolated CL.

For now, this name is at least consistent with the existing
"isPrefPolicyControlled" in the same behavior.

https://codereview.chromium.org/2696903005/diff/140001/ui/webui/resources/cr_...
File ui/webui/resources/cr_elements/policy/cr_policy_pref_behavior.js (right):

https://codereview.chromium.org/2696903005/diff/140001/ui/webui/resources/cr_...
ui/webui/resources/cr_elements/policy/cr_policy_pref_behavior.js:14:
isPrefPolicyControlled: function() {
On 2017/02/28 19:42:40, dschuyler wrote:
> How about changing this to isPrefEnforced? That seems more specific to what
it's
> doing internally (and it drops the "Policy" part).

I already suggested that we not include the extra churn of renaming this in this
CL (see previous comment).

https://codereview.chromium.org/2696903005/diff/140001/ui/webui/resources/cr_...
ui/webui/resources/cr_elements/policy/cr_policy_pref_behavior.js:24:
hasPrefPolicyIndicator: function() {
On 2017/02/28 19:42:40, dschuyler wrote:
> How about hasPrefIndicator since RECOMMENDED isn't a policy (afaik).

See previous comment.

[stevenjb, 2017-02-28 20:04:37]

https://codereview.chromium.org/2696903005/diff/140001/chrome/browser/resourc...
File chrome/browser/resources/settings/controls/controlled_button.html (right):

https://codereview.chromium.org/2696903005/diff/140001/chrome/browser/resourc...
chrome/browser/resources/settings/controls/controlled_button.html:45: <template
is="dom-if" if="[[hasPrefPolicyIndicator(pref.*)]]" restamp>
On 2017/02/28 19:42:40, dschuyler wrote:
> Let's make this more general by dropping the "Policy" part, e.g.
> hasPrefIndicator or hasControlledIndicator. 

May I suggest that if we do any significant renaming we do it in a separate CL
once the current in-progress CLs have landed? 

Currently the indicators are in a "policy" directory and are all prefixed with
cr-policy. Using "policy" to also describe indicators that may come form an
extension may not be ideal, but if we decide to drop "policy" we should do so
consistently and in an isolated CL.

For now, this name is at least consistent with the existing
"isPrefPolicyControlled" in the same behavior.

https://codereview.chromium.org/2696903005/diff/140001/ui/webui/resources/cr_...
File ui/webui/resources/cr_elements/policy/cr_policy_pref_behavior.js (right):

https://codereview.chromium.org/2696903005/diff/140001/ui/webui/resources/cr_...
ui/webui/resources/cr_elements/policy/cr_policy_pref_behavior.js:14:
isPrefPolicyControlled: function() {
On 2017/02/28 19:42:40, dschuyler wrote:
> How about changing this to isPrefEnforced? That seems more specific to what
it's
> doing internally (and it drops the "Policy" part).

I already suggested that we not include the extra churn of renaming this in this
CL (see previous comment).

https://codereview.chromium.org/2696903005/diff/140001/ui/webui/resources/cr_...
ui/webui/resources/cr_elements/policy/cr_policy_pref_behavior.js:24:
hasPrefPolicyIndicator: function() {
On 2017/02/28 19:42:40, dschuyler wrote:
> How about hasPrefIndicator since RECOMMENDED isn't a policy (afaik).

See previous comment.

[Dan Beam, 2017-02-28 20:07:06]

dbeam@chromium.org changed reviewers:
+ dbeam@chromium.org

[Dan Beam, 2017-02-28 20:07:08]

https://codereview.chromium.org/2696903005/diff/140001/ui/webui/resources/cr_...
File ui/webui/resources/cr_elements/policy/cr_policy_pref_behavior.js (right):

https://codereview.chromium.org/2696903005/diff/140001/ui/webui/resources/cr_...
ui/webui/resources/cr_elements/policy/cr_policy_pref_behavior.js:14:
isPrefPolicyControlled: function() {
On 2017/02/28 20:04:37, stevenjb wrote:
> On 2017/02/28 19:42:40, dschuyler wrote:
> > How about changing this to isPrefEnforced? That seems more specific to what
> it's
> > doing internally (and it drops the "Policy" part).
> 
> I already suggested that we not include the extra churn of renaming this in
this
> CL (see previous comment).

and i still disagree because the code is now wrong-ish ;)

[stevenjb, 2017-02-28 20:08:49]

On 2017/02/28 20:07:08, Dan Beam wrote:
>
https://codereview.chromium.org/2696903005/diff/140001/ui/webui/resources/cr_...
> File ui/webui/resources/cr_elements/policy/cr_policy_pref_behavior.js (right):
> 
>
https://codereview.chromium.org/2696903005/diff/140001/ui/webui/resources/cr_...
> ui/webui/resources/cr_elements/policy/cr_policy_pref_behavior.js:14:
> isPrefPolicyControlled: function() {
> On 2017/02/28 20:04:37, stevenjb wrote:
> > On 2017/02/28 19:42:40, dschuyler wrote:
> > > How about changing this to isPrefEnforced? That seems more specific to
what
> > it's
> > > doing internally (and it drops the "Policy" part).
> > 
> > I already suggested that we not include the extra churn of renaming this in
> this
> > CL (see previous comment).
> 
> and i still disagree because the code is now wrong-ish ;)

Please clarify, there have been multiple proposals. What is wrong-ish and what
should change to what? Again, my suggestion is not that we shouldn't consider
renaming, but we should do so consistently and separately.

[Dan Beam, 2017-02-28 20:08:56]

ultimately there's a bajillion "policy" in the paths, file names, method names
etc. so renaming something in this CL (which is already fairly big) is not high
prio for me.

michaelpg@: does this have any impact on time until interactive?

[dschuyler, 2017-02-28 20:09:30]

https://codereview.chromium.org/2696903005/diff/140001/ui/webui/resources/cr_...
File ui/webui/resources/cr_elements/policy/README.md (right):

https://codereview.chromium.org/2696903005/diff/140001/ui/webui/resources/cr_...
ui/webui/resources/cr_elements/policy/README.md:13: tooltip, icon, and
visibility of the indicator.
Just a suggestion, totally optional:
The above is a good overview of the features of this code, which is cool. To add
to it, I'd find it super useful to have a list of instructions and/or and
example to search for. e.g.

"To add a new indicator:
1. 
2."

or

"For an example of a correctly implemented indicator, search the code for
______."

[dschuyler, 2017-02-28 20:15:26]

On 2017/02/28 20:08:49, stevenjb wrote:
> On 2017/02/28 20:07:08, Dan Beam wrote:
> >
>
https://codereview.chromium.org/2696903005/diff/140001/ui/webui/resources/cr_...
> > File ui/webui/resources/cr_elements/policy/cr_policy_pref_behavior.js
(right):
> > 
> >
>
https://codereview.chromium.org/2696903005/diff/140001/ui/webui/resources/cr_...
> > ui/webui/resources/cr_elements/policy/cr_policy_pref_behavior.js:14:
> > isPrefPolicyControlled: function() {
> > On 2017/02/28 20:04:37, stevenjb wrote:
> > > On 2017/02/28 19:42:40, dschuyler wrote:
> > > > How about changing this to isPrefEnforced? That seems more specific to
> what
> > > it's
> > > > doing internally (and it drops the "Policy" part).
> > > 
> > > I already suggested that we not include the extra churn of renaming this
in
> > this
> > > CL (see previous comment).
> > 
> > and i still disagree because the code is now wrong-ish ;)
> 
> Please clarify, there have been multiple proposals. What is wrong-ish and what
> should change to what? Again, my suggestion is not that we shouldn't consider
> renaming, but we should do so consistently and separately.

Okay, not a big deal. I had seen that getPolicyIndicatorTooltip was renamed to
drop the Policy part of the name in this CL.
I thought doing that consistently would be appropriate.

It can wait for another CL, np. Please add a TODO() and/or a tech-dept crbug
about the naming if we don't want to further change names in this CL.

[Dan Beam, 2017-02-28 20:16:48]

On 2017/02/28 20:08:49, stevenjb wrote:
> On 2017/02/28 20:07:08, Dan Beam wrote:
> >
>
https://codereview.chromium.org/2696903005/diff/140001/ui/webui/resources/cr_...
> > File ui/webui/resources/cr_elements/policy/cr_policy_pref_behavior.js
(right):
> > 
> >
>
https://codereview.chromium.org/2696903005/diff/140001/ui/webui/resources/cr_...
> > ui/webui/resources/cr_elements/policy/cr_policy_pref_behavior.js:14:
> > isPrefPolicyControlled: function() {
> > On 2017/02/28 20:04:37, stevenjb wrote:
> > > On 2017/02/28 19:42:40, dschuyler wrote:
> > > > How about changing this to isPrefEnforced? That seems more specific to
> what
> > > it's
> > > > doing internally (and it drops the "Policy" part).
> > > 
> > > I already suggested that we not include the extra churn of renaming this
in
> > this
> > > CL (see previous comment).
> > 
> > and i still disagree because the code is now wrong-ish ;)
> 
> Please clarify, there have been multiple proposals. What is wrong-ish and what
> should change to what? Again, my suggestion is not that we shouldn't consider
> renaming, but we should do so consistently and separately.

there has been a lot of comments on this CL, and this was a direct response (so
it didn't show up in context in the file), but I just saw your earlier message

https://codereview.chromium.org/2696903005/diff/140001/chrome/browser/resourc...

"""
May I suggest that if we do any significant renaming we do it in a separate CL
once the current in-progress CLs have landed? 

Currently the indicators are in a "policy" directory and are all prefixed with
cr-policy. Using "policy" to also describe indicators that may come form an
extension may not be ideal, but if we decide to drop "policy" we should do so
consistently and in an isolated CL.

For now, this name is at least consistent with the existing
"isPrefPolicyControlled" in the same behavior.
"""

and agree with you.  by "wrong-ish" i only meant that this code was originally
added for the purpose of showing policy indicators but has changed to mean
either policy or extensions controlling a preference's effective value.  these
are different source of control, and are not really that related.  the notion of
extension control did exist previously, but neither of us actually figured out a
way to reproduce the situation in which an extension would trigger the
"extension controlled" codepath, so I removed the code.

fyi: it's being added back here by scottchen@

https://codereview.chromium.org/2720503007/

[chromium-reviews, 2017-02-28 20:46:31]

Yep. sgtm


On Tue, Feb 28, 2017 at 1:16 PM, <dbeam@chromium.org> wrote:

> On 2017/02/28 20:08:49, stevenjb wrote:
> > On 2017/02/28 20:07:08, Dan Beam wrote:
> > >
> >
> https://codereview.chromium.org/2696903005/diff/140001/ui/
> webui/resources/cr_elements/policy/cr_policy_pref_behavior.js
> > > File ui/webui/resources/cr_elements/policy/cr_policy_pref_behavior.js
> (right):
> > >
> > >
> >
> https://codereview.chromium.org/2696903005/diff/140001/ui/
> webui/resources/cr_elements/policy/cr_policy_pref_behavior.js#newcode14
> > > ui/webui/resources/cr_elements/policy/cr_policy_pref_behavior.js:14:
> > > isPrefPolicyControlled: function() {
> > > On 2017/02/28 20:04:37, stevenjb wrote:
> > > > On 2017/02/28 19:42:40, dschuyler wrote:
> > > > > How about changing this to isPrefEnforced? That seems more
> specific to
> > what
> > > > it's
> > > > > doing internally (and it drops the "Policy" part).
> > > >
> > > > I already suggested that we not include the extra churn of renaming
> this
> in
> > > this
> > > > CL (see previous comment).
> > >
> > > and i still disagree because the code is now wrong-ish ;)
> >
> > Please clarify, there have been multiple proposals. What is wrong-ish
> and what
> > should change to what? Again, my suggestion is not that we shouldn't
> consider
> > renaming, but we should do so consistently and separately.
>
> there has been a lot of comments on this CL, and this was a direct
> response (so
> it didn't show up in context in the file), but I just saw your earlier
> message
>
> https://codereview.chromium.org/2696903005/diff/140001/
> chrome/browser/resources/settings/controls/controlled_
> button.html#newcode45
>
> """
> May I suggest that if we do any significant renaming we do it in a
> separate CL
> once the current in-progress CLs have landed?
>
> Currently the indicators are in a "policy" directory and are all prefixed
> with
> cr-policy. Using "policy" to also describe indicators that may come form an
> extension may not be ideal, but if we decide to drop "policy" we should do
> so
> consistently and in an isolated CL.
>
> For now, this name is at least consistent with the existing
> "isPrefPolicyControlled" in the same behavior.
> """
>
> and agree with you. by "wrong-ish" i only meant that this code was
> originally
> added for the purpose of showing policy indicators but has changed to mean
> either policy or extensions controlling a preference's effective value.
> these
> are different source of control, and are not really that related. the
> notion of
> extension control did exist previously, but neither of us actually figured
> out a
> way to reproduce the situation in which an extension would trigger the
> "extension controlled" codepath, so I removed the code.
>
> fyi: it's being added back here by scottchen@
>
> https://codereview.chromium.org/2720503007/
>
> https://codereview.chromium.org/2696903005/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[michaelpg, 2017-02-28 21:55:49]

michaelpg@chromium.org changed reviewers:
- stevenjb@google.com

[michaelpg, 2017-02-28 21:55:50]

I think everything was addressed in the comments or message chain -- LMK if I
missed something or you have suggestions for the README

https://codereview.chromium.org/2696903005/diff/140001/chrome/browser/resourc...
File chrome/browser/resources/settings/controls/controlled_button.html (right):

https://codereview.chromium.org/2696903005/diff/140001/chrome/browser/resourc...
chrome/browser/resources/settings/controls/controlled_button.html:45: <template
is="dom-if" if="[[hasPrefPolicyIndicator(pref.*)]]" restamp>
On 2017/02/28 20:04:37, stevenjb wrote:
> On 2017/02/28 19:42:40, dschuyler wrote:
> > Let's make this more general by dropping the "Policy" part, e.g.
> > hasPrefIndicator or hasControlledIndicator. 
> 
> May I suggest that if we do any significant renaming we do it in a separate CL
> once the current in-progress CLs have landed? 
> 
> Currently the indicators are in a "policy" directory and are all prefixed with
> cr-policy. Using "policy" to also describe indicators that may come form an
> extension may not be ideal, but if we decide to drop "policy" we should do so
> consistently and in an isolated CL.
> 
> For now, this name is at least consistent with the existing
> "isPrefPolicyControlled" in the same behavior.

Acknowledged.

https://codereview.chromium.org/2696903005/diff/140001/ui/webui/resources/cr_...
File ui/webui/resources/cr_elements/policy/README.md (right):

https://codereview.chromium.org/2696903005/diff/140001/ui/webui/resources/cr_...
ui/webui/resources/cr_elements/policy/README.md:13: tooltip, icon, and
visibility of the indicator.
On 2017/02/28 20:09:29, dschuyler wrote:
> Just a suggestion, totally optional:
> The above is a good overview of the features of this code, which is cool. To
add
> to it, I'd find it super useful to have a list of instructions and/or and
> example to search for. e.g.
> 
> "To add a new indicator:
> 1. 
> 2."
> 
> or
> 
> "For an example of a correctly implemented indicator, search the code for
> ______."
> 

Were you looking for instructions on creating a new indicator in policy/, or for
displaying a policy/ indicator elsewhere in the UI?

I've expanded the README a bit for clarity, LMK where it can be improved.

https://codereview.chromium.org/2696903005/diff/140001/ui/webui/resources/cr_...
File ui/webui/resources/cr_elements/policy/cr_policy_pref_behavior.js (right):

https://codereview.chromium.org/2696903005/diff/140001/ui/webui/resources/cr_...
ui/webui/resources/cr_elements/policy/cr_policy_pref_behavior.js:14:
isPrefPolicyControlled: function() {
On 2017/02/28 20:07:07, Dan Beam wrote:
> On 2017/02/28 20:04:37, stevenjb wrote:
> > On 2017/02/28 19:42:40, dschuyler wrote:
> > > How about changing this to isPrefEnforced? That seems more specific to
what
> > it's
> > > doing internally (and it drops the "Policy" part).
> > 
> > I already suggested that we not include the extra churn of renaming this in
> this
> > CL (see previous comment).
> 
> and i still disagree because the code is now wrong-ish ;)

I added a TODO about the policy name. *this* function is correct-ish, LMK if
there is something actionable to do.

https://codereview.chromium.org/2696903005/diff/140001/ui/webui/resources/cr_...
ui/webui/resources/cr_elements/policy/cr_policy_pref_behavior.js:24:
hasPrefPolicyIndicator: function() {
On 2017/02/28 19:42:40, dschuyler wrote:
> How about hasPrefIndicator since RECOMMENDED isn't a policy (afaik).

RECOMMENDED is a policy, just a non-enforced one. It's set by the administrator.

At least, that's my understanding. The "policy" is "we recommend using X".

[stevenjb, 2017-02-28 21:59:01]

https://codereview.chromium.org/2696903005/diff/140001/ui/webui/resources/cr_...
File ui/webui/resources/cr_elements/policy/cr_policy_pref_behavior.js (right):

https://codereview.chromium.org/2696903005/diff/140001/ui/webui/resources/cr_...
ui/webui/resources/cr_elements/policy/cr_policy_pref_behavior.js:24:
hasPrefPolicyIndicator: function() {
On 2017/02/28 21:55:50, michaelpg wrote:
> On 2017/02/28 19:42:40, dschuyler wrote:
> > How about hasPrefIndicator since RECOMMENDED isn't a policy (afaik).
> 
> RECOMMENDED is a policy, just a non-enforced one. It's set by the
administrator.
> 
> At least, that's my understanding. The "policy" is "we recommend using X".

That's also a good point, you are correct about what 'recommended' means.

[dschuyler, 2017-02-28 22:16:33]

Thanks for the Readme improvements!

Tip: I think Dan had a question about performance -- was that addressed?

lgtm

https://codereview.chromium.org/2696903005/diff/160001/ui/webui/resources/cr_...
File ui/webui/resources/cr_elements/policy/README.md (right):

https://codereview.chromium.org/2696903005/diff/160001/ui/webui/resources/cr_...
ui/webui/resources/cr_elements/policy/README.md:3: Setting that can't be
controlled by the current user often show an icon and a
maybe
s/Setting/Settings/

or 
s/Setting/A setting/

https://codereview.chromium.org/2696903005/diff/160001/ui/webui/resources/cr_...
ui/webui/resources/cr_elements/policy/README.md:18: `CrPolicyStrings` is
defined.
maybe s/assuming/assuming an entry in/
(since CrPolicyStrings will be defined unconditionally iiuc).

[michaelpg, 2017-03-01 00:33:47]

On 2017/02/28 20:08:56, Dan Beam wrote:
> ultimately there's a bajillion "policy" in the paths, file names, method names
> etc. so renaming something in this CL (which is already fairly big) is not
high
> prio for me.
> 
> michaelpg@: does this have any impact on time until interactive?

Performance impact is doubtful. Only "significant" changes are: removing a
behavior from controlled_button.js, inlining a tiny amount of CSS, and
moving some data binding annotations to computed properties.

I've tried running lighthouse but haven't been able to get flags working yet.

[michaelpg, 2017-03-01 00:34:01]

https://codereview.chromium.org/2696903005/diff/160001/ui/webui/resources/cr_...
File ui/webui/resources/cr_elements/policy/README.md (right):

https://codereview.chromium.org/2696903005/diff/160001/ui/webui/resources/cr_...
ui/webui/resources/cr_elements/policy/README.md:3: Setting that can't be
controlled by the current user often show an icon and a
On 2017/02/28 22:16:33, dschuyler wrote:
> maybe
> s/Setting/Settings/
> 
> or 
> s/Setting/A setting/

Done.

https://codereview.chromium.org/2696903005/diff/160001/ui/webui/resources/cr_...
ui/webui/resources/cr_elements/policy/README.md:18: `CrPolicyStrings` is
defined.
On 2017/02/28 22:16:33, dschuyler wrote:
> maybe s/assuming/assuming an entry in/
> (since CrPolicyStrings will be defined unconditionally iiuc).

No, the actual object needs to be defined -- we do this in settings_ui.js and
some tests.

If it's not defined we just don't display tooltips. I think OOBE is like that.

[Dan Beam, 2017-03-01 08:35:17]

dbeam@chromium.org changed reviewers:
- dbeam@chromium.org

[michaelpg, 2017-03-01 21:26:36]

On 2017/02/28 20:08:56, Dan Beam wrote:
> ultimately there's a bajillion "policy" in the paths, file names, method names
> etc. so renaming something in this CL (which is already fairly big) is not
high
> prio for me.
> 
> michaelpg@: does this have any impact on time until interactive?

We're good performance-wise. It has no impact on first meaningful paint, and a
decrease on time until interactive (meaning, it's faster) that is NOT
statistically significant.

[michaelpg, 2017-03-01 21:27:08]

The CQ bit was checked by michaelpg@chromium.org

[michaelpg, 2017-03-01 21:27:09]

The patchset sent to the CQ was uploaded after l-g-t-m from
stevenjb@chromium.org, dschuyler@chromium.org
Link to the patchset: https://codereview.chromium.org/2696903005/#ps200001
(title: "rebase")

[commit-bot: I haz the power, 2017-03-01 21:27:37]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Dan Beam, 2017-03-01 21:55:33]

On 2017/03/01 21:26:36, michaelpg wrote:
> On 2017/02/28 20:08:56, Dan Beam wrote:
> > ultimately there's a bajillion "policy" in the paths, file names, method
names
> > etc. so renaming something in this CL (which is already fairly big) is not
> high
> > prio for me.
> > 
> > michaelpg@: does this have any impact on time until interactive?
> 
> We're good performance-wise. It has no impact on first meaningful paint, and a
> decrease on time until interactive (meaning, it's faster) that is NOT
> statistically significant.

makes sense, thank you for the diligence!

[commit-bot: I haz the power, 2017-03-01 22:24:20]

CQ is committing da patch.

Bot data: {"patchset_id": 200001, "attempt_start_ts": 1488403628813800,
"parent_rev": "dc779d9f0fab468f70ef7fb2cf5029e2373473dc", "commit_rev":
"e3d7d44eb069441b0c2c10a51128193d940dcc53"}

[commit-bot: I haz the power, 2017-03-01 22:25:02]

Description was changed from

==========
Move common cr-policy-indicator behavior into CrPolicyIndicatorBehavior

This will make it simpler to create new indicators or add indicators to
existing controls.

BUG=317936
R=stevenjb@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Move common cr-policy-indicator behavior into CrPolicyIndicatorBehavior

This will make it simpler to create new indicators or add indicators to
existing controls.

BUG=317936
R=stevenjb@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2696903005
Cr-Commit-Position: refs/heads/master@{#454069}
Committed:
https://chromium.googlesource.com/chromium/src/+/e3d7d44eb069441b0c2c10a51128...
==========

[commit-bot: I haz the power, 2017-03-01 22:25:05]

Committed patchset #9 (id:200001) as
https://chromium.googlesource.com/chromium/src/+/e3d7d44eb069441b0c2c10a51128...