Polymer Settings Scaffold in MD-Settings

Polymer Settings Scaffold in MD-Settings

This adds resources to Chrome and exposes cr-settings from
chrome://md-settings.

Follow-up to https://codereview.chromium.org/981203007

Screenshot for this CL plus the next CL (for a11y-page and downloads-page): http://i.imgur.com/pZBLW4I.png

BUG=464979
Committed: https://crrev.com/2c2fd2c4bc46d2b42d5897037eab7b0c50a9ed3b
Cr-Commit-Position: refs/heads/master@{#321005}

[michaelpg, 2015-03-13 22:59:24]

Patchset #1 (id:1) has been deleted

[michaelpg, 2015-03-13 23:29:53]

Patchset #2 (id:40001) has been deleted

[michaelpg, 2015-03-13 23:34:05]

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

[michaelpg, 2015-03-13 23:34:06]

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

https://codereview.chromium.org/1007803003/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/dummy_page/dummy_page.html:1: <link
rel="import" href="chrome://resources/polymer/polymer/polymer.html">
This is just moved from md_settings.html. It's only a placeholder.

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

https://codereview.chromium.org/1007803003/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/settings_main/settings_main.html:14:
<cr-settings-a11y-page prefs="{{prefs}}"></cr-settings-a11y-page>
Removing a11y-page. Next CL will add it back but that will involve changes to
a11y-page and downloads-page, so I've kept them out of this CL.

[michaelpg, 2015-03-13 23:35:04]

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

[Jeremy Klein, 2015-03-13 23:51:51]

jlklein@chromium.org changed reviewers:
+ jlklein@chromium.org

[Jeremy Klein, 2015-03-13 23:51:52]

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

https://codereview.chromium.org/1007803003/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/settings.js:17: });
nit: Just collapse this onto one line: Polymer('cr-settings', {});

https://codereview.chromium.org/1007803003/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/settings_page/settings_page.css (right):

https://codereview.chromium.org/1007803003/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/settings_page.css:5: :host {
Can we include a fileoverview type comment here to indicate that these are
styles shared by all settings pages?

https://codereview.chromium.org/1007803003/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/settings_page.css:13: :host
/deep/ cr-settings-page-header {
Shouldn't this just be in the header css?

https://codereview.chromium.org/1007803003/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/settings_page/settings_page_header.css
(right):

https://codereview.chromium.org/1007803003/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/settings_page_header.css:1: /*
Copyright 2015 The Chromium Authors. All rights reserved.
I still think these should be in c/b/r/settings/page_header, but I won't block
this cl on it.

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

https://codereview.chromium.org/1007803003/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/settings_page_header.html:6:
<div id="icon-container">
nit: this should be a class.

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

https://codereview.chromium.org/1007803003/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/settings_resources.grd:24: <structure
name="IDR_SETTINGS_CR_SETTINGS_DRAWER_CSS"
*sigh*... we should really write that generator script for this stuff.

[Oren Blasberg, 2015-03-14 00:03:29]

orenb@chromium.org changed reviewers:
+ orenb@chromium.org

[Oren Blasberg, 2015-03-14 00:03:30]

lgtm

nice!

[michaelpg, 2015-03-17 06:30:16]

Sorry this took so long, just a few changes.

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

https://codereview.chromium.org/1007803003/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/settings.js:17: });
On 2015/03/13 23:51:51, Jeremy Klein wrote:
> nit: Just collapse this onto one line: Polymer('cr-settings', {});

Done.

https://codereview.chromium.org/1007803003/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/settings_page/settings_page.css (right):

https://codereview.chromium.org/1007803003/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/settings_page.css:5: :host {
On 2015/03/13 23:51:51, Jeremy Klein wrote:
> Can we include a fileoverview type comment here to indicate that these are
> styles shared by all settings pages?

Done.

https://codereview.chromium.org/1007803003/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/settings_page.css:13: :host
/deep/ cr-settings-page-header {
On 2015/03/13 23:51:51, Jeremy Klein wrote:
> Shouldn't this just be in the header css?

I think it belongs here as it's specifying constraints for the header. The
/deep/ is unnecessary though.

https://codereview.chromium.org/1007803003/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/settings_page/settings_page_header.css
(right):

https://codereview.chromium.org/1007803003/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/settings_page_header.css:1: /*
Copyright 2015 The Chromium Authors. All rights reserved.
On 2015/03/13 23:51:51, Jeremy Klein wrote:
> I still think these should be in c/b/r/settings/page_header, but I won't block
> this cl on it.

I'm a little wary of "c/b/r/settings/main" and "c/b/r/settings/menu" because for
instance, "cr-settings-main" makes sense for a component IMO but it's not a very
helpful directory name.

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

https://codereview.chromium.org/1007803003/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/settings_page_header.html:6:
<div id="icon-container">
On 2015/03/13 23:51:51, Jeremy Klein wrote:
> nit: this should be a class.

I'm not sure what you mean. Do you mean we should wrap core-icon in cr-icon? I
don't know if that's necessary as it's a pretty basic building block.

[Jeremy Klein, 2015-03-17 06:38:00]

lgtm

https://codereview.chromium.org/1007803003/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/settings_page/settings_page.css (right):

https://codereview.chromium.org/1007803003/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/settings_page.css:13: :host
/deep/ cr-settings-page-header {
On 2015/03/17 06:30:15, michaelpg wrote:
> On 2015/03/13 23:51:51, Jeremy Klein wrote:
> > Shouldn't this just be in the header css?
> 
> I think it belongs here as it's specifying constraints for the header. The
> /deep/ is unnecessary though.

I suppose that works for me. Getting rid of the /deep/ is a +.

https://codereview.chromium.org/1007803003/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/settings_page/settings_page_header.css
(right):

https://codereview.chromium.org/1007803003/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/settings_page_header.css:1: /*
Copyright 2015 The Chromium Authors. All rights reserved.
On 2015/03/17 06:30:15, michaelpg wrote:
> On 2015/03/13 23:51:51, Jeremy Klein wrote:
> > I still think these should be in c/b/r/settings/page_header, but I won't
block
> > this cl on it.
> 
> I'm a little wary of "c/b/r/settings/main" and "c/b/r/settings/menu" because
for
> instance, "cr-settings-main" makes sense for a component IMO but it's not a
very
> helpful directory name.

Yeah I see what you mean. I am concerned about where we draw the line though.
Consistency is beginning to be a problem here... Let's chat at the meeting
tomorrow about this.

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

https://codereview.chromium.org/1007803003/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/settings_page_header.html:6:
<div id="icon-container">
On 2015/03/17 06:30:15, michaelpg wrote:
> On 2015/03/13 23:51:51, Jeremy Klein wrote:
> > nit: this should be a class.
> 
> I'm not sure what you mean. Do you mean we should wrap core-icon in cr-icon? I
> don't know if that's necessary as it's a pretty basic building block.

No, sorry I mean the id should be a class instead here. If it needs to be an id
for convenient js access (looks like it isn't used that way at the moment), it
should be camel-case instead of dashes.

[michaelpg, 2015-03-17 21:52:05]

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

https://codereview.chromium.org/1007803003/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/settings_page_header.html:6:
<div id="icon-container">
On 2015/03/17 06:38:00, Jeremy Klein wrote:
> On 2015/03/17 06:30:15, michaelpg wrote:
> > On 2015/03/13 23:51:51, Jeremy Klein wrote:
> > > nit: this should be a class.
> > 
> > I'm not sure what you mean. Do you mean we should wrap core-icon in cr-icon?
I
> > don't know if that's necessary as it's a pretty basic building block.
> 
> No, sorry I mean the id should be a class instead here. If it needs to be an
id
> for convenient js access (looks like it isn't used that way at the moment), it
> should be camel-case instead of dashes.

Oh, haha. Yeah, it doesn't have to be an ID and it makes more sense as a class.

[michaelpg, 2015-03-17 21:52:09]

The CQ bit was checked by michaelpg@chromium.org

[michaelpg, 2015-03-17 21:52:09]

The patchset sent to the CQ was uploaded after l-g-t-m from orenb@chromium.org,
jlklein@chromium.org
Link to the patchset: https://codereview.chromium.org/1007803003/#ps100001
(title: " ")

[commit-bot: I haz the power, 2015-03-17 21:53:50]

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

[commit-bot: I haz the power, 2015-03-17 21:53:53]

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

[commit-bot: I haz the power, 2015-03-17 21:53:56]

No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM may have been provided, it was from a non-committer,
_not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[Kyle Horimoto, 2015-03-17 21:59:04]

khorimoto@chromium.org changed reviewers:
+ khorimoto@chromium.org

[Kyle Horimoto, 2015-03-17 21:59:04]

lgtm

[michaelpg, 2015-03-17 21:59:15]

The CQ bit was checked by michaelpg@chromium.org

[commit-bot: I haz the power, 2015-03-17 22:00:10]

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

[commit-bot: I haz the power, 2015-03-17 23:00:20]

Committed patchset #4 (id:100001)

[commit-bot: I haz the power, 2015-03-17 23:01:18]

Patchset 4 (id:??) landed as
https://crrev.com/2c2fd2c4bc46d2b42d5897037eab7b0c50a9ed3b
Cr-Commit-Position: refs/heads/master@{#321005}