MD Settings: implement support for extension-controlled startup URLs

MD Settings: implement support for extension-controlled startup URLs

R=dpapad@chromium.org
BUG=614265
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/696a7dd93480b82519ceefb404d96abeba0f02e7
Cr-Commit-Position: refs/heads/master@{#433390}

[Dan Beam, 2016-11-18 23:20:43]

Description was changed from

==========
MD Settings: implement support for extension-controlled startup URLs

R=dpapad@chromium.org
BUG=614265
==========

to

==========
MD Settings: implement support for extension-controlled startup URLs

R=dpapad@chromium.org
BUG=614265
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[Dan Beam, 2016-11-18 23:28:51]

i'm starting to understand what you mean by the "on_startup_*" vs
"startup_pages_*" split not really being necessary...

[dpapad, 2016-11-18 23:46:28]

https://codereview.chromium.org/2520573002/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/controls/controlled_radio_button.js
(right):

https://codereview.chromium.org/2520573002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/controls/controlled_radio_button.js:33: return
this.pref.enforcement == chrome.settingsPrivate.Enforcement.ENFORCED;
This is subtle. What is the difference between |pref.enforcement| and
|pref.policyEnforcement| which the  isPrefPolicyControlled refers to? Should we
just update |isPrefPolicyControlled| to look for both?

https://codereview.chromium.org/2520573002/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/on_startup_page/on_startup_page.html
(right):

https://codereview.chromium.org/2520573002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/on_startup_page/on_startup_page.html:41:
<settings-startup-urls-page prefs="[[prefs]]">
In almost all cases where we pass |prefs| we use two-mway binding "{{prefs}}".
Is this different? Or should we use 1-way everywhere else?

https://codereview.chromium.org/2520573002/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/on_startup_page/startup_url_entry.html
(right):

https://codereview.chromium.org/2520573002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/on_startup_page/startup_url_entry.html:25:
<template is="dom-if" if="[[editable]]">
Can you add a test checking that a non editable entry does not display the
paper-icon-button?

https://codereview.chromium.org/2520573002/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/on_startup_page/startup_urls_page.html
(right):

https://codereview.chromium.org/2520573002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/on_startup_page/startup_urls_page.html:24:
#container settings-startup-url-entry:not([editable]) {
Do we gain anything by over-specifying the selector? (same question for line
20).

Why not just the following?
settings-startup-url-entry:not([editable]) { ...}

[Dan Beam, 2016-11-18 23:51:04]

https://codereview.chromium.org/2520573002/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/controls/controlled_radio_button.js
(right):

https://codereview.chromium.org/2520573002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/controls/controlled_radio_button.js:33: return
this.pref.enforcement == chrome.settingsPrivate.Enforcement.ENFORCED;
On 2016/11/18 23:46:28, dpapad wrote:
> This is subtle. What is the difference between |pref.enforcement| and
> |pref.policyEnforcement| which the  isPrefPolicyControlled refers to? Should
we
> just update |isPrefPolicyControlled| to look for both?

the code you're looking at is outdated, there's only enforcement now

also, isPrefPolicyControlled() does exactly what it says: returns true if the
pref is POLICY controlled.  we need the radio group to be disabled if it's
POLICY controlled OR EXTENSION controlled.  they're not the same.

https://codereview.chromium.org/2520573002/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/on_startup_page/on_startup_page.html
(right):

https://codereview.chromium.org/2520573002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/on_startup_page/on_startup_page.html:41:
<settings-startup-urls-page prefs="[[prefs]]">
On 2016/11/18 23:46:28, dpapad wrote:
> In almost all cases where we pass |prefs| we use two-mway binding "{{prefs}}".
> Is this different? Or should we use 1-way everywhere else? 

i don't set prefs.* in the startup pages.  that's the only reason notify: true
is required

https://codereview.chromium.org/2520573002/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/on_startup_page/startup_url_entry.html
(right):

https://codereview.chromium.org/2520573002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/on_startup_page/startup_url_entry.html:25:
<template is="dom-if" if="[[editable]]">
On 2016/11/18 23:46:28, dpapad wrote:
> Can you add a test checking that a non editable entry does not display the
> paper-icon-button?

yes (this is exactly the thing I planned on testing, actually)

https://codereview.chromium.org/2520573002/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/on_startup_page/startup_urls_page.html
(right):

https://codereview.chromium.org/2520573002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/on_startup_page/startup_urls_page.html:24:
#container settings-startup-url-entry:not([editable]) {
On 2016/11/18 23:46:28, dpapad wrote:
> Do we gain anything by over-specifying the selector? (same question for line
> 20).
> 
> Why not just the following?
> settings-startup-url-entry:not([editable]) { ...}

required for specificity (had your version originally, didn't work)

[Dan Beam, 2016-11-18 23:55:55]

The CQ bit was checked by dbeam@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-18 23:56:46]

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

[Dan Beam, 2016-11-19 00:09:01]

The CQ bit was checked by dbeam@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-19 00:09:32]

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

