MD Downloads: use <iron-list> to render download items

MD Downloads: use <iron-list> to render download items

Allows us to render only the visible(-ish) items based on the size of the window.
For me that's about ~20 but I'm using a funky portrait monitor (uber tall).

R=esprehn@chromium.org
BUG=526577, 526583
NOPRESUBMIT=true  # because crisper.js

Committed: https://crrev.com/9dc53f65ae568a624734e9ecc5e70880cb33479e
Cr-Commit-Position: refs/heads/master@{#354221}

[Dan Beam, 2015-10-02 05:30:44]

Patchset #1 (id:1) has been deleted

[Dan Beam, 2015-10-02 05:37:07]

https://codereview.chromium.org/1375333004/diff/20001/chrome/browser/resource...
File chrome/browser/resources/md_downloads/item.js (left):

https://codereview.chromium.org/1375333004/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_downloads/item.js:13: /** @private
{!downloads.ThrottledIconLoader} */
will remove this class fully separately

https://codereview.chromium.org/1375333004/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_downloads/item.js:27: readyPromise: {
wasn't needed

https://codereview.chromium.org/1375333004/diff/20001/chrome/browser/resource...
File chrome/browser/resources/md_downloads/manager.js (left):

https://codereview.chromium.org/1375333004/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_downloads/manager.js:87: },
breaks a11y, but that's now your battle

[Dan Beam, 2015-10-02 23:09:47]

/cc aboxhall@ as fyi

https://codereview.chromium.org/1375333004/diff/20001/chrome/browser/resource...
File chrome/browser/resources/md_downloads/manager.js (left):

https://codereview.chromium.org/1375333004/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_downloads/manager.js:87: },
On 2015/10/02 05:37:06, Dan Beam wrote:
> breaks a11y, but that's now your battle

aboxhall@ doesn't see this as a huge issue, and vtsaran@ and dtseng@ confirm the
ability to jump over lists (or move within the AX tree) exists in all 3 major
screen readers (jaws, voiceover, cvox)

the only aspect of this change that's now slightly unknown is how screen readers
deal with infinite (partially rendered) lists.

[esprehn, 2015-10-03 05:11:54]

lgtm w/ nits, I might call it indexMap instead, but this is fine. :)

https://codereview.chromium.org/1375333004/diff/40001/chrome/browser/resource...
File chrome/browser/resources/md_downloads/item.js (right):

https://codereview.chromium.org/1375333004/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_downloads/item.js:91:
'observeIsDangerous_(isDangerous_, data.file_path)',
So dangerous!

https://codereview.chromium.org/1375333004/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_downloads/item.js:242: this.$['file-icon'].src =
'chrome://fileicon/' + filePath;
If we make our ids camel case this is prettier. Not for  this patch but
something to consider.

https://codereview.chromium.org/1375333004/diff/40002/chrome/browser/resource...
File chrome/browser/resources/md_downloads/item.html (left):

https://codereview.chromium.org/1375333004/diff/40002/chrome/browser/resource...
chrome/browser/resources/md_downloads/item.html:10: <link rel="import"
href="chrome://downloads/throttled_icon_loader.html">
Does anything use this now?

https://codereview.chromium.org/1375333004/diff/40002/chrome/browser/resource...
File chrome/browser/resources/md_downloads/item.js (right):

https://codereview.chromium.org/1375333004/diff/40002/chrome/browser/resource...
chrome/browser/resources/md_downloads/item.js:242: this.$['file-icon'].src =
'chrome://fileicon/' + filePath;
If you use camelCase you can avoid the brackets and quotes (don't need to fix
this here though)

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

https://codereview.chromium.org/1375333004/diff/40002/chrome/browser/resource...
chrome/browser/resources/md_downloads/manager.js:80: this.idMap_[id] = i;  //
Associated by ID for fast lookup.
I hate right side comments, but I guess this is normal in WebUI? It's usually
preferred to put it above the line in blink. :)

https://codereview.chromium.org/1375333004/diff/40002/chrome/browser/resource...
chrome/browser/resources/md_downloads/manager.js:113: this.set('items_.' +
this.idMap_[data.id] + '.item', data);
idMap is more like indexMap right?

