MD Settings: Add Protected Content toggle to Site Settings

MD Settings: Add Protected Content toggle to Site Settings

Adds the toggle with the same text as the Old Options. The site
exceptions portion is not included in this CL.

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

Review-Url: https://codereview.chromium.org/2673783002
Cr-Commit-Position: refs/heads/master@{#448133}
Committed: https://chromium.googlesource.com/chromium/src/+/2e34f7eb3596b5ff981a4d1554bec36765e23f5d

[tommycli, 2017-02-02 23:23:35]

Description was changed from

==========
MD Settings: Add Protected Content toggle to Site Settings

Adds the toggle with the same text as the Old Options. The site
exceptions portion is not included in this CL.

BUG=687783
==========

to

==========
MD Settings: Add Protected Content toggle to Site Settings

Adds the toggle with the same text as the Old Options. The site
exceptions portion is not included in this CL.

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

[tommycli, 2017-02-02 23:24:29]

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

[commit-bot: I haz the power, 2017-02-02 23:25:09]

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

[tommycli, 2017-02-02 23:25:14]

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

[tommycli, 2017-02-02 23:25:14]

dschuyler: PTAL, thanks!

[tommycli, 2017-02-02 23:48:30]

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

[tommycli, 2017-02-02 23:48:54]

On 2017/02/02 23:25:14, tommycli wrote:
> dschuyler: PTAL, thanks!

Screenshots in c#4 of bug.

[commit-bot: I haz the power, 2017-02-02 23:48:54]

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-02-03 00:49:21]

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

[commit-bot: I haz the power, 2017-02-03 00:49:22]

Dry run: This issue passed the CQ dry run.

[tommycli, 2017-02-03 20:18:52]

tommycli@chromium.org changed reviewers:
+ dpapad@chromium.org

[tommycli, 2017-02-03 20:18:52]

dpapad ptal, thanks!

[dpapad, 2017-02-03 22:03:14]

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

https://codereview.chromium.org/2673783002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/privacy_page/privacy_page.html:442:
<settings-subpage page-title="$i18n{siteSettingsProtectedContent}">
I think this blows up searching. Every subpage is expected to have an
associated-control attribute pointing to the element that opens the subpage, see
examples https://cs.chromium.org/search/?q=associated-control&type=cs.

The repro steps for seeing if this works would be to search for a term appearing
in the new subpage and see if the page is properly highlighted with a search
bubble.

[dpapad, 2017-02-03 22:04:26]

On 2017/02/03 at 22:03:14, dpapad wrote:
>
https://codereview.chromium.org/2673783002/diff/20001/chrome/browser/resource...
> File chrome/browser/resources/settings/privacy_page/privacy_page.html (right):
> 
>
https://codereview.chromium.org/2673783002/diff/20001/chrome/browser/resource...
> chrome/browser/resources/settings/privacy_page/privacy_page.html:442:
<settings-subpage page-title="$i18n{siteSettingsProtectedContent}">
> I think this blows up searching. Every subpage is expected to have an
associated-control attribute pointing to the element that opens the subpage, see
examples https://cs.chromium.org/search/?q=associated-control&type=cs.
> 
> The repro steps for seeing if this works would be to search for a term
appearing in the new subpage and see if the page is properly highlighted with a
search bubble.

BTW, this is the assertion I am expecting this will hit,
https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/search....

[tommycli, 2017-02-03 22:09:03]

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

[tommycli, 2017-02-03 22:09:15]

thanks!

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

https://codereview.chromium.org/2673783002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/privacy_page/privacy_page.html:442:
<settings-subpage page-title="$i18n{siteSettingsProtectedContent}">
On 2017/02/03 22:03:14, dpapad wrote:
> I think this blows up searching. Every subpage is expected to have an
> associated-control attribute pointing to the element that opens the subpage,
see
> examples https://cs.chromium.org/search/?q=associated-control&type=cs.
> 
> The repro steps for seeing if this works would be to search for a term
appearing
> in the new subpage and see if the page is properly highlighted with a search
> bubble.

Done. I added a no-search attribute like the other subpages have. (Now I know
why they have that)

[commit-bot: I haz the power, 2017-02-03 22:09:34]

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

[dpapad, 2017-02-03 22:20:14]

https://codereview.chromium.org/2673783002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/privacy_page/privacy_page.html (right):

https://codereview.chromium.org/2673783002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/privacy_page/privacy_page.html:441: <template
is="dom-if" route-path="/content/protectedContent">
Is this worth a test? There are quite a few site-settings related tests, perhaps
adding one will not have too much overhead?
https://cs.chromium.org/chromium/src/chrome/test/data/webui/settings/cr_setti...

https://codereview.chromium.org/2673783002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/privacy_page/privacy_page.html:443: no-search>
The no-search attribute should be on the template, not here. Such that the
searching algorinthm, will not even bother force rendering this template, when a
search occurs.


BTW, I am realizing that we could improve our tests to catch such cases
(forgetting to add no-search or associated-control). I'll make a note and see if
I can add such a test in the future. Basically load chrome://md-settings, search
for "a" (assuming there will be an occurrence in every single section), ensure
there are no exceptions and that searching completes successfully.

https://codereview.chromium.org/2673783002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/settings_resources.grd (right):

https://codereview.chromium.org/2673783002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/settings_resources.grd:854:
allowexternalscript="true" />
Is the allowexternscript needed?

[tommycli, 2017-02-03 22:50:51]

thanks!

https://codereview.chromium.org/2673783002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/privacy_page/privacy_page.html (right):