[dpapad, 2016-11-19 00:20:15]

https://codereview.chromium.org/2520573002/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/controls/controlled_radio_button.js
(right):

https://codereview.chromium.org/2520573002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/controls/controlled_radio_button.js:33: return
this.pref.enforcement == chrome.settingsPrivate.Enforcement.ENFORCED;
On 2016/11/18 at 23:51:04, Dan Beam wrote:
> On 2016/11/18 23:46:28, dpapad wrote:
> > This is subtle. What is the difference between |pref.enforcement| and
> > |pref.policyEnforcement| which the  isPrefPolicyControlled refers to? Should
we
> > just update |isPrefPolicyControlled| to look for both?
> 
> the code you're looking at is outdated, there's only enforcement now
> 
> also, isPrefPolicyControlled() does exactly what it says: returns true if the
pref is POLICY controlled.  we need the radio group to be disabled if it's
POLICY controlled OR EXTENSION controlled.  they're not the same.

Ok. I am slightly confused, so I am going to ask qustions to get a better
understanding.

Q1: CrPolicyPrefBehavior, is about POLICY controlled things only, and
PrefControlBehavior has nothing to do with prefs that are "controlled by"
(policy, extension, etc). Is my understanding accurate? If yes, isn't it
confusing if it has nothing to do with "controlled by" things?

Q2: Is this element using any of the CrPolicyPrefBehavior methods anymore? Or
can that behavior be removed?

[Dan Beam, 2016-11-19 00:36:21]

The CQ bit was checked by dbeam@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-19 00:37:17]

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

[Dan Beam, 2016-11-19 00:47:55]

The CQ bit was checked by dbeam@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-19 00:48:40]

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

[Dan Beam, 2016-11-19 00:50:05]

https://codereview.chromium.org/2520573002/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/controls/controlled_radio_button.js
(right):

https://codereview.chromium.org/2520573002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/controls/controlled_radio_button.js:33: return
this.pref.enforcement == chrome.settingsPrivate.Enforcement.ENFORCED;
On 2016/11/19 00:20:15, dpapad wrote:
> On 2016/11/18 at 23:51:04, Dan Beam wrote:
> > On 2016/11/18 23:46:28, dpapad wrote:
> > > This is subtle. What is the difference between |pref.enforcement| and
> > > |pref.policyEnforcement| which the  isPrefPolicyControlled refers to?
Should
> we
> > > just update |isPrefPolicyControlled| to look for both?
> > 
> > the code you're looking at is outdated, there's only enforcement now
> > 
> > also, isPrefPolicyControlled() does exactly what it says: returns true if
the
> pref is POLICY controlled.  we need the radio group to be disabled if it's
> POLICY controlled OR EXTENSION controlled.  they're not the same.
> 
> Ok. I am slightly confused, so I am going to ask qustions to get a better
> understanding.
> 
> Q1: CrPolicyPrefBehavior, is about POLICY controlled things only,

yes

> and
> PrefControlBehavior has nothing to do with prefs that are "controlled by"
> (policy, extension, etc). Is my understanding accurate?

yes

