Switch the downloads API over to IDL/json_schema_compiler

Switch the downloads API over to IDL/json_schema_compiler

Modify ppapi/generators/idl_parser.py to
0: not require a file-level comment (but generate the same parse tree structure),
1: actually support ext_attrs (modifiers) for dictionaries, and
2: support [ext_attr=(symbols|values)].

Modify json_schema_compiler to
0: use "base::Value" and any_helper.ANY_CLASS instead of Value and Any in order to support ArrayBuffers named |value| or |any|,
1: actually test that namespaces and dictionaries are sorted correctly,
2: fix HGenerator._FieldDependencyOrder(),
3: support [inline_doc] on dictionaries and enums,
4: support descriptions on enums,
5: support documentation_permissions_required,
6: support [legalValues=(values...)].

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

[benjhayden, 2012-06-28 14:42:23]

PTAL

[noelallen1, 2012-07-02 17:56:00]

Changes to the idl_parser.py and idl_basics.py LGTM.

[asargent_no_longer_on_chrome, 2012-07-02 23:43:48]

LGTM with a couple of minor issues to fix

http://codereview.chromium.org/10639020/diff/21002/chrome/browser/extensions/...
File chrome/browser/extensions/api/downloads/downloads_api.cc (right):

http://codereview.chromium.org/10639020/diff/21002/chrome/browser/extensions/...
chrome/browser/extensions/api/downloads/downloads_api.cc:364: static
base::LazyInstance<SortTypeMap> sorter_types =
nit: Why have this as a lazy instance static data structure instead of just a
function with a switch or chain of if/else statements that returns one of the
DownloadQuery::SORT_* values for a given string? It's not like it's
computationally expensive to select from among the small handful of values with
string matching, so it doesn't seem worth it to have the overhead of the
LazyInstance access locking as well as the persistent memory space (note that
LazyInstance pre-allocates space for the pointed-to value even if you don't
initialize it).

http://codereview.chromium.org/10639020/diff/21002/chrome/browser/extensions/...
chrome/browser/extensions/api/downloads/downloads_api.cc:397: static
base::LazyInstance<FilterTypeMap> filter_types =
same nit here - no need for a static lazy instance

http://codereview.chromium.org/10639020/diff/21002/chrome/common/extensions/a...
File chrome/common/extensions/api/downloads.idl (right):

http://codereview.chromium.org/10639020/diff/21002/chrome/common/extensions/a...
chrome/common/extensions/api/downloads.idl:160: // <code>orderBy</code> with a
hyphen: '-startTime'.
From the implemenation, it looks like you allow specifying secondary sort orders
by passing more field names in this string with a space as separator. Did you
intentionally omit that from the documentation here? 

(Also, the idiomatic thing to do with javascript would be to make this parameter
either a single string or array of strings, instead of using an inline separator
character. The way to do this with IDL would be to use a "union type":
http://dev.w3.org/2006/webapi/WebIDL/#idl-union but I don't think we support
that yet. I'm sure we will want to add this eventually, but if happened to want
to take this on in a follow-up CL or something that would be awesome).

http://codereview.chromium.org/10639020/diff/21002/chrome/common/extensions/a...
chrome/common/extensions/api/downloads.idl:265: // Describes a change in a
DownloadItem's <code>fileSize</code>.
Optional suggestion: Repeating the word "DownloadItem" in each of these
descriptions seems verbose. Can you simplify these to just:

// The change in url, if any.
...
// The change in filename, if any.
...

etc.

http://codereview.chromium.org/10639020/diff/21002/ppapi/generators/idl_parse...
File ppapi/generators/idl_parser.py (right):

http://codereview.chromium.org/10639020/diff/21002/ppapi/generators/idl_parse...
ppapi/generators/idl_parser.py:301: # A dictionary contains is a named list of
optional and required members.
can you fix the typo in this comment while you're in here? ("contains is a" ->
"is a")

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

