Slice MD downloads into more components

Slice MD downloads into more components

downloads.{
  ActionService: tells the browser to modify downloads
* Manager: the whole page (required for CSS var expansion)
* Toolbar: blue, top bar with search/title/actions
}

* has an actual UI/<element>

Also adding a shared_styles.css with variables to be @apply()ed or
--used through downloads.

BUG=425626
R=jlklein@chromium.org

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

[Dan Beam, 2015-07-16 03:00:24]

Patchset #2 (id:20001) has been deleted

[Dan Beam, 2015-07-16 03:00:30]

Patchset #1 (id:1) has been deleted

[Dan Beam, 2015-07-16 03:02:44]

Patchset #1 (id:40001) has been deleted

[Jeremy Klein, 2015-07-16 18:54:54]

https://codereview.chromium.org/1240853002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/md_downloads/action_service.js (right):

https://codereview.chromium.org/1240853002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_downloads/action_service.js:6: /** @interface */
Include description of its purpose.

https://codereview.chromium.org/1240853002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/md_downloads/item_view.css (right):

https://codereview.chromium.org/1240853002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_downloads/item_view.css:6: width: 606px;  /* Matches
#actions in toolbar.css. */
Maybe worth exporting to a shared css variable at some point (not needed in this
CL)

https://codereview.chromium.org/1240853002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/md_downloads/item_view.html (right):

https://codereview.chromium.org/1240853002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_downloads/item_view.html:60: <link rel="import"
href="chrome://downloads/action_service.html">
These imports are usually all outside the dom-module at the top of the file. Why
the change?

https://codereview.chromium.org/1240853002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/md_downloads/manager.js (right):

https://codereview.chromium.org/1240853002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_downloads/manager.js:187: e.canExecute = searchTerm
&& !searchTerm.contains(activeElement);
This is a cool trick for determining focus. I've never seen that, but i like it
:-).

https://codereview.chromium.org/1240853002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_downloads/manager.js:213:
this.toolbar_.setActionService(this);
This creates a circular dependency that makes these two components really
tightly coupled. I realize that the interface technically breaks that loop, but
it still seems like the coupling here isn't ideal. Events would mean that the
toolbar wouldn't need any knowledge of the action service at all, but would
create a lot of extra boilerplate. The other option is an ActionService which
has no explicit knowledge of the toolbar's existence. That would also make this
a bit easier to test independently.

Regardless, I won't block on this, but it's something to consider.

https://codereview.chromium.org/1240853002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/md_downloads/toolbar.html (right):

https://codereview.chromium.org/1240853002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_downloads/toolbar.html:6: <paper-button
id="clear-all" i18n-content="clearAll"
y u no cr-elements

https://codereview.chromium.org/1240853002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_downloads/toolbar.html:8:
hidden$="{{!canClearAll}}"></paper-button>
nit: [[]] here and other hidden attributes.

https://codereview.chromium.org/1240853002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_downloads/toolbar.html:25: <link rel="import"
href="chrome://resources/polymer/v1_0/polymer/polymer.html">
same comment about imports being at the top outside the dom-module.

https://codereview.chromium.org/1240853002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/md_downloads/toolbar.js (right):

https://codereview.chromium.org/1240853002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_downloads/toolbar.js:5: cr.define('downloads',
function() {
I wouldn't be surprised if the compiler pass does strange things here wrt to the
type of this element because it may not be aware of what cr.define is doing.
However, in Chrome world, the ChromePass does run before PolymerPass so maybe
the rewriting that ChromePass does is enough? Let's chat about the details here
offline.

https://codereview.chromium.org/1240853002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_downloads/toolbar.js:28: this.searchTerm =
this.$['search-term'];
Consider making searchTerm a declared property with a type specified. On the
other hand, I don't really like the idea of exposing an element in the local dom
as part of the public API unless it's totally necessary. Can we expose a
"hasUndo" function instead?

[Dan Beam, 2015-07-18 00:55:20]

Patchset #2 (id:80001) has been deleted

[Dan Beam, 2015-07-18 01:02:21]

https://codereview.chromium.org/1240853002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/md_downloads/action_service.js (right):

https://codereview.chromium.org/1240853002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_downloads/action_service.js:6: /** @interface */
On 2015/07/16 18:54:53, Jeremy Klein wrote:
> Include description of its purpose.

Delorted.

https://codereview.chromium.org/1240853002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/md_downloads/item_view.css (right):

https://codereview.chromium.org/1240853002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_downloads/item_view.css:6: width: 606px;  /* Matches
#actions in toolbar.css. */
On 2015/07/16 18:54:53, Jeremy Klein wrote:
> Maybe worth exporting to a shared css variable at some point (not needed in
this
> CL)

Acknowledged.

https://codereview.chromium.org/1240853002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/md_downloads/item_view.html (right):

https://codereview.chromium.org/1240853002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_downloads/item_view.html:60: <link rel="import"
href="chrome://downloads/action_service.html">
On 2015/07/16 18:54:53, Jeremy Klein wrote:
> These imports are usually all outside the dom-module at the top of the file.
Why
> the change?

this is how all the downloads code and Polymer examples are.

where are you getting "usually" from?

https://codereview.chromium.org/1240853002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/md_downloads/manager.js (right):

https://codereview.chromium.org/1240853002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_downloads/manager.js:187: e.canExecute = searchTerm
&& !searchTerm.contains(activeElement);
On 2015/07/16 18:54:53, Jeremy Klein wrote:
> This is a cool trick for determining focus. I've never seen that, but i like
it
> :-).