> If yes, isn't it
> confusing if it has nothing to do with "controlled by" things?

it's "control" as in a button or input.  we should rename it, along with
SettingsBooleanControlBehaviorImpl.

> 
> Q2: Is this element using any of the CrPolicyPrefBehavior methods anymore? Or
> can that behavior be removed?

removed

[Dan Beam, 2016-11-19 01:21:45]

added tests and fixed closure compile, ptal

[Dan Beam, 2016-11-19 01:21:50]

The CQ bit was checked by dbeam@chromium.org to run a CQ dry run

[Dan Beam, 2016-11-19 01:21:54]

Patchset #8 (id:130001) has been deleted

[Dan Beam, 2016-11-19 01:22:00]

Patchset #7 (id:110001) has been deleted

[Dan Beam, 2016-11-19 01:22:12]

Patchset #6 (id:50009) has been deleted

[commit-bot: I haz the power, 2016-11-19 01:22:17]

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

[Dan Beam, 2016-11-19 01:22:31]

Patchset #4 (id:60001) has been deleted

[dpapad, 2016-11-19 01:32:39]

LGTM

https://codereview.chromium.org/2520573002/diff/150001/chrome/test/data/webui...
File chrome/test/data/webui/settings/startup_urls_page_test.js (right):

https://codereview.chromium.org/2520573002/diff/150001/chrome/test/data/webui...
chrome/test/data/webui/settings/startup_urls_page_test.js:319: element.editable
= true;
Nit (optional): Perhaps do this in setup() so that by default setup() creates an
editable entry (as it was before this CL).

[Dan Beam, 2016-11-19 01:55:28]

https://codereview.chromium.org/2520573002/diff/150001/chrome/test/data/webui...
File chrome/test/data/webui/settings/startup_urls_page_test.js (right):

https://codereview.chromium.org/2520573002/diff/150001/chrome/test/data/webui...
chrome/test/data/webui/settings/startup_urls_page_test.js:319: element.editable
= true;
On 2016/11/19 01:32:39, dpapad wrote:
> Nit (optional): Perhaps do this in setup() so that by default setup() creates
an
> editable entry (as it was before this CL).

eh, that results in more touching of the prod code (because otherwise the
'Editable' test fails, and I don't like using assertEquals(.style.display,
'none'))

[Dan Beam, 2016-11-19 01:55:49]

https://codereview.chromium.org/2520573002/diff/150001/chrome/test/data/webui...
File chrome/test/data/webui/settings/startup_urls_page_test.js (right):

https://codereview.chromium.org/2520573002/diff/150001/chrome/test/data/webui...
chrome/test/data/webui/settings/startup_urls_page_test.js:319: element.editable
= true;
On 2016/11/19 01:55:28, Dan Beam wrote:
> On 2016/11/19 01:32:39, dpapad wrote:
> > Nit (optional): Perhaps do this in setup() so that by default setup()
creates
> an
> > editable entry (as it was before this CL).
> 
> eh, that results in more touching of the prod code (because otherwise the
> 'Editable' test fails, and I don't like using assertEquals(.style.display,
> 'none'))

(if I don't add restamp to the dom-if in the prod code)

[Dan Beam, 2016-11-19 01:56:05]

The CQ bit was unchecked by dbeam@chromium.org

[Dan Beam, 2016-11-19 01:56:12]

The CQ bit was checked by dbeam@chromium.org

[commit-bot: I haz the power, 2016-11-19 01:56:49]

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

[commit-bot: I haz the power, 2016-11-19 03:30:24]

Committed patchset #5 (id:150001)

[commit-bot: I haz the power, 2016-11-19 03:34:44]

Description was changed from

==========
MD Settings: implement support for extension-controlled startup URLs

R=dpapad@chromium.org
BUG=614265
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
MD Settings: implement support for extension-controlled startup URLs

R=dpapad@chromium.org
BUG=614265
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/696a7dd93480b82519ceefb404d96abeba0f02e7
Cr-Commit-Position: refs/heads/master@{#433390}
==========

[commit-bot: I haz the power, 2016-11-19 03:34:45]

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