[MD settings] move extra options in category settings

[MD settings] move extra options in category settings

This CL refactors the site settings category code to move one-off
details from that code. This was done by moving much of the code to a
new category default setting module. The site_settings_category has been
replaced with content_setting_exceptions.

related to bug 662596
BUG=None
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/891846bf42988a708ce18472546e1361387507a6
Cr-Commit-Position: refs/heads/master@{#439243}

[dschuyler, 2016-12-08 23:19:17]

Description was changed from

==========
[MD settings] move extra options in category settings

This CL refactors the site settings category code to move one-off
details from that code. This was done by moving much of the code to a
new category default module that allows more customization from the
privacy settings code.

related to bug 662596
BUG=None
==========

to

==========
[MD settings] move extra options in category settings

This CL refactors the site settings category code to move one-off
details from that code. This was done by moving much of the code to a
new category default module that allows more customization from the
privacy settings code.

related to bug 662596
BUG=None
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[dschuyler, 2016-12-08 23:19:19]

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

[commit-bot: I haz the power, 2016-12-08 23:19:43]

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, 2016-12-08 23:29:30]

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

[commit-bot: I haz the power, 2016-12-08 23:29:31]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[dschuyler, 2016-12-08 23:42:05]

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

[commit-bot: I haz the power, 2016-12-08 23:42:47]

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

[dschuyler, 2016-12-08 23:43:29]

Patchset #1 (id:1) has been deleted

[dschuyler, 2016-12-08 23:46:26]

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

[dschuyler, 2016-12-08 23:46:27]

This is a prequel CL for CL 2509163004. It makes the category page more generic
and moves custom bits elsewhere to reduce the dom-if's and one-off vars and
such.

[commit-bot: I haz the power, 2016-12-08 23:54:41]

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

[commit-bot: I haz the power, 2016-12-08 23:54:42]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[tommycli, 2016-12-09 00:13:19]

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

https://codereview.chromium.org/2554403005/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/privacy_page/privacy_page.html:223:
<category-default class="category-default"
I see that each of these has the class="category-default". What's the reason?

https://codereview.chromium.org/2554403005/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/privacy_page/privacy_page.html:264:
<category-default class="category-default"
Does every single site-settings-category node have a category-default as a child
element?

If so - I would recommend just making category-default part of
site-settings-category. You can pass in the extra options such as
sub-option-label and sub-option-secondary as attributes of
site-settings-category.

That would also allow you to avoid the one-off differences per data type.

I see you also sometimes need to put some extra controls either before OR after
the main controls.

In that case I suggest using
https://www.polymer-project.org/1.0/docs/devguide/local-dom#dom-distribution

So the template of site-settings-category might look like:

<template>
  <content select=".before-default-controls"></content>

  <!-- All the controls within category-default here -->

  <content select=".after-default-controls"></content>
</template>

https://codereview.chromium.org/2554403005/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/privacy_page/privacy_page.html:387: <if
expr="chromeos">
May be cleaner to add this link in separate CL.

https://codereview.chromium.org/2554403005/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/privacy_page/privacy_page.html:389: <div
class="flex">
Can this flex class be consolidated into the parent div. In other words, do we
need the extra div?

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

https://codereview.chromium.org/2554403005/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/privacy_page/privacy_page.js:173:
'documentation/en/flashplayer/help/settings_manager07.html');
Interesting -- in other places, I've seen URLs be placed in the C++ and loaded
in via load time data. Also... is it intentional that the documentation is
specifically for the english version? maybe defer this to separate cl

[dschuyler, 2016-12-09 01:19:17]

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

[commit-bot: I haz the power, 2016-12-09 01:20:07]

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, 2016-12-09 03:22:54]

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

[commit-bot: I haz the power, 2016-12-09 03:22:54]

Dry run: This issue passed the CQ dry run.

[dschuyler, 2016-12-13 23:57:38]

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

https://codereview.chromium.org/2554403005/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/privacy_page/privacy_page.html:223:
<category-default class="category-default"
On 2016/12/09 00:13:19, tommycli wrote:
> I see that each of these has the class="category-default". What's the reason?

The category-default holds those top one or two toggles that enable/disable the
category.

