Return error to chrome.downloads.removeFile

Return error to chrome.downloads.removeFile

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

[asargent_no_longer_on_chrome, 2014-01-14 22:36:03]

extensions part lgtm

https://codereview.chromium.org/134373003/diff/140001/chrome/browser/extensio...
File chrome/browser/extensions/api/downloads/downloads_api.h (right):

https://codereview.chromium.org/134373003/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/api/downloads/downloads_api.h:44: extern const char
kFileNotRemoved[];
nit: Is this constant (or any of the others here) used outside of
downloads_api.cc? If not, then they can just go in an anonymous nameside inside
downloads_api.cc

[benjhayden, 2014-01-16 00:13:24]

https://codereview.chromium.org/134373003/diff/140001/chrome/browser/extensio...
File chrome/browser/extensions/api/downloads/downloads_api.h (right):

https://codereview.chromium.org/134373003/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/api/downloads/downloads_api.h:44: extern const char
kFileNotRemoved[];
On 2014/01/14 22:36:04, Antony Sargent wrote:
> nit: Is this constant (or any of the others here) used outside of
> downloads_api.cc? If not, then they can just go in an anonymous nameside
inside
> downloads_api.cc

Yes, most of them are used in downloads_api_browsertest.cc. The ones that aren't
used there probably should be.

[benjhayden, 2014-01-16 00:14:22]

Asanka, mind reviewing my DownloadItem[Impl] changes?

[asanka, 2014-01-16 16:05:50]

https://codereview.chromium.org/134373003/diff/240001/content/browser/downloa...
File content/browser/download/download_item_impl.cc (right):

https://codereview.chromium.org/134373003/diff/240001/content/browser/downloa...
content/browser/download/download_item_impl.cc:621: if ((GetState() !=
DownloadItem::COMPLETE) ||
Can you also check whether current_path_ is empty here? If so, assume success
since there's no download file.

https://codereview.chromium.org/134373003/diff/240001/content/browser/downloa...
content/browser/download/download_item_impl.cc:623: return;
You'll want to invoke the callback before returning.

https://codereview.chromium.org/134373003/diff/240001/content/browser/downloa...
content/browser/download/download_item_impl.cc:637: if (success) {
Nit: no braces

https://codereview.chromium.org/134373003/diff/240001/content/public/browser/...
File content/public/browser/download_item.h (right):

https://codereview.chromium.org/134373003/diff/240001/content/public/browser/...
content/public/browser/download_item.h:241: // true.
Can you document how the callback will be invoked?

[benjhayden, 2014-01-16 17:09:18]

https://codereview.chromium.org/134373003/diff/240001/content/browser/downloa...
File content/browser/download/download_item_impl.cc (right):

https://codereview.chromium.org/134373003/diff/240001/content/browser/downloa...
content/browser/download/download_item_impl.cc:621: if ((GetState() !=
DownloadItem::COMPLETE) ||
On 2014/01/16 16:05:51, asanka wrote:
> Can you also check whether current_path_ is empty here? If so, assume success
> since there's no download file.

Done.

https://codereview.chromium.org/134373003/diff/240001/content/browser/downloa...
content/browser/download/download_item_impl.cc:623: return;
On 2014/01/16 16:05:51, asanka wrote:
> You'll want to invoke the callback before returning.

Done.

https://codereview.chromium.org/134373003/diff/240001/content/browser/downloa...
content/browser/download/download_item_impl.cc:637: if (success) {
On 2014/01/16 16:05:51, asanka wrote:
> Nit: no braces

Done.

https://codereview.chromium.org/134373003/diff/240001/content/public/browser/...
File content/public/browser/download_item.h (right):

https://codereview.chromium.org/134373003/diff/240001/content/public/browser/...
content/public/browser/download_item.h:241: // true.
On 2014/01/16 16:05:51, asanka wrote:
> Can you document how the callback will be invoked?

Done.

[asanka, 2014-01-16 18:15:52]

Hmm. Wouldn't it be a problem that the callback may not be invoked? Arguably,
the previous behavior when the deletion failed was equally unreliable.

[benjhayden, 2014-01-16 23:10:19]

On 2014/01/16 18:15:52, asanka wrote:
> Hmm. Wouldn't it be a problem that the callback may not be invoked? Arguably,
> the previous behavior when the deletion failed was equally unreliable.

PTAL

[benjhayden, 2014-01-16 23:10:19]

On 2014/01/16 18:15:52, asanka wrote:
> Hmm. Wouldn't it be a problem that the callback may not be invoked? Arguably,
> the previous behavior when the deletion failed was equally unreliable.

PTAL

[asanka, 2014-01-16 23:18:40]

lgtm. Thanks!

[commit-bot: I haz the power, 2014-01-16 23:30:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/benjhayden@chromium.org/134373003/620001

[commit-bot: I haz the power, 2014-01-16 23:54:35]

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

[benjhayden, 2014-01-17 00:04:21]

Owners review please.
sky: content/public/test/mock_download_item.h
joi: content/public/browser/download_item.h
Thanks!

[sky, 2014-01-17 00:36:58]

mock_download_item.h LGTM

[Jói, 2014-01-17 08:45:13]

//content/public LGTM.

[commit-bot: I haz the power, 2014-01-17 15:22:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/benjhayden@chromium.org/134373003/620001

[commit-bot: I haz the power, 2014-01-17 17:09:53]

Change committed as 245541