Implement chrome.downloads.erase()

chrome/common/extensions/api/downloads.idl

Index: chrome/common/extensions/api/downloads.idl
diff --git a/chrome/common/extensions/api/downloads.idl b/chrome/common/extensions/api/downloads.idl
index c3e21828a57542761b6cbf7864f79f0447dcb01d..110932c1856a9a69cd531a033259e9d564ef3995 100644
--- a/chrome/common/extensions/api/downloads.idl
+++ b/chrome/common/extensions/api/downloads.idl
@@ -365,9 +365,10 @@ namespace downloads {
     // |downloadId|: The identifier for the downloaded file.
     static void open(long downloadId);
 
-    // Erase matching $ref:DownloadItem from history
-    [nodoc] static void erase(DownloadQuery query,
-                              optional EraseCallback callback);
+    // Erase matching $ref:DownloadItem from history. An $ref.onErased event
+    // will fire for each $ref.DownloadItem that matches <code>query</code>,
+    // then <code>callback</code> will be called.
+    static void erase(DownloadQuery query, optional EraseCallback callback);

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

I don't understand what runs the callback?  Do you have a test testing the
callback behavior?

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

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]

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]

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]

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]

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

 
     // TODO(benjhayden) Comment.
     [nodoc] static void setDestination(long downloadId, DOMString relativePath);