Support scoped types in PPAPI IDL.

Support scoped types in PPAPI IDL.

IDL parser didn't support namespaces, which however are supported by JSON.
This patch adds grammar to allow adding a namespace as a prefix to any type.

TEST=json_schema_test.py + tested manually that it works.
BUG=158654
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=261911

[mtomasz, 2014-03-13 06:52:37]

@kalman, @nbarth: PTAL. Thanks.

[mtomasz, 2014-03-13 06:55:12]

@noelallen: PTAL, since @kalman is OOO.

[Nils Barth (inactive), 2014-03-13 07:24:54]

idl_parser changes LGTM w/ nits.

https://codereview.chromium.org/197873009/diff/1/ppapi/generators/idl_parser.py
File ppapi/generators/idl_parser.py (right):

https://codereview.chromium.org/197873009/diff/1/ppapi/generators/idl_parser....
ppapi/generators/idl_parser.py:277: p[0] = "".join(p[1:])
Style: '' (single quotes in Python).

[noelallen_use_chromium, 2014-03-13 16:31:05]

otherwise LGTM

https://codereview.chromium.org/197873009/diff/1/ppapi/generators/idl_parser.py
File ppapi/generators/idl_parser.py (right):

https://codereview.chromium.org/197873009/diff/1/ppapi/generators/idl_parser....
ppapi/generators/idl_parser.py:275: """typeref : SYMBOL
This does not appear to be recursive.  It only supports SYMBOL and SYMBOL.SYMBOL
 what about SYMBOL.SYMBOL.SYMBOL?   See namespace bellow.

[mtomasz, 2014-03-14 00:36:43]

https://codereview.chromium.org/197873009/diff/1/ppapi/generators/idl_parser.py
File ppapi/generators/idl_parser.py (right):

https://codereview.chromium.org/197873009/diff/1/ppapi/generators/idl_parser....
ppapi/generators/idl_parser.py:275: """typeref : SYMBOL
On 2014/03/13 16:31:05, noelallen wrote:
> This does not appear to be recursive.  It only supports SYMBOL and
SYMBOL.SYMBOL
>  what about SYMBOL.SYMBOL.SYMBOL?   See namespace bellow.

Done.

https://codereview.chromium.org/197873009/diff/1/ppapi/generators/idl_parser....
ppapi/generators/idl_parser.py:277: p[0] = "".join(p[1:])
On 2014/03/13 07:24:54, Nils Barth wrote:
> Style: '' (single quotes in Python).

Done.

[Nils Barth (inactive), 2014-03-14 00:41:44]

How about merging the two rules, suitably renamed?
(As discussed offline.)

https://codereview.chromium.org/197873009/diff/1/ppapi/generators/idl_parser.py
File ppapi/generators/idl_parser.py (right):

https://codereview.chromium.org/197873009/diff/1/ppapi/generators/idl_parser....
ppapi/generators/idl_parser.py:275: """typeref : SYMBOL
On 2014/03/13 16:31:05, noelallen wrote:
> This does not appear to be recursive.  It only supports SYMBOL and
SYMBOL.SYMBOL
>  what about SYMBOL.SYMBOL.SYMBOL?   See namespace bellow.

Good point! (>.<)

Actually, this grammar rule is identical to |namespace_name|,
so we could reuse it,
preferably by renaming it to |scoped_name|.

Chatted with Tomasz, and he noted correctly that it would be
confusing to use |namespace_name| for typeref (since they're
semantically different), but |scoped_name| should be clear.

[noelallen_use_chromium, 2014-03-14 00:44:41]

+1 to scoped_name

[mtomasz, 2014-03-14 00:49:46]

On 2014/03/14 00:44:41, noelallen wrote:
> +1 to scoped_name

Talked to @nbarth, and this would be confusing to use scoped_name in place of
type in the arguments grammar.
@nbarth suggested changing p_typeref to:

typeref : SYMBOL | namespace_name '.' typeref

This works same way, but it is very logical and easy to understand.
WDYT?

[mtomasz, 2014-03-14 00:50:24]

On 2014/03/14 00:49:46, mtomasz wrote:
> On 2014/03/14 00:44:41, noelallen wrote:
> > +1 to scoped_name
> 
> Talked to @nbarth, and this would be confusing to use scoped_name in place of
> type in the arguments grammar.
> @nbarth suggested changing p_typeref to:
> 
> typeref : SYMBOL | namespace_name '.' typeref
> 
> This works same way, but it is very logical and easy to understand.
> WDYT?

Correction:
typeref : SYMBOL | namespace_name '.' SYMBOL

[Nils Barth (inactive), 2014-03-14 00:52:10]

https://codereview.chromium.org/197873009/diff/1/ppapi/generators/idl_parser.py
File ppapi/generators/idl_parser.py (right):

https://codereview.chromium.org/197873009/diff/1/ppapi/generators/idl_parser....
ppapi/generators/idl_parser.py:275: """typeref : SYMBOL
Discussed offline; syntactically they're the same,
but the grammar is clearer if we say
typeref : SYMBOL
        | namespace_name '.' SYMBOL

