chrome.experimental.downloads stubs

chrome.experimental.downloads stubs
http://goo.gl/6hO1n
Future CLs will contain implementations for the functions and events.


BUG=
TEST=


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=96778

[benjhayden, 2011-07-27 15:23:32]

Stubs for chrome.experimental.downloads functions.
Future CLs will include implementations, events (in
ExtensionBrowserEventRouter), tests, etc. Getting the stubs in allows all of
that to happen in parallel.
Note that since this is in experimental, we can change it in any way at any time
until we move it out of experimental. This does not lock us into anything.

[Mihai Parparita -not on Chrome, 2011-07-27 16:28:08]

http://codereview.chromium.org/7480046/diff/1/chrome/common/extensions/api/ex...
File chrome/common/extensions/api/extension_api.json (right):

http://codereview.chromium.org/7480046/diff/1/chrome/common/extensions/api/ex...
chrome/common/extensions/api/extension_api.json:3057: "namespace":
"experimental.downloads",
It's probably a good idea to add the "nodoc": true property here, otherwise
these stub methods will start to show up at
http://code.google.com/chrome/extensions/trunk/experimental.html as soon as this
CL lands.

[benjhayden, 2011-07-27 18:40:31]

http://codereview.chromium.org/7480046/diff/1/chrome/common/extensions/api/ex...
File chrome/common/extensions/api/extension_api.json (right):

http://codereview.chromium.org/7480046/diff/1/chrome/common/extensions/api/ex...
chrome/common/extensions/api/extension_api.json:3057: "namespace":
"experimental.downloads",
On 2011/07/27 16:28:08, Mihai Parparita wrote:
> It's probably a good idea to add the "nodoc": true property here, otherwise
> these stub methods will start to show up at
> http://code.google.com/chrome/extensions/trunk/experimental.html as soon as
this
> CL lands.

Done.

[asanka, 2011-07-27 19:31:35]

Would extension_downloads_api.{h,cc} fit in better with the naming convention?

[benjhayden, 2011-07-28 15:31:42]

On 2011/07/27 19:31:35, asanka wrote:
> Would extension_downloads_api.{h,cc} fit in better with the naming convention?

Done.

[Randy Smith (Not in Mondays), 2011-07-28 20:11:08]

LGTM, but I'm not a good person for really evaluating this, since it's primarily
about fitting in with extensions patterns, and I don't know those patterns.

http://codereview.chromium.org/7480046/diff/8001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_downloads_api.h (right):

http://codereview.chromium.org/7480046/diff/8001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_downloads_api.h:27: // http://goo.gl/6hO1n
This seems like a weird thing to have uncommented in a file.  Can you comment as
to what it means or remove it?  And will this URL work globally on the network?

[benjhayden, 2011-07-28 20:32:06]

http://codereview.chromium.org/7480046/diff/8001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_downloads_api.h (right):

http://codereview.chromium.org/7480046/diff/8001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_downloads_api.h:27: // http://goo.gl/6hO1n
On 2011/07/28 20:11:09, rdsmith wrote:
> This seems like a weird thing to have uncommented in a file.  Can you comment
as
> to what it means or remove it?  And will this URL work globally on the
network?

Commented, and yes, goo.gl and the docs link that it points to are globally
accessible.

[Mihai Parparita -not on Chrome, 2011-07-28 23:18:29]

http://codereview.chromium.org/7480046/diff/14001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_downloads_api.cc (right):

http://codereview.chromium.org/7480046/diff/14001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_downloads_api.cc:28:
EXTENSION_FUNCTION_VALIDATE(options_->GetString("url", &url_));
Key names should be in constants (see extension_tts_api_constants for an example
of a contant-heavy API that was recently added).

http://codereview.chromium.org/7480046/diff/14001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_downloads_api.cc:29: if (url_.empty()) {
You can use minLength in the schema file to enforce this.

http://codereview.chromium.org/7480046/diff/14001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_downloads_api.cc:35: error_ = "I'm afraid I
can't do that.";
Error names should be in constants. Also, is there a better error we can give
here? When would the resource dispatcher host be null? Should this just be a
CHECK?

