C++->Java string constants auto-generator

C++->Java string constants auto-generator

This adds a script for generating Java constants for C++ strings.

BUG=526932

[lshang, 2016-02-08 10:10:41]

Patchset #6 (id:100001) has been deleted

[lshang, 2016-02-08 10:10:52]

Patchset #6 (id:120001) has been deleted

[lshang, 2016-02-08 10:11:16]

Patchset #6 (id:140001) has been deleted

[lshang, 2016-02-08 10:16:10]

lshang@chromium.org changed reviewers:
+ raymes@chromium.org

[lshang, 2016-02-08 10:16:10]

PTAL thanks!

[raymes, 2016-02-16 00:58:21]

https://codereview.chromium.org/1664113002/diff/80001/build/android/gyp/java_...
File build/android/gyp/java_cpp_string.py (right):

https://codereview.chromium.org/1664113002/diff/80001/build/android/gyp/java_...
build/android/gyp/java_cpp_string.py:18: class StringDefinition(object):
I guess this is actually a list of strings. So perhaps we should call it
StringListDefinition?

https://codereview.chromium.org/1664113002/diff/80001/build/android/gyp/java_...
build/android/gyp/java_cpp_string.py:63: class HeaderParser(object):
This is no longer parsing a header file, so perhaps we should name it
differently

https://codereview.chromium.org/1664113002/diff/80001/build/android/gyp/java_...
build/android/gyp/java_cpp_string.py:68:
r'^\s*const\s*char\s*(\w+)\[\]\s*\=\s*([^,\n]+)?"([^"]*)";')
We may not need ([^,\n]+)?

https://codereview.chromium.org/1664113002/diff/80001/build/android/gyp/java_...
build/android/gyp/java_cpp_string.py:90: self._current_key = None
maybe call this:
_current_multiline_string_key

https://codereview.chromium.org/1664113002/diff/80001/build/android/gyp/java_...
build/android/gyp/java_cpp_string.py:131: self._in_comment = True
Could we just ignore multi line comments, as in the enum version?

https://codereview.chromium.org/1664113002/diff/80001/build/android/gyp/java_...
build/android/gyp/java_cpp_string.py:151: string_key = self._current_key
Maybe a slightly different approach would be safer:

if self._current_key:
  string_multi_line_end = HeaderParser.multi_string_line_end_re.match(line)
  if (!string_multi_line_end)
    raise Exception('...')
  ...

You would need to move that higher up in the function though

https://codereview.chromium.org/1664113002/diff/80001/build/android/gyp/java_...
build/android/gyp/java_cpp_string.py:187: return
os.sep.join(script_components[build_index:])
Hmm I'm not sure I understand why this changed?

[lshang, 2016-02-16 23:00:20]

Raymes, could you PTALA? Thanks!

https://codereview.chromium.org/1664113002/diff/80001/build/android/gyp/java_...
File build/android/gyp/java_cpp_string.py (right):

https://codereview.chromium.org/1664113002/diff/80001/build/android/gyp/java_...
build/android/gyp/java_cpp_string.py:18: class StringDefinition(object):
On 2016/02/16 00:58:21, raymes wrote:
> I guess this is actually a list of strings. So perhaps we should call it
> StringListDefinition?

Done.

https://codereview.chromium.org/1664113002/diff/80001/build/android/gyp/java_...
build/android/gyp/java_cpp_string.py:63: class HeaderParser(object):
On 2016/02/16 00:58:20, raymes wrote:
> This is no longer parsing a header file, so perhaps we should name it
> differently

Done.

https://codereview.chromium.org/1664113002/diff/80001/build/android/gyp/java_...
build/android/gyp/java_cpp_string.py:68:
r'^\s*const\s*char\s*(\w+)\[\]\s*\=\s*([^,\n]+)?"([^"]*)";')
On 2016/02/16 00:58:21, raymes wrote:
> We may not need ([^,\n]+)?

Done.

https://codereview.chromium.org/1664113002/diff/80001/build/android/gyp/java_...
build/android/gyp/java_cpp_string.py:90: self._current_key = None
On 2016/02/16 00:58:21, raymes wrote:
> maybe call this:
> _current_multiline_string_key

Done.

https://codereview.chromium.org/1664113002/diff/80001/build/android/gyp/java_...
build/android/gyp/java_cpp_string.py:131: self._in_comment = True
On 2016/02/16 00:58:21, raymes wrote:
> Could we just ignore multi line comments, as in the enum version?

Done.

