Add cr-policy-network-indicator and add to internet settings

Add cr-policy-network-indicator and add to internet settings

This CL:
* Moves settings/PolicyControllable -> cr_elements/CrPolicyPrefBehavior
* Turns CrPolicyIndicator into a general purpose behavior, CrPolicyIndicatorBehavior
* Adds CrPolicyPrefBehavior for pref specific behavior
* Adds CrPolicyPrefIndicator for pref based indicators
* Adds CrPolicyNetworkBehavior for networking specific behavior
* Adds CrPolicyNetworkIndicator for network based indicators

This CL should also make it easier to include just the behaviors for
Closure compilation.

[stevenjb, 2015-10-02 23:00:22]

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

[michaelpg, 2015-10-03 18:52:14]

this naming scheme is confusing. how do "CrPolicySettings" and
"CrPolicyPrefIndicator" and "CrPolicyIndicator" fit together?

I would suggest:

CrPolicyIndicatorBehavior: A behavior for a policy indicator.
    Your "CrPolicyIndicator".
CrPrefPolicyBehavior: A behavior for a pref that may be policy-controlled.
    Your "CrPolicySettings".
CrPrefPolicyIndicator: A policy indicator for a pref.
    Your "CrPolicyPrefIndicator".
CrNetworkingPolicyBehavior: A behavior for a policy that controls networking.
    Your "CrPolicyNetworking".
CrNetworkingPolicyIndicator: A policy indicator for networking.
    Your "CrPolicyNetworkingIndicator".

IMO this makes the relationships clear: a PrefPolicyIndicator implements the
PrefPolicy and PolicyIndicator behaviors. a NetworkingPolicyIndicator is similar
but uses the NetworkingPolicy behavior to inform the PolicyIndicator.


Rationale:
1. I think we should follow the Polymer convention of naming behaviors
ThingBehavior to avoid confusion: CrPolicyIndicatorBehavior.
2. if we have two types of PolicyIndicators, one for Prefs and one for Networks,
I would call them the PrefPolicyIndicator and the NetworkPolicyIndicator. That
parses in English as "an indicator of {pref,network} policy". It also keeps the
"PolicyIndicator" intact, kind of like how NonClientFrameView has a subclass,
BrowserNonClientFrameView, not "NonClientBrowerFrameView".


A secondary concern: doesn't Pref stuff belong in Settings?

[Dan Beam, 2015-10-04 17:21:20]

On 2015/10/03 18:52:14, michaelpg (OOO til 19th) wrote:
> A secondary concern: doesn't Pref stuff belong in Settings?

I'm just skimming but: preferences are used (either respected or set) from
outside of settings, btw

[michaelpg, 2015-10-04 17:25:48]

On 2015/10/04 17:21:20, Dan Beam wrote:
> On 2015/10/03 18:52:14, michaelpg (OOO til 19th) wrote:
> > A secondary concern: doesn't Pref stuff belong in Settings?
> 
> I'm just skimming but: preferences are used (either respected or set) from
> outside of settings, btw

yes, but our general pattern is to avoid moving things to shared locations until
a second consumer shows up.

and having stuff in ui/webui/resources reference loadTimeData values without
documenting what file needs to be included (which, currently, is
md_settings_localized_blah) is weeeird. Although cr_policy_indicator.js is
already guilty of that.

[Dan Beam, 2015-10-04 19:28:29]

On 2015/10/04 17:25:48, michaelpg (OOO til 19th) wrote:
> On 2015/10/04 17:21:20, Dan Beam wrote:
> > On 2015/10/03 18:52:14, michaelpg (OOO til 19th) wrote:
> > > A secondary concern: doesn't Pref stuff belong in Settings?
> > 
> > I'm just skimming but: preferences are used (either respected or set) from
> > outside of settings, btw
> 
> yes, but our general pattern is to avoid moving things to shared locations
until
> a second consumer shows up.
> 
> and having stuff in ui/webui/resources reference loadTimeData values without
> documenting what file needs to be included (which, currently, is
> md_settings_localized_blah) is weeeird. Although cr_policy_indicator.js is
> already guilty of that.

well, regarding load_time_data.js being around: that's why we should move to
html imports because you can express and de-dupe dependencies

you're right that using loadTimeData.getThing('dataSourceSpecificName') is odd,
though, unless we split messages to a shared data source

[stevenjb, 2015-10-05 17:44:48]