...since semantically name.space.type (scope*d* name)
is different from name.space (scop*e* name).

[mtomasz, 2014-03-14 00:58:58]

On 2014/03/14 00:52:10, Nils Barth wrote:
>
https://codereview.chromium.org/197873009/diff/1/ppapi/generators/idl_parser.py
> File ppapi/generators/idl_parser.py (right):
> 
>
https://codereview.chromium.org/197873009/diff/1/ppapi/generators/idl_parser....
> ppapi/generators/idl_parser.py:275: """typeref : SYMBOL
> Discussed offline; syntactically they're the same,
> but the grammar is clearer if we say
> typeref : SYMBOL
>         | namespace_name '.' SYMBOL
> 
> ...since semantically name.space.type (scope*d* name)
> is different from name.space (scop*e* name).

Strangely, this grammar doesn't work.

test/idl_basics.idl(84) : Error : Unexpected symbol value after symbol SomeType.
84: static void function20(other_namespace.SomeType value);

[mtomasz, 2014-03-14 01:37:47]

On 2014/03/14 00:58:58, mtomasz wrote:
> On 2014/03/14 00:52:10, Nils Barth wrote:
> >
>
https://codereview.chromium.org/197873009/diff/1/ppapi/generators/idl_parser.py
> > File ppapi/generators/idl_parser.py (right):
> > 
> >
>
https://codereview.chromium.org/197873009/diff/1/ppapi/generators/idl_parser....
> > ppapi/generators/idl_parser.py:275: """typeref : SYMBOL
> > Discussed offline; syntactically they're the same,
> > but the grammar is clearer if we say
> > typeref : SYMBOL
> >         | namespace_name '.' SYMBOL
> > 
> > ...since semantically name.space.type (scope*d* name)
> > is different from name.space (scop*e* name).
> 
> Strangely, this grammar doesn't work.
> 
> test/idl_basics.idl(84) : Error : Unexpected symbol value after symbol
SomeType.
> 84: static void function20(other_namespace.SomeType value);

Done. I introduc ed the scoped_name grammars and aliased namespace_name and
typeref to them. PTAL.

[Nils Barth (inactive), 2014-03-14 01:40:41]

On 2014/03/14 01:37:47, mtomasz wrote:
> Done. I introduced the scoped_name grammars and aliased namespace_name and
> typeref to them. PTAL.

LGTM, thanks for making this clean!

[mtomasz, 2014-03-14 01:55:33]

The CQ bit was checked by mtomasz@chromium.org

