Site Settings: Give each category its own route.

Site Settings: Give each category its own route.

BUG=543635
Committed: https://crrev.com/d4d06d3ea549c1f56b9c8cf6ec627c7a9c99c6de
Cr-Commit-Position: refs/heads/master@{#368315}

[Finnur, 2015-12-23 17:15:56]

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

[Finnur, 2015-12-23 17:15:56]

This somewhere along the lines of what you had in mind?

[Finnur, 2016-01-04 13:09:35]

Happy new year! :)
PTAL.

[tommycli, 2016-01-04 18:12:09]

finnur: Happy new year to you also. Here you go!

https://codereview.chromium.org/1546993003/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/settings_page/settings_router.js (right):

https://codereview.chromium.org/1546993003/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/settings_router.js:162: subpage:
['site-settings', 'site-settings-category'],
final element is site-settings-cookies right?

and same below

https://codereview.chromium.org/1546993003/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/settings_router.js:163:
subpageTitles: ['siteSettings', 'siteSettingsCategoryPageTitle'],
final element is siteSettingsCookiesPageTitle right?

and same below

https://codereview.chromium.org/1546993003/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/site_settings/site_settings_behavior.js
(right):

https://codereview.chromium.org/1546993003/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_settings_behavior.js:198:
computeCategoryFromDisplayName: function(displayName) {
Is this really the display name? I thought the display name was localized,
whereas this looks more like the final url segment

https://codereview.chromium.org/1546993003/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/site_settings/site_settings_category.js
(right):

https://codereview.chromium.org/1546993003/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_settings_category.js:72:
var category = this.computeCategoryFromDisplayName(displayName);
same comment as before regarding display name

https://codereview.chromium.org/1546993003/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_settings_category.js:74:
this.category = category;
setting this.category using the final part of the URL seems fragile and
unpolymeric.

It seems like it would be better if it was passed in as a attribute, i.e.

<site-settings-category prefs="{{prefs}}" category="cookies">
</site-settings-category>

[Finnur, 2016-01-05 15:54:07]

No code update, just a couple of clarification requests...

https://codereview.chromium.org/1546993003/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/settings_page/settings_router.js (right):

https://codereview.chromium.org/1546993003/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/settings_router.js:162: subpage:
['site-settings', 'site-settings-category'],
I'm not sure how to proceed here...

These sub-page strings equal Polymer element ids, right?

As such, there's only a site-settings-category element (which knows how to
display any category). Therefore there's no need for a specific
site-settings-cookies element.

I mean, I guess we could create one for each category and embed a
site-settings-category object within, or something... is that what you are
suggesting?

https://codereview.chromium.org/1546993003/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/settings_router.js:163:
subpageTitles: ['siteSettings', 'siteSettingsCategoryPageTitle'],
Yes, if we can resolve the above.

https://codereview.chromium.org/1546993003/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/site_settings/site_settings_category.js
(right):

https://codereview.chromium.org/1546993003/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_settings_category.js:72:
var category = this.computeCategoryFromDisplayName(displayName);
You prefer that to be named computeCategoryFromUrlPart?

https://codereview.chromium.org/1546993003/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_settings_category.js:74:
this.category = category;
I am already doing that (setting the category), through the privacy_page.html.
It binds |category| with its own |selectedCategory| variable, which is set in
site_settings_page.js.

This code is to handle the case where you refresh the page when displaying a
certain category. At that point, all we have is the URL and the |category|
variable is undefined. In other words: This code links up the pieces as they
were before refreshing.

[tommycli, 2016-01-05 18:28:58]

Hello finnur, hope below helps. Thanks Tommy

https://codereview.chromium.org/1546993003/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/settings_page/settings_router.js (right):

https://codereview.chromium.org/1546993003/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/settings_router.js:145: url:
'/siteSettings/category',
This route is no longer needed right?

https://codereview.chromium.org/1546993003/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/settings_router.js:162: subpage:
['site-settings', 'site-settings-category'],
On 2016/01/05 15:54:07, Finnur wrote:
> I'm not sure how to proceed here...
> 
> These sub-page strings equal Polymer element ids, right?
> 
> As such, there's only a site-settings-category element (which knows how to
> display any category). Therefore there's no need for a specific
> site-settings-cookies element.
> 
> I mean, I guess we could create one for each category and embed a
> site-settings-category object within, or something... is that what you are
> suggesting?

Hello,

No, the subpage strings here do not equal Polymer element ids.

They equal the ids of neon-animatable elements within settings-animated-pages.

See
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res...

I'm suggesting you do something within privacy_page.html:

<settings-animated-pages id="pages" current-route="{{currentRoute}}"
section="privacy">
  ...
  <neon-animatable id="site-settings-cookies">
    <site-settings-category category="cookies"></site-settings-category>
  </neon-animatable>
  <neon-animatable id="site-settings-fullscreen">
    <site-settings-category category="fullscreen"></site-settings-category>
  </neon-animatable>

  ...