Acknowledged.

https://codereview.chromium.org/1240853002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_downloads/manager.js:213:
this.toolbar_.setActionService(this);
On 2015/07/16 18:54:53, Jeremy Klein wrote:
> This creates a circular dependency that makes these two components really
> tightly coupled. I realize that the interface technically breaks that loop,
but
> it still seems like the coupling here isn't ideal. Events would mean that the
> toolbar wouldn't need any knowledge of the action service at all, but would
> create a lot of extra boilerplate. The other option is an ActionService which
> has no explicit knowledge of the toolbar's existence. That would also make
this
> a bit easier to test independently.
> 
> Regardless, I won't block on this, but it's something to consider.

Done.

https://codereview.chromium.org/1240853002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/md_downloads/toolbar.html (right):

https://codereview.chromium.org/1240853002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_downloads/toolbar.html:6: <paper-button
id="clear-all" i18n-content="clearAll"
On 2015/07/16 18:54:54, Jeremy Klein wrote:
> y u no cr-elements

Done.

https://codereview.chromium.org/1240853002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_downloads/toolbar.html:8:
hidden$="{{!canClearAll}}"></paper-button>
On 2015/07/16 18:54:54, Jeremy Klein wrote:
> nit: [[]] here and other hidden attributes.

Done.

https://codereview.chromium.org/1240853002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_downloads/toolbar.html:25: <link rel="import"
href="chrome://resources/polymer/v1_0/polymer/polymer.html">
On 2015/07/16 18:54:54, Jeremy Klein wrote:
> same comment about imports being at the top outside the dom-module.

Acknowledged.

https://codereview.chromium.org/1240853002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/md_downloads/toolbar.js (right):

