[Resumption 10/12] Use DI::IsDone to check for terminal downloads.

Use DownloadItem::IsDone to check for terminal downloads.

When downloads can be resumed, a download transitioning to INTERRUPTED
state is not necessarily entering a terminal state. Therefore, a
download being in IN_PROGRESS state is not a sufficient indicator of
whether a download may eventually complete. Use DownloadItem::IsDone()
to indicate this.

TBR=estade
BUG=7648
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=204526

[asanka, 2013-06-05 15:57:47]

I'll update https://codereview.chromium.org/14955004/ to depend on this.

[Randy Smith (Not in Mondays), 2013-06-05 20:21:44]

LGTM (though if the open state isn't persisted, that should go onto your list.)

https://codereview.chromium.org/16007017/diff/6001/chrome/browser/download/do...
File chrome/browser/download/download_shelf_context_menu.cc (right):

https://codereview.chromium.org/16007017/diff/6001/chrome/browser/download/do...
chrome/browser/download/download_shelf_context_menu.cc:186: if (download_item_
&& download_item_->IsPartialDownload())
Do we persist the open state across browser restarts?

https://codereview.chromium.org/16007017/diff/6001/content/public/browser/dow...
File content/public/browser/download_item.h (right):

https://codereview.chromium.org/16007017/diff/6001/content/public/browser/dow...
content/public/browser/download_item.h:177: virtual bool IsPartialDownload()
const = 0;
Just raising the question: Do we want to keep this name for the method? 
IsPartialDownload() implies to me a method that has to do with how much of the
download has completed.  Maybe we want something more like "IsDone()" or
"IsTerminal()"?  (And reverse all the tests, obviously.)  Your call.

[Randy Smith (Not in Mondays), 2013-06-05 20:22:21]

Whoops, one nit.  The issue description says "will eventually complete".  I'd
suggest instead "may eventually complete"; we don't actually know what's going
to happen in the future.

[asanka, 2013-06-05 21:33:47]

+joi for content/public

https://codereview.chromium.org/16007017/diff/6001/chrome/browser/download/do...
File chrome/browser/download/download_shelf_context_menu.cc (right):

https://codereview.chromium.org/16007017/diff/6001/chrome/browser/download/do...
chrome/browser/download/download_shelf_context_menu.cc:186: if (download_item_
&& download_item_->IsPartialDownload())
On 2013/06/05 20:21:44, rdsmith wrote:
> Do we persist the open state across browser restarts?

We persist the "opened" state, but not the "will open when complete" state.
Added to the list, but I think it's going to be very low priority.

https://codereview.chromium.org/16007017/diff/6001/content/public/browser/dow...
File content/public/browser/download_item.h (right):

https://codereview.chromium.org/16007017/diff/6001/content/public/browser/dow...
content/public/browser/download_item.h:177: virtual bool IsPartialDownload()
const = 0;
On 2013/06/05 20:21:44, rdsmith wrote:
> Just raising the question: Do we want to keep this name for the method? 
> IsPartialDownload() implies to me a method that has to do with how much of the
> download has completed.  Maybe we want something more like "IsDone()" or
> "IsTerminal()"?  (And reverse all the tests, obviously.)  Your call.

Changed to IsDone().

[Jói, 2013-06-06 11:41:48]

//content/public LGTM

[commit-bot: I haz the power, 2013-06-06 14:48:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/asanka@chromium.org/16007017/23001

[commit-bot: I haz the power, 2013-06-06 14:55:39]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[asanka, 2013-06-06 14:59:03]

TBR=estade for chrome/browser/ui/webui

[commit-bot: I haz the power, 2013-06-06 14:59:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/asanka@chromium.org/16007017/23001

[commit-bot: I haz the power, 2013-06-06 16:54:44]

Change committed as 204526