[Resumption 11/12] Support resuming interrupted downloads from the downloads shelf.

[Downloads] Support resuming interrupted downloads from the downloads shelf.

The download shelf context menu will have a 'Resume' item for
interrupted downloads. This option is available if
'--enable-download-resumption' flag is enabled.

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

[Randy Smith (Not in Mondays), 2013-06-03 21:48:40]

Two high level thoughts:
* I don't see any dependencies on any flag; is there something I'm not
understanding?
* Do we want to both have toggle_pause and resume as separate concepts?  It
seems like it would be better to separate out "Pause" and "Resume" all the way
up the stack?  (If you'd prefer not, I'm happy to go with your preference, but I
want to at least raise the question.)

[asanka, 2013-06-03 22:35:39]

On 2013/06/03 21:48:40, rdsmith wrote:
> Two high level thoughts:
> * I don't see any dependencies on any flag; is there something I'm not
> understanding?

The flag determines the outcome of DownloadItem::CanResume() which was being
relied on in patchset 1. However, if the flag is absent, the resulting context
menu for an interrupted download was still different. The old behavior was
restored in patchset 2.

Items in "[]" are disabled.

Windows (before or (after without flag)):
 Learn more

Windows (after with flag):
 Resume
 Learn more
 ---
 Cancel

non-Windows (before or (after without flag)):
 [Open]
 [Always open type]
 ---
 [Pause]
 [Show in folder]
 ---
 [Cancel]

non-Windows (after with flag):
 Resume
 ---
 Cancel

 * Do we want to both have toggle_pause and resume as separate concepts?  It
> seems like it would be better to separate out "Pause" and "Resume" all the way
> up the stack?  (If you'd prefer not, I'm happy to go with your preference, but
I
> want to at least raise the question.)

The TOGGLE_PAUSE option allows us to use the same menu definition for an
in-progress download regardless of whether the download is paused or not. The
text for the menu is determined at the time the menu is displayed ("pause" or
"resume").

[Randy Smith (Not in Mondays), 2013-06-04 17:08:59]

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

https://codereview.chromium.org/14955004/diff/6002/chrome/browser/download/do...
chrome/browser/download/download_shelf_context_menu.cc:88: return
download_item_->IsPartialDownload() || download_item_->CanResume();
Why not IsInterrupted() instead of CanResume()?  Seems more accurate to my
uneducated eye.

https://codereview.chromium.org/14955004/diff/6002/chrome/browser/download/do...
chrome/browser/download/download_shelf_context_menu.cc:92: return
download_item_->CanResume();
This is true for IN_PROGRESS downloads as well.  Won't that mean that both the
TOGGLE_PAUSE and RESUME_INTERRUPTED menus show up in the shelf for IN_PROGRESS
downloads?

https://codereview.chromium.org/14955004/diff/6002/chrome/browser/download/do...
chrome/browser/download/download_shelf_context_menu.cc:158:
download_item_->Resume();
Any reason not to just unilaterally resume?  It looks like the resume code is a
no-op in the cases you care about.

[asanka, 2013-06-04 18:52:38]

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

https://codereview.chromium.org/14955004/diff/6002/chrome/browser/download/do...
chrome/browser/download/download_shelf_context_menu.cc:88: return
download_item_->IsPartialDownload() || download_item_->CanResume();
On 2013/06/04 17:08:59, rdsmith wrote:
> Why not IsInterrupted() instead of CanResume()?  Seems more accurate to my
> uneducated eye.  

In theory, IsPartialDownload() should imply that the download can be cancelled.
Thus the additional check should be unnecessary. That'll be fixed in an upcoming
CL. I can consider landing that beforehand since that would make the code
clearer.

Without that CL, CanResume() takes into consideration whether the download can
be resumed at all. Thus the option to cancel is only offered if its expected to
do something. IsInterrupted() would unconditionally offer to cancel interrupted
downloads. I've added a check to see if resumption is enabled so that in the
absence of the flag, the previous behavior is preserved.

https://codereview.chromium.org/14955004/diff/6002/chrome/browser/download/do...
chrome/browser/download/download_shelf_context_menu.cc:92: return
download_item_->CanResume();
On 2013/06/04 17:08:59, rdsmith wrote:
> This is true for IN_PROGRESS downloads as well.  Won't that mean that both the
> TOGGLE_PAUSE and RESUME_INTERRUPTED menus show up in the shelf for IN_PROGRESS
> downloads?

IsCommandIdEnabled() is only called on items that have already been added to a
menu. I've added an additional condition to make sure.

https://codereview.chromium.org/14955004/diff/6002/chrome/browser/download/do...
chrome/browser/download/download_shelf_context_menu.cc:158:
download_item_->Resume();
On 2013/06/04 17:08:59, rdsmith wrote:
> Any reason not to just unilaterally resume?  It looks like the resume code is
a
> no-op in the cases you care about.

Done.

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

LGTM.

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

https://codereview.chromium.org/14955004/diff/6002/chrome/browser/download/do...
chrome/browser/download/download_shelf_context_menu.cc:88: return
download_item_->IsPartialDownload() || download_item_->CanResume();
On 2013/06/04 18:52:39, asanka wrote:
> On 2013/06/04 17:08:59, rdsmith wrote:
> > Why not IsInterrupted() instead of CanResume()?  Seems more accurate to my
> > uneducated eye.  
> 
> In theory, IsPartialDownload() should imply that the download can be
cancelled.
> Thus the additional check should be unnecessary. That'll be fixed in an
upcoming
> CL. I can consider landing that beforehand since that would make the code
> clearer.

SGTM; I'm neutral about the order the CLs are landed in.