https://codereview.chromium.org/1664113002/diff/80001/build/android/gyp/java_...
build/android/gyp/java_cpp_string.py:151: string_key = self._current_key
On 2016/02/16 00:58:21, raymes wrote:
> Maybe a slightly different approach would be safer:
> 
> if self._current_key:
>   string_multi_line_end = HeaderParser.multi_string_line_end_re.match(line)
>   if (!string_multi_line_end)
>     raise Exception('...')
>   ...
> 
> You would need to move that higher up in the function though

Done.

https://codereview.chromium.org/1664113002/diff/80001/build/android/gyp/java_...
build/android/gyp/java_cpp_string.py:187: return
os.sep.join(script_components[build_index:])
On 2016/02/16 00:58:21, raymes wrote:
> Hmm I'm not sure I understand why this changed?

Done.
java_cpp_enum.py changed this on 2016-01-12, but I referred to the old version
of it. Should keep it up-to-date.

[raymes, 2016-02-18 02:22:32]

https://codereview.chromium.org/1664113002/diff/220001/build/android/gyp/java...
File build/android/gyp/java_cpp_string.py (right):

https://codereview.chromium.org/1664113002/diff/220001/build/android/gyp/java...
build/android/gyp/java_cpp_string.py:110: if self._current_multiline_string_key:
Perhaps make a comment here about what's going on:
# If we're continuing the start of a string definition, we need to match 
# the end part of the string defintiion.

https://codereview.chromium.org/1664113002/diff/220001/build/android/gyp/java...
build/android/gyp/java_cpp_string.py:115: self._path)
nit: indentation

https://codereview.chromium.org/1664113002/diff/220001/build/android/gyp/java...
build/android/gyp/java_cpp_string.py:116: else:
nit: the else isn't needed here

https://codereview.chromium.org/1664113002/diff/220001/build/android/gyp/java...
build/android/gyp/java_cpp_string.py:131: elif
multi_line_generator_directive_start:
nit: elif->if

https://codereview.chromium.org/1664113002/diff/220001/build/android/gyp/java...
build/android/gyp/java_cpp_string.py:152: elif single_string_line:
nit: elif->if

https://codereview.chromium.org/1664113002/diff/220001/build/android/gyp/java...
File build/android/gyp/java_cpp_string_tests.py (right):

https://codereview.chromium.org/1664113002/diff/220001/build/android/gyp/java...
build/android/gyp/java_cpp_string_tests.py:87: def
testParseThrowsOnEmptyClassName(self):
nit: testParseThrowsOnMissingClassDirective

https://codereview.chromium.org/1664113002/diff/220001/build/android/gyp/java...
build/android/gyp/java_cpp_string_tests.py:117: def
testParseMultiLineDirectiveWithOtherDirective(self):
nit: testParseTwoMultilineDirectives ?

https://codereview.chromium.org/1664113002/diff/220001/build/android/gyp/java...
build/android/gyp/java_cpp_string_tests.py:169: 
I can think of a few more cases:
1) There are strings defined before the directive, those should be ignored (or
maybe we should just throw an exception in that case).
2) There is a multiline string where the second line is not the end of the
string. This should throw an exception
3) Test that comments are ignored, even if they are interleaved with strings

[lshang, 2016-02-29 07:45:03]

https://codereview.chromium.org/1664113002/diff/220001/build/android/gyp/java...
File build/android/gyp/java_cpp_string.py (right):

https://codereview.chromium.org/1664113002/diff/220001/build/android/gyp/java...
build/android/gyp/java_cpp_string.py:110: if self._current_multiline_string_key:
On 2016/02/18 02:22:32, raymes wrote:
> Perhaps make a comment here about what's going on:
> # If we're continuing the start of a string definition, we need to match 
> # the end part of the string defintiion.

Done.

https://codereview.chromium.org/1664113002/diff/220001/build/android/gyp/java...
build/android/gyp/java_cpp_string.py:115: self._path)
On 2016/02/18 02:22:32, raymes wrote:
> nit: indentation

Done.

https://codereview.chromium.org/1664113002/diff/220001/build/android/gyp/java...
build/android/gyp/java_cpp_string.py:116: else:
On 2016/02/18 02:22:32, raymes wrote:
> nit: the else isn't needed here

Done.

https://codereview.chromium.org/1664113002/diff/220001/build/android/gyp/java...
build/android/gyp/java_cpp_string.py:131: elif
multi_line_generator_directive_start:
On 2016/02/18 02:22:32, raymes wrote:
> nit: elif->if