http://codereview.chromium.org/10639020/diff/21002/tools/json_schema_compiler...
tools/json_schema_compiler/model.py:193: elif 'enum' in json and
all(isinstance(value, basestring)
This isinstance test seems kind of hacky - can we instead do the following?

elif 'enum' in json and 'type' in json and json['type'] == 'string':

[not at google - send to devlin, 2012-07-04 13:28:47]

json schema compiler lgtm

[benjhayden, 2012-07-09 20:18:09]

Antony, PTAL.

http://codereview.chromium.org/10639020/diff/21002/chrome/browser/extensions/...
File chrome/browser/extensions/api/downloads/downloads_api.cc (right):

http://codereview.chromium.org/10639020/diff/21002/chrome/browser/extensions/...
chrome/browser/extensions/api/downloads/downloads_api.cc:364: static
base::LazyInstance<SortTypeMap> sorter_types =
On 2012/07/02 23:43:48, Antony Sargent wrote:
> nit: Why have this as a lazy instance static data structure instead of just a
> function with a switch or chain of if/else statements that returns one of the
> DownloadQuery::SORT_* values for a given string? It's not like it's
> computationally expensive to select from among the small handful of values
with
> string matching, so it doesn't seem worth it to have the overhead of the
> LazyInstance access locking as well as the persistent memory space (note that
> LazyInstance pre-allocates space for the pointed-to value even if you don't
> initialize it). 
> 

I'd love to use a switch statement, but C++ doesn't support switching on
strings.
String comparison is not free. I haven't measured this code against the string
comparisons, but my starter project (an mp4 parsing/manipulation library) was
sped up by a huge factor when I optimized away a few dozen string comparisons,
even though it was still performing several other loops.
This is all running on the UI thread. How expensive is the lazy instance access
locking?
Looks like sizeof(sorter_types) in debug mode on my linux box is 48 bytes. I
don't have much experience with problems stemming from statically allocated
memory, but if 48 bytes is a problem, then our VLOG usage should be a P0
blocker.

Also, this is not actually new code, just moved from DownloadsSearchFunction. I
probably should have made that clearer.

http://codereview.chromium.org/10639020/diff/21002/chrome/browser/extensions/...
chrome/browser/extensions/api/downloads/downloads_api.cc:397: static
base::LazyInstance<FilterTypeMap> filter_types =
On 2012/07/02 23:43:48, Antony Sargent wrote:
> same nit here - no need for a static lazy instance

Same response.

http://codereview.chromium.org/10639020/diff/21002/chrome/common/extensions/a...
File chrome/common/extensions/api/downloads.idl (right):

http://codereview.chromium.org/10639020/diff/21002/chrome/common/extensions/a...
chrome/common/extensions/api/downloads.idl:160: // <code>orderBy</code> with a
hyphen: '-startTime'.
On 2012/07/02 23:43:48, Antony Sargent wrote:
> From the implemenation, it looks like you allow specifying secondary sort
orders
> by passing more field names in this string with a space as separator. Did you
> intentionally omit that from the documentation here? 
> 
> (Also, the idiomatic thing to do with javascript would be to make this
parameter
> either a single string or array of strings, instead of using an inline
separator
> character. The way to do this with IDL would be to use a "union type":
> http://dev.w3.org/2006/webapi/WebIDL/#idl-union but I don't think we support
> that yet. I'm sure we will want to add this eventually, but if happened to
want
> to take this on in a follow-up CL or something that would be awesome). 
> 

Filed a bug for myself: http://crbug.com/135636

http://codereview.chromium.org/10639020/diff/21002/chrome/common/extensions/a...
chrome/common/extensions/api/downloads.idl:265: // Describes a change in a
DownloadItem's <code>fileSize</code>.
On 2012/07/02 23:43:48, Antony Sargent wrote:
> Optional suggestion: Repeating the word "DownloadItem" in each of these
> descriptions seems verbose. Can you simplify these to just:
> 
> // The change in url, if any.
> ...
> // The change in filename, if any.
> ...
> 
> etc.
> 

Done.

http://codereview.chromium.org/10639020/diff/21002/ppapi/generators/idl_parse...
File ppapi/generators/idl_parser.py (right):

http://codereview.chromium.org/10639020/diff/21002/ppapi/generators/idl_parse...
ppapi/generators/idl_parser.py:301: # A dictionary contains is a named list of
optional and required members.
On 2012/07/02 23:43:48, Antony Sargent wrote:
> can you fix the typo in this comment while you're in here? ("contains is a" ->
> "is a")

Done.

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

