Settings Rewrite: Make a common settings-section component.

Settings Rewrite: Make a common settings-section component.

Adds a common settings-section component for sections within the new settings.

This component displays the correct icon and header for each section.

BUG=425627
NOPRESUBMIT=true
(PRESUBMIT has a false positive here)

Committed: https://crrev.com/b4a07c2fb9ad84fb317287024df29cecf07f5390
Cr-Commit-Position: refs/heads/master@{#340159}

[tommycli, 2015-07-21 19:09:33]

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

[commit-bot: I haz the power, 2015-07-21 19:10:06]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1241363002/20001

[tommycli, 2015-07-21 19:11:48]

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

[tommycli, 2015-07-21 19:11:49]

michaelpg: ptal

This implements more of the newly approved MD settings design (by creating a new
settings-section component to hold common styles).

[commit-bot: I haz the power, 2015-07-21 20:31:48]

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

[commit-bot: I haz the power, 2015-07-21 20:31:49]

Dry run: This issue passed the CQ dry run.

[michaelpg, 2015-07-21 22:04:01]

I'd like to see a mini design doc for the new structure before doing more code
reviews. There was work still to be done on the old layout that still applies to
the new structure. Specifically it should address:

* the hierarchy between pages, i.e., which components #include which other
components
* how sections and sub-pages will be composed (what Polymer behaviors should we
define) 
* how the transitions on pages/sections will work
* how routing will move to the correct page/section/sub-page. How many
iron-pages and/or neon-animated-pages do we need?

Tommy, Dan, does this sound reasonable?

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

https://codereview.chromium.org/1241363002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/a11y_page/a11y_page.html:8:
<cr-settings-section page-title="[[pageTitle]]" icon="[[icon]]">
Distributing all of the page content to another element adds some complexity and
doesn't give us much. It would be simpler to make a <cr-settings-section-header>
element and use that here.

For the <paper-material> (or whatever we use as the container for a top-level
section), we could move that outside of the individual pages, so a page isn't
tied to one style. This would enable us to include a given page as a top-level
page (by using <paper-material>) or as a sub-page (by using... something else).
It would also mean, if we want to change the container to something else, we
don't have to edit each individual page's file.

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

https://codereview.chromium.org/1241363002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/basic_page/basic_page.html:10:
<cr-settings-internet-page route="internet">
So for example, I would put here:

<paper-material route="internet">
  <cr-settings-internet-page>
  </cr-settings-internet-page>
</paper-material>

etc. for the below. When we decide these should look different we can just
find/replace in this one file (and advanced_page.html).

The way these sections should look (the size & shadow of the paper-material, the
margins between each section) can be defined by a CSS file shared by basic-page
and advanced-page.

https://codereview.chromium.org/1241363002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/settings_page/settings_section.css
(right):

https://codereview.chromium.org/1241363002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/settings_section.css:16:
[header] {
why an attribute instead of a class/ID?

https://codereview.chromium.org/1241363002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/settings_section.css:26:
padding-right: 16px;
-webkit-padding-end

https://codereview.chromium.org/1241363002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/settings_section.css:27: }
nit: newline

https://codereview.chromium.org/1241363002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/settings_page/settings_section.html
(right):

https://codereview.chromium.org/1241363002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/settings_section.html:11: <div
class="title">[[pageTitle]]</div>
wat

[tommycli, 2015-07-22 19:06:11]

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

[tommycli, 2015-07-22 19:06:42]

michaelpg: thanks!

The inter-patch delta might be confused because i think a rebase took place for
some of these files.

Tommy

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

https://codereview.chromium.org/1241363002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/a11y_page/a11y_page.html:8:
<cr-settings-section page-title="[[pageTitle]]" icon="[[icon]]">
On 2015/07/21 22:04:01, michaelpg wrote:
> Distributing all of the page content to another element adds some complexity
and
> doesn't give us much. It would be simpler to make a
<cr-settings-section-header>
> element and use that here.
> 
> For the <paper-material> (or whatever we use as the container for a top-level
> section), we could move that outside of the individual pages, so a page isn't
> tied to one style. This would enable us to include a given page as a top-level
> page (by using <paper-material>) or as a sub-page (by using... something
else).
> It would also mean, if we want to change the container to something else, we
> don't have to edit each individual page's file.

Done.

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

https://codereview.chromium.org/1241363002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/basic_page/basic_page.html:10:
<cr-settings-internet-page route="internet">
On 2015/07/21 22:04:01, michaelpg wrote:
> So for example, I would put here:
> 
> <paper-material route="internet">
>   <cr-settings-internet-page>
>   </cr-settings-internet-page>
> </paper-material>
> 
> etc. for the below. When we decide these should look different we can just
> find/replace in this one file (and advanced_page.html).
> 
> The way these sections should look (the size & shadow of the paper-material,
the
> margins between each section) can be defined by a CSS file shared by
basic-page
> and advanced-page.

Done.

https://codereview.chromium.org/1241363002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/settings_page/settings_section.css
(right):

https://codereview.chromium.org/1241363002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/settings_section.css:16:
[header] {
On 2015/07/21 22:04:01, michaelpg wrote:
> why an attribute instead of a class/ID?

Done.

https://codereview.chromium.org/1241363002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/settings_section.css:26:
padding-right: 16px;
On 2015/07/21 22:04:01, michaelpg wrote:
> -webkit-padding-end

Why webkit-padding-end instead of padding-right?

https://codereview.chromium.org/1241363002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/settings_section.css:27: }
On 2015/07/21 22:04:01, michaelpg wrote:
> nit: newline

Done.

https://codereview.chromium.org/1241363002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/settings_page/settings_section.html
(right):

https://codereview.chromium.org/1241363002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/settings_section.html:11: <div
class="title">[[pageTitle]]</div>
On 2015/07/21 22:04:01, michaelpg wrote:
> wat

Done.

[commit-bot: I haz the power, 2015-07-22 19:07:28]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1241363002/60001

[michaelpg, 2015-07-22 21:01:32]

michaelpg@chromium.org changed reviewers:
+ dbeam@chromium.org

[michaelpg, 2015-07-22 21:01:33]

+dbeam

https://codereview.chromium.org/1241363002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/a11y_page/a11y_page.html (left):

https://codereview.chromium.org/1241363002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/a11y_page/a11y_page.html:7:
href="chrome://md-settings/settings_page/settings_page.css">
settings_page.css still has useful CSS (for cr-settings-checkbox). How does that
styling get applied now?

https://codereview.chromium.org/1241363002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/internet_page/internet_detail_page.html
(right):

https://codereview.chromium.org/1241363002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/internet_detail_page.html:15:
href="chrome://md-settings/settings_page/settings_page.css">
if this is the only change for this file, remove it from the CL

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

https://codereview.chromium.org/1241363002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/search_engines_page/search_engines_page.html:8:
href="chrome://md-settings/settings_page/settings_page.css">
same

https://codereview.chromium.org/1241363002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/settings_page/settings_section.html
(right):

https://codereview.chromium.org/1241363002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/settings_section.html:2: <link
rel="import" href="chrome://resources/polymer/v1_0/polymer/polymer.html">
nit: import polymer before polymer elements

https://codereview.chromium.org/1241363002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/settings_page/settings_section.js
(right):

https://codereview.chromium.org/1241363002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/settings_section.js:12: *   
<cr-settings-section page-title="[[pageTitle]]", icon="[[icon]]">
no comma

https://codereview.chromium.org/1241363002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/settings_section.js:26: if
(contentElement !== undefined &&
nit: use != instead of !==

https://codereview.chromium.org/1241363002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/settings_section.js:29:
this.pageTitle = contentElement.properties.pageTitle.value();
does contentElement.pageTitle not work?

https://codereview.chromium.org/1241363002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/settings_section.js:29:
this.pageTitle = contentElement.properties.pageTitle.value();
this means updating a page's pageTitle or icon has no effect. I can't say for
sure that we wouldn't want to do that. We could add our own observer I guess.

dbeam@, does this look ok to you or is it too indirect?

https://codereview.chromium.org/1241363002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/users_page/user_list.html (right):

https://codereview.chromium.org/1241363002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/users_page/user_list.html:8: <div
class="user-list soft-border">
"soft-border" is another example of a class that won't be applied without the
reference to settings_page.css in this element, unless we apply it some other
way.

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

https://codereview.chromium.org/1241363002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/users_page/users_page.html:14:
<cr-settings-section page-title="[[pageTitle]]" icon="[[icon]]">
this is from the initial patch

[commit-bot: I haz the power, 2015-07-22 21:03:02]

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

[commit-bot: I haz the power, 2015-07-22 21:03:03]

Dry run: This issue passed the CQ dry run.

[Dan Beam, 2015-07-22 21:56:31]

https://codereview.chromium.org/1241363002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/settings_page/settings_section.js
(right):

https://codereview.chromium.org/1241363002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/settings_section.js:29:
this.pageTitle = contentElement.properties.pageTitle.value();
On 2015/07/22 21:01:32, michaelpg wrote:
> does contentElement.pageTitle not work?

^ this

https://codereview.chromium.org/1241363002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/settings_section.js:29:
this.pageTitle = contentElement.properties.pageTitle.value();
On 2015/07/22 21:01:32, michaelpg wrote:
> this means updating a page's pageTitle or icon has no effect. I can't say for
> sure that we wouldn't want to do that. We could add our own observer I guess.
> 
> dbeam@, does this look ok to you or is it too indirect?

why are we duplicating the title to begin with?  can't we just have a parent
property or id?

[Dan Beam, 2015-07-22 22:03:07]

https://codereview.chromium.org/1241363002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/settings_page/settings_section.js
(right):

https://codereview.chromium.org/1241363002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/settings_section.js:29:
this.pageTitle = contentElement.properties.pageTitle.value();
On 2015/07/22 21:56:31, Dan Beam wrote:
> On 2015/07/22 21:01:32, michaelpg wrote:
> > this means updating a page's pageTitle or icon has no effect. I can't say
for
> > sure that we wouldn't want to do that. We could add our own observer I
guess.
> > 
> > dbeam@, does this look ok to you or is it too indirect?
> 
> why are we duplicating the title to begin with?  can't we just have a parent
> property or id?

scratch the parent part, thought this was a subpage

[Dan Beam, 2015-07-22 23:12:17]

https://codereview.chromium.org/1241363002/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/settings_page/settings_section.html
(right):

https://codereview.chromium.org/1241363002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/settings_page/settings_section.html:11: <div
class="title">[[pageTitle]]</div>
fix indent

https://codereview.chromium.org/1241363002/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/settings/basic_page/basic_page.html (right):

https://codereview.chromium.org/1241363002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/basic_page/basic_page.html:14:
<cr-settings-section route="internet">
why is the route here?

https://codereview.chromium.org/1241363002/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/settings/internet_page/internet_page.html (right):

https://codereview.chromium.org/1241363002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/internet_page/internet_page.html:8:
<network-summary></network-summary>
soo many little files :(

https://codereview.chromium.org/1241363002/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_page/settings_section.html
(right):

https://codereview.chromium.org/1241363002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_page/settings_section.html:11: <div
class="title">{{pageTitle}}</div>
if this makes someone happy I suppose that's OK and less duplicative, but I
think enforcing a hierarchy is brittle and results in lots of optional
parameters added later

just allowing each section to define their icon/title (the OPPOSITE of this) is
fine with me.

we can just do this until one of the mocks breaks these assumptions ;)

https://codereview.chromium.org/1241363002/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_page/settings_section.js
(right):

https://codereview.chromium.org/1241363002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_page/settings_section.js:33:
this.icon = contentElement.properties.icon.value;
why are we setting titles in JS vs just with i18n-content or i18n-values?

[michaelpg, 2015-07-22 23:26:55]

https://codereview.chromium.org/1241363002/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_page/settings_section.js
(right):

https://codereview.chromium.org/1241363002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_page/settings_section.js:33:
this.icon = contentElement.properties.icon.value;
On 2015/07/22 23:12:17, Dan Beam wrote:
> why are we setting titles in JS vs just with i18n-content or i18n-values?

basically we bound the header to the title of the selected page, when the header
was a standalone component in the old settings structure. If we only need it in
the page itself, and the menu, I guess there's no need to make it a property.

[tommycli, 2015-07-22 23:54:38]

dbeam/michaelpg:

Okay this is my latest patchset.

Here I discarded the 'smart' retrieval of the title and icon, and just
explicitly specify it in basic_page.html and advanced_page.html.

I realize this duplicates the title and icon data, but this was the cleanest
solution imo.

Tommy

https://codereview.chromium.org/1241363002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/a11y_page/a11y_page.html (left):

https://codereview.chromium.org/1241363002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/a11y_page/a11y_page.html:7:
href="chrome://md-settings/settings_page/settings_page.css">
On 2015/07/22 21:01:32, michaelpg wrote:
> settings_page.css still has useful CSS (for cr-settings-checkbox). How does
that
> styling get applied now?

Done.

https://codereview.chromium.org/1241363002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/internet_page/internet_detail_page.html
(right):

https://codereview.chromium.org/1241363002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/internet_detail_page.html:15:
href="chrome://md-settings/settings_page/settings_page.css">
On 2015/07/22 21:01:32, michaelpg wrote:
> if this is the only change for this file, remove it from the CL

Done.

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

https://codereview.chromium.org/1241363002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/search_engines_page/search_engines_page.html:8:
href="chrome://md-settings/settings_page/settings_page.css">
On 2015/07/22 21:01:32, michaelpg wrote:
> same

Done.

https://codereview.chromium.org/1241363002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/settings_page/settings_section.html
(right):

https://codereview.chromium.org/1241363002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/settings_section.html:2: <link
rel="import" href="chrome://resources/polymer/v1_0/polymer/polymer.html">
On 2015/07/22 21:01:32, michaelpg wrote:
> nit: import polymer before polymer elements

Done.

https://codereview.chromium.org/1241363002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/settings_page/settings_section.js
(right):

https://codereview.chromium.org/1241363002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/settings_section.js:12: *   
<cr-settings-section page-title="[[pageTitle]]", icon="[[icon]]">
On 2015/07/22 21:01:32, michaelpg wrote:
> no comma

Done.

https://codereview.chromium.org/1241363002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/settings_section.js:26: if
(contentElement !== undefined &&
On 2015/07/22 21:01:32, michaelpg wrote:
> nit: use != instead of !==

Deleted this.

https://codereview.chromium.org/1241363002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/settings_section.js:29:
this.pageTitle = contentElement.properties.pageTitle.value();
On 2015/07/22 21:01:32, michaelpg wrote:
> this means updating a page's pageTitle or icon has no effect. I can't say for
> sure that we wouldn't want to do that. We could add our own observer I guess.
> 
> dbeam@, does this look ok to you or is it too indirect?

Deleted this.

https://codereview.chromium.org/1241363002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/settings_section.js:29:
this.pageTitle = contentElement.properties.pageTitle.value();
On 2015/07/22 21:01:32, michaelpg wrote:
> does contentElement.pageTitle not work?

Deleted this.

https://codereview.chromium.org/1241363002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/users_page/user_list.html (right):

https://codereview.chromium.org/1241363002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/users_page/user_list.html:8: <div
class="user-list soft-border">
On 2015/07/22 21:01:33, michaelpg wrote:
> "soft-border" is another example of a class that won't be applied without the
> reference to settings_page.css in this element, unless we apply it some other
> way.

Done.

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

https://codereview.chromium.org/1241363002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/users_page/users_page.html:14:
<cr-settings-section page-title="[[pageTitle]]" icon="[[icon]]">
On 2015/07/22 21:01:33, michaelpg wrote:
> this is from the initial patch

Done.

https://codereview.chromium.org/1241363002/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/settings/basic_page/basic_page.html (right):

https://codereview.chromium.org/1241363002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/basic_page/basic_page.html:14:
<cr-settings-section route="internet">
On 2015/07/22 23:12:17, Dan Beam wrote:
> why is the route here?

Per our discussion, I think we should just keep it here.

https://codereview.chromium.org/1241363002/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/settings/internet_page/internet_page.html (right):

https://codereview.chromium.org/1241363002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/internet_page/internet_page.html:8:
<network-summary></network-summary>
On 2015/07/22 23:12:17, Dan Beam wrote:
> soo many little files :(

Will inline in separate CL

https://codereview.chromium.org/1241363002/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_page/settings_section.html
(right):

https://codereview.chromium.org/1241363002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_page/settings_section.html:11: <div
class="title">{{pageTitle}}</div>
On 2015/07/22 23:12:17, Dan Beam wrote:
> if this makes someone happy I suppose that's OK and less duplicative, but I
> think enforcing a hierarchy is brittle and results in lots of optional
> parameters added later
> 
> just allowing each section to define their icon/title (the OPPOSITE of this)
is
> fine with me.
> 
> we can just do this until one of the mocks breaks these assumptions ;)

Acknowledged.

[Dan Beam, 2015-07-23 03:50:51]

added BUG=425627 to CL desc.

"why do you hate me?" - BUG= lines

[tommycli, 2015-07-23 16:51:35]

On 2015/07/23 03:50:51, Dan Beam wrote:
> added BUG=425627 to CL desc.
> 
> "why do you hate me?" - BUG= lines

dbeam: thanks heh, sry

[michaelpg, 2015-07-23 18:21:17]

lgtm

https://codereview.chromium.org/1241363002/diff/150001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_page/settings_section.css
(right):

https://codereview.chromium.org/1241363002/diff/150001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_page/settings_section.css:37:
.soft-border {
unused (used in settings_page.css)

https://codereview.chromium.org/1241363002/diff/150001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_page/settings_section.css:42:
.page-content {
unused (used in settings_page.css)

[Dan Beam, 2015-07-23 18:33:09]

lgtm

https://codereview.chromium.org/1241363002/diff/150001/chrome/browser/resourc...
File chrome/browser/resources/settings/advanced_page/advanced_page.html (right):

https://codereview.chromium.org/1241363002/diff/150001/chrome/browser/resourc...
chrome/browser/resources/settings/advanced_page/advanced_page.html:16:
i18n-values="page-title:dateTimePageTitle">
why not pageTitle instead of page-title?

https://codereview.chromium.org/1241363002/diff/150001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_page/settings_section.css
(right):

https://codereview.chromium.org/1241363002/diff/150001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_page/settings_section.css:26:
padding-right: 16px;
-webkit-padding-end

https://codereview.chromium.org/1241363002/diff/150001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_page/settings_section.js
(right):

https://codereview.chromium.org/1241363002/diff/150001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_page/settings_section.js:26:
pageTitle: {
can't these be simplified to:

  pageTitle: String,

  icon: String,

?

[tommycli, 2015-07-23 19:00:43]

michaelpg, dbeam: thanks guys!

https://codereview.chromium.org/1241363002/diff/150001/chrome/browser/resourc...
File chrome/browser/resources/settings/advanced_page/advanced_page.html (right):

https://codereview.chromium.org/1241363002/diff/150001/chrome/browser/resourc...
chrome/browser/resources/settings/advanced_page/advanced_page.html:16:
i18n-values="page-title:dateTimePageTitle">
On 2015/07/23 18:33:08, Dan Beam wrote:
> why not pageTitle instead of page-title?

Hey. 'pageTitle' doesn't work. I believe that Polymer expects the attribute
parameters to be 'hyphen-space' and maps that to 'camelCase' attributes.

https://codereview.chromium.org/1241363002/diff/150001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_page/settings_section.css
(right):

https://codereview.chromium.org/1241363002/diff/150001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_page/settings_section.css:26:
padding-right: 16px;
On 2015/07/23 18:33:08, Dan Beam wrote:
> -webkit-padding-end

Done.

https://codereview.chromium.org/1241363002/diff/150001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_page/settings_section.css:37:
.soft-border {
On 2015/07/23 18:21:17, michaelpg wrote:
> unused (used in settings_page.css)

Done.

https://codereview.chromium.org/1241363002/diff/150001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_page/settings_section.css:42:
.page-content {
On 2015/07/23 18:21:17, michaelpg wrote:
> unused (used in settings_page.css)

Done.

https://codereview.chromium.org/1241363002/diff/150001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_page/settings_section.js
(right):

https://codereview.chromium.org/1241363002/diff/150001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_page/settings_section.js:26:
pageTitle: {
On 2015/07/23 18:33:08, Dan Beam wrote:
> can't these be simplified to:
> 
>   pageTitle: String,
> 
>   icon: String,
> 
> ?

Done. Good call. it worked

[tommycli, 2015-07-23 19:00:52]

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

[commit-bot: I haz the power, 2015-07-23 19:01:37]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1241363002/170001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1241363002/170001

[commit-bot: I haz the power, 2015-07-23 19:14:13]

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

[commit-bot: I haz the power, 2015-07-23 19:14:14]

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

[tommycli, 2015-07-23 20:05:53]

The CQ bit was checked by tommycli@chromium.org

[tommycli, 2015-07-23 20:05:54]

The patchset sent to the CQ was uploaded after l-g-t-m from dbeam@chromium.org,
michaelpg@chromium.org
Link to the patchset: https://codereview.chromium.org/1241363002/#ps170001
(title: " ")

[commit-bot: I haz the power, 2015-07-23 20:06:11]

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

[commit-bot: I haz the power, 2015-07-23 20:44:55]

Committed patchset #10 (id:170001)

[commit-bot: I haz the power, 2015-07-23 20:45:53]

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