Implement chrome.downloads.erase()

Implement chrome.downloads.erase()
BUG=12133
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=173078

[benjhayden, 2012-12-10 17:44:16]

PTAL

[Randy Smith (Not in Mondays), 2012-12-10 19:04:36]

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

https://codereview.chromium.org/10834129/diff/19001/chrome/common/extensions/...
chrome/common/extensions/api/downloads.idl:371: static void erase(DownloadQuery
query, optional EraseCallback callback);
I don't understand what runs the callback?  Do you have a test testing the
callback behavior?

[benjhayden, 2012-12-10 19:10:48]

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

https://codereview.chromium.org/10834129/diff/19001/chrome/common/extensions/...
chrome/common/extensions/api/downloads.idl:371: static void erase(DownloadQuery
query, optional EraseCallback callback);
On 2012/12/10 19:04:36, rdsmith wrote:
> I don't understand what runs the callback?  Do you have a test testing the
> callback behavior?

This is in the same boat as search(), download(), getFileIcon(), cancel(),
resume(), pause().
There's no way to test that the javascript callback is called properly. That's
something that we have to trust the extension framework to handle correctly.
downloads_api_unittest.cc tests the callback indirectly by testing the
ExtensionFunction's "response" and "error", which the extension framework
promises to pass on to the javascript run-time.

[Randy Smith (Not in Mondays), 2012-12-10 19:17:27]

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

https://codereview.chromium.org/10834129/diff/19001/chrome/common/extensions/...
chrome/common/extensions/api/downloads.idl:371: static void erase(DownloadQuery
query, optional EraseCallback callback);
On 2012/12/10 19:10:48, benjhayden_chromium wrote:
> On 2012/12/10 19:04:36, rdsmith wrote:
> > I don't understand what runs the callback?  Do you have a test testing the
> > callback behavior?
> 
> This is in the same boat as search(), download(), getFileIcon(), cancel(),
> resume(), pause().
> There's no way to test that the javascript callback is called properly. That's
> something that we have to trust the extension framework to handle correctly.
> downloads_api_unittest.cc tests the callback indirectly by testing the
> ExtensionFunction's "response" and "error", which the extension framework
> promises to pass on to the javascript run-time.

Sorry, I'm sure that this is because I don't look at the code for months at a
time, but where is the callback fired from?  

The thing I'm confused about is that the code that iterates through the items to
be erased / removed isn't integrated with the extensions framework, but the
callback is specific to this API function invocation.  The only implementation
that puts those two things together I can imagine is that the callback is hooked
up to OnDownloadRemoved(), but only for the duration of the API function call,
and that means that if a remove occurs from some other source (UI), the callback
will trigger for that remove as well, which violates the API description.

[benjhayden, 2012-12-10 19:26:29]

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

https://codereview.chromium.org/10834129/diff/19001/chrome/common/extensions/...
chrome/common/extensions/api/downloads.idl:371: static void erase(DownloadQuery
query, optional EraseCallback callback);
On 2012/12/10 19:17:27, rdsmith wrote:
> On 2012/12/10 19:10:48, benjhayden_chromium wrote:
> > On 2012/12/10 19:04:36, rdsmith wrote:
> > > I don't understand what runs the callback?  Do you have a test testing the
> > > callback behavior?
> > 
> > This is in the same boat as search(), download(), getFileIcon(), cancel(),
> > resume(), pause().
> > There's no way to test that the javascript callback is called properly.
That's
> > something that we have to trust the extension framework to handle correctly.
> > downloads_api_unittest.cc tests the callback indirectly by testing the
> > ExtensionFunction's "response" and "error", which the extension framework
> > promises to pass on to the javascript run-time.
> 
> Sorry, I'm sure that this is because I don't look at the code for months at a
> time, but where is the callback fired from?  
> 
> The thing I'm confused about is that the code that iterates through the items
to
> be erased / removed isn't integrated with the extensions framework, but the
> callback is specific to this API function invocation.  The only implementation
> that puts those two things together I can imagine is that the callback is
hooked
> up to OnDownloadRemoved(), but only for the duration of the API function call,
> and that means that if a remove occurs from some other source (UI), the
callback
> will trigger for that remove as well, which violates the API description.  

The code that iterates through the items to be removed is
ExtensionEraseFunction, which is-a SyncExtensionFunction, which calls
ExtensionFunction::SendResponse after calling ExtensionEraseFunction::RunImpl.
SendResponse sends an IPC to the renderer containing ExtensionEraseFunction's
response, error_, and an identifier that was assigned by the javascript
extension framework to match up chrome.* API calls with callbacks.
There is no Observer -- ExtensionEraseFunction::RunImpl trusts that calling
Remove() on a DownloadItem will remove it.

[Randy Smith (Not in Mondays), 2012-12-11 15:28:01]