[commit-bot: I haz the power, 2014-03-14 01:56:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mtomasz@chromium.org/197873009/60001

[commit-bot: I haz the power, 2014-03-14 02:04:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-14 02:04:14]

Try jobs failed on following builders:
  tryserver.chromium on win_chromium_compile_dbg

[mtomasz, 2014-03-14 05:11:10]

On 2014/03/14 02:04:14, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   tryserver.chromium on win_chromium_compile_dbg

FYI:

Unfortunately cc_generator.py doesn't generate a proper CC code,
eg:

params->value = Parseidl_other_namespace::SomeType(value_as_string)
instead of:
params->value = idl_other_namespace::ParseSomeType(value_as_string)

I'll take a look at it quickly.

[mtomasz, 2014-03-14 06:00:29]

On 2014/03/14 05:11:10, mtomasz wrote:
> On 2014/03/14 02:04:14, I haz the power (commit-bot) wrote:
> > Try jobs failed on following builders:
> >   tryserver.chromium on win_chromium_compile_dbg
> 
> FYI:
> 
> Unfortunately cc_generator.py doesn't generate a proper CC code,
> eg:
> 
> params->value = Parseidl_other_namespace::SomeType(value_as_string)
> instead of:
> params->value = idl_other_namespace::ParseSomeType(value_as_string)
> 
> I'll take a look at it quickly.

It shouldn't work for JSON either. Seems that generating string->enum is broken
for namespaced types. I was thinking about splitting the cpp_type (here:
idl_other_namespace::SomeType), to idl_other_namespace and SomeType, and putting
the 'Parse' prefix before the type name, and after the namespace. However,
it seems non-trivial to do it neatly. There is eg.
schema_util.(SplitNamespace|StripNamespace|GetNamespace) but it doesn't work
because type_.name doesn't contain the namespace.
I'll come back to this CL once I have more time.

[Nils Barth (inactive), 2014-03-17 05:12:37]

> On 2014/03/14 00:52:10, Nils Barth wrote:
> > Discussed offline; syntactically they're the same,
> > but the grammar is clearer if we say
> > typeref : SYMBOL
> >         | namespace_name '.' SYMBOL
> > 
> > ...since semantically name.space.type (scope*d* name)
> > is different from name.space (scop*e* name).
> 
> Strangely, this grammar doesn't work.
> 
> test/idl_basics.idl(84) : Error : Unexpected symbol value after symbol
SomeType.
> 84: static void function20(other_namespace.SomeType value);

(This is solved by using one underlying rule, but for reference...)

I think the problem is that you're mixing left branching and right branching;
typeref : SYMBOL
        | namespace_name '.' SYMBOL
namespace_name : SYMBOL
               | SYMBOL '.' namespace_name
Your grammar is unambiguous, but it's causing problems for the table-based
parser.
In general in LL/LALR parsers, you want to prefer right branching.
(This isn't left recursion though: typeref branches left, then namespace_name
recurses right.)

So I think what's happening is that namespace_name eats
other_namespace.SomeType,
and then fails to find a following '.', instead finding 'value'.
That is, when the parser encounters a.b (other_namespace.SomeType) while
applying the
typeref rule inside the arguments rule, it applies typeref = namespace_name '.'
SYMBOL
...and then namespace_name keeps getting applied, matching a.b, and then fails
when you get back to typeref and it expects a '.' (but instead gets the
following SYMBOL 'value')

FWIW, the grammar given for namespace handling is not LL(1);
it's ok in this case (we can be LALR, but LL is usual easier to recognize and
debug),
but an LL(1) parser will have trouble with:
namespace_name : SYMBOL
               | SYMBOL '.' namespace_name
...b/c it's got a FIRST/FIRST conflict (both rules start with SYMBOL)

...so it should be:
namespace_name : SYMBOL namespace_name_rest
namespace_name_rest : '.' namespace_name
                    |
...or maybe:
namespace_name : SYMBOL namespace_name_rest
namespace_name_rest : '.' SYMBOL namespace_name_rest
                    |
(aka, "left factoring")

References:
http://en.wikipedia.org/wiki/LL_parser#Left_Factoring
http://en.wikipedia.org/wiki/Left_recursion

[not at google - send to devlin, 2014-03-17 14:00:28]

tests lgtm

https://codereview.chromium.org/197873009/diff/60001/tools/json_schema_compil...
File tools/json_schema_compiler/idl_schema_test.py (right):

https://codereview.chromium.org/197873009/diff/60001/tools/json_schema_compil...
tools/json_schema_compiler/idl_schema_test.py:105: expected = [{'name':'value',
'$ref':'other_namespace.SomeType'}]
nit: spaces after the : (this file is oddly inconsistent)

[mtomasz, 2014-03-28 04:48:31]

https://codereview.chromium.org/197873009/diff/60001/tools/json_schema_compil...
File tools/json_schema_compiler/idl_schema_test.py (right):

https://codereview.chromium.org/197873009/diff/60001/tools/json_schema_compil...
tools/json_schema_compiler/idl_schema_test.py:105: expected = [{'name':'value',
'$ref':'other_namespace.SomeType'}]
On 2014/03/17 14:00:29, kalman wrote:
> nit: spaces after the : (this file is oddly inconsistent)

Done.

[mtomasz, 2014-03-28 04:49:49]