Done.

https://codereview.chromium.org/1664113002/diff/220001/build/android/gyp/java...
build/android/gyp/java_cpp_string.py:152: elif single_string_line:
On 2016/02/18 02:22:31, raymes wrote:
> nit: elif->if

Done.

https://codereview.chromium.org/1664113002/diff/220001/build/android/gyp/java...
File build/android/gyp/java_cpp_string_tests.py (right):

https://codereview.chromium.org/1664113002/diff/220001/build/android/gyp/java...
build/android/gyp/java_cpp_string_tests.py:87: def
testParseThrowsOnEmptyClassName(self):
On 2016/02/18 02:22:32, raymes wrote:
> nit: testParseThrowsOnMissingClassDirective

Done.

https://codereview.chromium.org/1664113002/diff/220001/build/android/gyp/java...
build/android/gyp/java_cpp_string_tests.py:117: def
testParseMultiLineDirectiveWithOtherDirective(self):
On 2016/02/18 02:22:32, raymes wrote:
> nit: testParseTwoMultilineDirectives ?

Done.

https://codereview.chromium.org/1664113002/diff/220001/build/android/gyp/java...
build/android/gyp/java_cpp_string_tests.py:169: 
On 2016/02/18 02:22:32, raymes wrote:
> I can think of a few more cases:
> 1) There are strings defined before the directive, those should be ignored (or
> maybe we should just throw an exception in that case).
> 2) There is a multiline string where the second line is not the end of the
> string. This should throw an exception
> 3) Test that comments are ignored, even if they are interleaved with strings

Done.

[lshang, 2016-02-29 07:47:08]

Hi Raymes, I made some changes according to your comments. I also removed gn/gyp
changes relating to ContentSettingsType, as we discussed today. So this
generator is not used on ContentSettingsType for now. :-)

[raymes, 2016-03-01 02:49:35]

lgtm

[raymes, 2016-03-01 02:51:50]

https://codereview.chromium.org/1664113002/diff/260001/build/android/gyp/java...
File build/android/gyp/java_cpp_string_tests.py (right):

https://codereview.chromium.org/1664113002/diff/260001/build/android/gyp/java...
build/android/gyp/java_cpp_string_tests.py:30: entries=[('L1', 'hello'), ('L2',
'world')])
nit: indentation

https://codereview.chromium.org/1664113002/diff/260001/build/android/gyp/java...
build/android/gyp/java_cpp_string_tests.py:51: def testIgnoreComments(self):
nit: this is really an end-to-end test, so you could call it,
"testParseAndGenerate"

You could also have a separate small test for parsing which parses something
with comments and checks that parsing happens successfully.

[lshang, 2016-03-01 03:49:37]

lshang@chromium.org changed reviewers:
+ agrieve@chromium.org

[lshang, 2016-03-01 03:49:38]

agrieve@, PTAL thanks!:-)

https://codereview.chromium.org/1664113002/diff/260001/build/android/gyp/java...
File build/android/gyp/java_cpp_string_tests.py (right):

https://codereview.chromium.org/1664113002/diff/260001/build/android/gyp/java...
build/android/gyp/java_cpp_string_tests.py:30: entries=[('L1', 'hello'), ('L2',
'world')])
On 2016/03/01 02:51:49, raymes wrote:
> nit: indentation

Done.

https://codereview.chromium.org/1664113002/diff/260001/build/android/gyp/java...
build/android/gyp/java_cpp_string_tests.py:51: def testIgnoreComments(self):
On 2016/03/01 02:51:49, raymes wrote:
> nit: this is really an end-to-end test, so you could call it,
> "testParseAndGenerate"
> 
> You could also have a separate small test for parsing which parses something
> with comments and checks that parsing happens successfully.

Done.

[agrieve, 2016-03-02 02:59:30]

lgtm

https://codereview.chromium.org/1664113002/diff/280001/build/android/gyp/java...
File build/android/gyp/java_cpp_string.py (right):

https://codereview.chromium.org/1664113002/diff/280001/build/android/gyp/java...
build/android/gyp/java_cpp_string.py:193: #for string_definition in
string_definitions:
nit: delete

[lshang, 2016-07-11 00:03:52]

Description was changed from

==========
C++->Java string constants auto-generator

This adds a script for generating Java constants for C++ strings.

BUG=526932
==========

to

==========
C++->Java string constants auto-generator

This adds a script for generating Java constants for C++ strings.

BUG=526932
==========