This is necessary, I think, since you will want to support animated transitions
between subpages - right?

https://codereview.chromium.org/1546993003/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/site_settings/site_settings_category.js
(right):

https://codereview.chromium.org/1546993003/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_settings_category.js:72:
var category = this.computeCategoryFromDisplayName(displayName);
On 2016/01/05 15:54:07, Finnur wrote:
> You prefer that to be named computeCategoryFromUrlPart?

Actually I think this code is probably just unneeded per our discussion within
settings_router.js

https://codereview.chromium.org/1546993003/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_settings_category.js:74:
this.category = category;
On 2016/01/05 15:54:07, Finnur wrote:
> I am already doing that (setting the category), through the privacy_page.html.
> It binds |category| with its own |selectedCategory| variable, which is set in
> site_settings_page.js.
> 
> This code is to handle the case where you refresh the page when displaying a
> certain category. At that point, all we have is the URL and the |category|
> variable is undefined. In other words: This code links up the pieces as they
> were before refreshing.

Oh I see.

If you define the routes as suggested in settings_router.html, the refresh part
should be taken care of automatically (without needing this code)

[Finnur, 2016-01-06 14:53:47]

https://codereview.chromium.org/1546993003/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/settings_page/settings_router.js (right):

https://codereview.chromium.org/1546993003/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/settings_router.js:145: url:
'/siteSettings/category',
Correct.

https://codereview.chromium.org/1546993003/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/settings_router.js:162: subpage:
['site-settings', 'site-settings-category'],
Convinced.

However, I've not had luck now nor in the past with setting the |category| value
via HTML like you suggest (Dan mentioned talking to the Polymer team about that,
because it seems like a common need). I therefore still need to set it
dynamically in site_settings_page.js. 

