chrome.experimental.downloads

This CL will not be submitted. This CL is for high-level implementation direction feedback only. Nits will be ignored. This code will definitely change before I submit it, if only to improve safety and testability and implement tests and handle edge cases and fix lint and split it up into smaller CLs for easier digestion.
Please direct comments on the API to the draft at http://goo.gl/6hO1n

TODO:
implement the other functions and events
edge cases
find race conditions
tests
chunk CLs
experiment with html5 dnd and batch creations/erases

[Sam Kerner (Chrome), 2011-06-21 03:37:40]

Ben,
   You are on the right track.  You have the interface from javascript to C++ in
place.  Now that you have written some of the C++ side of an extension API, I
would read the C++ implementation of another API to see the conventions for
validating input and setting error_ on errors.  The chrome.window.* functions
are a good example:

http://www.google.com/codesearch#OAMlx_jo-ck/src/chrome/browser/extensions/ex...

http://codereview.chromium.org/7192016/diff/4001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_downloads.cc (right):

http://codereview.chromium.org/7192016/diff/4001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_downloads.cc:99: // I wonder what thread
this method is called on.
A DCHECK() that you are on the right thread would be a good idea.

http://codereview.chromium.org/7192016/diff/4001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_downloads.cc:184: if ((args_.get() == NULL)
||
The complex guarding if() makes me a bit nervous...  We have some methods and
macros to make validating inputs to the C++ side of extensions API code easier. 
For example, to get the first argument, which should be a dictionary, do this:

  
  EXTENSION_FUNCTION_VALIDATE(args_->GetDictionary(0, &dict));

This macro crashes if args[0] is not a dictionary.  The marshaling code reads
extensions_api.json and checks types, so if you get here and the types of the
args are wrong something very bad is happening.  The macro
EXTENSION_FUNCTION_VALIDATE() crashes the extension's process if the type is
wrong:  Better to die on a bug then to mishandle data because we make a bad
assumption about types.

Some good examples of args processing code are here:
http://www.google.com/codesearch#OAMlx_jo-ck/src/chrome/browser/extensions/ex...

http://codereview.chromium.org/7192016/diff/4001/chrome/renderer/resources/ex...
File chrome/renderer/resources/extension_process_bindings.js (right):

http://codereview.chromium.org/7192016/diff/4001/chrome/renderer/resources/ex...
chrome/renderer/resources/extension_process_bindings.js:745:
apiFunctions["experimental.downloads.download"].customCallback =
Why do you need to set a custom callback?

[Paweł Hajdan Jr., 2011-06-21 08:21:56]

Thank you for working on this. However, the download code is still very unstable
and is being heavily refactored. Please remove _any_ references to the internals
(like the download states or some kind of DownloadItem for JS proxy) from the
API, because when it becomes an API, we'll be stuck with it, which is extremely
bad. Even if it's experimental and breaking changes would be fine, it would
still make refactoring harder.

If possible, please postpone adding this extension API until the code is more
clean and stable, or only add an _extremely_ limited subset (not sure if the
latter makes much sense).

[rdsmith_google.com, 2011-06-21 13:21:54]

Pawel: I disagree with you here.  I believe the Downloads code is at
the stage where we can do this type of addition safely in parallel
with the refactoring.  We've spent some time discussing where we want
the state system in the download code to end up (short version:
combining interrupted & cancelled with a "interruption reason" that
can be user action, leaving everything else alone) and we're managing
the extensions API to head in that direction.  And there's a lot of
demand for an extensions API, and it will enable easier work on the
revamping of the UI going forward.

-- Randy

On Tue, Jun 21, 2011 at 4:21 AM,  <phajdan.jr@chromium.org> wrote:
> Thank you for working on this. However, the download code is still very
> unstable
> and is being heavily refactored. Please remove _any_ references to the
> internals
> (like the download states or some kind of DownloadItem for JS proxy) from
> the
> API, because when it becomes an API, we'll be stuck with it, which is
> extremely
> bad. Even if it's experimental and breaking changes would be fine, it would
> still make refactoring harder.
>
> If possible, please postpone adding this extension API until the code is
> more
> clean and stable, or only add an _extremely_ limited subset (not sure if the
> latter makes much sense).
>
> http://codereview.chromium.org/7192016/
>

