Add enum class support to json_schema_compiler (idl files)

Add enum class support to json_schema_compiler (idl files)

Allows an .idl file to generate headers with enum classes that can be
forward declared. This can help avoid a proliferation of transitive
"hard" dependencies where a generated header needs to be included from a
component's publicly exposed headers.

To change an enum to an enum class, simply prefix with [enum_class].
E.g. "enum Foo { a };" changes to "[enum_class] enum Foo { a };"

Then instead of generating

enum Foo { FOO_NONE, FOO_A, FOO_LAST = FOO_A };

generates

enum class Foo { NONE, A, LAST = A };

BUG=611898, 612382

[tapted, 2016-05-17 05:37:05]

Description was changed from

==========
Add enum class support to json_schema_compiler

Use it for ui::AXEvent

git grep -l ui::AX_EVENT_ | xargs sed -i -e 's/ui::AX_EVENT_/ui::AXEvent::/g'

BUG=611898
==========

to

==========
Add enum class support to json_schema_compiler

Use it for ui::AXEvent

git grep -l ui::AX_EVENT_ | xargs sed -i -e 's/ui::AX_EVENT_/ui::AXEvent::/g'

BUG=611898, 612382
==========

[tapted, 2016-05-17 08:19:02]

Description was changed from

==========
Add enum class support to json_schema_compiler

Use it for ui::AXEvent

git grep -l ui::AX_EVENT_ | xargs sed -i -e 's/ui::AX_EVENT_/ui::AXEvent::/g'

BUG=611898, 612382
==========

to

==========
Add enum class support to json_schema_compiler (idl files)

Allows an .idl file to generate headers with enum classes that can be
forward declared. This can help avoid a proliferation of transitive
"hard" dependencies where a generated header needs to be included from a
component's publicly exposed headers.

To change an enum to an enum class, simply prefix with [class]. E.g.
"enum Foo { a };" changes to "[class] enum Foo { a };"

Then instead of generating

enum Foo { FOO_NONE, FOO_A, FOO LAST = FOO_A };

generates

enum class Foo { NONE, A, LAST = A };

BUG=611898, 612382
==========

[tapted, 2016-05-17 08:19:38]

Description was changed from

==========
Add enum class support to json_schema_compiler (idl files)

Allows an .idl file to generate headers with enum classes that can be
forward declared. This can help avoid a proliferation of transitive
"hard" dependencies where a generated header needs to be included from a
component's publicly exposed headers.

To change an enum to an enum class, simply prefix with [class]. E.g.
"enum Foo { a };" changes to "[class] enum Foo { a };"

Then instead of generating

enum Foo { FOO_NONE, FOO_A, FOO LAST = FOO_A };

generates

enum class Foo { NONE, A, LAST = A };

BUG=611898, 612382
==========

to

==========
Add enum class support to json_schema_compiler (idl files)

Allows an .idl file to generate headers with enum classes that can be
forward declared. This can help avoid a proliferation of transitive
"hard" dependencies where a generated header needs to be included from a
component's publicly exposed headers.

To change an enum to an enum class, simply prefix with [class]. E.g.
"enum Foo { a };" changes to "[class] enum Foo { a };"

Then instead of generating

enum Foo { FOO_NONE, FOO_A, FOO_LAST = FOO_A };

generates

enum class Foo { NONE, A, LAST = A };

BUG=611898, 612382
==========

[tapted, 2016-05-17 12:49:38]

Patchset #4 (id:60001) has been deleted

[tapted, 2016-05-17 12:50:00]

Patchset #3 (id:40001) has been deleted

[tapted, 2016-05-17 12:59:44]

tapted@chromium.org changed reviewers:
+ rdevlin.cronin@chromium.org

[tapted, 2016-05-17 12:59:44]

Guess I might as well publish this out. Follow-up that applies this to AXEvent
is in http://crrev.com/1988613002. A couple of views headers also expose AXRole,
so that would be next.

[Devlin, 2016-05-17 19:04:11]

Is the goal to eventually fully phase out vanilla enums here?  (I'd be
supportive of that.)  If so, can you add a TODO somewhere in here to do the
conversions and remove support for non-class enums?

https://codereview.chromium.org/1982193002/diff/140001/tools/json_schema_comp...
File tools/json_schema_compiler/cpp_type_generator.py (right):

