Move chrome.downloads out of experimental to dev

Move chrome.downloads out of experimental to dev

Owners breakdown:
aa (LG'd): chrome/browser/extensions/*, chrome/common/extensions/extension_permission_set
mkearney: New docs are staged in https://www/~benjhayden/no_crawl/downloads_docs/downloads.html (sorry, only available inside Google)
everybody else: feel free to review or remove yourself as you wish


Currently implemented and documented: download(), search(), pause(), resume(), cancel(), getFileIcon(), onCreated, onChanged, onErased.

Move the implementation from chrome/browser/download to chrome/browser/extensions/api/downloads in preparation for using json_schema_compiler.

Restrict chrome.downloads to the dev channel.

Rewrite download() to use DownloadManager/DownloadUrlParameters.

Remove "headers[].binaryValue". UTF16 has the same code-point range as UTF8, is less standard, and a PITA in general. There are javascript solutions to converting UTF16->UTF8 and ArrayBuffer->string if an extension really needs it. I can't imagine a situation where an extension would need to pass an ArrayBuffer directly to download(), but if one arises then there are multiple backward-compatible solutions.

Fix ExtensionDownloadsEventRouter in incognito.

Rename DownloadDelta fields from 'old'/'new' to 'previous'/'current'. 'new' is frequently a reserved keyword, so even if the IDL parser accepted it and translated '_new' to 'new', it would still be highlighted confusingly in developers' editors.
OnChanged was never publicly documented, so this should not affect anything but my test.js.

Remove the properties from the API. The platform is moving away from #defines such as those. The json_schema_compiler doesn't even support them. It's just easier to type 'complete' than chrome.downloads.STATE_COMPLETE, and those strings (besides the errors) were never going to change, anyway.

I got tired of merging a very long line in api_index.html, so I added a newline in static/api_index.html.


TODO(benjhayden): Test download() and ExtensionDownloadsEventRouter in incognito. See WIP http://codereview.chromium.org/10542038/

BUG=117014, 87436
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=143432

[Aaron Boodman, 2012-03-20 05:33:50]

http://codereview.chromium.org/9617010/diff/2001/chrome/app/generated_resourc...
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/9617010/diff/2001/chrome/app/generated_resourc...
chrome/app/generated_resources.grd:4052: <message
name="IDS_EXTENSION_PROMPT_WARNING_DOWNLOADS" desc="Permission string for access
to downloads.">
Don't think we need a warning for being able to download files.

http://codereview.chromium.org/9617010/diff/2001/chrome/common/extensions/api...
File chrome/common/extensions/api/downloads.json (right):

http://codereview.chromium.org/9617010/diff/2001/chrome/common/extensions/api...
chrome/common/extensions/api/downloads.json:9: "ERROR_GENERIC": {
I realize what you're trying to do here, but I don't think any other APIs have
these kind of error constants. I'd rather be consistent until we have a sensible
error handling mechanism.

http://codereview.chromium.org/9617010/diff/2001/chrome/common/extensions/api...
chrome/common/extensions/api/downloads.json:27: "name": "download",
Shouldn't there be a change to download_extension_function.h to change the
DECLARE_EXTENSION_FUNCTION macro?

http://codereview.chromium.org/9617010/diff/2001/chrome/common/extensions/api...
chrome/common/extensions/api/downloads.json:29: "description": "Download a URL.
If the URL uses the HTTP[S] protocol, then the request will include all cookies
currently set for its hostname. If the download started successfully, |callback|
will be called with the new DownloadItem’s |id|. If there was an error starting
the download, then |callback| will be called with |downloadId|=undefined and
chrome.extension.lastError will be set. If the URL’s hostname is not specified
in the |permissions| section of the extension’s manifest, then the
|chrome.extensions.lastError| object will indicate that the extension does not
have permission to access that hostname.",
I assume cookies are sent with download requests? If so, we need to require host
permissions for this API, similar to what we do for e.g., the cookies API or
chrome.tabs.executeScript. This is an important security issue and we can't ship
without it. Sorry nobody noticed it earlier.

http://codereview.chromium.org/9617010/diff/2001/chrome/common/extensions/ext...
File chrome/common/extensions/extension_permission_set.h (right):

http://codereview.chromium.org/9617010/diff/2001/chrome/common/extensions/ext...
chrome/common/extensions/extension_permission_set.h:54: kDownloads,
This has changed. See _permission_features.json.

[benjhayden, 2012-05-31 20:07:56]

PTAL

I'm sorry that this CL looks like a nightmarish mythical creature. I can
separate out the json_schema_compiler changes if you want after a couple rounds
of high-level review (i.e. basic buy-in), but that still leaves a nightmarish
number of files. I'm open to suggestions for how to make this CL more
manageable.

LMK if you want to go through it verbally and I'll schedule a VC.

[Randy Smith (Not in Mondays), 2012-06-01 18:21:52]

On 2012/05/31 20:07:56, benjhayden_chromium wrote:
> PTAL
> 
> I'm sorry that this CL looks like a nightmarish mythical creature. I can
> separate out the json_schema_compiler changes if you want after a couple
rounds
> of high-level review (i.e. basic buy-in), but that still leaves a nightmarish
> number of files. I'm open to suggestions for how to make this CL more
> manageable.
> 
> LMK if you want to go through it verbally and I'll schedule a VC.

Would it be possible to separate out the following three into their own CLs? 
Would gain much in reducing the number of heads on the hydra?  

(I'm happy for you to get basic buy-in at this level, and if we have to review
it at this size, we'll do it, but if we can find ways to break it down that
would be better.)

---------

Rewrite download() to use DownloadManager/DownloadUrlParameters. 

Fix download() in incognito. 

Rename DownloadDelta fields from 'old'/'new' to 'previous'/'current'. 'new' is
frequently a reserved keyword, so even if the IDL parser accepted it and
translated '_new' to 'new', it would still be highlighted confusingly in
developers' editors. 
OnChanged was never publicly documented, so this should not affect anything but
my test.js.

[Randy Smith (Not in Mondays), 2012-06-01 18:25:17]

On 2012/06/01 18:21:52, rdsmith wrote:
> On 2012/05/31 20:07:56, benjhayden_chromium wrote:
> > PTAL
> > 
> > I'm sorry that this CL looks like a nightmarish mythical creature. I can
> > separate out the json_schema_compiler changes if you want after a couple
> rounds
> > of high-level review (i.e. basic buy-in), but that still leaves a
nightmarish
> > number of files. I'm open to suggestions for how to make this CL more
> > manageable.
> > 
> > LMK if you want to go through it verbally and I'll schedule a VC.
> 
> Would it be possible to separate out the following three into their own CLs? 
> Would gain much in reducing the number of heads on the hydra?  
> 
> (I'm happy for you to get basic buy-in at this level, and if we have to review
> it at this size, we'll do it, but if we can find ways to break it down that
> would be better.)

Aaaannndd ... looking over the CL, my belief is that I'm not particularly
relevant to basic buy-in, or even sugggestions about how to break this down into
manageable chunks, as most of it seems extension related.  Aaron?

> 
> ---------
> 
> Rewrite download() to use DownloadManager/DownloadUrlParameters. 
> 
> Fix download() in incognito. 
> 
> Rename DownloadDelta fields from 'old'/'new' to 'previous'/'current'. 'new' is
> frequently a reserved keyword, so even if the IDL parser accepted it and
> translated '_new' to 'new', it would still be highlighted confusingly in
> developers' editors. 
> OnChanged was never publicly documented, so this should not affect anything
but
> my test.js.

[benjhayden, 2012-06-01 18:46:10]

On 2012/06/01 18:21:52, rdsmith wrote:
> On 2012/05/31 20:07:56, benjhayden_chromium wrote:
> > PTAL
> > 
> > I'm sorry that this CL looks like a nightmarish mythical creature. I can
> > separate out the json_schema_compiler changes if you want after a couple
> rounds
> > of high-level review (i.e. basic buy-in), but that still leaves a
nightmarish
> > number of files. I'm open to suggestions for how to make this CL more
> > manageable.
> > 
> > LMK if you want to go through it verbally and I'll schedule a VC.
> 
> Would it be possible to separate out the following three into their own CLs? 
> Would gain much in reducing the number of heads on the hydra?  
> 
> (I'm happy for you to get basic buy-in at this level, and if we have to review
> it at this size, we'll do it, but if we can find ways to break it down that
> would be better.)
> 
> ---------
> 
> Rewrite download() to use DownloadManager/DownloadUrlParameters. 

This is a significant rewrite (~100 LoC), but it wouldn't change the number of
files changed by this CL, and it wouldn't change the amount of change to
downloads_api.cc much because args_ parsing changed so much.

> 
> Fix download() in incognito. 

This is about 5 LoC.
I'm still working out how to test download() in incognito. I think that will
involve creating a couple new apitest extensions.

> 
> Rename DownloadDelta fields from 'old'/'new' to 'previous'/'current'. 'new' is
> frequently a reserved keyword, so even if the IDL parser accepted it and
> translated '_new' to 'new', it would still be highlighted confusingly in
> developers' editors. 
> OnChanged was never publicly documented, so this should not affect anything
but
> my test.js.

This is just a find-all-replace that mostly impacted test.js. All of the changes
to test.js in this CL are find-all-replaces, so I don't think that backing out
one of them will be worth the effort.


So it's less of a hydra and more of a gorgon: one big ugly face (moving to
json_schema_compiler), lots of little snakes along for the ride.
Also, hydra don't petrify its victims, and this CL clearly does.

I'm glad that we could identify exactly which nightmarish mythical creature this
CL is.

[battre, 2012-06-01 20:43:45]

Please ask kalman for tools/json_schema_compiler/*,
ppapi/generators/idl_parser.py - he is your man and I am OOO for a week.

[benjhayden, 2012-06-01 20:47:16]

On 2012/06/01 20:43:45, battre wrote:
> Please ask kalman for tools/json_schema_compiler/*,
> ppapi/generators/idl_parser.py - he is your man and I am OOO for a week.

Thanks, Dominic!

[not at google - send to devlin, 2012-06-04 04:16:49]

Looked at common/extensions, the documentation generator, and the non-IDL stuff
in tools/json_schema_compiler.
 
final comment: Please add tests for this stuff into
tools/json_schema_compiler/tests.


I'm not the owner of the stuff in ppapi (nor have I seen the code before), so
when asargent looks at the IDL changes perhaps he can suggest a reviewer for
that.

P.S. I'm not sure what the story is with this change since it's almost 3 months
old.  Is the description up to date?

http://codereview.chromium.org/9617010/diff/49002/chrome/common/extensions/ap...
File chrome/common/extensions/api/downloads.idl (right):

http://codereview.chromium.org/9617010/diff/49002/chrome/common/extensions/ap...
chrome/common/extensions/api/downloads.idl:37: //
href='samples.html'>Samples</a>.</p>
I don't think that this is a desirable change.  Dealing with a code comment is
much more awkward than dealing with plain HTML... for example, syntax
highlighting doesn't work in editors.  Browsers can't open this file to preview
it.  You need to modify an IDL file to modify documentation, so it will be
harder to cherry-pick changes into releases.

Another data point: we're currently in the process of re-writing how
documentation works anyway.  Ping me if you want the design doc, but basically
it will remove needing to generate anything at all.

So if all this buys is needing to change only 1 file rather than 2, I'd rather
we didn't do it.

http://codereview.chromium.org/9617010/diff/49002/chrome/common/extensions/do...
File chrome/common/extensions/docs/js/api_page_generator.js (right):

http://codereview.chromium.org/9617010/diff/49002/chrome/common/extensions/do...
chrome/common/extensions/docs/js/api_page_generator.js:292:
document.getElementById('static').innerHTML = mod.description;
See my comment in the IDL file for why I think this shouldn't be done.

http://codereview.chromium.org/9617010/diff/49002/tools/json_schema_compiler/...
File tools/json_schema_compiler/cpp_type_generator.py (right):

http://codereview.chromium.org/9617010/diff/49002/tools/json_schema_compiler/...
tools/json_schema_compiler/cpp_type_generator.py:178: key=lambda
namespace_types: namespace_types[0].name):
Shouldn't we be sorting by namespace name, not type name?  In which case this
can be key=attrgetter('name').

nit: Incidentally the arguments to sort() should line up vertically.

http://codereview.chromium.org/9617010/diff/49002/tools/json_schema_compiler/...
tools/json_schema_compiler/cpp_type_generator.py:180: for type_ in types:
It occurs to me that these should probably be sorted by their name, too.

http://codereview.chromium.org/9617010/diff/49002/tools/json_schema_compiler/...
File tools/json_schema_compiler/h_generator.py (right):

http://codereview.chromium.org/9617010/diff/49002/tools/json_schema_compiler/...
tools/json_schema_compiler/h_generator.py:113:
prop.ref_type.startswith(self._namespace.name + '.')):
Perhaps add GetSchemaNamespace to schema_util.py then compare that to
self._namespace.name?

nit: also try not to break up expressions over multiple lines like this, and if
you do then make logical components line up vertically.  Easier to read.

http://codereview.chromium.org/9617010/diff/49002/tools/json_schema_compiler/...
tools/json_schema_compiler/h_generator.py:180: c.Substitute({'classname':
classname})
mind deleting the above line while you're here?

http://codereview.chromium.org/9617010/diff/49002/tools/json_schema_compiler/...
tools/json_schema_compiler/h_generator.py:184: # TODO: c.Sblock('enum
%(classname)s {') ... .Eblock('};')
Please expand on this TODO: add your name, and explain what doing that code
snippet would achieve and why you'd want to do it.

Or just do it now...?

http://codereview.chromium.org/9617010/diff/49002/tools/json_schema_compiler/...
File tools/json_schema_compiler/idl_schema.py (right):

http://codereview.chromium.org/9617010/diff/49002/tools/json_schema_compiler/...
tools/json_schema_compiler/idl_schema.py:105: def __init__(self, enum_node):
please get asargent@ to look at the contents of this file

[not at google - send to devlin, 2012-06-04 04:17:25]

+asargent

[benjhayden, 2012-06-04 14:09:32]

On 2012/06/04 04:16:49, kalman wrote:
> Looked at common/extensions, the documentation generator, and the non-IDL
stuff
> in tools/json_schema_compiler.
>  
> final comment: Please add tests for this stuff into
> tools/json_schema_compiler/tests.
> 
> 
> I'm not the owner of the stuff in ppapi (nor have I seen the code before), so
> when asargent looks at the IDL changes perhaps he can suggest a reviewer for
> that.
> 
> P.S. I'm not sure what the story is with this change since it's almost 3
months
> old.  Is the description up to date?

Just wanted to respond to this question first.
Yes, the description is up to date.
I've just been working on this gorgon of a CL off-and-on for a while in between
unrelated bugs.

Thanks for the quick turn-around! I'll address the rest of your comments and say
P-T-A-L when ready for another round.

Aaron: feel free to take a look at browser/extensions whenever you're ready.


> 
>
http://codereview.chromium.org/9617010/diff/49002/chrome/common/extensions/ap...
> File chrome/common/extensions/api/downloads.idl (right):
> 
>
http://codereview.chromium.org/9617010/diff/49002/chrome/common/extensions/ap...
> chrome/common/extensions/api/downloads.idl:37: //
> href='samples.html'>Samples</a>.</p>
> I don't think that this is a desirable change.  Dealing with a code comment is
> much more awkward than dealing with plain HTML... for example, syntax
> highlighting doesn't work in editors.  Browsers can't open this file to
preview
> it.  You need to modify an IDL file to modify documentation, so it will be
> harder to cherry-pick changes into releases.
> 
> Another data point: we're currently in the process of re-writing how
> documentation works anyway.  Ping me if you want the design doc, but basically
> it will remove needing to generate anything at all.
> 
> So if all this buys is needing to change only 1 file rather than 2, I'd rather
> we didn't do it.
> 
>
http://codereview.chromium.org/9617010/diff/49002/chrome/common/extensions/do...
> File chrome/common/extensions/docs/js/api_page_generator.js (right):
> 
>
http://codereview.chromium.org/9617010/diff/49002/chrome/common/extensions/do...
> chrome/common/extensions/docs/js/api_page_generator.js:292:
> document.getElementById('static').innerHTML = mod.description;
> See my comment in the IDL file for why I think this shouldn't be done.
> 
>
http://codereview.chromium.org/9617010/diff/49002/tools/json_schema_compiler/...
> File tools/json_schema_compiler/cpp_type_generator.py (right):
> 
>
http://codereview.chromium.org/9617010/diff/49002/tools/json_schema_compiler/...
> tools/json_schema_compiler/cpp_type_generator.py:178: key=lambda
> namespace_types: namespace_types[0].name):
> Shouldn't we be sorting by namespace name, not type name?  In which case this
> can be key=attrgetter('name').
> 
> nit: Incidentally the arguments to sort() should line up vertically.
> 
>
http://codereview.chromium.org/9617010/diff/49002/tools/json_schema_compiler/...
> tools/json_schema_compiler/cpp_type_generator.py:180: for type_ in types:
> It occurs to me that these should probably be sorted by their name, too.
> 
>
http://codereview.chromium.org/9617010/diff/49002/tools/json_schema_compiler/...
> File tools/json_schema_compiler/h_generator.py (right):
> 
>
http://codereview.chromium.org/9617010/diff/49002/tools/json_schema_compiler/...
> tools/json_schema_compiler/h_generator.py:113:
> prop.ref_type.startswith(self._namespace.name + '.')):
> Perhaps add GetSchemaNamespace to schema_util.py then compare that to
> self._namespace.name?
> 
> nit: also try not to break up expressions over multiple lines like this, and
if
> you do then make logical components line up vertically.  Easier to read.
> 
>
http://codereview.chromium.org/9617010/diff/49002/tools/json_schema_compiler/...
> tools/json_schema_compiler/h_generator.py:180: c.Substitute({'classname':
> classname})
> mind deleting the above line while you're here?
> 
>
http://codereview.chromium.org/9617010/diff/49002/tools/json_schema_compiler/...
> tools/json_schema_compiler/h_generator.py:184: # TODO: c.Sblock('enum
> %(classname)s {') ... .Eblock('};')
> Please expand on this TODO: add your name, and explain what doing that code
> snippet would achieve and why you'd want to do it.
> 
> Or just do it now...?
> 
>
http://codereview.chromium.org/9617010/diff/49002/tools/json_schema_compiler/...
> File tools/json_schema_compiler/idl_schema.py (right):
> 
>
http://codereview.chromium.org/9617010/diff/49002/tools/json_schema_compiler/...
> tools/json_schema_compiler/idl_schema.py:105: def __init__(self, enum_node):
> please get asargent@ to look at the contents of this file

[benjhayden, 2012-06-04 20:33:15]

PTAL

http://codereview.chromium.org/9617010/diff/49002/chrome/common/extensions/ap...
File chrome/common/extensions/api/downloads.idl (right):

http://codereview.chromium.org/9617010/diff/49002/chrome/common/extensions/ap...
chrome/common/extensions/api/downloads.idl:37: //
href='samples.html'>Samples</a>.</p>
On 2012/06/04 04:16:49, kalman wrote:
> I don't think that this is a desirable change.  Dealing with a code comment is
> much more awkward than dealing with plain HTML... for example, syntax
> highlighting doesn't work in editors.  Browsers can't open this file to
preview
> it.  You need to modify an IDL file to modify documentation, so it will be
> harder to cherry-pick changes into releases.
> 
> Another data point: we're currently in the process of re-writing how
> documentation works anyway.  Ping me if you want the design doc, but basically
> it will remove needing to generate anything at all.

If you have a link handy, I'm curious, otherwise don't spend too much time
writing it up.

> 
> So if all this buys is needing to change only 1 file rather than 2, I'd rather
> we didn't do it.

Done.

http://codereview.chromium.org/9617010/diff/49002/chrome/common/extensions/do...
File chrome/common/extensions/docs/js/api_page_generator.js (right):

http://codereview.chromium.org/9617010/diff/49002/chrome/common/extensions/do...
chrome/common/extensions/docs/js/api_page_generator.js:292:
document.getElementById('static').innerHTML = mod.description;
On 2012/06/04 04:16:49, kalman wrote:
> See my comment in the IDL file for why I think this shouldn't be done.

Done.

http://codereview.chromium.org/9617010/diff/49002/tools/json_schema_compiler/...
File tools/json_schema_compiler/cpp_type_generator.py (right):

http://codereview.chromium.org/9617010/diff/49002/tools/json_schema_compiler/...
tools/json_schema_compiler/cpp_type_generator.py:178: key=lambda
namespace_types: namespace_types[0].name):
On 2012/06/04 04:16:49, kalman wrote:
> Shouldn't we be sorting by namespace name, not type name?  In which case this
> can be key=attrgetter('name').

namespace_types is a 2-tuple (namespace, types). It doesn't look like i can
compose itemgetter with attrgetter.

> 
> nit: Incidentally the arguments to sort() should line up vertically.

Done.

http://codereview.chromium.org/9617010/diff/49002/tools/json_schema_compiler/...
tools/json_schema_compiler/cpp_type_generator.py:180: for type_ in types:
On 2012/06/04 04:16:49, kalman wrote:
> It occurs to me that these should probably be sorted by their name, too.

Done.

http://codereview.chromium.org/9617010/diff/49002/tools/json_schema_compiler/...
File tools/json_schema_compiler/h_generator.py (right):

http://codereview.chromium.org/9617010/diff/49002/tools/json_schema_compiler/...
tools/json_schema_compiler/h_generator.py:113:
prop.ref_type.startswith(self._namespace.name + '.')):
On 2012/06/04 04:16:49, kalman wrote:
> Perhaps add GetSchemaNamespace to schema_util.py then compare that to
> self._namespace.name?
> 
> nit: also try not to break up expressions over multiple lines like this, and
if
> you do then make logical components line up vertically.  Easier to read.

Done.

http://codereview.chromium.org/9617010/diff/49002/tools/json_schema_compiler/...
tools/json_schema_compiler/h_generator.py:180: c.Substitute({'classname':
classname})
On 2012/06/04 04:16:49, kalman wrote:
> mind deleting the above line while you're here?

Done.

http://codereview.chromium.org/9617010/diff/49002/tools/json_schema_compiler/...
tools/json_schema_compiler/h_generator.py:184: # TODO: c.Sblock('enum
%(classname)s {') ... .Eblock('};')
On 2012/06/04 04:16:49, kalman wrote:
> Please expand on this TODO: add your name, and explain what doing that code
> snippet would achieve and why you'd want to do it.
> 
> Or just do it now...?

Done.

http://codereview.chromium.org/9617010/diff/49002/tools/json_schema_compiler/...
File tools/json_schema_compiler/idl_schema.py (right):

http://codereview.chromium.org/9617010/diff/49002/tools/json_schema_compiler/...
tools/json_schema_compiler/idl_schema.py:105: def __init__(self, enum_node):
On 2012/06/04 04:16:49, kalman wrote:
> please get asargent@ to look at the contents of this file

Done.

[Aaron Boodman, 2012-06-05 04:42:59]

http://codereview.chromium.org/9617010/diff/64002/chrome/common/extensions/ap...
File chrome/common/extensions/api/_permission_features.json (right):

http://codereview.chromium.org/9617010/diff/64002/chrome/common/extensions/ap...
chrome/common/extensions/api/_permission_features.json:101: "extension",
"packaged_app", "hosted_app", "platform_app"
Remove platform_app and hosted_app.

http://codereview.chromium.org/9617010/diff/64002/chrome/common/extensions/ap...
File chrome/common/extensions/api/downloads.idl (right):

http://codereview.chromium.org/9617010/diff/64002/chrome/common/extensions/ap...
chrome/common/extensions/api/downloads.idl:5: [permissions=downloads] namespace
downloads {
Can this go on the line above?

http://codereview.chromium.org/9617010/diff/64002/chrome/common/extensions/ap...
chrome/common/extensions/api/downloads.idl:331: [nodoc] static void
erase(DownloadQuery query, optional EraseCallback callback);
Why are these all nodoc?

[benjhayden, 2012-06-05 13:42:32]

PTAL

http://codereview.chromium.org/9617010/diff/64002/chrome/common/extensions/ap...
File chrome/common/extensions/api/_permission_features.json (right):

http://codereview.chromium.org/9617010/diff/64002/chrome/common/extensions/ap...
chrome/common/extensions/api/_permission_features.json:101: "extension",
"packaged_app", "hosted_app", "platform_app"
On 2012/06/05 04:43:00, Aaron Boodman wrote:
> Remove platform_app and hosted_app.

Done.

http://codereview.chromium.org/9617010/diff/64002/chrome/common/extensions/ap...
File chrome/common/extensions/api/downloads.idl (right):

http://codereview.chromium.org/9617010/diff/64002/chrome/common/extensions/ap...
chrome/common/extensions/api/downloads.idl:5: [permissions=downloads] namespace
downloads {
On 2012/06/05 04:43:00, Aaron Boodman wrote:
> Can this go on the line above?

Done.

http://codereview.chromium.org/9617010/diff/64002/chrome/common/extensions/ap...
chrome/common/extensions/api/downloads.idl:331: [nodoc] static void
erase(DownloadQuery query, optional EraseCallback callback);
On 2012/06/05 04:43:00, Aaron Boodman wrote:
> Why are these all nodoc?

They aren't implemented yet.

[asargent_no_longer_on_chrome, 2012-06-07 00:18:07]

Apologies for the latency on the review - I was on vacation for several days and
am now digging myself out.

http://codereview.chromium.org/9617010/diff/71002/chrome/common/extensions/ap...
File chrome/common/extensions/api/downloads.idl (right):

http://codereview.chromium.org/9617010/diff/71002/chrome/common/extensions/ap...
chrome/common/extensions/api/downloads.idl:16: long[]? binaryValue;
Note that you can use "ArrayBuffer?" here instead if you want - the
serialization of the data should then be more efficient, and the developer can
then use any of the *ArrayBufferView classes to read data from the buffer.
https://developer.mozilla.org/en/JavaScript_typed_arrays/ArrayBufferView

http://codereview.chromium.org/9617010/diff/71002/chrome/common/extensions/ap...
chrome/common/extensions/api/downloads.idl:253: enum IconSize {SMALL = 16, LARGE
= 32};
Unfortunately WebIDL only supports strings for enum values, so while the
underlying parser we share with PPAPI deviates from WebIDL here and in several
other ways, I don't want us to for our APIs. 

http://dev.w3.org/2006/webapi/WebIDL/#idl-enums

(BTW, we currently don't properly require quotes around the string values inside
enums, but I have an open bug for fixing that - crbug.com/130201 )


My preference is to stick to the WebIDL spec as closely as possible, and use the
relief valve of extended attributes to annotate anything we want to specify that
WebIDL doesn't have a construction for. 

So, we could combine typedef and extended attribute support to achieve something
similar to what you have here. For example, we might do:

typedef [legalValues=(16,32)] long IconSize;

dictionary GetFileIconOptions {
  IconSize? size;
};

http://codereview.chromium.org/9617010/diff/71002/ppapi/generators/idl_parser.py
File ppapi/generators/idl_parser.py (right):

http://codereview.chromium.org/9617010/diff/71002/ppapi/generators/idl_parser...
ppapi/generators/idl_parser.py:227: p[0] = ListFromConcat(Copyright, p[2], p[3])
Unfortunately there are some specific assumptions about the parse tree of some
Pepper API IDL files that I think this change might break. I agree with kalman
that we probably want to keep overview documentation in the .html files and just
use comments in the .idl file for narrow plaintext documentation of specific
functions/parameters/objects.

http://codereview.chromium.org/9617010/diff/71002/tools/json_schema_compiler/...
File tools/json_schema_compiler/idl_schema.py (right):

http://codereview.chromium.org/9617010/diff/71002/tools/json_schema_compiler/...
tools/json_schema_compiler/idl_schema.py:111: enum_type = 'integer'
As I mentioned in another comment, the WebIDL spec unfortunately does not allow
anything except string constants in enums, so we don't want to add
integer/float/double support.

[not at google - send to devlin, 2012-06-07 03:00:53]

Sorry for the lengthy reviewing!

http://codereview.chromium.org/9617010/diff/71002/chrome/common/extensions/ap...
File chrome/common/extensions/api/alarms.idl (left):

http://codereview.chromium.org/9617010/diff/71002/chrome/common/extensions/ap...
chrome/common/extensions/api/alarms.idl:6: // TODO(mpcomplete): We need
documentation before we can release this.
Do we no longer require file-level comments?

http://codereview.chromium.org/9617010/diff/71002/tools/json_schema_compiler/...
File tools/json_schema_compiler/cpp_type_generator.py (right):

http://codereview.chromium.org/9617010/diff/71002/tools/json_schema_compiler/...
tools/json_schema_compiler/cpp_type_generator.py:179: return
namespace_types[0].name
I think this would be easier to read like

def SortByNamespace(namespace_and_types):
  (namespace, types) = namespace_and_types
  return namespace.name

http://codereview.chromium.org/9617010/diff/71002/tools/json_schema_compiler/...
tools/json_schema_compiler/cpp_type_generator.py:187: for stripped_type_name,
type_json in sorted(
this isn't json, it's a type. So call this type_

http://codereview.chromium.org/9617010/diff/71002/tools/json_schema_compiler/...
tools/json_schema_compiler/cpp_type_generator.py:189: key=itemgetter(0)):
I've been starting at this code for like 20 minutes and only now really
understand what's going on.  My Python parsing skills need much improvement yes,
but all the mapping and sorting and closures stuff is too convoluted for my
little brain.

Could we be consistent with the SortByNamespace function and have a
SortByTypename function next to it or something. Then

for type_ in sorted(types, key=SortByTypename):
  type_name = schema_util.StripSchemaNamespace(type_);
  ...

I would find that much easier to understand.

http://codereview.chromium.org/9617010/diff/71002/tools/json_schema_compiler/...
tools/json_schema_compiler/cpp_type_generator.py:195:
self.GetType(type_json.item_type,
mind fixing up this indentation while you're here?

c.Substitute({
  'name': stripped_type_name,
  'item_type': self.GetType(type_json.item_type, wrap_optional=True),
})

http://codereview.chromium.org/9617010/diff/71002/tools/json_schema_compiler/...
File tools/json_schema_compiler/h_generator.py (right):

http://codereview.chromium.org/9617010/diff/71002/tools/json_schema_compiler/...
tools/json_schema_compiler/h_generator.py:112: if
schema_util.IsLocalReference(prop, self._namespace.name):
IsReferenceInNamespace?

though I would still have preferred

if prop.type_ == PropertyType.REF and \
   schema_util.GetNamespace(prop.ref_type) != self._namespace.name:

because passing a function that only operates on a certain type (here, REF)
shouldn't need to check that what it's passed is that type. it should raise an
error if it's not the type that it's passed.

besides which, we shouldn't be adding such specific functionality to util
classes.  over-specific util classes (util classes in general, actually) tend to
just make the code that uses them harder to understand, at least, for me.

http://codereview.chromium.org/9617010/diff/71002/tools/json_schema_compiler/...
tools/json_schema_compiler/h_generator.py:191: # double enums.
We do generate code for constants that are defined inside properties.

Anyway, if I understand this correctly, this comment only applies for when the
type is an integer... and an enum? So a more accurate TODO would be

TODO: if this type is an enum, generate an enum block with the corresponding
values.

that said, I don't know if that'd even be possible, since those enums don't have
names... they're just part of the constraints of the types themselves, and we
don't do that kind of validation as part of this compiler.

So yeah, in summary: I don't think this should be a TODO :)

[Randy Smith (Not in Mondays), 2012-06-07 16:29:16]

Removing myself from review list; I don't see how my expertise is useful here,
and I don't think there are any learning curves that need to be climbed.  Put me
back on (with specific requests) if either of those assumptions isn't true.

http://codereview.chromium.org/9617010/diff/81003/chrome/common/extensions/ap...
File chrome/common/extensions/api/alarms.idl (left):

http://codereview.chromium.org/9617010/diff/81003/chrome/common/extensions/ap...
chrome/common/extensions/api/alarms.idl:6: // TODO(mpcomplete): We need
documentation before we can release this.
Random question: I understand how your change could have resulted in the
appropriate removal of line 5, but should line 6 have gone away too?

[benjhayden, 2012-06-07 20:47:16]

On 2012/06/07 16:29:16, rdsmith wrote:
> Removing myself from review list; I don't see how my expertise is useful here,
> and I don't think there are any learning curves that need to be climbed.  Put
me
> back on (with specific requests) if either of those assumptions isn't true.

SGTM. Just one specific question:
Are you ok with having ChromeDownloadManagerDelegate create and own
ExtensionDownloadsEventRouter?
Like ChromeDownloadManagerDelegate, EDER is a chrome-level concept and its
lifetime should match DownloadManager. There should be separate EDERs for
on-record and off-record managers. Things like EDER usually belong in
ExtensionSystem, but there does not appear to be separate ExtensionSystems for
on-record and off-record profiles, so it cannot own EDER.


> 
>
http://codereview.chromium.org/9617010/diff/81003/chrome/common/extensions/ap...
> File chrome/common/extensions/api/alarms.idl (left):
> 
>
http://codereview.chromium.org/9617010/diff/81003/chrome/common/extensions/ap...
> chrome/common/extensions/api/alarms.idl:6: // TODO(mpcomplete): We need
> documentation before we can release this.
> Random question: I understand how your change could have resulted in the
> appropriate removal of line 5, but should line 6 have gone away too?

[Randy Smith (Not in Mondays), 2012-06-07 21:11:10]

On 2012/06/07 20:47:16, benjhayden_chromium wrote:
> On 2012/06/07 16:29:16, rdsmith wrote:
> > Removing myself from review list; I don't see how my expertise is useful
here,
> > and I don't think there are any learning curves that need to be climbed. 
Put
> me
> > back on (with specific requests) if either of those assumptions isn't true.
> 
> SGTM. Just one specific question:
> Are you ok with having ChromeDownloadManagerDelegate create and own
> ExtensionDownloadsEventRouter?
> Like ChromeDownloadManagerDelegate, EDER is a chrome-level concept and its
> lifetime should match DownloadManager. There should be separate EDERs for
> on-record and off-record managers. Things like EDER usually belong in
> ExtensionSystem, but there does not appear to be separate ExtensionSystems for
> on-record and off-record profiles, so it cannot own EDER.

I'm ok with that--as best I understand the structure of the system, that's the
right thing.  Are there any other subsystems with parallel situations?  I.e.
primary implementation in content, but requiring an event router in chrome?  If
so, modeling what we do on those systems might be reasonable.  If not, I'm fine
with CDMD owning EDER.

> 
> 
> > 
> >
>
http://codereview.chromium.org/9617010/diff/81003/chrome/common/extensions/ap...
> > File chrome/common/extensions/api/alarms.idl (left):
> > 
> >
>
http://codereview.chromium.org/9617010/diff/81003/chrome/common/extensions/ap...
> > chrome/common/extensions/api/alarms.idl:6: // TODO(mpcomplete): We need
> > documentation before we can release this.
> > Random question: I understand how your change could have resulted in the
> > appropriate removal of line 5, but should line 6 have gone away too?

[Aaron Boodman, 2012-06-07 21:55:05]

LGTM / hand-off to kalman and asargent

[benjhayden, 2012-06-08 16:41:52]

Thanks, Aaron!

Kalman, Asargent, PTAL.

http://codereview.chromium.org/9617010/diff/71002/chrome/common/extensions/ap...
File chrome/common/extensions/api/alarms.idl (left):

http://codereview.chromium.org/9617010/diff/71002/chrome/common/extensions/ap...
chrome/common/extensions/api/alarms.idl:6: // TODO(mpcomplete): We need
documentation before we can release this.
On 2012/06/07 03:00:54, kalman wrote:
> Do we no longer require file-level comments?

See ongoing discussion in
http://codereview.chromium.org/9617010/diff/71002/ppapi/generators/idl_parser...

http://codereview.chromium.org/9617010/diff/71002/chrome/common/extensions/ap...
File chrome/common/extensions/api/downloads.idl (right):

http://codereview.chromium.org/9617010/diff/71002/chrome/common/extensions/ap...
chrome/common/extensions/api/downloads.idl:16: long[]? binaryValue;
On 2012/06/07 00:18:08, Antony Sargent wrote:
> Note that you can use "ArrayBuffer?" here instead if you want - the
> serialization of the data should then be more efficient, and the developer can
> then use any of the *ArrayBufferView classes to read data from the buffer.
> https://developer.mozilla.org/en/JavaScript_typed_arrays/ArrayBufferView
> 

Done.

http://codereview.chromium.org/9617010/diff/71002/chrome/common/extensions/ap...
chrome/common/extensions/api/downloads.idl:253: enum IconSize {SMALL = 16, LARGE
= 32};
On 2012/06/07 00:18:08, Antony Sargent wrote:
> Unfortunately WebIDL only supports strings for enum values, so while the
> underlying parser we share with PPAPI deviates from WebIDL here and in several
> other ways, I don't want us to for our APIs. 
> 
> http://dev.w3.org/2006/webapi/WebIDL/#idl-enums
> 
> (BTW, we currently don't properly require quotes around the string values
inside
> enums, but I have an open bug for fixing that - crbug.com/130201 )
> 
> 
> My preference is to stick to the WebIDL spec as closely as possible, and use
the
> relief valve of extended attributes to annotate anything we want to specify
that
> WebIDL doesn't have a construction for. 
> 
> So, we could combine typedef and extended attribute support to achieve
something
> similar to what you have here. For example, we might do:
> 
> typedef [legalValues=(16,32)] long IconSize;
> 
> dictionary GetFileIconOptions {
>   IconSize? size;
> };
> 

Done.

http://codereview.chromium.org/9617010/diff/71002/ppapi/generators/idl_parser.py
File ppapi/generators/idl_parser.py (right):

http://codereview.chromium.org/9617010/diff/71002/ppapi/generators/idl_parser...
ppapi/generators/idl_parser.py:227: p[0] = ListFromConcat(Copyright, p[2], p[3])
On 2012/06/07 00:18:08, Antony Sargent wrote:
> Unfortunately there are some specific assumptions about the parse tree of some
> Pepper API IDL files that I think this change might break. I agree with kalman
> that we probably want to keep overview documentation in the .html files and
just
> use comments in the .idl file for narrow plaintext documentation of specific
> functions/parameters/objects. 

I moved the documentation back into the html file, but there would still be
TODOs in IDL files about this issue if I removed p_top_short().

Note that p_top_short() does not replace p_top(), so ppapi IDLs can continue to
use and rely on filedocs.

Do I really need to remove p_top_short()? If so, can you elaborate?

http://codereview.chromium.org/9617010/diff/71002/tools/json_schema_compiler/...
File tools/json_schema_compiler/cpp_type_generator.py (right):

http://codereview.chromium.org/9617010/diff/71002/tools/json_schema_compiler/...
tools/json_schema_compiler/cpp_type_generator.py:179: return
namespace_types[0].name
On 2012/06/07 03:00:54, kalman wrote:
> I think this would be easier to read like
> 
> def SortByNamespace(namespace_and_types):
>   (namespace, types) = namespace_and_types
>   return namespace.name

Done.

http://codereview.chromium.org/9617010/diff/71002/tools/json_schema_compiler/...
tools/json_schema_compiler/cpp_type_generator.py:187: for stripped_type_name,
type_json in sorted(
On 2012/06/07 03:00:54, kalman wrote:
> this isn't json, it's a type. So call this type_

That makes it difficult to see how it's used because it's a substring of
'stripped_type_name', so how does 'type_obj' look?

http://codereview.chromium.org/9617010/diff/71002/tools/json_schema_compiler/...
tools/json_schema_compiler/cpp_type_generator.py:189: key=itemgetter(0)):
On 2012/06/07 03:00:54, kalman wrote:
> I've been starting at this code for like 20 minutes and only now really
> understand what's going on.  My Python parsing skills need much improvement
yes,
> but all the mapping and sorting and closures stuff is too convoluted for my
> little brain.
> 
> Could we be consistent with the SortByNamespace function and have a
> SortByTypename function next to it or something. Then
> 
> for type_ in sorted(types, key=SortByTypename):
>   type_name = schema_util.StripSchemaNamespace(type_);
>   ...
> 
> I would find that much easier to understand.

I wanted to avoid running StripSchemaNamespace twice.

How does the new list-comprehension look?

http://codereview.chromium.org/9617010/diff/71002/tools/json_schema_compiler/...
tools/json_schema_compiler/cpp_type_generator.py:195:
self.GetType(type_json.item_type,
On 2012/06/07 03:00:54, kalman wrote:
> mind fixing up this indentation while you're here?
> 
> c.Substitute({
>   'name': stripped_type_name,
>   'item_type': self.GetType(type_json.item_type, wrap_optional=True),
> })

Done.

http://codereview.chromium.org/9617010/diff/71002/tools/json_schema_compiler/...
File tools/json_schema_compiler/h_generator.py (right):

http://codereview.chromium.org/9617010/diff/71002/tools/json_schema_compiler/...
tools/json_schema_compiler/h_generator.py:112: if
schema_util.IsLocalReference(prop, self._namespace.name):
On 2012/06/07 03:00:54, kalman wrote:
> IsReferenceInNamespace?
> 
> though I would still have preferred
> 
> if prop.type_ == PropertyType.REF and \
>    schema_util.GetNamespace(prop.ref_type) != self._namespace.name:
> 
> because passing a function that only operates on a certain type (here, REF)
> shouldn't need to check that what it's passed is that type. it should raise an
> error if it's not the type that it's passed.
> 
> besides which, we shouldn't be adding such specific functionality to util
> classes.  over-specific util classes (util classes in general, actually) tend
to
> just make the code that uses them harder to understand, at least, for me.

Done.

http://codereview.chromium.org/9617010/diff/71002/tools/json_schema_compiler/...
tools/json_schema_compiler/h_generator.py:191: # double enums.
On 2012/06/07 03:00:54, kalman wrote:
> We do generate code for constants that are defined inside properties.
> 
> Anyway, if I understand this correctly, this comment only applies for when the
> type is an integer... and an enum? So a more accurate TODO would be
> 
> TODO: if this type is an enum, generate an enum block with the corresponding
> values.
> 
> that said, I don't know if that'd even be possible, since those enums don't
have
> names... they're just part of the constraints of the types themselves, and we
> don't do that kind of validation as part of this compiler.
> 
> So yeah, in summary: I don't think this should be a TODO :)

Antony suggested using extended attributes to be more compliant with the WebIDL
spec:
http://codereview.chromium.org/9617010/diff/71002/chrome/common/extensions/ap...

http://codereview.chromium.org/9617010/diff/71002/tools/json_schema_compiler/...
File tools/json_schema_compiler/idl_schema.py (right):

http://codereview.chromium.org/9617010/diff/71002/tools/json_schema_compiler/...
tools/json_schema_compiler/idl_schema.py:111: enum_type = 'integer'
On 2012/06/07 00:18:08, Antony Sargent wrote:
> As I mentioned in another comment, the WebIDL spec unfortunately does not
allow
> anything except string constants in enums, so we don't want to add
> integer/float/double support.

Done.

http://codereview.chromium.org/9617010/diff/81003/chrome/common/extensions/ap...
File chrome/common/extensions/api/alarms.idl (left):

http://codereview.chromium.org/9617010/diff/81003/chrome/common/extensions/ap...
chrome/common/extensions/api/alarms.idl:6: // TODO(mpcomplete): We need
documentation before we can release this.
On 2012/06/07 16:29:17, rdsmith wrote:
> Random question: I understand how your change could have resulted in the
> appropriate removal of line 5, but should line 6 have gone away too?

I'll ask mpcomplete, but chrome.alarms is already visible on the dev channel.
It's just missing a top-level doc.
http://code.google.com/chrome/extensions/dev/alarms.html

[battre, 2012-06-08 17:50:56]

Quick comment on the side: chrome.downloads contains the evaluation of RegEx via
ICU in the browser process. We are hesitating to launch this for the declarative
WebRequest API for security reasons and plan to replace this with RE2. I wonder
whether this blocks the release of chrome.downloads out of experimental.

The migration to RE2 is trivial (http://codereview.chromium.org/10387190) as
soon as we have RE2 in the code base, which is less trivial.

[benjhayden, 2012-06-08 18:29:50]

On 2012/06/08 17:50:56, battre wrote:
> Quick comment on the side: chrome.downloads contains the evaluation of RegEx
via
> ICU in the browser process. We are hesitating to launch this for the
declarative
> WebRequest API for security reasons and plan to replace this with RE2. I
wonder
> whether this blocks the release of chrome.downloads out of experimental.
> 
> The migration to RE2 is trivial (http://codereview.chromium.org/10387190) as
> soon as we have RE2 in the code base, which is less trivial.

Can you describe the security issue briefly?
Do you have an ETA for RE2?
Is there a crbug.com issue?
Who should we talk to about whether to block chrome.downloads for this? jschuh?

[Aaron Boodman, 2012-06-08 18:55:37]

I thought I talked about the regex issue with one of you before, but maybe I
forogt...

It's fine to move from experimental to dev as you are now (if anything, I think
this is an improvement). But moving beyond dev won't be possible until you
switch to re2 or sandbox the regex evaluation.

If you decide to move to re2 (which I suggest), please ensure that you have
designed the APIs such that it would be possible to move regular expression
evaluation into a sandbox if necessary. The API shouldn't assume that regex
evaulation is synchronous.

Note also that re2 has slightly different syntax for captures groups than ICU.
Dominic has some code that handles the differences. See
http://codereview.chromium.org/10449069/ for details.

===

Background on security:

As a matter of principle, Chrome sandboxes interpretation of any moderately
complex format by potentially malicious code. For example, we sandbox jpeg, png,
xml, json, html, javascript, and many other formats. The issue is just that it's
hard to write a secure interpreter or parser, and security issues with such code
are very common.

We're willing to make an experimental exception for RE2 when used by extensions,
because: a) it's a bit harder to encounter malicious extensions than malicious
web contnet, b) re2 guarantees runtime and memory bounds, c) re2 has done well
in fuzz testing, and c) we have designed the APIs (in webRequest at least) such
that we *could* sandbox later if necessary.

[benjhayden, 2012-06-11 15:06:40]

On 2012/06/08 18:55:37, Aaron Boodman wrote:
> I thought I talked about the regex issue with one of you before, but maybe I
> forogt...
> 
> It's fine to move from experimental to dev as you are now (if anything, I
think
> this is an improvement). But moving beyond dev won't be possible until you
> switch to re2 or sandbox the regex evaluation.
> 
> If you decide to move to re2 (which I suggest), please ensure that you have
> designed the APIs such that it would be possible to move regular expression
> evaluation into a sandbox if necessary. The API shouldn't assume that regex
> evaulation is synchronous.
> 
> Note also that re2 has slightly different syntax for captures groups than ICU.
> Dominic has some code that handles the differences. See
> http://codereview.chromium.org/10449069/ for details.
> 
> ===
> 
> Background on security:
> 
> As a matter of principle, Chrome sandboxes interpretation of any moderately
> complex format by potentially malicious code. For example, we sandbox jpeg,
png,
> xml, json, html, javascript, and many other formats. The issue is just that
it's
> hard to write a secure interpreter or parser, and security issues with such
code
> are very common.
> 
> We're willing to make an experimental exception for RE2 when used by
extensions,
> because: a) it's a bit harder to encounter malicious extensions than malicious
> web contnet, b) re2 guarantees runtime and memory bounds, c) re2 has done well
> in fuzz testing, and c) we have designed the APIs (in webRequest at least)
such
> that we *could* sandbox later if necessary.

DownloadsSearchFunction is currently a SyncExtensionFunction, but it could
easily be converted into an AsyncExtensionFunction, and DownloadQuery is built
entirely out of Callbacks, so it would be easy to convert
DownloadQuery::Search() to async, also.

Thanks!

[not at google - send to devlin, 2012-06-12 18:34:38]

lgtm

http://codereview.chromium.org/9617010/diff/84005/tools/json_schema_compiler/...
File tools/json_schema_compiler/cc_generator.py (left):

http://codereview.chromium.org/9617010/diff/84005/tools/json_schema_compiler/...
tools/json_schema_compiler/cc_generator.py:48: .Append('using
base::BinaryValue;')
why?

This is a cc file. I would prefer to keep the using.

http://codereview.chromium.org/9617010/diff/84005/tools/json_schema_compiler/...
File tools/json_schema_compiler/cc_generator.py (right):

http://codereview.chromium.org/9617010/diff/84005/tools/json_schema_compiler/...
tools/json_schema_compiler/cc_generator.py:492:
c.Append('%(dst)s->%(name)s.reset(new ' + any_helper.ANY_CLASS + '());')
Is this the only non *Value -> base::*Value change to this file?

Mind reverting the changes to this file then just adding this back?

http://codereview.chromium.org/9617010/diff/84005/tools/json_schema_compiler/...
File tools/json_schema_compiler/h_generator.py (right):

http://codereview.chromium.org/9617010/diff/84005/tools/json_schema_compiler/...
tools/json_schema_compiler/h_generator.py:202:
.Append('scoped_ptr<base::DictionaryValue> ToValue() const;')
thanks :)

http://codereview.chromium.org/9617010/diff/84005/tools/json_schema_compiler/...
File tools/json_schema_compiler/model.py (right):

http://codereview.chromium.org/9617010/diff/84005/tools/json_schema_compiler/...
tools/json_schema_compiler/model.py:194: for value in json['enum']):
Could you move the second check inside this block? As it stands this will fall
through to a generic ParseException; we should at least throw one with a nicer
error message (if one at all).

[asargent_no_longer_on_chrome, 2012-06-12 19:08:16]

http://codereview.chromium.org/9617010/diff/84005/chrome/common/extensions/ap...
File chrome/common/extensions/api/downloads.idl (right):

http://codereview.chromium.org/9617010/diff/84005/chrome/common/extensions/ap...
chrome/common/extensions/api/downloads.idl:27: [legalValues=(GET, POST)]
DOMString? method;
we can use enums for string values, just not integer/float/etc.

So this should probably be:

enum Method { GET, POST };

[nodoc] dictionary DownloadOptions {
  ...
  Method method;
  ...
};

http://codereview.chromium.org/9617010/diff/84005/chrome/common/extensions/ap...
chrome/common/extensions/api/downloads.idl:264: [inline_doc] DownloadLongDiff?
fileSize;
General comment:

It seems weird that we need to use use [inline_doc] so many places - should we
switch to having that be the default, and you have to explicitly opt-out of it?

I guess I don't really understand what inline_doc means and why some
parameters/properties have are annotated with it and others aren't.

http://codereview.chromium.org/9617010/diff/84005/chrome/common/extensions/ap...
chrome/common/extensions/api/downloads.idl:340: [inline_doc] optional
GetFileIconOptions options,
It seems kind of weird that the options paramter here is marked inline_doc, but
the GetFileIconOptions type is marked as nodoc.

http://codereview.chromium.org/9617010/diff/84005/ppapi/generators/idl_parser.py
File ppapi/generators/idl_parser.py (right):

http://codereview.chromium.org/9617010/diff/84005/ppapi/generators/idl_parser...
ppapi/generators/idl_parser.py:379: def p_ext_attr_list_values(self, p):
I don't see you reference this p_ext_attr_list_values (or any of the following
added ones) in a production that is reachable from the top level - do you still
need it?

http://codereview.chromium.org/9617010/diff/84005/ppapi/generators/idl_parser...
ppapi/generators/idl_parser.py:533: def p_expression_float(self, p):
Do you still need this? I don't see any literal float values in your
downloads.idl file.

[benjhayden, 2012-06-12 20:15:42]

Thanks! PTAL.

http://codereview.chromium.org/9617010/diff/84005/chrome/common/extensions/ap...
File chrome/common/extensions/api/downloads.idl (right):

http://codereview.chromium.org/9617010/diff/84005/chrome/common/extensions/ap...
chrome/common/extensions/api/downloads.idl:27: [legalValues=(GET, POST)]
DOMString? method;
On 2012/06/12 19:08:16, Antony Sargent wrote:
> we can use enums for string values, just not integer/float/etc.
> 
> So this should probably be:
> 
> enum Method { GET, POST };
> 
> [nodoc] dictionary DownloadOptions {
>   ...
>   Method method;
>   ...
> };
> 

Done.

http://codereview.chromium.org/9617010/diff/84005/chrome/common/extensions/ap...
chrome/common/extensions/api/downloads.idl:264: [inline_doc] DownloadLongDiff?
fileSize;
On 2012/06/12 19:08:16, Antony Sargent wrote:
> General comment:
> 
> It seems weird that we need to use use [inline_doc] so many places - should we
> switch to having that be the default, and you have to explicitly opt-out of
it?
> 
> I guess I don't really understand what inline_doc means and why some
> parameters/properties have are annotated with it and others aren't.
> 

See https://www/~benjhayden/no_crawl/downloads_docs/downloads.html

inline_doc moves documentation of properties from the Types section to wherever
the type is used (either another Type in the Types section, or somewhere in the
Methods or Events sections) so that readers don't need to bounce around the page
and mentally compose types. It also looks much less scary to see 3 Types instead
of 11.
Generally, I use inline_doc for types that are only used in one place or types
that only make sense in the context of their containers.
inline_doc might be unnecessary or less necessary if IDL supported inline
dictionary definition like C's inline structs.

http://codereview.chromium.org/9617010/diff/84005/chrome/common/extensions/ap...
chrome/common/extensions/api/downloads.idl:340: [inline_doc] optional
GetFileIconOptions options,
On 2012/06/12 19:08:16, Antony Sargent wrote:
> It seems kind of weird that the options paramter here is marked inline_doc,
but
> the GetFileIconOptions type is marked as nodoc. 
> 

A "move" operation is composed of a "copy" operation and a "delete" operation.
inline_doc doesn't "move" the doc, it just copies it. The nodoc is the "delete"
operation that completes the "move".
Without [nodoc] on GetFileIconOptions, the type would be documented both in the
getFileIcon() method and in the Types section.

http://codereview.chromium.org/9617010/diff/84005/ppapi/generators/idl_parser.py
File ppapi/generators/idl_parser.py (right):

http://codereview.chromium.org/9617010/diff/84005/ppapi/generators/idl_parser...
ppapi/generators/idl_parser.py:379: def p_ext_attr_list_values(self, p):
On 2012/06/12 19:08:16, Antony Sargent wrote:
> I don't see you reference this p_ext_attr_list_values (or any of the following
> added ones) in a production that is reachable from the top level - do you
still
> need it?
> 

p_ext_attr_list_values() extends |ext_attr_list|.
p_values() and p_symbols() are reachable from p_ext_attr_list_values().

http://codereview.chromium.org/9617010/diff/84005/ppapi/generators/idl_parser...
ppapi/generators/idl_parser.py:533: def p_expression_float(self, p):
On 2012/06/12 19:08:16, Antony Sargent wrote:
> Do you still need this? I don't see any literal float values in your
> downloads.idl file.
> 

Done.

http://codereview.chromium.org/9617010/diff/84005/tools/json_schema_compiler/...
File tools/json_schema_compiler/cc_generator.py (left):

http://codereview.chromium.org/9617010/diff/84005/tools/json_schema_compiler/...
tools/json_schema_compiler/cc_generator.py:48: .Append('using
base::BinaryValue;')
On 2012/06/12 18:34:38, kalman wrote:
> why?
> 
> This is a cc file. I would prefer to keep the using.

I ran into a very confusing name collision with an ArrayBuffer named |value|.
"Value" is too generic a term to place in the default namespace in a cc file
that contains title-cased names from input files.

http://codereview.chromium.org/9617010/diff/84005/tools/json_schema_compiler/...
File tools/json_schema_compiler/cc_generator.py (right):

http://codereview.chromium.org/9617010/diff/84005/tools/json_schema_compiler/...
tools/json_schema_compiler/cc_generator.py:492:
c.Append('%(dst)s->%(name)s.reset(new ' + any_helper.ANY_CLASS + '());')
On 2012/06/12 18:34:38, kalman wrote:
> Is this the only non *Value -> base::*Value change to this file?
> 
> Mind reverting the changes to this file then just adding this back?

See my response to your other comment in this file.

http://codereview.chromium.org/9617010/diff/84005/tools/json_schema_compiler/...
File tools/json_schema_compiler/h_generator.py (right):

http://codereview.chromium.org/9617010/diff/84005/tools/json_schema_compiler/...
tools/json_schema_compiler/h_generator.py:202:
.Append('scoped_ptr<base::DictionaryValue> ToValue() const;')
On 2012/06/12 18:34:38, kalman wrote:
> thanks :)

Done.

http://codereview.chromium.org/9617010/diff/84005/tools/json_schema_compiler/...
File tools/json_schema_compiler/model.py (right):

http://codereview.chromium.org/9617010/diff/84005/tools/json_schema_compiler/...
tools/json_schema_compiler/model.py:194: for value in json['enum']):
On 2012/06/12 18:34:38, kalman wrote:
> Could you move the second check inside this block? As it stands this will fall
> through to a generic ParseException; we should at least throw one with a nicer
> error message (if one at all).

Actually, I did this specifically so that it would fall through to the |'type'
in json| branch, which handles |legalValues=(integer*)|.

[benjhayden, 2012-06-12 20:17:57]

I ran ppapi/generators/generate.py --test, and it printed the same warnings as
it prints in a fresh checkout: $(diff fresh_warnings these_warnings) is empty.
I also fabricated an empty Filedoc in p_top_short() just in case.

[asargent_no_longer_on_chrome, 2012-06-13 05:05:59]

Hmm, on further inspection, I have some concerns about the idea of inline_doc as
well as my proposed syntax for non-string enums. Sorry for giving you such a
runaround! In order to get you unblocked on the main point of this CL, can you
go back to keeping the API description in JSON?

[benjhayden, 2012-06-13 14:01:26]

On 2012/06/13 05:05:59, Antony Sargent wrote:
> Hmm, on further inspection, I have some concerns about the idea of inline_doc
as
> well as my proposed syntax for non-string enums. Sorry for giving you such a
> runaround! In order to get you unblocked on the main point of this CL, can you
> go back to keeping the API description in JSON?

I scheduled a VC to chat.
I'd like to see if there's a way to get this in with IDL and inline_doc, but
I'll go ahead and start ripping out the IDL changes just in case.

Benjamin: Calendar said 1:30pmEDT/10:30amPDT is outside your normal hours
(7pm-4am EDT), but it also says you're in SFO so I don't know. I'm sorry if you
can't make 10:30amPDT. I'm happy to hold the json_schema_compiler changes for
you if you want, but I'd rather not schedule a VC for 7pmEDT unless necessary.

[benjhayden, 2012-06-13 18:16:48]

On 2012/06/13 14:01:26, benjhayden_chromium wrote:
> On 2012/06/13 05:05:59, Antony Sargent wrote:
> > Hmm, on further inspection, I have some concerns about the idea of
inline_doc
> as
> > well as my proposed syntax for non-string enums. Sorry for giving you such a
> > runaround! In order to get you unblocked on the main point of this CL, can
you
> > go back to keeping the API description in JSON?
> 
> I scheduled a VC to chat.
> I'd like to see if there's a way to get this in with IDL and inline_doc, but
> I'll go ahead and start ripping out the IDL changes just in case.
> 
> Benjamin: Calendar said 1:30pmEDT/10:30amPDT is outside your normal hours
> (7pm-4am EDT), but it also says you're in SFO so I don't know. I'm sorry if
you
> can't make 10:30amPDT. I'm happy to hold the json_schema_compiler changes for
> you if you want, but I'd rather not schedule a VC for 7pmEDT unless necessary.

I'll summarize our VC.
I'm going to finish splitting the IDL stuff out into a separate CL, and add
noelallen@ to that CL for the ppapi change.
Instead of annotating the *references* with [inline_doc], I'm going to annotate
the *dictionary* definitions with [inline_doc]. This will require fewer
instances of the funny-looking annotation, and obviate the need for the
funny-looking [nodoc]. We discussed making the doc generator try to infer
whether a type should be inlined based on its usage and decided that it was too
magical. We discussed using some naming convention for types that should be
inlined and decided that it was also too magical.
Moving the doc generator out of devs' way and into app engine is an awesome
idea. I almost wish I was the intern who gets to play with Python all summer.

Thanks for meeting with me!

[mkearney, 2012-06-13 20:15:45]

Hey, Ben

I just gave this a quick look and there's a TODO in a warning. This is probably
in the template, but figured I leave you to check it.

I might be able to give this more time later in the week, but for now, it's a
starting point.

http://codereview.chromium.org/9617010/diff/80010/chrome/common/extensions/do...
File chrome/common/extensions/docs/downloads.html (right):

http://codereview.chromium.org/9617010/diff/80010/chrome/common/extensions/do...
chrome/common/extensions/docs/downloads.html:269: <a href="TODO">Learn more</a>.
This shouldn't be here (broken link). Where is this coming from? I haven't seen
it in other templates.

[benjhayden, 2012-06-13 20:38:40]

http://codereview.chromium.org/9617010/diff/80010/chrome/common/extensions/do...
File chrome/common/extensions/docs/downloads.html (right):

http://codereview.chromium.org/9617010/diff/80010/chrome/common/extensions/do...
chrome/common/extensions/docs/downloads.html:269: <a href="TODO">Learn more</a>.
On 2012/06/13 20:15:46, mkearney wrote:
> This shouldn't be here (broken link). Where is this coming from? I haven't
seen
> it in other templates.

http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/common/extensions/d...

http://code.google.com/chrome/extensions/trunk/declarativeWebRequest.html

[mkearney, 2012-06-14 15:12:28]

Hi, Ben

Most of these are nits. It would be nice if DangerType and State values were in
a list format rather than paragraph.

Also, regarding the "Learn More". I just found out what this is for - early
access APIs. It's not good to have a broken link, but I think this is being
sorted outside the scope of downloads.

http://codereview.chromium.org/9617010/diff/80010/chrome/common/extensions/ap...
File chrome/common/extensions/api/downloads.json (right):

http://codereview.chromium.org/9617010/diff/80010/chrome/common/extensions/ap...
chrome/common/extensions/api/downloads.json:8: "description": "This event fires
with the DownloadItem object when a download begins.",
This should link to DownloadItem

http://codereview.chromium.org/9617010/diff/80010/chrome/common/extensions/ap...
chrome/common/extensions/api/downloads.json:19: "description": "Fires with the
download id when a download is erased from history.",
I would make this downloadId in code brackets.

http://codereview.chromium.org/9617010/diff/80010/chrome/common/extensions/ap...
chrome/common/extensions/api/downloads.json:30: "description": "When any of a
DownloadItem's properties except <code>bytesReceived</code> changes, this event
fires with the download id and an object containing the properties that
changed.",
Again, this should be a link to DownloadItem

http://codereview.chromium.org/9617010/diff/80010/chrome/common/extensions/ap...
chrome/common/extensions/api/downloads.json:30: "description": "When any of a
DownloadItem's properties except <code>bytesReceived</code> changes, this event
fires with the download id and an object containing the properties that
changed.",
Again, make this downloadId in code brackets.

http://codereview.chromium.org/9617010/diff/80010/chrome/common/extensions/ap...
chrome/common/extensions/api/downloads.json:37: "description": "Describes a
change in a DownloadItem's <code>danger</code>.",
General comment to sweep DownloadItem.

http://codereview.chromium.org/9617010/diff/80010/chrome/common/extensions/ap...
chrome/common/extensions/api/downloads.json:237: "description": "post body",
Capitalize post and add a full-stop.

http://codereview.chromium.org/9617010/diff/80010/chrome/common/extensions/ap...
chrome/common/extensions/api/downloads.json:317: "description": "true if the
user has accepted the download's danger.",
Capitalize true.

http://codereview.chromium.org/9617010/diff/80010/chrome/common/extensions/ap...
chrome/common/extensions/api/downloads.json:347: "description": "absolute local
path",
Capitalize absolute and add full-stop.

http://codereview.chromium.org/9617010/diff/80010/chrome/common/extensions/ap...
chrome/common/extensions/api/downloads.json:377: "description": "true if the
download has stopped reading data from the host, but kept the connection open.",
Capitalize true.

http://codereview.chromium.org/9617010/diff/80010/chrome/common/extensions/ap...
chrome/common/extensions/api/downloads.json:382: "description": "This
space-separated string of search terms that may be grouped using quotation marks
limits results to downloads whose filename or url contain all of the search
terms that do not begin with a dash '-' and none of the search terms that do
begin with a dash.",
url should be URL.

http://codereview.chromium.org/9617010/diff/80010/chrome/common/extensions/ap...
chrome/common/extensions/api/downloads.json:422: "description": "absolute URL",
Capitalize absolute and add full-stop.

http://codereview.chromium.org/9617010/diff/80010/chrome/common/extensions/ap...
chrome/common/extensions/api/downloads.json:427: "description": "Limits results
to downloads whose url matches the given regular expression.",
url should be URL.

http://codereview.chromium.org/9617010/diff/80010/chrome/common/extensions/ap...
chrome/common/extensions/api/downloads.json:505: "description": "Retrieve an
icon for the specified download. For new downloads, file icons are available
after the onCreated event has been received. The image returned by this function
while a download is in progress may be different from the image returned after
the download is complete. Icon retrieval is done by querying the underlying
operating system or toolkit depending on the platform. The icon that is returned
will therefore depend on a number of factors including state of the download,
platform, registered file types and visual theme. If a file icon cannot be
determined, <a
href='extension.html#property-lastError'>chrome.extension.lastError</a> will
contain an error message.",
onCreated event should link to the event.

http://codereview.chromium.org/9617010/diff/80010/chrome/common/extensions/ap...
chrome/common/extensions/api/downloads.json:570: "description":
"TODO(benjhayden) Comment.",
Do you still need to add something here?

http://codereview.chromium.org/9617010/diff/80010/chrome/common/extensions/ap...
chrome/common/extensions/api/downloads.json:667: "description": "post body",
Capitalize post and add full-stop.

http://codereview.chromium.org/9617010/diff/80010/chrome/common/extensions/ap...
chrome/common/extensions/api/downloads.json:716: "description": "'file': The
download's filename is suspicious. 'url': The download's URL is known to be
malicious. 'content': The downloaded file is known to be malicious. 'uncommon':
The download's URL is not commonly downloaded and could be dangerous. 'safe':
The download presents no known danger to the user's computer. These string
constants will never change, however the set of DangerTypes may change.",
Can we format this in a list, rather than a paragraph?

http://codereview.chromium.org/9617010/diff/80010/chrome/common/extensions/ap...
chrome/common/extensions/api/downloads.json:728: "description": "'in_progress':
the download is currently receiving data from the server. 'interrupted': an
error broke the connection with the file host. 'complete': the download
completed successfully. These string constants will never change, however the
set of States may change.",
Can we format this in a list, rather than a paragraph?

http://codereview.chromium.org/9617010/diff/80010/chrome/common/extensions/ap...
chrome/common/extensions/api/downloads.json:751: "description": "true if the
user has accepted the download's danger.",
Capitalize true.

http://codereview.chromium.org/9617010/diff/80010/chrome/common/extensions/ap...
chrome/common/extensions/api/downloads.json:794: "description": "true if the
download has stopped reading data from the host, but kept the connection open.",
Capitalize true.

http://codereview.chromium.org/9617010/diff/80010/chrome/common/extensions/ap...
chrome/common/extensions/api/downloads.json:814: "description": "absolute URL",
Capitalize absolute and add full-stop.

http://codereview.chromium.org/9617010/diff/80010/chrome/common/extensions/ap...
chrome/common/extensions/api/downloads.json:836: "description": "true if the
user has accepted the download's danger.",
Capitalize true.

http://codereview.chromium.org/9617010/diff/80010/chrome/common/extensions/ap...
chrome/common/extensions/api/downloads.json:866: "description": "absolute local
path",
Capitalize absolute and add full-stop.

http://codereview.chromium.org/9617010/diff/80010/chrome/common/extensions/ap...
chrome/common/extensions/api/downloads.json:896: "description": "true if the
download has stopped reading data from the host, but kept the connection open.",
Capitalize true.

http://codereview.chromium.org/9617010/diff/80010/chrome/common/extensions/ap...
chrome/common/extensions/api/downloads.json:941: "description": "absolute URL",
Capitalize absolute.

http://codereview.chromium.org/9617010/diff/80010/chrome/common/extensions/ap...
chrome/common/extensions/api/downloads.json:946: "description": "Limits results
to downloads whose url matches the given regular expression.",
url should be URL.

[benjhayden, 2012-06-14 16:44:13]

PTAL

http://codereview.chromium.org/9617010/diff/80010/chrome/common/extensions/ap...
File chrome/common/extensions/api/downloads.json (right):

http://codereview.chromium.org/9617010/diff/80010/chrome/common/extensions/ap...
chrome/common/extensions/api/downloads.json:8: "description": "This event fires
with the DownloadItem object when a download begins.",
On 2012/06/14 15:12:28, mkearney wrote:
> This should link to DownloadItem

Done.

http://codereview.chromium.org/9617010/diff/80010/chrome/common/extensions/ap...
chrome/common/extensions/api/downloads.json:19: "description": "Fires with the
download id when a download is erased from history.",
On 2012/06/14 15:12:28, mkearney wrote:
> I would make this downloadId in code brackets.

Done.

http://codereview.chromium.org/9617010/diff/80010/chrome/common/extensions/ap...
chrome/common/extensions/api/downloads.json:30: "description": "When any of a
DownloadItem's properties except <code>bytesReceived</code> changes, this event
fires with the download id and an object containing the properties that
changed.",
On 2012/06/14 15:12:28, mkearney wrote:
> Again, this should be a link to DownloadItem

Done.

http://codereview.chromium.org/9617010/diff/80010/chrome/common/extensions/ap...
chrome/common/extensions/api/downloads.json:30: "description": "When any of a
DownloadItem's properties except <code>bytesReceived</code> changes, this event
fires with the download id and an object containing the properties that
changed.",
On 2012/06/14 15:12:28, mkearney wrote:
> Again, make this downloadId in code brackets.

Done.

http://codereview.chromium.org/9617010/diff/80010/chrome/common/extensions/ap...
chrome/common/extensions/api/downloads.json:37: "description": "Describes a
change in a DownloadItem's <code>danger</code>.",
On 2012/06/14 15:12:28, mkearney wrote:
> General comment to sweep DownloadItem.

Done.

http://codereview.chromium.org/9617010/diff/80010/chrome/common/extensions/ap...
chrome/common/extensions/api/downloads.json:237: "description": "post body",
On 2012/06/14 15:12:28, mkearney wrote:
> Capitalize post and add a full-stop.

Done.

http://codereview.chromium.org/9617010/diff/80010/chrome/common/extensions/ap...
chrome/common/extensions/api/downloads.json:317: "description": "true if the
user has accepted the download's danger.",
On 2012/06/14 15:12:28, mkearney wrote:
> Capitalize true.

Done.

http://codereview.chromium.org/9617010/diff/80010/chrome/common/extensions/ap...
chrome/common/extensions/api/downloads.json:347: "description": "absolute local
path",
On 2012/06/14 15:12:28, mkearney wrote:
> Capitalize absolute and add full-stop.

Done.

http://codereview.chromium.org/9617010/diff/80010/chrome/common/extensions/ap...
chrome/common/extensions/api/downloads.json:377: "description": "true if the
download has stopped reading data from the host, but kept the connection open.",
On 2012/06/14 15:12:28, mkearney wrote:
> Capitalize true.

Done.

http://codereview.chromium.org/9617010/diff/80010/chrome/common/extensions/ap...
chrome/common/extensions/api/downloads.json:382: "description": "This
space-separated string of search terms that may be grouped using quotation marks
limits results to downloads whose filename or url contain all of the search
terms that do not begin with a dash '-' and none of the search terms that do
begin with a dash.",
On 2012/06/14 15:12:28, mkearney wrote:
> url should be URL.

I added code tags and a link to DownloadItem instead. How does that look?

http://codereview.chromium.org/9617010/diff/80010/chrome/common/extensions/ap...
chrome/common/extensions/api/downloads.json:422: "description": "absolute URL",
On 2012/06/14 15:12:28, mkearney wrote:
> Capitalize absolute and add full-stop.

Done.

http://codereview.chromium.org/9617010/diff/80010/chrome/common/extensions/ap...
chrome/common/extensions/api/downloads.json:427: "description": "Limits results
to downloads whose url matches the given regular expression.",
On 2012/06/14 15:12:28, mkearney wrote:
> url should be URL.

Done.

http://codereview.chromium.org/9617010/diff/80010/chrome/common/extensions/ap...
chrome/common/extensions/api/downloads.json:505: "description": "Retrieve an
icon for the specified download. For new downloads, file icons are available
after the onCreated event has been received. The image returned by this function
while a download is in progress may be different from the image returned after
the download is complete. Icon retrieval is done by querying the underlying
operating system or toolkit depending on the platform. The icon that is returned
will therefore depend on a number of factors including state of the download,
platform, registered file types and visual theme. If a file icon cannot be
determined, <a
href='extension.html#property-lastError'>chrome.extension.lastError</a> will
contain an error message.",
On 2012/06/14 15:12:28, mkearney wrote:
> onCreated event should link to the event.

Done.

http://codereview.chromium.org/9617010/diff/80010/chrome/common/extensions/ap...
chrome/common/extensions/api/downloads.json:570: "description":
"TODO(benjhayden) Comment.",
On 2012/06/14 15:12:28, mkearney wrote:
> Do you still need to add something here?

This function is nodoc and not yet implemented. I'll write the comment when I
implement it.

http://codereview.chromium.org/9617010/diff/80010/chrome/common/extensions/ap...
chrome/common/extensions/api/downloads.json:667: "description": "post body",
On 2012/06/14 15:12:28, mkearney wrote:
> Capitalize post and add full-stop.

Done.

http://codereview.chromium.org/9617010/diff/80010/chrome/common/extensions/ap...
chrome/common/extensions/api/downloads.json:716: "description": "'file': The
download's filename is suspicious. 'url': The download's URL is known to be
malicious. 'content': The downloaded file is known to be malicious. 'uncommon':
The download's URL is not commonly downloaded and could be dangerous. 'safe':
The download presents no known danger to the user's computer. These string
constants will never change, however the set of DangerTypes may change.",
On 2012/06/14 15:12:28, mkearney wrote:
> Can we format this in a list, rather than a paragraph?

Done.

http://codereview.chromium.org/9617010/diff/80010/chrome/common/extensions/ap...
chrome/common/extensions/api/downloads.json:728: "description": "'in_progress':
the download is currently receiving data from the server. 'interrupted': an
error broke the connection with the file host. 'complete': the download
completed successfully. These string constants will never change, however the
set of States may change.",
On 2012/06/14 15:12:28, mkearney wrote:
> Can we format this in a list, rather than a paragraph?

Done.

http://codereview.chromium.org/9617010/diff/80010/chrome/common/extensions/ap...
chrome/common/extensions/api/downloads.json:751: "description": "true if the
user has accepted the download's danger.",
On 2012/06/14 15:12:28, mkearney wrote:
> Capitalize true.

Done.

http://codereview.chromium.org/9617010/diff/80010/chrome/common/extensions/ap...
chrome/common/extensions/api/downloads.json:794: "description": "true if the
download has stopped reading data from the host, but kept the connection open.",
On 2012/06/14 15:12:28, mkearney wrote:
> Capitalize true.

Done.

http://codereview.chromium.org/9617010/diff/80010/chrome/common/extensions/ap...
chrome/common/extensions/api/downloads.json:814: "description": "absolute URL",
On 2012/06/14 15:12:28, mkearney wrote:
> Capitalize absolute and add full-stop.

Done.

http://codereview.chromium.org/9617010/diff/80010/chrome/common/extensions/ap...
chrome/common/extensions/api/downloads.json:836: "description": "true if the
user has accepted the download's danger.",
On 2012/06/14 15:12:28, mkearney wrote:
> Capitalize true.

Done.

http://codereview.chromium.org/9617010/diff/80010/chrome/common/extensions/ap...
chrome/common/extensions/api/downloads.json:866: "description": "absolute local
path",
On 2012/06/14 15:12:28, mkearney wrote:
> Capitalize absolute and add full-stop.

Done.

http://codereview.chromium.org/9617010/diff/80010/chrome/common/extensions/ap...
chrome/common/extensions/api/downloads.json:896: "description": "true if the
download has stopped reading data from the host, but kept the connection open.",
On 2012/06/14 15:12:28, mkearney wrote:
> Capitalize true.

Done.

http://codereview.chromium.org/9617010/diff/80010/chrome/common/extensions/ap...
chrome/common/extensions/api/downloads.json:941: "description": "absolute URL",
On 2012/06/14 15:12:28, mkearney wrote:
> Capitalize absolute.

Done.

http://codereview.chromium.org/9617010/diff/80010/chrome/common/extensions/ap...
chrome/common/extensions/api/downloads.json:946: "description": "Limits results
to downloads whose url matches the given regular expression.",
On 2012/06/14 15:12:28, mkearney wrote:
> url should be URL.

Done.

[benjhayden, 2012-06-20 14:40:03]

PTAL

[mkearney, 2012-06-20 14:59:48]

Hey, just one nit, but it's in a lot of places.

The link to DownloadItems has the 's' outside of the link.

http://codereview.chromium.org/9617010/diff/74080/chrome/common/extensions/ap...
File chrome/common/extensions/api/downloads.json (right):

http://codereview.chromium.org/9617010/diff/74080/chrome/common/extensions/ap...
chrome/common/extensions/api/downloads.json:301: "description": "Find <a
href='#type-DownloadItem'>DownloadItem</a>s. Set <code>query</code> to the empty
object to get all <a href='#type-DownloadItem'>DownloadItem</a>s. To get a
specific <a href='#type-DownloadItem'>DownloadItem</a>, set only the
<code>id</code> field.",
Move 's' inside cross-reference for both DownloadItems here.

I'm going to add a general comment, as this is the same across most
DownloadItems links.

[benjhayden, 2012-06-20 15:42:22]

Thanks!

I'll hit the CQ as soon as the win_rel trybot comes back.

http://codereview.chromium.org/9617010/diff/74080/chrome/common/extensions/ap...
File chrome/common/extensions/api/downloads.json (right):

http://codereview.chromium.org/9617010/diff/74080/chrome/common/extensions/ap...
chrome/common/extensions/api/downloads.json:301: "description": "Find <a
href='#type-DownloadItem'>DownloadItem</a>s. Set <code>query</code> to the empty
object to get all <a href='#type-DownloadItem'>DownloadItem</a>s. To get a
specific <a href='#type-DownloadItem'>DownloadItem</a>, set only the
<code>id</code> field.",
On 2012/06/20 14:59:48, mkearney wrote:
> Move 's' inside cross-reference for both DownloadItems here.
> 
> I'm going to add a general comment, as this is the same across most
> DownloadItems links.

Done.

[commit-bot: I haz the power, 2012-06-21 14:49:03]

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

[commit-bot: I haz the power, 2012-06-21 14:49:19]

Presubmit check for 9617010-100002 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Messages **
If this change requires manual test instructions to QA team, add
TEST=[instructions].

** Presubmit Warnings **
This change modifies the extension docs but the generated docs have not been
updated properly. See chrome/common/extensions/docs/README.txt for more info.
 - Changes to chrome/common/extensions/docs/static/api_index.html not reflected
in generated doc.
 - Changes to chrome/common/extensions/docs/static/downloads.html not reflected
in generated doc.
 - Changes to sample
chrome/common/extensions/docs/examples/api/downloads/download_links/popup.js
have not been re-zipped.
First build DumpRenderTree, then update the docs by running:
  chrome/common/extensions/docs/build/build.py

Presubmit checks took 4.6s to calculate.

[benjhayden, 2012-06-21 19:44:49]

dcommitted because chrome/common/extensions/PRESUBMIT.py is not smart enough for
this change.