On 2015/10/04 19:28:29, Dan Beam wrote:
> On 2015/10/04 17:25:48, michaelpg (OOO til 19th) wrote:
> > On 2015/10/04 17:21:20, Dan Beam wrote:
> > > On 2015/10/03 18:52:14, michaelpg (OOO til 19th) wrote:
> > > > A secondary concern: doesn't Pref stuff belong in Settings?
> > > 
> > > I'm just skimming but: preferences are used (either respected or set) from
> > > outside of settings, btw
> > 
> > yes, but our general pattern is to avoid moving things to shared locations
> until
> > a second consumer shows up.
> > 
> > and having stuff in ui/webui/resources reference loadTimeData values without
> > documenting what file needs to be included (which, currently, is
> > md_settings_localized_blah) is weeeird. Although cr_policy_indicator.js is
> > already guilty of that.
> 
> well, regarding load_time_data.js being around: that's why we should move to
> html imports because you can express and de-dupe dependencies
> 
> you're right that using loadTimeData.getThing('dataSourceSpecificName') is
odd,
> though, unless we split messages to a shared data source

WRT location of policy web UI:
* We will want to use this in the Extensions UI at least, so it needs to be in a
shared location.
* I think that the policy indicator code should all be in one place rather than
scattered so I would prefer to have it all in cr_elements/.
* I agree that using settings specific localized strings even in the short term
is wonky, so I will separate that into its own .cc file.

WRT naming:
* I am fine with appending 'Behavior' to the names and the other name changes.

[stevenjb, 2015-10-05 18:14:08]

On 2015/10/05 17:44:48, stevenjb wrote:
> On 2015/10/04 19:28:29, Dan Beam wrote:
> > On 2015/10/04 17:25:48, michaelpg (OOO til 19th) wrote:
> > > On 2015/10/04 17:21:20, Dan Beam wrote:
> > > > On 2015/10/03 18:52:14, michaelpg (OOO til 19th) wrote:
> > > > > A secondary concern: doesn't Pref stuff belong in Settings?
> > > > 
> > > > I'm just skimming but: preferences are used (either respected or set)
from
> > > > outside of settings, btw
> > > 
> > > yes, but our general pattern is to avoid moving things to shared locations
> > until
> > > a second consumer shows up.
> > > 
> > > and having stuff in ui/webui/resources reference loadTimeData values
without
> > > documenting what file needs to be included (which, currently, is
> > > md_settings_localized_blah) is weeeird. Although cr_policy_indicator.js is
> > > already guilty of that.
> > 
> > well, regarding load_time_data.js being around: that's why we should move to
> > html imports because you can express and de-dupe dependencies
> > 
> > you're right that using loadTimeData.getThing('dataSourceSpecificName') is
> odd,
> > though, unless we split messages to a shared data source
> 
> WRT location of policy web UI:
> * We will want to use this in the Extensions UI at least, so it needs to be in
a
> shared location.
> * I think that the policy indicator code should all be in one place rather
than
> scattered so I would prefer to have it all in cr_elements/.
> * I agree that using settings specific localized strings even in the short
term
> is wonky, so I will separate that into its own .cc file.
> 
> WRT naming:
> * I am fine with appending 'Behavior' to the names and the other name changes.

Actually, one place I differ in naming style is, e.g. CrPolicyPrefBehavior
instead of CrPrefPolicyBehavior. Even though the latter is more "human readable"
(in English), I prefer to use a hierarchical naming scheme for two reasons:
* It makes file / include ordering more readable.
* It makes it slightly easier to grep for, e.g. CrPolicy* of find cr_policy*.

[stevenjb, 2015-10-05 18:17:42]

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

[michaelpg, 2015-10-05 19:02:02]

ok. I'm fine with all of that. Only have 15 minutes of free Internet left
today so probably won't be able to review :-)

On Mon, Oct 5, 2015 at 11:44 PM, <stevenjb@chromium.org> wrote:

