downloads: prevent "Clear all" from removing in progress downloads

downloads: prevent "Clear all" from removing in progress downloads.

R=benjhayden@chromium.org
BUG=440741
Committed: https://crrev.com/6796a8b5cb45f30c72e64d32261a9e2219d45981
Cr-Commit-Position: refs/heads/master@{#308196}

[Dan Beam, 2014-12-11 23:20:50]

Patchset #1 (id:1) has been deleted

[benjhayden, 2014-12-11 23:36:24]

Looks straightforward to me, just a couple questions about the request handle.

LGTM

Might want to think about consistently using different words for undoable remove
(MarkToRemove?) versus permanent remove (Remove?).

https://codereview.chromium.org/795353002/diff/20001/content/browser/download...
File content/browser/download/download_item_impl.cc (right):

https://codereview.chromium.org/795353002/diff/20001/content/browser/download...
content/browser/download/download_item_impl.cc:418: if (state_ ==
IN_PROGRESS_INTERNAL && request_handle_) {
Should this be checked everywhere that request_handle_ is used?
Or would it be possible to construct a request handle, either real or mock?

[Dan Beam, 2014-12-12 00:07:02]

https://codereview.chromium.org/795353002/diff/20001/content/browser/download...
File content/browser/download/download_item_impl.cc (right):

https://codereview.chromium.org/795353002/diff/20001/content/browser/download...
content/browser/download/download_item_impl.cc:418: if (state_ ==
IN_PROGRESS_INTERNAL && request_handle_) {
On 2014/12/11 23:36:24, benjhayden_chromium wrote:
> Should this be checked everywhere that request_handle_ is used?
> Or would it be possible to construct a request handle, either real or mock?

we talked about this on chat -- adding a way to mock something internal to the
download manager (a content/ feature) from here (chrome/) requires more effort
and fun than it's probably currently worth.  additionally, the more
|request_handle_ == NULL| checks I add, the less safe the code gets when it
*shouldn't* be null (which is currently only in this one case).

I've added a TODO() for now.

[Dan Beam, 2014-12-12 00:07:19]

The CQ bit was checked by dbeam@chromium.org

[commit-bot: I haz the power, 2014-12-12 00:09:17]

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

[commit-bot: I haz the power, 2014-12-12 01:59:36]

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

[commit-bot: I haz the power, 2014-12-12 01:59:37]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Dan Beam, 2014-12-12 04:28:35]

Patchset #3 (id:60001) has been deleted

[Dan Beam, 2014-12-12 04:29:30]

https://codereview.chromium.org/795353002/diff/40001/content/browser/download...
File content/browser/download/download_item_impl.cc (right):

https://codereview.chromium.org/795353002/diff/40001/content/browser/download...
content/browser/download/download_item_impl.cc:165:
DCHECK_NE(IN_PROGRESS_INTERNAL, state_);
hit this in the tests, rewrote with gmock, ptal

[benjhayden, 2014-12-12 22:43:13]

lgtm

[Dan Beam, 2014-12-12 22:43:47]

The CQ bit was checked by dbeam@chromium.org

[commit-bot: I haz the power, 2014-12-12 22:45:42]

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

[commit-bot: I haz the power, 2014-12-12 23:52:15]

Committed patchset #3 (id:80001)

[commit-bot: I haz the power, 2014-12-12 23:52:57]

Patchset 3 (id:??) landed as
https://crrev.com/6796a8b5cb45f30c72e64d32261a9e2219d45981
Cr-Commit-Position: refs/heads/master@{#308196}