Extract policy indicator behavior

Extract policy indicator behavior

This CL:
* Moves settings/PolicyControllable -> cr_elements/CrPolicyPrefBehavior
* Moves other pref specific behavior to CrPolicyPrefBehavior
* Extacts shared behaviors from CrPolicyIndicator into a general purpose
  behavior, CrPolicyIndicatorBehavior
* Renames CrPolicyIndicator to CrPolicyPrefIndicator

This CL should also make it easier to include just the behaviors for
Closure compilation. Otherwise it contains no functional changes,
merely does some re-factoring to prep for the addition of a network indicator
(See https://codereview.chromium.org/1369403006/ for the complete CL).

BUG=521791
Committed: https://crrev.com/5f7fd124ff6dc5aab4b5fe9f9fa3e020bb630e85
Cr-Commit-Position: refs/heads/master@{#354417}

[stevenjb, 2015-10-06 00:55:56]

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

[stevenjb, 2015-10-06 00:57:48]

Extracting the pref indicator re-factoring for
https://codereview.chromium.org/1369403006/

[michaelpg, 2015-10-07 18:18:35]

beautiful. lgtm

\o/ behaviors

https://codereview.chromium.org/1391433003/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/languages_page/compiled_resources.gyp
(right):

https://codereview.chromium.org/1391433003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/languages_page/compiled_resources.gyp:26:
'../../../../../ui/webui/resources/cr_elements/v1_0/policy/cr_policy_indicator_behavior.js',
Wish we could just include the .gyp target like we do for other dependses.

https://codereview.chromium.org/1391433003/diff/1/ui/webui/resources/cr_eleme...
File ui/webui/resources/cr_elements/v1_0/policy/cr_policy_indicator_behavior.js
(right):

https://codereview.chromium.org/1391433003/diff/1/ui/webui/resources/cr_eleme...
ui/webui/resources/cr_elements/v1_0/policy/cr_policy_indicator_behavior.js:83:
// This case is not handled here since it requires knowlede of the
knowledge

https://codereview.chromium.org/1391433003/diff/1/ui/webui/resources/cr_eleme...
File ui/webui/resources/cr_elements/v1_0/policy/cr_policy_pref_indicator.js
(right):

https://codereview.chromium.org/1391433003/diff/1/ui/webui/resources/cr_eleme...
ui/webui/resources/cr_elements/v1_0/policy/cr_policy_pref_indicator.js:30:
controllingUser: {type: String, value: ''},
TODO (for me or you): extract this to a separate indicator when we add more
indicators using user instead of pref (ie, get rid of my hax)

[stevenjb, 2015-10-07 18:33:38]

dbeam@ - do you want to take a look at this one or are you OK with Michael's
review?

https://codereview.chromium.org/1391433003/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/languages_page/compiled_resources.gyp
(right):

https://codereview.chromium.org/1391433003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/languages_page/compiled_resources.gyp:26:
'../../../../../ui/webui/resources/cr_elements/v1_0/policy/cr_policy_indicator_behavior.js',
On 2015/10/07 18:18:35, michaelpg (OOO til 19th) wrote:
> Wish we could just include the .gyp target like we do for other dependses.

Actually, we can now:
         
'../../../../../ui/webui/resources/cr_elements/v1_0/policy/compiled_resources.gyp:cr_policy_indicator_behavior',

Done.

https://codereview.chromium.org/1391433003/diff/1/ui/webui/resources/cr_eleme...
File ui/webui/resources/cr_elements/v1_0/policy/cr_policy_indicator_behavior.js
(right):

https://codereview.chromium.org/1391433003/diff/1/ui/webui/resources/cr_eleme...
ui/webui/resources/cr_elements/v1_0/policy/cr_policy_indicator_behavior.js:83:
// This case is not handled here since it requires knowlede of the
On 2015/10/07 18:18:35, michaelpg (OOO til 19th) wrote:
> knowledge

Done.

https://codereview.chromium.org/1391433003/diff/1/ui/webui/resources/cr_eleme...
File ui/webui/resources/cr_elements/v1_0/policy/cr_policy_pref_indicator.js
(right):

https://codereview.chromium.org/1391433003/diff/1/ui/webui/resources/cr_eleme...
ui/webui/resources/cr_elements/v1_0/policy/cr_policy_pref_indicator.js:30:
controllingUser: {type: String, value: ''},
On 2015/10/07 18:18:35, michaelpg (OOO til 19th) wrote:
> TODO (for me or you): extract this to a separate indicator when we add more
> indicators using user instead of pref (ie, get rid of my hax)

Done.

[Dan Beam, 2015-10-07 18:40:58]

https://codereview.chromium.org/1391433003/diff/1/ui/webui/resources/cr_eleme...
File ui/webui/resources/cr_elements/v1_0/policy/cr_policy_indicator_behavior.js
(right):

https://codereview.chromium.org/1391433003/diff/1/ui/webui/resources/cr_eleme...
ui/webui/resources/cr_elements/v1_0/policy/cr_policy_indicator_behavior.js:36:
getIcon: function(type) {
remember, these are mixed in with |this| on anything that use this behavior. 
this is kind of like an observer interfaces in C++: the methods should probably
be specific enough that we don't conflict/know which interface they belong to
just by looking at the name.

https://codereview.chromium.org/1391433003/diff/1/ui/webui/resources/cr_eleme...
ui/webui/resources/cr_elements/v1_0/policy/cr_policy_indicator_behavior.js:63:
},
nit: this is probably generic enough to be it's own behavior (and otherwise
would likely conflict when mixed in)

  /**
   * @param {string} id The ID of the translation.
   * @param {...string} var_args Placeholders used in the string.
   * @return {string} The translated text.
   */
  i18n: function(id, var_args) {
    return loadTimeData.getStringF.apply(loadTimeData, arguments);
  },

[stevenjb, 2015-10-07 23:37:15]

Dan, PTAL

https://codereview.chromium.org/1391433003/diff/1/ui/webui/resources/cr_eleme...
File ui/webui/resources/cr_elements/v1_0/policy/cr_policy_indicator_behavior.js
(right):

https://codereview.chromium.org/1391433003/diff/1/ui/webui/resources/cr_eleme...
ui/webui/resources/cr_elements/v1_0/policy/cr_policy_indicator_behavior.js:36:
getIcon: function(type) {
On 2015/10/07 18:40:58, Dan Beam wrote:
> remember, these are mixed in with |this| on anything that use this behavior. 
> this is kind of like an observer interfaces in C++: the methods should
probably
> be specific enough that we don't conflict/know which interface they belong to
> just by looking at the name.

Fair point. -> getPolicyIndicatorIcon

https://codereview.chromium.org/1391433003/diff/1/ui/webui/resources/cr_eleme...
ui/webui/resources/cr_elements/v1_0/policy/cr_policy_indicator_behavior.js:63:
},
On 2015/10/07 18:40:58, Dan Beam wrote:
> nit: this is probably generic enough to be it's own behavior (and otherwise
> would likely conflict when mixed in)
> 
>   /**
>    * @param {string} id The ID of the translation.
>    * @param {...string} var_args Placeholders used in the string.
>    * @return {string} The translated text.
>    */
>   i18n: function(id, var_args) {
>     return loadTimeData.getStringF.apply(loadTimeData, arguments);
>   },