> On 2015/10/05 17:44:48, stevenjb wrote:
>
>> On 2015/10/04 19:28:29, Dan Beam wrote:
>> > On 2015/10/04 17:25:48, michaelpg (OOO til 19th) wrote:
>> > > On 2015/10/04 17:21:20, Dan Beam wrote:
>> > > > On 2015/10/03 18:52:14, michaelpg (OOO til 19th) wrote:
>> > > > > A secondary concern: doesn't Pref stuff belong in Settings?
>> > > >
>> > > > I'm just skimming but: preferences are used (either respected or
>> set)
>>
> from
>
>> > > > outside of settings, btw
>> > >
>> > > yes, but our general pattern is to avoid moving things to shared
>> locations
>> > until
>> > > a second consumer shows up.
>> > >
>> > > and having stuff in ui/webui/resources reference loadTimeData values
>>
> without
>
>> > > documenting what file needs to be included (which, currently, is
>> > > md_settings_localized_blah) is weeeird. Although
>> cr_policy_indicator.js is
>> > > already guilty of that.
>> >
>> > well, regarding load_time_data.js being around: that's why we should
>> move to
>> > html imports because you can express and de-dupe dependencies
>> >
>> > you're right that using loadTimeData.getThing('dataSourceSpecificName')
>> is
>> odd,
>> > though, unless we split messages to a shared data source
>>
>
> WRT location of policy web UI:
>> * We will want to use this in the Extensions UI at least, so it needs to
>> be in
>>
> a
>
>> shared location.
>> * I think that the policy indicator code should all be in one place rather
>>
> than
>
>> scattered so I would prefer to have it all in cr_elements/.
>> * I agree that using settings specific localized strings even in the short
>>
> term
>
>> is wonky, so I will separate that into its own .cc file.
>>
>
> WRT naming:
>> * I am fine with appending 'Behavior' to the names and the other name
>> changes.
>>
>
> Actually, one place I differ in naming style is, e.g. CrPolicyPrefBehavior
> instead of CrPrefPolicyBehavior. Even though the latter is more "human
> readable"
> (in English), I prefer to use a hierarchical naming scheme for two reasons:
> * It makes file / include ordering more readable.
> * It makes it slightly easier to grep for, e.g. CrPolicy* of find
> cr_policy*.
>
>
>
> https://codereview.chromium.org/1369403006/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[stevenjb, 2015-10-05 22:28:42]

PTAL

[Dan Beam, 2015-10-05 23:59:13]

This CL:
* Moves settings/PolicyControllable -> cr_elements/CrPolicyPrefBehavior
* Turns CrPolicyIndicator into a general purpose behavior,
CrPolicyIndicatorBehavior
* Adds CrPolicyPrefBehavior for pref specific behavior
* Adds CrPolicyPrefIndicator for pref based indicators
* Adds CrPolicyNetworkBehavior for networking specific behavior
* Adds CrPolicyNetworkIndicator for network based indicators

^ can you do this in more CLs rather than just 1?

[stevenjb, 2015-10-06 00:07:46]

On 2015/10/05 23:59:13, Dan Beam wrote:
> This CL:
> * Moves settings/PolicyControllable -> cr_elements/CrPolicyPrefBehavior
> * Turns CrPolicyIndicator into a general purpose behavior,
> CrPolicyIndicatorBehavior
> * Adds CrPolicyPrefBehavior for pref specific behavior
> * Adds CrPolicyPrefIndicator for pref based indicators
> * Adds CrPolicyNetworkBehavior for networking specific behavior
> * Adds CrPolicyNetworkIndicator for network based indicators
> 
> ^ can you do this in more CLs rather than just 1?

I can try to break it up a little, but I'm not sure how much sense the
intermediate steps will make. (Opinions seem to vary on non-functional
intermediate CLs). I'll see what I can do.

[stevenjb, 2015-10-06 01:03:33]

On 2015/10/06 00:07:46, stevenjb wrote:
> On 2015/10/05 23:59:13, Dan Beam wrote:
> > This CL:
> > * Moves settings/PolicyControllable -> cr_elements/CrPolicyPrefBehavior
> > * Turns CrPolicyIndicator into a general purpose behavior,
> > CrPolicyIndicatorBehavior
> > * Adds CrPolicyPrefBehavior for pref specific behavior
> > * Adds CrPolicyPrefIndicator for pref based indicators
> > * Adds CrPolicyNetworkBehavior for networking specific behavior
> > * Adds CrPolicyNetworkIndicator for network based indicators
> > 
> > ^ can you do this in more CLs rather than just 1?
> 
> I can try to break it up a little, but I'm not sure how much sense the
> intermediate steps will make. (Opinions seem to vary on non-functional
> intermediate CLs). I'll see what I can do.

OK, separating the pref indicator re-factoring and the addition of the network
indicator makes a lot of sense:
* https://codereview.chromium.org/1391433003 - re-factoring
* https://codereview.chromium.org/1376383003 - Add cr-policy-network-indicator