http://codereview.chromium.org/10639020/diff/21002/tools/json_schema_compiler...
tools/json_schema_compiler/model.py:193: elif 'enum' in json and
all(isinstance(value, basestring)
On 2012/07/02 23:43:48, Antony Sargent wrote:
> This isinstance test seems kind of hacky - can we instead do the following?
> 
> elif 'enum' in json and 'type' in json and json['type'] == 'string':
> 

Done.

[asargent_no_longer_on_chrome, 2012-07-09 22:09:43]

http://codereview.chromium.org/10639020/diff/21002/chrome/browser/extensions/...
File chrome/browser/extensions/api/downloads/downloads_api.cc (right):

http://codereview.chromium.org/10639020/diff/21002/chrome/browser/extensions/...
chrome/browser/extensions/api/downloads/downloads_api.cc:364: static
base::LazyInstance<SortTypeMap> sorter_types =
On 2012/07/09 20:18:09, benjhayden_chromium wrote:
> On 2012/07/02 23:43:48, Antony Sargent wrote:
> > nit: Why have this as a lazy instance static data structure instead of just
a
> > function with a switch or chain of if/else statements that returns one of
the
> > DownloadQuery::SORT_* values for a given string? It's not like it's
> > computationally expensive to select from among the small handful of values
> with
> > string matching, so it doesn't seem worth it to have the overhead of the
> > LazyInstance access locking as well as the persistent memory space (note
that
> > LazyInstance pre-allocates space for the pointed-to value even if you don't
> > initialize it). 
> > 
> 
> I'd love to use a switch statement, but C++ doesn't support switching on
> strings.
> String comparison is not free. I haven't measured this code against the string
> comparisons, but my starter project (an mp4 parsing/manipulation library) was
> sped up by a huge factor when I optimized away a few dozen string comparisons,
> even though it was still performing several other loops.
> This is all running on the UI thread. How expensive is the lazy instance
access
> locking?
> Looks like sizeof(sorter_types) in debug mode on my linux box is 48 bytes. I
> don't have much experience with problems stemming from statically allocated
> memory, but if 48 bytes is a problem, then our VLOG usage should be a P0
> blocker.
> 
> Also, this is not actually new code, just moved from DownloadsSearchFunction.
I
> probably should have made that clearer.

It's probably not a huge deal either way. But that extra 48 bytes gets paid by
every chrome user regardless of whether they have any extensions installed, let
alone any that use the downloads API and will use this particular query
function. I guess we probably won't pay any locking cost except at LazyInstance
access time, so that wouldn't affect us when not going through this code path. 

In contrast the runtime cost of string comparisons only affects the actual calls
to the function, which will happen a vanishingly small number of times, and I'm
sure is plenty fast.

[benjhayden, 2012-07-10 18:32:04]

I'll commit tomorrow morning if there are no further comments.

http://codereview.chromium.org/10639020/diff/21002/chrome/browser/extensions/...
File chrome/browser/extensions/api/downloads/downloads_api.cc (right):

http://codereview.chromium.org/10639020/diff/21002/chrome/browser/extensions/...
chrome/browser/extensions/api/downloads/downloads_api.cc:364: static
base::LazyInstance<SortTypeMap> sorter_types =
On 2012/07/09 22:09:43, Antony Sargent wrote:
> On 2012/07/09 20:18:09, benjhayden_chromium wrote:
> > On 2012/07/02 23:43:48, Antony Sargent wrote:
> > > nit: Why have this as a lazy instance static data structure instead of
just
> a
> > > function with a switch or chain of if/else statements that returns one of
> the
> > > DownloadQuery::SORT_* values for a given string? It's not like it's
> > > computationally expensive to select from among the small handful of values
> > with
> > > string matching, so it doesn't seem worth it to have the overhead of the
> > > LazyInstance access locking as well as the persistent memory space (note
> that
> > > LazyInstance pre-allocates space for the pointed-to value even if you
don't
> > > initialize it). 
> > > 
> > 
> > I'd love to use a switch statement, but C++ doesn't support switching on
> > strings.
> > String comparison is not free. I haven't measured this code against the
string
> > comparisons, but my starter project (an mp4 parsing/manipulation library)
was
> > sped up by a huge factor when I optimized away a few dozen string
comparisons,
> > even though it was still performing several other loops.
> > This is all running on the UI thread. How expensive is the lazy instance
> access
> > locking?
> > Looks like sizeof(sorter_types) in debug mode on my linux box is 48 bytes. I
> > don't have much experience with problems stemming from statically allocated
> > memory, but if 48 bytes is a problem, then our VLOG usage should be a P0
> > blocker.
> > 
> > Also, this is not actually new code, just moved from
DownloadsSearchFunction.
> I
> > probably should have made that clearer.
> 
> It's probably not a huge deal either way. But that extra 48 bytes gets paid by
> every chrome user regardless of whether they have any extensions installed,
let
> alone any that use the downloads API and will use this particular query
> function. I guess we probably won't pay any locking cost except at
LazyInstance
> access time, so that wouldn't affect us when not going through this code path.

> 
> In contrast the runtime cost of string comparisons only affects the actual
calls
> to the function, which will happen a vanishingly small number of times, and
I'm
> sure is plenty fast. 
> 

Added a TODO to switch to string comparisons.
I'll probably get around to it Soon, but not this CL.

[commit-bot: I haz the power, 2012-07-11 19:36:06]

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

[commit-bot: I haz the power, 2012-07-11 21:03:28]

Change committed as 146201