Convert the downloads page to Polymer 0.8

Convert the downloads page to Polymer 0.8

BUG=485381
Committed: https://crrev.com/89b448592636ebedd970a604bbd8b4d3abd151b1
Cr-Commit-Position: refs/heads/master@{#329554}

[Jeremy Klein, 2015-05-12 04:21:03]

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

[michaelpg, 2015-05-12 05:52:14]

https://codereview.chromium.org/1135013005/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/downloads_page/downloads_page.html
(right):

https://codereview.chromium.org/1135013005/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/downloads_page/downloads_page.html:16: <span
class="location-label" i18n-content="downloadsLocationLabel"></span>
Why not make this a good old <label for=""> since we're not using core-label?

https://codereview.chromium.org/1135013005/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/downloads_page/downloads_page.js (right):

https://codereview.chromium.org/1135013005/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/downloads_page/downloads_page.js:37: route: {
I'm probably just out of the loop, but what's the rationale for not using the
shorthand (route: String)?

https://codereview.chromium.org/1135013005/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/downloads_page/downloads_page.js:69: * Name of
the 'core-icon' to show.
iron-icon

[Jeremy Klein, 2015-05-12 06:19:27]

https://codereview.chromium.org/1135013005/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/downloads_page/downloads_page.html
(right):

https://codereview.chromium.org/1135013005/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/downloads_page/downloads_page.html:16: <span
class="location-label" i18n-content="downloadsLocationLabel"></span>
On 2015/05/12 05:52:14, michaelpg wrote:
> Why not make this a good old <label for=""> since we're not using core-label?

Good point :-). Done.

https://codereview.chromium.org/1135013005/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/downloads_page/downloads_page.js (right):

https://codereview.chromium.org/1135013005/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/downloads_page/downloads_page.js:37: route: {
On 2015/05/12 05:52:14, michaelpg wrote:
> I'm probably just out of the loop, but what's the rationale for not using the
> shorthand (route: String)?

In this case, no reason. Done.

FWIW there is a subtle difference here if the property is reflected to an
attribute. With no "value: ''", the attribute isn't added at all by default, but
with it, you get route=''.

https://codereview.chromium.org/1135013005/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/downloads_page/downloads_page.js:69: * Name of
the 'core-icon' to show.
On 2015/05/12 05:52:14, michaelpg wrote:
> iron-icon

Done.

[michaelpg, 2015-05-12 06:50:19]

https://codereview.chromium.org/1135013005/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/downloads_page/downloads_page.js (right):

https://codereview.chromium.org/1135013005/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/downloads_page/downloads_page.js:37: route: {
On 2015/05/12 06:19:27, Jeremy Klein wrote:
> On 2015/05/12 05:52:14, michaelpg wrote:
> > I'm probably just out of the loop, but what's the rationale for not using
the
> > shorthand (route: String)?
> 
> In this case, no reason. Done.
> 
> FWIW there is a subtle difference here if the property is reflected to an
> attribute. With no "value: ''", the attribute isn't added at all by default,
but
> with it, you get route=''.

I don't see this behavior -- either way, for me, the attribute is not present
(getAttribute('route') === null). The difference I do see is that with "value:
''", the instance of the element will have a property "route" set to ''
(otherwise, no such property is defined on the instance).

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

https://codereview.chromium.org/1135013005/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/downloads_page/downloads_page.html:16: <label
class="location-label" i18n-content="downloadsLocationLabel">
I think i18n-content will result in the HTML content of this element being
replaced. In which case I would move the i18n-content to a <span> inside the
label, or set for="downloadsPath" on the label and keep the <cr-input> adjacent
to the label.

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