https://codereview.chromium.org/2554403005/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/privacy_page/privacy_page.html:264:
<category-default class="category-default"
On 2016/12/09 00:13:19, tommycli wrote:
> Does every single site-settings-category node have a category-default as a
child
> element?
> 
> If so - I would recommend just making category-default part of
> site-settings-category. You can pass in the extra options such as
> sub-option-label and sub-option-secondary as attributes of
> site-settings-category.
> 
> That would also allow you to avoid the one-off differences per data type.
> 
> I see you also sometimes need to put some extra controls either before OR
after
> the main controls.
> 
> In that case I suggest using
> https://www.polymer-project.org/1.0/docs/devguide/local-dom#dom-distribution
> 
> So the template of site-settings-category might look like:
> 
> <template>
>   <content select=".before-default-controls"></content>
> 
>   <!-- All the controls within category-default here -->
> 
>   <content select=".after-default-controls"></content>
> </template>

The intent of the CL is to move the bits that are specific to different
categories out of the general site-settings-category page. I can consider other
ways to achieve that.

https://codereview.chromium.org/2554403005/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/privacy_page/privacy_page.html:387: <if
expr="chromeos">
On 2016/12/09 00:13:19, tommycli wrote:
> May be cleaner to add this link in separate CL.

Done.

https://codereview.chromium.org/2554403005/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/privacy_page/privacy_page.html:389: <div
class="flex">
On 2016/12/09 00:13:19, tommycli wrote:
> Can this flex class be consolidated into the parent div. In other words, do we
> need the extra div?

I was trying to avoid making edits to the code I was moving, but this does seem
like a simple change.

Done in a separate CL.

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

https://codereview.chromium.org/2554403005/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/privacy_page/privacy_page.js:173:
'documentation/en/flashplayer/help/settings_manager07.html');
On 2016/12/09 00:13:19, tommycli wrote:
> Interesting -- in other places, I've seen URLs be placed in the C++ and loaded
> in via load time data. Also... is it intentional that the documentation is
> specifically for the english version? maybe defer this to separate cl

This code is moved. I didn't look into the details of the implementation. (I did
check that it worked though).
There may be a bug here, but I think that should be fixed by a specific CL
rather than a refactor.

[dschuyler, 2016-12-14 00:00:53]

I've been thinking on this further. I'd like to factor out the pieces of the
category further. e.g. the media-settings don't need to be brought in by a
selector in the category, they could simply be placed in correct place in
privacy.html. Similar for the category-default, the extra-options and the
exceptions.

[dschuyler, 2016-12-14 01:00:31]

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

[commit-bot: I haz the power, 2016-12-14 01:01:05]

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, 2016-12-14 02:09:09]

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

[commit-bot: I haz the power, 2016-12-14 02:09:10]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[dschuyler, 2016-12-14 23:47:43]

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

[commit-bot: I haz the power, 2016-12-14 23:48:14]

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

[dschuyler, 2016-12-14 23:54:13]

Description was changed from

==========
[MD settings] move extra options in category settings

This CL refactors the site settings category code to move one-off
details from that code. This was done by moving much of the code to a
new category default module that allows more customization from the
privacy settings code.

related to bug 662596
BUG=None
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
[MD settings] move extra options in category settings

This CL refactors the site settings category code to move one-off
details from that code. This was done by moving much of the code to a
new category default setting module. The site_settings_category has been
replaced with content_setting_exceptions.

related to bug 662596
BUG=None
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[dschuyler, 2016-12-14 23:56:03]

This patch includes the refactoring and renaming we discussed off-line.
Something we didn't look at before were the changes to the unit tests, though
this is largely rearranging things for the refactoring and renaming.

[commit-bot: I haz the power, 2016-12-15 00:14:25]

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

[commit-bot: I haz the power, 2016-12-15 00:14:26]

Dry run: 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, 2016-12-15 21:36:38]

lgtm except below:

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

https://codereview.chromium.org/2554403005/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/privacy_page/privacy_page.html:223:
<category-setting-exceptions selected-site="{{selectedSite}}"
This can be in a followup CL, but reading the code, I am 95% sure you can remove
all instances of selected-site attributes and selectedSite properties.

It appears that site-list now sets the URLSearchParameter, so using this for
two-day data binding is no longer needed.

