JSON Schema Compiler supports functions as PropertyTypes.

JSON Schema Compiler supports functions as PropertyTypes.

Before, the compiler raised an exception when a function was used as a property,
which happens whenever a function is passed as a parameter.

The solution presented here is to create a bool has_<function_name> to allow
the hand-written C++ code to know whether or not the function was passed in.

BUG=138850
TEST=function_as_parameter


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

[chebert, 2012-07-25 00:12:48]

This is another change needed by the context_menus api.

[not at google - send to devlin, 2012-07-25 01:32:33]

nice one

https://chromiumcodereview.appspot.com/10824002/diff/2001/tools/json_schema_c...
File tools/json_schema_compiler/cc_generator.py (right):

https://chromiumcodereview.appspot.com/10824002/diff/2001/tools/json_schema_c...
tools/json_schema_compiler/cc_generator.py:219: if prop.type_ ==
PropertyType.FUNCTION:
Comment why you're only generating for optional.

https://chromiumcodereview.appspot.com/10824002/diff/2001/tools/json_schema_c...
tools/json_schema_compiler/cc_generator.py:221: # getting a value
nit: fullstop

https://chromiumcodereview.appspot.com/10824002/diff/2001/tools/json_schema_c...
tools/json_schema_compiler/cc_generator.py:223:
'%s->GetWithoutPathExpansion("%s", &%s);' %
just use HasKey, don't need to Get it too.

https://chromiumcodereview.appspot.com/10824002/diff/2001/tools/json_schema_c...
tools/json_schema_compiler/cc_generator.py:262: if prop.optional and not
prop.type_ == PropertyType.FUNCTION:
nit: assign this condition to a local variable and read it here and below.

https://chromiumcodereview.appspot.com/10824002/diff/2001/tools/json_schema_c...
tools/json_schema_compiler/cc_generator.py:330: elif prop.type_ ==
PropertyType.FUNCTION:
Hm, well, for consistency with the way that functions come in from the renderer,
this should return an empty object (i.e. empty DictionaryValue).

I just spent a while looking through how onclick even arrives at the browser in
the first place, and it looks like a bit of a fluke; in V8ValueConverterImpl it
just happens that functions get serialised to an empty object in json; and
that's how they're going to come into json-schema-compiler.

I'm tempted to just ignore going back from has_foo to a value, but you never
know; that might surprise somebody at some point. So let's just make it
seriialize to an empty object.

(Add comment to that effect btw)

https://chromiumcodereview.appspot.com/10824002/diff/2001/tools/json_schema_c...
tools/json_schema_compiler/cc_generator.py:484: c.Append('%s->has_%s = true;' %
(dst, prop.unix_name))
comment plz, the stuff about the renderer I said in the comment above.

https://chromiumcodereview.appspot.com/10824002/diff/2001/tools/json_schema_c...
File tools/json_schema_compiler/cpp_type_generator.py (right):

https://chromiumcodereview.appspot.com/10824002/diff/2001/tools/json_schema_c...
tools/json_schema_compiler/cpp_type_generator.py:176: # NONE value generated
instead.
Add comment like "We only generate "has" methods for FUNCTIONS, so don't need to
wrap them."

https://chromiumcodereview.appspot.com/10824002/diff/2001/tools/json_schema_c...
tools/json_schema_compiler/cpp_type_generator.py:178: PropertyType.FUNCTION):
nit: each condition on its own line, like

if (wrap_optional and
    prop.optional and
    prop.type_ not in [PropertyType.ENUM, PropertyType.FUNCTION]):

https://chromiumcodereview.appspot.com/10824002/diff/2001/tools/json_schema_c...
File tools/json_schema_compiler/cpp_util.py (right):

https://chromiumcodereview.appspot.com/10824002/diff/2001/tools/json_schema_c...
tools/json_schema_compiler/cpp_util.py:58: PropertyType.FUNCTION:
'Value::TYPE_BOOLEAN',
Should this be DICTIONARY if it serializes to an empty dict?

https://chromiumcodereview.appspot.com/10824002/diff/2001/tools/json_schema_c...
File tools/json_schema_compiler/h_generator.py (right):

https://chromiumcodereview.appspot.com/10824002/diff/2001/tools/json_schema_c...
tools/json_schema_compiler/h_generator.py:161: 'as a parameter.')
include the function name here, and include the overall comment either way if
there is one?

https://chromiumcodereview.appspot.com/10824002/diff/2001/tools/json_schema_c...
tools/json_schema_compiler/h_generator.py:165: c.Append('bool has_%s;' %
prop.unix_name)
i think you only want this if it's optional too.

https://chromiumcodereview.appspot.com/10824002/diff/2001/tools/json_schema_c...
File tools/json_schema_compiler/model.py (right):

https://chromiumcodereview.appspot.com/10824002/diff/2001/tools/json_schema_c...
tools/json_schema_compiler/model.py:365: #continue
delete this and the whole comment above, it's no longer relevant.

[chebert, 2012-07-25 18:38:30]

The empty dictionary value was a much better idea. The code comes out way
cleaner.

https://chromiumcodereview.appspot.com/10824002/diff/2001/tools/json_schema_c...
File tools/json_schema_compiler/cc_generator.py (right):

https://chromiumcodereview.appspot.com/10824002/diff/2001/tools/json_schema_c...
tools/json_schema_compiler/cc_generator.py:219: if prop.type_ ==
PropertyType.FUNCTION:
the empty dict solved a lot of problems, so a lot of these comments will no
longer be relevant. 

On 2012/07/25 01:32:33, kalman wrote:
> Comment why you're only generating for optional.