Agreed, and it's on my TODO list. In the meanwhile, renamed i18n_.

https://codereview.chromium.org/1391433003/diff/1/ui/webui/resources/cr_eleme...
ui/webui/resources/cr_elements/v1_0/policy/cr_policy_indicator_behavior.js:71:
getTooltipText: function(type, name) {
Also renamed -> getPolicyIndicatorTooltip

[stevenjb, 2015-10-13 17:30:40]

dbeam@ - ping

[stevenjb, 2015-10-14 20:10:11]

Policy behavior CL 3/3
PTAL

[Dan Beam, 2015-10-15 04:31:01]

lgtm w/suggestions

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

https://codereview.chromium.org/1391433003/diff/140001/ui/webui/resources/cr_...
ui/webui/resources/cr_elements/v1_0/policy/cr_policy_indicator_behavior.js:63:
},
nit: can haz I18nBehavior soon?

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

https://codereview.chromium.org/1391433003/diff/140001/ui/webui/resources/cr_...
ui/webui/resources/cr_elements/v1_0/policy/cr_policy_pref_behavior.js:37: return
CrPolicyIndicatorType.EXTENSION;
nit: use switches, no elses after returns

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

https://codereview.chromium.org/1391433003/diff/140001/ui/webui/resources/cr_...
ui/webui/resources/cr_elements/v1_0/policy/cr_policy_pref_indicator.js:40:
observers: ['prefPolicyChanged_(pref.policySource, pref.policyEnforcement)'],
it seems like indicatorType should just be computed, i.e.

  properties: {
    indicatoryType: {
      type: String,
      value: CrPolicyIndicatorType.NONE,
      computed: 'getIndicatorType(pref.policySource, pref.policyEnforcement)',
    },
  },

