MD Settings: add extension-control indicator for autofill and password.

MD Settings: add extension-control indicator for autofill and password.

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

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

[scottchen, 2017-03-07 20:43:05]

Description was changed from

==========
MD Settings: add extension-control indicator for autofill and password.

BUG=693226
==========

to

==========
MD Settings: add extension-control indicator for autofill and password.

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

[scottchen, 2017-03-07 21:39:33]

scottchen@chromium.org changed reviewers:
+ dbeam@chromium.org, dschuyler@chromium.org

[scottchen, 2017-03-07 21:39:34]

See resulting screenshot: http://imgur.com/a/SFz5r

https://codereview.chromium.org/2733273002/diff/20001/chrome/browser/resource...
File
chrome/browser/resources/settings/passwords_and_forms_page/passwords_and_forms_page.html
(right):

https://codereview.chromium.org/2733273002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/passwords_and_forms_page/passwords_and_forms_page.html:36:
<div class$="[[secClass_(prefs.autofill.enabled.value)]]">
This conditional "secondary-action" class is to avoid confusing division lines
like this: http://imgur.com/a/cadCV

There's also a TODO comment in the js file mentioning the plans to move this
toggle into the subpage itself.

https://codereview.chromium.org/2733273002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/passwords_and_forms_page/passwords_and_forms_page.html:68:
<div class$="[[secClass_(prefs.credentials_enable_service.value)]]">
same as above.

https://codereview.chromium.org/2733273002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/passwords_and_forms_page/passwords_and_forms_page.html:83:
"[[prefs.credentials_enable_service.extensionCanBeDisabled]]"
Not sure what I can do about the 80-col situation here.. can't break up anything
starting from the first quotation mark.

https://codereview.chromium.org/2733273002/diff/20001/chrome/browser/resource...
File
chrome/browser/resources/settings/passwords_and_forms_page/passwords_and_forms_page.js
(right):

https://codereview.chromium.org/2733273002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/passwords_and_forms_page/passwords_and_forms_page.js:64:
// subpages themselves, this code *should* be eliminated soon.
See "remove toggles from the top-level and add ON/OFF toggles to each respected
subpage" in this audit tracker:
https://docs.google.com/spreadsheets/d/17uvG_CTZ7N16B7hSdobwSc8VQO6xFPk2Q3Hz4...

[dschuyler, 2017-03-07 22:22:48]

https://codereview.chromium.org/2733273002/diff/20001/chrome/browser/resource...
File
chrome/browser/resources/settings/passwords_and_forms_page/passwords_and_forms_page.html
(right):

https://codereview.chromium.org/2733273002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/passwords_and_forms_page/passwords_and_forms_page.html:36:
<div class$="[[secClass_(prefs.autofill.enabled.value)]]">
On 2017/03/07 21:39:34, scottchen wrote:
> This conditional "secondary-action" class is to avoid confusing division lines
> like this: http://imgur.com/a/cadCV
> 
> There's also a TODO comment in the js file mentioning the plans to move this
> toggle into the subpage itself.

I agree that line looks confusing in that case.
Hey, also please avoid abbreviations. i.e. please name secClass_ something else
(maybe just un-abbreviate).

https://codereview.chromium.org/2733273002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/passwords_and_forms_page/passwords_and_forms_page.html:37:
<settings-toggle-button class="start" id="autofillToggle"
The class="start" should be just inside a .settings-box or .list-item.
I'd like to suggest an alternative, but I'm not sure what this is trying to
address. Let's chat about this.

https://codereview.chromium.org/2733273002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/passwords_and_forms_page/passwords_and_forms_page.html:83:
"[[prefs.credentials_enable_service.extensionCanBeDisabled]]"
On 2017/03/07 21:39:34, scottchen wrote:
> Not sure what I can do about the 80-col situation here.. can't break up
anything
> starting from the first quotation mark.

I think we can break after the "[[

https://codereview.chromium.org/2733273002/diff/20001/chrome/browser/resource...
File
chrome/browser/resources/settings/passwords_and_forms_page/passwords_and_forms_page.js
(right):