https://chromiumcodereview.appspot.com/10824002/diff/2001/tools/json_schema_c...
tools/json_schema_compiler/cc_generator.py:221: # getting a value
On 2012/07/25 01:32:33, kalman wrote:
> nit: fullstop

Done.

https://chromiumcodereview.appspot.com/10824002/diff/2001/tools/json_schema_c...
tools/json_schema_compiler/cc_generator.py:262: if prop.optional and not
prop.type_ == PropertyType.FUNCTION:
On 2012/07/25 01:32:33, kalman wrote:
> nit: assign this condition to a local variable and read it here and below.

Done.

https://chromiumcodereview.appspot.com/10824002/diff/2001/tools/json_schema_c...
tools/json_schema_compiler/cc_generator.py:330: elif prop.type_ ==
PropertyType.FUNCTION:
On 2012/07/25 01:32:33, kalman wrote:
> Hm, well, for consistency with the way that functions come in from the
renderer,
> this should return an empty object (i.e. empty DictionaryValue).
> 
> I just spent a while looking through how onclick even arrives at the browser
in
> the first place, and it looks like a bit of a fluke; in V8ValueConverterImpl
it
> just happens that functions get serialised to an empty object in json; and
> that's how they're going to come into json-schema-compiler.
> 
> I'm tempted to just ignore going back from has_foo to a value, but you never
> know; that might surprise somebody at some point. So let's just make it
> seriialize to an empty object.
> 
> (Add comment to that effect btw)

Done.

https://chromiumcodereview.appspot.com/10824002/diff/2001/tools/json_schema_c...
tools/json_schema_compiler/cc_generator.py:484: c.Append('%s->has_%s = true;' %
(dst, prop.unix_name))
On 2012/07/25 01:32:33, kalman wrote:
> comment plz, the stuff about the renderer I said in the comment above.

Done.

https://chromiumcodereview.appspot.com/10824002/diff/2001/tools/json_schema_c...
File tools/json_schema_compiler/cpp_type_generator.py (right):

https://chromiumcodereview.appspot.com/10824002/diff/2001/tools/json_schema_c...
tools/json_schema_compiler/cpp_type_generator.py:176: # NONE value generated
instead.
On 2012/07/25 01:32:33, kalman wrote:
> Add comment like "We only generate "has" methods for FUNCTIONS, so don't need
to
> wrap them."

Done.

https://chromiumcodereview.appspot.com/10824002/diff/2001/tools/json_schema_c...
tools/json_schema_compiler/cpp_type_generator.py:178: PropertyType.FUNCTION):
On 2012/07/25 01:32:33, kalman wrote:
> nit: each condition on its own line, like
> 
> if (wrap_optional and
>     prop.optional and
>     prop.type_ not in [PropertyType.ENUM, PropertyType.FUNCTION]):

Done.

https://chromiumcodereview.appspot.com/10824002/diff/2001/tools/json_schema_c...
File tools/json_schema_compiler/cpp_util.py (right):

https://chromiumcodereview.appspot.com/10824002/diff/2001/tools/json_schema_c...
tools/json_schema_compiler/cpp_util.py:58: PropertyType.FUNCTION:
'Value::TYPE_BOOLEAN',
On 2012/07/25 01:32:33, kalman wrote:
> Should this be DICTIONARY if it serializes to an empty dict?

Done.

https://chromiumcodereview.appspot.com/10824002/diff/2001/tools/json_schema_c...
File tools/json_schema_compiler/h_generator.py (right):

https://chromiumcodereview.appspot.com/10824002/diff/2001/tools/json_schema_c...
tools/json_schema_compiler/h_generator.py:161: 'as a parameter.')
On 2012/07/25 01:32:33, kalman wrote:
> include the function name here, and include the overall comment either way if
> there is one?

Done.

https://chromiumcodereview.appspot.com/10824002/diff/2001/tools/json_schema_c...
tools/json_schema_compiler/h_generator.py:165: c.Append('bool has_%s;' %
prop.unix_name)
On 2012/07/25 01:32:33, kalman wrote:
> i think you only want this if it's optional too.

Done.

https://chromiumcodereview.appspot.com/10824002/diff/2001/tools/json_schema_c...
File tools/json_schema_compiler/model.py (right):

https://chromiumcodereview.appspot.com/10824002/diff/2001/tools/json_schema_c...
tools/json_schema_compiler/model.py:365: #continue
On 2012/07/25 01:32:33, kalman wrote:
> delete this and the whole comment above, it's no longer relevant.

Done.

[not at google - send to devlin, 2012-07-26 00:01:02]

lgtm, nice.

https://chromiumcodereview.appspot.com/10824002/diff/12001/tools/json_schema_...
File tools/json_schema_compiler/h_generator.py (right):

https://chromiumcodereview.appspot.com/10824002/diff/12001/tools/json_schema_...
tools/json_schema_compiler/h_generator.py:164: )
need the parens?

btw the line below can be moved inside these.

(c.Append(...)
  .Append())
)
return c

[commit-bot: I haz the power, 2012-07-26 00:07:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hebert.christopherj@chromium.org/10824...

[commit-bot: I haz the power, 2012-07-26 00:08:01]

Presubmit check for 10824002-10002 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome

Presubmit checks took 4.8s to calculate.

[Nico, 2012-07-26 00:12:19]

lgtm

[commit-bot: I haz the power, 2012-07-26 00:12:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hebert.christopherj@chromium.org/10824...

[commit-bot: I haz the power, 2012-07-26 02:18:16]

Change committed as 148480