That means the |category| value will get lost on page refresh and needs to be
set explicitly via the URL (requiring the computeCategoryFromDisplayName still,
which I've renamed to clarify a bit, hopefully).

Would like to remove it, but don't see how at the moment.

https://codereview.chromium.org/1546993003/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/settings_router.js:163:
subpageTitles: ['siteSettings', 'siteSettingsCategoryPageTitle'],
Done.

https://codereview.chromium.org/1546993003/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/site_settings/site_settings_behavior.js
(right):

https://codereview.chromium.org/1546993003/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_settings_behavior.js:73: *
A utility function to lookup a category name from its enum.
It is easier to review this file if you don't look at what changed since last
uploaded patch but the diff from trunk, as in:
https://codereview.chromium.org/1546993003/diff/40001/chrome/browser/resource...

[tommycli, 2016-01-06 19:10:34]

finnur: I hope the below comments help. This is a bit complicated, so hopefully
we can untangle this.

Tommy

https://codereview.chromium.org/1546993003/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/settings_page/settings_router.js (right):

https://codereview.chromium.org/1546993003/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/settings_router.js:162: subpage:
['site-settings', 'site-settings-category'],
On 2016/01/06 14:53:46, Finnur wrote:
> Convinced.
> 
> However, I've not had luck now nor in the past with setting the |category|
value
> via HTML like you suggest (Dan mentioned talking to the Polymer team about
that,
> because it seems like a common need). I therefore still need to set it
> dynamically in site_settings_page.js. 
> 
> That means the |category| value will get lost on page refresh and needs to be
> set explicitly via the URL (requiring the computeCategoryFromDisplayName
still,
> which I've renamed to clarify a bit, hopefully).
> 
> Would like to remove it, but don't see how at the moment.

You can only define this.category from HTML if 'category' is a polymer property.
i.e., you need to add something like

  category: {
    type: String,
  }

under 'properties' in site_settings_category.js.

settings_section.js does something very similar:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res...

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

https://codereview.chromium.org/1546993003/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/privacy_page/privacy_page.html:102:
prefs="{{prefs}}" category-selected="{{categorySelected}}">
I also noticed that you have this 'categorySelected' property. Once we get the
URL routing done correctly, I think the selected category can just be passed via
the URL/route, and we can eliminate this property.

https://codereview.chromium.org/1546993003/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/privacy_page/privacy_page.html:111:
current-route="{{currentRoute}}" category="{{categorySelected}}">
Here you are passing in the categorySelected variable, which is a number.

Instead, i think you should be passing in a string constant, i.e.
category="camera" and same below.

[Finnur, 2016-01-07 13:14:49]

All addressed, I believe. PTAL.

https://codereview.chromium.org/1546993003/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/settings_page/settings_router.js (right):

https://codereview.chromium.org/1546993003/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/settings_router.js:162: subpage:
['site-settings', 'site-settings-category'],
|category| is already defined; it used to reside in this file, but Steven moved
it into site_settings_behavior, which this derives from.

However, I made another attempt and was successful this time. I suspect either
selectedCategory or the url parsing was messing with my last attempt. After I
removed both of those and tried again it worked, with minor modifications.

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

https://codereview.chromium.org/1546993003/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/privacy_page/privacy_page.html:102:
prefs="{{prefs}}" category-selected="{{categorySelected}}">
Eliminated.

https://codereview.chromium.org/1546993003/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/privacy_page/privacy_page.html:111:
current-route="{{currentRoute}}" category="{{categorySelected}}">
Numbers also work and I think are more appropriate here than a string -- that
anyway needs to be converted into a number before being used. |category| is also
defined as a number.

[tommycli, 2016-01-07 19:30:37]

lgtm sans below comments:

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

https://codereview.chromium.org/1546993003/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/privacy_page/privacy_page.html:111:
current-route="{{currentRoute}}" category="13">
I'm not thrilled with these numeric constants being in here, especially since
they are a repeat of ContentSettingsType.

I think you can make it look like {{ContentSettingsType.CAMERA}}, etc. I asked
how on the group chat, and here was the response I got:

--

Does anyone know if it's possible to get a numeric enum value out of JS into a
polymer binding?
i.e. {{settings.ContentSettingsType.CAMERA}}?
7 mins

Demetrios Papadopoulos
Never tried this, not sure if static variables are in the scope the HTML
bindings can see. But you could always pull it out into a property on the
Polymer object.

Michael Giuffrida 
only if settings is in the bound model, ie this.ContentSettingsType =
settings.ContentSettingsType

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

https://codereview.chromium.org/1546993003/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_settings_behavior.js:109:
computeCategoryFromTextId: function(textId) {
I think this is no longer used.

[tommycli, 2016-01-07 19:31:31]

On 2016/01/07 19:30:37, tommycli wrote:
> lgtm sans below comments:
> 
>
https://codereview.chromium.org/1546993003/diff/80001/chrome/browser/resource...
> File chrome/browser/resources/settings/privacy_page/privacy_page.html (right):
> 
>
https://codereview.chromium.org/1546993003/diff/80001/chrome/browser/resource...
> chrome/browser/resources/settings/privacy_page/privacy_page.html:111:
> current-route="{{currentRoute}}" category="13">
> I'm not thrilled with these numeric constants being in here, especially since
> they are a repeat of ContentSettingsType.
> 
> I think you can make it look like {{ContentSettingsType.CAMERA}}, etc. I asked
> how on the group chat, and here was the response I got:
> 
> --
> 
> Does anyone know if it's possible to get a numeric enum value out of JS into a
> polymer binding?
> i.e. {{settings.ContentSettingsType.CAMERA}}?
> 7 mins
> 
> Demetrios Papadopoulos
> Never tried this, not sure if static variables are in the scope the HTML
> bindings can see. But you could always pull it out into a property on the
> Polymer object.
> 
> Michael Giuffrida 
> only if settings is in the bound model, ie this.ContentSettingsType =
> settings.ContentSettingsType
> 
>
https://codereview.chromium.org/1546993003/diff/80001/chrome/browser/resource...
> File chrome/browser/resources/settings/site_settings/site_settings_behavior.js
> (right):
> 
>
https://codereview.chromium.org/1546993003/diff/80001/chrome/browser/resource...
> chrome/browser/resources/settings/site_settings/site_settings_behavior.js:109:
> computeCategoryFromTextId: function(textId) {
> I think this is no longer used.

Micheal also provided the following:

http://jsfiddle.net/cmma6mc9/

[Finnur, 2016-01-08 10:36:15]

Good suggestions. All addressed. Checking in.

Thanks for the review!

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

https://codereview.chromium.org/1546993003/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_settings_behavior.js:109:
computeCategoryFromTextId: function(textId) {
Correct. Removed.

[Finnur, 2016-01-08 10:36:21]

The CQ bit was checked by finnur@chromium.org

[Finnur, 2016-01-08 10:36:21]

The patchset sent to the CQ was uploaded after l-g-t-m from
tommycli@chromium.org
Link to the patchset: https://codereview.chromium.org/1546993003/#ps100001
(title: "Address feedback")

[commit-bot: I haz the power, 2016-01-08 10:36:38]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1546993003/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1546993003/100001

[commit-bot: I haz the power, 2016-01-08 12:08:32]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-01-08 12:09:25]

Description was changed from

==========
Site Settings: Give each category its own route.

BUG=543635
==========

to

==========
Site Settings: Give each category its own route.

BUG=543635

Committed: https://crrev.com/d4d06d3ea549c1f56b9c8cf6ec627c7a9c99c6de
Cr-Commit-Position: refs/heads/master@{#368315}
==========

[commit-bot: I haz the power, 2016-01-08 12:09:25]

Patchset 6 (id:??) landed as
https://crrev.com/d4d06d3ea549c1f56b9c8cf6ec627c7a9c99c6de
Cr-Commit-Position: refs/heads/master@{#368315}

[tommycli, 2016-01-08 17:57:33]

lgtm again, looking really good