http://codereview.chromium.org/7480046/diff/14001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_downloads_api.cc:41:
options_->GetString("filename", &filename_);
The pattern for optional keys is to use if (options->HasKey(kKeyName) {
EXTENSION_FUNCTION_VALIDATE(...)

http://codereview.chromium.org/7480046/diff/14001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_downloads_api.h (right):

http://codereview.chromium.org/7480046/diff/14001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_downloads_api.h:53: void
BeginDownloadOnIOThread();
These functions don't appear to have implementations.

http://codereview.chromium.org/7480046/diff/14001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_downloads_api.h:57: base::DictionaryValue*
options_;
Do you actually need all the all the arguments as members? For example, you
already seem to extract all that's necessary out of options_.

http://codereview.chromium.org/7480046/diff/14001/chrome/common/extensions/ap...
File chrome/common/extensions/api/extension_api.json (right):

http://codereview.chromium.org/7480046/diff/14001/chrome/common/extensions/ap...
chrome/common/extensions/api/extension_api.json:3221: {"type": "integer",
"name": "download_id"}
For consistency, the name of this parameter should be downloadId.

http://codereview.chromium.org/7480046/diff/14001/chrome/common/extensions/ap...
chrome/common/extensions/api/extension_api.json:3228: "value": "in progress",
Enum values are generally in hacker_style.

http://codereview.chromium.org/7480046/diff/14001/chrome/common/extensions/ap...
chrome/common/extensions/api/extension_api.json:3264: "url": {"type": "string",
"description": "absolute URL"},
Probably premature at this point, but all the descriptions should be sentences
and capitalized accordingly.

You may also consider not having the full schema in this CL, only the minimum
that you need for the stubs, and adding it piecemeal.

[Mihai Parparita -not on Chrome, 2011-07-28 23:50:54]

On Thu, Jul 28, 2011 at 1:11 PM,  <rdsmith@chromium.org> wrote:
> LGTM, but I'm not a good person for really evaluating this, since it's
> primarily
> about fitting in with extensions patterns, and I don't know those patterns.

I'll do the review from the extensions side.

Mihai

[benjhayden, 2011-08-01 21:43:51]

I'll finish the DownloadDelta/Download*Diff json tomorrow. PTAL at the rest.
Thanks!

http://codereview.chromium.org/7480046/diff/14001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_downloads_api.cc (right):

http://codereview.chromium.org/7480046/diff/14001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_downloads_api.cc:28:
EXTENSION_FUNCTION_VALIDATE(options_->GetString("url", &url_));
On 2011/07/28 23:18:29, Mihai Parparita wrote:
> Key names should be in constants (see extension_tts_api_constants for an
example
> of a contant-heavy API that was recently added).

Done.

http://codereview.chromium.org/7480046/diff/14001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_downloads_api.cc:29: if (url_.empty()) {
On 2011/07/28 23:18:29, Mihai Parparita wrote:
> You can use minLength in the schema file to enforce this.

Done.

http://codereview.chromium.org/7480046/diff/14001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_downloads_api.cc:35: error_ = "I'm afraid I
can't do that.";
On 2011/07/28 23:18:29, Mihai Parparita wrote:
> Error names should be in constants. Also, is there a better error we can give
> here? When would the resource dispatcher host be null? Should this just be a
> CHECK?

Punted to a future CL that will use rdh_.

http://codereview.chromium.org/7480046/diff/14001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_downloads_api.cc:41:
options_->GetString("filename", &filename_);
On 2011/07/28 23:18:29, Mihai Parparita wrote:
> The pattern for optional keys is to use if (options->HasKey(kKeyName) {
> EXTENSION_FUNCTION_VALIDATE(...)

Done.

http://codereview.chromium.org/7480046/diff/14001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_downloads_api.h (right):

http://codereview.chromium.org/7480046/diff/14001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_downloads_api.h:53: void
BeginDownloadOnIOThread();
On 2011/07/28 23:18:29, Mihai Parparita wrote:
> These functions don't appear to have implementations.

Done.

http://codereview.chromium.org/7480046/diff/14001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_downloads_api.h:57: base::DictionaryValue*
options_;
On 2011/07/28 23:18:29, Mihai Parparita wrote:
> Do you actually need all the all the arguments as members? For example, you
> already seem to extract all that's necessary out of options_.

Done.

http://codereview.chromium.org/7480046/diff/14001/chrome/common/extensions/ap...
File chrome/common/extensions/api/extension_api.json (right):

http://codereview.chromium.org/7480046/diff/14001/chrome/common/extensions/ap...
chrome/common/extensions/api/extension_api.json:3221: {"type": "integer",
"name": "download_id"}
On 2011/07/28 23:18:29, Mihai Parparita wrote:
> For consistency, the name of this parameter should be downloadId.

Done.

http://codereview.chromium.org/7480046/diff/14001/chrome/common/extensions/ap...
chrome/common/extensions/api/extension_api.json:3228: "value": "in progress",
On 2011/07/28 23:18:29, Mihai Parparita wrote:
> Enum values are generally in hacker_style.

Done.

http://codereview.chromium.org/7480046/diff/14001/chrome/common/extensions/ap...
chrome/common/extensions/api/extension_api.json:3264: "url": {"type": "string",
"description": "absolute URL"},
On 2011/07/28 23:18:29, Mihai Parparita wrote:
> Probably premature at this point, but all the descriptions should be sentences
> and capitalized accordingly.
> 
> You may also consider not having the full schema in this CL, only the minimum
> that you need for the stubs, and adding it piecemeal.

Done.

[benjhayden, 2011-08-02 19:35:39]

DownloadDelta done.
The api.json should be basically complete. When the API is implemented and a
little more mature in a few weeks/months, I'll remove the nodoc and clean up the
descriptions and generate the docs.

[Mihai Parparita -not on Chrome, 2011-08-09 19:51:41]

LGTM, just some style nits.

http://codereview.chromium.org/7480046/diff/22001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_downloads_api.h (right):

http://codereview.chromium.org/7480046/diff/22001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_downloads_api.h:61: int dl_id_;
Nit: These can be removed for now.

http://codereview.chromium.org/7480046/diff/22001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_downloads_api_constants.cc (right):

http://codereview.chromium.org/7480046/diff/22001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_downloads_api_constants.cc:4: #include
"chrome/browser/extensions/extension_downloads_api_constants.h"
Ditto about more empty lines.

http://codereview.chromium.org/7480046/diff/22001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_downloads_api_constants.cc:10: const char
kSaveAsKey[] = "save_as";
I think this is saveAs in extension_api.json now.

http://codereview.chromium.org/7480046/diff/22001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_downloads_api_constants.h (right):

http://codereview.chromium.org/7480046/diff/22001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_downloads_api_constants.h:4: #ifndef
CHROME_BROWSER_EXTENSIONS_EXTENSION_DOWNLOADS_API_CONSTANTS_H_
Nit: This file could use some empty lines to help with scannability
(before/after the #include guard, in the namespace block body, etc.)

[benjhayden, 2011-08-09 20:59:50]

http://codereview.chromium.org/7480046/diff/22001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_downloads_api.h (right):

http://codereview.chromium.org/7480046/diff/22001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_downloads_api.h:61: int dl_id_;
On 2011/08/09 19:51:42, Mihai Parparita wrote:
> Nit: These can be removed for now.

Done.

http://codereview.chromium.org/7480046/diff/22001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_downloads_api_constants.cc (right):

http://codereview.chromium.org/7480046/diff/22001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_downloads_api_constants.cc:4: #include
"chrome/browser/extensions/extension_downloads_api_constants.h"
On 2011/08/09 19:51:42, Mihai Parparita wrote:
> Ditto about more empty lines.

Done.

http://codereview.chromium.org/7480046/diff/22001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_downloads_api_constants.cc:10: const char
kSaveAsKey[] = "save_as";
On 2011/08/09 19:51:42, Mihai Parparita wrote:
> I think this is saveAs in extension_api.json now.

Done.

http://codereview.chromium.org/7480046/diff/22001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_downloads_api_constants.h (right):

http://codereview.chromium.org/7480046/diff/22001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_downloads_api_constants.h:4: #ifndef
CHROME_BROWSER_EXTENSIONS_EXTENSION_DOWNLOADS_API_CONSTANTS_H_
On 2011/08/09 19:51:42, Mihai Parparita wrote:
> Nit: This file could use some empty lines to help with scannability
> (before/after the #include guard, in the namespace block body, etc.)

Done.

[benjhayden, 2011-08-09 21:07:29]

Thanks for your time!
I'll submit this tomorrow morning after merging and recompiling if there aren't
any more comments, then I have a few CLs just about ready for review containing
the implementation.

[Mihai Parparita -not on Chrome, 2011-08-09 21:10:27]

Sounds good, still LGTM.

Mihai

On Tue, Aug 9, 2011 at 2:07 PM,  <benjhayden@chromium.org> wrote:
> Thanks for your time!
> I'll submit this tomorrow morning after merging and recompiling if there
> aren't
> any more comments, then I have a few CLs just about ready for review
> containing
> the implementation.
>
> http://codereview.chromium.org/7480046/
>

[commit-bot: I haz the power, 2011-08-15 16:17:16]

Change committed as 96778