On 2014/03/28 04:48:31, mtomasz wrote:
>
https://codereview.chromium.org/197873009/diff/60001/tools/json_schema_compil...
> File tools/json_schema_compiler/idl_schema_test.py (right):
> 
>
https://codereview.chromium.org/197873009/diff/60001/tools/json_schema_compil...
> tools/json_schema_compiler/idl_schema_test.py:105: expected =
[{'name':'value',
> '$ref':'other_namespace.SomeType'}]
> On 2014/03/17 14:00:29, kalman wrote:
> > nit: spaces after the : (this file is oddly inconsistent)
> 
> Done.

I just fixed the code generator. It seems that it works very well now.
@nbarth: Thanks for the investigation about the grammar. I simplified it
according to your suggestion.
@kalman, @noelallen: PTAL.

Thanks.

[Nils Barth (inactive), 2014-03-28 05:01:19]

Grammar suggestion.

https://codereview.chromium.org/197873009/diff/80001/ppapi/generators/idl_par...
File ppapi/generators/idl_parser.py (right):

https://codereview.chromium.org/197873009/diff/80001/ppapi/generators/idl_par...
ppapi/generators/idl_parser.py:277: """scoped_name : SYMBOL
This should instead be:
scoped_name : SYMBOL scoped_name_rest

scoped_name_rest : '.' scoped_name
                 |

...which makes this LL(1).
(Right branching, 1 token of look-ahead.)

[mtomasz, 2014-03-28 05:19:38]

https://codereview.chromium.org/197873009/diff/80001/ppapi/generators/idl_par...
File ppapi/generators/idl_parser.py (right):

https://codereview.chromium.org/197873009/diff/80001/ppapi/generators/idl_par...
ppapi/generators/idl_parser.py:277: """scoped_name : SYMBOL
On 2014/03/28 05:01:19, Nils Barth wrote:
> This should instead be:
> scoped_name : SYMBOL scoped_name_rest
> 
> scoped_name_rest : '.' scoped_name
>                  |
> 
> ...which makes this LL(1).
> (Right branching, 1 token of look-ahead.)

Done.

[Nils Barth (inactive), 2014-03-28 05:25:13]

https://codereview.chromium.org/197873009/diff/80001/ppapi/generators/idl_par...
File ppapi/generators/idl_parser.py (right):

