New C++ -> Java enum build rule + parser/generator.

New C++ -> Java enum build rule + parser/generator.

This adds a new build rule for generating Java constants for C++ enums.

BUG=405532
TBR=brettw@chromium.org

Committed: https://crrev.com/9610607d5adf4afd2ad821c94434cd749041268a
Cr-Commit-Position: refs/heads/master@{#294153}

[jam, 2014-08-19 19:44:34]

this looks great, thank you.

lgtm

please add a tracking bug for this work. we should switch all of the enums that
used the old way to this

[Yaron, 2014-08-20 05:52:02]

While I like this change because the c++ code is cleaner, and overall there's
less code so that's a good thing. However, I still worry about changes that are
made with no regard to the java layer. Maybe I'm overly-paranoid? One way to
ameliorate that (and handle multiple enums in a file) could be a syntactically
significant comment before the enum. wdyt?

https://codereview.chromium.org/484603004/diff/1/build/android/gyp/enum_prepr...
File build/android/gyp/enum_preprocess.py (right):

https://codereview.chromium.org/484603004/diff/1/build/android/gyp/enum_prepr...
build/android/gyp/enum_preprocess.py:3: # Copyright 2013 The Chromium Authors.
All rights reserved.
2014

https://codereview.chromium.org/484603004/diff/1/build/android/gyp/enum_prepr...
build/android/gyp/enum_preprocess.py:4: # Use of this source code is governed by
a BSD-style license that can be
Please add tests.

https://codereview.chromium.org/484603004/diff/1/build/android/gyp/enum_prepr...
build/android/gyp/enum_preprocess.py:45: raise Exception('Only one enum per
header file is supported.')
Are there really no examples of this?

https://codereview.chromium.org/484603004/diff/1/build/android/gyp/enum_prepr...
build/android/gyp/enum_preprocess.py:138: if options.stamp:
We have a definite output file. Why is a stamp necessary?

https://codereview.chromium.org/484603004/diff/1/build/android/java_cpp_enum....
File build/android/java_cpp_enum.gypi (right):

https://codereview.chromium.org/484603004/diff/1/build/android/java_cpp_enum....
build/android/java_cpp_enum.gypi:6: # to generate Java source files from
templates that are processed
Could use a little more description as it's more strict thant hat.

https://codereview.chromium.org/484603004/diff/1/build/android/java_cpp_enum....
build/android/java_cpp_enum.gypi:11: #    'target_name':
'android_net_java_constants',
The first example of this, isn't using this structure of name. Consider
revising.

https://codereview.chromium.org/484603004/diff/1/build/android/java_cpp_enum....
build/android/java_cpp_enum.gypi:17: #      'package_name': 'org/chromium/ui',
Maybe this is copied from elsewhere, but this should be org/chromium/net to be
consitent.

https://codereview.chromium.org/484603004/diff/1/ui/gfx/android/java_bitmap.h
File ui/gfx/android/java_bitmap.h (right):

https://codereview.chromium.org/484603004/diff/1/ui/gfx/android/java_bitmap.h...
ui/gfx/android/java_bitmap.h:19: enum BitmapConfig {
Shouldn't this be BitmapFormat? I guess that's orthogonal to this change.

https://codereview.chromium.org/484603004/diff/1/ui/gfx/android/java_bitmap.h...
ui/gfx/android/java_bitmap.h:19: enum BitmapConfig {
What about files that have multiple enums defined? Should they really all get
processed? You'll either need to specify in gyp, or have some sort of annotation
(in comments) that this should be used. Alternatively, you could enforce that
the Java name has to match the C++ name and update gyp accordingly.

[mkosiba (inactive), 2014-08-20 14:07:53]

On 2014/08/19 19:44:34, jam wrote:
> this looks great, thank you.
> 
> lgtm
> 
> please add a tracking bug for this work. we should switch all of the enums
that
> used the old way to this

http://crbug.com/405532

[jam, 2014-08-20 18:28:53]

On 2014/08/20 05:52:02, Yaron wrote:
> While I like this change because the c++ code is cleaner, and overall there's
> less code so that's a good thing. However, I still worry about changes that
are
> made with no regard to the java layer. 

I don't understand this? How would this be different with this cl than before?
i.e. today when someone changes an enum value, they don't see anything about the
java files.

> Maybe I'm overly-paranoid? One way to
> ameliorate that (and handle multiple enums in a file) could be a syntactically
> significant comment before the enum. wdyt?

[Yaron, 2014-08-21 02:26:11]

On Wed, Aug 20, 2014 at 11:28 AM, <jam@chromium.org> wrote:

> On 2014/08/20 05:52:02, Yaron wrote:
>
>> While I like this change because the c++ code is cleaner, and overall
>> there's
>> less code so that's a good thing. However, I still worry about changes
>> that
>>
> are
>
>> made with no regard to the java layer.
>>
>
> I don't understand this? How would this be different with this cl than
> before?

i.e. today when someone changes an enum value, they don't see anything
> about the
> java files.


Right, but for enums which aren't just in a .h, it can be clear that
there's something else involved (i.e. why isn't it just in the .h file). My
concern is speculative, and what we had isn't a great solution so I'm still
fine with this.

>
>
>  Maybe I'm overly-paranoid? One way to
>> ameliorate that (and handle multiple enums in a file) could be a
>> syntactically
>> significant comment before the enum. wdyt?
>>
>
> https://codereview.chromium.org/484603004/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[mkosiba (inactive), 2014-09-02 16:00:57]

right. So I changed this to be 'comment-driven' since that makes it the most
obvious to the person editing the file.

The only downside of this approach is that I now need to run the script at gyp
time to figure out what the list of output files is supposed to be. This
shouldn't be a big problem unless we suddenly have 1000's of these generated
enums.

https://codereview.chromium.org/484603004/diff/1/build/android/gyp/enum_prepr...
File build/android/gyp/enum_preprocess.py (right):

https://codereview.chromium.org/484603004/diff/1/build/android/gyp/enum_prepr...
build/android/gyp/enum_preprocess.py:3: # Copyright 2013 The Chromium Authors.
All rights reserved.
On 2014/08/20 05:52:02, Yaron_OOO_8.28-8.29 wrote:
> 2014

Done.

https://codereview.chromium.org/484603004/diff/1/build/android/gyp/enum_prepr...
build/android/gyp/enum_preprocess.py:4: # Use of this source code is governed by
a BSD-style license that can be
On 2014/08/20 05:52:02, Yaron_OOO_8.28-8.29 wrote:
> Please add tests.

Done.

https://codereview.chromium.org/484603004/diff/1/build/android/gyp/enum_prepr...
build/android/gyp/enum_preprocess.py:45: raise Exception('Only one enum per
header file is supported.')
On 2014/08/20 05:52:02, Yaron_OOO_8.28-8.29 wrote:
> Are there really no examples of this?

Done.

https://codereview.chromium.org/484603004/diff/1/build/android/gyp/enum_prepr...
build/android/gyp/enum_preprocess.py:138: if options.stamp:
On 2014/08/20 05:52:02, Yaron_OOO_8.28-8.29 wrote:
> We have a definite output file. Why is a stamp necessary?

Done.

https://codereview.chromium.org/484603004/diff/1/build/android/java_cpp_enum....
File build/android/java_cpp_enum.gypi (right):

https://codereview.chromium.org/484603004/diff/1/build/android/java_cpp_enum....
build/android/java_cpp_enum.gypi:6: # to generate Java source files from
templates that are processed
On 2014/08/20 05:52:02, Yaron_OOO_8.28-8.29 wrote:
> Could use a little more description as it's more strict thant hat.

Done.

https://codereview.chromium.org/484603004/diff/1/build/android/java_cpp_enum....
build/android/java_cpp_enum.gypi:11: #    'target_name':
'android_net_java_constants',
On 2014/08/20 05:52:02, Yaron_OOO_8.28-8.29 wrote:
> The first example of this, isn't using this structure of name. Consider
> revising.

Done.

https://codereview.chromium.org/484603004/diff/1/build/android/java_cpp_enum....
build/android/java_cpp_enum.gypi:17: #      'package_name': 'org/chromium/ui',
On 2014/08/20 05:52:02, Yaron_OOO_8.28-8.29 wrote:
> Maybe this is copied from elsewhere, but this should be org/chromium/net to be
> consitent.

Done.

https://codereview.chromium.org/484603004/diff/1/ui/gfx/android/java_bitmap.h
File ui/gfx/android/java_bitmap.h (right):

https://codereview.chromium.org/484603004/diff/1/ui/gfx/android/java_bitmap.h...
ui/gfx/android/java_bitmap.h:19: enum BitmapConfig {
On 2014/08/20 05:52:02, Yaron_OOO_8.28-8.29 wrote:
> Shouldn't this be BitmapFormat? I guess that's orthogonal to this change.

Done.

[Yaron, 2014-09-03 01:41:05]

lgtm

[mkosiba (inactive), 2014-09-03 08:44:03]

The CQ bit was checked by mkosiba@chromium.org

[commit-bot: I haz the power, 2014-09-03 08:45:16]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mkosiba@chromium.org/484603004/20001

[commit-bot: I haz the power, 2014-09-03 10:45:34]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_aosp on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...)
  android_chromium_gn_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  ios_rel_device_ninja on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  win_chromium_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[commit-bot: I haz the power, 2014-09-03 10:50:11]

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

[commit-bot: I haz the power, 2014-09-03 10:50:12]

Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)

[mkosiba (inactive), 2014-09-03 16:27:22]

mkosiba@chromium.org changed reviewers:
+ brettw@chromium.org, skyostil@chromium.org, tedchoc@chromium.org

[mkosiba (inactive), 2014-09-03 16:27:23]

I had to make a couple of minor changes to support gn. This is my first gn
patch, so I have no idea whether my approach is good (it does work though).

brettw@chromium.org: Please review gn changes. 
skyostil@chromium.org: Please review ui/gfx
tedchoc: Please review the rest of ui/

https://codereview.chromium.org/484603004/diff/40001/build/config/android/rul...
File build/config/android/rules.gni (right):

https://codereview.chromium.org/484603004/diff/40001/build/config/android/rul...
build/config/android/rules.gni:302: outputs += [gen_dir + "/" + output]
I couldn't figure out how to make rebase_path work here. I kept getting errors
like:

> The given file should be in the output directory. ...
> I interpreted this as:
>
gen/ui/android/window_open_disposition_srcjar__generate_enum/enums/org/chromium/ui/WindowOpenDisposition.java".

[Sami, 2014-09-03 19:04:59]

Cool, ui/gfx lgtm with nits.

https://codereview.chromium.org/484603004/diff/40001/ui/gfx/android/java_bitm...
File ui/gfx/android/java_bitmap.cc (right):

https://codereview.chromium.org/484603004/diff/40001/ui/gfx/android/java_bitm...
ui/gfx/android/java_bitmap.cc:1: // Copyright (c) 2013 The Chromium Authors. All
rights reserved.
Intentional? It's 2014 by the way :)

https://codereview.chromium.org/484603004/diff/40001/ui/gfx/android/java_bitm...
File ui/gfx/android/java_bitmap.h (right):

https://codereview.chromium.org/484603004/diff/40001/ui/gfx/android/java_bitm...
ui/gfx/android/java_bitmap.h:16: // Define Bitmap Config values like
BITMAP_CONFIG_ARGB_8888 in a
nit: Update the example in the comment. Or maybe this part of the comment is
redundant now.

[Ted C, 2014-09-03 19:37:36]

On 2014/09/03 19:04:59, Sami wrote:
> Cool, ui/gfx lgtm with nits.
> 
>
https://codereview.chromium.org/484603004/diff/40001/ui/gfx/android/java_bitm...
> File ui/gfx/android/java_bitmap.cc (right):
> 
>
https://codereview.chromium.org/484603004/diff/40001/ui/gfx/android/java_bitm...
> ui/gfx/android/java_bitmap.cc:1: // Copyright (c) 2013 The Chromium Authors.
All
> rights reserved.
> Intentional? It's 2014 by the way :)
> 
>
https://codereview.chromium.org/484603004/diff/40001/ui/gfx/android/java_bitm...
> File ui/gfx/android/java_bitmap.h (right):
> 
>
https://codereview.chromium.org/484603004/diff/40001/ui/gfx/android/java_bitm...
> ui/gfx/android/java_bitmap.h:16: // Define Bitmap Config values like
> BITMAP_CONFIG_ARGB_8888 in a
> nit: Update the example in the comment. Or maybe this part of the comment is
> redundant now.

ui/ - lgtm

[brettw, 2014-09-03 21:23:39]

https://codereview.chromium.org/484603004/diff/40001/build/config/android/rul...
File build/config/android/rules.gni (right):

https://codereview.chromium.org/484603004/diff/40001/build/config/android/rul...
build/config/android/rules.gni:302: outputs += [gen_dir + "/" + output]
The thing that reads the outputs variable will interpret the paths to be
relative to the current directory. You can do that or make them absolute. I'd
do:

  rebase_path(invoker.outputs, ".", target_gen_dir)

which takes a file relative to target_gen_dir (which is what your docs say this
is) to be relative to ".".

[mkosiba (inactive), 2014-09-04 11:31:22]

https://codereview.chromium.org/484603004/diff/40001/build/config/android/rul...
File build/config/android/rules.gni (right):

https://codereview.chromium.org/484603004/diff/40001/build/config/android/rul...
build/config/android/rules.gni:302: outputs += [gen_dir + "/" + output]
On 2014/09/03 21:23:38, brettw wrote:
> The thing that reads the outputs variable will interpret the paths to be
> relative to the current directory. You can do that or make them absolute. I'd
> do:
> 
>   rebase_path(invoker.outputs, ".", target_gen_dir)
> 
> which takes a file relative to target_gen_dir (which is what your docs say
this
> is) to be relative to ".". 

That just changes the error to:

  The given file should be in the output directory. Normally you would specify
  "$target_out_dir/foo" or "$target_gen_dir/foo". I interpreted this as
 
"../../out/Debug/gen/ui/android/window_open_disposition_srcjar__generate_enum/enums/org/chromium/ui/WindowOpenDisposition.java"

https://codereview.chromium.org/484603004/diff/40001/ui/gfx/android/java_bitm...
File ui/gfx/android/java_bitmap.cc (right):

https://codereview.chromium.org/484603004/diff/40001/ui/gfx/android/java_bitm...
ui/gfx/android/java_bitmap.cc:1: // Copyright (c) 2013 The Chromium Authors. All
rights reserved.
On 2014/09/03 19:04:58, Sami wrote:
> Intentional? It's 2014 by the way :) 

Done.

https://codereview.chromium.org/484603004/diff/40001/ui/gfx/android/java_bitm...
File ui/gfx/android/java_bitmap.h (right):

https://codereview.chromium.org/484603004/diff/40001/ui/gfx/android/java_bitm...
ui/gfx/android/java_bitmap.h:16: // Define Bitmap Config values like
BITMAP_CONFIG_ARGB_8888 in a
On 2014/09/03 19:04:59, Sami wrote:
> nit: Update the example in the comment. Or maybe this part of the comment is
> redundant now.

ok, I assumed the comment meant these are supposed to match the
android.graphics.Bitmap.Config values but that's not the case (we have functions
to convert from one to the other) so it's probably just a stale comment from the
times we didn't have any auto-generators.

[mkosiba (inactive), 2014-09-08 08:51:53]

ping?

[brettw, 2014-09-08 19:56:15]

https://codereview.chromium.org/484603004/diff/40001/build/config/android/rul...
File build/config/android/rules.gni (right):

https://codereview.chromium.org/484603004/diff/40001/build/config/android/rul...
build/config/android/rules.gni:302: outputs += [gen_dir + "/" + output]
Oh, sorry. The checker doesn't know about relative paths. I made a note to fix
this.

You can wrap that call to get_path_info(rebase_path(....call from above...),
"abspath") to convert them to be absolute.

[mkosiba (inactive), 2014-09-09 09:00:12]

https://codereview.chromium.org/484603004/diff/40001/build/config/android/rul...
File build/config/android/rules.gni (right):

https://codereview.chromium.org/484603004/diff/40001/build/config/android/rul...
build/config/android/rules.gni:302: outputs += [gen_dir + "/" + output]
On 2014/09/08 19:56:15, brettw wrote:
> Oh, sorry. The checker doesn't know about relative paths. I made a note to fix
> this.
> 
> You can wrap that call to get_path_info(rebase_path(....call from above...),
> "abspath") to convert them to be absolute.

Ok, this seems to work although I could have sworn I'd tried that before and it
didn't....

[mkosiba (inactive), 2014-09-09 09:00:55]

mkosiba@chromium.org changed reviewers:
+ sky@chromium.org

[mkosiba (inactive), 2014-09-09 09:00:56]

sky@chromium.org: Please rubber-stamp ui/base

[sky, 2014-09-09 15:48:17]

LGTM

[mkosiba (inactive), 2014-09-10 09:35:03]

brett - I'm going to land this assuming you have no other feedback on the .gn
changes. If that's not the case I'll gladly address them in a followup CL.

[mkosiba (inactive), 2014-09-10 09:35:16]

The CQ bit was checked by mkosiba@chromium.org

[commit-bot: I haz the power, 2014-09-10 09:37:58]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mkosiba@chromium.org/484603004/80001

[commit-bot: I haz the power, 2014-09-10 09:43:47]

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

[commit-bot: I haz the power, 2014-09-10 09:43:48]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[mkosiba (inactive), 2014-09-10 09:53:39]

The CQ bit was checked by mkosiba@chromium.org

[commit-bot: I haz the power, 2014-09-10 09:54:04]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mkosiba@chromium.org/484603004/80001

[commit-bot: I haz the power, 2014-09-10 10:19:27]

Committed patchset #5 (id:80001) as e4601e00b4574b60405f419448d78d47eff92e7b

[commit-bot: I haz the power, 2014-09-10 10:56:24]

Patchset 5 (id:??) landed as
https://crrev.com/9610607d5adf4afd2ad821c94434cd749041268a
Cr-Commit-Position: refs/heads/master@{#294153}

[brettw, 2014-09-11 17:37:02]

lgtm