https://codereview.chromium.org/2733273002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/passwords_and_forms_page/passwords_and_forms_page.js:57:
this.fire('refresh-pref', 'proxy');
It looks like this was copied from system_page.js
Is it really 'proxy' that we want to refresh here? I may be misunderstanding,
please tell me more about how this works (or whether I'm correct and this
shouldn't be 'proxy').

[Dan Beam, 2017-03-07 22:43:20]

https://codereview.chromium.org/2733273002/diff/20001/chrome/browser/resource...
File
chrome/browser/resources/settings/passwords_and_forms_page/passwords_and_forms_page.js
(right):

https://codereview.chromium.org/2733273002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/passwords_and_forms_page/passwords_and_forms_page.js:57:
this.fire('refresh-pref', 'proxy');
On 2017/03/07 22:22:48, dschuyler wrote:
> It looks like this was copied from system_page.js
> Is it really 'proxy' that we want to refresh here? I may be misunderstanding,
> please tell me more about how this works (or whether I'm correct and this
> shouldn't be 'proxy').

no, as far as I can tell this is a completely unrelated pref path

scottchen@: only some things aren't refreshed.  in fact, I think only the New
Tab page might need this.  proxy and other things might be OK.

did you verify you actually need a refresh()?  the fact that this code refreshes
the wrong pref implies "no" :)

https://codereview.chromium.org/2733273002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/passwords_and_forms_page/passwords_and_forms_page.js:60:
/** @private */
@param

https://codereview.chromium.org/2733273002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/passwords_and_forms_page/passwords_and_forms_page.js:61:
secClass_: function(condition) {
condition should be named less generically

[Dan Beam, 2017-03-07 22:44:02]

https://codereview.chromium.org/2733273002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/controls/settings_input.html (right):

https://codereview.chromium.org/2733273002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/controls/settings_input.html:26: <template
is="dom-if" if="[[hasPrefPolicyIndicator(pref.*)]]" restamp>
why does this need restamp?  the other ones exist for testing.

oh yeah, can you add a test? ;)

[scottchen, 2017-03-08 00:38:47]

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

[commit-bot: I haz the power, 2017-03-08 00:39:45]

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

[scottchen, 2017-03-08 00:40:21]

Uploaded fixes for most comments in patch 3. Will be adding tests as patch 4.

https://codereview.chromium.org/2733273002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/controls/settings_input.html (right):

https://codereview.chromium.org/2733273002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/controls/settings_input.html:26: <template
is="dom-if" if="[[hasPrefPolicyIndicator(pref.*)]]" restamp>
On 2017/03/07 22:44:02, Dan Beam wrote:
> why does this need restamp?  the other ones exist for testing.
> 
> oh yeah, can you add a test? ;)

Will do in the next patch shortly

https://codereview.chromium.org/2733273002/diff/20001/chrome/browser/resource...
File
chrome/browser/resources/settings/passwords_and_forms_page/passwords_and_forms_page.html
(right):

https://codereview.chromium.org/2733273002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/passwords_and_forms_page/passwords_and_forms_page.html:36:
<div class$="[[secClass_(prefs.autofill.enabled.value)]]">
On 2017/03/07 22:22:48, dschuyler wrote:
> On 2017/03/07 21:39:34, scottchen wrote:
> > This conditional "secondary-action" class is to avoid confusing division
lines
> > like this: http://imgur.com/a/cadCV
> > 
> > There's also a TODO comment in the js file mentioning the plans to move this
> > toggle into the subpage itself.
> 
> I agree that line looks confusing in that case.
> Hey, also please avoid abbreviations. i.e. please name secClass_ something
else
> (maybe just un-abbreviate). 

Done.

https://codereview.chromium.org/2733273002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/passwords_and_forms_page/passwords_and_forms_page.html:37:
<settings-toggle-button class="start" id="autofillToggle"
On 2017/03/07 22:22:48, dschuyler wrote:
> The class="start" should be just inside a .settings-box or .list-item.
> I'd like to suggest an alternative, but I'm not sure what this is trying to
> address. Let's chat about this.

This is actually not needed - I originally had other test elements in the same
container, so I added this class to make it look less broken when debugging.
Removing.

https://codereview.chromium.org/2733273002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/passwords_and_forms_page/passwords_and_forms_page.html:83:
"[[prefs.credentials_enable_service.extensionCanBeDisabled]]"
On 2017/03/07 22:22:48, dschuyler wrote:
> On 2017/03/07 21:39:34, scottchen wrote:
> > Not sure what I can do about the 80-col situation here.. can't break up
> anything
> > starting from the first quotation mark.
> 
> I think we can break after the "[[

looks like this works!

https://codereview.chromium.org/2733273002/diff/20001/chrome/browser/resource...
File
chrome/browser/resources/settings/passwords_and_forms_page/passwords_and_forms_page.js
(right):