https://codereview.chromium.org/1240853002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_downloads/toolbar.js:5: cr.define('downloads',
function() {
On 2015/07/16 18:54:54, Jeremy Klein wrote:
> I wouldn't be surprised if the compiler pass does strange things here wrt to
the
> type of this element because it may not be aware of what cr.define is doing.
> However, in Chrome world, the ChromePass does run before PolymerPass so maybe
> the rewriting that ChromePass does is enough? Let's chat about the details
here
> offline.

fwiw: we could also do

cr.exportPath('downloads');

downloads.Toolbar = Polymer(...);

https://codereview.chromium.org/1240853002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_downloads/toolbar.js:28: this.searchTerm =
this.$['search-term'];
On 2015/07/16 18:54:54, Jeremy Klein wrote:
> Consider making searchTerm a declared property with a type specified. On the
> other hand, I don't really like the idea of exposing an element in the local
dom
> as part of the public API unless it's totally necessary. Can we expose a
> "hasUndo" function instead?

Done.

[Dan Beam, 2015-07-18 01:08:05]

+michaelpg@ as FYI

[Jeremy Klein, 2015-07-20 17:09:57]

https://codereview.chromium.org/1240853002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/md_downloads/item_view.html (right):

https://codereview.chromium.org/1240853002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_downloads/item_view.html:60: <link rel="import"
href="chrome://downloads/action_service.html">
On 2015/07/18 01:02:20, Dan Beam wrote:
> On 2015/07/16 18:54:53, Jeremy Klein wrote:
> > These imports are usually all outside the dom-module at the top of the file.
> Why
> > the change?
> 
> this is how all the downloads code and Polymer examples are.
> 
> where are you getting "usually" from?

The only import I've ever seen inside the dom-module is the styles for the
element being defined. Which Polymer examples are you looking at? They probably
need to be updated.

See all paper-* and iron-* elements (https://github.com/PolymerElements), all
settings code, media-router code, oobe, all examples in the quick start guide
(https://www.polymer-project.org/1.0/docs/start/quick-tour.html) and all
elements in Google3.

While I don't know for sure if there's a functional difference here, I wouldn't
be surprised if subtle issues came up from importing other element definitions
into the actual dom-module of an element.

[Dan Beam, 2015-07-20 17:25:53]

https://codereview.chromium.org/1240853002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/md_downloads/item_view.html (right):

https://codereview.chromium.org/1240853002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_downloads/item_view.html:60: <link rel="import"
href="chrome://downloads/action_service.html">
On 2015/07/20 17:09:57, Jeremy Klein wrote:
> On 2015/07/18 01:02:20, Dan Beam wrote:
> > On 2015/07/16 18:54:53, Jeremy Klein wrote:
> > > These imports are usually all outside the dom-module at the top of the
file.
> > Why
> > > the change?
> > 
> > this is how all the downloads code and Polymer examples are.
> > 
> > where are you getting "usually" from?
> 
> The only import I've ever seen inside the dom-module is the styles for the
> element being defined. Which Polymer examples are you looking at? They
probably
> need to be updated.
> 
> See all paper-* and iron-* elements (https://github.com/PolymerElements), all
> settings code, media-router code, oobe, all examples in the quick start guide
> (https://www.polymer-project.org/1.0/docs/start/quick-tour.html) and all
> elements in Google3.
> 
> While I don't know for sure if there's a functional difference here, I
wouldn't
> be surprised if subtle issues came up from importing other element definitions
> into the actual dom-module of an element.

Done.

[Jeremy Klein, 2015-07-20 17:40:18]

lgtm

https://codereview.chromium.org/1240853002/diff/110001/chrome/browser/resourc...
File chrome/browser/resources/md_downloads/toolbar.html (right):

https://codereview.chromium.org/1240853002/diff/110001/chrome/browser/resourc...
chrome/browser/resources/md_downloads/toolbar.html:9: <dom-module
id="downloads-toolbar" is="polymer-toolbar">
Where did the is come from? Just left over from somewhere?

[Dan Beam, 2015-07-20 17:41:48]

https://codereview.chromium.org/1240853002/diff/110001/chrome/browser/resourc...
File chrome/browser/resources/md_downloads/toolbar.html (right):

https://codereview.chromium.org/1240853002/diff/110001/chrome/browser/resourc...
chrome/browser/resources/md_downloads/toolbar.html:9: <dom-module
id="downloads-toolbar" is="polymer-toolbar">
On 2015/07/20 17:40:18, Jeremy Klein wrote:
> Where did the is come from? Just left over from somewhere?

@_@ yes?

[Dan Beam, 2015-07-20 17:41:57]

Patchset #2 (id:100001) has been deleted

[Dan Beam, 2015-07-20 18:24:46]

The CQ bit was checked by dbeam@chromium.org

[Dan Beam, 2015-07-20 18:24:46]

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/1240853002/#ps150001
(title: "merge origin/master")

[commit-bot: I haz the power, 2015-07-20 18:25:22]

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

[commit-bot: I haz the power, 2015-07-20 18:31:30]

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

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

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...)

[Dan Beam, 2015-07-20 20:44:21]

The CQ bit was checked by dbeam@chromium.org

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

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

[commit-bot: I haz the power, 2015-07-20 20:54:37]

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

[commit-bot: I haz the power, 2015-07-20 20:54:38]

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...)

[Dan Beam, 2015-07-20 21:02:45]

The CQ bit was checked by dbeam@chromium.org

[Dan Beam, 2015-07-20 21:02:45]

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/1240853002/#ps170001
(title: "license")

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

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

[commit-bot: I haz the power, 2015-07-20 21:12:41]

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

[commit-bot: I haz the power, 2015-07-20 21:12:41]

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...)

[Dan Beam, 2015-07-20 22:38:40]

The CQ bit was checked by dbeam@chromium.org

[commit-bot: I haz the power, 2015-07-20 22:39:13]

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

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

Committed patchset #5 (id:170001)

[commit-bot: I haz the power, 2015-07-20 22:47:23]

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