https://codereview.chromium.org/2554403005/diff/80001/chrome/browser/resource...
File
chrome/browser/resources/settings/site_settings/category_default_setting.html
(right):

https://codereview.chromium.org/2554403005/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/category_default_setting.html:9:
<dom-module id="category-default-setting">
This and the corresponding js file are basically straight copies right? other
than consolidating flashAskFirst_ and cookiesSessionOnly_ into a single
property?

https://codereview.chromium.org/2554403005/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/category_default_setting.html:11:
<style include="settings-shared"></style>
I see there used to be a rule for paper-toggle-button in site_settings_category.
Is that no longer needed?

https://codereview.chromium.org/2554403005/diff/80001/chrome/browser/resource...
File
chrome/browser/resources/settings/site_settings/category_setting_exceptions.js
(right):

https://codereview.chromium.org/2554403005/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/category_setting_exceptions.js:18:
selectedSite: {
As stated before this is probably not needed anymore. Consider maybe even
removing it in this CL (or before this CL) since adding new code propagating
something no longer needed sucks.

https://codereview.chromium.org/2554403005/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/category_setting_exceptions.js:25:
this.PermissionValues = settings.PermissionValues;
This can probably be in created (in case it's needed sooner). Also add an /**
@override */

https://codereview.chromium.org/2554403005/diff/80001/chrome/browser/resource...
File chrome/browser/resources/settings/site_settings/compiled_resources2.gyp
(right):

https://codereview.chromium.org/2554403005/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/compiled_resources2.gyp:163:
'target_name': 'category_setting_exceptions',
Should we add a target for category_default_setting also?

Also the exceptions element no longer needs to depend on the behavior target,
nor the webui.

https://codereview.chromium.org/2554403005/diff/80001/chrome/test/data/webui/...
File chrome/test/data/webui/settings/category_setting_exceptions_tests.js
(right):

https://codereview.chromium.org/2554403005/diff/80001/chrome/test/data/webui/...
chrome/test/data/webui/settings/category_setting_exceptions_tests.js:13: var
testElement;
= null; I've normally seen

https://codereview.chromium.org/2554403005/diff/80001/chrome/test/data/webui/...
chrome/test/data/webui/settings/category_setting_exceptions_tests.js:33:
assertTrue(!!testElement);
Well if there's legitimately nothing that merits testing we can just delete this
whole file. I agree (btw) that there's not much worth testing there.

[dschuyler, 2016-12-16 19:15:32]

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

[commit-bot: I haz the power, 2016-12-16 19:16:06]

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

[dschuyler, 2016-12-16 20:39:46]

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

https://codereview.chromium.org/2554403005/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/privacy_page/privacy_page.html:223:
<category-setting-exceptions selected-site="{{selectedSite}}"
On 2016/12/15 21:36:38, tommycli wrote:
> This can be in a followup CL, but reading the code, I am 95% sure you can
remove
> all instances of selected-site attributes and selectedSite properties.
> 
> It appears that site-list now sets the URLSearchParameter, so using this for
> two-day data binding is no longer needed.

Done.

https://codereview.chromium.org/2554403005/diff/80001/chrome/browser/resource...
File
chrome/browser/resources/settings/site_settings/category_default_setting.html
(right):

https://codereview.chromium.org/2554403005/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/category_default_setting.html:9:
<dom-module id="category-default-setting">
On 2016/12/15 21:36:38, tommycli wrote:
> This and the corresponding js file are basically straight copies right? other
> than consolidating flashAskFirst_ and cookiesSessionOnly_ into a single
> property?

That's the intention.

https://codereview.chromium.org/2554403005/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/category_default_setting.html:11:
<style include="settings-shared"></style>
On 2016/12/15 21:36:38, tommycli wrote:
> I see there used to be a rule for paper-toggle-button in
site_settings_category.
> Is that no longer needed?

Correct (err, it wasn't needed in the code before this CL either, its use was
older than that).

https://codereview.chromium.org/2554403005/diff/80001/chrome/browser/resource...
File
chrome/browser/resources/settings/site_settings/category_setting_exceptions.js
(right):

https://codereview.chromium.org/2554403005/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/category_setting_exceptions.js:18:
selectedSite: {
On 2016/12/15 21:36:38, tommycli wrote:
> As stated before this is probably not needed anymore. Consider maybe even
> removing it in this CL (or before this CL) since adding new code propagating
> something no longer needed sucks.

Done.

https://codereview.chromium.org/2554403005/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/category_setting_exceptions.js:25:
this.PermissionValues = settings.PermissionValues;
On 2016/12/15 21:36:38, tommycli wrote:
> This can probably be in created (in case it's needed sooner). Also add an /**
> @override */

That doesn't work. I tried it before putting this patch up. The trouble iirc is
that |settings| is not fully setup yet.

https://codereview.chromium.org/2554403005/diff/80001/chrome/browser/resource...
File chrome/browser/resources/settings/site_settings/compiled_resources2.gyp
(right):

https://codereview.chromium.org/2554403005/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/compiled_resources2.gyp:163:
'target_name': 'category_setting_exceptions',
On 2016/12/15 21:36:38, tommycli wrote:
> Should we add a target for category_default_setting also?
> 
> Also the exceptions element no longer needs to depend on the behavior target,
> nor the webui.

Done.

https://codereview.chromium.org/2554403005/diff/80001/chrome/test/data/webui/...
File chrome/test/data/webui/settings/category_setting_exceptions_tests.js
(right):

https://codereview.chromium.org/2554403005/diff/80001/chrome/test/data/webui/...
chrome/test/data/webui/settings/category_setting_exceptions_tests.js:13: var
testElement;
On 2016/12/15 21:36:38, tommycli wrote:
> = null; I've normally seen

Is there a reason null is better than undefined here?

https://codereview.chromium.org/2554403005/diff/80001/chrome/test/data/webui/...
chrome/test/data/webui/settings/category_setting_exceptions_tests.js:33:
assertTrue(!!testElement);
On 2016/12/15 21:36:38, tommycli wrote:
> Well if there's legitimately nothing that merits testing we can just delete
this
> whole file. I agree (btw) that there's not much worth testing there.

Acknowledged.

[tommycli, 2016-12-16 20:46:15]

lgtm except:

consider removing category_setting_exceptions_tests.js, since it just creates
the element.

[commit-bot: I haz the power, 2016-12-16 21:19:10]

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

[commit-bot: I haz the power, 2016-12-16 21:19:11]

Dry run: Try jobs failed on following builders:
  closure_compilation on master.tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)

[dschuyler, 2016-12-16 22:27:48]

The CQ bit was checked by dschuyler@chromium.org

[commit-bot: I haz the power, 2016-12-16 22:28:20]

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-12-17 00:10:10]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1481927268251150,
"parent_rev": "e4f7fd4296803c7615be2707d5c4ae5e0f2a1b65", "commit_rev":
"dab0af05eea6ccf3c1db4e39d4f9f4795aedbbe8"}

[commit-bot: I haz the power, 2016-12-17 00:10:37]

Description was changed from

==========
[MD settings] move extra options in category settings

This CL refactors the site settings category code to move one-off
details from that code. This was done by moving much of the code to a
new category default setting module. The site_settings_category has been
replaced with content_setting_exceptions.

related to bug 662596
BUG=None
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
[MD settings] move extra options in category settings

This CL refactors the site settings category code to move one-off
details from that code. This was done by moving much of the code to a
new category default setting module. The site_settings_category has been
replaced with content_setting_exceptions.

related to bug 662596
BUG=None
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2554403005
==========

[commit-bot: I haz the power, 2016-12-17 00:10:39]

Committed patchset #5 (id:100001)

[commit-bot: I haz the power, 2016-12-17 00:13:59]

Description was changed from

==========
[MD settings] move extra options in category settings

This CL refactors the site settings category code to move one-off
details from that code. This was done by moving much of the code to a
new category default setting module. The site_settings_category has been
replaced with content_setting_exceptions.

related to bug 662596
BUG=None
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2554403005
==========

to

==========
[MD settings] move extra options in category settings

This CL refactors the site settings category code to move one-off
details from that code. This was done by moving much of the code to a
new category default setting module. The site_settings_category has been
replaced with content_setting_exceptions.

related to bug 662596
BUG=None
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/891846bf42988a708ce18472546e1361387507a6
Cr-Commit-Position: refs/heads/master@{#439243}
==========

[commit-bot: I haz the power, 2016-12-17 00:14:01]

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