Remove the DownloadItem::TogglePause() interface.

Remove the DownloadItem::TogglePause() interface.

Replace it with explict Pause() and Resume() interfaces.

R=asanka@chromium.org
R=benjhayden@chromium.org


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

[Randy Smith (Not in Mondays), 2012-12-28 23:28:50]

Ben, Asanka: PTAL?  

Asanka: Whole CL.
Ben: Could you take a look at the extensions & downloads.js DownloadsDOMHandler
changes?  In addition to the code, I'd love your feedback on the interface
choice for downloads.html (exporting the TogglePause split all the way up to
downloads.js).

[benjhayden, 2013-01-02 16:19:50]

https://codereview.chromium.org/11711003/diff/1/chrome/browser/ui/webui/downl...
File chrome/browser/ui/webui/downloads_dom_handler.cc (right):

https://codereview.chromium.org/11711003/diff/1/chrome/browser/ui/webui/downl...
chrome/browser/ui/webui/downloads_dom_handler.cc:258:
base::Bind(&DownloadsDOMHandler::Pause,
Why remove the "Handle" prefix?

[asanka, 2013-01-02 16:44:50]

LGTM modulo nits and Ben's comments.

https://codereview.chromium.org/11711003/diff/1/chrome/browser/automation/tes...
File chrome/browser/automation/testing_automation_provider.cc (right):

https://codereview.chromium.org/11711003/diff/1/chrome/browser/automation/tes...
chrome/browser/automation/testing_automation_provider.cc:2700: // This will
still return if download has already completed.
I'm not very familiar with the automation interface. But it seems to me that we
won't generate an OnDownloadUpdated() notification if the download is complete.
So the observer that we add above might not fire reliably.

https://codereview.chromium.org/11711003/diff/1/chrome/browser/extensions/api...
File chrome/browser/extensions/api/downloads/downloads_api.cc (right):

https://codereview.chromium.org/11711003/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/downloads/downloads_api.cc:706: } else if
(!download_item->IsPaused()) {
Nit: Since we now have Pause() and Resume(), I don't think we need to test for
IsPaused() at every call site.

[Randy Smith (Not in Mondays), 2013-01-02 20:16:01]

Incorporated comments.  PTAL.

Do either of you feel comfortable enough with the automation interface to 
review my changes there?

https://codereview.chromium.org/11711003/diff/1/chrome/browser/automation/tes...
File chrome/browser/automation/testing_automation_provider.cc (right):

https://codereview.chromium.org/11711003/diff/1/chrome/browser/automation/tes...
chrome/browser/automation/testing_automation_provider.cc:2700: // This will
still return if download has already completed.
On 2013/01/02 16:44:50, asanka wrote:
> I'm not very familiar with the automation interface. But it seems to me that
we
> won't generate an OnDownloadUpdated() notification if the download is
complete.
> So the observer that we add above might not fire reliably.

You are quite correct.  I had originally thought that the problem was already
there, so I wasn't going to fix it for this CL, but when I look more closely at
the old code, TogglePause() always fires an UpdateObservers(), even if the state
isn't changed.  I'll fix this.

https://codereview.chromium.org/11711003/diff/1/chrome/browser/extensions/api...
File chrome/browser/extensions/api/downloads/downloads_api.cc (right):

https://codereview.chromium.org/11711003/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/downloads/downloads_api.cc:706: } else if
(!download_item->IsPaused()) {
On 2013/01/02 16:44:50, asanka wrote:
> Nit: Since we now have Pause() and Resume(), I don't think we need to test for
> IsPaused() at every call site.

Done.

https://codereview.chromium.org/11711003/diff/1/chrome/browser/ui/webui/downl...
File chrome/browser/ui/webui/downloads_dom_handler.cc (right):

https://codereview.chromium.org/11711003/diff/1/chrome/browser/ui/webui/downl...
chrome/browser/ui/webui/downloads_dom_handler.cc:258:
base::Bind(&DownloadsDOMHandler::Pause,
On 2013/01/02 16:19:50, benjhayden_chromium wrote:
> Why remove the "Handle" prefix?

Whoops; I had missed that bit of consistency.  Done.

[benjhayden, 2013-01-03 16:37:29]

LGTM
The automation interface looks good to me, assuming you ran the pyauto tests.

[Randy Smith (Not in Mondays), 2013-01-04 21:35:09]

Looking for stamps:

phadjan.jr@: chrome/browser/automation, chrome/test*, content/public/test/*

estade@: chrome/browser/ui/webui/*, chrome/browser/resources/*

[Paweł Hajdan Jr., 2013-01-04 22:47:54]

Is pyauto coverage redundant (i.e. do we have other tests that cover the same
scenario)? If so, please delete them, we're transitioning off pyauto because of
flakiness issues.

WDYT?

[Randy Smith (Not in Mondays), 2013-01-04 22:58:17]

On 2013/01/04 22:47:54, Paweł Hajdan Jr. wrote:
> Is pyauto coverage redundant (i.e. do we have other tests that cover the same
> scenario)? If so, please delete them, we're transitioning off pyauto because
of
> flakiness issues.
> 
> WDYT?

Ben's volunteered to get the testing currently covered by the pyauto tests into
the browser tests, but that hasn't happened yet.  I don't see any browser or
unit tests that test the pause/resume functionality, unfortunately.

[Paweł Hajdan Jr., 2013-01-04 23:07:53]

Alright, LGTM. Thanks for checking!

[Evan Stade, 2013-01-05 02:14:44]

lgtm

[commit-bot: I haz the power, 2013-01-07 16:35:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rdsmith@chromium.org/11711003/22001

[commit-bot: I haz the power, 2013-01-07 18:03:21]

Change committed as 175357