Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
291903003:
Generate java bindings for constants. (Closed)
|
Created:
6 years, 7 months ago by qsr Modified:
6 years, 6 months ago CC:
chromium-reviews, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org, abarth-chromium, Aaron Boodman, darin (slow to review), ben+mojo_chromium.org, mlamouri (slow - plz ping) Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionGenerate java bindings for constants.
This CL is the first CL introducing java bindings. It only generates
constants.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=273307
Patch Set 1 #
Total comments: 15
Patch Set 2 : Follow review #Patch Set 3 : Regenerate file #Patch Set 4 : Add private constructor to constants class. #
Total comments: 42
Patch Set 5 : Follow review #
Total comments: 14
Patch Set 6 : Follow review #Patch Set 7 : Fix component cutting. #Patch Set 8 : Fix RE #
Total comments: 2
Patch Set 9 : Follow review #Patch Set 10 : Fix dir creation? #Messages
Total messages: 51 (0 generated)
I cut the generation CL in part. This part only generate top level constants. I didn't cut the python file though. If that's fine with you.
+ Tommy for question in SampleServiceConstants.java:11 Thanks for splitting this up Benjamin. I've got a couple of high level questions before I dive into the details. https://chromiumcodereview.appspot.com/291903003/diff/1/mojo/android/javatest... File mojo/android/javatests/src/org/chromium/mojo/bindings/BindingsTest.java (right): https://chromiumcodereview.appspot.com/291903003/diff/1/mojo/android/javatest... mojo/android/javatests/src/org/chromium/mojo/bindings/BindingsTest.java:23: assertEquals(3, SampleServiceConstants.THREE); Test it is of type Byte.TYPE, and is constant too? https://chromiumcodereview.appspot.com/291903003/diff/1/mojo/not-to-commit/ja... File mojo/not-to-commit/java_mojo/mojo_public_test_interfaces/src/org/chromium/mojo/bindings/test/sample/SampleServiceConstants.java (right): https://chromiumcodereview.appspot.com/291903003/diff/1/mojo/not-to-commit/ja... mojo/not-to-commit/java_mojo/mojo_public_test_interfaces/src/org/chromium/mojo/bindings/test/sample/SampleServiceConstants.java:11: package org.chromium.mojo.bindings.test.sample; I don't really like the idea of needing a separate class for constants within a mojom file. I can see from the previous CL there was discussion over whether a mojo module is a Class or package, and if it is a package I realize we need a separate class for outer constants. I chatted with Marcus this morning as to whether mojo modules should be packages or classes, and there seem to be pros and cons to either side - as I see it: package: + clearer separation of implementation for individual structs / interfaces + similar to CPP bindings where a module is a namespace - requires Constants class for outer constants in mojom - if inner mojom structs/interfaces have the same name, then classes which use these generated bindings end up having to fully specify the package/class to differentiate (e.g., org.chromium.mojo.bindings.test.sample.Foo vs org.chromium.mojo.bindings.test.other_sample.Foo). class: + no need for outer Constants class + inner mojom structs/interfaces can always be differentated via the outer class name - e.g., Sample.Foo v.s. OtherSample.Foo) - potential for massive genrated implementation classes which make it difficult to read through generated code (thought this seems similar to the CPP binding generator). - semantically it seems to make more sense for a mojo module to correspond a Java package I can't really see a clear advantage one way or another, so I'm kindof agnostic to whichever is choosen, but wanted to start the debate here in case there is any better ideas we haven't thought of. Tommy: Marcus mentions you did a lot of work with the protobuf generators for Sync - did you encounter any similar issues and how are they solved in protobuf? https://chromiumcodereview.appspot.com/291903003/diff/1/mojo/not-to-commit/ja... mojo/not-to-commit/java_mojo/mojo_public_test_interfaces/src/org/chromium/mojo/bindings/test/sample/SampleServiceConstants.java:13: public final class SampleServiceConstants { I would just call this "Constants" and rely on the package name (or outer class if we go that route) to differentiate. https://chromiumcodereview.appspot.com/291903003/diff/1/mojo/not-to-commit/ja... mojo/not-to-commit/java_mojo/mojo_public_test_interfaces/src/org/chromium/mojo/bindings/test/sample/SampleServiceConstants.java:18: } Does this need to be a static block, or could it simply be an assignment? https://chromiumcodereview.appspot.com/291903003/diff/1/mojo/public/tools/bin... File mojo/public/tools/bindings/generators/mojom_java_generator.py (right): https://chromiumcodereview.appspot.com/291903003/diff/1/mojo/public/tools/bin... mojo/public/tools/bindings/generators/mojom_java_generator.py:40: _spec_to_decryption_method = { should this be _spec_to_encode_method? https://chromiumcodereview.appspot.com/291903003/diff/1/mojo/public/tools/bin... File mojo/public/tools/bindings/mojom_bindings_generator.gypi (right): https://chromiumcodereview.appspot.com/291903003/diff/1/mojo/public/tools/bin... mojo/public/tools/bindings/mojom_bindings_generator.gypi:8: 'rule_name': 'Generate C++ source files from mojom files', nit - "Generate C++, JS and Java source files from mojom files" ? ;)
https://chromiumcodereview.appspot.com/291903003/diff/1/mojo/android/javatest... File mojo/android/javatests/src/org/chromium/mojo/bindings/BindingsTest.java (right): https://chromiumcodereview.appspot.com/291903003/diff/1/mojo/android/javatest... mojo/android/javatests/src/org/chromium/mojo/bindings/BindingsTest.java:23: assertEquals(3, SampleServiceConstants.THREE); On 2014/05/20 12:43:29, rmcilroy wrote: > Test it is of type Byte.TYPE, and is constant too? Done. https://chromiumcodereview.appspot.com/291903003/diff/1/mojo/not-to-commit/ja... File mojo/not-to-commit/java_mojo/mojo_public_test_interfaces/src/org/chromium/mojo/bindings/test/sample/SampleServiceConstants.java (right): https://chromiumcodereview.appspot.com/291903003/diff/1/mojo/not-to-commit/ja... mojo/not-to-commit/java_mojo/mojo_public_test_interfaces/src/org/chromium/mojo/bindings/test/sample/SampleServiceConstants.java:11: package org.chromium.mojo.bindings.test.sample; Well, I do not have a strong opinion either. I started with one file per mojom, because it seemed to be that it would be easier to identify generated code that way. Marcus seemed to be in favor of one class per entity, so I changed. https://chromiumcodereview.appspot.com/291903003/diff/1/mojo/not-to-commit/ja... mojo/not-to-commit/java_mojo/mojo_public_test_interfaces/src/org/chromium/mojo/bindings/test/sample/SampleServiceConstants.java:13: public final class SampleServiceConstants { On 2014/05/20 12:43:29, rmcilroy wrote: > I would just call this "Constants" and rely on the package name (or outer class > if we go that route) to differentiate. Two mojom can end in the same java package (they can have the same module name) by design. So I need to use the mojom filename there. https://chromiumcodereview.appspot.com/291903003/diff/1/mojo/not-to-commit/ja... mojo/not-to-commit/java_mojo/mojo_public_test_interfaces/src/org/chromium/mojo/bindings/test/sample/SampleServiceConstants.java:18: } On 2014/05/20 12:43:29, rmcilroy wrote: > Does this need to be a static block, or could it simply be an assignment? At the time, it needed to be a static block. I moved from it since, will update. https://chromiumcodereview.appspot.com/291903003/diff/1/mojo/public/tools/bin... File mojo/public/tools/bindings/generators/mojom_java_generator.py (right): https://chromiumcodereview.appspot.com/291903003/diff/1/mojo/public/tools/bin... mojo/public/tools/bindings/generators/mojom_java_generator.py:40: _spec_to_decryption_method = { On 2014/05/20 12:43:29, rmcilroy wrote: > should this be _spec_to_encode_method? Hum, nope, decode is the right name here, and my method below is just wrong. Thanks. https://chromiumcodereview.appspot.com/291903003/diff/1/mojo/public/tools/bin... File mojo/public/tools/bindings/mojom_bindings_generator.gypi (right): https://chromiumcodereview.appspot.com/291903003/diff/1/mojo/public/tools/bin... mojo/public/tools/bindings/mojom_bindings_generator.gypi:8: 'rule_name': 'Generate C++ source files from mojom files', On 2014/05/20 12:43:29, rmcilroy wrote: > nit - "Generate C++, JS and Java source files from mojom files" ? ;) Done.
Any chance we can find a way to advance on this?
https://chromiumcodereview.appspot.com/291903003/diff/1/mojo/not-to-commit/ja... File mojo/not-to-commit/java_mojo/mojo_public_test_interfaces/src/org/chromium/mojo/bindings/test/sample/SampleServiceConstants.java (right): https://chromiumcodereview.appspot.com/291903003/diff/1/mojo/not-to-commit/ja... mojo/not-to-commit/java_mojo/mojo_public_test_interfaces/src/org/chromium/mojo/bindings/test/sample/SampleServiceConstants.java:11: package org.chromium.mojo.bindings.test.sample; On 2014/05/20 13:12:45, qsr wrote: > Well, I do not have a strong opinion either. I started with one file per mojom, > because it seemed to be that it would be easier to identify generated code that > way. Marcus seemed to be in favor of one class per entity, so I changed. Given what you mention below with different mojom files being able define the same mojo module, then I am now definitely in support of each mojo module being a package rather than a Class. I still don't like the idea of having a separate Constants file, however I've sent an email to the mojo mailing list asking whether we even need the concept of module level constants - they don't seem to be used anywhere currently and if they aren't required then this would avoid this issue entirely. https://chromiumcodereview.appspot.com/291903003/diff/1/mojo/not-to-commit/ja... mojo/not-to-commit/java_mojo/mojo_public_test_interfaces/src/org/chromium/mojo/bindings/test/sample/SampleServiceConstants.java:18: } On 2014/05/20 13:12:45, qsr wrote: > On 2014/05/20 12:43:29, rmcilroy wrote: > > Does this need to be a static block, or could it simply be an assignment? > > At the time, it needed to be a static block. I moved from it since, will > update. Still seems to be a static block in the latest patch set. https://chromiumcodereview.appspot.com/291903003/diff/1/mojo/public/tools/bin... File mojo/public/tools/bindings/generators/mojom_java_generator.py (right): https://chromiumcodereview.appspot.com/291903003/diff/1/mojo/public/tools/bin... mojo/public/tools/bindings/generators/mojom_java_generator.py:40: _spec_to_decryption_method = { On 2014/05/20 13:12:45, qsr wrote: > On 2014/05/20 12:43:29, rmcilroy wrote: > > should this be _spec_to_encode_method? > > Hum, nope, decode is the right name here, and my method below is just wrong. > Thanks. Well "decryption" wasn't the right name either ;).
> Still seems to be a static block in the latest patch set. Forgot to regenerate the file before uploading. Should be correct now. > > https://chromiumcodereview.appspot.com/291903003/diff/1/mojo/public/tools/bin... > File mojo/public/tools/bindings/generators/mojom_java_generator.py (right): > > https://chromiumcodereview.appspot.com/291903003/diff/1/mojo/public/tools/bin... > mojo/public/tools/bindings/generators/mojom_java_generator.py:40: > _spec_to_decryption_method = { > On 2014/05/20 13:12:45, qsr wrote: > > On 2014/05/20 12:43:29, rmcilroy wrote: > > > should this be _spec_to_encode_method? > > > > Hum, nope, decode is the right name here, and my method below is just wrong. > > Thanks. > > Well "decryption" wasn't the right name either ;). True enough.
On 2014/05/22 15:52:07, rmcilroy wrote: > https://chromiumcodereview.appspot.com/291903003/diff/1/mojo/not-to-commit/ja... > File > mojo/not-to-commit/java_mojo/mojo_public_test_interfaces/src/org/chromium/mojo/bindings/test/sample/SampleServiceConstants.java > (right): > > https://chromiumcodereview.appspot.com/291903003/diff/1/mojo/not-to-commit/ja... > mojo/not-to-commit/java_mojo/mojo_public_test_interfaces/src/org/chromium/mojo/bindings/test/sample/SampleServiceConstants.java:11: > package org.chromium.mojo.bindings.test.sample; > On 2014/05/20 13:12:45, qsr wrote: > > Well, I do not have a strong opinion either. I started with one file per > mojom, > > because it seemed to be that it would be easier to identify generated code > that > > way. Marcus seemed to be in favor of one class per entity, so I changed. > > Given what you mention below with different mojom files being able define the > same mojo module, then I am now definitely in support of each mojo module being > a package rather than a Class. I still don't like the idea of having a separate > Constants file, however I've sent an email to the mojo mailing list asking > whether we even need the concept of module level constants - they don't seem to > be used anywhere currently and if they aren't required then this would avoid > this issue entirely. We started the discussion on the list, but if it is fine with you, I'd like we continue with this CL in the meantime. If constants are removed, we can remove this part, but on the other end, we seem to agree on what to generate in presence of constants, and I also think that: 1) They could be useful in the idl -> what if you need multiple field in a module across different structs to be initialized to the same value? 2) The definition of the idl should not be the common denominator of what is idiomatic in all target languages. > > https://chromiumcodereview.appspot.com/291903003/diff/1/mojo/not-to-commit/ja... > mojo/not-to-commit/java_mojo/mojo_public_test_interfaces/src/org/chromium/mojo/bindings/test/sample/SampleServiceConstants.java:18: > } > On 2014/05/20 13:12:45, qsr wrote: > > On 2014/05/20 12:43:29, rmcilroy wrote: > > > Does this need to be a static block, or could it simply be an assignment? > > > > At the time, it needed to be a static block. I moved from it since, will > > update. > > Still seems to be a static block in the latest patch set. > > https://chromiumcodereview.appspot.com/291903003/diff/1/mojo/public/tools/bin... > File mojo/public/tools/bindings/generators/mojom_java_generator.py (right): > > https://chromiumcodereview.appspot.com/291903003/diff/1/mojo/public/tools/bin... > mojo/public/tools/bindings/generators/mojom_java_generator.py:40: > _spec_to_decryption_method = { > On 2014/05/20 13:12:45, qsr wrote: > > On 2014/05/20 12:43:29, rmcilroy wrote: > > > should this be _spec_to_encode_method? > > > > Hum, nope, decode is the right name here, and my method below is just wrong. > > Thanks. > > Well "decryption" wasn't the right name either ;).
https://codereview.chromium.org/291903003/diff/60001/mojo/public/tools/bindin... File mojo/public/tools/bindings/generators/java_templates/constant_definition.tmpl (right): https://codereview.chromium.org/291903003/diff/60001/mojo/public/tools/bindin... mojo/public/tools/bindings/generators/java_templates/constant_definition.tmpl:4: public static final {{constant.kind|java_type}} {{constant|constant_name}} = {{build_default(module, constant.kind, constant.value)|indent(4)}}; Do we need build_default()? The cpp binding generator gets away withexpression_to_text(), I'm pretty sure we could too assuming a mojom const can't be anything other than a number or string literal? If not, could you add an examples to the the sample mojom files which use the other const types. https://codereview.chromium.org/291903003/diff/60001/mojo/public/tools/bindin... File mojo/public/tools/bindings/generators/java_templates/constants.java.tmpl (right): https://codereview.chromium.org/291903003/diff/60001/mojo/public/tools/bindin... mojo/public/tools/bindings/generators/java_templates/constants.java.tmpl:11: } nit - newline between constructor and end of class "}" (you could also just do "private {{main_entity}}() { }" on one line if you like. https://codereview.chromium.org/291903003/diff/60001/mojo/public/tools/bindin... File mojo/public/tools/bindings/generators/mojom_java_generator.py (right): https://codereview.chromium.org/291903003/diff/60001/mojo/public/tools/bindin... mojo/public/tools/bindings/generators/mojom_java_generator.py:16: GENERATOR_PREFIX = 'java' Is this needed - it doesn't seem to be referenced. https://codereview.chromium.org/291903003/diff/60001/mojo/public/tools/bindin... mojo/public/tools/bindings/generators/mojom_java_generator.py:70: def NamespaceToArray(namespace): I think this should be called PackageToArray(package): for Java https://codereview.chromium.org/291903003/diff/60001/mojo/public/tools/bindin... mojo/public/tools/bindings/generators/mojom_java_generator.py:73: def GetNameFromString(string): I would call this "UpperCamelCase" and move it with the other CamelCase, etc functions. https://codereview.chromium.org/291903003/diff/60001/mojo/public/tools/bindin... mojo/public/tools/bindings/generators/mojom_java_generator.py:76: def GetName(kind): I would get rid of this method - the name is confusing compared to GetNameForKind, and it is easy enough to just call UpperCamelCase(kind.name) where it is needed. https://codereview.chromium.org/291903003/diff/60001/mojo/public/tools/bindin... mojo/public/tools/bindings/generators/mojom_java_generator.py:84: assert package[0] == 'EXPRESSION' It's probably clearer to include "else" branches even if the "if" returns - e.g: if 'JavaPackage' in module.attributes: package = module.attributes['JavaPackage'] if isinstance(package, basestring): return package else: assert package[0] == 'EXPRESSION' assert len(package[1]) == 1 return package[1][0][1:-1].encode('string_escape') else: return "org.chromium.mojom." + module.namespace (also lots of other examples below) https://codereview.chromium.org/291903003/diff/60001/mojo/public/tools/bindin... mojo/public/tools/bindings/generators/mojom_java_generator.py:94: return "org.chromium.mojo.bindings.Struct" I think this would be clearer as: if not method: return "org.chromium.mojo.bindings.Struct" else if method.response_parameters: return "org.chromium.mojo.bindings.MessageWithRequestId" else: return "org.chromium.mojo.bindings.Message" https://codereview.chromium.org/291903003/diff/60001/mojo/public/tools/bindin... mojo/public/tools/bindings/generators/mojom_java_generator.py:96: def GetFlags(method, is_parameter): Could you remove this until it's used in a later CL please (I'm not sure how it would be used and would like to review it in context) https://codereview.chromium.org/291903003/diff/60001/mojo/public/tools/bindin... mojo/public/tools/bindings/generators/mojom_java_generator.py:103: def NewArray(kind, size): nit - move below GetMethodName https://codereview.chromium.org/291903003/diff/60001/mojo/public/tools/bindin... mojo/public/tools/bindings/generators/mojom_java_generator.py:108: def GetNameHierachy(kind): Could you make GetNameHierarchy an inner function of GetNameForKind - this seems to be the only place it's used. https://codereview.chromium.org/291903003/diff/60001/mojo/public/tools/bindin... mojo/public/tools/bindings/generators/mojom_java_generator.py:147: return GetPackage(token.module) + '.' + entity_value Could you please reduce this method to the bare minimum for the Const generation (I think "return token" should be enough). I would like to review it in context with the Enum,Struct,Interface generation CLs if possible. https://codereview.chromium.org/291903003/diff/60001/mojo/public/tools/bindin... mojo/public/tools/bindings/generators/mojom_java_generator.py:150: def ExpressionToText(value, module): This looks the same as the method in the CPP bindings generator - could it be pulled out into a separate utility class in the pylib directory? The CapitalizeFirst, CamelCase and UpperCaseConstant methods are also candidates to move to a utility class. https://codereview.chromium.org/291903003/diff/60001/mojo/public/tools/bindin... mojo/public/tools/bindings/generators/mojom_java_generator.py:174: return '_'.join([x.upper() for x in parts]) I think you could do this more easily with a regex. How about: def UpperCaseConstant(name): name = re.sub('(.)([A-Z][a-z]+)', r'\1_\2', name) # insert '_' between anything and a Title name (e.g, HTTPEntry2FooBar -> HTTP_Entry2_FooBar) name = re.sub('(.)([0-9])', r'\1_\2', name) # insert '_' between anything and a number (e.g, HTTP_Entry2_FooBar -> HTTP_Entry_2_FooBar) name = re.sub('([a-z0-9])([A-Z])', r'\1_\2', name) # insert '_' between lower or number blocks and start of upper blocks (e.g., HTTP_Entry_2_FooBar -> HTTP_Entry_2_Foo_Bar) return name.upper() https://codereview.chromium.org/291903003/diff/60001/mojo/public/tools/bindin... mojo/public/tools/bindings/generators/mojom_java_generator.py:177: return 'DEFAULT_' + UpperCaseConstant(GetName(field)) Please remove this until the CL where it is used. https://codereview.chromium.org/291903003/diff/60001/mojo/public/tools/bindin... mojo/public/tools/bindings/generators/mojom_java_generator.py:188: def IsPointerArrayKind(kind): Please remove IsPointerArrayKind, GetResponseStructFromMethod and GetStructFromMethod and GetMethodName until the CL where they are used. https://codereview.chromium.org/291903003/diff/60001/mojo/public/tools/bindin... mojo/public/tools/bindings/generators/mojom_java_generator.py:206: def GetConstantsMainEntityFullyQualifiedName(module): This doesn't seem to be used - can it be removed (or at least removed from this CL until it's needed)? https://codereview.chromium.org/291903003/diff/60001/mojo/public/tools/bindin... mojo/public/tools/bindings/generators/mojom_java_generator.py:211: return (GetNameFromString(module.path.split('/')[-1].rsplit('.', 1)[0]) + Please add a comment describing that this is building a name by extracting the mojom's filename and prepending it to Constants. If you could also add the annotation support as discussed that would be great (feel free to add it in a separate CL if you prefer though). https://codereview.chromium.org/291903003/diff/60001/mojo/public/tools/bindin... mojo/public/tools/bindings/generators/mojom_java_generator.py:246: "method_structs": self.GetStructsFromMethods(), I can't see any uses of "method_structs" either here or in the mega CL - can it be removed?
https://chromiumcodereview.appspot.com/291903003/diff/60001/mojo/public/tools... File mojo/public/tools/bindings/generators/java_templates/constant_definition.tmpl (right): https://chromiumcodereview.appspot.com/291903003/diff/60001/mojo/public/tools... mojo/public/tools/bindings/generators/java_templates/constant_definition.tmpl:4: public static final {{constant.kind|java_type}} {{constant|constant_name}} = {{build_default(module, constant.kind, constant.value)|indent(4)}}; On 2014/05/25 22:55:10, rmcilroy wrote: > Do we need build_default()? The cpp binding generator gets away > withexpression_to_text(), I'm pretty sure we could too assuming a mojom const > can't be anything other than a number or string literal? If not, could you add > an examples to the the sample mojom files which use the other const types. Updated build_default() to be a lot simpler, but I let it there, because this will also be used for default field value, and I'd like to let this at a single place if we ever update the idl to allow default array and struct as it was a few days ago. https://chromiumcodereview.appspot.com/291903003/diff/60001/mojo/public/tools... File mojo/public/tools/bindings/generators/java_templates/constants.java.tmpl (right): https://chromiumcodereview.appspot.com/291903003/diff/60001/mojo/public/tools... mojo/public/tools/bindings/generators/java_templates/constants.java.tmpl:11: } On 2014/05/25 22:55:10, rmcilroy wrote: > nit - newline between constructor and end of class "}" (you could also just do > "private {{main_entity}}() { }" on one line if you like. Done. https://chromiumcodereview.appspot.com/291903003/diff/60001/mojo/public/tools... File mojo/public/tools/bindings/generators/mojom_java_generator.py (right): https://chromiumcodereview.appspot.com/291903003/diff/60001/mojo/public/tools... mojo/public/tools/bindings/generators/mojom_java_generator.py:16: GENERATOR_PREFIX = 'java' On 2014/05/25 22:55:10, rmcilroy wrote: > Is this needed - it doesn't seem to be referenced. It is used by mojo/public/tools/bindings/mojom_bindings_generator.py do filter arguments passed to this generator. https://chromiumcodereview.appspot.com/291903003/diff/60001/mojo/public/tools... mojo/public/tools/bindings/generators/mojom_java_generator.py:70: def NamespaceToArray(namespace): Removed. https://chromiumcodereview.appspot.com/291903003/diff/60001/mojo/public/tools... mojo/public/tools/bindings/generators/mojom_java_generator.py:73: def GetNameFromString(string): On 2014/05/25 22:55:10, rmcilroy wrote: > I would call this "UpperCamelCase" and move it with the other CamelCase, etc > functions. Refactored naming function. This method doesn't exist anymore. https://chromiumcodereview.appspot.com/291903003/diff/60001/mojo/public/tools... mojo/public/tools/bindings/generators/mojom_java_generator.py:76: def GetName(kind): On 2014/05/25 22:55:10, rmcilroy wrote: > I would get rid of this method - the name is confusing compared to > GetNameForKind, and it is easy enough to just call UpperCamelCase(kind.name) > where it is needed. Done. https://chromiumcodereview.appspot.com/291903003/diff/60001/mojo/public/tools... mojo/public/tools/bindings/generators/mojom_java_generator.py:84: assert package[0] == 'EXPRESSION' That's not the style other python file use. Reversed the first if so that it is more readable, in the style of: if x return ... if y return etc. https://chromiumcodereview.appspot.com/291903003/diff/60001/mojo/public/tools... mojo/public/tools/bindings/generators/mojom_java_generator.py:94: return "org.chromium.mojo.bindings.Struct" Method removed until further CL. https://chromiumcodereview.appspot.com/291903003/diff/60001/mojo/public/tools... mojo/public/tools/bindings/generators/mojom_java_generator.py:96: def GetFlags(method, is_parameter): On 2014/05/25 22:55:10, rmcilroy wrote: > Could you remove this until it's used in a later CL please (I'm not sure how it > would be used and would like to review it in context) Done. https://chromiumcodereview.appspot.com/291903003/diff/60001/mojo/public/tools... mojo/public/tools/bindings/generators/mojom_java_generator.py:103: def NewArray(kind, size): Removed. https://chromiumcodereview.appspot.com/291903003/diff/60001/mojo/public/tools... mojo/public/tools/bindings/generators/mojom_java_generator.py:108: def GetNameHierachy(kind): On 2014/05/25 22:55:10, rmcilroy wrote: > Could you make GetNameHierarchy an inner function of GetNameForKind - this seems > to be the only place it's used. Done. https://chromiumcodereview.appspot.com/291903003/diff/60001/mojo/public/tools... mojo/public/tools/bindings/generators/mojom_java_generator.py:147: return GetPackage(token.module) + '.' + entity_value On 2014/05/25 22:55:10, rmcilroy wrote: > Could you please reduce this method to the bare minimum for the Const generation > (I think "return token" should be enough). I would like to review it in context > with the Enum,Struct,Interface generation CLs if possible. This is mostly the bare minimum. A constant can reference any other constant or enum value. Even for this CL, as constant could reference another class level constant. I updated this method to try to be a little clearer. https://chromiumcodereview.appspot.com/291903003/diff/60001/mojo/public/tools... mojo/public/tools/bindings/generators/mojom_java_generator.py:150: def ExpressionToText(value, module): On 2014/05/25 22:55:10, rmcilroy wrote: > This looks the same as the method in the CPP bindings generator - could it be > pulled out into a separate utility class in the pylib directory? The > CapitalizeFirst, CamelCase and UpperCaseConstant methods are also candidates to > move to a utility class. We can do this in a further CL if you do not mind. Moreover, the TranslateConstants is completely different, so if we extract it into a parameter, it really doesn't contain much. For the naming method, I guess we can moe it into an utility class, but right now, this file is the only user. https://chromiumcodereview.appspot.com/291903003/diff/60001/mojo/public/tools... mojo/public/tools/bindings/generators/mojom_java_generator.py:174: return '_'.join([x.upper() for x in parts]) This has been completely refactored. https://chromiumcodereview.appspot.com/291903003/diff/60001/mojo/public/tools... mojo/public/tools/bindings/generators/mojom_java_generator.py:177: return 'DEFAULT_' + UpperCaseConstant(GetName(field)) On 2014/05/25 22:55:10, rmcilroy wrote: > Please remove this until the CL where it is used. Done. https://chromiumcodereview.appspot.com/291903003/diff/60001/mojo/public/tools... mojo/public/tools/bindings/generators/mojom_java_generator.py:188: def IsPointerArrayKind(kind): On 2014/05/25 22:55:10, rmcilroy wrote: > Please remove IsPointerArrayKind, GetResponseStructFromMethod and > GetStructFromMethod and GetMethodName until the CL where they are used. Done. https://chromiumcodereview.appspot.com/291903003/diff/60001/mojo/public/tools... mojo/public/tools/bindings/generators/mojom_java_generator.py:206: def GetConstantsMainEntityFullyQualifiedName(module): This is now used. https://chromiumcodereview.appspot.com/291903003/diff/60001/mojo/public/tools... mojo/public/tools/bindings/generators/mojom_java_generator.py:211: return (GetNameFromString(module.path.split('/')[-1].rsplit('.', 1)[0]) + Added the comment. I'd rather do the annotation in another targeted CL. https://chromiumcodereview.appspot.com/291903003/diff/60001/mojo/public/tools... mojo/public/tools/bindings/generators/mojom_java_generator.py:246: "method_structs": self.GetStructsFromMethods(), On 2014/05/25 22:55:10, rmcilroy wrote: > I can't see any uses of "method_structs" either here or in the mega CL - can it > be removed? Done.
Looks much cleaner, thanks. A couple of last suggestions. https://chromiumcodereview.appspot.com/291903003/diff/60001/mojo/public/tools... File mojo/public/tools/bindings/generators/java_templates/constant_definition.tmpl (right): https://chromiumcodereview.appspot.com/291903003/diff/60001/mojo/public/tools... mojo/public/tools/bindings/generators/java_templates/constant_definition.tmpl:4: public static final {{constant.kind|java_type}} {{constant|constant_name}} = {{build_default(module, constant.kind, constant.value)|indent(4)}}; On 2014/05/26 08:41:20, qsr wrote: > On 2014/05/25 22:55:10, rmcilroy wrote: > > Do we need build_default()? The cpp binding generator gets away > > withexpression_to_text(), I'm pretty sure we could too assuming a mojom const > > can't be anything other than a number or string literal? If not, could you add > > an examples to the the sample mojom files which use the other const types. > > Updated build_default() to be a lot simpler, but I let it there, because this > will also be used for default field value, and I'd like to let this at a single > place if we ever update the idl to allow default array and struct as it was a > few days ago. I am fine with build_default existing (and being used for fields), but would prefer you use expression_to_text() here currently otherwise we are supporting a different IDL from the CPP generator. It would be easy to swap the filter used here if the IDL added back support for const arrays/structs. https://chromiumcodereview.appspot.com/291903003/diff/60001/mojo/public/tools... File mojo/public/tools/bindings/generators/mojom_java_generator.py (right): https://chromiumcodereview.appspot.com/291903003/diff/60001/mojo/public/tools... mojo/public/tools/bindings/generators/mojom_java_generator.py:150: def ExpressionToText(value, module): On 2014/05/26 08:41:20, qsr wrote: > On 2014/05/25 22:55:10, rmcilroy wrote: > > This looks the same as the method in the CPP bindings generator - could it be > > pulled out into a separate utility class in the pylib directory? The > > CapitalizeFirst, CamelCase and UpperCaseConstant methods are also candidates > to > > move to a utility class. > > We can do this in a further CL if you do not mind. Moreover, the > TranslateConstants is completely different, so if we extract it into a > parameter, it really doesn't contain much. > > For the naming method, I guess we can moe it into an utility class, but right > now, this file is the only user. Doing this in a separate cl is fine. BTW, I was only suggesting moving ExpressionToText, since as you say TranslateConstants is different per binding generator. https://chromiumcodereview.appspot.com/291903003/diff/80001/mojo/public/tools... File mojo/public/tools/bindings/generators/mojom_java_generator.py (right): https://chromiumcodereview.appspot.com/291903003/diff/80001/mojo/public/tools... mojo/public/tools/bindings/generators/mojom_java_generator.py:47: return True How about using the regex's I suggested in the last CL to transform the string to '_' separated, the split on '_' (either existing in string or added via regex transform)? https://chromiumcodereview.appspot.com/291903003/diff/80001/mojo/public/tools... mojo/public/tools/bindings/generators/mojom_java_generator.py:64: def UpperCaseHackerStyle(name): I've never heard of hacker style and can't find it online. Could we just call it UpperCaseUnderlined or ConstantStyle? https://chromiumcodereview.appspot.com/291903003/diff/80001/mojo/public/tools... mojo/public/tools/bindings/generators/mojom_java_generator.py:88: if 'JavaPackage' not in module.attributes: In the case of this function, I still think if/else blocking would be clearer here, at least between attribute and non attribute approach. Also I think it was cleaner having the positive if conditional, without the "not", as you had it before. https://chromiumcodereview.appspot.com/291903003/diff/80001/mojo/public/tools... mojo/public/tools/bindings/generators/mojom_java_generator.py:120: entity_value = GetNameForElement(named_value) /s/entity_value/entity_name I think. https://chromiumcodereview.appspot.com/291903003/diff/80001/mojo/public/tools... mojo/public/tools/bindings/generators/mojom_java_generator.py:122: if (not isinstance(named_value, mojom.EnumValue) and This seems a bit confusing to me, how about: if named_value.parent_kind: return GetJavaType(named_value.parent_kind) + '.' + entity_value else: # Handle the case where named_value is a module level constant. if isinstance(named_value, mojom.Constant): entity_value = GetConstantMainEntityName + '.' + entity_value return GetPackage(named_value.module) + '.' + entity_value Then you can remove GetConstantsMainEntityFullyQualifiedName. https://chromiumcodereview.appspot.com/291903003/diff/80001/mojo/public/tools... mojo/public/tools/bindings/generators/mojom_java_generator.py:132: if re.match('^[0-9]+$', token): Comment that you add Long suffix to all number literals.
https://chromiumcodereview.appspot.com/291903003/diff/80001/mojo/public/tools... File mojo/public/tools/bindings/generators/mojom_java_generator.py (right): https://chromiumcodereview.appspot.com/291903003/diff/80001/mojo/public/tools... mojo/public/tools/bindings/generators/mojom_java_generator.py:47: return True On 2014/05/26 11:13:48, rmcilroy wrote: > How about using the regex's I suggested in the last CL to transform the string > to '_' separated, the split on '_' (either existing in string or added via regex > transform)? Done, but I do not find that clearer. Also didn't cut on numbers, as I do not think it is necessary. https://chromiumcodereview.appspot.com/291903003/diff/80001/mojo/public/tools... mojo/public/tools/bindings/generators/mojom_java_generator.py:64: def UpperCaseHackerStyle(name): On 2014/05/26 11:13:48, rmcilroy wrote: > I've never heard of hacker style and can't find it online. Could we just call it > UpperCaseUnderlined or ConstantStyle? Done. Don't remember where I found it was called hacker style... The web is indeed completely empty about this... https://chromiumcodereview.appspot.com/291903003/diff/80001/mojo/public/tools... mojo/public/tools/bindings/generators/mojom_java_generator.py:88: if 'JavaPackage' not in module.attributes: On 2014/05/26 11:13:48, rmcilroy wrote: > In the case of this function, I still think if/else blocking would be clearer > here, at least between attribute and non attribute approach. Also I think it was > cleaner having the positive if conditional, without the "not", as you had it > before. Move the conditional back. But I really don't like the else. It is nowhere in the python file used in mojo/ https://chromiumcodereview.appspot.com/291903003/diff/80001/mojo/public/tools... mojo/public/tools/bindings/generators/mojom_java_generator.py:120: entity_value = GetNameForElement(named_value) On 2014/05/26 11:13:48, rmcilroy wrote: > /s/entity_value/entity_name I think. Done. https://chromiumcodereview.appspot.com/291903003/diff/80001/mojo/public/tools... mojo/public/tools/bindings/generators/mojom_java_generator.py:122: if (not isinstance(named_value, mojom.EnumValue) and On 2014/05/26 11:13:48, rmcilroy wrote: > This seems a bit confusing to me, how about: > > if named_value.parent_kind: > return GetJavaType(named_value.parent_kind) + '.' + entity_value > else: I still think the else after a return is not in any other py file here, so I'd prefer not to do it. If you want I can add a blank line for better separation. > # Handle the case where named_value is a module level constant. > if isinstance(named_value, mojom.Constant): The instance is not correct. A constant value (contrary to a Constant declaration) is just a plain mojom.NamedValue, so the only way to check for this is checking it is not an EnumValue. But changing the order helps for readability, so done this, also will get rid of getConstantsMainEntityFullyQualifiedName(named_value.module) by inlining it there. > entity_value = GetConstantMainEntityName + '.' + entity_value > return GetPackage(named_value.module) + '.' + entity_value > > Then you can remove GetConstantsMainEntityFullyQualifiedName. https://chromiumcodereview.appspot.com/291903003/diff/80001/mojo/public/tools... mojo/public/tools/bindings/generators/mojom_java_generator.py:132: if re.match('^[0-9]+$', token): On 2014/05/26 11:13:48, rmcilroy wrote: > Comment that you add Long suffix to all number literals. Done.
https://chromiumcodereview.appspot.com/291903003/diff/60001/mojo/public/tools... File mojo/public/tools/bindings/generators/java_templates/constant_definition.tmpl (right): https://chromiumcodereview.appspot.com/291903003/diff/60001/mojo/public/tools... mojo/public/tools/bindings/generators/java_templates/constant_definition.tmpl:4: public static final {{constant.kind|java_type}} {{constant|constant_name}} = {{build_default(module, constant.kind, constant.value)|indent(4)}}; I don't understand. The macro is only using expression_to_text().
Sorry for the delay, yesterday was a holiday in the UK. lgtm with comments, thanks. https://chromiumcodereview.appspot.com/291903003/diff/60001/mojo/public/tools... File mojo/public/tools/bindings/generators/java_templates/constant_definition.tmpl (right): https://chromiumcodereview.appspot.com/291903003/diff/60001/mojo/public/tools... mojo/public/tools/bindings/generators/java_templates/constant_definition.tmpl:4: public static final {{constant.kind|java_type}} {{constant|constant_name}} = {{build_default(module, constant.kind, constant.value)|indent(4)}}; On 2014/05/26 12:33:43, qsr wrote: > I don't understand. The macro is only using expression_to_text(). I was presuming you were going to update the build_default macro in a later CL to support arrays and objects again when it was used to build default field values - is this not the case? If not, then this is fine, though maybe renaming the macro to build_constant() would be clearer? https://chromiumcodereview.appspot.com/291903003/diff/80001/mojo/public/tools... File mojo/public/tools/bindings/generators/mojom_java_generator.py (right): https://chromiumcodereview.appspot.com/291903003/diff/80001/mojo/public/tools... mojo/public/tools/bindings/generators/mojom_java_generator.py:88: if 'JavaPackage' not in module.attributes: On 2014/05/26 12:05:42, qsr wrote: > On 2014/05/26 11:13:48, rmcilroy wrote: > > In the case of this function, I still think if/else blocking would be clearer > > here, at least between attribute and non attribute approach. Also I think it > was > > cleaner having the positive if conditional, without the "not", as you had it > > before. > > Move the conditional back. But I really don't like the else. It is nowhere in > the python file used in mojo/ OK, we disagree on this, but I'm not going to bikeshed https://chromiumcodereview.appspot.com/291903003/diff/80001/mojo/public/tools... mojo/public/tools/bindings/generators/mojom_java_generator.py:122: if (not isinstance(named_value, mojom.EnumValue) and On 2014/05/26 12:05:42, qsr wrote: > On 2014/05/26 11:13:48, rmcilroy wrote: > > This seems a bit confusing to me, how about: > > > > if named_value.parent_kind: > > return GetJavaType(named_value.parent_kind) + '.' + entity_value > > else: > > I still think the else after a return is not in any other py file here, so I'd > prefer not to do it. If you want I can add a blank line for better separation. > > > # Handle the case where named_value is a module level constant. > > if isinstance(named_value, mojom.Constant): > > The instance is not correct. A constant value (contrary to a Constant > declaration) is just a plain mojom.NamedValue, so the only way to check for this > is checking it is not an EnumValue. But changing the order helps for > readability, so done this, also will get rid of > getConstantsMainEntityFullyQualifiedName(named_value.module) by inlining it > there. > > > entity_value = GetConstantMainEntityName + '.' + entity_value > > return GetPackage(named_value.module) + '.' + entity_value > > > > Then you can remove GetConstantsMainEntityFullyQualifiedName. > Ok, this looks better, thanks. https://chromiumcodereview.appspot.com/291903003/diff/130001/mojo/public/tool... File mojo/public/tools/bindings/generators/mojom_java_generator.py (right): https://chromiumcodereview.appspot.com/291903003/diff/130001/mojo/public/tool... mojo/public/tools/bindings/generators/mojom_java_generator.py:137: def GetConstantsMainEntityFullyQualifiedName(module): Remove please.
On 2014/05/27 09:44:34, rmcilroy wrote: > Sorry for the delay, yesterday was a holiday in the UK. > > lgtm with comments, thanks. > > https://chromiumcodereview.appspot.com/291903003/diff/60001/mojo/public/tools... > File > mojo/public/tools/bindings/generators/java_templates/constant_definition.tmpl > (right): > > https://chromiumcodereview.appspot.com/291903003/diff/60001/mojo/public/tools... > mojo/public/tools/bindings/generators/java_templates/constant_definition.tmpl:4: > public static final {{constant.kind|java_type}} {{constant|constant_name}} = > {{build_default(module, constant.kind, constant.value)|indent(4)}}; > On 2014/05/26 12:33:43, qsr wrote: > > I don't understand. The macro is only using expression_to_text(). > > I was presuming you were going to update the build_default macro in a later CL > to support arrays and objects again when it was used to build default field > values - is this not the case? If not, then this is fine, though maybe renaming > the macro to build_constant() would be clearer? My point is that when we accept constant that are something else that expression, we will probably accept that at all level, constant, default value, etc. That's the reason I wanted this macro here -> When we extend the IDL, we have one single place to update. If we do extend but artificially restrict it to be different for constants and default values, we can rename this. If that fine with you? > > https://chromiumcodereview.appspot.com/291903003/diff/80001/mojo/public/tools... > File mojo/public/tools/bindings/generators/mojom_java_generator.py (right): > > https://chromiumcodereview.appspot.com/291903003/diff/80001/mojo/public/tools... > mojo/public/tools/bindings/generators/mojom_java_generator.py:88: if > 'JavaPackage' not in module.attributes: > On 2014/05/26 12:05:42, qsr wrote: > > On 2014/05/26 11:13:48, rmcilroy wrote: > > > In the case of this function, I still think if/else blocking would be > clearer > > > here, at least between attribute and non attribute approach. Also I think it > > was > > > cleaner having the positive if conditional, without the "not", as you had > it > > > before. > > > > Move the conditional back. But I really don't like the else. It is nowhere in > > the python file used in mojo/ > > OK, we disagree on this, but I'm not going to bikeshed > > https://chromiumcodereview.appspot.com/291903003/diff/80001/mojo/public/tools... > mojo/public/tools/bindings/generators/mojom_java_generator.py:122: if (not > isinstance(named_value, mojom.EnumValue) and > On 2014/05/26 12:05:42, qsr wrote: > > On 2014/05/26 11:13:48, rmcilroy wrote: > > > This seems a bit confusing to me, how about: > > > > > > if named_value.parent_kind: > > > return GetJavaType(named_value.parent_kind) + '.' + entity_value > > > else: > > > > I still think the else after a return is not in any other py file here, so > I'd > > prefer not to do it. If you want I can add a blank line for better separation. > > > > > # Handle the case where named_value is a module level constant. > > > if isinstance(named_value, mojom.Constant): > > > > The instance is not correct. A constant value (contrary to a Constant > > declaration) is just a plain mojom.NamedValue, so the only way to check for > this > > is checking it is not an EnumValue. But changing the order helps for > > readability, so done this, also will get rid of > > getConstantsMainEntityFullyQualifiedName(named_value.module) by inlining it > > there. > > > > > entity_value = GetConstantMainEntityName + '.' + entity_value > > > return GetPackage(named_value.module) + '.' + entity_value > > > > > > Then you can remove GetConstantsMainEntityFullyQualifiedName. > > > > Ok, this looks better, thanks. > > https://chromiumcodereview.appspot.com/291903003/diff/130001/mojo/public/tool... > File mojo/public/tools/bindings/generators/mojom_java_generator.py (right): > > https://chromiumcodereview.appspot.com/291903003/diff/130001/mojo/public/tool... > mojo/public/tools/bindings/generators/mojom_java_generator.py:137: def > GetConstantsMainEntityFullyQualifiedName(module): > Remove please. Stupid me...
https://chromiumcodereview.appspot.com/291903003/diff/130001/mojo/public/tool... File mojo/public/tools/bindings/generators/mojom_java_generator.py (right): https://chromiumcodereview.appspot.com/291903003/diff/130001/mojo/public/tool... mojo/public/tools/bindings/generators/mojom_java_generator.py:137: def GetConstantsMainEntityFullyQualifiedName(module): On 2014/05/27 09:44:35, rmcilroy wrote: > Remove please. Done.
viettrungluu@ -> Could you take a look for OWNERS. Thanks.
> > I was presuming you were going to update the build_default macro in a later CL > > to support arrays and objects again when it was used to build default field > > values - is this not the case? If not, then this is fine, though maybe > renaming > > the macro to build_constant() would be clearer? > > My point is that when we accept constant that are something else that > expression, we will probably accept that at all level, constant, default value, > etc. That's the reason I wanted this macro here -> When we extend the IDL, we > have one single place to update. If we do extend but artificially restrict it to > be different for constants and default values, we can rename this. If that fine > with you? Ok this is fine for now.
> > I was presuming you were going to update the build_default macro in a later CL > > to support arrays and objects again when it was used to build default field > > values - is this not the case? If not, then this is fine, though maybe > renaming > > the macro to build_constant() would be clearer? > > My point is that when we accept constant that are something else that > expression, we will probably accept that at all level, constant, default value, > etc. That's the reason I wanted this macro here -> When we extend the IDL, we > have one single place to update. If we do extend but artificially restrict it to > be different for constants and default values, we can rename this. If that fine > with you? Ok this is fine for now.
> > I was presuming you were going to update the build_default macro in a later CL > > to support arrays and objects again when it was used to build default field > > values - is this not the case? If not, then this is fine, though maybe > renaming > > the macro to build_constant() would be clearer? > > My point is that when we accept constant that are something else that > expression, we will probably accept that at all level, constant, default value, > etc. That's the reason I wanted this macro here -> When we extend the IDL, we > have one single place to update. If we do extend but artificially restrict it to > be different for constants and default values, we can rename this. If that fine > with you? Ok this is fine for now.
lgtm
The CQ bit was checked by qsr@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/qsr@chromium.org/291903003/150001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/18...)
The CQ bit was checked by qsr@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/qsr@chromium.org/291903003/170001
The CQ bit was unchecked by qsr@chromium.org
The CQ bit was checked by qsr@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/qsr@chromium.org/291903003/150001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_dbg_triggered_tests on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)
The CQ bit was unchecked by qsr@chromium.org
The CQ bit was checked by qsr@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/qsr@chromium.org/291903003/150001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_dbg_triggered_tests on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_dbg_triggered_tests on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)
The CQ bit was checked by qsr@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/qsr@chromium.org/291903003/150001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_dbg_triggered_tests on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)
The CQ bit was checked by qsr@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/qsr@chromium.org/291903003/150002
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_chromium_gn_compile_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...) android_dbg_triggered_tests on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...) linux_chromium_chromeos_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_chromium_gn_compile_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...)
The CQ bit was checked by qsr@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/qsr@chromium.org/291903003/150002
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_chromium_gn_compile_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...) linux_chromium_chromeos_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
The CQ bit was unchecked by qsr@chromium.org
The CQ bit was checked by qsr@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/qsr@chromium.org/291903003/150002
Message was sent while issue was closed.
Change committed as 273307
Message was sent while issue was closed.
A revert of this CL has been created in https://codereview.chromium.org/302923002/ by mathp@chromium.org. The reason for reverting is: Breaking a bot (Linux Builder (dbg)(32)). |