https://codereview.chromium.org/2673783002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/privacy_page/privacy_page.html:441: <template
is="dom-if" route-path="/content/protectedContent">
On 2017/02/03 22:20:14, dpapad wrote:
> Is this worth a test? There are quite a few site-settings related tests,
perhaps
> adding one will not have too much overhead?
>
https://cs.chromium.org/chromium/src/chrome/test/data/webui/settings/cr_setti...

Well... since there's no real logic to this section, I feel like a test would be
basically testing site-toggle-button and (in the next CL)
category-settings-exceptions, both which individually have tests.

feel free to disagree, and i'll be happy to write a test for this subpage.

https://codereview.chromium.org/2673783002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/privacy_page/privacy_page.html:443: no-search>
On 2017/02/03 22:20:14, dpapad wrote:
> The no-search attribute should be on the template, not here. Such that the
> searching algorinthm, will not even bother force rendering this template, when
a
> search occurs.
> 
> 
> BTW, I am realizing that we could improve our tests to catch such cases
> (forgetting to add no-search or associated-control). I'll make a note and see
if
> I can add such a test in the future. Basically load chrome://md-settings,
search
> for "a" (assuming there will be an occurrence in every single section), ensure
> there are no exceptions and that searching completes successfully.

Done.

https://codereview.chromium.org/2673783002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/settings_resources.grd (right):

https://codereview.chromium.org/2673783002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/settings_resources.grd:854:
allowexternalscript="true" />
On 2017/02/03 22:20:14, dpapad wrote:
> Is the allowexternscript needed?

Done.

[tommycli, 2017-02-03 22:53:55]

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

[commit-bot: I haz the power, 2017-02-03 22:54:33]

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

[dpapad, 2017-02-03 23:06:20]

LGTM

https://codereview.chromium.org/2673783002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/privacy_page/privacy_page.html (right):

https://codereview.chromium.org/2673783002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/privacy_page/privacy_page.html:441: <template
is="dom-if" route-path="/content/protectedContent">
On 2017/02/03 at 22:50:51, tommycli wrote:
> On 2017/02/03 22:20:14, dpapad wrote:
> > Is this worth a test? There are quite a few site-settings related tests,
perhaps
> > adding one will not have too much overhead?
> >
https://cs.chromium.org/chromium/src/chrome/test/data/webui/settings/cr_setti...
> 
> Well... since there's no real logic to this section, I feel like a test would
be basically testing site-toggle-button and (in the next CL)
category-settings-exceptions, both which individually have tests.
> 
> feel free to disagree, and i'll be happy to write a test for this subpage.

Explanation is good enough for me.

[tommycli, 2017-02-03 23:09:52]

On 2017/02/03 23:06:20, dpapad wrote:
> LGTM
> 
>
https://codereview.chromium.org/2673783002/diff/40001/chrome/browser/resource...
> File chrome/browser/resources/settings/privacy_page/privacy_page.html (right):
> 
>
https://codereview.chromium.org/2673783002/diff/40001/chrome/browser/resource...
> chrome/browser/resources/settings/privacy_page/privacy_page.html:441:
<template
> is="dom-if" route-path="/content/protectedContent">
> On 2017/02/03 at 22:50:51, tommycli wrote:
> > On 2017/02/03 22:20:14, dpapad wrote:
> > > Is this worth a test? There are quite a few site-settings related tests,
> perhaps
> > > adding one will not have too much overhead?
> > >
>
https://cs.chromium.org/chromium/src/chrome/test/data/webui/settings/cr_setti...
> > 
> > Well... since there's no real logic to this section, I feel like a test
would
> be basically testing site-toggle-button and (in the next CL)
> category-settings-exceptions, both which individually have tests.
> > 
> > feel free to disagree, and i'll be happy to write a test for this subpage.
> 
> Explanation is good enough for me.

thanks!

[tommycli, 2017-02-03 23:10:00]

The CQ bit was unchecked by tommycli@chromium.org

[tommycli, 2017-02-03 23:10:00]

The CQ bit was checked by tommycli@chromium.org

[commit-bot: I haz the power, 2017-02-03 23:10:39]

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-02-03 23:18:38]

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

[commit-bot: I haz the power, 2017-02-03 23:18:39]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[tommycli, 2017-02-03 23:23:16]

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

[tommycli, 2017-02-03 23:23:17]

stevenjb: PTAL prefs_util.cc thanks!

[stevenjb, 2017-02-03 23:29:14]

lgtm

[tommycli, 2017-02-03 23:36:30]

The CQ bit was checked by tommycli@chromium.org

[commit-bot: I haz the power, 2017-02-03 23:37:15]

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-02-04 00:55:29]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1486164990251840,
"parent_rev": "f007b70562c84bb115c9178c59a507ee61a7a00d", "commit_rev":
"2e34f7eb3596b5ff981a4d1554bec36765e23f5d"}

[commit-bot: I haz the power, 2017-02-04 00:56:06]

Description was changed from

==========
MD Settings: Add Protected Content toggle to Site Settings

Adds the toggle with the same text as the Old Options. The site
exceptions portion is not included in this CL.

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

to

==========
MD Settings: Add Protected Content toggle to Site Settings

Adds the toggle with the same text as the Old Options. The site
exceptions portion is not included in this CL.

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

Review-Url: https://codereview.chromium.org/2673783002
Cr-Commit-Position: refs/heads/master@{#448133}
Committed:
https://chromium.googlesource.com/chromium/src/+/2e34f7eb3596b5ff981a4d1554be...
==========

[commit-bot: I haz the power, 2017-02-04 00:56:07]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/2e34f7eb3596b5ff981a4d1554be...