https://codereview.chromium.org/1375333004/diff/40002/chrome/browser/ui/webui...
File chrome/browser/ui/webui/downloads_dom_handler.cc (right):

https://codereview.chromium.org/1375333004/diff/40002/chrome/browser/ui/webui...
chrome/browser/ui/webui/downloads_dom_handler.cc:67: return MdDownloadsEnabled()
? 300 : 150;
woah!

[esprehn, 2015-10-03 05:12:13]

Also you need to revulcanize after this patch?

[michaelpg, 2015-10-03 05:53:09]

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

[michaelpg, 2015-10-03 05:53:09]

lgtm

https://codereview.chromium.org/1375333004/diff/40002/chrome/browser/resource...
File chrome/browser/resources/md_downloads/item.js (right):

https://codereview.chromium.org/1375333004/diff/40002/chrome/browser/resource...
chrome/browser/resources/md_downloads/item.js:240: if (this.data &&
!this.isDangerous_) {
i thought this can only be called when this.data.file_path is not undefined, so
this.data is always truthy here

[Dan Beam, 2015-10-04 08:27:08]

https://codereview.chromium.org/1375333004/diff/40001/chrome/browser/resource...
File chrome/browser/resources/md_downloads/item.js (right):

https://codereview.chromium.org/1375333004/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_downloads/item.js:91:
'observeIsDangerous_(isDangerous_, data.file_path)',
On 2015/10/03 05:11:54, esprehn wrote:
> So dangerous!

Acknowledged.

https://codereview.chromium.org/1375333004/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_downloads/item.js:242: this.$['file-icon'].src =
'chrome://fileicon/' + filePath;
On 2015/10/03 05:11:54, esprehn wrote:
> If we make our ids camel case this is prettier. Not for  this patch but
> something to consider.

Acknowledged.

https://codereview.chromium.org/1375333004/diff/40002/chrome/browser/resource...
File chrome/browser/resources/md_downloads/item.html (left):

https://codereview.chromium.org/1375333004/diff/40002/chrome/browser/resource...
chrome/browser/resources/md_downloads/item.html:10: <link rel="import"
href="chrome://downloads/throttled_icon_loader.html">
On 2015/10/03 05:11:54, esprehn wrote:
> Does anything use this now?

the pre-Polymer page. I didn't want to move the file and make the diff twice as
big in this CL.

https://codereview.chromium.org/1375333004/diff/40002/chrome/browser/resource...
File chrome/browser/resources/md_downloads/item.js (right):

https://codereview.chromium.org/1375333004/diff/40002/chrome/browser/resource...
chrome/browser/resources/md_downloads/item.js:240: if (this.data &&
!this.isDangerous_) {
On 2015/10/03 05:53:09, michaelpg (OOO til 19th) wrote:
> i thought this can only be called when this.data.file_path is not undefined,
so
> this.data is always truthy here

not true with observers as stevenjb@ and dschuyler@ both found out. that !==
undefined logic only applies to templates afaik

https://codereview.chromium.org/1375333004/diff/40002/chrome/browser/resource...
chrome/browser/resources/md_downloads/item.js:242: this.$['file-icon'].src =
'chrome://fileicon/' + filePath;
On 2015/10/03 05:11:54, esprehn wrote:
> If you use camelCase you can avoid the brackets and quotes (don't need to fix
> this here though)

I know and we've discussed this previously but we've advocates for ids-like-this
for a few years before polymer so the signals are now a bit mixed. I believe I
removed the presubmit check for this but the style guide rule may still be
there.  I don't find it too horrible.

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

https://codereview.chromium.org/1375333004/diff/40002/chrome/browser/resource...
chrome/browser/resources/md_downloads/manager.js:80: this.idMap_[id] = i;  //
Associated by ID for fast lookup.
On 2015/10/03 05:11:54, esprehn wrote:
> I hate right side comments, but I guess this is normal in WebUI? It's usually
> preferred to put it above the line in blink. :)

JS/closure is fine with them

https://codereview.chromium.org/1375333004/diff/40002/chrome/browser/resource...
chrome/browser/resources/md_downloads/manager.js:113: this.set('items_.' +
this.idMap_[data.id] + '.item', data);
On 2015/10/03 05:11:54, esprehn wrote:
> idMap is more like indexMap right?

Done.

https://codereview.chromium.org/1375333004/diff/40002/chrome/browser/ui/webui...
File chrome/browser/ui/webui/downloads_dom_handler.cc (right):

https://codereview.chromium.org/1375333004/diff/40002/chrome/browser/ui/webui...
chrome/browser/ui/webui/downloads_dom_handler.cc:67: return MdDownloadsEnabled()
? 300 : 150;
On 2015/10/03 05:11:54, esprehn wrote:
> woah!

Acknowledged.

[Dan Beam, 2015-10-06 00:26:58]

btw, this CL is currently blocked on:
https://github.com/PolymerElements/iron-list/issues/116

[Dan Beam, 2015-10-07 00:54:23]

/cc blasten@ and tjsavage@

I tried Emmanuel's patch[1] and mid-list updates (i.e. in-progress downloads
finishing) now work well (thanks!).