https://codereview.chromium.org/197873009/diff/80001/ppapi/generators/idl_par...
ppapi/generators/idl_parser.py:277: """scoped_name : SYMBOL
Thanks!
LGTM again!

[mtomasz, 2014-03-28 07:32:44]

I just fixed schema_loader.cc, which didn't work for Chrome's API IDLs. The
reason is that in that IDLs we use camelCase namespace names, which is *not*
equal to underscored file name of that IDL. As a result, schema_loader.py was
failing.

The recent patch fixes it by converting camelCased name space to underscored
namespace when trying to find the file having the namespace defined.

This sounds like hacky heuristics, but anyway we've been doing heuristics in the
schema_loader.py already, by guessing file extension (json/idl).

[not at google - send to devlin, 2014-03-28 18:42:24]

https://codereview.chromium.org/197873009/diff/110001/tools/json_schema_compi...
File tools/json_schema_compiler/cc_generator.py (right):

https://codereview.chromium.org/197873009/diff/110001/tools/json_schema_compi...
tools/json_schema_compiler/cc_generator.py:689: underlying_type.item_type,  #
Pass ref.
what does "Pass ref" mean?

https://codereview.chromium.org/197873009/diff/110001/tools/json_schema_compi...
tools/json_schema_compiler/cc_generator.py:804: # namespace.
It surprises me that enums need special treatment like this. what's different
about enums vs normal $ref aliases?

https://codereview.chromium.org/197873009/diff/110001/tools/json_schema_compi...
File tools/json_schema_compiler/schema_loader.py (right):

https://codereview.chromium.org/197873009/diff/110001/tools/json_schema_compi...
tools/json_schema_compiler/schema_loader.py:40: namespace_name.replace('.',
'_')).lower()
there is already a helper for this sort of thing in model.UnixName

[mtomasz, 2014-03-29 04:22:52]

https://codereview.chromium.org/197873009/diff/110001/tools/json_schema_compi...
File tools/json_schema_compiler/cc_generator.py (right):

https://codereview.chromium.org/197873009/diff/110001/tools/json_schema_compi...
tools/json_schema_compiler/cc_generator.py:804: # namespace.
On 2014/03/28 18:42:25, kalman wrote:
> It surprises me that enums need special treatment like this. what's different
> about enums vs normal $ref aliases?

For enums we need to generate C++ code like this:
hello = namespace::ParseHello().

So we need to inject 'Parse' after the namespace, and before the type name.

[mtomasz, 2014-03-31 01:39:19]

Done. @kalman: PTAL. Thanks.

https://codereview.chromium.org/197873009/diff/110001/tools/json_schema_compi...
File tools/json_schema_compiler/cc_generator.py (right):

https://codereview.chromium.org/197873009/diff/110001/tools/json_schema_compi...
tools/json_schema_compiler/cc_generator.py:689: underlying_type.item_type,  #
Pass ref.
On 2014/03/28 18:42:25, kalman wrote:
> what does "Pass ref" mean?

PropertyType of the passed type is equal to REF. Removed, since confusing. Done.

https://codereview.chromium.org/197873009/diff/110001/tools/json_schema_compi...
tools/json_schema_compiler/cc_generator.py:804: # namespace.
On 2014/03/29 04:22:53, mtomasz wrote:
> On 2014/03/28 18:42:25, kalman wrote:
> > It surprises me that enums need special treatment like this. what's
different
> > about enums vs normal $ref aliases?
> 
> For enums we need to generate C++ code like this:
> hello = namespace::ParseHello().
> 
> So we need to inject 'Parse' after the namespace, and before the type name.

I simplified the code. That's correct, that we don't need separate logic for
ENUM and REF. Done.

https://codereview.chromium.org/197873009/diff/110001/tools/json_schema_compi...
File tools/json_schema_compiler/schema_loader.py (right):

https://codereview.chromium.org/197873009/diff/110001/tools/json_schema_compi...
tools/json_schema_compiler/schema_loader.py:40: namespace_name.replace('.',
'_')).lower()
On 2014/03/28 18:42:25, kalman wrote:
> there is already a helper for this sort of thing in model.UnixName

Done.

[not at google - send to devlin, 2014-03-31 15:18:58]

https://codereview.chromium.org/197873009/diff/150001/tools/json_schema_compi...
File tools/json_schema_compiler/cc_generator.py (left):

https://codereview.chromium.org/197873009/diff/150001/tools/json_schema_compi...
tools/json_schema_compiler/cc_generator.py:813:
self._type_helper.GetEnumNoneValue(type_)))
if/when you do change this back, mind indenting this 2 spaces to align it
properly?

https://codereview.chromium.org/197873009/diff/150001/tools/json_schema_compi...
File tools/json_schema_compiler/cc_generator.py (right):

https://codereview.chromium.org/197873009/diff/150001/tools/json_schema_compi...
tools/json_schema_compiler/cc_generator.py:792: type_,
can you just pass in the correct underlying type here? i.e. call
_GenerateStringToEnumVersion with FollowRef(type_) rather than just type_? and
assert that the type that gets passed into here is an enum?

https://codereview.chromium.org/197873009/diff/150001/tools/json_schema_compi...
tools/json_schema_compiler/cc_generator.py:802: cpp_type_fullname =
self._type_helper.GetCppType(type_)
this isn't used

https://codereview.chromium.org/197873009/diff/150001/tools/json_schema_compi...
tools/json_schema_compiler/cc_generator.py:805: enum_type = (type_ if
type_.property_type == PropertyType.ENUM else
see comment above, note that FollowRef does this check already (i.e. this should
be the same as just enum_type = self._type_helper.FollowRef(type_) without the
extra ternary-if)

https://codereview.chromium.org/197873009/diff/150001/tools/json_schema_compi...
tools/json_schema_compiler/cc_generator.py:807: cpp_type_namespace =
(enum_type.namespace.unix_name
if you make sure this includes the '::' then that will simplify the code below.

also, ternary-if is pretty awkward syntax in python and rarely appropriate. I
would prefer if you wrote it as

cpp_type_namespace = ''
if enum_type.namespace:
  cpp_type_namespace = enum_type.namespace + '::'

https://codereview.chromium.org/197873009/diff/150001/tools/json_schema_compi...
tools/json_schema_compiler/cc_generator.py:809: cpp_type_name =
self._type_helper.GetCppType(enum_type)
if you go back to inlining this, and implement the suggestion above, then the
code below hardly needs to change.

[Nils Barth (inactive), 2014-04-01 01:39:16]

(Minor style comment.)

https://codereview.chromium.org/197873009/diff/150001/tools/json_schema_compi...
File tools/json_schema_compiler/cc_generator.py (right):

https://codereview.chromium.org/197873009/diff/150001/tools/json_schema_compi...
tools/json_schema_compiler/cc_generator.py:807: cpp_type_namespace =
(enum_type.namespace.unix_name
On 2014/03/31 15:18:59, kalman wrote:
> also, ternary-if is pretty awkward syntax in python and rarely appropriate. I
> would prefer if you wrote it as
> 
> cpp_type_namespace = ''
> if enum_type.namespace:
>   cpp_type_namespace = enum_type.namespace + '::'

BTW, as a style point, an alternative style is single assignment
with the conditional expression expanded:
if enum_type.namespace:
  cpp_type_namespace = enum_type.namespace + '::'
else:
  cpp_type_namespace = ''

(We use this is Blink bindings generation, but of course style should
agree with surrounding code, so should follow Ben's style rec.)

[not at google - send to devlin, 2014-04-01 01:40:24]

https://codereview.chromium.org/197873009/diff/150001/tools/json_schema_compi...
File tools/json_schema_compiler/cc_generator.py (right):

https://codereview.chromium.org/197873009/diff/150001/tools/json_schema_compi...
tools/json_schema_compiler/cc_generator.py:807: cpp_type_namespace =
(enum_type.namespace.unix_name
On 2014/04/01 01:39:16, Nils Barth wrote:
> On 2014/03/31 15:18:59, kalman wrote:
> > also, ternary-if is pretty awkward syntax in python and rarely appropriate.
I
> > would prefer if you wrote it as
> > 
> > cpp_type_namespace = ''
> > if enum_type.namespace:
> >   cpp_type_namespace = enum_type.namespace + '::'
> 
> BTW, as a style point, an alternative style is single assignment
> with the conditional expression expanded:
> if enum_type.namespace:
>   cpp_type_namespace = enum_type.namespace + '::'
> else:
>   cpp_type_namespace = ''
> 
> (We use this is Blink bindings generation, but of course style should
> agree with surrounding code, so should follow Ben's style rec.)

I don't mind either way.

[mtomasz, 2014-04-03 21:50:21]

@kalman. PTAL one more time. I cleaned up and fixed cpp_type_generator.py.
Thanks.

https://codereview.chromium.org/197873009/diff/150001/tools/json_schema_compi...
File tools/json_schema_compiler/cc_generator.py (left):

https://codereview.chromium.org/197873009/diff/150001/tools/json_schema_compi...
tools/json_schema_compiler/cc_generator.py:813:
self._type_helper.GetEnumNoneValue(type_)))
On 2014/03/31 15:18:59, kalman wrote:
> if/when you do change this back, mind indenting this 2 spaces to align it
> properly?

Done.

https://codereview.chromium.org/197873009/diff/150001/tools/json_schema_compi...
File tools/json_schema_compiler/cc_generator.py (right):

https://codereview.chromium.org/197873009/diff/150001/tools/json_schema_compi...
tools/json_schema_compiler/cc_generator.py:792: type_,
On 2014/03/31 15:18:59, kalman wrote:
> can you just pass in the correct underlying type here? i.e. call
> _GenerateStringToEnumVersion with FollowRef(type_) rather than just type_? and
> assert that the type that gets passed into here is an enum?

Done.

https://codereview.chromium.org/197873009/diff/150001/tools/json_schema_compi...
tools/json_schema_compiler/cc_generator.py:802: cpp_type_fullname =
self._type_helper.GetCppType(type_)
On 2014/03/31 15:18:59, kalman wrote:
> this isn't used

Done.

https://codereview.chromium.org/197873009/diff/150001/tools/json_schema_compi...
tools/json_schema_compiler/cc_generator.py:805: enum_type = (type_ if
type_.property_type == PropertyType.ENUM else
On 2014/03/31 15:18:59, kalman wrote:
> see comment above, note that FollowRef does this check already (i.e. this
should
> be the same as just enum_type = self._type_helper.FollowRef(type_) without the
> extra ternary-if)

Done.

https://codereview.chromium.org/197873009/diff/150001/tools/json_schema_compi...
tools/json_schema_compiler/cc_generator.py:807: cpp_type_namespace =
(enum_type.namespace.unix_name
On 2014/04/01 01:40:25, kalman wrote:
> On 2014/04/01 01:39:16, Nils Barth wrote:
> > On 2014/03/31 15:18:59, kalman wrote:
> > > also, ternary-if is pretty awkward syntax in python and rarely
appropriate.
> I
> > > would prefer if you wrote it as
> > > 
> > > cpp_type_namespace = ''
> > > if enum_type.namespace:
> > >   cpp_type_namespace = enum_type.namespace + '::'
> > 
> > BTW, as a style point, an alternative style is single assignment
> > with the conditional expression expanded:
> > if enum_type.namespace:
> >   cpp_type_namespace = enum_type.namespace + '::'
> > else:
> >   cpp_type_namespace = ''
> > 
> > (We use this is Blink bindings generation, but of course style should
> > agree with surrounding code, so should follow Ben's style rec.)
> 
> I don't mind either way.

Done.

https://codereview.chromium.org/197873009/diff/150001/tools/json_schema_compi...
tools/json_schema_compiler/cc_generator.py:809: cpp_type_name =
self._type_helper.GetCppType(enum_type)
On 2014/03/31 15:18:59, kalman wrote:
> if you go back to inlining this, and implement the suggestion above, then the
> code below hardly needs to change.

Done.

[not at google - send to devlin, 2014-04-03 21:56:31]

lgtm

https://codereview.chromium.org/197873009/diff/210001/tools/json_schema_compi...
File tools/json_schema_compiler/cpp_type_generator.py (right):

https://codereview.chromium.org/197873009/diff/210001/tools/json_schema_compi...
tools/json_schema_compiler/cpp_type_generator.py:110: elif (type_.property_type
== PropertyType.ENUM or
you can do

elif type_.property_type in (PropertyType.ENUM,
                             PropertyType.OBJECT,
                             PropertyType.CHOICES)

if you prefer.

[mtomasz, 2014-04-03 22:33:37]

https://codereview.chromium.org/197873009/diff/210001/tools/json_schema_compi...
File tools/json_schema_compiler/cpp_type_generator.py (right):

https://codereview.chromium.org/197873009/diff/210001/tools/json_schema_compi...
tools/json_schema_compiler/cpp_type_generator.py:110: elif (type_.property_type
== PropertyType.ENUM or
On 2014/04/03 21:56:32, kalman wrote:
> you can do
> 
> elif type_.property_type in (PropertyType.ENUM,
>                              PropertyType.OBJECT,
>                              PropertyType.CHOICES)
> 
> if you prefer.

Done.

[mtomasz, 2014-04-03 22:33:44]

The CQ bit was checked by mtomasz@chromium.org

[commit-bot: I haz the power, 2014-04-03 22:35:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mtomasz@chromium.org/197873009/230001

[commit-bot: I haz the power, 2014-04-03 23:38:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-03 23:38:55]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_rel

[mtomasz, 2014-04-03 23:55:54]

The CQ bit was checked by mtomasz@chromium.org

[commit-bot: I haz the power, 2014-04-03 23:56:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mtomasz@chromium.org/197873009/230001

[commit-bot: I haz the power, 2014-04-04 00:31:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-04 00:31:03]

Try jobs failed on following builders:
  tryserver.chromium on mac_chromium_rel

[mtomasz, 2014-04-04 00:43:21]

The CQ bit was checked by mtomasz@chromium.org

[commit-bot: I haz the power, 2014-04-04 00:45:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mtomasz@chromium.org/197873009/230001

[commit-bot: I haz the power, 2014-04-04 01:04:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-04 01:04:21]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_chromeos_rel

[mtomasz, 2014-04-04 19:47:59]

The CQ bit was checked by mtomasz@chromium.org

[commit-bot: I haz the power, 2014-04-04 19:48:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mtomasz@chromium.org/197873009/260001

[commit-bot: I haz the power, 2014-04-04 21:26:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mtomasz@chromium.org/197873009/260001

[commit-bot: I haz the power, 2014-04-04 23:02:45]

Change committed as 261911