A few minor changes to the chrome.downloads extension API

A few minor changes to the chrome.downloads extension API

Reviewers:
jhawkins: webui
Done:
asanka: */download/*
jam: content/public/*
asargent: */extensions/*

Replaced the "Invalid operation" error message with more specific and helpful messages

Add DownloadItem.referrer

Add DownloadItem.estimatedEndTime

Add DownloadItem.canResume

Change DownloadQuery.limit to default to 1000

Change 'conflict_action' to 'conflictAction'.

Change DownloadQuery.orderBy and DownloadQuery.query to arrays of strings

Remove DownloadItem.dangerAccepted in favor of DownloadItem.danger == 'accepted'

Add showDefaultFolder().

Disallow access to packaged_apps.

download() now updates the Download.Sources histogram. http://crbug.com/240322

Document using startedAfter and limit to page through search() results.

DownloadItem.error is now a string enum instead of mysterious numbers.

Add downloads.removeFile(id) and DownloadItem::DeleteFile() so that the file may be deleted separately from the history entry.

Calling open() for dangerous downloads has always been guaranteed to return kInvalidOperation because unaccepted dangerous downloads cannot transition to state='complete', and open() returns kInvalidOperation for incomplete items.

We can wait until after launch to figure out whether there needs to be some mechanism to allow extensions to override the request header blacklist. This seems like an edge case for which we may never receive feature requests.

Staged docs preview: http://basho.cam.corp.google.com:8000/extensions/downloads.html#type-DownloadItem

BUG=240322
R=asanka@chromium.org, asargent@chromium.org, jam@chromium.org

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

[benjhayden, 2013-06-14 20:24:52]

PTAL

[asanka, 2013-06-28 21:59:59]

https://codereview.chromium.org/16924017/diff/19001/chrome/browser/download/d...
File chrome/browser/download/download_danger_prompt.h (right):

https://codereview.chromium.org/16924017/diff/19001/chrome/browser/download/d...
chrome/browser/download/download_danger_prompt.h:44: const base::Closure&
destroyed);
I'm guessing this would be the equivalent of the new Dismissed action being
added in https://codereview.chromium.org/18179004/? Could we add a callback that
takes a result parameter instead of adding closures?

[benjhayden, 2013-07-11 15:32:28]

PTAL

https://codereview.chromium.org/16924017/diff/19001/chrome/browser/download/d...
File chrome/browser/download/download_danger_prompt.h (right):

https://codereview.chromium.org/16924017/diff/19001/chrome/browser/download/d...
chrome/browser/download/download_danger_prompt.h:44: const base::Closure&
destroyed);
On 2013/06/28 21:59:59, asanka wrote:
> I'm guessing this would be the equivalent of the new Dismissed action being
> added in https://codereview.chromium.org/18179004/? Could we add a callback
that
> takes a result parameter instead of adding closures?

Done.

[asanka, 2013-07-16 19:57:52]

https://codereview.chromium.org/16924017/diff/121001/chrome/browser/extension...
File chrome/browser/extensions/api/downloads/downloads_api.cc (right):

https://codereview.chromium.org/16924017/diff/121001/chrome/browser/extension...
chrome/browser/extensions/api/downloads/downloads_api.cc:1136:
download_item->GetFullPath()),
I'd avoid deleting files unless the download is COMPLETE.

If the download is CANCELLED, and there's a file at download->GetFullPath(),
that file doesn't belong to the download.

If the download is INTERRUPTED, it's a bit unclear. For resumption, the
intermediate file is kept around if GetFullPath() is not empty. Deleting this
file isn't fatal and won't prevent a resumption (the download will just restart
from the beginning). Arguably, the correct way to "give up" on an INTERRUPTED
download is to call Cancel(). However deleting the intermediate file could be a
way to free up disk space, while still allowing resumption if desired at a later
point in time. If you want to allow deleting the intermediate file for an
INTERRUPTED download, check if GetFullPath() is empty.

https://codereview.chromium.org/16924017/diff/121001/chrome/browser/extension...
chrome/browser/extensions/api/downloads/downloads_api.cc:1291: DownloadShelf*
shelf = GetCurrentBrowser()->window()->GetDownloadShelf();
Is it expected that this will operate on the foremost browser if this is called
from a background page? This seems like a function that's specific to a browser
window.

https://codereview.chromium.org/16924017/diff/121001/chrome/browser/extension...
chrome/browser/extensions/api/downloads/downloads_api.cc:1294: shelf->Show();
What's supposed to happen if there are no downloads on the shelf?
Hide()/Unhide() will take the intended visual state of the shelf into
consideration.

https://codereview.chromium.org/16924017/diff/121001/chrome/browser/extension...
chrome/browser/extensions/api/downloads/downloads_api.cc:1613: (iter.key() !=
kEstimatedEndTimeKey)) {
Why exclude kEstimatedEndTimeKey? It can also disappear if the transfer drops
for a substantial amount of time  (~10s currently) or if the download is paused.

https://codereview.chromium.org/16924017/diff/121001/chrome/browser/extension...
File chrome/browser/extensions/api/downloads/downloads_api_unittest.cc (right):

https://codereview.chromium.org/16924017/diff/121001/chrome/browser/extension...
chrome/browser/extensions/api/downloads/downloads_api_unittest.cc:185: LOG(INFO)
<< "occam caught " << new_event->Debug();
to be removed?

[asargent_no_longer_on_chrome, 2013-07-17 22:36:57]

https://codereview.chromium.org/16924017/diff/156001/chrome/browser/extension...
File chrome/browser/extensions/api/downloads/downloads_api.cc (right):

https://codereview.chromium.org/16924017/diff/156001/chrome/browser/extension...
chrome/browser/extensions/api/downloads/downloads_api.cc:856: std::string*
message_out,
style nit: no newline after opening (

https://codereview.chromium.org/16924017/diff/156001/chrome/browser/extension...
chrome/browser/extensions/api/downloads/downloads_api.cc:858: const std::string&
message_in) {
Would it make sense to have this take const char* instead, and only do the
autoconversion to std::string a little below in the body if |error| is true?
That would avoid unneeded std::string object creations right? (This is called so
infrequently that it probably doesn't matter one way or the other)

One other minor point - it might read slightly better at the call sites if
|error| were the first argument instead of the second, since that's the
condition you care about as you're reading the line.

https://codereview.chromium.org/16924017/diff/156001/chrome/browser/extension...
chrome/browser/extensions/api/downloads/downloads_api.cc:1312: }
style nit: single line if/else bodies can optionally omit braces (looks like you
do so elsewhere in the file, so you should probably do so here too)

https://codereview.chromium.org/16924017/diff/156001/chrome/common/extensions...
File chrome/common/extensions/api/downloads.idl (right):

https://codereview.chromium.org/16924017/diff/156001/chrome/common/extensions...
chrome/common/extensions/api/downloads.idl:108: DOMString referrer;
I forget, do we tend to have the convention elsewhere in developer-facing API's
to spell this properly as you have here, or to propagate the misspelling of
'referer' from the on-the-wire format
(http://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html#sec14)

https://codereview.chromium.org/16924017/diff/156001/chrome/common/extensions...
chrome/common/extensions/api/downloads.idl:136: // Estimated time when the
download will complete in ISO 8601 format.
Is it worth mentioning in this comment that developers can just pass this string
into the constructor for a javascript Date object?

https://codereview.chromium.org/16924017/diff/156001/chrome/common/extensions...
chrome/common/extensions/api/downloads.idl:156: DOMString? error;
It's sort of a bummer to take a presumably strongly typed value from the C++
code and lose that type information by stuffing it into a string here. Did you
do this just to avoid tying the C++ code too tightly to the JS API? Were you
just not sure about how we'd handle adding new values on the C++ side?

https://codereview.chromium.org/16924017/diff/156001/chrome/common/extensions...
chrome/common/extensions/api/downloads.idl:433: static void show(optional long
downloadId);
Another option instead of making this argument optional would be to just have
two methods:

show(long downloadId)
showFolder()

Any reason not to do it this way?

[benjhayden, 2013-07-19 15:53:54]

https://codereview.chromium.org/16924017/diff/121001/chrome/browser/extension...
File chrome/browser/extensions/api/downloads/downloads_api.cc (right):

https://codereview.chromium.org/16924017/diff/121001/chrome/browser/extension...
chrome/browser/extensions/api/downloads/downloads_api.cc:1136:
download_item->GetFullPath()),
On 2013/07/16 19:57:52, asanka wrote:
> I'd avoid deleting files unless the download is COMPLETE.
> 
> If the download is CANCELLED, and there's a file at download->GetFullPath(),
> that file doesn't belong to the download.
> 
> If the download is INTERRUPTED, it's a bit unclear. For resumption, the
> intermediate file is kept around if GetFullPath() is not empty. Deleting this
> file isn't fatal and won't prevent a resumption (the download will just
restart
> from the beginning). Arguably, the correct way to "give up" on an INTERRUPTED
> download is to call Cancel(). However deleting the intermediate file could be
a
> way to free up disk space, while still allowing resumption if desired at a
later
> point in time. If you want to allow deleting the intermediate file for an
> INTERRUPTED download, check if GetFullPath() is empty.

Done.

https://codereview.chromium.org/16924017/diff/121001/chrome/browser/extension...
chrome/browser/extensions/api/downloads/downloads_api.cc:1291: DownloadShelf*
shelf = GetCurrentBrowser()->window()->GetDownloadShelf();
On 2013/07/16 19:57:52, asanka wrote:
> Is it expected that this will operate on the foremost browser if this is
called
> from a background page? This seems like a function that's specific to a
browser
> window.
> 

Yep, it will probably primarily be called from background event pages, which are
often the only javascript context that an extension has.

Should it take a window id from the chrome.windows API? Or a flag to operate on
all windows for the profile versus the current window?

https://codereview.chromium.org/16924017/diff/121001/chrome/browser/extension...
chrome/browser/extensions/api/downloads/downloads_api.cc:1294: shelf->Show();
On 2013/07/16 19:57:52, asanka wrote:
> What's supposed to happen if there are no downloads on the shelf?
> Hide()/Unhide() will take the intended visual state of the shelf into
> consideration.

Done.

https://codereview.chromium.org/16924017/diff/121001/chrome/browser/extension...
chrome/browser/extensions/api/downloads/downloads_api.cc:1613: (iter.key() !=
kEstimatedEndTimeKey)) {
On 2013/07/16 19:57:52, asanka wrote:
> Why exclude kEstimatedEndTimeKey? It can also disappear if the transfer drops
> for a substantial amount of time  (~10s currently) or if the download is
paused.

estimatedEndTime is not in DownloadDelta because it is as volatile as
bytesReceived.
DownloadDelta is restricted to less-volatile fields in order to reduce IPCs.

https://codereview.chromium.org/16924017/diff/121001/chrome/browser/extension...
File chrome/browser/extensions/api/downloads/downloads_api_unittest.cc (right):

https://codereview.chromium.org/16924017/diff/121001/chrome/browser/extension...
chrome/browser/extensions/api/downloads/downloads_api_unittest.cc:185: LOG(INFO)
<< "occam caught " << new_event->Debug();
On 2013/07/16 19:57:52, asanka wrote:
> to be removed?

Done.

https://codereview.chromium.org/16924017/diff/156001/chrome/browser/extension...
File chrome/browser/extensions/api/downloads/downloads_api.cc (right):

https://codereview.chromium.org/16924017/diff/156001/chrome/browser/extension...
chrome/browser/extensions/api/downloads/downloads_api.cc:856: std::string*
message_out,
On 2013/07/17 22:36:57, Antony Sargent wrote:
> style nit: no newline after opening (

Done.

https://codereview.chromium.org/16924017/diff/156001/chrome/browser/extension...
chrome/browser/extensions/api/downloads/downloads_api.cc:858: const std::string&
message_in) {
On 2013/07/17 22:36:57, Antony Sargent wrote:
> Would it make sense to have this take const char* instead, and only do the
> autoconversion to std::string a little below in the body if |error| is true?
> That would avoid unneeded std::string object creations right? (This is called
so
> infrequently that it probably doesn't matter one way or the other)
> 
> One other minor point - it might read slightly better at the call sites if
> |error| were the first argument instead of the second, since that's the
> condition you care about as you're reading the line.

Done.

https://codereview.chromium.org/16924017/diff/156001/chrome/browser/extension...
chrome/browser/extensions/api/downloads/downloads_api.cc:1312: }
On 2013/07/17 22:36:57, Antony Sargent wrote:
> style nit: single line if/else bodies can optionally omit braces (looks like
you
> do so elsewhere in the file, so you should probably do so here too)

Done.

https://codereview.chromium.org/16924017/diff/156001/chrome/common/extensions...
File chrome/common/extensions/api/downloads.idl (right):

https://codereview.chromium.org/16924017/diff/156001/chrome/common/extensions...
chrome/common/extensions/api/downloads.idl:108: DOMString referrer;
On 2013/07/17 22:36:57, Antony Sargent wrote:
> I forget, do we tend to have the convention elsewhere in developer-facing
API's
> to spell this properly as you have here, or to propagate the misspelling of
> 'referer' from the on-the-wire format
> (http://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html#sec14)

chrome.privacy.websites.referrersEnabled
chrome.history.VisitItem.referringVisitId

https://codereview.chromium.org/16924017/diff/156001/chrome/common/extensions...
chrome/common/extensions/api/downloads.idl:136: // Estimated time when the
download will complete in ISO 8601 format.
On 2013/07/17 22:36:57, Antony Sargent wrote:
> Is it worth mentioning in this comment that developers can just pass this
string
> into the constructor for a javascript Date object?

Done.

https://codereview.chromium.org/16924017/diff/156001/chrome/common/extensions...
chrome/common/extensions/api/downloads.idl:156: DOMString? error;
On 2013/07/17 22:36:57, Antony Sargent wrote:
> It's sort of a bummer to take a presumably strongly typed value from the C++
> code and lose that type information by stuffing it into a string here. Did you
> do this just to avoid tying the C++ code too tightly to the JS API? Were you
> just not sure about how we'd handle adding new values on the C++ side?

I did it because it was easiest, but you're right, an explicit enum is better.
Done.

https://codereview.chromium.org/16924017/diff/156001/chrome/common/extensions...
chrome/common/extensions/api/downloads.idl:433: static void show(optional long
downloadId);
On 2013/07/17 22:36:57, Antony Sargent wrote:
> Another option instead of making this argument optional would be to just have
> two methods:
> 
> show(long downloadId)
> showFolder()
> 
> Any reason not to do it this way?
> 
> 

It is a little strange that this one function does two slightly different
things, so I'm happy to split it into a different function, but 'showFolder'
doesn't feel quite right because show() also shows a folder. showDefaultFolder()
seems long. Any other suggestions?

[asargent_no_longer_on_chrome, 2013-07-22 19:15:52]

lgtm

https://codereview.chromium.org/16924017/diff/156001/chrome/common/extensions...
File chrome/common/extensions/api/downloads.idl (right):

https://codereview.chromium.org/16924017/diff/156001/chrome/common/extensions...
chrome/common/extensions/api/downloads.idl:108: DOMString referrer;
On 2013/07/19 15:53:55, benjhayden_chromium wrote:
> On 2013/07/17 22:36:57, Antony Sargent wrote:
> > I forget, do we tend to have the convention elsewhere in developer-facing
> API's
> > to spell this properly as you have here, or to propagate the misspelling of
> > 'referer' from the on-the-wire format
> > (http://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html#sec14)
> 
> chrome.privacy.websites.referrersEnabled
> chrome.history.VisitItem.referringVisitId

Ok, let's keep it as you have it here then.

https://codereview.chromium.org/16924017/diff/156001/chrome/common/extensions...
chrome/common/extensions/api/downloads.idl:433: static void show(optional long
downloadId);
On 2013/07/19 15:53:55, benjhayden_chromium wrote:
> On 2013/07/17 22:36:57, Antony Sargent wrote:
> > Another option instead of making this argument optional would be to just
have
> > two methods:
> > 
> > show(long downloadId)
> > showFolder()
> > 
> > Any reason not to do it this way?
> > 
> > 
> 
> It is a little strange that this one function does two slightly different
> things, so I'm happy to split it into a different function, but 'showFolder'
> doesn't feel quite right because show() also shows a folder.
showDefaultFolder()
> seems long. Any other suggestions?

That doesn't seem all that long to me. But here are some other ideas for you:

showItem / showFolder
showDownloadItem / showDownloadFolder

[asanka, 2013-07-23 21:13:47]

https://codereview.chromium.org/16924017/diff/121001/chrome/browser/extension...
File chrome/browser/extensions/api/downloads/downloads_api.cc (right):

https://codereview.chromium.org/16924017/diff/121001/chrome/browser/extension...
chrome/browser/extensions/api/downloads/downloads_api.cc:1291: DownloadShelf*
shelf = GetCurrentBrowser()->window()->GetDownloadShelf();
On 2013/07/19 15:53:55, benjhayden_chromium wrote:
> On 2013/07/16 19:57:52, asanka wrote:
> > Is it expected that this will operate on the foremost browser if this is
> called
> > from a background page? This seems like a function that's specific to a
> browser
> > window.
> > 
> 
> Yep, it will probably primarily be called from background event pages, which
are
> often the only javascript context that an extension has.
> 
> Should it take a window id from the chrome.windows API? Or a flag to operate
on
> all windows for the profile versus the current window?

The browser window for a download is determined by the WebContents that
initiated the download. So if an extension were to listen for an onCreate and
wants to hide the shelf, then it would have to wait for the associated browser
window to come into the foreground so that it can hide the shelf.

Even then, hiding the shelf only works if the shelf is visible at the time the
extension API call makes it to the browser process. The visibility of the shelf
is a bit complicated since we delay opening it for downloads that are temporary
or are marked for opening automatically. To test this, you can start a slow
download of a type that you've set to auto-open.

In the browser process, calling DownloadItemModel.SetShouldShowInShelf() during
an OnStarted callback for a download prevents it from showing up in the shelf.
However, this doesn't scale to extensions since the roundtrip to the extension
process could be too late.

I'm not sure what a reliable way to implement this is short of having an API
that disables all future downloads from showing up on the shelf and also closing
any shelf that's visible.

[asargent_no_longer_on_chrome, 2013-07-23 21:36:57]

https://codereview.chromium.org/16924017/diff/121001/chrome/browser/extension...
File chrome/browser/extensions/api/downloads/downloads_api.cc (right):

https://codereview.chromium.org/16924017/diff/121001/chrome/browser/extension...
chrome/browser/extensions/api/downloads/downloads_api.cc:1291: DownloadShelf*
shelf = GetCurrentBrowser()->window()->GetDownloadShelf();
On 2013/07/23 21:13:48, asanka wrote:
> On 2013/07/19 15:53:55, benjhayden_chromium wrote:
> > On 2013/07/16 19:57:52, asanka wrote:
> > > Is it expected that this will operate on the foremost browser if this is
> > called
> > > from a background page? This seems like a function that's specific to a
> > browser
> > > window.
> > > 
> > 
> > Yep, it will probably primarily be called from background event pages, which
> are
> > often the only javascript context that an extension has.
> > 
> > Should it take a window id from the chrome.windows API? Or a flag to operate
> on
> > all windows for the profile versus the current window?
> 
> The browser window for a download is determined by the WebContents that
> initiated the download. So if an extension were to listen for an onCreate and
> wants to hide the shelf, then it would have to wait for the associated browser
> window to come into the foreground so that it can hide the shelf.
> 
> Even then, hiding the shelf only works if the shelf is visible at the time the
> extension API call makes it to the browser process. The visibility of the
shelf
> is a bit complicated since we delay opening it for downloads that are
temporary
> or are marked for opening automatically. To test this, you can start a slow
> download of a type that you've set to auto-open.
> 
> In the browser process, calling DownloadItemModel.SetShouldShowInShelf()
during
> an OnStarted callback for a download prevents it from showing up in the shelf.
> However, this doesn't scale to extensions since the roundtrip to the extension
> process could be too late.
> 
> I'm not sure what a reliable way to implement this is short of having an API
> that disables all future downloads from showing up on the shelf and also
closing
> any shelf that's visible.

Note that chrome can be running but with no current browser windows (eg on Mac,
or win/linux if you have extensions/apps with the "background" permission
installed). Also the comments above ExtensionFunction::GetCurrentBrowser mention
that during parts of startup/shutdown the result can be null. Anyhow, you should
check for null return from GetCurrentBrowser here. :)

[benjhayden, 2013-07-24 20:36:44]

PTAL
I removed setShelfVisible and am pursuing other solutions to disabling the
shelf.
https://codereview.chromium.org/19789018/

[asargent_no_longer_on_chrome, 2013-07-24 21:06:41]

still lgtm

[benjhayden, 2013-07-26 13:38:20]

Andy, Asanka, James, John: PTAL

[benjhayden, 2013-07-26 15:41:56]

asargent: */extensions/* 
asanka: */download/* 
jam: content/public/* 
jhawkins: webui

[jam, 2013-07-26 15:42:56]

content/public lgtm

[asanka, 2013-07-26 15:47:22]

What's the general policy regarding enums for a stable extensions API? Is it
considered an API change to add/remove enums?

https://codereview.chromium.org/16924017/diff/336001/chrome/browser/extension...
File chrome/browser/extensions/api/downloads/downloads_api.cc (right):

https://codereview.chromium.org/16924017/diff/336001/chrome/browser/extension...
chrome/browser/extensions/api/downloads/downloads_api.cc:1068:
Fault(download_item->GetState() != DownloadItem::IN_PROGRESS,
Use CanResume() here since resuming an interrupted download is also valid.

https://codereview.chromium.org/16924017/diff/336001/chrome/browser/extension...
File chrome/browser/extensions/api/downloads/downloads_api_browsertest.cc
(right):

https://codereview.chromium.org/16924017/diff/336001/chrome/browser/extension...
chrome/browser/extensions/api/downloads/downloads_api_browsertest.cc:3612:
EXPECT_EQ(InterruptReasonContentToExtension( \
Could we use COMPILE_ASSERT for this?

https://codereview.chromium.org/16924017/diff/336001/chrome/renderer/resource...
File chrome/renderer/resources/extensions/downloads_custom_bindings.js (right):

https://codereview.chromium.org/16924017/diff/336001/chrome/renderer/resource...
chrome/renderer/resources/extensions/downloads_custom_bindings.js:29:
(typeof(result.conflictAction) == 'string'))) {
Shouldn't it be an error if the conflict action is some other type?

[benjhayden, 2013-07-26 16:28:26]

On 2013/07/26 15:47:22, asanka wrote:
> What's the general policy regarding enums for a stable extensions API? Is it
> considered an API change to add/remove enums?

I believe that it is generally acceptable to add/remove values, but not to
change the spelling.

[asargent_no_longer_on_chrome, 2013-07-26 17:35:09]

On 2013/07/26 16:28:26, benjhayden_chromium wrote:
> On 2013/07/26 15:47:22, asanka wrote:
> > What's the general policy regarding enums for a stable extensions API? Is it
> > considered an API change to add/remove enums?
> 
> I believe that it is generally acceptable to add/remove values, but not to
> change the spelling.

One subtle point: it's only ok to remove values from an enum exposed to a
javascript extension/app API if that enum is only ever used for an output
parameter (eg some data we're returning as the result of an API call), but *not*
if it's used as an input parameter, because that would mean that a developer's
code that was working in version X could stop working in version X+1.

[benjhayden, 2013-07-26 19:56:25]

PTAL

https://codereview.chromium.org/16924017/diff/336001/chrome/browser/extension...
File chrome/browser/extensions/api/downloads/downloads_api.cc (right):

https://codereview.chromium.org/16924017/diff/336001/chrome/browser/extension...
chrome/browser/extensions/api/downloads/downloads_api.cc:1068:
Fault(download_item->GetState() != DownloadItem::IN_PROGRESS,
On 2013/07/26 15:47:22, asanka wrote:
> Use CanResume() here since resuming an interrupted download is also valid.

Done.

https://codereview.chromium.org/16924017/diff/336001/chrome/browser/extension...
File chrome/browser/extensions/api/downloads/downloads_api_browsertest.cc
(right):

https://codereview.chromium.org/16924017/diff/336001/chrome/browser/extension...
chrome/browser/extensions/api/downloads/downloads_api_browsertest.cc:3612:
EXPECT_EQ(InterruptReasonContentToExtension( \
On 2013/07/26 15:47:22, asanka wrote:
> Could we use COMPILE_ASSERT for this?

non-constexpr function 'InterruptReasonContentToExtension' cannot be used in a
constant expression

https://codereview.chromium.org/16924017/diff/336001/chrome/renderer/resource...
File chrome/renderer/resources/extensions/downloads_custom_bindings.js (right):

https://codereview.chromium.org/16924017/diff/336001/chrome/renderer/resource...
chrome/renderer/resources/extensions/downloads_custom_bindings.js:29:
(typeof(result.conflictAction) == 'string'))) {
On 2013/07/26 15:47:22, asanka wrote:
> Shouldn't it be an error if the conflict action is some other type?

suggest doesn't take a callback to set runtime.lastError for, but I can
console.error().

[asanka, 2013-07-26 20:28:27]

LGTM with the fix below.

https://codereview.chromium.org/16924017/diff/381001/chrome/browser/extension...
File chrome/browser/extensions/api/downloads/downloads_api.cc (right):

https://codereview.chromium.org/16924017/diff/381001/chrome/browser/extension...
chrome/browser/extensions/api/downloads/downloads_api.cc:1069:
Fault(download_item->IsPaused() && !download_item->CanResume(),
!download_item->CanResume() should be sufficient.

The condition inside Fault(...) as written is false for completed or cancelled
downloads.

[benjhayden, 2013-07-28 17:37:53]

https://codereview.chromium.org/16924017/diff/381001/chrome/browser/extension...
File chrome/browser/extensions/api/downloads/downloads_api.cc (right):

https://codereview.chromium.org/16924017/diff/381001/chrome/browser/extension...
chrome/browser/extensions/api/downloads/downloads_api.cc:1069:
Fault(download_item->IsPaused() && !download_item->CanResume(),
On 2013/07/26 20:28:28, asanka wrote:
> !download_item->CanResume() should be sufficient.
> 
> The condition inside Fault(...) as written is false for completed or cancelled
> downloads.

We wanted it to be not an error to call resume() for un-paused downloads.
Removing IsPaused() here breaks a test.

[benjhayden, 2013-07-28 18:36:15]

Committed patchset #6 manually as r214133 (presubmit successful).