> Without that CL, CanResume() takes into consideration whether the download can
> be resumed at all. Thus the option to cancel is only offered if its expected
to
> do something. IsInterrupted() would unconditionally offer to cancel
interrupted
> downloads. I've added a check to see if resumption is enabled so that in the
> absence of the flag, the previous behavior is preserved.

So long-term I'm uncomfortable with this, just because I think it's confusing
for some interrupted downloads to be cancelable and some not.  I guess I'll just
say that long-term you should think about potential confusion on the user end,
and make sure that the UI gives clues about why a download can/cannot be
resumed/cancelled.

https://codereview.chromium.org/14955004/diff/6002/chrome/browser/download/do...
chrome/browser/download/download_shelf_context_menu.cc:92: return
download_item_->CanResume();
On 2013/06/04 18:52:39, asanka wrote:
> On 2013/06/04 17:08:59, rdsmith wrote:
> > This is true for IN_PROGRESS downloads as well.  Won't that mean that both
the
> > TOGGLE_PAUSE and RESUME_INTERRUPTED menus show up in the shelf for
IN_PROGRESS
> > downloads?
> 
> IsCommandIdEnabled() is only called on items that have already been added to a
> menu. I've added an additional condition to make sure.

Ah, sorry, missed that.

[asanka, 2013-06-05 20:09:26]

Thanks!

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

https://codereview.chromium.org/14955004/diff/6002/chrome/browser/download/do...
chrome/browser/download/download_shelf_context_menu.cc:88: return
download_item_->IsPartialDownload() || download_item_->CanResume();
On 2013/06/05 20:05:34, rdsmith wrote:
> On 2013/06/04 18:52:39, asanka wrote:
> > On 2013/06/04 17:08:59, rdsmith wrote:
> > > Why not IsInterrupted() instead of CanResume()?  Seems more accurate to my
> > > uneducated eye.  
> > 
> > In theory, IsPartialDownload() should imply that the download can be
> cancelled.
> > Thus the additional check should be unnecessary. That'll be fixed in an
> upcoming
> > CL. I can consider landing that beforehand since that would make the code
> > clearer.
> 
> SGTM; I'm neutral about the order the CLs are landed in.
> 
> > Without that CL, CanResume() takes into consideration whether the download
can
> > be resumed at all. Thus the option to cancel is only offered if its expected
> to
> > do something. IsInterrupted() would unconditionally offer to cancel
> interrupted
> > downloads. I've added a check to see if resumption is enabled so that in the
> > absence of the flag, the previous behavior is preserved.
> 
> So long-term I'm uncomfortable with this, just because I think it's confusing
> for some interrupted downloads to be cancelable and some not.  I guess I'll
just
> say that long-term you should think about potential confusion on the user end,
> and make sure that the UI gives clues about why a download can/cannot be
> resumed/cancelled.

Yup. Long term all interrupted downloads will be resumable (they would at least
attempt to restart). The constraint imposed by requiring the corresponding
WebContents to stick around will go away soon. The confusion should be
temporary.

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

https://codereview.chromium.org/14955004/diff/13001/chrome/browser/download/d...
File chrome/browser/download/download_shelf_context_menu.cc (right):

https://codereview.chromium.org/14955004/diff/13001/chrome/browser/download/d...
chrome/browser/download/download_shelf_context_menu.cc:92: return
download_item_->IsInterrupted() && download_item_->CanResume();
Actually, an extra high-level question: Why not just expand the TOGGLE_PAUSE
item to do the resume interrupted functionality?  (Not blocking landing; just
curious as to why you made this design choice.)

[asanka, 2013-06-05 20:29:16]

https://codereview.chromium.org/14955004/diff/13001/chrome/browser/download/d...
File chrome/browser/download/download_shelf_context_menu.cc (right):

https://codereview.chromium.org/14955004/diff/13001/chrome/browser/download/d...
chrome/browser/download/download_shelf_context_menu.cc:92: return
download_item_->IsInterrupted() && download_item_->CanResume();
On 2013/06/05 20:12:55, rdsmith wrote:
> Actually, an extra high-level question: Why not just expand the TOGGLE_PAUSE
> item to do the resume interrupted functionality?  (Not blocking landing; just
> curious as to why you made this design choice.)

Good question. I did it that way the first time around, and TOGGLE_PAUSE seemed
like the wrong action to trigger a resumption for an interrupted download. Back
when this CL was started, resumption was via
DownloadItem::ResumeInterruptedDownload().

I'll change it back to TOGGLE_PAUSE with appropriate documentation.

[asanka, 2013-06-05 22:10:04]

On 2013/06/05 20:29:16, asanka wrote:
>
https://codereview.chromium.org/14955004/diff/13001/chrome/browser/download/d...
> File chrome/browser/download/download_shelf_context_menu.cc (right):
> 
>
https://codereview.chromium.org/14955004/diff/13001/chrome/browser/download/d...
> chrome/browser/download/download_shelf_context_menu.cc:92: return
> download_item_->IsInterrupted() && download_item_->CanResume();
> On 2013/06/05 20:12:55, rdsmith wrote:
> > Actually, an extra high-level question: Why not just expand the TOGGLE_PAUSE
> > item to do the resume interrupted functionality?  (Not blocking landing;
just
> > curious as to why you made this design choice.)
> 
> Good question. I did it that way the first time around, and TOGGLE_PAUSE
seemed
> like the wrong action to trigger a resumption for an interrupted download.
Back
> when this CL was started, resumption was via
> DownloadItem::ResumeInterruptedDownload().
> 
> I'll change it back to TOGGLE_PAUSE with appropriate documentation.

Done.

[Randy Smith (Not in Mondays), 2013-06-06 15:11:59]

LGTM>

[commit-bot: I haz the power, 2013-06-06 19:09:03]

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

[commit-bot: I haz the power, 2013-06-07 04:55:21]

Change committed as 204710