Sorry, I though I sent this comment off yesterday :-{.

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

https://codereview.chromium.org/10834129/diff/19001/chrome/common/extensions/...
chrome/common/extensions/api/downloads.idl:371: static void erase(DownloadQuery
query, optional EraseCallback callback);
On 2012/12/10 19:26:29, benjhayden_chromium wrote:
> On 2012/12/10 19:17:27, rdsmith wrote:
> > On 2012/12/10 19:10:48, benjhayden_chromium wrote:
> > > On 2012/12/10 19:04:36, rdsmith wrote:
> > > > I don't understand what runs the callback?  Do you have a test testing
the
> > > > callback behavior?
> > > 
> > > This is in the same boat as search(), download(), getFileIcon(), cancel(),
> > > resume(), pause().
> > > There's no way to test that the javascript callback is called properly.
> That's
> > > something that we have to trust the extension framework to handle
correctly.
> > > downloads_api_unittest.cc tests the callback indirectly by testing the
> > > ExtensionFunction's "response" and "error", which the extension framework
> > > promises to pass on to the javascript run-time.
> > 
> > Sorry, I'm sure that this is because I don't look at the code for months at
a
> > time, but where is the callback fired from?  
> > 
> > The thing I'm confused about is that the code that iterates through the
items
> to
> > be erased / removed isn't integrated with the extensions framework, but the
> > callback is specific to this API function invocation.  The only
implementation
> > that puts those two things together I can imagine is that the callback is
> hooked
> > up to OnDownloadRemoved(), but only for the duration of the API function
call,
> > and that means that if a remove occurs from some other source (UI), the
> callback
> > will trigger for that remove as well, which violates the API description.  
> 
> The code that iterates through the items to be removed is
> ExtensionEraseFunction, which is-a SyncExtensionFunction, which calls
> ExtensionFunction::SendResponse after calling ExtensionEraseFunction::RunImpl.
> SendResponse sends an IPC to the renderer containing ExtensionEraseFunction's
> response, error_, and an identifier that was assigned by the javascript
> extension framework to match up chrome.* API calls with callbacks.
> There is no Observer -- ExtensionEraseFunction::RunImpl trusts that calling
> Remove() on a DownloadItem will remove it.

So what's the source dataflow for the ids that are passed to the callback?  From
looking at the code those ids are generated in ExtensionsEraseFunction::RunImpl,
and aren't stored in the ExtensionsEraseFunction object.  

(Feel free to IM me if this exchange could be improved by quicker interaction.)

[benjhayden, 2012-12-11 18:47:45]

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

https://codereview.chromium.org/10834129/diff/19001/chrome/common/extensions/...
chrome/common/extensions/api/downloads.idl:371: static void erase(DownloadQuery
query, optional EraseCallback callback);
On 2012/12/11 15:28:01, rdsmith wrote:
> On 2012/12/10 19:26:29, benjhayden_chromium wrote:
> > On 2012/12/10 19:17:27, rdsmith wrote:
> > > On 2012/12/10 19:10:48, benjhayden_chromium wrote:
> > > > On 2012/12/10 19:04:36, rdsmith wrote:
> > > > > I don't understand what runs the callback?  Do you have a test testing
> the
> > > > > callback behavior?
> > > > 
> > > > This is in the same boat as search(), download(), getFileIcon(),
cancel(),
> > > > resume(), pause().
> > > > There's no way to test that the javascript callback is called properly.
> > That's
> > > > something that we have to trust the extension framework to handle
> correctly.
> > > > downloads_api_unittest.cc tests the callback indirectly by testing the
> > > > ExtensionFunction's "response" and "error", which the extension
framework
> > > > promises to pass on to the javascript run-time.
> > > 
> > > Sorry, I'm sure that this is because I don't look at the code for months
at
> a
> > > time, but where is the callback fired from?  
> > > 
> > > The thing I'm confused about is that the code that iterates through the
> items
> > to
> > > be erased / removed isn't integrated with the extensions framework, but
the
> > > callback is specific to this API function invocation.  The only
> implementation
> > > that puts those two things together I can imagine is that the callback is
> > hooked
> > > up to OnDownloadRemoved(), but only for the duration of the API function
> call,
> > > and that means that if a remove occurs from some other source (UI), the
> > callback
> > > will trigger for that remove as well, which violates the API description. 

> > 
> > The code that iterates through the items to be removed is
> > ExtensionEraseFunction, which is-a SyncExtensionFunction, which calls
> > ExtensionFunction::SendResponse after calling
ExtensionEraseFunction::RunImpl.
> > SendResponse sends an IPC to the renderer containing
ExtensionEraseFunction's
> > response, error_, and an identifier that was assigned by the javascript
> > extension framework to match up chrome.* API calls with callbacks.
> > There is no Observer -- ExtensionEraseFunction::RunImpl trusts that calling
> > Remove() on a DownloadItem will remove it.
> 
> So what's the source dataflow for the ids that are passed to the callback? 
From
> looking at the code those ids are generated in
ExtensionsEraseFunction::RunImpl,
> and aren't stored in the ExtensionsEraseFunction object.  
> 
> (Feel free to IM me if this exchange could be improved by quicker
interaction.)

Oh! Sorry, I completely missed that the callback takes an array of the ids.
PTAL at ExtensionEraseFunction and downloads_api_unittest.cc.

[Randy Smith (Not in Mondays), 2012-12-11 18:54:23]

LGTM.

[mkearney1, 2012-12-13 20:10:24]

lgtm

[commit-bot: I haz the power, 2012-12-13 23:21:28]

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

[commit-bot: I haz the power, 2012-12-14 04:13:04]

Change committed as 173078