https://codereview.chromium.org/2733273002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/passwords_and_forms_page/passwords_and_forms_page.js:57:
this.fire('refresh-pref', 'proxy');
On 2017/03/07 22:43:20, Dan Beam wrote:
> On 2017/03/07 22:22:48, dschuyler wrote:
> > It looks like this was copied from system_page.js
> > Is it really 'proxy' that we want to refresh here? I may be
misunderstanding,
> > please tell me more about how this works (or whether I'm correct and this
> > shouldn't be 'proxy').
> 
> no, as far as I can tell this is a completely unrelated pref path
> 
> scottchen@: only some things aren't refreshed.  in fact, I think only the New
> Tab page might need this.  proxy and other things might be OK.
> 
> did you verify you actually need a refresh()?  the fact that this code
refreshes
> the wrong pref implies "no" :)

Removed this and UI still updates, so it looks like we don't need this.

https://codereview.chromium.org/2733273002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/passwords_and_forms_page/passwords_and_forms_page.js:60:
/** @private */
On 2017/03/07 22:43:20, Dan Beam wrote:
> @param

Done.

https://codereview.chromium.org/2733273002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/passwords_and_forms_page/passwords_and_forms_page.js:61:
secClass_: function(condition) {
On 2017/03/07 22:43:20, Dan Beam wrote:
> condition should be named less generically

Done.

[commit-bot: I haz the power, 2017-03-08 00:54:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-08 00:54:10]

Dry run: Try jobs failed on following builders:
  closure_compilation on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/closure_compila...)

[scottchen, 2017-03-08 18:28:24]

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

[commit-bot: I haz the power, 2017-03-08 18:28:53]

Dry run: 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, 2017-03-08 19:37:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-08 19:37:30]

Dry run: This issue passed the CQ dry run.

[dschuyler, 2017-03-09 00:01:22]

lgtm after an import is added.

https://codereview.chromium.org/2733273002/diff/60001/chrome/browser/resource...
File
chrome/browser/resources/settings/passwords_and_forms_page/passwords_and_forms_page.html
(right):

https://codereview.chromium.org/2733273002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/passwords_and_forms_page/passwords_and_forms_page.html:37:
<settings-toggle-button id="autofillToggle"
Please import the settings_toggle_button.html. Even though it's not strictly
needed (i.e. this likely works as-is), it's a style thing in chromium to
import/include all the code used (even if it's redundant).

[Dan Beam, 2017-03-09 00:59:21]

lgtm

[scottchen, 2017-03-09 22:55:17]

dbeam@ and dschuyler@ can one of you re-LG? I added some tests in patch 5 and 6
for the page, and also for settings-toggle-button's indicator behaviors.

I don't think there currently is any tests for settings-input, but the change I
made to that element is basically tested by my tests for settings-toggle-button
(since they extend the same behavior).

https://codereview.chromium.org/2733273002/diff/60001/chrome/browser/resource...
File
chrome/browser/resources/settings/passwords_and_forms_page/passwords_and_forms_page.html
(right):

https://codereview.chromium.org/2733273002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/passwords_and_forms_page/passwords_and_forms_page.html:37:
<settings-toggle-button id="autofillToggle"
On 2017/03/09 00:01:22, dschuyler wrote:
> Please import the settings_toggle_button.html. Even though it's not strictly
> needed (i.e. this likely works as-is), it's a style thing in chromium to
> import/include all the code used (even if it's redundant).

Done.

[Dan Beam, 2017-03-10 00:19:26]

lgtm

[scottchen, 2017-03-10 00:40:30]

The CQ bit was checked by scottchen@chromium.org

[scottchen, 2017-03-10 00:40:30]

The patchset sent to the CQ was uploaded after l-g-t-m from
dschuyler@chromium.org
Link to the patchset: https://codereview.chromium.org/2733273002/#ps100001
(title: "add tests for settings-toggle-button")

[commit-bot: I haz the power, 2017-03-10 00:41:10]

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, 2017-03-10 01:54:24]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1489106430605260,
"parent_rev": "9018c99b7f73801b958f5079f68a8fda1519f480", "commit_rev":
"d8ee83729c29c767b7465dc23940b83a8a88fb89"}

[commit-bot: I haz the power, 2017-03-10 01:55:44]

Description was changed from

==========
MD Settings: add extension-control indicator for autofill and password.

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

to

==========
MD Settings: add extension-control indicator for autofill and password.

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

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

[commit-bot: I haz the power, 2017-03-10 01:55:45]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/d8ee83729c29c767b7465dc23940...

[scottchen, 2017-03-10 03:47:26]

Patchset #7 (id:120001) has been deleted