Add an initial implementation of the downloads MD settings page.

Add an initial implementation of the downloads MD settings page.

Note that the "Change" button for selecting the downloads path doesn't do
anything meaningful yet. We still need to figure out how to call into the
C++ layer to display the appropriate picker.

BUG=464785
Committed: https://crrev.com/61f613741603561b14ddf497f110848d592c626a
Cr-Commit-Position: refs/heads/master@{#319787}

[Oren Blasberg, 2015-03-06 19:15:53]

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

[Oren Blasberg, 2015-03-06 19:19:12]

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

[Jeremy Klein, 2015-03-06 19:40:14]

https://codereview.chromium.org/988783002/diff/20001/chrome/browser/resources...
File chrome/browser/resources/settings/downloads_page/demo.html (right):

https://codereview.chromium.org/988783002/diff/20001/chrome/browser/resources...
chrome/browser/resources/settings/downloads_page/demo.html:7:
<cr-settings-downloads-page></cr-settings-downloads-page>
Don't you need a prefs?

https://codereview.chromium.org/988783002/diff/20001/chrome/browser/resources...
File chrome/browser/resources/settings/downloads_page/downloads_page.css
(right):

https://codereview.chromium.org/988783002/diff/20001/chrome/browser/resources...
chrome/browser/resources/settings/downloads_page/downloads_page.css:10:
margin-right: 10px;
RTL. Hah, you told me on mine, now I get to tell you on yours :-).

https://codereview.chromium.org/988783002/diff/20001/chrome/browser/resources...
File chrome/browser/resources/settings/downloads_page/downloads_page.html
(right):

https://codereview.chromium.org/988783002/diff/20001/chrome/browser/resources...
chrome/browser/resources/settings/downloads_page/downloads_page.html:24:
<cr-checkbox checked="" for>
checked="{{prefs.settings.downloads.promptForDownload}}"

https://codereview.chromium.org/988783002/diff/20001/chrome/browser/resources...
File chrome/browser/resources/settings/prefs/prefs.js (right):

https://codereview.chromium.org/988783002/diff/20001/chrome/browser/resources...
chrome/browser/resources/settings/prefs/prefs.js:89: // TODO(jlklein): Actually
pull the data out of prefs and initialize.
nit: no need for the duplicate TODO.

[Oren Blasberg, 2015-03-06 19:53:16]

https://codereview.chromium.org/988783002/diff/20001/chrome/browser/resources...
File chrome/browser/resources/settings/downloads_page/demo.html (right):

https://codereview.chromium.org/988783002/diff/20001/chrome/browser/resources...
chrome/browser/resources/settings/downloads_page/demo.html:7:
<cr-settings-downloads-page></cr-settings-downloads-page>
On 2015/03/06 19:40:14, Jeremy Klein wrote:
> Don't you need a prefs?

Done.

https://codereview.chromium.org/988783002/diff/20001/chrome/browser/resources...
File chrome/browser/resources/settings/downloads_page/downloads_page.css
(right):

https://codereview.chromium.org/988783002/diff/20001/chrome/browser/resources...
chrome/browser/resources/settings/downloads_page/downloads_page.css:10:
margin-right: 10px;
On 2015/03/06 19:40:14, Jeremy Klein wrote:
> RTL. Hah, you told me on mine, now I get to tell you on yours :-).

Done.

https://codereview.chromium.org/988783002/diff/20001/chrome/browser/resources...
File chrome/browser/resources/settings/prefs/prefs.js (right):

https://codereview.chromium.org/988783002/diff/20001/chrome/browser/resources...
chrome/browser/resources/settings/prefs/prefs.js:89: // TODO(jlklein): Actually
pull the data out of prefs and initialize.
On 2015/03/06 19:40:14, Jeremy Klein wrote:
> nit: no need for the duplicate TODO.

Done.

[Jeremy Klein, 2015-03-06 20:01:17]

lgtm
With nits.

https://codereview.chromium.org/988783002/diff/40001/chrome/browser/resources...
File chrome/browser/resources/settings/downloads_page/demo.html (right):