https://codereview.chromium.org/1982193002/diff/140001/tools/json_schema_comp...
tools/json_schema_compiler/cpp_type_generator.py:58: return
self._AddEnumScope(type_, 'NONE', is_in_declaration, False)
The fact that optional enums are NONEs instead of just an empty unique_ptr is
pretty annoying, and there's even a TODO lying around to fix it. [1]  Since
everywhere we change to an enum class will have to be updated, maybe this is a
good time to change that?  WDYT?

https://codereview.chromium.org/1982193002/diff/140001/tools/json_schema_comp...
File tools/json_schema_compiler/test/idl_basics.idl (right):

https://codereview.chromium.org/1982193002/diff/140001/tools/json_schema_comp...
tools/json_schema_compiler/test/idl_basics.idl:20:
[cpp_enum_prefix_override="prefix"] enum EnumWithPrefix {
Do we want to support prefixes for vanilla enums?  What's the use case?

https://codereview.chromium.org/1982193002/diff/140001/tools/json_schema_comp...
tools/json_schema_compiler/test/idl_basics.idl:30:
[class,cpp_enum_prefix_override="prefix"] enum EnumClassWithPrefix {
nit: cpp_enum_prefix_override is quite a mouthful.  Maybe just "prefix"? 
Conversely, [class] actually seems not-as-descriptive - maybe [use_class] or a
full [enum_class]?

These are just my opinions, and don't really affect all that much, so I'll leave
it up to you.

[tapted, 2016-05-18 09:35:05]

Description was changed from

==========
Add enum class support to json_schema_compiler (idl files)

Allows an .idl file to generate headers with enum classes that can be
forward declared. This can help avoid a proliferation of transitive
"hard" dependencies where a generated header needs to be included from a
component's publicly exposed headers.

To change an enum to an enum class, simply prefix with [class]. E.g.
"enum Foo { a };" changes to "[class] enum Foo { a };"

Then instead of generating

enum Foo { FOO_NONE, FOO_A, FOO_LAST = FOO_A };

generates

enum class Foo { NONE, A, LAST = A };

BUG=611898, 612382
==========

to

==========
Add enum class support to json_schema_compiler (idl files)

Allows an .idl file to generate headers with enum classes that can be
forward declared. This can help avoid a proliferation of transitive
"hard" dependencies where a generated header needs to be included from a
component's publicly exposed headers.

To change an enum to an enum class, simply prefix with [enum_class].
E.g. "enum Foo { a };" changes to "[enum_class] enum Foo { a };"

Then instead of generating

enum Foo { FOO_NONE, FOO_A, FOO_LAST = FOO_A };

generates

enum class Foo { NONE, A, LAST = A };

BUG=611898, 612382
==========

[tapted, 2016-05-18 09:35:54]

On 2016/05/17 19:04:11, Devlin wrote:
> Is the goal to eventually fully phase out vanilla enums here?  (I'd be
> supportive of that.)

Yup - I think that makes the most sense. It would be confusing to have a mix of
usage in Chrome, particularly for things from the same idl file.

> If so, can you add a TODO somewhere in here to do the
> conversions and remove support for non-class enums?

Done.

https://codereview.chromium.org/1982193002/diff/140001/tools/json_schema_comp...
File tools/json_schema_compiler/cpp_type_generator.py (right):

https://codereview.chromium.org/1982193002/diff/140001/tools/json_schema_comp...
tools/json_schema_compiler/cpp_type_generator.py:58: return
self._AddEnumScope(type_, 'NONE', is_in_declaration, False)
On 2016/05/17 19:04:10, Devlin wrote:
> The fact that optional enums are NONEs instead of just an empty unique_ptr is
> pretty annoying, and there's even a TODO lying around to fix it. [1]  Since
> everywhere we change to an enum class will have to be updated, maybe this is a
> good time to change that?  WDYT?

[1] -> null pointer? :)

But I think it would be tricky to map a unique_ptr into the use cases for things
like AXEvent and other things from ax_enums.idl. The same enums are used
extensively in browser-only UI, without touching json at all (no base::*Value
etc.). And, e.g., ui::AX_EVENT_NONE is used often as a sentinel value. So I
think we would still need `NONE` at least for the stuff in ax_enums.idl, but we
could perhaps phase it out of some other generated types that only ever operate
using base::Value and `NONE` is an error state rather than a ~"don't care"
state.

https://codereview.chromium.org/1982193002/diff/140001/tools/json_schema_comp...
File tools/json_schema_compiler/test/idl_basics.idl (right):

https://codereview.chromium.org/1982193002/diff/140001/tools/json_schema_comp...
tools/json_schema_compiler/test/idl_basics.idl:20:
[cpp_enum_prefix_override="prefix"] enum EnumWithPrefix {
On 2016/05/17 19:04:10, Devlin wrote:
> Do we want to support prefixes for vanilla enums?  What's the use case?

Ah, it's actually the prefixes for class-enums that I wasn't sure about :).
prefixes are used already in ax_enums.idl. E.g., there it has:

[cpp_enum_prefix_override="ax_attr"] enum AXStringAttribute {
    access_key,
  ...
}

[cpp_enum_prefix_override="ax_attr"] enum AXIntAttribute {
    scroll_x,
  ...
}

These generate

AX_ATTR_ACCESS_KEY
AX_ATTR_SCROLL_X


When these switch directly to enum classes, currently the approach will be

AXStringAttribute::AX_ATTR_ACCESS_KEY
AXIntAttribute::AX_ATTR_SCROLL_X


or the prefix could be removed to become

AXStringAttribute::ACCESS_KEY
AXIntAttribute::SCROLL_X


or -- depending how they're used -- we could merge them, and just have

AXAttr::ACCESS_KEY
AXAttr::SCROLL_X


But that might violate some use cases -- perhaps there is type matching for
overloads somewhere.


Perhaps for these it would make sense to keep them as non-class enums and turn
cpp_enum_prefix_override+enum_class into a hard error. For now I didn't really
add anything special, but just ensured it wouldn't fail. And if somewhere
there's a preference to have an additional FOO_ prefix on class enums too, then
that's supported.

https://codereview.chromium.org/1982193002/diff/140001/tools/json_schema_comp...
tools/json_schema_compiler/test/idl_basics.idl:30:
[class,cpp_enum_prefix_override="prefix"] enum EnumClassWithPrefix {
On 2016/05/17 19:04:10, Devlin wrote:
> nit: cpp_enum_prefix_override is quite a mouthful.  Maybe just "prefix"? 
> Conversely, [class] actually seems not-as-descriptive - maybe [use_class] or a
> full [enum_class]?
> 
> These are just my opinions, and don't really affect all that much, so I'll
leave
> it up to you.

Yup I agree 'class' is a bit terse - went with enum_class.

changing cpp_enum_prefix_override would alter some idl files, so is a bit
orthogonal. (I just wanted to ensure there was some test coverage for this
existing use case :).

[Devlin, 2016-05-19 00:33:12]

lgtm

https://codereview.chromium.org/1982193002/diff/140001/tools/json_schema_comp...
File tools/json_schema_compiler/cpp_type_generator.py (right):

https://codereview.chromium.org/1982193002/diff/140001/tools/json_schema_comp...
tools/json_schema_compiler/cpp_type_generator.py:58: return
self._AddEnumScope(type_, 'NONE', is_in_declaration, False)
On 2016/05/18 09:35:54, tapted wrote:
> On 2016/05/17 19:04:10, Devlin wrote:
> > The fact that optional enums are NONEs instead of just an empty unique_ptr
is
> > pretty annoying, and there's even a TODO lying around to fix it. [1]  Since
> > everywhere we change to an enum class will have to be updated, maybe this is
a
> > good time to change that?  WDYT?
> 
> [1] -> null pointer? :)
> 
> But I think it would be tricky to map a unique_ptr into the use cases for
things
> like AXEvent and other things from ax_enums.idl. The same enums are used
> extensively in browser-only UI, without touching json at all (no base::*Value
> etc.). And, e.g., ui::AX_EVENT_NONE is used often as a sentinel value. So I
> think we would still need `NONE` at least for the stuff in ax_enums.idl, but
we
> could perhaps phase it out of some other generated types that only ever
operate
> using base::Value and `NONE` is an error state rather than a ~"don't care"
> state.

Whoops, [1] ->
https://code.google.com/p/chromium/codesearch#chromium/src/tools/json_schema_...

Is there any case outside of ax events that actually uses NONE for something
other than optional values?  It's really inconsistent, so I'd like to fix it at
some point, and, given that this is a massive enum overhauling, it seemed
appropriate.  But... ah well.

https://codereview.chromium.org/1982193002/diff/180001/tools/json_schema_comp...
File tools/json_schema_compiler/h_generator.py (right):

https://codereview.chromium.org/1982193002/diff/180001/tools/json_schema_comp...
tools/json_schema_compiler/h_generator.py:144: c.Sblock('enum %s%s {' %
(type_.is_class and 'class ' or '', enum_name))
ha nifty, but is this pythonic?  Maybe it's my c++-brain, but I think I'd still
prefer:
'class ' if type_.is_class else ''

https://codereview.chromium.org/1982193002/diff/180001/tools/json_schema_comp...
File tools/json_schema_compiler/model.py (right):

https://codereview.chromium.org/1982193002/diff/180001/tools/json_schema_comp...
tools/json_schema_compiler/model.py:215: self.is_class = json.get('enum_class',
False)
nit: here, too, I'd prefer this to be enum_class or is_enum_class.

[tapted, 2016-05-19 12:22:33]

(I'll wait for after branch for this - I don't think it's risky, but there's no
point stressing things)

https://codereview.chromium.org/1982193002/diff/140001/tools/json_schema_comp...
File tools/json_schema_compiler/cpp_type_generator.py (right):

https://codereview.chromium.org/1982193002/diff/140001/tools/json_schema_comp...
tools/json_schema_compiler/cpp_type_generator.py:58: return
self._AddEnumScope(type_, 'NONE', is_in_declaration, False)
On 2016/05/19 00:33:12, Devlin wrote:
> On 2016/05/18 09:35:54, tapted wrote:
> > On 2016/05/17 19:04:10, Devlin wrote:
> > > The fact that optional enums are NONEs instead of just an empty unique_ptr
> is
> > > pretty annoying, and there's even a TODO lying around to fix it. [1] 
Since
> > > everywhere we change to an enum class will have to be updated, maybe this
is
> a
> > > good time to change that?  WDYT?
> > 
> > [1] -> null pointer? :)
> > 
> > But I think it would be tricky to map a unique_ptr into the use cases for
> things
> > like AXEvent and other things from ax_enums.idl. The same enums are used
> > extensively in browser-only UI, without touching json at all (no
base::*Value
> > etc.). And, e.g., ui::AX_EVENT_NONE is used often as a sentinel value. So I
> > think we would still need `NONE` at least for the stuff in ax_enums.idl, but
> we
> > could perhaps phase it out of some other generated types that only ever
> operate
> > using base::Value and `NONE` is an error state rather than a ~"don't care"
> > state.
> 
> Whoops, [1] ->
>
https://code.google.com/p/chromium/codesearch#chromium/src/tools/json_schema_...
> 
> Is there any case outside of ax events that actually uses NONE for something
> other than optional values?  It's really inconsistent, so I'd like to fix it
at
> some point, and, given that this is a massive enum overhauling, it seemed
> appropriate.  But... ah well.

