Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
317273006:
Add serialization/deserialization of structs for mojo java bindings. (Closed)
|
Created:
6 years, 6 months ago by qsr Modified:
6 years, 5 months ago CC:
chromium-reviews, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, tfarina, yzshen+watch_chromium.org, abarth-chromium, Aaron Boodman, darin (slow to review), ben+mojo_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd serialization/deserialization of structs for mojo java bindings.
R=viettrungluu@chromium.org,rmcilroy@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=281255
Patch Set 1 #
Total comments: 1
Patch Set 2 : Fix indentation #
Total comments: 34
Patch Set 3 : Follow review #
Total comments: 58
Patch Set 4 : Follow review #
Total comments: 4
Patch Set 5 : Follow review #Patch Set 6 : Remove findbugs exclusion now that fields are used. #Patch Set 7 : Rebase over message receiver CL. #Patch Set 8 : Fix findbugs issues #
Total comments: 1
Patch Set 9 : #Messages
Total messages: 21 (0 generated)
https://codereview.chromium.org/317273006/diff/1/mojo/android/javatests/src/o... File mojo/android/javatests/src/org/chromium/mojo/bindings/BindingsTest.java (right): https://codereview.chromium.org/317273006/diff/1/mojo/android/javatests/src/o... mojo/android/javatests/src/org/chromium/mojo/bindings/BindingsTest.java:245: checkField(DefaultsTest.class.getField("a22"), Thing.class); This changes is just an interface of git cl. They will disappear when the struct generation CL land.
First round of comments. https://codereview.chromium.org/317273006/diff/20001/mojo/bindings/java/src/o... File mojo/bindings/java/src/org/chromium/mojo/bindings/Decoder.java (right): https://codereview.chromium.org/317273006/diff/20001/mojo/bindings/java/src/o... mojo/bindings/java/src/org/chromium/mojo/bindings/Decoder.java:20: * the given offset in the byte buffer and allows to deserialized basic types. A Decoder is a helper class for deserializing a mojo struct. It enables deserialization of basic types from a {@link Message} object at a given offset into it's byte buffer. https://codereview.chromium.org/317273006/diff/20001/mojo/bindings/java/src/o... mojo/bindings/java/src/org/chromium/mojo/bindings/Decoder.java:28: /** nit - newline above https://codereview.chromium.org/317273006/diff/20001/mojo/bindings/java/src/o... mojo/bindings/java/src/org/chromium/mojo/bindings/Decoder.java:75: * the message to deserialize from. /s/the/The https://codereview.chromium.org/317273006/diff/20001/mojo/bindings/java/src/o... mojo/bindings/java/src/org/chromium/mojo/bindings/Decoder.java:99: int offset) { /s/offset/baseOffset https://codereview.chromium.org/317273006/diff/20001/mojo/bindings/java/src/o... mojo/bindings/java/src/org/chromium/mojo/bindings/Decoder.java:105: mValidator.claimMemory(mBaseOffset, mBaseOffset + 8); Make '8' a constant in the DataHeader class. https://codereview.chromium.org/317273006/diff/20001/mojo/bindings/java/src/o... mojo/bindings/java/src/org/chromium/mojo/bindings/Decoder.java:111: public Decoder getDecoderAtPosition(int offset) { Can this be private? https://codereview.chromium.org/317273006/diff/20001/mojo/bindings/java/src/o... mojo/bindings/java/src/org/chromium/mojo/bindings/Decoder.java:116: * Returns a view of the underlying message of this decoder, starting at the given offset. it's unclear what "the given offset" is. Do you need this method at all? If so clarify in the JavaDoc. Also, the name "getRemainingMessage()" suggests this returns what has not been processed by readByte, etc., whereas it actually returns the Message from the point where getDecoderAtPosition() set the baseOffset. Maybe getMessage() is more appropriate name? https://codereview.chromium.org/317273006/diff/20001/mojo/bindings/java/src/o... mojo/bindings/java/src/org/chromium/mojo/bindings/Decoder.java:195: return getDecoderAtPosition(newPosition); maybe add a comment that the getDecoderAtPosition will validate that the pointer's address is valid? https://codereview.chromium.org/317273006/diff/20001/mojo/bindings/java/src/o... mojo/bindings/java/src/org/chromium/mojo/bindings/Decoder.java:357: // TODO(qsr): To be implemented when interfaces proxy and stubs are implemented. throw an UnsupportedOperationException("Unimplemented operation") (or something similar) here. https://codereview.chromium.org/317273006/diff/20001/mojo/bindings/java/src/o... mojo/bindings/java/src/org/chromium/mojo/bindings/Decoder.java:366: return new InterfaceRequest<S>(); ditto (and return null) https://codereview.chromium.org/317273006/diff/20001/mojo/bindings/java/src/o... mojo/bindings/java/src/org/chromium/mojo/bindings/Decoder.java:391: result[i] = d.readHandle(8 + 4 * i); factor '4' out as a constant (related to the size of the handle when serialized). https://codereview.chromium.org/317273006/diff/20001/mojo/bindings/java/src/o... mojo/bindings/java/src/org/chromium/mojo/bindings/Decoder.java:415: public DataPipe.ProducerHandle[] readProducerHandles(int offset) { Could make a generic "readHandle<T extends Handle>" and "readHandles<T extends Handle>" to avoid the per-handle duplication? If you still want the concrete methods for each handle type that's fine, but it would be good if they could all just call these generic helper methods. https://codereview.chromium.org/317273006/diff/20001/mojo/bindings/java/src/o... File mojo/bindings/java/src/org/chromium/mojo/bindings/Struct.java (right): https://codereview.chromium.org/317273006/diff/20001/mojo/bindings/java/src/o... mojo/bindings/java/src/org/chromium/mojo/bindings/Struct.java:17: public static class DataHeader { final https://codereview.chromium.org/317273006/diff/20001/mojo/not-to-commit/mojo_... File mojo/not-to-commit/mojo_public_test_interfaces/src/org/chromium/mojo/bindings/test/imported/Point.java (right): https://codereview.chromium.org/317273006/diff/20001/mojo/not-to-commit/mojo_... mojo/not-to-commit/mojo_public_test_interfaces/src/org/chromium/mojo/bindings/test/imported/Point.java:24: public static Point deserialize(org.chromium.mojo.bindings.Message message) { To keep things consistent with serialize, could you make deserialize a method in Struct (which has a generic return type <T extends Struct>, and then have Point extend Struct<Point>)? https://codereview.chromium.org/317273006/diff/20001/mojo/not-to-commit/mojo_... mojo/not-to-commit/mojo_public_test_interfaces/src/org/chromium/mojo/bindings/test/imported/Point.java:28: public static Point deserialize(org.chromium.mojo.bindings.Decoder decoder0) { /s/deserialize/decode https://codereview.chromium.org/317273006/diff/20001/mojo/not-to-commit/mojo_... File mojo/not-to-commit/mojo_public_test_interfaces/src/org/chromium/mojo/bindings/test/imported/Size.java (right): https://codereview.chromium.org/317273006/diff/20001/mojo/not-to-commit/mojo_... mojo/not-to-commit/mojo_public_test_interfaces/src/org/chromium/mojo/bindings/test/imported/Size.java:15: private static final DataHeader DEFAULT_STRUCT_INFO = new DataHeader(16, 2); make the size a private static final field which is used throught
https://codereview.chromium.org/317273006/diff/20001/mojo/bindings/java/src/o... File mojo/bindings/java/src/org/chromium/mojo/bindings/Decoder.java (right): https://codereview.chromium.org/317273006/diff/20001/mojo/bindings/java/src/o... mojo/bindings/java/src/org/chromium/mojo/bindings/Decoder.java:415: public DataPipe.ProducerHandle[] readProducerHandles(int offset) { On 2014/06/20 13:47:33, rmcilroy wrote: > Could make a generic "readHandle<T extends Handle>" and "readHandles<T extends > Handle>" to avoid the per-handle duplication? If you still want the concrete > methods for each handle type that's fine, but it would be good if they could all > just call these generic helper methods. I do not know how to do this. A generic method cannot create an array of the right type. Also, I need to call the right handle. The best I can do is create a array of untyped handle, and then in each method recreate the right array and transform the handle. It would be more factored at the price of a second array allocation. I chose not to create the array, but I can change it if you'd prefer. https://codereview.chromium.org/317273006/diff/20001/mojo/not-to-commit/mojo_... File mojo/not-to-commit/mojo_public_test_interfaces/src/org/chromium/mojo/bindings/test/imported/Point.java (right): https://codereview.chromium.org/317273006/diff/20001/mojo/not-to-commit/mojo_... mojo/not-to-commit/mojo_public_test_interfaces/src/org/chromium/mojo/bindings/test/imported/Point.java:24: public static Point deserialize(org.chromium.mojo.bindings.Message message) { On 2014/06/20 13:47:34, rmcilroy wrote: > To keep things consistent with serialize, could you make deserialize a method in > Struct (which has a generic return type <T extends Struct>, and then have Point > extend Struct<Point>)? I do not know how to do this. This method is static -> you cannot have abstact static methods, nor override a static method.
https://codereview.chromium.org/317273006/diff/20001/mojo/bindings/java/src/o... File mojo/bindings/java/src/org/chromium/mojo/bindings/Decoder.java (right): https://codereview.chromium.org/317273006/diff/20001/mojo/bindings/java/src/o... mojo/bindings/java/src/org/chromium/mojo/bindings/Decoder.java:415: public DataPipe.ProducerHandle[] readProducerHandles(int offset) { On 2014/06/20 15:31:52, qsr wrote: > On 2014/06/20 13:47:33, rmcilroy wrote: > > Could make a generic "readHandle<T extends Handle>" and "readHandles<T extends > > Handle>" to avoid the per-handle duplication? If you still want the concrete > > methods for each handle type that's fine, but it would be good if they could > all > > just call these generic helper methods. > > I do not know how to do this. A generic method cannot create an array of the > right type. Also, I need to call the right handle. The best I can do is create a > array of untyped handle, and then in each method recreate the right array and > transform the handle. It would be more factored at the price of a second array > allocation. I chose not to create the array, but I can change it if you'd > prefer. You are right. Ok ignore this comment (I prefer the multiple methods to having to create two arrays each time). https://codereview.chromium.org/317273006/diff/20001/mojo/not-to-commit/mojo_... File mojo/not-to-commit/mojo_public_test_interfaces/src/org/chromium/mojo/bindings/test/imported/Point.java (right): https://codereview.chromium.org/317273006/diff/20001/mojo/not-to-commit/mojo_... mojo/not-to-commit/mojo_public_test_interfaces/src/org/chromium/mojo/bindings/test/imported/Point.java:24: public static Point deserialize(org.chromium.mojo.bindings.Message message) { On 2014/06/20 15:31:52, qsr wrote: > On 2014/06/20 13:47:34, rmcilroy wrote: > > To keep things consistent with serialize, could you make deserialize a method > in > > Struct (which has a generic return type <T extends Struct>, and then have > Point > > extend Struct<Point>)? > > I do not know how to do this. This method is static -> you cannot have abstact > static methods, nor override a static method. Of course - missed that this was static. Ok, ignore this.
https://chromiumcodereview.appspot.com/317273006/diff/20001/mojo/bindings/jav... File mojo/bindings/java/src/org/chromium/mojo/bindings/Decoder.java (right): https://chromiumcodereview.appspot.com/317273006/diff/20001/mojo/bindings/jav... mojo/bindings/java/src/org/chromium/mojo/bindings/Decoder.java:20: * the given offset in the byte buffer and allows to deserialized basic types. On 2014/06/20 13:47:33, rmcilroy wrote: > A Decoder is a helper class for deserializing a mojo struct. It enables > deserialization of basic types from a {@link Message} object at a given offset > into it's byte buffer. Done. https://chromiumcodereview.appspot.com/317273006/diff/20001/mojo/bindings/jav... mojo/bindings/java/src/org/chromium/mojo/bindings/Decoder.java:28: /** On 2014/06/20 13:47:33, rmcilroy wrote: > nit - newline above Done. https://chromiumcodereview.appspot.com/317273006/diff/20001/mojo/bindings/jav... mojo/bindings/java/src/org/chromium/mojo/bindings/Decoder.java:75: * the message to deserialize from. On 2014/06/20 13:47:33, rmcilroy wrote: > /s/the/The Done. https://chromiumcodereview.appspot.com/317273006/diff/20001/mojo/bindings/jav... mojo/bindings/java/src/org/chromium/mojo/bindings/Decoder.java:99: int offset) { On 2014/06/20 13:47:34, rmcilroy wrote: > /s/offset/baseOffset Done. https://chromiumcodereview.appspot.com/317273006/diff/20001/mojo/bindings/jav... mojo/bindings/java/src/org/chromium/mojo/bindings/Decoder.java:105: mValidator.claimMemory(mBaseOffset, mBaseOffset + 8); On 2014/06/20 13:47:34, rmcilroy wrote: > Make '8' a constant in the DataHeader class. Done. https://chromiumcodereview.appspot.com/317273006/diff/20001/mojo/bindings/jav... mojo/bindings/java/src/org/chromium/mojo/bindings/Decoder.java:111: public Decoder getDecoderAtPosition(int offset) { On 2014/06/20 13:47:33, rmcilroy wrote: > Can this be private? Done. https://chromiumcodereview.appspot.com/317273006/diff/20001/mojo/bindings/jav... mojo/bindings/java/src/org/chromium/mojo/bindings/Decoder.java:116: * Returns a view of the underlying message of this decoder, starting at the given offset. This is a remanining of the time where I wanted to keep cached serialization. But given that this is not possible to send back unmodified a message I received, this is not useful anymore. Deleted. https://chromiumcodereview.appspot.com/317273006/diff/20001/mojo/bindings/jav... mojo/bindings/java/src/org/chromium/mojo/bindings/Decoder.java:195: return getDecoderAtPosition(newPosition); On 2014/06/20 13:47:33, rmcilroy wrote: > maybe add a comment that the getDecoderAtPosition will validate that the > pointer's address is valid? Done. https://chromiumcodereview.appspot.com/317273006/diff/20001/mojo/bindings/jav... mojo/bindings/java/src/org/chromium/mojo/bindings/Decoder.java:357: // TODO(qsr): To be implemented when interfaces proxy and stubs are implemented. On 2014/06/20 13:47:33, rmcilroy wrote: > throw an UnsupportedOperationException("Unimplemented operation") (or something > similar) here. Done. https://chromiumcodereview.appspot.com/317273006/diff/20001/mojo/bindings/jav... mojo/bindings/java/src/org/chromium/mojo/bindings/Decoder.java:366: return new InterfaceRequest<S>(); On 2014/06/20 13:47:33, rmcilroy wrote: > ditto (and return null) Done. https://chromiumcodereview.appspot.com/317273006/diff/20001/mojo/bindings/jav... mojo/bindings/java/src/org/chromium/mojo/bindings/Decoder.java:391: result[i] = d.readHandle(8 + 4 * i); On 2014/06/20 13:47:34, rmcilroy wrote: > factor '4' out as a constant (related to the size of the handle when > serialized). Done. https://chromiumcodereview.appspot.com/317273006/diff/20001/mojo/bindings/jav... File mojo/bindings/java/src/org/chromium/mojo/bindings/Struct.java (right): https://chromiumcodereview.appspot.com/317273006/diff/20001/mojo/bindings/jav... mojo/bindings/java/src/org/chromium/mojo/bindings/Struct.java:17: public static class DataHeader { On 2014/06/20 13:47:34, rmcilroy wrote: > final Done. https://chromiumcodereview.appspot.com/317273006/diff/20001/mojo/not-to-commi... File mojo/not-to-commit/mojo_public_test_interfaces/src/org/chromium/mojo/bindings/test/imported/Point.java (right): https://chromiumcodereview.appspot.com/317273006/diff/20001/mojo/not-to-commi... mojo/not-to-commit/mojo_public_test_interfaces/src/org/chromium/mojo/bindings/test/imported/Point.java:28: public static Point deserialize(org.chromium.mojo.bindings.Decoder decoder0) { On 2014/06/20 13:47:34, rmcilroy wrote: > /s/deserialize/decode Done. https://chromiumcodereview.appspot.com/317273006/diff/20001/mojo/not-to-commi... File mojo/not-to-commit/mojo_public_test_interfaces/src/org/chromium/mojo/bindings/test/imported/Size.java (right): https://chromiumcodereview.appspot.com/317273006/diff/20001/mojo/not-to-commi... mojo/not-to-commit/mojo_public_test_interfaces/src/org/chromium/mojo/bindings/test/imported/Size.java:15: private static final DataHeader DEFAULT_STRUCT_INFO = new DataHeader(16, 2); On 2014/06/20 13:47:34, rmcilroy wrote: > make the size a private static final field which is used throught Done.
Next batch of comments https://chromiumcodereview.appspot.com/317273006/diff/40001/mojo/android/java... File mojo/android/javatests/src/org/chromium/mojo/bindings/BindingsTest.java (right): https://chromiumcodereview.appspot.com/317273006/diff/40001/mojo/android/java... mojo/android/javatests/src/org/chromium/mojo/bindings/BindingsTest.java:67: boolean[][] b = new boolean[5][]; How about just: foo.arrayOfArrayOfBools = new boolean[][] { {true, false, true}, {false}, {false}, {false}, {false} }; https://chromiumcodereview.appspot.com/317273006/diff/40001/mojo/android/java... mojo/android/javatests/src/org/chromium/mojo/bindings/BindingsTest.java:81: foo.extraBars = bars; similarly: foo.extraBars = new Bar[] { newBar(), newBar() }; https://chromiumcodereview.appspot.com/317273006/diff/40001/mojo/android/java... mojo/android/javatests/src/org/chromium/mojo/bindings/BindingsTest.java:234: // Checking creation of a Foo object. Checking serialization and deserialization of a Foo object. https://chromiumcodereview.appspot.com/317273006/diff/40001/mojo/bindings/jav... File mojo/bindings/java/src/org/chromium/mojo/bindings/BindingsHelper.java (right): https://chromiumcodereview.appspot.com/317273006/diff/40001/mojo/bindings/jav... mojo/bindings/java/src/org/chromium/mojo/bindings/BindingsHelper.java:12: * Alignment in byte for mojo serialization. /s/byte/bytes https://chromiumcodereview.appspot.com/317273006/diff/40001/mojo/bindings/jav... File mojo/bindings/java/src/org/chromium/mojo/bindings/Decoder.java (right): https://chromiumcodereview.appspot.com/317273006/diff/40001/mojo/bindings/jav... mojo/bindings/java/src/org/chromium/mojo/bindings/Decoder.java:111: int size = readInt(0); nit - extract '0' and '4' as constants in DataHeader and use them here (and in encoder) https://chromiumcodereview.appspot.com/317273006/diff/40001/mojo/bindings/jav... File mojo/bindings/java/src/org/chromium/mojo/bindings/DeserializationException.java (right): https://chromiumcodereview.appspot.com/317273006/diff/40001/mojo/bindings/jav... mojo/bindings/java/src/org/chromium/mojo/bindings/DeserializationException.java:8: * Error when deserializing a mojo archive. "archive" doesn't seem to be a name used to describe seriailized mojo messages AFAIKT (and implies writing to disk to me). Would "Represents an error when deserializing a mojo message."? https://chromiumcodereview.appspot.com/317273006/diff/40001/mojo/bindings/jav... mojo/bindings/java/src/org/chromium/mojo/bindings/DeserializationException.java:13: * Constructor. More informative javadoc would be nice. https://chromiumcodereview.appspot.com/317273006/diff/40001/mojo/bindings/jav... mojo/bindings/java/src/org/chromium/mojo/bindings/DeserializationException.java:20: * Constructor. ditto https://chromiumcodereview.appspot.com/317273006/diff/40001/mojo/bindings/jav... File mojo/bindings/java/src/org/chromium/mojo/bindings/Encoder.java (right): https://chromiumcodereview.appspot.com/317273006/diff/40001/mojo/bindings/jav... mojo/bindings/java/src/org/chromium/mojo/bindings/Encoder.java:37: private static class EncoderInformation { Maybe EncoderState (and update javadoc) is more understandable? Also, move above to top of class to avoid splitting Encoder's field definitions. https://chromiumcodereview.appspot.com/317273006/diff/40001/mojo/bindings/jav... mojo/bindings/java/src/org/chromium/mojo/bindings/Encoder.java:40: * The core to encode interfaces. The core used to encode interfaces. https://chromiumcodereview.appspot.com/317273006/diff/40001/mojo/bindings/jav... mojo/bindings/java/src/org/chromium/mojo/bindings/Encoder.java:45: * The ByteBuffer to write to. The ByteBuffer to which the message will be encoded. https://chromiumcodereview.appspot.com/317273006/diff/40001/mojo/bindings/jav... mojo/bindings/java/src/org/chromium/mojo/bindings/Encoder.java:60: * Constructor. javadoc parameters https://chromiumcodereview.appspot.com/317273006/diff/40001/mojo/bindings/jav... mojo/bindings/java/src/org/chromium/mojo/bindings/Encoder.java:135: * Encoder a {@link DataHeader}. Also resize the buffer to allow writing the data section if Encoder a {@link DataHeader} and claim the amount of memory required for the data section (resizing the buffer if required). https://chromiumcodereview.appspot.com/317273006/diff/40001/mojo/bindings/jav... mojo/bindings/java/src/org/chromium/mojo/bindings/Encoder.java:139: mEncoderInformation.dataEnd = mEncoderInformation.dataEnd + s.size; I think it makes more sense to update dataEnd and growIfNeeded in one method. Maybe something like: mEncoderInformation.claimMemory(s.size); Also make dataEnd private and add a method called something like getNextUnclaimedDataOffset() for where you need to read dataEnd. https://chromiumcodereview.appspot.com/317273006/diff/40001/mojo/bindings/jav... mojo/bindings/java/src/org/chromium/mojo/bindings/Encoder.java:206: encode((long) mEncoderInformation.dataEnd - (mBaseOffset + offset), offset); maybe pull this out into a private method "encodedPointerToNextUnclaimedData(offset)" to be used here and in array serialization? https://chromiumcodereview.appspot.com/317273006/diff/40001/mojo/bindings/jav... mojo/bindings/java/src/org/chromium/mojo/bindings/Encoder.java:207: Encoder e = new Encoder(mEncoderInformation); This is a bit confusing that we end up creating two Encoder's when encoding a struct (one here and one when struct.encode() calls getEncoderAtDataOffset). Could we avoid the extra encoder creation here and just pass the "this" to v.encode()? https://chromiumcodereview.appspot.com/317273006/diff/40001/mojo/bindings/jav... mojo/bindings/java/src/org/chromium/mojo/bindings/Encoder.java:226: if (v != null && v.isValid()) { nit - do this check the other way around to be consistent with the "if (v == null) {" check's above. https://chromiumcodereview.appspot.com/317273006/diff/40001/mojo/bindings/jav... mojo/bindings/java/src/org/chromium/mojo/bindings/Encoder.java:262: return encoderForArray(8, length, offset); nit - use constants for element sizes https://chromiumcodereview.appspot.com/317273006/diff/40001/mojo/bindings/jav... mojo/bindings/java/src/org/chromium/mojo/bindings/Encoder.java:409: e.encode(new DataHeader(DataHeader.HEADER_SIZE + BindingsHelper.align(byteSize), length)); nit - instead of the above two lines, can you just use: Encoder arrayEncoder = getEncoderAtDataOffset(new DataHeader(DataHeader.HEADER_SIZE + BindingsHelper.align(byteSize), length)); https://chromiumcodereview.appspot.com/317273006/diff/40001/mojo/not-to-commi... File mojo/not-to-commit/mojo_public_test_interfaces/src/org/chromium/mojo/bindings/test/mojom/imported/Size.java (right): https://chromiumcodereview.appspot.com/317273006/diff/40001/mojo/not-to-commi... mojo/not-to-commit/mojo_public_test_interfaces/src/org/chromium/mojo/bindings/test/mojom/imported/Size.java:32: if (0 < mainDataHeader.numFields) { nit - "if (mainDataHeader.numFields > 0)" https://chromiumcodereview.appspot.com/317273006/diff/40001/mojo/not-to-commi... mojo/not-to-commit/mojo_public_test_interfaces/src/org/chromium/mojo/bindings/test/mojom/imported/Size.java:33: result.width = decoder0.readInt(8); Could you pull the offset's out as private static final constants at the to of the generated struct, and use them both in decode and encode? https://chromiumcodereview.appspot.com/317273006/diff/40001/mojo/not-to-commi... File mojo/not-to-commit/mojo_public_test_interfaces/src/org/chromium/mojo/bindings/test/mojom/imported/Thing.java (right): https://chromiumcodereview.appspot.com/317273006/diff/40001/mojo/not-to-commi... mojo/not-to-commit/mojo_public_test_interfaces/src/org/chromium/mojo/bindings/test/mojom/imported/Thing.java:43: org.chromium.mojo.bindings.Decoder decoder1 = decoder0.readPointer(16); Could you add the null check at the top of struct's decode() method (and return null if decoder is null) then you could just do: org.chromium.mojo.bindings.Decoder decoder1 = decoder0.readPointer(16); result.location = Point.decode(decoder1); https://chromiumcodereview.appspot.com/317273006/diff/40001/mojo/not-to-commi... File mojo/not-to-commit/mojo_public_test_interfaces/src/org/chromium/mojo/bindings/test/mojom/test_structs/NamedRegion.java (right): https://chromiumcodereview.appspot.com/317273006/diff/40001/mojo/not-to-commi... mojo/not-to-commit/mojo_public_test_interfaces/src/org/chromium/mojo/bindings/test/mojom/test_structs/NamedRegion.java:64: encoder1.encode(rects[i0], 8 + 8 * i0); constants here (element size and DataHeader.HEADER_SIZE) https://chromiumcodereview.appspot.com/317273006/diff/40001/mojo/public/tools... File mojo/public/tools/bindings/generators/java_templates/struct_definition.tmpl (right): https://chromiumcodereview.appspot.com/317273006/diff/40001/mojo/public/tools... mojo/public/tools/bindings/generators/java_templates/struct_definition.tmpl:28: {% else %} comment that kind|is_pointer_array_kind. https://chromiumcodereview.appspot.com/317273006/diff/40001/mojo/public/tools... File mojo/public/tools/bindings/generators/mojom_java_generator.py (right): https://chromiumcodereview.appspot.com/317273006/diff/40001/mojo/public/tools... mojo/public/tools/bindings/generators/mojom_java_generator.py:149: additionalParams = ', %s.BUILDER' % GetJavaType(context, interfaceKind) I think this would be clearer as: if isinstance(kind, mojom.Interface: additionalParams = ', %s.BUILDER' % GetJavaType(context, kind) if (IsArray(kind) and isinstance(kind.kind, mojom.Interface)): additionalParams = ', %s.BUILDER' % GetJavaType(context, kind.kind) https://chromiumcodereview.appspot.com/317273006/diff/40001/mojo/public/tools... mojo/public/tools/bindings/generators/mojom_java_generator.py:163: additionalParams = ', %s.BUILDER' % GetJavaType(context, interfaceKind) ditto
https://chromiumcodereview.appspot.com/317273006/diff/40001/mojo/android/java... File mojo/android/javatests/src/org/chromium/mojo/bindings/BindingsTest.java (right): https://chromiumcodereview.appspot.com/317273006/diff/40001/mojo/android/java... mojo/android/javatests/src/org/chromium/mojo/bindings/BindingsTest.java:67: boolean[][] b = new boolean[5][]; On 2014/06/25 13:14:38, rmcilroy wrote: > How about just: > foo.arrayOfArrayOfBools = new boolean[][] { > {true, false, true}, {false}, {false}, {false}, {false} }; Done. https://chromiumcodereview.appspot.com/317273006/diff/40001/mojo/android/java... mojo/android/javatests/src/org/chromium/mojo/bindings/BindingsTest.java:81: foo.extraBars = bars; On 2014/06/25 13:14:38, rmcilroy wrote: > similarly: > foo.extraBars = new Bar[] { newBar(), newBar() }; Done. https://chromiumcodereview.appspot.com/317273006/diff/40001/mojo/android/java... mojo/android/javatests/src/org/chromium/mojo/bindings/BindingsTest.java:234: // Checking creation of a Foo object. On 2014/06/25 13:14:38, rmcilroy wrote: > Checking serialization and deserialization of a Foo object. Done. https://chromiumcodereview.appspot.com/317273006/diff/40001/mojo/bindings/jav... File mojo/bindings/java/src/org/chromium/mojo/bindings/BindingsHelper.java (right): https://chromiumcodereview.appspot.com/317273006/diff/40001/mojo/bindings/jav... mojo/bindings/java/src/org/chromium/mojo/bindings/BindingsHelper.java:12: * Alignment in byte for mojo serialization. On 2014/06/25 13:14:38, rmcilroy wrote: > /s/byte/bytes Done. https://chromiumcodereview.appspot.com/317273006/diff/40001/mojo/bindings/jav... File mojo/bindings/java/src/org/chromium/mojo/bindings/Decoder.java (right): https://chromiumcodereview.appspot.com/317273006/diff/40001/mojo/bindings/jav... mojo/bindings/java/src/org/chromium/mojo/bindings/Decoder.java:111: int size = readInt(0); On 2014/06/25 13:14:38, rmcilroy wrote: > nit - extract '0' and '4' as constants in DataHeader and use them here (and in > encoder) Done. https://chromiumcodereview.appspot.com/317273006/diff/40001/mojo/bindings/jav... File mojo/bindings/java/src/org/chromium/mojo/bindings/DeserializationException.java (right): https://chromiumcodereview.appspot.com/317273006/diff/40001/mojo/bindings/jav... mojo/bindings/java/src/org/chromium/mojo/bindings/DeserializationException.java:8: * Error when deserializing a mojo archive. On 2014/06/25 13:14:39, rmcilroy wrote: > "archive" doesn't seem to be a name used to describe seriailized mojo messages > AFAIKT (and implies writing to disk to me). Would "Represents an error when > deserializing a mojo message."? Archive is the name we use in the public documentation: https://docs.google.com/a/google.com/document/d/1jNcsxOdO3Al52s6lIrMOOgY7KXB7... But changed anyway as this is not used anywhere else in the code. https://chromiumcodereview.appspot.com/317273006/diff/40001/mojo/bindings/jav... mojo/bindings/java/src/org/chromium/mojo/bindings/DeserializationException.java:13: * Constructor. On 2014/06/25 13:14:39, rmcilroy wrote: > More informative javadoc would be nice. Done. https://chromiumcodereview.appspot.com/317273006/diff/40001/mojo/bindings/jav... mojo/bindings/java/src/org/chromium/mojo/bindings/DeserializationException.java:20: * Constructor. On 2014/06/25 13:14:38, rmcilroy wrote: > ditto Done. https://chromiumcodereview.appspot.com/317273006/diff/40001/mojo/bindings/jav... File mojo/bindings/java/src/org/chromium/mojo/bindings/Encoder.java (right): https://chromiumcodereview.appspot.com/317273006/diff/40001/mojo/bindings/jav... mojo/bindings/java/src/org/chromium/mojo/bindings/Encoder.java:37: private static class EncoderInformation { On 2014/06/25 13:14:39, rmcilroy wrote: > Maybe EncoderState (and update javadoc) is more understandable? Also, move > above to top of class to avoid splitting Encoder's field definitions. Done. https://chromiumcodereview.appspot.com/317273006/diff/40001/mojo/bindings/jav... mojo/bindings/java/src/org/chromium/mojo/bindings/Encoder.java:40: * The core to encode interfaces. On 2014/06/25 13:14:39, rmcilroy wrote: > The core used to encode interfaces. Done. https://chromiumcodereview.appspot.com/317273006/diff/40001/mojo/bindings/jav... mojo/bindings/java/src/org/chromium/mojo/bindings/Encoder.java:45: * The ByteBuffer to write to. On 2014/06/25 13:14:39, rmcilroy wrote: > The ByteBuffer to which the message will be encoded. Done. https://chromiumcodereview.appspot.com/317273006/diff/40001/mojo/bindings/jav... mojo/bindings/java/src/org/chromium/mojo/bindings/Encoder.java:60: * Constructor. On 2014/06/25 13:14:39, rmcilroy wrote: > javadoc parameters Done. https://chromiumcodereview.appspot.com/317273006/diff/40001/mojo/bindings/jav... mojo/bindings/java/src/org/chromium/mojo/bindings/Encoder.java:135: * Encoder a {@link DataHeader}. Also resize the buffer to allow writing the data section if On 2014/06/25 13:14:39, rmcilroy wrote: > Encoder a {@link DataHeader} and claim the amount of memory required for the > data section (resizing the buffer if required). Done. https://chromiumcodereview.appspot.com/317273006/diff/40001/mojo/bindings/jav... mojo/bindings/java/src/org/chromium/mojo/bindings/Encoder.java:139: mEncoderInformation.dataEnd = mEncoderInformation.dataEnd + s.size; On 2014/06/25 13:14:39, rmcilroy wrote: > I think it makes more sense to update dataEnd and growIfNeeded in one method. > Maybe something like: > mEncoderInformation.claimMemory(s.size); > Also make dataEnd private and add a method called something like > getNextUnclaimedDataOffset() for where you need to read dataEnd. Added claim memory. But kept dataEnd as is, as this is still mostly a state class. https://chromiumcodereview.appspot.com/317273006/diff/40001/mojo/bindings/jav... mojo/bindings/java/src/org/chromium/mojo/bindings/Encoder.java:206: encode((long) mEncoderInformation.dataEnd - (mBaseOffset + offset), offset); On 2014/06/25 13:14:39, rmcilroy wrote: > maybe pull this out into a private method > "encodedPointerToNextUnclaimedData(offset)" to be used here and in array > serialization? Done. https://chromiumcodereview.appspot.com/317273006/diff/40001/mojo/bindings/jav... mojo/bindings/java/src/org/chromium/mojo/bindings/Encoder.java:207: Encoder e = new Encoder(mEncoderInformation); On 2014/06/25 13:14:39, rmcilroy wrote: > This is a bit confusing that we end up creating two Encoder's when encoding a > struct (one here and one when struct.encode() calls getEncoderAtDataOffset). > Could we avoid the extra encoder creation here and just pass the "this" to > v.encode()? Yes, this was a bug. Thanks. https://chromiumcodereview.appspot.com/317273006/diff/40001/mojo/bindings/jav... mojo/bindings/java/src/org/chromium/mojo/bindings/Encoder.java:226: if (v != null && v.isValid()) { On 2014/06/25 13:14:39, rmcilroy wrote: > nit - do this check the other way around to be consistent with the "if (v == > null) {" check's above. Done. https://chromiumcodereview.appspot.com/317273006/diff/40001/mojo/bindings/jav... mojo/bindings/java/src/org/chromium/mojo/bindings/Encoder.java:262: return encoderForArray(8, length, offset); On 2014/06/25 13:14:39, rmcilroy wrote: > nit - use constants for element sizes Done. https://chromiumcodereview.appspot.com/317273006/diff/40001/mojo/bindings/jav... mojo/bindings/java/src/org/chromium/mojo/bindings/Encoder.java:409: e.encode(new DataHeader(DataHeader.HEADER_SIZE + BindingsHelper.align(byteSize), length)); On 2014/06/25 13:14:39, rmcilroy wrote: > nit - instead of the above two lines, can you just use: > Encoder arrayEncoder = getEncoderAtDataOffset(new > DataHeader(DataHeader.HEADER_SIZE + BindingsHelper.align(byteSize), length)); Done. https://chromiumcodereview.appspot.com/317273006/diff/40001/mojo/not-to-commi... File mojo/not-to-commit/mojo_public_test_interfaces/src/org/chromium/mojo/bindings/test/mojom/imported/Size.java (right): https://chromiumcodereview.appspot.com/317273006/diff/40001/mojo/not-to-commi... mojo/not-to-commit/mojo_public_test_interfaces/src/org/chromium/mojo/bindings/test/mojom/imported/Size.java:32: if (0 < mainDataHeader.numFields) { On 2014/06/25 13:14:39, rmcilroy wrote: > nit - "if (mainDataHeader.numFields > 0)" Done. https://chromiumcodereview.appspot.com/317273006/diff/40001/mojo/not-to-commi... mojo/not-to-commit/mojo_public_test_interfaces/src/org/chromium/mojo/bindings/test/mojom/imported/Size.java:33: result.width = decoder0.readInt(8); On 2014/06/25 13:14:39, rmcilroy wrote: > Could you pull the offset's out as private static final constants at the to of > the generated struct, and use them both in decode and encode? I don't think it is a good idea. This is a generated file, there is no risk of wrong typing a constant. Moreover, this port of the code is mostly write only, there is nothing interesting here. And having the constants will clutter this file for nothing. To be frank, I'm not even sure having extracted the size is really useful. https://chromiumcodereview.appspot.com/317273006/diff/40001/mojo/not-to-commi... File mojo/not-to-commit/mojo_public_test_interfaces/src/org/chromium/mojo/bindings/test/mojom/imported/Thing.java (right): https://chromiumcodereview.appspot.com/317273006/diff/40001/mojo/not-to-commi... mojo/not-to-commit/mojo_public_test_interfaces/src/org/chromium/mojo/bindings/test/mojom/imported/Thing.java:43: org.chromium.mojo.bindings.Decoder decoder1 = decoder0.readPointer(16); On 2014/06/25 13:14:39, rmcilroy wrote: > Could you add the null check at the top of struct's decode() method (and return > null if decoder is null) then you could just do: > org.chromium.mojo.bindings.Decoder decoder1 = decoder0.readPointer(16); > result.location = Point.decode(decoder1); Given that we will still have this kind of construction for arrays, do you think it is worth it changing it for structs? https://chromiumcodereview.appspot.com/317273006/diff/40001/mojo/not-to-commi... File mojo/not-to-commit/mojo_public_test_interfaces/src/org/chromium/mojo/bindings/test/mojom/test_structs/NamedRegion.java (right): https://chromiumcodereview.appspot.com/317273006/diff/40001/mojo/not-to-commi... mojo/not-to-commit/mojo_public_test_interfaces/src/org/chromium/mojo/bindings/test/mojom/test_structs/NamedRegion.java:64: encoder1.encode(rects[i0], 8 + 8 * i0); On 2014/06/25 13:14:39, rmcilroy wrote: > constants here (element size and DataHeader.HEADER_SIZE) Ok, but this will then be ugly. We now have a big FQN. https://chromiumcodereview.appspot.com/317273006/diff/40001/mojo/public/tools... File mojo/public/tools/bindings/generators/java_templates/struct_definition.tmpl (right): https://chromiumcodereview.appspot.com/317273006/diff/40001/mojo/public/tools... mojo/public/tools/bindings/generators/java_templates/struct_definition.tmpl:28: {% else %} On 2014/06/25 13:14:39, rmcilroy wrote: > comment that kind|is_pointer_array_kind. Done. https://chromiumcodereview.appspot.com/317273006/diff/40001/mojo/public/tools... File mojo/public/tools/bindings/generators/mojom_java_generator.py (right): https://chromiumcodereview.appspot.com/317273006/diff/40001/mojo/public/tools... mojo/public/tools/bindings/generators/mojom_java_generator.py:149: additionalParams = ', %s.BUILDER' % GetJavaType(context, interfaceKind) On 2014/06/25 13:14:40, rmcilroy wrote: > I think this would be clearer as: > if isinstance(kind, mojom.Interface: > additionalParams = ', %s.BUILDER' % GetJavaType(context, kind) > if (IsArray(kind) and isinstance(kind.kind, mojom.Interface)): > additionalParams = ', %s.BUILDER' % GetJavaType(context, kind.kind) > Done. https://chromiumcodereview.appspot.com/317273006/diff/40001/mojo/public/tools... mojo/public/tools/bindings/generators/mojom_java_generator.py:163: additionalParams = ', %s.BUILDER' % GetJavaType(context, interfaceKind) On 2014/06/25 13:14:40, rmcilroy wrote: > ditto Done.
https://chromiumcodereview.appspot.com/317273006/diff/40001/mojo/not-to-commi... File mojo/not-to-commit/mojo_public_test_interfaces/src/org/chromium/mojo/bindings/test/mojom/imported/Size.java (right): https://chromiumcodereview.appspot.com/317273006/diff/40001/mojo/not-to-commi... mojo/not-to-commit/mojo_public_test_interfaces/src/org/chromium/mojo/bindings/test/mojom/imported/Size.java:33: result.width = decoder0.readInt(8); On 2014/06/25 14:43:05, qsr wrote: > On 2014/06/25 13:14:39, rmcilroy wrote: > > Could you pull the offset's out as private static final constants at the to of > > the generated struct, and use them both in decode and encode? > > I don't think it is a good idea. This is a generated file, there is no risk of > wrong typing a constant. Moreover, this port of the code is mostly write only, > there is nothing interesting here. And having the constants will clutter this > file for nothing. > > To be frank, I'm not even sure having extracted the size is really useful. Frankly I think the exposed use of offsets at all is not ideal. Given that, as far as I can tell, you only ever encode/decode objects in-order on a given encoder/decoder object, do you really need to explicitly pass the offsets? Could you not simply keep track of the current offset in the encoder / decoder object rather than explicitly in the generated code? https://chromiumcodereview.appspot.com/317273006/diff/40001/mojo/not-to-commi... File mojo/not-to-commit/mojo_public_test_interfaces/src/org/chromium/mojo/bindings/test/mojom/imported/Thing.java (right): https://chromiumcodereview.appspot.com/317273006/diff/40001/mojo/not-to-commi... mojo/not-to-commit/mojo_public_test_interfaces/src/org/chromium/mojo/bindings/test/mojom/imported/Thing.java:43: org.chromium.mojo.bindings.Decoder decoder1 = decoder0.readPointer(16); On 2014/06/25 14:43:05, qsr wrote: > On 2014/06/25 13:14:39, rmcilroy wrote: > > Could you add the null check at the top of struct's decode() method (and > return > > null if decoder is null) then you could just do: > > org.chromium.mojo.bindings.Decoder decoder1 = decoder0.readPointer(16); > > result.location = Point.decode(decoder1); > > Given that we will still have this kind of construction for arrays, do you think > it is worth it changing it for structs? Yes I think it would be worth it, particularly since array construction will often embed struct generation of elements, so this would at least reduce the nesting there too. https://chromiumcodereview.appspot.com/317273006/diff/40001/mojo/not-to-commi... File mojo/not-to-commit/mojo_public_test_interfaces/src/org/chromium/mojo/bindings/test/mojom/test_structs/NamedRegion.java (right): https://chromiumcodereview.appspot.com/317273006/diff/40001/mojo/not-to-commi... mojo/not-to-commit/mojo_public_test_interfaces/src/org/chromium/mojo/bindings/test/mojom/test_structs/NamedRegion.java:64: encoder1.encode(rects[i0], 8 + 8 * i0); On 2014/06/25 14:43:05, qsr wrote: > On 2014/06/25 13:14:39, rmcilroy wrote: > > constants here (element size and DataHeader.HEADER_SIZE) > > Ok, but this will then be ugly. We now have a big FQN. Yes it's a bit uglier, but at least it's obvious what the numbers represent. Thanks https://chromiumcodereview.appspot.com/317273006/diff/60001/mojo/android/java... File mojo/android/javatests/src/org/chromium/mojo/bindings/BindingsTest.java (right): https://chromiumcodereview.appspot.com/317273006/diff/60001/mojo/android/java... mojo/android/javatests/src/org/chromium/mojo/bindings/BindingsTest.java:230: System.err.println(serializedFoo.buffer.getInt(4)); Leftover debug code? https://chromiumcodereview.appspot.com/317273006/diff/60001/mojo/bindings/jav... File mojo/bindings/java/src/org/chromium/mojo/bindings/Encoder.java (right): https://chromiumcodereview.appspot.com/317273006/diff/60001/mojo/bindings/jav... mojo/bindings/java/src/org/chromium/mojo/bindings/Encoder.java:419: assert mEncoderState.dataEnd != -1; I'm not sure why this assert is required here but nowhere else?
https://chromiumcodereview.appspot.com/317273006/diff/40001/mojo/not-to-commi... File mojo/not-to-commit/mojo_public_test_interfaces/src/org/chromium/mojo/bindings/test/mojom/imported/Size.java (right): https://chromiumcodereview.appspot.com/317273006/diff/40001/mojo/not-to-commi... mojo/not-to-commit/mojo_public_test_interfaces/src/org/chromium/mojo/bindings/test/mojom/imported/Size.java:33: result.width = decoder0.readInt(8); On 2014/06/26 17:10:25, rmcilroy wrote: > On 2014/06/25 14:43:05, qsr wrote: > > On 2014/06/25 13:14:39, rmcilroy wrote: > > > Could you pull the offset's out as private static final constants at the to > of > > > the generated struct, and use them both in decode and encode? > > > > I don't think it is a good idea. This is a generated file, there is no risk > of > > wrong typing a constant. Moreover, this port of the code is mostly write only, > > there is nothing interesting here. And having the constants will clutter this > > file for nothing. > > > > To be frank, I'm not even sure having extracted the size is really useful. > > Frankly I think the exposed use of offsets at all is not ideal. Given that, as > far as I can tell, you only ever encode/decode objects in-order on a given > encoder/decoder object, do you really need to explicitly pass the offsets? > Could you not simply keep track of the current offset in the encoder / decoder > object rather than explicitly in the generated code? We do encode/decode in order, but we have to handle holes. see: https://chromiumcodereview.appspot.com/317273006/diff/60001/mojo/not-to-commi... https://chromiumcodereview.appspot.com/317273006/diff/60001/mojo/not-to-commi... The first one has a hole, the second one if filling it with an int. Keeping track of the offset makes it harder in the encoder/decoder, because now, I need to either keep track of alignement, or skip part in the generated code. Skipping makes it also hard on the decoding size, because depending on the version I might need to either skip, or read a data at a certain time. And I'm not even speaking of boolean that must be packed. All in hole, I started with the state in the Encoder/Decoder, and changed it that way because it makes both the generation and the Encoder/Decoder simpler. I don't see any issue with offset in the generated code. This part of the code is of no interest for anymore. If it hurts your eyes, I can create an package private internal class and push this code there. But that means more method and more classes while I don't think having those 2 methods is such a problem. https://chromiumcodereview.appspot.com/317273006/diff/60001/mojo/android/java... File mojo/android/javatests/src/org/chromium/mojo/bindings/BindingsTest.java (right): https://chromiumcodereview.appspot.com/317273006/diff/60001/mojo/android/java... mojo/android/javatests/src/org/chromium/mojo/bindings/BindingsTest.java:230: System.err.println(serializedFoo.buffer.getInt(4)); On 2014/06/26 17:10:26, rmcilroy wrote: > Leftover debug code? Yes. Thanks. https://chromiumcodereview.appspot.com/317273006/diff/60001/mojo/bindings/jav... File mojo/bindings/java/src/org/chromium/mojo/bindings/Encoder.java (right): https://chromiumcodereview.appspot.com/317273006/diff/60001/mojo/bindings/jav... mojo/bindings/java/src/org/chromium/mojo/bindings/Encoder.java:419: assert mEncoderState.dataEnd != -1; On 2014/06/26 17:10:26, rmcilroy wrote: > I'm not sure why this assert is required here but nowhere else? Neither do I. Removed. Thanks.
gentle ping?
https://chromiumcodereview.appspot.com/317273006/diff/40001/mojo/not-to-commi... File mojo/not-to-commit/mojo_public_test_interfaces/src/org/chromium/mojo/bindings/test/mojom/imported/Size.java (right): https://chromiumcodereview.appspot.com/317273006/diff/40001/mojo/not-to-commi... mojo/not-to-commit/mojo_public_test_interfaces/src/org/chromium/mojo/bindings/test/mojom/imported/Size.java:33: result.width = decoder0.readInt(8); On 2014/06/27 12:41:47, qsr wrote: > On 2014/06/26 17:10:25, rmcilroy wrote: > > On 2014/06/25 14:43:05, qsr wrote: > > > On 2014/06/25 13:14:39, rmcilroy wrote: > > > > Could you pull the offset's out as private static final constants at the > to > > of > > > > the generated struct, and use them both in decode and encode? > > > > > > I don't think it is a good idea. This is a generated file, there is no risk > > of > > > wrong typing a constant. Moreover, this port of the code is mostly write > only, > > > there is nothing interesting here. And having the constants will clutter > this > > > file for nothing. > > > > > > To be frank, I'm not even sure having extracted the size is really useful. > > > > Frankly I think the exposed use of offsets at all is not ideal. Given that, as > > far as I can tell, you only ever encode/decode objects in-order on a given > > encoder/decoder object, do you really need to explicitly pass the offsets? > > Could you not simply keep track of the current offset in the encoder / decoder > > object rather than explicitly in the generated code? > > We do encode/decode in order, but we have to handle holes. > see: > https://chromiumcodereview.appspot.com/317273006/diff/60001/mojo/not-to-commi... > https://chromiumcodereview.appspot.com/317273006/diff/60001/mojo/not-to-commi... > > The first one has a hole, the second one if filling it with an int. > > Keeping track of the offset makes it harder in the encoder/decoder, because > now, I need to either keep track of alignement, or skip part in the generated > code. Skipping makes it also hard on the decoding size, because depending on the > version I might need to either skip, or read a data at a certain time. > > And I'm not even speaking of boolean that must be packed. > > All in hole, I started with the state in the Encoder/Decoder, and changed it > that way because it makes both the generation and the Encoder/Decoder simpler. > > I don't see any issue with offset in the generated code. This part of the code > is of no interest for anymore. > > If it hurts your eyes, I can create an package private internal class and push > this code there. But that means more method and more classes while I don't think > having those 2 methods is such a problem. I don't appreciate the sarcasm here - yes this is generated code, but I am reviewing a CL which generates this code, and other non-generated code will have to interact both with the code and the Encoder/Decoder classes, so I don't think it is unreasonable to discuss design alternatives for the code. In any case, I understand the issue with the holes / alignment and see how that could make things tricky, however, both the C++ and JS bindings generator's do manage to do this without raw offsets. C++ seems to use raw structs AFAICT (which we obviously can't do in Java), however, the JS bindings generator uses an encoder which keeps track of the current offset, and provides operations to skip holes. Personally I think this would be a lot cleaner, and I really think it makes a lot of sense to share as similar a design to other language binding generators as possible. I also really like the way the JS bindings decode multi-dimensional arrays, e.g.: val.multi_array_of_strings = decoder.decodeArrayPointer(new codec.ArrayOf(new codec.ArrayOf(codec.String))); I'm sure with a bit of work we could use the same type of approach for the Java bindings, would this not be possible in Java? I don't want to block the work your doing here, but I really think there is an opportunity to maintain a design which is much more consistent with the other bindings generators. If you disagree, and if the other Mojo owners are fine with the approach you are taking here, then feel free to bypass my review - I'm not an OWNER here so all my comments are just advice in any case. https://chromiumcodereview.appspot.com/317273006/diff/40001/mojo/not-to-commi... File mojo/not-to-commit/mojo_public_test_interfaces/src/org/chromium/mojo/bindings/test/mojom/imported/Thing.java (right): https://chromiumcodereview.appspot.com/317273006/diff/40001/mojo/not-to-commi... mojo/not-to-commit/mojo_public_test_interfaces/src/org/chromium/mojo/bindings/test/mojom/imported/Thing.java:43: org.chromium.mojo.bindings.Decoder decoder1 = decoder0.readPointer(16); On 2014/06/26 17:10:25, rmcilroy wrote: > On 2014/06/25 14:43:05, qsr wrote: > > On 2014/06/25 13:14:39, rmcilroy wrote: > > > Could you add the null check at the top of struct's decode() method (and > > return > > > null if decoder is null) then you could just do: > > > org.chromium.mojo.bindings.Decoder decoder1 = decoder0.readPointer(16); > > > result.location = Point.decode(decoder1); > > > > Given that we will still have this kind of construction for arrays, do you > think > > it is worth it changing it for structs? > > Yes I think it would be worth it, particularly since array construction will > often embed struct generation of elements, so this would at least reduce the > nesting there too. Ping? https://chromiumcodereview.appspot.com/317273006/diff/100002/build/android/fi... File build/android/findbugs_filter/findbugs_exclude.xml (left): https://chromiumcodereview.appspot.com/317273006/diff/100002/build/android/fi... build/android/findbugs_filter/findbugs_exclude.xml:107: Ignore mojo generated files. Is there any way you can tweak the generator to avoid this exclude? If not, please add to the comment a clear explanation of why you are adding this exclude, and why it's safe.
On 2014/07/01 15:53:46, rmcilroy wrote: > https://chromiumcodereview.appspot.com/317273006/diff/40001/mojo/not-to-commi... > File > mojo/not-to-commit/mojo_public_test_interfaces/src/org/chromium/mojo/bindings/test/mojom/imported/Size.java > (right): > > https://chromiumcodereview.appspot.com/317273006/diff/40001/mojo/not-to-commi... > mojo/not-to-commit/mojo_public_test_interfaces/src/org/chromium/mojo/bindings/test/mojom/imported/Size.java:33: > result.width = decoder0.readInt(8); > On 2014/06/27 12:41:47, qsr wrote: > > On 2014/06/26 17:10:25, rmcilroy wrote: > > > On 2014/06/25 14:43:05, qsr wrote: > > > > On 2014/06/25 13:14:39, rmcilroy wrote: > > > > > Could you pull the offset's out as private static final constants at the > > to > > > of > > > > > the generated struct, and use them both in decode and encode? > > > > > > > > I don't think it is a good idea. This is a generated file, there is no > risk > > > of > > > > wrong typing a constant. Moreover, this port of the code is mostly write > > only, > > > > there is nothing interesting here. And having the constants will clutter > > this > > > > file for nothing. > > > > > > > > To be frank, I'm not even sure having extracted the size is really > useful. > > > > > > Frankly I think the exposed use of offsets at all is not ideal. Given that, > as > > > far as I can tell, you only ever encode/decode objects in-order on a given > > > encoder/decoder object, do you really need to explicitly pass the offsets? > > > Could you not simply keep track of the current offset in the encoder / > decoder > > > object rather than explicitly in the generated code? > > > > We do encode/decode in order, but we have to handle holes. > > see: > > > https://chromiumcodereview.appspot.com/317273006/diff/60001/mojo/not-to-commi... > > > https://chromiumcodereview.appspot.com/317273006/diff/60001/mojo/not-to-commi... > > > > The first one has a hole, the second one if filling it with an int. > > > > Keeping track of the offset makes it harder in the encoder/decoder, because > > now, I need to either keep track of alignement, or skip part in the generated > > code. Skipping makes it also hard on the decoding size, because depending on > the > > version I might need to either skip, or read a data at a certain time. > > > > And I'm not even speaking of boolean that must be packed. > > > > All in hole, I started with the state in the Encoder/Decoder, and changed it > > that way because it makes both the generation and the Encoder/Decoder simpler. > > > > I don't see any issue with offset in the generated code. This part of the > code > > is of no interest for anymore. > > > > If it hurts your eyes, I can create an package private internal class and > push > > this code there. But that means more method and more classes while I don't > think > > having those 2 methods is such a problem. > > I don't appreciate the sarcasm here - yes this is generated code, but I am > reviewing a CL which generates this code, and other non-generated code will have > to interact both with the code and the Encoder/Decoder classes, so I don't think > it is unreasonable to discuss design alternatives for the code. I'm really sorry. I didn't mean to be sarcastic, and my choice of words was very wrong. The suggestion was not sarcastic. I agree with you that people will look at this code to find out how to use it. But I think that the serialization/deserialization is not the part that is interesting for viewers, and that pushing it out might ease the reading at the price of a new class... > > In any case, I understand the issue with the holes / alignment and see how that > could make things tricky, however, both the C++ and JS bindings generator's do > manage to do this without raw offsets. C++ seems to use raw structs AFAICT > (which we obviously can't do in Java), however, the JS bindings generator uses > an encoder which keeps track of the current offset, and provides operations to > skip holes. Personally I think this would be a lot cleaner, and I really think > it makes a lot of sense to share as similar a design to other language binding > generators as possible. The main difference is that none of the other generator handle decoding for a different version than the current one. If you add this, it makes it very cumbersome to keep track of the information in the decoder. > > I also really like the way the JS bindings decode multi-dimensional arrays, > e.g.: > val.multi_array_of_strings = decoder.decodeArrayPointer(new codec.ArrayOf(new > codec.ArrayOf(codec.String))); > I'm sure with a bit of work we could use the same type of approach for the Java > bindings, would this not be possible in Java? I didn't look into this, but yes, I think we can make this work, if we are ready to use reflection to create arrays. I had tried not to use reflection any place. Do you think it is worth using it there? > > I don't want to block the work your doing here, but I really think there is an > opportunity to maintain a design which is much more consistent with the other > bindings generators. If you disagree, and if the other Mojo owners are fine > with the approach you are taking here, then feel free to bypass my review - I'm > not an OWNER here so all my comments are just advice in any case. > > https://chromiumcodereview.appspot.com/317273006/diff/40001/mojo/not-to-commi... > File > mojo/not-to-commit/mojo_public_test_interfaces/src/org/chromium/mojo/bindings/test/mojom/imported/Thing.java > (right): > > https://chromiumcodereview.appspot.com/317273006/diff/40001/mojo/not-to-commi... > mojo/not-to-commit/mojo_public_test_interfaces/src/org/chromium/mojo/bindings/test/mojom/imported/Thing.java:43: > org.chromium.mojo.bindings.Decoder decoder1 = decoder0.readPointer(16); > On 2014/06/26 17:10:25, rmcilroy wrote: > > On 2014/06/25 14:43:05, qsr wrote: > > > On 2014/06/25 13:14:39, rmcilroy wrote: > > > > Could you add the null check at the top of struct's decode() method (and > > > return > > > > null if decoder is null) then you could just do: > > > > org.chromium.mojo.bindings.Decoder decoder1 = decoder0.readPointer(16); > > > > result.location = Point.decode(decoder1); > > > > > > Given that we will still have this kind of construction for arrays, do you > > think > > > it is worth it changing it for structs? > > > > Yes I think it would be worth it, particularly since array construction will > > often embed struct generation of elements, so this would at least reduce the > > nesting there too. > > Ping? > > https://chromiumcodereview.appspot.com/317273006/diff/100002/build/android/fi... > File build/android/findbugs_filter/findbugs_exclude.xml (left): > > https://chromiumcodereview.appspot.com/317273006/diff/100002/build/android/fi... > build/android/findbugs_filter/findbugs_exclude.xml:107: Ignore mojo generated > files. > Is there any way you can tweak the generator to avoid this exclude? If not, > please add to the comment a clear explanation of why you are adding this > exclude, and why it's safe. I'm not adding this exclude -> I'm removing it. I had to add it when the struct were generated without the serialization because findbugs was complaining about naked field that are not used. Now that we used those in the serialization/deserialization I can remove this temporary exclusion.
On 2014/07/01 16:06:41, qsr wrote: > On 2014/07/01 15:53:46, rmcilroy wrote: > > > https://chromiumcodereview.appspot.com/317273006/diff/40001/mojo/not-to-commi... > > File > > > mojo/not-to-commit/mojo_public_test_interfaces/src/org/chromium/mojo/bindings/test/mojom/imported/Size.java > > (right): > > > > > https://chromiumcodereview.appspot.com/317273006/diff/40001/mojo/not-to-commi... > > > mojo/not-to-commit/mojo_public_test_interfaces/src/org/chromium/mojo/bindings/test/mojom/imported/Size.java:33: > > result.width = decoder0.readInt(8); > > On 2014/06/27 12:41:47, qsr wrote: > > > On 2014/06/26 17:10:25, rmcilroy wrote: > > > > On 2014/06/25 14:43:05, qsr wrote: > > > > > On 2014/06/25 13:14:39, rmcilroy wrote: > > > > > > Could you pull the offset's out as private static final constants at > the > > > to > > > > of > > > > > > the generated struct, and use them both in decode and encode? > > > > > > > > > > I don't think it is a good idea. This is a generated file, there is no > > risk > > > > of > > > > > wrong typing a constant. Moreover, this port of the code is mostly write > > > only, > > > > > there is nothing interesting here. And having the constants will clutter > > > this > > > > > file for nothing. > > > > > > > > > > To be frank, I'm not even sure having extracted the size is really > > useful. > > > > > > > > Frankly I think the exposed use of offsets at all is not ideal. Given > that, > > as > > > > far as I can tell, you only ever encode/decode objects in-order on a given > > > > encoder/decoder object, do you really need to explicitly pass the offsets? > > > > > Could you not simply keep track of the current offset in the encoder / > > decoder > > > > object rather than explicitly in the generated code? > > > > > > We do encode/decode in order, but we have to handle holes. > > > see: > > > > > > https://chromiumcodereview.appspot.com/317273006/diff/60001/mojo/not-to-commi... > > > > > > https://chromiumcodereview.appspot.com/317273006/diff/60001/mojo/not-to-commi... > > > > > > The first one has a hole, the second one if filling it with an int. > > > > > > Keeping track of the offset makes it harder in the encoder/decoder, because > > > now, I need to either keep track of alignement, or skip part in the > generated > > > code. Skipping makes it also hard on the decoding size, because depending on > > the > > > version I might need to either skip, or read a data at a certain time. > > > > > > And I'm not even speaking of boolean that must be packed. > > > > > > All in hole, I started with the state in the Encoder/Decoder, and changed > it > > > that way because it makes both the generation and the Encoder/Decoder > simpler. > > > > > > I don't see any issue with offset in the generated code. This part of the > > code > > > is of no interest for anymore. > > > > > > If it hurts your eyes, I can create an package private internal class and > > push > > > this code there. But that means more method and more classes while I don't > > think > > > having those 2 methods is such a problem. > > > > I don't appreciate the sarcasm here - yes this is generated code, but I am > > reviewing a CL which generates this code, and other non-generated code will > have > > to interact both with the code and the Encoder/Decoder classes, so I don't > think > > it is unreasonable to discuss design alternatives for the code. > > I'm really sorry. I didn't mean to be sarcastic, and my choice of words was > very wrong. The suggestion was not sarcastic. I agree with you that people will > look at this code to find out how to use it. But I think that the > serialization/deserialization is not the part that is interesting for viewers, > and that pushing it out might ease the reading at the price of a new class... That's ok, I understand that tone is difficult to convey in a code review. > > In any case, I understand the issue with the holes / alignment and see how > that > > could make things tricky, however, both the C++ and JS bindings generator's do > > manage to do this without raw offsets. C++ seems to use raw structs AFAICT > > (which we obviously can't do in Java), however, the JS bindings generator uses > > an encoder which keeps track of the current offset, and provides operations to > > skip holes. Personally I think this would be a lot cleaner, and I really > think > > it makes a lot of sense to share as similar a design to other language binding > > generators as possible. > > The main difference is that none of the other generator handle decoding for a > different version than the current one. If you add this, it makes it very > cumbersome to keep track of the information in the decoder. Is there a reason that we need to support this in Java bindings generator when the other bindings don't support it yet? I would rather just reach parity with the other bindings generators before we start adding new features (maybe this feature will have been completely re-thought by the time the C++ and JS bindings implement it). > > I also really like the way the JS bindings decode multi-dimensional arrays, > > e.g.: > > val.multi_array_of_strings = decoder.decodeArrayPointer(new > codec.ArrayOf(new > > codec.ArrayOf(codec.String))); > > I'm sure with a bit of work we could use the same type of approach for the > Java > > bindings, would this not be possible in Java? > > I didn't look into this, but yes, I think we can make this work, if we are > ready to use reflection to create arrays. I had tried not to use reflection any > place. Do you think it is worth using it there? If you were thinking of Arrays.newInstance, then I think it's use is justified here (and could wouldn't have a noticeable perf impact). WDYT? It would remove a fair bit of complexity from both the generated code and the Encoder/Decoder objects I think. > > I don't want to block the work your doing here, but I really think there is an > > opportunity to maintain a design which is much more consistent with the other > > bindings generators. If you disagree, and if the other Mojo owners are fine > > with the approach you are taking here, then feel free to bypass my review - > I'm > > not an OWNER here so all my comments are just advice in any case. > > > > > https://chromiumcodereview.appspot.com/317273006/diff/40001/mojo/not-to-commi... > > File > > > mojo/not-to-commit/mojo_public_test_interfaces/src/org/chromium/mojo/bindings/test/mojom/imported/Thing.java > > (right): > > > > > https://chromiumcodereview.appspot.com/317273006/diff/40001/mojo/not-to-commi... > > > mojo/not-to-commit/mojo_public_test_interfaces/src/org/chromium/mojo/bindings/test/mojom/imported/Thing.java:43: > > org.chromium.mojo.bindings.Decoder decoder1 = decoder0.readPointer(16); > > On 2014/06/26 17:10:25, rmcilroy wrote: > > > On 2014/06/25 14:43:05, qsr wrote: > > > > On 2014/06/25 13:14:39, rmcilroy wrote: > > > > > Could you add the null check at the top of struct's decode() method (and > > > > return > > > > > null if decoder is null) then you could just do: > > > > > org.chromium.mojo.bindings.Decoder decoder1 = > decoder0.readPointer(16); > > > > > result.location = Point.decode(decoder1); > > > > > > > > Given that we will still have this kind of construction for arrays, do you > > > think > > > > it is worth it changing it for structs? > > > > > > Yes I think it would be worth it, particularly since array construction will > > > often embed struct generation of elements, so this would at least reduce the > > > nesting there too. > > > > Ping? > > > > > https://chromiumcodereview.appspot.com/317273006/diff/100002/build/android/fi... > > File build/android/findbugs_filter/findbugs_exclude.xml (left): > > > > > https://chromiumcodereview.appspot.com/317273006/diff/100002/build/android/fi... > > build/android/findbugs_filter/findbugs_exclude.xml:107: Ignore mojo generated > > files. > > Is there any way you can tweak the generator to avoid this exclude? If not, > > please add to the comment a clear explanation of why you are adding this > > exclude, and why it's safe. > > I'm not adding this exclude -> I'm removing it. I had to add it when the struct > were generated without the serialization because findbugs was complaining about > naked field that are not used. Now that we used those in the > serialization/deserialization I can remove this temporary exclusion.
> Is there a reason that we need to support this in Java bindings generator when > the other bindings don't support it yet? I would rather just reach parity with > the other bindings generators before we start adding new features (maybe this > feature will have been completely re-thought by the time the C++ and JS bindings > implement it). The archive format do specify the mechanism. So whether we implement it or not now, I do not want to choose a design that will make it harder to do so. Moreover, it is quite easy to implement and so it made sense to me to implement it. I also spoke with darin@ about this that also seemed to think it was a good idea. Also, about using offset, this came up there: https://codereview.chromium.org/264293003/ when I was trying to implement what you are suggesting. In the end, I now really think that having offset in the generated file makes sense. It simplifies both the generation and the helper classes, and anyway, there would be a hidden knowledge of those offset in the order of the encoding/decoding calls otherwise. > If you were thinking of Arrays.newInstance, then I think it's use is justified > here (and could wouldn't have a noticeable perf impact). WDYT? It would remove > a fair bit of complexity from both the generated code and the Encoder/Decoder > objects I think. First, I'd like us to go forward with this CL as is, and we can go into refactoring it if we want later on, as this is blocking the following ones. Also, using intermediary codec would mean we will need another class hierarchy of object, some of those (any array) would need creating object at request, which mean more class, more methods, more memory pressure. I also think that the js bindings have a bug for array of bools that ends up not being packed. I do not know, at this moment, how to do this with the codec idea, so we would need to special case this. I'm also not sure I will only need Arrays.newInstance, I'll probably need Arrays.get/set because I will have to pass arrays as Object, and will not be able to cast to the right class, because the right class will be a generic. Also, I will need reflection to create things like int[][].class to pass to newInstance. In the end, I do not think it is worth it.
I'll LGTM to unblock you. I still think there is scope to make this a bit cleaner, but if the mojo owners are happy with it then I'm fine with it being submitted as is for now and improved as the bindings evolve. On 2 Jul 2014 08:59, <qsr@chromium.org> wrote: >> >> Is there a reason that we need to support this in Java bindings generator when >> the other bindings don't support it yet? I would rather just reach parity > > with >> >> the other bindings generators before we start adding new features (maybe this >> feature will have been completely re-thought by the time the C++ and JS > > bindings >> >> implement it). > > > The archive format do specify the mechanism. So whether we implement it or not > now, I do not want to choose a design that will make it harder to do so. > Moreover, it is quite easy to implement and so it made sense to me to implement > it. I also spoke with darin@ about this that also seemed to think it was a good > idea. > > Also, about using offset, this came up there: >https://codereview.chromium.org/264293003/when I was trying to implement what > you are suggesting. In the end, I now really think that having offset in the > generated file makes sense. It simplifies both the generation and the helper > classes, and anyway, there would be a hidden knowledge of those offset in the > order of the encoding/decoding calls otherwise. OK, if maintaining the offset in the encoder isn't going to work I'm fine with not doing that (although i still disagree that we should be adding features that are unsupported in the other bindings until we reach feature parity with them). As I stated originally, I don't necessarily have a problem with the offsets being in the generated files, but don't like the fact that they are literals scattered across the class. I think this makes debugging and code understanding more difficult. I would prefer a private static final inner class which has constants for these values, but if you strongly disagree I can live with them. >> If you were thinking of Arrays.newInstance, then I think it's use is justified >> here (and could wouldn't have a noticeable perf impact). WDYT? It would > > remove >> >> a fair bit of complexity from both the generated code and the Encoder/Decoder >> objects I think. > > > First, I'd like us to go forward with this CL as is, and we can go into > refactoring it if we want later on, as this is blocking the following ones. > Also, using intermediary codec would mean we will need another class hierarchy > of object, some of those (any array) would need creating object at request, > which mean more class, more methods, more memory pressure. I also think that the > js bindings have a bug for array of bools that ends up not being packed. I do > not know, at this moment, how to do this with the codec idea, so we would need > to special case this. I'm also not sure I will only need Arrays.newInstance, > I'll probably need Arrays.get/set because I will have to pass arrays as Object, > and will not be able to cast to the right class, because the right class will be > a generic. Also, I will need reflection to create things like int[][].class to > pass to newInstance. In the end, I do not think it is worth it. I think there should be a way to do this without Array.get/set, but yeah, Java Arrays and Generics don't mix too well so I'm fine for leaving this as potential future investigation. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2014/07/02 21:11:51, chromium-reviews wrote: > I'll LGTM to unblock you. I still think there is scope to make this a bit > cleaner, but if the mojo owners are happy with it then I'm fine with it > being submitted as is for now and improved as the bindings evolve. > > On 2 Jul 2014 08:59, <mailto:qsr@chromium.org> wrote: > >> > >> Is there a reason that we need to support this in Java bindings > generator when > >> the other bindings don't support it yet? I would rather just reach > parity > > > > with > >> > >> the other bindings generators before we start adding new features (maybe > this > >> feature will have been completely re-thought by the time the C++ and JS > > > > bindings > >> > >> implement it). > > > > > > The archive format do specify the mechanism. So whether we implement it > or not > > now, I do not want to choose a design that will make it harder to do so. > > Moreover, it is quite easy to implement and so it made sense to me to > implement > > it. I also spoke with darin@ about this that also seemed to think it was > a good > > idea. > > > > Also, about using offset, this came up there: > >https://codereview.chromium.org/264293003/when I was trying to implement > what > > you are suggesting. In the end, I now really think that having offset in > the > > generated file makes sense. It simplifies both the generation and the > helper > > classes, and anyway, there would be a hidden knowledge of those offset in > the > > order of the encoding/decoding calls otherwise. > > OK, if maintaining the offset in the encoder isn't going to work I'm fine > with not doing that (although i still disagree that we should be adding > features that are unsupported in the other bindings until we reach feature > parity with them). > > As I stated originally, I don't necessarily have a problem with the offsets > being in the generated files, but don't like the fact that they are > literals scattered across the class. I think this makes debugging and code > understanding more difficult. I would prefer a private static final inner > class which has constants for these values, but if you strongly disagree I > can live with them. > > >> If you were thinking of Arrays.newInstance, then I think it's use is > justified > >> here (and could wouldn't have a noticeable perf impact). WDYT? It would > > > > remove > >> > >> a fair bit of complexity from both the generated code and the > Encoder/Decoder > >> objects I think. > > > > > > First, I'd like us to go forward with this CL as is, and we can go into > > refactoring it if we want later on, as this is blocking the following > ones. > > Also, using intermediary codec would mean we will need another class > hierarchy > > of object, some of those (any array) would need creating object at > request, > > which mean more class, more methods, more memory pressure. I also think > that the > > js bindings have a bug for array of bools that ends up not being packed. > I do > > not know, at this moment, how to do this with the codec idea, so we would > need > > to special case this. I'm also not sure I will only need > Arrays.newInstance, > > I'll probably need Arrays.get/set because I will have to pass arrays as > Object, > > and will not be able to cast to the right class, because the right class > will be > > a generic. Also, I will need reflection to create things like > int[][].class to > > pass to newInstance. In the end, I do not think it is worth it. > > I think there should be a way to do this without Array.get/set, but yeah, > Java Arrays and Generics don't mix too well so I'm fine for leaving this as > potential future investigation. Hum... At this point I do not see how. And even worst, I feel we would need boxing of all primitive types. For the design used by the js bindings to work, we need to define a generic Codec interface. Then we would implement it for each of the types. It means that this interface has to use generic, which mean using Object as its input/output type, which mean boxing any primitive types. Now for array, we will not be able to use a Object[] as the generic type, because primitive arrays do not extend Object[]. Then we will need to pass an Object, and we would not be able to cast it to the right array type of the same reason. So we would need Array.get/set to access its data. > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org.
On 2014/07/02 21:11:51, chromium-reviews wrote: > I'll L G T M to unblock you. I still think there is scope to make this a bit > cleaner, but if the mojo owners are happy with it then I'm fine with it > being submitted as is for now and improved as the bindings evolve. I guess you answered this by mail and not with the code review tool... The system did not consider this as a real L G T M
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/317273006/150001
Message was sent while issue was closed.
Change committed as 281255 |
