downloads: break downloads.js into more classes/files.

downloads: break downloads.js into more classes/files.

chrome://downloads' code has gotten less flexible than we'd like over
the years and needs a little TLC.

This CL heavily refactors chrome://downloads code by ripping apart
downloads.js into:

  downloads.{
    Item: a light data wrapper that has-an ItemView
    ItemView: responsible for beautiful downloads
    Manager: owns and manages all the items
  }

as well as moving large portions of programmatic DOM creation (i.e. tons
of document.createElement() calls) into an HTML template that's cloned.

The JS refactor loosens the coupling between various layers, more
clearly show responsibilities, and makes the code easier to understand,
test, and maintain (in theory).

The new way C++ => JS communication works is:

- if an item will be hidden or shown (e.g. remove, undo, new)
 - C++ sends a whole new list to the JS
 - the JS is now much smarter about re-using DOM nodes
  - this preserves focus and has better performance

- if an item is simply updated (e.g. downloading in progress)
 - C++ sends an update to that specific item

R=asanka@chromium.org
BUG=446412
Committed: https://crrev.com/85c004102bdcf0e5d741559b2235fdcb9d4aaa19
Cr-Commit-Position: refs/heads/master@{#320204}

[Dan Beam, 2015-03-03 04:15:01]

let me know if you like this approach first, then i'll test

[asanka, 2015-03-03 18:11:39]

I'm good with this. You should remove the sorting done at
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...
since order or sent downloads wouldn't matter after this CL. Perhaps toss in a
comment there that the WebUI will take care of display order, as it should.

[Dan Beam, 2015-03-03 18:58:04]

On 2015/03/03 18:11:39, asanka wrote:
> I'm good with this. You should remove the sorting done at
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...
> since order or sent downloads wouldn't matter after this CL. Perhaps toss in a
> comment there that the WebUI will take care of display order, as it should.

If we limit without sorting we probably wont get the freshest items.  So we'd
have to remove the limit as well... :/.

I'm starting to think we should just blow away the whole list every time it's
modified.  The current code doesn't respect kMaxDownloads correctly, I think
(likely my fault).

Assume a user has 200 downloads and kMaxDownloads = 100.  If the user clears the
100 showing items, 100 more items show up, and the user presses Ctrl+z => we're
likely to end up with 200 items (100 showing + 100 updated).

[Dan Beam, 2015-03-03 19:31:05]

https://codereview.chromium.org/977473002/diff/1/chrome/browser/resources/dow...
File chrome/browser/resources/downloads/downloads.js (right):

https://codereview.chromium.org/977473002/diff/1/chrome/browser/resources/dow...
chrome/browser/resources/downloads/downloads.js:213:
this.node_.insertBefore(this.downloads_[id].node, before && before.node);
i could possibly do this:

  if (startSorted.length > MAX_DOWNLOADS)
    this.remove(startSorted.pop().id_);

but it's not crazy efficient in the Clear All + undo case

[asanka, 2015-03-03 20:43:46]

On 2015/03/03 18:58:04, Dan Beam wrote:
> On 2015/03/03 18:11:39, asanka wrote:
> > I'm good with this. You should remove the sorting done at
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...
> > since order or sent downloads wouldn't matter after this CL. Perhaps toss in
a
> > comment there that the WebUI will take care of display order, as it should.
> 
> If we limit without sorting we probably wont get the freshest items.  So we'd
> have to remove the limit as well... :/.
> 
> I'm starting to think we should just blow away the whole list every time it's
> modified.  The current code doesn't respect kMaxDownloads correctly, I think
> (likely my fault).
> 
> Assume a user has 200 downloads and kMaxDownloads = 100.  If the user clears
the
> 100 showing items, 100 more items show up, and the user presses Ctrl+z =>
we're
> likely to end up with 200 items (100 showing + 100 updated).

Of course you are right. I glanced over at Download.HistorySize2 (which needs
some massaging before the data can be interpreted) and it looks like a
substantial number of users are over the 150 download limit. This really needs
to be reworked.

In the meantime, how about something like this?

- Make OnDownloadUpdated() do nothing if update_scheduled_ is true. We are going
to send all the downloads anyway, and hence there isn't much point in sending an
individual download.

- Make Undo() always schedule an update prior to modifying any downloads.

The outcome of this combination should be to send the entire list of downloads
after an undo of a removal. It's excessive, but has the correct outcome + we are
already sending the whole list whenever a download is removed. Due
kMaxDownloads, we'd only be sending 150 downloads over at worst.

One of the things I've been thinking about is to avoid keeping all the downloads
in memory all the time. It might be worth considering how the downloads history
WebUI could be driven by the history database directly rather than having to
keep all downloads in memory for the entire browser session. While the page is
open, it'll need to keep track of all the downloads, but that'll consume
renderer memory only while the page is running.

[asanka, 2015-03-03 20:45:56]

https://codereview.chromium.org/977473002/diff/1/chrome/browser/resources/dow...
File chrome/browser/resources/downloads/downloads.js (right):

https://codereview.chromium.org/977473002/diff/1/chrome/browser/resources/dow...
chrome/browser/resources/downloads/downloads.js:213:
this.node_.insertBefore(this.downloads_[id].node, before && before.node);
On 2015/03/03 19:31:05, Dan Beam wrote:
> i could possibly do this:
> 
>   if (startSorted.length > MAX_DOWNLOADS)
>     this.remove(startSorted.pop().id_);
> 
> but it's not crazy efficient in the Clear All + undo case

Yeah. For undo operations that affect multiple downloads, we'd probably be
better off sending the whole list. I've suggested that we do so for single
downloads as well and be done with it. If that's too excessive, then maybe we
can consider something like this for the single download case.

[Dan Beam, 2015-03-04 00:46:20]

https://codereview.chromium.org/977473002/diff/1/chrome/browser/resources/dow...
File chrome/browser/resources/downloads/downloads.js (right):

https://codereview.chromium.org/977473002/diff/1/chrome/browser/resources/dow...
chrome/browser/resources/downloads/downloads.js:213:
this.node_.insertBefore(this.downloads_[id].node, before && before.node);
On 2015/03/03 20:45:56, asanka wrote:
> On 2015/03/03 19:31:05, Dan Beam wrote:
> > i could possibly do this:
> > 
> >   if (startSorted.length > MAX_DOWNLOADS)
> >     this.remove(startSorted.pop().id_);
> > 
> > but it's not crazy efficient in the Clear All + undo case
> 
> Yeah. For undo operations that affect multiple downloads, we'd probably be
> better off sending the whole list. I've suggested that we do so for single
> downloads as well and be done with it. If that's too excessive, then maybe we
> can consider something like this for the single download case.

the problem with rebuilding DOM is that it messes up focus.  we recently fixed
this on chrome://extensions and it'd be a pretty huge bummer to regress it here.

[Dan Beam, 2015-03-05 07:24:58]

Patchset #2 (id:20001) has been deleted

[Dan Beam, 2015-03-05 07:30:29]

Patchset #2 (id:40001) has been deleted

[Dan Beam, 2015-03-05 19:15:16]

Patchset #3 (id:80001) has been deleted

[Dan Beam, 2015-03-05 19:15:22]

Patchset #2 (id:60001) has been deleted

[Dan Beam, 2015-03-05 19:29:42]

Patchset #2 (id:100001) has been deleted

[Dan Beam, 2015-03-05 19:54:37]

Patchset #2 (id:120001) has been deleted

[Dan Beam, 2015-03-05 23:31:20]

Patchset #2 (id:140001) has been deleted

[Dan Beam, 2015-03-05 23:40:49]

Patchset #2 (id:160001) has been deleted

[Dan Beam, 2015-03-06 02:47:05]

ok, things seem to be working OK to the best of my knowledge.  i'll try this on
windows, soon, but probably OK to start reviewing.  fwiw: I did hack the JS to
say a download was state='DANGER' and DangerType=DANGEROUS_HOST to show the
warning and that code is pretty simple and similar to the dangerous file UI
(which I was able to get working).

[Dan Beam, 2015-03-09 16:43:15]

ping asanka

[asanka, 2015-03-10 04:15:51]

https://codereview.chromium.org/977473002/diff/310001/chrome/app/generated_re...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/977473002/diff/310001/chrome/app/generated_re...
chrome/app/generated_resources.grd:2064: +        Downloaded by &lt;a href="<ph
name="EXTENSION_URL">$1</ph>"&gt;<ph name="EXTENSION">$2<ex>The Best Chrome
Extension Ever</ex></ph>&lt;/a&gt;
Note to self: Make sure EXTENSION_URL is adequately sanitized.

https://codereview.chromium.org/977473002/diff/310001/chrome/browser/resource...
File chrome/browser/resources/downloads/externs.js (right):

https://codereview.chromium.org/977473002/diff/310001/chrome/browser/resource...
chrome/browser/resources/downloads/externs.js:1: // Copyright 2014 The Chromium
Authors. All rights reserved.
2015

https://codereview.chromium.org/977473002/diff/310001/chrome/browser/resource...
chrome/browser/resources/downloads/externs.js:6: * @fileoverview Externs for
objects send from C++ to JS for chrome://downloads.
s/send/sent/

https://codereview.chromium.org/977473002/diff/310001/chrome/browser/resource...
File chrome/browser/resources/downloads/focus_row.js (right):

https://codereview.chromium.org/977473002/diff/310001/chrome/browser/resource...
chrome/browser/resources/downloads/focus_row.js:75:
element.setAttribute('column-type', type);
Rather than using [column-type], can we code the column type into the template?

https://codereview.chromium.org/977473002/diff/310001/chrome/browser/resource...
chrome/browser/resources/downloads/focus_row.js:85: shouldFocus_:
function(element) {
Doesn't cr.ui.FocusRow deal with visibility?

https://codereview.chromium.org/977473002/diff/310001/chrome/browser/resource...
File chrome/browser/resources/downloads/item.js (right):

https://codereview.chromium.org/977473002/diff/310001/chrome/browser/resource...
chrome/browser/resources/downloads/item.js:50: delete this.view;
seems 'this.view = null' is preferred?

https://codereview.chromium.org/977473002/diff/310001/chrome/browser/resource...
File chrome/browser/resources/downloads/item_view.js (right):

https://codereview.chromium.org/977473002/diff/310001/chrome/browser/resource...
chrome/browser/resources/downloads/item_view.js:145: if (img.src == request.url)
This is probably OK, but Blink does (or didn't) guarantee that the src of an
HTMLImageElement would be the same as the one that was written. I.e.:

   img.src = url;
   if (img.src == url) {
     // not guaranteed to get here.
   }

https://codereview.chromium.org/977473002/diff/310001/chrome/browser/resource...
chrome/browser/resources/downloads/item_view.js:163: update: function(data) {
Shall we break this up?

https://codereview.chromium.org/977473002/diff/310001/chrome/browser/resource...
chrome/browser/resources/downloads/item_view.js:205: var isInterrupted =
data.state == Item.States.INTERRUPTED;
@const for consistency

https://codereview.chromium.org/977473002/diff/310001/chrome/browser/resource...
chrome/browser/resources/downloads/item_view.js:226: var controlledByExtension =
data.by_ext_id && data.by_ext_name;
@const

https://codereview.chromium.org/977473002/diff/310001/chrome/browser/resource...
chrome/browser/resources/downloads/item_view.js:349: return
loadTimeData.getString('status_paused');
For paused downloads, we should still display the progress so far. This is
probably a change in behavior from what chrome://downloads does now.

https://codereview.chromium.org/977473002/diff/310001/chrome/browser/resource...
chrome/browser/resources/downloads/item_view.js:350: case Item.States.DANGEROUS:
should be a notreached.

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

https://codereview.chromium.org/977473002/diff/310001/chrome/browser/ui/webui...
chrome/browser/ui/webui/downloads_dom_handler.cc:327:
new_downloads_.insert(download_item->GetId());
If OnDownloadCreated() is called for a download restored from history (e.g.
because the history DB is slow and chrome://downloads was opened before history
could be read), then it's possible there won't be another OnDownloadUpdated() to
cause the downloads to be sent.

https://codereview.chromium.org/977473002/diff/310001/chrome/browser/ui/webui...
chrome/browser/ui/webui/downloads_dom_handler.cc:341: SendCurrentDownloads();
If we go this route, then a subsequent CallUpdateItem() would be redundant.

https://codereview.chromium.org/977473002/diff/310001/chrome/browser/ui/webui...
chrome/browser/ui/webui/downloads_dom_handler.cc:346:
!IsDownloadDisplayable(*download_item)) {
IsDownloadDisplayable() could be returning false for a download that's never
supposed to be shown in our UI (e.g. it's an app download). If new_downloads_
were to be emptied, then ScheduleSendCurrentDownloads() would be called on each
update of such a download.

https://codereview.chromium.org/977473002/diff/310001/chrome/browser/ui/webui...
chrome/browser/ui/webui/downloads_dom_handler.cc:491:
model.SetShouldShowInShelf(true);
Why was this necessary?

https://codereview.chromium.org/977473002/diff/310001/chrome/browser/ui/webui...
chrome/browser/ui/webui/downloads_dom_handler.cc:492:
model.SetIsBeingRevived(true);
It seems the only purpose of SetIsBeingRevived() is to suppress CallUpdateItem()
and instead call ScheduleSendCurrentDownloads(). That could be accomplished by
pre-emptively calling ScheduleSendCurrentDownloads() when undoing a deletion and
checking for update_scheduled_ in OnDownloadUpdated(). CallUpdateItem() is
redudant if there's a scheduled update, so that'll be something we should
consider doing anyway.

https://codereview.chromium.org/977473002/diff/310001/chrome/browser/ui/webui...
chrome/browser/ui/webui/downloads_dom_handler.cc:610: for (auto it =
filtered_items.begin(); it != filtered_items.end(); ++it) {
Use a range based for instead.

[Dan Beam, 2015-03-10 05:34:27]

Patchset #10 (id:330001) has been deleted

[Dan Beam, 2015-03-10 05:41:03]

https://codereview.chromium.org/977473002/diff/310001/chrome/app/generated_re...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/977473002/diff/310001/chrome/app/generated_re...
chrome/app/generated_resources.grd:2064: +        Downloaded by &lt;a href="<ph
name="EXTENSION_URL">$1</ph>"&gt;<ph name="EXTENSION">$2<ex>The Best Chrome
Extension Ever</ex></ph>&lt;/a&gt;
On 2015/03/10 04:15:50, asanka wrote:
> Note to self: Make sure EXTENSION_URL is adequately sanitized.

Done.

https://codereview.chromium.org/977473002/diff/310001/chrome/browser/resource...
File chrome/browser/resources/downloads/externs.js (right):

https://codereview.chromium.org/977473002/diff/310001/chrome/browser/resource...
chrome/browser/resources/downloads/externs.js:1: // Copyright 2014 The Chromium
Authors. All rights reserved.
On 2015/03/10 04:15:50, asanka wrote:
> 2015

Done.

https://codereview.chromium.org/977473002/diff/310001/chrome/browser/resource...
chrome/browser/resources/downloads/externs.js:6: * @fileoverview Externs for
objects send from C++ to JS for chrome://downloads.
On 2015/03/10 04:15:50, asanka wrote:
> s/send/sent/

Done.

https://codereview.chromium.org/977473002/diff/310001/chrome/browser/resource...
File chrome/browser/resources/downloads/focus_row.js (right):

https://codereview.chromium.org/977473002/diff/310001/chrome/browser/resource...
chrome/browser/resources/downloads/focus_row.js:75:
element.setAttribute('column-type', type);
On 2015/03/10 04:15:51, asanka wrote:
> Rather than using [column-type], can we code the column type into the
template?

i copied the code from here:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res...

i understand we don't always get a chance to change code that's already in
place, so this is a good time to re-evaluate.  but a) hcarmona@ is more familiar
with this code, and b) I'd really like to do this separately if possible -- this
CL is a pain for you to review because it's large and complex [enough already].

https://codereview.chromium.org/977473002/diff/310001/chrome/browser/resource...
chrome/browser/resources/downloads/focus_row.js:85: shouldFocus_:
function(element) {
On 2015/03/10 04:15:50, asanka wrote:
> Doesn't cr.ui.FocusRow deal with visibility?

no,
https://code.google.com/p/chromium/codesearch#chromium/src/ui/webui/resources...

https://codereview.chromium.org/977473002/diff/310001/chrome/browser/resource...
File chrome/browser/resources/downloads/item.js (right):

https://codereview.chromium.org/977473002/diff/310001/chrome/browser/resource...
chrome/browser/resources/downloads/item.js:50: delete this.view;
On 2015/03/10 04:15:51, asanka wrote:
> seems 'this.view = null' is preferred?

eh, i've seen things that talk about delete not being optimized but i haven't
actually seen much evidence of this...

the difference is that `delete this.view` actually clears the enumerable
property, so

  >> this.view = null
  >> 'view' in this
  true
  >> delete this.view;
  >> 'view' in this
  false

anyways, moved to null

https://codereview.chromium.org/977473002/diff/310001/chrome/browser/resource...
File chrome/browser/resources/downloads/item_view.js (right):

https://codereview.chromium.org/977473002/diff/310001/chrome/browser/resource...
chrome/browser/resources/downloads/item_view.js:145: if (img.src == request.url)
On 2015/03/10 04:15:51, asanka wrote:
> This is probably OK, but Blink does (or didn't) guarantee that the src of an
> HTMLImageElement would be the same as the one that was written. I.e.:
> 
>    img.src = url;
>    if (img.src == url) {
>      // not guaranteed to get here.
>    }

unlikely to be problematic in our case, but I see what you're saying.

  el.style.color = 'red';
  if (el.style.color == 'red') {
    // never get here, .color gives back rgb(255, 0, 0) or something
  }

fwiw: basically copied from here
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res...

https://codereview.chromium.org/977473002/diff/310001/chrome/browser/resource...
chrome/browser/resources/downloads/item_view.js:163: update: function(data) {
On 2015/03/10 04:15:51, asanka wrote:
> Shall we break this up?

the reason I didn't is because when there's a separate method:
1) people think it might be called from multiple places (or should be)
2) you have to re-verify some state (e.g. assert(somethingThatsAlreadyTrue); at
the top or maybe recreate var isSomethingHelper = ...)

the current code does this (updateDangerousFile) and i felt it was kind of
confusing.

https://codereview.chromium.org/977473002/diff/310001/chrome/browser/resource...
chrome/browser/resources/downloads/item_view.js:205: var isInterrupted =
data.state == Item.States.INTERRUPTED;
On 2015/03/10 04:15:51, asanka wrote:
> @const for consistency

Done.

https://codereview.chromium.org/977473002/diff/310001/chrome/browser/resource...
chrome/browser/resources/downloads/item_view.js:226: var controlledByExtension =
data.by_ext_id && data.by_ext_name;
On 2015/03/10 04:15:51, asanka wrote:
> @const

Done.

https://codereview.chromium.org/977473002/diff/310001/chrome/browser/resource...
chrome/browser/resources/downloads/item_view.js:349: return
loadTimeData.getString('status_paused');
On 2015/03/10 04:15:51, asanka wrote:
> For paused downloads, we should still display the progress so far. This is
> probably a change in behavior from what chrome://downloads does now.

chrome://downloads currently shows "Paused": http://i.imgur.com/JxYEvP4.png

do you want to change the behavior?  this is the only place we use this string
(and it's added to the data source for a reason).

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res...
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...

https://codereview.chromium.org/977473002/diff/310001/chrome/browser/resource...
chrome/browser/resources/downloads/item_view.js:350: case Item.States.DANGEROUS:
On 2015/03/10 04:15:51, asanka wrote:
> should be a notreached.

copied from here, but I assume the code has changed
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res...

Done.

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

https://codereview.chromium.org/977473002/diff/310001/chrome/browser/ui/webui...
chrome/browser/ui/webui/downloads_dom_handler.cc:327:
new_downloads_.insert(download_item->GetId());
On 2015/03/10 04:15:51, asanka wrote:
> If OnDownloadCreated() is called for a download restored from history (e.g.
> because the history DB is slow and chrome://downloads was opened before
history
> could be read), then it's possible there won't be another OnDownloadUpdated()
to
> cause the downloads to be sent.

added back the original code

https://codereview.chromium.org/977473002/diff/310001/chrome/browser/ui/webui...
chrome/browser/ui/webui/downloads_dom_handler.cc:341: SendCurrentDownloads();
On 2015/03/10 04:15:51, asanka wrote:
> If we go this route, then a subsequent CallUpdateItem() would be redundant.

Done.

https://codereview.chromium.org/977473002/diff/310001/chrome/browser/ui/webui...
chrome/browser/ui/webui/downloads_dom_handler.cc:346:
!IsDownloadDisplayable(*download_item)) {
On 2015/03/10 04:15:51, asanka wrote:
> IsDownloadDisplayable() could be returning false for a download that's never
> supposed to be shown in our UI (e.g. it's an app download). If new_downloads_
> were to be emptied, then ScheduleSendCurrentDownloads() would be called on
each
> update of such a download.

this matches what the current code does

https://codereview.chromium.org/977473002/diff/310001/chrome/browser/ui/webui...
chrome/browser/ui/webui/downloads_dom_handler.cc:491:
model.SetShouldShowInShelf(true);
On 2015/03/10 04:15:51, asanka wrote:
> Why was this necessary?

DownloadsDOMHandler::Handle{Remove,ClearAll}() run
DownloadItemModel(item).SetShouldShowInShelf(false) on each item to ensure
IsDownloadDisplayable(*item) returns false.

when the removal is undone, this needs to be reversed so the item is visible
again.

https://codereview.chromium.org/977473002/diff/310001/chrome/browser/ui/webui...
chrome/browser/ui/webui/downloads_dom_handler.cc:492:
model.SetIsBeingRevived(true);
On 2015/03/10 04:15:51, asanka wrote:
> It seems the only purpose of SetIsBeingRevived() is to suppress
CallUpdateItem()
> and instead call ScheduleSendCurrentDownloads(). That could be accomplished by
> pre-emptively calling ScheduleSendCurrentDownloads() when undoing a deletion
and
> checking for update_scheduled_ in OnDownloadUpdated().

that would work with only 1 handler.

ALL handlers need to know about this state, which is why we decided to put it in
the model.

> CallUpdateItem() is
> redudant if there's a scheduled update, so that'll be something we should
> consider doing anyway.

that's fixed now

https://codereview.chromium.org/977473002/diff/310001/chrome/browser/ui/webui...
chrome/browser/ui/webui/downloads_dom_handler.cc:610: for (auto it =
filtered_items.begin(); it != filtered_items.end(); ++it) {
On 2015/03/10 04:15:51, asanka wrote:
> Use a range based for instead.

Done.

[Dan Beam, 2015-03-11 15:20:46]

ping asanka@

[asanka, 2015-03-11 19:57:50]

LGTM. The comments are minor and can be postponed to follow ups given the size
of this CL. Thanks! and apologies for the delay.

https://codereview.chromium.org/977473002/diff/310001/chrome/browser/resource...
File chrome/browser/resources/downloads/focus_row.js (right):

https://codereview.chromium.org/977473002/diff/310001/chrome/browser/resource...
chrome/browser/resources/downloads/focus_row.js:75:
element.setAttribute('column-type', type);
On 2015/03/10 05:41:02, Dan Beam wrote:
> On 2015/03/10 04:15:51, asanka wrote:
> > Rather than using [column-type], can we code the column type into the
> template?
> 
> i copied the code from here:
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res...
> 
> i understand we don't always get a chance to change code that's already in
> place, so this is a good time to re-evaluate.  but a) hcarmona@ is more
familiar
> with this code, and b) I'd really like to do this separately if possible --
this
> CL is a pain for you to review because it's large and complex [enough
already].

SG.

https://codereview.chromium.org/977473002/diff/310001/chrome/browser/resource...
File chrome/browser/resources/downloads/item.js (right):

https://codereview.chromium.org/977473002/diff/310001/chrome/browser/resource...
chrome/browser/resources/downloads/item.js:50: delete this.view;
On 2015/03/10 05:41:02, Dan Beam wrote:
> On 2015/03/10 04:15:51, asanka wrote:
> > seems 'this.view = null' is preferred?
> 
> eh, i've seen things that talk about delete not being optimized but i haven't
> actually seen much evidence of this...
> 
> the difference is that `delete this.view` actually clears the enumerable
> property, so
> 
>   >> this.view = null
>   >> 'view' in this
>   true
>   >> delete this.view;
>   >> 'view' in this
>   false
> 
> anyways, moved to null

Cool. I'm no expert on this, and I'm okay with either. I was pointing out the
style guide preference.

https://codereview.chromium.org/977473002/diff/310001/chrome/browser/resource...
File chrome/browser/resources/downloads/item_view.js (right):

https://codereview.chromium.org/977473002/diff/310001/chrome/browser/resource...
chrome/browser/resources/downloads/item_view.js:163: update: function(data) {
On 2015/03/10 05:41:02, Dan Beam wrote:
> On 2015/03/10 04:15:51, asanka wrote:
> > Shall we break this up?
> 
> the reason I didn't is because when there's a separate method:
> 1) people think it might be called from multiple places (or should be)
> 2) you have to re-verify some state (e.g. assert(somethingThatsAlreadyTrue);
at
> the top or maybe recreate var isSomethingHelper = ...)
> 
> the current code does this (updateDangerousFile) and i felt it was kind of
> confusing.

Ok

https://codereview.chromium.org/977473002/diff/310001/chrome/browser/resource...
chrome/browser/resources/downloads/item_view.js:349: return
loadTimeData.getString('status_paused');
On 2015/03/10 05:41:02, Dan Beam wrote:
> On 2015/03/10 04:15:51, asanka wrote:
> > For paused downloads, we should still display the progress so far. This is
> > probably a change in behavior from what chrome://downloads does now.
> 
> chrome://downloads currently shows "Paused": http://i.imgur.com/JxYEvP4.png
> 
> do you want to change the behavior?  this is the only place we use this string
> (and it's added to the data source for a reason).
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res...
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...

I'd like to change the behavior since it's one of the areas where the UI in
chrome://downloads differs from the shelf. The shelf clearly indicates how far
along the download is if it's paused, while the page just says "Paused" with no
further information.

However, I think we should limit this CL a code refactor and push any behavior
changes to separate CLs.

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

https://codereview.chromium.org/977473002/diff/310001/chrome/browser/ui/webui...
chrome/browser/ui/webui/downloads_dom_handler.cc:346:
!IsDownloadDisplayable(*download_item)) {
On 2015/03/10 05:41:03, Dan Beam wrote:
> On 2015/03/10 04:15:51, asanka wrote:
> > IsDownloadDisplayable() could be returning false for a download that's never
> > supposed to be shown in our UI (e.g. it's an app download). If
new_downloads_
> > were to be emptied, then ScheduleSendCurrentDownloads() would be called on
> each
> > update of such a download.
> 
> this matches what the current code does

True, but it's still going to be quite expensive to send all the downloads on
every update. We'd also run into this case if chrome://downloads is opened while
an app download is in progress.

If it's not going to be addressed here, we should make a note of it to address
this later.

https://codereview.chromium.org/977473002/diff/350001/chrome/browser/resource...
File chrome/browser/resources/downloads/downloads.html (right):

https://codereview.chromium.org/977473002/diff/350001/chrome/browser/resource...
chrome/browser/resources/downloads/downloads.html:77:
i18n-values=".innerHTML:control_by_extension"></span>
Why does control_by_extension contain HTML? Can we make that HTML be a part of
this template?

[Dan Beam, 2015-03-11 21:50:29]

The CQ bit was checked by dbeam@chromium.org

[commit-bot: I haz the power, 2015-03-11 21:51:41]

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

[Dan Beam, 2015-03-11 21:55:06]

addressing the rest in a followup right now

https://codereview.chromium.org/977473002/diff/350001/chrome/browser/resource...
File chrome/browser/resources/downloads/downloads.html (right):

https://codereview.chromium.org/977473002/diff/350001/chrome/browser/resource...
chrome/browser/resources/downloads/downloads.html:77:
i18n-values=".innerHTML:control_by_extension"></span>
On 2015/03/11 19:57:50, asanka wrote:
> Why does control_by_extension contain HTML? Can we make that HTML be a part of
> this template?

generally the guidance I've gotten is to simply embed HTML into the translations
so the translators have sufficient context and can move the link where is
natural in the text.

[commit-bot: I haz the power, 2015-03-11 22:28:44]

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

[commit-bot: I haz the power, 2015-03-11 22:28:45]

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-03-11 22:46:11]

dbeam@chromium.org changed reviewers:
+ thestig@chromium.org

[Dan Beam, 2015-03-11 22:46:12]

+thestig@ for chrome/browser/browser_resources.grd

[Lei Zhang, 2015-03-11 23:01:38]

https://codereview.chromium.org/977473002/diff/350001/chrome/browser/browser_...
File chrome/browser/browser_resources.grd (right):

https://codereview.chromium.org/977473002/diff/350001/chrome/browser/browser_...
chrome/browser/browser_resources.grd:134: <include
name="IDR_DOWNLOAD_FOCUS_ROW_JS" file="resources\downloads\focus_row.js"
type="BINDATA" />
Alphabetical order? What about externs.js?

[Dan Beam, 2015-03-11 23:38:34]

https://codereview.chromium.org/977473002/diff/350001/chrome/browser/browser_...
File chrome/browser/browser_resources.grd (right):

https://codereview.chromium.org/977473002/diff/350001/chrome/browser/browser_...
chrome/browser/browser_resources.grd:134: <include
name="IDR_DOWNLOAD_FOCUS_ROW_JS" file="resources\downloads\focus_row.js"
type="BINDATA" />
On 2015/03/11 23:01:38, Lei Zhang wrote:
> Alphabetical order? What about externs.js?

whoops.  Done.  (they were in dependency order... but that's not particularly
helpful.)

externs.js isn't necessary (it's just for compiler)

[Lei Zhang, 2015-03-11 23:47:44]

lgtm

[Dan Beam, 2015-03-11 23:48:26]

The CQ bit was checked by dbeam@chromium.org

[Dan Beam, 2015-03-11 23:48:27]

The patchset sent to the CQ was uploaded after l-g-t-m from asanka@chromium.org
Link to the patchset: https://codereview.chromium.org/977473002/#ps370001
(title: "thestig@ review")

[commit-bot: I haz the power, 2015-03-11 23:49:14]

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

[commit-bot: I haz the power, 2015-03-12 01:33:46]

Committed patchset #11 (id:370001)

[commit-bot: I haz the power, 2015-03-12 01:34:15]

Patchset 11 (id:??) landed as
https://crrev.com/85c004102bdcf0e5d741559b2235fdcb9d4aaa19
Cr-Commit-Position: refs/heads/master@{#320204}