https://codereview.chromium.org/988783002/diff/40001/chrome/browser/resources...
chrome/browser/resources/settings/downloads_page/demo.html:10: var page =
document.querySelector('cr-settings-downloads-page');
Note: You'll need to pull this out into a separate script if you want to test it
in real chrome rather than with serve_elements.py.

https://codereview.chromium.org/988783002/diff/40001/chrome/browser/resources...
File chrome/browser/resources/settings/downloads_page/downloads_page.css
(right):

https://codereview.chromium.org/988783002/diff/40001/chrome/browser/resources...
chrome/browser/resources/settings/downloads_page/downloads_page.css:24:
margin-right: 10px;
missed an rtl spot.

https://codereview.chromium.org/988783002/diff/40001/chrome/browser/resources...
File chrome/browser/resources/settings/downloads_page/downloads_page.js (right):

https://codereview.chromium.org/988783002/diff/40001/chrome/browser/resources...
chrome/browser/resources/settings/downloads_page/downloads_page.js:1: /*
Copyright 2015 The Chromium Authors. All rights reserved.
nit: C++ style header comment (//)

https://codereview.chromium.org/988783002/diff/40001/chrome/browser/resources...
chrome/browser/resources/settings/downloads_page/downloads_page.js:35:
this.$.downloadsPath.value = '/Downloads';
I guess you might as well also set the prefs value here. 

this.prefs.settings.downloads.downloadLocation

[Oren Blasberg, 2015-03-06 21:40:25]

https://codereview.chromium.org/988783002/diff/40001/chrome/browser/resources...
File chrome/browser/resources/settings/downloads_page/demo.html (right):

https://codereview.chromium.org/988783002/diff/40001/chrome/browser/resources...
chrome/browser/resources/settings/downloads_page/demo.html:10: var page =
document.querySelector('cr-settings-downloads-page');
On 2015/03/06 20:01:17, Jeremy Klein wrote:
> Note: You'll need to pull this out into a separate script if you want to test
it
> in real chrome rather than with serve_elements.py.

Done.

https://codereview.chromium.org/988783002/diff/40001/chrome/browser/resources...
File chrome/browser/resources/settings/downloads_page/downloads_page.css
(right):

https://codereview.chromium.org/988783002/diff/40001/chrome/browser/resources...
chrome/browser/resources/settings/downloads_page/downloads_page.css:24:
margin-right: 10px;
On 2015/03/06 20:01:17, Jeremy Klein wrote:
> missed an rtl spot.

Done.

https://codereview.chromium.org/988783002/diff/40001/chrome/browser/resources...
File chrome/browser/resources/settings/downloads_page/downloads_page.js (right):

https://codereview.chromium.org/988783002/diff/40001/chrome/browser/resources...
chrome/browser/resources/settings/downloads_page/downloads_page.js:1: /*
Copyright 2015 The Chromium Authors. All rights reserved.
On 2015/03/06 20:01:17, Jeremy Klein wrote:
> nit: C++ style header comment (//)

Done.

https://codereview.chromium.org/988783002/diff/40001/chrome/browser/resources...
chrome/browser/resources/settings/downloads_page/downloads_page.js:35:
this.$.downloadsPath.value = '/Downloads';
On 2015/03/06 20:01:17, Jeremy Klein wrote:
> I guess you might as well also set the prefs value here. 
> 
> this.prefs.settings.downloads.downloadLocation

Done.

[michaelpg, 2015-03-08 05:37:36]

https://codereview.chromium.org/988783002/diff/100001/chrome/browser/resource...
File chrome/browser/resources/settings/downloads_page/demo.js (right):

https://codereview.chromium.org/988783002/diff/100001/chrome/browser/resource...
chrome/browser/resources/settings/downloads_page/demo.js:6: * Wire up the prefs
to the downloads page.
optional nit: I'm not sure this really need to be a JSDoc, as opposed to a
single // comment

https://codereview.chromium.org/988783002/diff/100001/chrome/browser/resource...
chrome/browser/resources/settings/downloads_page/demo.js:8: window.onload =
function() {
Prefer window.addEventListener('polymer-ready', ...) to ensure that "page.prefs"
has been set up as a published property.

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

https://codereview.chromium.org/988783002/diff/100001/chrome/browser/resource...
chrome/browser/resources/settings/downloads_page/downloads_page.html:16:
<cr-input id="downloadsPath" floatingLabel="false" size="36"
FYI, per the Chrome style guide, IDs use dash-form, not camelCase:
http://www.chromium.org/developers/web-development-style-guide

But Polymer's this.$ hash is so convenient and would be uglier if IDs had
dashes:
    this.$.downloadsPath
    this.$['downloads-path']

So I would say as long as we're consistent within elements, let's ignore this
rule. any objections?

The rationale for id-hyphens in the Google HTML style guide is "to improve
understanding and scannability" which I think camelCase also does just fine.

https://codereview.chromium.org/988783002/diff/100001/chrome/browser/resource...
chrome/browser/resources/settings/downloads_page/downloads_page.html:16:
<cr-input id="downloadsPath" floatingLabel="false" size="36"
Does size="36" work? I don't see it as an attribute on cr-input (or the <input>
within core-input)

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

https://codereview.chromium.org/988783002/diff/100001/chrome/browser/resource...
chrome/browser/resources/settings/downloads_page/downloads_page.js:7: *
'cr-settings-downloads-page' is the settings page containing all downloads
nit: let's not claim this contains ALL the downloads settings.

https://codereview.chromium.org/988783002/diff/100001/chrome/browser/resource...
chrome/browser/resources/settings/downloads_page/downloads_page.js:29: prefs:
null
nit: add trailing comma. You may already know this, but I think this is
suggested mainly to make git history clearer -- when someone adds another
property after this, they'll be "blamed" for this line as well, just for
appending a comma.

TL;DR: if something catches fire I want to make sure I can point my pitchfork in
your direction

https://codereview.chromium.org/988783002/diff/100001/chrome/browser/resource...
File chrome/browser/resources/settings/prefs/prefs.js (right):

https://codereview.chromium.org/988783002/diff/100001/chrome/browser/resource...
chrome/browser/resources/settings/prefs/prefs.js:84: this.settings.downloads = {
If we're matching pref_names.cc, this would be "download" (singular).

https://codereview.chromium.org/988783002/diff/100001/chrome/browser/resource...
chrome/browser/resources/settings/prefs/prefs.js:85: downloadLocation: '',
If we're matching pref_names.cc, this would be defaultDirectory.

[Kyle Horimoto, 2015-03-09 20:16:21]

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

[Kyle Horimoto, 2015-03-09 20:16:21]

https://codereview.chromium.org/988783002/diff/100001/chrome/browser/resource...
File chrome/browser/resources/settings/downloads_page/demo.js (right):

https://codereview.chromium.org/988783002/diff/100001/chrome/browser/resource...
chrome/browser/resources/settings/downloads_page/demo.js:6: * Wire up the prefs
to the downloads page.
On 2015/03/08 05:37:36, michaelpg wrote:
> optional nit: I'm not sure this really need to be a JSDoc, as opposed to a
> single // comment

Yep, I don't think this an appropriate JSDoc use case.

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

https://codereview.chromium.org/988783002/diff/100001/chrome/browser/resource...
chrome/browser/resources/settings/downloads_page/downloads_page.css:17: cr-input
/deep/ #input {
Isn't this a more appropriate use case for ::shadow over /deep/?

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

https://codereview.chromium.org/988783002/diff/100001/chrome/browser/resource...
chrome/browser/resources/settings/downloads_page/downloads_page.html:16:
<cr-input id="downloadsPath" floatingLabel="false" size="36"
On 2015/03/08 05:37:36, michaelpg wrote:
> FYI, per the Chrome style guide, IDs use dash-form, not camelCase:
> http://www.chromium.org/developers/web-development-style-guide
> 
> But Polymer's this.$ hash is so convenient and would be uglier if IDs had
> dashes:
>     this.$.downloadsPath
>     this.$['downloads-path']
> 
> So I would say as long as we're consistent within elements, let's ignore this
> rule. any objections?
> 
> The rationale for id-hyphens in the Google HTML style guide is "to improve
> understanding and scannability" which I think camelCase also does just fine.

Probably good to bring this up on the list.

[Oren Blasberg, 2015-03-09 21:05:06]

https://codereview.chromium.org/988783002/diff/100001/chrome/browser/resource...
File chrome/browser/resources/settings/downloads_page/demo.js (right):

https://codereview.chromium.org/988783002/diff/100001/chrome/browser/resource...
chrome/browser/resources/settings/downloads_page/demo.js:6: * Wire up the prefs
to the downloads page.
On 2015/03/09 20:16:21, Kyle Horimoto wrote:
> On 2015/03/08 05:37:36, michaelpg wrote:
> > optional nit: I'm not sure this really need to be a JSDoc, as opposed to a
> > single // comment
> 
> Yep, I don't think this an appropriate JSDoc use case.

Done.

https://codereview.chromium.org/988783002/diff/100001/chrome/browser/resource...
chrome/browser/resources/settings/downloads_page/demo.js:8: window.onload =
function() {
On 2015/03/08 05:37:36, michaelpg wrote:
> Prefer window.addEventListener('polymer-ready', ...) to ensure that
"page.prefs"
> has been set up as a published property.

Done.

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

https://codereview.chromium.org/988783002/diff/100001/chrome/browser/resource...
chrome/browser/resources/settings/downloads_page/downloads_page.css:17: cr-input
/deep/ #input {
On 2015/03/09 20:16:21, Kyle Horimoto wrote:
> Isn't this a more appropriate use case for ::shadow over /deep/?

Done.

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

https://codereview.chromium.org/988783002/diff/100001/chrome/browser/resource...
chrome/browser/resources/settings/downloads_page/downloads_page.html:16:
<cr-input id="downloadsPath" floatingLabel="false" size="36"
On 2015/03/09 20:16:21, Kyle Horimoto wrote:
> On 2015/03/08 05:37:36, michaelpg wrote:
> > FYI, per the Chrome style guide, IDs use dash-form, not camelCase:
> > http://www.chromium.org/developers/web-development-style-guide
> > 
> > But Polymer's this.$ hash is so convenient and would be uglier if IDs had
> > dashes:
> >     this.$.downloadsPath
> >     this.$['downloads-path']
> > 
> > So I would say as long as we're consistent within elements, let's ignore
this
> > rule. any objections?
> > 
> > The rationale for id-hyphens in the Google HTML style guide is "to improve
> > understanding and scannability" which I think camelCase also does just fine.
> 
> Probably good to bring this up on the list.

OK, I'll keep it camel case for now.
Removed size=36.

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

https://codereview.chromium.org/988783002/diff/100001/chrome/browser/resource...
chrome/browser/resources/settings/downloads_page/downloads_page.js:7: *
'cr-settings-downloads-page' is the settings page containing all downloads
On 2015/03/08 05:37:36, michaelpg wrote:
> nit: let's not claim this contains ALL the downloads settings.

Why not? Doesn't it contain them?

https://codereview.chromium.org/988783002/diff/100001/chrome/browser/resource...
chrome/browser/resources/settings/downloads_page/downloads_page.js:29: prefs:
null
On 2015/03/08 05:37:36, michaelpg wrote:
> nit: add trailing comma. You may already know this, but I think this is
> suggested mainly to make git history clearer -- when someone adds another
> property after this, they'll be "blamed" for this line as well, just for
> appending a comma.
> 
> TL;DR: if something catches fire I want to make sure I can point my pitchfork
in
> your direction

Done.

https://codereview.chromium.org/988783002/diff/100001/chrome/browser/resource...
File chrome/browser/resources/settings/prefs/prefs.js (right):

https://codereview.chromium.org/988783002/diff/100001/chrome/browser/resource...
chrome/browser/resources/settings/prefs/prefs.js:84: this.settings.downloads = {
On 2015/03/08 05:37:36, michaelpg wrote:
> If we're matching pref_names.cc, this would be "download" (singular).

Done.

https://codereview.chromium.org/988783002/diff/100001/chrome/browser/resource...
chrome/browser/resources/settings/prefs/prefs.js:85: downloadLocation: '',
On 2015/03/08 05:37:36, michaelpg wrote:
> If we're matching pref_names.cc, this would be defaultDirectory.

Done.

[michaelpg, 2015-03-09 21:24:08]

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

https://codereview.chromium.org/988783002/diff/100001/chrome/browser/resource...
chrome/browser/resources/settings/downloads_page/downloads_page.js:7: *
'cr-settings-downloads-page' is the settings page containing all downloads
On 2015/03/09 21:05:05, Oren Blasberg wrote:
> On 2015/03/08 05:37:36, michaelpg wrote:
> > nit: let's not claim this contains ALL the downloads settings.
> 
> Why not? Doesn't it contain them?

For example it's missing auto-open settings, which you can set in the downloads
bar (and then a button to "clear auto-opening settings" appears in
chrome://settings).

Moreover we'd have to update this every time we add a setting that isn't on this
page or remove a setting from this page... it just seems like an unnecessary
claim.

But it's just a nit.

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

https://codereview.chromium.org/988783002/diff/120001/chrome/browser/resource...
chrome/browser/resources/settings/downloads_page/downloads_page.html:19: for>
Why the separate lines?

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

https://codereview.chromium.org/988783002/diff/120001/chrome/browser/resource...
chrome/browser/resources/settings/downloads_page/downloads_page.js:40:
this.$.promptForDownload.checked;
hmm I'm a little confused by this. It doesn't look like checkbox has this ID. Is
this function necessary or can we do this with data binding?

[Oren Blasberg, 2015-03-09 21:39:13]

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

https://codereview.chromium.org/988783002/diff/100001/chrome/browser/resource...
chrome/browser/resources/settings/downloads_page/downloads_page.js:7: *
'cr-settings-downloads-page' is the settings page containing all downloads
On 2015/03/09 21:24:08, michaelpg wrote:
> On 2015/03/09 21:05:05, Oren Blasberg wrote:
> > On 2015/03/08 05:37:36, michaelpg wrote:
> > > nit: let's not claim this contains ALL the downloads settings.
> > 
> > Why not? Doesn't it contain them?
> 
> For example it's missing auto-open settings, which you can set in the
downloads
> bar (and then a button to "clear auto-opening settings" appears in
> chrome://settings).
> 
> Moreover we'd have to update this every time we add a setting that isn't on
this
> page or remove a setting from this page... it just seems like an unnecessary
> claim.
> 
> But it's just a nit.

Done.

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

https://codereview.chromium.org/988783002/diff/120001/chrome/browser/resource...
chrome/browser/resources/settings/downloads_page/downloads_page.html:19: for>
On 2015/03/09 21:24:08, michaelpg wrote:
> Why the separate lines?

Done.

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

https://codereview.chromium.org/988783002/diff/120001/chrome/browser/resource...
chrome/browser/resources/settings/downloads_page/downloads_page.js:40:
this.$.promptForDownload.checked;
On 2015/03/09 21:24:08, michaelpg wrote:
> hmm I'm a little confused by this. It doesn't look like checkbox has this ID.
Is
> this function necessary or can we do this with data binding?

Oops -- yes, you're right. I'd added this method in a temporary state, then
realized data binding was all that we needed, then forgot to delete this method.
Deleted.

[Kyle Horimoto, 2015-03-09 21:40:27]

lgtm

[michaelpg, 2015-03-09 22:29:46]

lgtm

[Oren Blasberg, 2015-03-09 22:32:57]

The CQ bit was checked by orenb@chromium.org

[Oren Blasberg, 2015-03-09 22:32:57]

The patchset sent to the CQ was uploaded after l-g-t-m from jlklein@chromium.org
Link to the patchset: https://codereview.chromium.org/988783002/#ps140001
(title: "Respond to Michael's comments.")

[commit-bot: I haz the power, 2015-03-09 22:33:40]

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

[commit-bot: I haz the power, 2015-03-10 00:13:01]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2015-03-10 00:13:56]

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