there are still issues when a user starts or removes a download.  this is
because the current implementation just resets <iron-list>.items for simplicity.

commenting out this line makes our UI work as desired:
https://github.com/PolymerElements/iron-list/blob/master/iron-list.html#L836

but it's unclear to me what other polymer consumers expect to happen when
resetting all items (if it's a similar set, keep scroll?  if it's an empty set:
reset scroll?)

fwiw: I think the browser handles this decently well already (would preserve
scroll and clamp to smaller size if the scroll container gets smaller)

[1] https://github.com/PolymerElements/iron-list/pull/117

[Dan Beam, 2015-10-14 04:58:11]

Patchset #5 (id:90001) has been deleted

[Dan Beam, 2015-10-14 22:01:04]

so this current patchset handles all previously encountered functional issues
(that mainly deal with scroll position resetting).  BUT the resetScrollPosition
magic is real bad.

blasten@: how do we fix this?

https://codereview.chromium.org/1375333004/diff/110001/chrome/browser/resourc...
File chrome/browser/resources/md_downloads/manager.js (right):

https://codereview.chromium.org/1375333004/diff/110001/chrome/browser/resourc...
chrome/browser/resources/md_downloads/manager.js:95:
this.$['downloads-list']._resetScrollPosition = resetScrollPosition;
blasten@: don't make me do it!

[Dan Beam, 2015-10-15 03:28:20]

Patchset #6 (id:130001) has been deleted

[Dan Beam, 2015-10-15 03:28:30]

Patchset #4 (id:70001) has been deleted

[Dan Beam, 2015-10-15 03:36:29]

Patchset #5 (id:150001) has been deleted

[Dan Beam, 2015-10-15 03:39:21]

https://codereview.chromium.org/1375333004/diff/110001/chrome/browser/resourc...
File chrome/browser/resources/md_downloads/manager.js (right):

https://codereview.chromium.org/1375333004/diff/110001/chrome/browser/resourc...
chrome/browser/resources/md_downloads/manager.js:88: });
wrapper objects no longer needed

https://codereview.chromium.org/1375333004/diff/110001/chrome/browser/resourc...
chrome/browser/resources/md_downloads/manager.js:95:
this.$['downloads-list']._resetScrollPosition = resetScrollPosition;
On 2015/10/14 22:01:04, Dan Beam wrote:
> blasten@: don't make me do it!

so I found that *not* resetting the scroll position is arguably worse because of
some bug in iron-list that seems to just hide the whole list's contents when you
change all the items.

i'm just going to commit for now while it's a little broken and fix before we
ship

https://codereview.chromium.org/1375333004/diff/110001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/downloads_dom_handler.cc (right):

https://codereview.chromium.org/1375333004/diff/110001/chrome/browser/ui/webu...
chrome/browser/ui/webui/downloads_dom_handler.cc:67: return MdDownloadsEnabled()
? 300 : 150;
reverted this for now

[Dan Beam, 2015-10-15 04:39:32]

The CQ bit was checked by dbeam@chromium.org

[Dan Beam, 2015-10-15 04:39:32]

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

[commit-bot: I haz the power, 2015-10-15 04:39:52]

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

[commit-bot: I haz the power, 2015-10-15 04:40:51]

Committed patchset #5 (id:170001)

[commit-bot: I haz the power, 2015-10-15 04:41:56]

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