`git grep ui::AX.*NONE` shows up a lot of other ax examples that would be tricky
to migrate from `none` (e.g. AX_ROLE, and others apart from AX_EVENT). And while
`git grep -C10 'api::.*_NONE'` under extensions/browser/api shows up mostly
error-state handling (DCHECK, NOTREACHED, and so forth), there are a lot of
situations where an enum is returned, and there's some logic that requires it to
be initialized to "some" value beforehand.

The change to make these methods return, e.g., a scoped_ptr<base::Value> instead
of an enum wouldn't be mechanical. Whereas changing from an enum to an enum
class is almost purely mechanical -- a simple sed script and `git cl format` do
most of the job. All that was left in the AXEvent change in
http://crrev.com/1988613002 was adding a static_cast<int> to an stream output.
So, while it is an overhaul, it's a straightforward one.

https://codereview.chromium.org/1982193002/diff/180001/tools/json_schema_comp...
File tools/json_schema_compiler/h_generator.py (right):

https://codereview.chromium.org/1982193002/diff/180001/tools/json_schema_comp...
tools/json_schema_compiler/h_generator.py:144: c.Sblock('enum %s%s {' %
(type_.is_class and 'class ' or '', enum_name))
On 2016/05/19 00:33:12, Devlin wrote:
> ha nifty, but is this pythonic?  Maybe it's my c++-brain, but I think I'd
still
> prefer:
> 'class ' if type_.is_class else ''

Ah, apparently this is me revealing how long it's been since I've done serious
python :). My brain had C++'s ternary if-else wired to this construct, but
Python 2.5 introduced an actual ternary syntax "A if condition else B" which is
used elsewhere in json_schema_compiler - so I've switched over to that.

https://codereview.chromium.org/1982193002/diff/180001/tools/json_schema_comp...
File tools/json_schema_compiler/model.py (right):

https://codereview.chromium.org/1982193002/diff/180001/tools/json_schema_comp...
tools/json_schema_compiler/model.py:215: self.is_class = json.get('enum_class',
False)
On 2016/05/19 00:33:12, Devlin wrote:
> nit: here, too, I'd prefer this to be enum_class or is_enum_class.

Done.