[bSh, 2011-06-21 14:44:17]

On 2011/06/21 08:21:56, Paweł Hajdan Jr. wrote:
> Thank you for working on this. However, the download code is still very
unstable
> and is being heavily refactored. Please remove _any_ references to the
internals
> (like the download states or some kind of DownloadItem for JS proxy) from the
> API, because when it becomes an API, we'll be stuck with it, which is
extremely
> bad. Even if it's experimental and breaking changes would be fine, it would
> still make refactoring harder.
> 
> If possible, please postpone adding this extension API until the code is more
> clean and stable, or only add an _extremely_ limited subset (not sure if the
> latter makes much sense).

As the issue Description states, this CL will not be submitted. Pieces of the
API will be checked in gradually one at a time with plenty of time for everybody
to review. This CL is just proof-of-concept and my own exploration into the
extensions and downloads systems. I do not expect this work to hold back
progress on refactoring the downloads system. I fully intend to handle merging
refactors into my work.
The downloads extensions API can stay in experimental until it settles down. We
can modify chrome.experimental.downloads.* as much as necessary in response to
downloads refactors. We won't be stuck with anything.
Apologies for not making this clearer in the issue Description.

[bSh, 2011-06-24 19:46:25]

http://codereview.chromium.org/7192016/diff/4001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_downloads.cc (right):

http://codereview.chromium.org/7192016/diff/4001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_downloads.cc:99: // I wonder what thread
this method is called on.
On 2011/06/21 03:37:40, Sam Kerner (Chrome) wrote:
> A DCHECK() that you are on the right thread would be a good idea.

Done.

http://codereview.chromium.org/7192016/diff/4001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_downloads.cc:184: if ((args_.get() == NULL)
||
On 2011/06/21 03:37:40, Sam Kerner (Chrome) wrote:
> The complex guarding if() makes me a bit nervous...  We have some methods and
> macros to make validating inputs to the C++ side of extensions API code
easier. 
> For example, to get the first argument, which should be a dictionary, do this:
> 
>   
>   EXTENSION_FUNCTION_VALIDATE(args_->GetDictionary(0, &dict));
> 
> This macro crashes if args[0] is not a dictionary.  The marshaling code reads
> extensions_api.json and checks types, so if you get here and the types of the
> args are wrong something very bad is happening.  The macro
> EXTENSION_FUNCTION_VALIDATE() crashes the extension's process if the type is
> wrong:  Better to die on a bug then to mishandle data because we make a bad
> assumption about types.
> 
> Some good examples of args processing code are here:
>
http://www.google.com/codesearch#OAMlx_jo-ck/src/chrome/browser/extensions/ex...

Done.

http://codereview.chromium.org/7192016/diff/4001/chrome/renderer/resources/ex...
File chrome/renderer/resources/extension_process_bindings.js (right):

http://codereview.chromium.org/7192016/diff/4001/chrome/renderer/resources/ex...
chrome/renderer/resources/extension_process_bindings.js:745:
apiFunctions["experimental.downloads.download"].customCallback =
On 2011/06/21 03:37:40, Sam Kerner (Chrome) wrote:
> Why do you need to set a custom callback?

This is the only way to pass more than one parameter to the extension's
callback. This customCallback wouldn't be necessary if we merged the error
parameter with chrome.extensions.lastError.

[bSh, 2011-06-28 21:00:18]

Still very much experimental and lots to do.

The kernel of this patch set is DownloadQuery:
http://codereview.chromium.org/7192016/diff/17001/chrome/browser/download/dow...
I'm planning on rewriting the existing four different
DownloadManager::GetDownloads() methods in terms of DownloadQuery. This should
fix some problems that I caused when I merged the four different maps/sets into
one DownloadMap.