and drop prefPolicyChanged_ and observers

[stevenjb, 2015-10-15 22:55:22]

Patchset #10 (id:180001) has been deleted

[stevenjb, 2015-10-15 22:56:22]

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

https://codereview.chromium.org/1391433003/diff/140001/ui/webui/resources/cr_...
ui/webui/resources/cr_elements/v1_0/policy/cr_policy_indicator_behavior.js:63:
},
On 2015/10/15 04:31:01, Dan Beam wrote:
> nit: can haz I18nBehavior soon?

Yes but I didn't want to do too much in one CL :P

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

https://codereview.chromium.org/1391433003/diff/140001/ui/webui/resources/cr_...
ui/webui/resources/cr_elements/v1_0/policy/cr_policy_pref_behavior.js:37: return
CrPolicyIndicatorType.EXTENSION;
On 2015/10/15 04:31:01, Dan Beam wrote:
> nit: use switches, no elses after returns

Done.

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

https://codereview.chromium.org/1391433003/diff/140001/ui/webui/resources/cr_...
ui/webui/resources/cr_elements/v1_0/policy/cr_policy_pref_indicator.js:40:
observers: ['prefPolicyChanged_(pref.policySource, pref.policyEnforcement)'],
On 2015/10/15 04:31:01, Dan Beam wrote:
> it seems like indicatorType should just be computed, i.e.
> 
>   properties: {
>     indicatoryType: {
>       type: String,
>       value: CrPolicyIndicatorType.NONE,
>       computed: 'getIndicatorType(pref.policySource, pref.policyEnforcement)',
>     },
>   },
> 
> and drop prefPolicyChanged_ and observers

Yes, better. Done.

[stevenjb, 2015-10-15 22:56:28]

The CQ bit was checked by stevenjb@chromium.org

[stevenjb, 2015-10-15 22:56:28]

The patchset sent to the CQ was uploaded after l-g-t-m from
michaelpg@chromium.org, dbeam@chromium.org
Link to the patchset: https://codereview.chromium.org/1391433003/#ps200001
(title: "Feedback")

[commit-bot: I haz the power, 2015-10-15 22:57:46]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1391433003/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1391433003/200001

[stevenjb, 2015-10-15 23:48:49]

The CQ bit was unchecked by stevenjb@chromium.org

[stevenjb, 2015-10-15 23:55:45]

The CQ bit was checked by stevenjb@chromium.org

[stevenjb, 2015-10-15 23:55:45]

The patchset sent to the CQ was uploaded after l-g-t-m from dbeam@chromium.org,
michaelpg@chromium.org
Link to the patchset: https://codereview.chromium.org/1391433003/#ps220001
(title: "Feedback")

[commit-bot: I haz the power, 2015-10-15 23:56:02]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1391433003/220001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1391433003/220001

[commit-bot: I haz the power, 2015-10-16 01:13:55]

Committed patchset #11 (id:220001)

[commit-bot: I haz the power, 2015-10-16 01:14:50]

Patchset 11 (id:??) landed as
https://crrev.com/5f7fd124ff6dc5aab4b5fe9f9fa3e020bb630e85
Cr-Commit-Position: refs/heads/master@{#354417}