https://codereview.chromium.org/1135013005/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/downloads_page/downloads_page.js:42: subpage:
{
same as above?

[Jeremy Klein, 2015-05-12 07:01:06]

On 2015/05/12 06:50:19, michaelpg wrote:
>
https://codereview.chromium.org/1135013005/diff/1/chrome/browser/resources/se...
> File chrome/browser/resources/settings/downloads_page/downloads_page.js
(right):
> 
>
https://codereview.chromium.org/1135013005/diff/1/chrome/browser/resources/se...
> chrome/browser/resources/settings/downloads_page/downloads_page.js:37: route:
{
> On 2015/05/12 06:19:27, Jeremy Klein wrote:
> > On 2015/05/12 05:52:14, michaelpg wrote:
> > > I'm probably just out of the loop, but what's the rationale for not using
> the
> > > shorthand (route: String)?
> > 
> > In this case, no reason. Done.
> > 
> > FWIW there is a subtle difference here if the property is reflected to an
> > attribute. With no "value: ''", the attribute isn't added at all by default,
> but
> > with it, you get route=''.
> 
> I don't see this behavior -- either way, for me, the attribute is not present
> (getAttribute('route') === null). The difference I do see is that with "value:
> ''", the instance of the element will have a property "route" set to ''
> (otherwise, no such property is defined on the instance).

Weird, even when reflectToAttribute is true? Someone on the Polymer team gave me
this advice, but maybe I misunderstood...

> 
>
https://codereview.chromium.org/1135013005/diff/60001/chrome/browser/resource...
> File chrome/browser/resources/settings/downloads_page/downloads_page.html
> (right):
> 
>
https://codereview.chromium.org/1135013005/diff/60001/chrome/browser/resource...
> chrome/browser/resources/settings/downloads_page/downloads_page.html:16:
<label
> class="location-label" i18n-content="downloadsLocationLabel">
> I think i18n-content will result in the HTML content of this element being
> replaced. In which case I would move the i18n-content to a <span> inside the
> label, or set for="downloadsPath" on the label and keep the <cr-input>
adjacent
> to the label.
> 
>
https://codereview.chromium.org/1135013005/diff/60001/chrome/browser/resource...
> File chrome/browser/resources/settings/downloads_page/downloads_page.js
(right):
> 
>
https://codereview.chromium.org/1135013005/diff/60001/chrome/browser/resource...
> chrome/browser/resources/settings/downloads_page/downloads_page.js:42:
subpage:
> {
> same as above?

[Jeremy Klein, 2015-05-12 07:01:15]

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

https://codereview.chromium.org/1135013005/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/downloads_page/downloads_page.html:16: <label
class="location-label" i18n-content="downloadsLocationLabel">
On 2015/05/12 06:50:19, michaelpg wrote:
> I think i18n-content will result in the HTML content of this element being
> replaced. In which case I would move the i18n-content to a <span> inside the
> label, or set for="downloadsPath" on the label and keep the <cr-input>
adjacent
> to the label.

Grr, good point... Can't use for because it's a presubmit error. Using
aria-labelledby I guess.

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

https://codereview.chromium.org/1135013005/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/downloads_page/downloads_page.js:42: subpage:
{
On 2015/05/12 06:50:19, michaelpg wrote:
> same as above?

I kind of like the idea of being explicit with this one, but don't feel too
strongly about it. Changed.

[michaelpg, 2015-05-12 07:27:47]

Please try testing the page ;-)

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

https://codereview.chromium.org/1135013005/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/downloads_page/downloads_page.html:16: <label
class="location-label" i18n-content="downloadsLocationLabel">
On 2015/05/12 07:01:15, Jeremy Klein wrote:
> On 2015/05/12 06:50:19, michaelpg wrote:
> > I think i18n-content will result in the HTML content of this element being
> > replaced. In which case I would move the i18n-content to a <span> inside the
> > label, or set for="downloadsPath" on the label and keep the <cr-input>
> adjacent
> > to the label.
> 
> Grr, good point... Can't use for because it's a presubmit error. Using
> aria-labelledby I guess.

Weird, you're right. I wonder why it matters.

In this case because the input is readonly it doesn't matter, but in general
it's worth noting that aria-labelledby doesn't do everything a label would do.
For example:

<span id="label">Label</span> <input type="text" aria-labelledby="label">

If we used <label for> or placed the <input> inside the span, then clicking on
the label would automatically focus the input. If we use aria-labelledby, that
doesn't happen.

In chrome://settings for example, we can change a checkbox by clicking on its
<label> because it's a label element.

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

https://codereview.chromium.org/1135013005/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/downloads_page/downloads_page.js:42: subpage:
{
On 2015/05/12 07:01:15, Jeremy Klein wrote:
> On 2015/05/12 06:50:19, michaelpg wrote:
> > same as above?
> 
> I kind of like the idea of being explicit with this one, but don't feel too
> strongly about it. Changed.

Either way.

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

https://codereview.chromium.org/1135013005/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/downloads_page/downloads_page.html:16: <div
id="locationLabel" i18n-content="downloadsLocationLabel"></div>
This puts the label above the input instead of alongside it.

https://codereview.chromium.org/1135013005/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/downloads_page/downloads_page.html:17:
<cr-input id="downloadsPath" floatingLabel="false"
BTW, looks like we need iron-input added to polymer_resources.grdp (and whatever
else iron-input depends on).

[michaelpg, 2015-05-12 07:32:00]

On 2015/05/12 07:01:06, Jeremy Klein wrote:
> On 2015/05/12 06:50:19, michaelpg wrote:
> >
>
https://codereview.chromium.org/1135013005/diff/1/chrome/browser/resources/se...
> > File chrome/browser/resources/settings/downloads_page/downloads_page.js
> (right):
> > 
> >
>
https://codereview.chromium.org/1135013005/diff/1/chrome/browser/resources/se...
> > chrome/browser/resources/settings/downloads_page/downloads_page.js:37:
route:
> {
> > On 2015/05/12 06:19:27, Jeremy Klein wrote:
> > > On 2015/05/12 05:52:14, michaelpg wrote:
> > > > I'm probably just out of the loop, but what's the rationale for not
using
> > the
> > > > shorthand (route: String)?
> > > 
> > > In this case, no reason. Done.
> > > 
> > > FWIW there is a subtle difference here if the property is reflected to an
> > > attribute. With no "value: ''", the attribute isn't added at all by
default,
> > but
> > > with it, you get route=''.
> > 
> > I don't see this behavior -- either way, for me, the attribute is not
present
> > (getAttribute('route') === null). The difference I do see is that with
"value:
> > ''", the instance of the element will have a property "route" set to ''
> > (otherwise, no such property is defined on the instance).
> 
> Weird, even when reflectToAttribute is true? Someone on the Polymer team gave
me
> this advice, but maybe I misunderstood...

With reflectToAttribute it will create the attribute, but only if a value is
provided (including '').

They desperately need to document this behavior.

> 
> > 
> >
>
https://codereview.chromium.org/1135013005/diff/60001/chrome/browser/resource...
> > File chrome/browser/resources/settings/downloads_page/downloads_page.html
> > (right):
> > 
> >
>
https://codereview.chromium.org/1135013005/diff/60001/chrome/browser/resource...
> > chrome/browser/resources/settings/downloads_page/downloads_page.html:16:
> <label
> > class="location-label" i18n-content="downloadsLocationLabel">
> > I think i18n-content will result in the HTML content of this element being
> > replaced. In which case I would move the i18n-content to a <span> inside the
> > label, or set for="downloadsPath" on the label and keep the <cr-input>
> adjacent
> > to the label.
> > 
> >
>
https://codereview.chromium.org/1135013005/diff/60001/chrome/browser/resource...
> > File chrome/browser/resources/settings/downloads_page/downloads_page.js
> (right):
> > 
> >
>
https://codereview.chromium.org/1135013005/diff/60001/chrome/browser/resource...
> > chrome/browser/resources/settings/downloads_page/downloads_page.js:42:
> subpage:
> > {
> > same as above?

[Jeremy Klein, 2015-05-12 22:24:18]

Also tested this out and fixed a couple things

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

https://codereview.chromium.org/1135013005/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/downloads_page/downloads_page.js:42: subpage:
{
On 2015/05/12 07:27:47, michaelpg wrote:
> On 2015/05/12 07:01:15, Jeremy Klein wrote:
> > On 2015/05/12 06:50:19, michaelpg wrote:
> > > same as above?
> > 
> > I kind of like the idea of being explicit with this one, but don't feel too
> > strongly about it. Changed.
> 
> Either way.

Went back to add readOnly.

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

https://codereview.chromium.org/1135013005/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/downloads_page/downloads_page.html:16: <div
id="locationLabel" i18n-content="downloadsLocationLabel"></div>
On 2015/05/12 07:27:47, michaelpg wrote:
> This puts the label above the input instead of alongside it.

No, this works because of the flexbox of the wrapper div.

https://codereview.chromium.org/1135013005/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/downloads_page/downloads_page.html:17:
<cr-input id="downloadsPath" floatingLabel="false"
On 2015/05/12 07:27:47, michaelpg wrote:
> BTW, looks like we need iron-input added to polymer_resources.grdp (and
whatever
> else iron-input depends on).

Done.

[michaelpg, 2015-05-12 22:45:04]

lgtm

https://codereview.chromium.org/1135013005/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/settings/downloads_page/downloads_page.css
(right):

https://codereview.chromium.org/1135013005/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/downloads_page/downloads_page.css:1: cr-input
{
license

[Jeremy Klein, 2015-05-12 22:47:51]

On 2015/05/12 22:45:04, michaelpg wrote:
> lgtm
> 
>
https://codereview.chromium.org/1135013005/diff/100001/chrome/browser/resourc...
> File chrome/browser/resources/settings/downloads_page/downloads_page.css
> (right):
> 
>
https://codereview.chromium.org/1135013005/diff/100001/chrome/browser/resourc...
> chrome/browser/resources/settings/downloads_page/downloads_page.css:1:
cr-input
> {
> license

Done.

[Jeremy Klein, 2015-05-12 22:47:55]

The CQ bit was checked by jlklein@chromium.org

[Jeremy Klein, 2015-05-12 22:47:55]

The patchset sent to the CQ was uploaded after l-g-t-m from
michaelpg@chromium.org
Link to the patchset: https://codereview.chromium.org/1135013005/#ps120001
(title: "css license")

[commit-bot: I haz the power, 2015-05-12 22:49:05]

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

[commit-bot: I haz the power, 2015-05-13 00:26:34]

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

[commit-bot: I haz the power, 2015-05-13 00:26:35]

Failed to apply patch for ui/webui/resources/polymer_resources.grdp:
While running git apply --index -3 -p1;
  error: patch failed: ui/webui/resources/polymer_resources.grdp:526
  Falling back to three-way merge...
  Applied patch to 'ui/webui/resources/polymer_resources.grdp' with conflicts.
  U ui/webui/resources/polymer_resources.grdp

Patch:       ui/webui/resources/polymer_resources.grdp
Index: ui/webui/resources/polymer_resources.grdp
diff --git a/ui/webui/resources/polymer_resources.grdp
b/ui/webui/resources/polymer_resources.grdp
index
9456a19035cc8b696bbce6e3078c161963cff5f3..830d21f13969a235a9d031b39596bc47ae0a6e14
100644
--- a/ui/webui/resources/polymer_resources.grdp
+++ b/ui/webui/resources/polymer_resources.grdp
@@ -526,6 +526,12 @@
     <structure
name="IDR_POLYMER_08_IRON_ICONSET_SVG_IRON_ICONSET_SVG_EXTRACTED_JS"
               
file="../../../third_party/polymer/v0_8/components-chromium/iron-iconset-svg/iron-iconset-svg-extracted.js"
                type="chrome_html" />
+    <structure name="IDR_POLYMER_08_IRON_INPUT_IRON_INPUT_HTML"
+              
file="../../../third_party/polymer/v0_8/components-chromium/iron-input/iron-input.html"
+               type="chrome_html" />
+    <structure name="IDR_POLYMER_08_IRON_INPUT_IRON_INPUT_EXTRACTED_JS"
+              
file="../../../third_party/polymer/v0_8/components-chromium/iron-input/iron-input-extracted.js"
+               type="chrome_html" />
     <structure name="IDR_POLYMER_08_IRON_META_IRON_META_HTML"
               
file="../../../third_party/polymer/v0_8/components-chromium/iron-meta/iron-meta.html"
                type="chrome_html" />
@@ -575,6 +581,12 @@
     <structure
name="IDR_POLYMER_08_PAPER_BEHAVIORS_PAPER_BUTTON_BEHAVIOR_EXTRACTED_JS"
               
file="../../../third_party/polymer/v0_8/components-chromium/paper-behaviors/paper-button-behavior-extracted.js"
                type="chrome_html" />
+    <structure
name="IDR_POLYMER_08_PAPER_BEHAVIORS_PAPER_RADIO_BUTTON_BEHAVIOR_HTML"
+              
file="../../../third_party/polymer/v0_8/components-chromium/paper-behaviors/paper-radio-button-behavior.html"
+               type="chrome_html" />
+    <structure
name="IDR_POLYMER_08_PAPER_BEHAVIORS_PAPER_RADIO_BUTTON_BEHAVIOR_EXTRACTED_JS"
+              
file="../../../third_party/polymer/v0_8/components-chromium/paper-behaviors/paper-radio-button-behavior-extracted.js"
+               type="chrome_html" />
     <structure name="IDR_POLYMER_08_PAPER_BUTTON_PAPER_BUTTON_EXTRACTED_JS"
               
file="../../../third_party/polymer/v0_8/components-chromium/paper-button/paper-button-extracted.js"
                type="chrome_html" />
@@ -602,6 +614,12 @@
     <structure name="IDR_POLYMER_08_PAPER_INPUT_PAPER_INPUT_HTML"
               
file="../../../third_party/polymer/v0_8/components-chromium/paper-input/paper-input.html"
                type="chrome_html" />
+    <structure
name="IDR_POLYMER_08_PAPER_INPUT_PAPER_INPUT_BEHAVIOR_EXTRACTED_JS"
+              
file="../../../third_party/polymer/v0_8/components-chromium/paper-input/paper-input-behavior-extracted.js"
+               type="chrome_html" />
+    <structure name="IDR_POLYMER_08_PAPER_INPUT_PAPER_INPUT_BEHAVIOR_HTML"
+              
file="../../../third_party/polymer/v0_8/components-chromium/paper-input/paper-input-behavior.html"
+               type="chrome_html" />
     <structure
name="IDR_POLYMER_08_PAPER_INPUT_PAPER_INPUT_CHAR_COUNTER_EXTRACTED_JS"
               
file="../../../third_party/polymer/v0_8/components-chromium/paper-input/paper-input-char-counter-extracted.js"
                type="chrome_html" />

[commit-bot: I haz the power, 2015-05-13 00:28:02]

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

[Jeremy Klein, 2015-05-13 00:32:20]

The CQ bit was checked by jlklein@chromium.org

[Jeremy Klein, 2015-05-13 00:32:20]

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

[commit-bot: I haz the power, 2015-05-13 00:32:44]

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

[commit-bot: I haz the power, 2015-05-13 01:19:13]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2015-05-13 01:19:53]

Patchset 8 (id:??) landed as
https://crrev.com/89b448592636ebedd970a604bbd8b4d3abd151b1
Cr-Commit-Position: refs/heads/master@{#329554}