Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
371603003:
Adding a router class to handle messages that expect responses. (Closed)
|
Created:
6 years, 5 months ago by qsr Modified:
6 years, 5 months ago Reviewers:
rmcilroy 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 Base URL:
svn://svn.chromium.org/chrome/trunk/src Project:
chromium Visibility:
Public. |
DescriptionAdding a router class to handle messages that expect responses.
This also introduce the notion of message header to allow routing.
R=rmcilroy@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=283767
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=284170
Patch Set 1 #Patch Set 2 : Add AutoCloseableRouterTest #Patch Set 3 : Fix rebasing #
Total comments: 76
Patch Set 4 : Follow review, adding MessageWithHeader #Patch Set 5 : Fix javadoc #Patch Set 6 : Refactor header validation. #
Total comments: 23
Patch Set 7 : #Patch Set 8 : FOllow review #
Total comments: 20
Patch Set 9 : Follow review #Patch Set 10 : Update javadoc #
Total comments: 2
Patch Set 11 : follow review #Patch Set 12 : rebase #Patch Set 13 : Fix complation after // change #Patch Set 14 : Do not use Objects.equals #Patch Set 15 : Fix findbugs issue #Messages
Total messages: 39 (0 generated)
gentle ping?
https://codereview.chromium.org/371603003/diff/40001/mojo/bindings/java/src/o... File mojo/bindings/java/src/org/chromium/mojo/bindings/AutoCloseableRouter.java (right): https://codereview.chromium.org/371603003/diff/40001/mojo/bindings/java/src/o... mojo/bindings/java/src/org/chromium/mojo/bindings/AutoCloseableRouter.java:32: mExecutor = core == null ? null : ExecutorFactory.getExecutorForCurrentThread(core); I don't think you need the 'core == null' check given that the class will not be useful if null is passed as core, since it will immediately close the object. I would prefer a NPE here if it is used incorrectly rather than a harder to debug error later when the router is unexpectedly closed. https://codereview.chromium.org/371603003/diff/40001/mojo/bindings/java/src/o... mojo/bindings/java/src/org/chromium/mojo/bindings/AutoCloseableRouter.java:100: if (mExecutor != null) { Keep track of whether the router is already closed and only close in finalizer if not already closed. Also log a great big noisy warning message here (and I would also advise to throw an exception in debug builds) because if this is ever reached then the user of the class is doing something wrong. https://codereview.chromium.org/371603003/diff/40001/mojo/bindings/java/src/o... File mojo/bindings/java/src/org/chromium/mojo/bindings/ConnectionErrorHandler.java (right): https://codereview.chromium.org/371603003/diff/40001/mojo/bindings/java/src/o... mojo/bindings/java/src/org/chromium/mojo/bindings/ConnectionErrorHandler.java:11: * message pipes. /s/An/A /s/of error/of an error/ https://codereview.chromium.org/371603003/diff/40001/mojo/bindings/java/src/o... File mojo/bindings/java/src/org/chromium/mojo/bindings/Message.java (right): https://codereview.chromium.org/371603003/diff/40001/mojo/bindings/java/src/o... mojo/bindings/java/src/org/chromium/mojo/bindings/Message.java:70: * to have a header. This will throw a {@link DeserializationException} it the start of the /s/it/if https://codereview.chromium.org/371603003/diff/40001/mojo/bindings/java/src/o... mojo/bindings/java/src/org/chromium/mojo/bindings/Message.java:73: public MessageHeader getHeader() { I'm not sure I like the idea of a Message optionally having header/payload. Could you have a new class - MessageWithHeader, which either extends or wraps Message, and modify acceptWithResponder to take a MessageWithHeader. You can then have a single place where you convert between Message and MessageWithHeader (namely Router.handleIncomingMessage) and ensure type safety the rest of the time. https://codereview.chromium.org/371603003/diff/40001/mojo/bindings/java/src/o... mojo/bindings/java/src/org/chromium/mojo/bindings/Message.java:97: void resetHeader() { Having to remember to resetHeader also seems error prone. If we have a separate class then we could have setRequestId be a method on MessageWithHeader, which could then internally ensure that the cached header/payload is invalidated appropriately, WDYT? https://codereview.chromium.org/371603003/diff/40001/mojo/bindings/java/src/o... File mojo/bindings/java/src/org/chromium/mojo/bindings/MessageHeader.java (right): https://codereview.chromium.org/371603003/diff/40001/mojo/bindings/java/src/o... mojo/bindings/java/src/org/chromium/mojo/bindings/MessageHeader.java:15: private static final int MESSAGE_WITH_REQUEST_ID_SIZE = 24; Group common fields and insert newlines to seperate - e.g,: SIMPLE_MESSAGE_SIZE MESSAGE_WITH_REQUEST_ID_SIZE SIMPLE_MESSAGE_STRUCT_INFO MESSAGE_WITH_REQUEST_ID_SIZE MESSAGE_WITH_REQUEST_ID_NUM_FIELDS MESSAGE_WITH_REQUEST_ID_STRUCT_INFO TYPE_OFFSET FLAGS_OFFSET REQUEST_ID_OFFSET https://codereview.chromium.org/371603003/diff/40001/mojo/bindings/java/src/o... mojo/bindings/java/src/org/chromium/mojo/bindings/MessageHeader.java:18: private static final DataHeader SIMPLE_MESSAGE_STRUCT_INFO = new DataHeader(SIMPLE_MESSAGE_SIZE, nit - newline at "new" instead of SIMPLE_MESSAGE_NUM_FIELDS. https://codereview.chromium.org/371603003/diff/40001/mojo/bindings/java/src/o... mojo/bindings/java/src/org/chromium/mojo/bindings/MessageHeader.java:20: private static final DataHeader MESSAGE_WITH_REQUEST_ID_STRUCT_INFO = new DataHeader( ditto https://codereview.chromium.org/371603003/diff/40001/mojo/bindings/java/src/o... mojo/bindings/java/src/org/chromium/mojo/bindings/MessageHeader.java:24: private static final int REQUEST_ID_OFFSET = 16; newline https://codereview.chromium.org/371603003/diff/40001/mojo/bindings/java/src/o... mojo/bindings/java/src/org/chromium/mojo/bindings/MessageHeader.java:41: * Constructor for the header of a message not having a response. /not having/which does not have https://codereview.chromium.org/371603003/diff/40001/mojo/bindings/java/src/o... mojo/bindings/java/src/org/chromium/mojo/bindings/MessageHeader.java:52: * Constructor for the header of a message having a response or being itself a response. ditto https://codereview.chromium.org/371603003/diff/40001/mojo/bindings/java/src/o... mojo/bindings/java/src/org/chromium/mojo/bindings/MessageHeader.java:79: if (mDataHeader.numFields == 2 && mDataHeader.size != SIMPLE_MESSAGE_STRUCT_INFO.size) { /s/2/SIMPLE_MESSAGE_NUM_FIELDS (and below for "3") https://codereview.chromium.org/371603003/diff/40001/mojo/bindings/java/src/o... mojo/bindings/java/src/org/chromium/mojo/bindings/MessageHeader.java:81: "Incorrect message size, expecting " + SIMPLE_MESSAGE_STRUCT_INFO.size Mention that you got a message with 2 fields and unexpected size (to differentiate from error below, which should also be updated similarly) https://codereview.chromium.org/371603003/diff/40001/mojo/bindings/java/src/o... mojo/bindings/java/src/org/chromium/mojo/bindings/MessageHeader.java:89: } I think this would be clearer if you factor out the validation from lines 69-89 to a private static helper function. https://codereview.chromium.org/371603003/diff/40001/mojo/bindings/java/src/o... mojo/bindings/java/src/org/chromium/mojo/bindings/MessageHeader.java:94: throw new DeserializationException("Incorrect message size, expecting at least " Also mention something in the message about why you expect the message to have this size https://codereview.chromium.org/371603003/diff/40001/mojo/bindings/java/src/o... mojo/bindings/java/src/org/chromium/mojo/bindings/MessageHeader.java:158: * Returns if the header has the expected flags. Only consider flags this class knows about to Returns true if... /s/consider/considers https://codereview.chromium.org/371603003/diff/40001/mojo/bindings/java/src/o... mojo/bindings/java/src/org/chromium/mojo/bindings/MessageHeader.java:159: * allow working with future version of the header format. /s/to allow working/in order to allow this class to work/ https://codereview.chromium.org/371603003/diff/40001/mojo/bindings/java/src/o... mojo/bindings/java/src/org/chromium/mojo/bindings/MessageHeader.java:168: * about to allow working with future version of the header format. same changes as validateHeader above https://codereview.chromium.org/371603003/diff/40001/mojo/bindings/java/src/o... mojo/bindings/java/src/org/chromium/mojo/bindings/MessageHeader.java:186: * return whether a message with the given flags must have a request Id. /s/return/Returns https://codereview.chromium.org/371603003/diff/40001/mojo/bindings/java/src/o... File mojo/bindings/java/src/org/chromium/mojo/bindings/MessageReceiverWithResponder.java (right): https://codereview.chromium.org/371603003/diff/40001/mojo/bindings/java/src/o... mojo/bindings/java/src/org/chromium/mojo/bindings/MessageReceiverWithResponder.java:14: * A variant on Accept that registers a MessageReceiver (known as the responder) to handle the A variant on {@link MessageReceiver#accept()} that registers a {@link MessageReceiver} (known as the responder) to handle the response message generated from the given message. The responder's {@link MessageReceiver#accept()} method will only be called after the response message has been received, which will always be after this method returns. (looking at RouterImpl, it looks like the accept message never gets called from within acceptWithResponder as far as I can tell, and this is a good thing - from experience, not knowing whether a callback will be called synchronously from within a method or posted to be called asynchronously later is a really bad idea and causes tricky bugs - I think you should just specify that the responder's accept method will always be called after this method returns. https://codereview.chromium.org/371603003/diff/40001/mojo/bindings/java/src/o... mojo/bindings/java/src/org/chromium/mojo/bindings/MessageReceiverWithResponder.java:18: boolean acceptWithResponder(Message message, MessageReceiver receiver); /s/receiver/responder https://codereview.chromium.org/371603003/diff/40001/mojo/bindings/java/src/o... File mojo/bindings/java/src/org/chromium/mojo/bindings/Router.java (right): https://codereview.chromium.org/371603003/diff/40001/mojo/bindings/java/src/o... mojo/bindings/java/src/org/chromium/mojo/bindings/Router.java:10: * A {@link Router} will handle mojo message and forward those to a {@link Connector}. It handles /s/It handles.../It deals with parsing of headers and adding of request ids in order to be able to match a response to a request./ https://codereview.chromium.org/371603003/diff/40001/mojo/bindings/java/src/o... File mojo/bindings/java/src/org/chromium/mojo/bindings/RouterImpl.java (right): https://codereview.chromium.org/371603003/diff/40001/mojo/bindings/java/src/o... mojo/bindings/java/src/org/chromium/mojo/bindings/RouterImpl.java:14: * Implementation of Router. {@link Router} https://codereview.chromium.org/371603003/diff/40001/mojo/bindings/java/src/o... mojo/bindings/java/src/org/chromium/mojo/bindings/RouterImpl.java:21: private class Thunk implements MessageReceiver { Thunk is not a very clear name for what the class does - I would just call it MessageReceiverImpl or something similar. https://codereview.chromium.org/371603003/diff/40001/mojo/bindings/java/src/o... mojo/bindings/java/src/org/chromium/mojo/bindings/RouterImpl.java:34: * The {@link Connector} connected to the handle. /s/connected/which is connected https://codereview.chromium.org/371603003/diff/40001/mojo/bindings/java/src/o... mojo/bindings/java/src/org/chromium/mojo/bindings/RouterImpl.java:39: * The {@link MessageReceiverWithResponder} that will deserialize and use the message received /s/deserialize and use/consume /s/message/messages https://codereview.chromium.org/371603003/diff/40001/mojo/bindings/java/src/o... mojo/bindings/java/src/org/chromium/mojo/bindings/RouterImpl.java:45: * The id of the next request that needs a response. It is auto-incremented. The next id to use for a request id which needs a response... https://codereview.chromium.org/371603003/diff/40001/mojo/bindings/java/src/o... mojo/bindings/java/src/org/chromium/mojo/bindings/RouterImpl.java:55: * Constructor. Detail parameters and that it uses the default asyncwaiter for the messagePipeHandle https://codereview.chromium.org/371603003/diff/40001/mojo/bindings/java/src/o... mojo/bindings/java/src/org/chromium/mojo/bindings/RouterImpl.java:62: * Constructor. Detail parameters https://codereview.chromium.org/371603003/diff/40001/mojo/bindings/java/src/o... mojo/bindings/java/src/org/chromium/mojo/bindings/RouterImpl.java:90: // A message without receiver is directly forwarded to the connector. /s/receiever/a responder https://codereview.chromium.org/371603003/diff/40001/mojo/bindings/java/src/o... mojo/bindings/java/src/org/chromium/mojo/bindings/RouterImpl.java:98: public boolean acceptWithResponder(Message message, MessageReceiver receiver) { /s/receiver/responder https://codereview.chromium.org/371603003/diff/40001/mojo/bindings/java/src/o... mojo/bindings/java/src/org/chromium/mojo/bindings/RouterImpl.java:108: assert !mResponders.containsKey(requestId); I don't think it's enough to just assert this - even in release builds we want to be sure that we never use a requestId which is inflight even when mNextRequestId overflows. https://codereview.chromium.org/371603003/diff/40001/mojo/bindings/java/src/o... mojo/bindings/java/src/org/chromium/mojo/bindings/RouterImpl.java:113: // Only keep the receiver is the message has been accepted. /s/receiever/responder https://codereview.chromium.org/371603003/diff/40001/mojo/bindings/java/src/o... mojo/bindings/java/src/org/chromium/mojo/bindings/RouterImpl.java:143: * Receive a message from the connector. describe what's returned https://codereview.chromium.org/371603003/diff/40001/mojo/bindings/java/src/o... mojo/bindings/java/src/org/chromium/mojo/bindings/RouterImpl.java:159: return false; should we teardown here too?
https://codereview.chromium.org/371603003/diff/40001/mojo/bindings/java/src/o... File mojo/bindings/java/src/org/chromium/mojo/bindings/AutoCloseableRouter.java (right): https://codereview.chromium.org/371603003/diff/40001/mojo/bindings/java/src/o... mojo/bindings/java/src/org/chromium/mojo/bindings/AutoCloseableRouter.java:32: mExecutor = core == null ? null : ExecutorFactory.getExecutorForCurrentThread(core); On 2014/07/10 19:03:57, rmcilroy wrote: > I don't think you need the 'core == null' check given that the class will not be > useful if null is passed as core, since it will immediately close the object. I > would prefer a NPE here if it is used incorrectly rather than a harder to debug > error later when the router is unexpectedly closed. Done. https://codereview.chromium.org/371603003/diff/40001/mojo/bindings/java/src/o... mojo/bindings/java/src/org/chromium/mojo/bindings/AutoCloseableRouter.java:100: if (mExecutor != null) { On 2014/07/10 19:03:57, rmcilroy wrote: > Keep track of whether the router is already closed and only close in finalizer > if not already closed. Also log a great big noisy warning message here (and I > would also advise to throw an exception in debug builds) because if this is ever > reached then the user of the class is doing something wrong. I cannot log anything, as there is no standard logging mechanism. I can throw unconditionally, as finalizer just log the exception and do not do anything. That will be enough to see the issues, but that will not break anything. https://codereview.chromium.org/371603003/diff/40001/mojo/bindings/java/src/o... File mojo/bindings/java/src/org/chromium/mojo/bindings/ConnectionErrorHandler.java (right): https://codereview.chromium.org/371603003/diff/40001/mojo/bindings/java/src/o... mojo/bindings/java/src/org/chromium/mojo/bindings/ConnectionErrorHandler.java:11: * message pipes. On 2014/07/10 19:03:57, rmcilroy wrote: > /s/An/A > /s/of error/of an error/ Done. https://codereview.chromium.org/371603003/diff/40001/mojo/bindings/java/src/o... File mojo/bindings/java/src/org/chromium/mojo/bindings/Message.java (right): https://codereview.chromium.org/371603003/diff/40001/mojo/bindings/java/src/o... mojo/bindings/java/src/org/chromium/mojo/bindings/Message.java:70: * to have a header. This will throw a {@link DeserializationException} it the start of the On 2014/07/10 19:03:57, rmcilroy wrote: > /s/it/if Done. https://codereview.chromium.org/371603003/diff/40001/mojo/bindings/java/src/o... mojo/bindings/java/src/org/chromium/mojo/bindings/Message.java:73: public MessageHeader getHeader() { On 2014/07/10 19:03:57, rmcilroy wrote: > I'm not sure I like the idea of a Message optionally having header/payload. > Could you have a new class - MessageWithHeader, which either extends or wraps > Message, and modify acceptWithResponder to take a MessageWithHeader. You can > then have a single place where you convert between Message and MessageWithHeader > (namely Router.handleIncomingMessage) and ensure type safety the rest of the > time. Done, except that I need to use header in all the message receiver... As well as when reading a message from a pipe. The only place where I do not want header is when serializing/deserializing a struct. https://codereview.chromium.org/371603003/diff/40001/mojo/bindings/java/src/o... mojo/bindings/java/src/org/chromium/mojo/bindings/Message.java:97: void resetHeader() { On 2014/07/10 19:03:57, rmcilroy wrote: > Having to remember to resetHeader also seems error prone. If we have a separate > class then we could have setRequestId be a method on MessageWithHeader, which > could then internally ensure that the cached header/payload is invalidated > appropriately, WDYT? Done. https://codereview.chromium.org/371603003/diff/40001/mojo/bindings/java/src/o... File mojo/bindings/java/src/org/chromium/mojo/bindings/MessageHeader.java (right): https://codereview.chromium.org/371603003/diff/40001/mojo/bindings/java/src/o... mojo/bindings/java/src/org/chromium/mojo/bindings/MessageHeader.java:15: private static final int MESSAGE_WITH_REQUEST_ID_SIZE = 24; On 2014/07/10 19:03:58, rmcilroy wrote: > Group common fields and insert newlines to seperate - e.g,: > > SIMPLE_MESSAGE_SIZE > MESSAGE_WITH_REQUEST_ID_SIZE > SIMPLE_MESSAGE_STRUCT_INFO > > MESSAGE_WITH_REQUEST_ID_SIZE > MESSAGE_WITH_REQUEST_ID_NUM_FIELDS > MESSAGE_WITH_REQUEST_ID_STRUCT_INFO > > TYPE_OFFSET > FLAGS_OFFSET > REQUEST_ID_OFFSET Done. https://codereview.chromium.org/371603003/diff/40001/mojo/bindings/java/src/o... mojo/bindings/java/src/org/chromium/mojo/bindings/MessageHeader.java:18: private static final DataHeader SIMPLE_MESSAGE_STRUCT_INFO = new DataHeader(SIMPLE_MESSAGE_SIZE, On 2014/07/10 19:03:58, rmcilroy wrote: > nit - newline at "new" instead of SIMPLE_MESSAGE_NUM_FIELDS. Done. https://codereview.chromium.org/371603003/diff/40001/mojo/bindings/java/src/o... mojo/bindings/java/src/org/chromium/mojo/bindings/MessageHeader.java:20: private static final DataHeader MESSAGE_WITH_REQUEST_ID_STRUCT_INFO = new DataHeader( On 2014/07/10 19:03:58, rmcilroy wrote: > ditto Done. https://codereview.chromium.org/371603003/diff/40001/mojo/bindings/java/src/o... mojo/bindings/java/src/org/chromium/mojo/bindings/MessageHeader.java:24: private static final int REQUEST_ID_OFFSET = 16; On 2014/07/10 19:03:58, rmcilroy wrote: > newline Done. https://codereview.chromium.org/371603003/diff/40001/mojo/bindings/java/src/o... mojo/bindings/java/src/org/chromium/mojo/bindings/MessageHeader.java:41: * Constructor for the header of a message not having a response. On 2014/07/10 19:03:58, rmcilroy wrote: > /not having/which does not have Done. https://codereview.chromium.org/371603003/diff/40001/mojo/bindings/java/src/o... mojo/bindings/java/src/org/chromium/mojo/bindings/MessageHeader.java:52: * Constructor for the header of a message having a response or being itself a response. On 2014/07/10 19:03:58, rmcilroy wrote: > ditto Done. https://codereview.chromium.org/371603003/diff/40001/mojo/bindings/java/src/o... mojo/bindings/java/src/org/chromium/mojo/bindings/MessageHeader.java:79: if (mDataHeader.numFields == 2 && mDataHeader.size != SIMPLE_MESSAGE_STRUCT_INFO.size) { On 2014/07/10 19:03:58, rmcilroy wrote: > /s/2/SIMPLE_MESSAGE_NUM_FIELDS (and below for "3") Done. https://codereview.chromium.org/371603003/diff/40001/mojo/bindings/java/src/o... mojo/bindings/java/src/org/chromium/mojo/bindings/MessageHeader.java:81: "Incorrect message size, expecting " + SIMPLE_MESSAGE_STRUCT_INFO.size On 2014/07/10 19:03:58, rmcilroy wrote: > Mention that you got a message with 2 fields and unexpected size (to > differentiate from error below, which should also be updated similarly) Done. https://codereview.chromium.org/371603003/diff/40001/mojo/bindings/java/src/o... mojo/bindings/java/src/org/chromium/mojo/bindings/MessageHeader.java:89: } On 2014/07/10 19:03:58, rmcilroy wrote: > I think this would be clearer if you factor out the validation from lines 69-89 > to a private static helper function. Done. https://codereview.chromium.org/371603003/diff/40001/mojo/bindings/java/src/o... mojo/bindings/java/src/org/chromium/mojo/bindings/MessageHeader.java:94: throw new DeserializationException("Incorrect message size, expecting at least " On 2014/07/10 19:03:58, rmcilroy wrote: > Also mention something in the message about why you expect the message to have > this size Done. https://codereview.chromium.org/371603003/diff/40001/mojo/bindings/java/src/o... mojo/bindings/java/src/org/chromium/mojo/bindings/MessageHeader.java:158: * Returns if the header has the expected flags. Only consider flags this class knows about to On 2014/07/10 19:03:58, rmcilroy wrote: > Returns true if... > /s/consider/considers Done. https://codereview.chromium.org/371603003/diff/40001/mojo/bindings/java/src/o... mojo/bindings/java/src/org/chromium/mojo/bindings/MessageHeader.java:159: * allow working with future version of the header format. On 2014/07/10 19:03:58, rmcilroy wrote: > /s/to allow working/in order to allow this class to work/ Done. https://codereview.chromium.org/371603003/diff/40001/mojo/bindings/java/src/o... mojo/bindings/java/src/org/chromium/mojo/bindings/MessageHeader.java:168: * about to allow working with future version of the header format. On 2014/07/10 19:03:58, rmcilroy wrote: > same changes as validateHeader above Done. https://codereview.chromium.org/371603003/diff/40001/mojo/bindings/java/src/o... mojo/bindings/java/src/org/chromium/mojo/bindings/MessageHeader.java:186: * return whether a message with the given flags must have a request Id. On 2014/07/10 19:03:58, rmcilroy wrote: > /s/return/Returns Done. https://codereview.chromium.org/371603003/diff/40001/mojo/bindings/java/src/o... File mojo/bindings/java/src/org/chromium/mojo/bindings/MessageReceiverWithResponder.java (right): https://codereview.chromium.org/371603003/diff/40001/mojo/bindings/java/src/o... mojo/bindings/java/src/org/chromium/mojo/bindings/MessageReceiverWithResponder.java:14: * A variant on Accept that registers a MessageReceiver (known as the responder) to handle the On 2014/07/10 19:03:58, rmcilroy wrote: > A variant on {@link MessageReceiver#accept()} that registers a {@link > MessageReceiver} (known as the responder) to handle the response message > generated from the given message. The responder's {@link > MessageReceiver#accept()} method will only be called after the response message > has been received, which will always be after this method returns. > > (looking at RouterImpl, it looks like the accept message never gets called from > within acceptWithResponder as far as I can tell, and this is a good thing - from > experience, not knowing whether a callback will be called synchronously from > within a method or posted to be called asynchronously later is a really bad idea > and causes tricky bugs - I think you should just specify that the responder's > accept method will always be called after this method returns. The issue with this is that it will force implementation of every service to have this constraint on their callbacks, as in the very rare case where the proxy is directly connected to the stub, this is what will happen -> the router will not call the accept method in this method, and but it will call the accept method of the stub, that will call the implementation, that will call the callback, that will call the router that will call the accept. The C++ implementation allows this to happen. https://codereview.chromium.org/371603003/diff/40001/mojo/bindings/java/src/o... mojo/bindings/java/src/org/chromium/mojo/bindings/MessageReceiverWithResponder.java:18: boolean acceptWithResponder(Message message, MessageReceiver receiver); On 2014/07/10 19:03:58, rmcilroy wrote: > /s/receiver/responder Done. https://codereview.chromium.org/371603003/diff/40001/mojo/bindings/java/src/o... File mojo/bindings/java/src/org/chromium/mojo/bindings/Router.java (right): https://codereview.chromium.org/371603003/diff/40001/mojo/bindings/java/src/o... mojo/bindings/java/src/org/chromium/mojo/bindings/Router.java:10: * A {@link Router} will handle mojo message and forward those to a {@link Connector}. It handles On 2014/07/10 19:03:58, rmcilroy wrote: > /s/It handles.../It deals with parsing of headers and adding of request ids in > order to be able to match a response to a request./ Done. https://codereview.chromium.org/371603003/diff/40001/mojo/bindings/java/src/o... File mojo/bindings/java/src/org/chromium/mojo/bindings/RouterImpl.java (right): https://codereview.chromium.org/371603003/diff/40001/mojo/bindings/java/src/o... mojo/bindings/java/src/org/chromium/mojo/bindings/RouterImpl.java:14: * Implementation of Router. On 2014/07/10 19:03:59, rmcilroy wrote: > {@link Router} Done. https://codereview.chromium.org/371603003/diff/40001/mojo/bindings/java/src/o... mojo/bindings/java/src/org/chromium/mojo/bindings/RouterImpl.java:21: private class Thunk implements MessageReceiver { On 2014/07/10 19:03:59, rmcilroy wrote: > Thunk is not a very clear name for what the class does - I would just call it > MessageReceiverImpl or something similar. It is the name the C++ implementation use. I'd like to keep the naming close. https://codereview.chromium.org/371603003/diff/40001/mojo/bindings/java/src/o... mojo/bindings/java/src/org/chromium/mojo/bindings/RouterImpl.java:34: * The {@link Connector} connected to the handle. On 2014/07/10 19:03:59, rmcilroy wrote: > /s/connected/which is connected Done. https://codereview.chromium.org/371603003/diff/40001/mojo/bindings/java/src/o... mojo/bindings/java/src/org/chromium/mojo/bindings/RouterImpl.java:39: * The {@link MessageReceiverWithResponder} that will deserialize and use the message received On 2014/07/10 19:03:59, rmcilroy wrote: > /s/deserialize and use/consume > /s/message/messages Done. https://codereview.chromium.org/371603003/diff/40001/mojo/bindings/java/src/o... mojo/bindings/java/src/org/chromium/mojo/bindings/RouterImpl.java:45: * The id of the next request that needs a response. It is auto-incremented. On 2014/07/10 19:03:59, rmcilroy wrote: > The next id to use for a request id which needs a response... Done. https://codereview.chromium.org/371603003/diff/40001/mojo/bindings/java/src/o... mojo/bindings/java/src/org/chromium/mojo/bindings/RouterImpl.java:55: * Constructor. On 2014/07/10 19:03:59, rmcilroy wrote: > Detail parameters and that it uses the default asyncwaiter for the > messagePipeHandle Done. https://codereview.chromium.org/371603003/diff/40001/mojo/bindings/java/src/o... mojo/bindings/java/src/org/chromium/mojo/bindings/RouterImpl.java:62: * Constructor. On 2014/07/10 19:03:59, rmcilroy wrote: > Detail parameters Done. https://codereview.chromium.org/371603003/diff/40001/mojo/bindings/java/src/o... mojo/bindings/java/src/org/chromium/mojo/bindings/RouterImpl.java:90: // A message without receiver is directly forwarded to the connector. On 2014/07/10 19:03:59, rmcilroy wrote: > /s/receiever/a responder Done. https://codereview.chromium.org/371603003/diff/40001/mojo/bindings/java/src/o... mojo/bindings/java/src/org/chromium/mojo/bindings/RouterImpl.java:98: public boolean acceptWithResponder(Message message, MessageReceiver receiver) { On 2014/07/10 19:03:59, rmcilroy wrote: > /s/receiver/responder Done. https://codereview.chromium.org/371603003/diff/40001/mojo/bindings/java/src/o... mojo/bindings/java/src/org/chromium/mojo/bindings/RouterImpl.java:108: assert !mResponders.containsKey(requestId); On 2014/07/10 19:03:59, rmcilroy wrote: > I don't think it's enough to just assert this - even in release builds we want > to be sure that we never use a requestId which is inflight even when > mNextRequestId overflows. Done. I don't think (famous last words) that it is expected that we will ever be able to send 2^64 messages... https://codereview.chromium.org/371603003/diff/40001/mojo/bindings/java/src/o... mojo/bindings/java/src/org/chromium/mojo/bindings/RouterImpl.java:113: // Only keep the receiver is the message has been accepted. On 2014/07/10 19:03:59, rmcilroy wrote: > /s/receiever/responder Done. https://codereview.chromium.org/371603003/diff/40001/mojo/bindings/java/src/o... mojo/bindings/java/src/org/chromium/mojo/bindings/RouterImpl.java:143: * Receive a message from the connector. On 2014/07/10 19:03:59, rmcilroy wrote: > describe what's returned Done. https://codereview.chromium.org/371603003/diff/40001/mojo/bindings/java/src/o... mojo/bindings/java/src/org/chromium/mojo/bindings/RouterImpl.java:159: return false; On 2014/07/10 19:03:59, rmcilroy wrote: > should we teardown here too? Not really. The message is routed, but the problem is on the other end, not on ours, Just dropping it should be fine.
https://codereview.chromium.org/371603003/diff/40001/mojo/bindings/java/src/o... File mojo/bindings/java/src/org/chromium/mojo/bindings/AutoCloseableRouter.java (right): https://codereview.chromium.org/371603003/diff/40001/mojo/bindings/java/src/o... mojo/bindings/java/src/org/chromium/mojo/bindings/AutoCloseableRouter.java:100: if (mExecutor != null) { On 2014/07/11 11:42:07, qsr wrote: > On 2014/07/10 19:03:57, rmcilroy wrote: > > Keep track of whether the router is already closed and only close in finalizer > > if not already closed. Also log a great big noisy warning message here (and I > > would also advise to throw an exception in debug builds) because if this is > ever > > reached then the user of the class is doing something wrong. > > I cannot log anything, as there is no standard logging mechanism. I can throw > unconditionally, as finalizer just log the exception and do not do anything. > That will be enough to see the issues, but that will not break anything. Could you use the C MojoLogger? (I'm fine with this being a followup CL given you've added the exception) https://codereview.chromium.org/371603003/diff/40001/mojo/bindings/java/src/o... File mojo/bindings/java/src/org/chromium/mojo/bindings/MessageReceiverWithResponder.java (right): https://codereview.chromium.org/371603003/diff/40001/mojo/bindings/java/src/o... mojo/bindings/java/src/org/chromium/mojo/bindings/MessageReceiverWithResponder.java:14: * A variant on Accept that registers a MessageReceiver (known as the responder) to handle the On 2014/07/11 11:42:08, qsr wrote: > On 2014/07/10 19:03:58, rmcilroy wrote: > > A variant on {@link MessageReceiver#accept()} that registers a {@link > > MessageReceiver} (known as the responder) to handle the response message > > generated from the given message. The responder's {@link > > MessageReceiver#accept()} method will only be called after the response > message > > has been received, which will always be after this method returns. > > > > (looking at RouterImpl, it looks like the accept message never gets called > from > > within acceptWithResponder as far as I can tell, and this is a good thing - > from > > experience, not knowing whether a callback will be called synchronously from > > within a method or posted to be called asynchronously later is a really bad > idea > > and causes tricky bugs - I think you should just specify that the responder's > > accept method will always be called after this method returns. > > The issue with this is that it will force implementation of every service to > have this constraint on their callbacks, as in the very rare case where the > proxy is directly connected to the stub, this is what will happen -> the router > will not call the accept method in this method, and but it will call the accept > method of the stub, that will call the implementation, that will call the > callback, that will call the router that will call the accept. > > The C++ implementation allows this to happen. Hmm, I really think this is generally a bad idea, but if the other mojo bindings allow this I guess there isn't anything we can do to avoid it. Thanks for updating the rest of the JavaDoc https://codereview.chromium.org/371603003/diff/100001/build/android/lint/supp... File build/android/lint/suppressions.xml (right): https://codereview.chromium.org/371603003/diff/100001/build/android/lint/supp... build/android/lint/suppressions.xml:29: <ignore regexp="mojo/bindings/java/src/.*"/> Could I ask what this is required for? https://codereview.chromium.org/371603003/diff/100001/mojo/android/javatests/... File mojo/android/javatests/src/org/chromium/mojo/bindings/AutoCloseableRouterTest.java (right): https://codereview.chromium.org/371603003/diff/100001/mojo/android/javatests/... mojo/android/javatests/src/org/chromium/mojo/bindings/AutoCloseableRouterTest.java:72: * Testing that an {@link AutoCloseableRouter} close its delegate when getting deallocated. Test that an {@link AutoCloseableRouter} closes its delegate when it is deallocated. https://codereview.chromium.org/371603003/diff/100001/mojo/android/javatests/... mojo/android/javatests/src/org/chromium/mojo/bindings/AutoCloseableRouterTest.java:88: System.runFinalization(); Hmm, both System.gc and System.runFinalization are only suggestions to the VM (and I don't think System.gc will do anything synchronously on newer versions of Android) so this test will probably be very flaky. I can't think of any solution though. https://codereview.chromium.org/371603003/diff/100001/mojo/android/javatests/... File mojo/android/javatests/src/org/chromium/mojo/bindings/RouterTest.java (right): https://codereview.chromium.org/371603003/diff/100001/mojo/android/javatests/... mojo/android/javatests/src/org/chromium/mojo/bindings/RouterTest.java:82: new Encoder(CoreImpl.getInstance(), header.getSize()); encoder = new Encoder(... ? https://codereview.chromium.org/371603003/diff/100001/mojo/bindings/java/src/... File mojo/bindings/java/src/org/chromium/mojo/bindings/AutoCloseableRouter.java (right): https://codereview.chromium.org/371603003/diff/100001/mojo/bindings/java/src/... mojo/bindings/java/src/org/chromium/mojo/bindings/AutoCloseableRouter.java:111: throw new IllegalStateException("Router should be closed."); /s/Router should be closed/Warning: Router objects should be explicitly closed when no longer required otherwise you may leak handles./ https://codereview.chromium.org/371603003/diff/100001/mojo/bindings/java/src/... File mojo/bindings/java/src/org/chromium/mojo/bindings/Message.java (right): https://codereview.chromium.org/371603003/diff/100001/mojo/bindings/java/src/... mojo/bindings/java/src/org/chromium/mojo/bindings/Message.java:20: public class Message { I think this can still be final since you are wrapping rather than extending https://codereview.chromium.org/371603003/diff/100001/mojo/bindings/java/src/... mojo/bindings/java/src/org/chromium/mojo/bindings/Message.java:48: public static int readAndDispatchMessage(MessagePipeHandle handle, Maybe this should be be part of MessageWithHeader since it only reads and dispatches messages with headers? https://codereview.chromium.org/371603003/diff/100001/mojo/bindings/java/src/... File mojo/bindings/java/src/org/chromium/mojo/bindings/MessageReceiver.java (right): https://codereview.chromium.org/371603003/diff/100001/mojo/bindings/java/src/... mojo/bindings/java/src/org/chromium/mojo/bindings/MessageReceiver.java:13: * Receive a {@link Message}. The {@link MessageReceiver} is allowed to mutable the message. {@link Message} -> {link MessageWithHeader} https://codereview.chromium.org/371603003/diff/100001/mojo/bindings/java/src/... File mojo/bindings/java/src/org/chromium/mojo/bindings/MessageWithHeader.java (right): https://codereview.chromium.org/371603003/diff/100001/mojo/bindings/java/src/... mojo/bindings/java/src/org/chromium/mojo/bindings/MessageWithHeader.java:23: this.mBaseMessage = baseMessage; Can you validate that the message has a header here? Is there many cases where you expect not to have to call getHeader on a MessageWithHeader? If not, then maybe you should just always create mHeader and keep it up-to-date with the base message?
https://codereview.chromium.org/371603003/diff/40001/mojo/bindings/java/src/o... File mojo/bindings/java/src/org/chromium/mojo/bindings/AutoCloseableRouter.java (right): https://codereview.chromium.org/371603003/diff/40001/mojo/bindings/java/src/o... mojo/bindings/java/src/org/chromium/mojo/bindings/AutoCloseableRouter.java:100: if (mExecutor != null) { On 2014/07/14 14:44:56, rmcilroy wrote: > On 2014/07/11 11:42:07, qsr wrote: > > On 2014/07/10 19:03:57, rmcilroy wrote: > > > Keep track of whether the router is already closed and only close in > finalizer > > > if not already closed. Also log a great big noisy warning message here (and > I > > > would also advise to throw an exception in debug builds) because if this is > > ever > > > reached then the user of the class is doing something wrong. > > > > I cannot log anything, as there is no standard logging mechanism. I can throw > > unconditionally, as finalizer just log the exception and do not do anything. > > That will be enough to see the issues, but that will not break anything. > > Could you use the C MojoLogger? (I'm fine with this being a followup CL given > you've added the exception) I cannot do this in this module, that is pure java and platform independant. If we want to be able to use the C MojoLogger, we need to add an API for it in Core, which mean designing a new (yet again) java interface for logging. Do you want this? Given that I expect java application to use their own native logging API, and that this is currently the only place where we need logging, I'm not sure we really want this. https://codereview.chromium.org/371603003/diff/100001/build/android/lint/supp... File build/android/lint/suppressions.xml (right): https://codereview.chromium.org/371603003/diff/100001/build/android/lint/supp... build/android/lint/suppressions.xml:29: <ignore regexp="mojo/bindings/java/src/.*"/> On 2014/07/14 14:44:56, rmcilroy wrote: > Could I ask what this is required for? The version of java lint has been upgraded. It now flags assert usage, asking to use an Android specific check for debug build and normal assert instead. This code is platform agnostic and cannot do anything like this, so this remove this lint warnings for those files. https://codereview.chromium.org/371603003/diff/100001/mojo/android/javatests/... File mojo/android/javatests/src/org/chromium/mojo/bindings/AutoCloseableRouterTest.java (right): https://codereview.chromium.org/371603003/diff/100001/mojo/android/javatests/... mojo/android/javatests/src/org/chromium/mojo/bindings/AutoCloseableRouterTest.java:72: * Testing that an {@link AutoCloseableRouter} close its delegate when getting deallocated. On 2014/07/14 14:44:57, rmcilroy wrote: > Test that an {@link AutoCloseableRouter} closes its delegate when it is > deallocated. Done. https://codereview.chromium.org/371603003/diff/100001/mojo/android/javatests/... mojo/android/javatests/src/org/chromium/mojo/bindings/AutoCloseableRouterTest.java:88: System.runFinalization(); On 2014/07/14 14:44:57, rmcilroy wrote: > Hmm, both System.gc and System.runFinalization are only suggestions to the VM > (and I don't think System.gc will do anything synchronously on newer versions of > Android) so this test will probably be very flaky. I can't think of any > solution though. Would you prefer I just remove the test? As you said, I do not have a lot of leeway here... https://codereview.chromium.org/371603003/diff/100001/mojo/android/javatests/... File mojo/android/javatests/src/org/chromium/mojo/bindings/RouterTest.java (right): https://codereview.chromium.org/371603003/diff/100001/mojo/android/javatests/... mojo/android/javatests/src/org/chromium/mojo/bindings/RouterTest.java:82: new Encoder(CoreImpl.getInstance(), header.getSize()); On 2014/07/14 14:44:57, rmcilroy wrote: > encoder = new Encoder(... ? Thanks. Was wondering why it was even passing. Happens that I was overwriting the old message. https://codereview.chromium.org/371603003/diff/100001/mojo/bindings/java/src/... File mojo/bindings/java/src/org/chromium/mojo/bindings/AutoCloseableRouter.java (right): https://codereview.chromium.org/371603003/diff/100001/mojo/bindings/java/src/... mojo/bindings/java/src/org/chromium/mojo/bindings/AutoCloseableRouter.java:111: throw new IllegalStateException("Router should be closed."); On 2014/07/14 14:44:57, rmcilroy wrote: > /s/Router should be closed/Warning: Router objects should be explicitly closed > when no longer required otherwise you may leak handles./ Done. https://codereview.chromium.org/371603003/diff/100001/mojo/bindings/java/src/... File mojo/bindings/java/src/org/chromium/mojo/bindings/Message.java (right): https://codereview.chromium.org/371603003/diff/100001/mojo/bindings/java/src/... mojo/bindings/java/src/org/chromium/mojo/bindings/Message.java:20: public class Message { On 2014/07/14 14:44:57, rmcilroy wrote: > I think this can still be final since you are wrapping rather than extending Done. https://codereview.chromium.org/371603003/diff/100001/mojo/bindings/java/src/... mojo/bindings/java/src/org/chromium/mojo/bindings/Message.java:48: public static int readAndDispatchMessage(MessagePipeHandle handle, On 2014/07/14 14:44:57, rmcilroy wrote: > Maybe this should be be part of MessageWithHeader since it only reads and > dispatches messages with headers? Done. https://codereview.chromium.org/371603003/diff/100001/mojo/bindings/java/src/... File mojo/bindings/java/src/org/chromium/mojo/bindings/MessageReceiver.java (right): https://codereview.chromium.org/371603003/diff/100001/mojo/bindings/java/src/... mojo/bindings/java/src/org/chromium/mojo/bindings/MessageReceiver.java:13: * Receive a {@link Message}. The {@link MessageReceiver} is allowed to mutable the message. On 2014/07/14 14:44:57, rmcilroy wrote: > {@link Message} -> {link MessageWithHeader} Done. https://codereview.chromium.org/371603003/diff/100001/mojo/bindings/java/src/... File mojo/bindings/java/src/org/chromium/mojo/bindings/MessageWithHeader.java (right): https://codereview.chromium.org/371603003/diff/100001/mojo/bindings/java/src/... mojo/bindings/java/src/org/chromium/mojo/bindings/MessageWithHeader.java:23: this.mBaseMessage = baseMessage; On 2014/07/14 14:44:57, rmcilroy wrote: > Can you validate that the message has a header here? Is there many cases where > you expect not to have to call getHeader on a MessageWithHeader? If not, then > maybe you should just always create mHeader and keep it up-to-date with the base > message? I expect not to need the header each time a method that doesn't have a response is called. In that case, I need to build the message with the headers, which will be done by struct#serializeWithHeader, and the result will be passed to Router#accept, which doesn't need the header as it will just pass it unconditionally to the connector. Having this done lazily saves this case. In those conditions, what would you prefer?
https://codereview.chromium.org/371603003/diff/40001/mojo/bindings/java/src/o... File mojo/bindings/java/src/org/chromium/mojo/bindings/AutoCloseableRouter.java (right): https://codereview.chromium.org/371603003/diff/40001/mojo/bindings/java/src/o... mojo/bindings/java/src/org/chromium/mojo/bindings/AutoCloseableRouter.java:100: if (mExecutor != null) { On 2014/07/15 12:03:15, qsr wrote: > On 2014/07/14 14:44:56, rmcilroy wrote: > > On 2014/07/11 11:42:07, qsr wrote: > > > On 2014/07/10 19:03:57, rmcilroy wrote: > > > > Keep track of whether the router is already closed and only close in > > finalizer > > > > if not already closed. Also log a great big noisy warning message here > (and > > I > > > > would also advise to throw an exception in debug builds) because if this > is > > > ever > > > > reached then the user of the class is doing something wrong. > > > > > > I cannot log anything, as there is no standard logging mechanism. I can > throw > > > unconditionally, as finalizer just log the exception and do not do anything. > > > That will be enough to see the issues, but that will not break anything. > > > > Could you use the C MojoLogger? (I'm fine with this being a followup CL given > > you've added the exception) > > I cannot do this in this module, that is pure java and platform independant. If > we want to be able to use the C MojoLogger, we need to add an API for it in > Core, which mean designing a new (yet again) java interface for logging. Do you > want this? Given that I expect java application to use their own native logging > API, and that this is currently the only place where we need logging, I'm not > sure we really want this. For this case it isn't necessary. I'm a bit worried that there is no unified way to do logging for the Java Mojo bindings though. https://codereview.chromium.org/371603003/diff/100001/build/android/lint/supp... File build/android/lint/suppressions.xml (right): https://codereview.chromium.org/371603003/diff/100001/build/android/lint/supp... build/android/lint/suppressions.xml:29: <ignore regexp="mojo/bindings/java/src/.*"/> On 2014/07/15 12:03:15, qsr wrote: > On 2014/07/14 14:44:56, rmcilroy wrote: > > Could I ask what this is required for? > > The version of java lint has been upgraded. It now flags assert usage, asking > to use an Android specific check for debug build and normal assert instead. This > code is platform agnostic and cannot do anything like this, so this remove this > lint warnings for those files. Thanks for the info. This is fine. https://codereview.chromium.org/371603003/diff/100001/mojo/android/javatests/... File mojo/android/javatests/src/org/chromium/mojo/bindings/AutoCloseableRouterTest.java (right): https://codereview.chromium.org/371603003/diff/100001/mojo/android/javatests/... mojo/android/javatests/src/org/chromium/mojo/bindings/AutoCloseableRouterTest.java:88: System.runFinalization(); On 2014/07/15 12:03:15, qsr wrote: > On 2014/07/14 14:44:57, rmcilroy wrote: > > Hmm, both System.gc and System.runFinalization are only suggestions to the VM > > (and I don't think System.gc will do anything synchronously on newer versions > of > > Android) so this test will probably be very flaky. I can't think of any > > solution though. > > Would you prefer I just remove the test? As you said, I do not have a lot of > leeway here... Yes https://codereview.chromium.org/371603003/diff/100001/mojo/bindings/java/src/... File mojo/bindings/java/src/org/chromium/mojo/bindings/MessageWithHeader.java (right): https://codereview.chromium.org/371603003/diff/100001/mojo/bindings/java/src/... mojo/bindings/java/src/org/chromium/mojo/bindings/MessageWithHeader.java:23: this.mBaseMessage = baseMessage; On 2014/07/15 12:03:16, qsr wrote: > On 2014/07/14 14:44:57, rmcilroy wrote: > > Can you validate that the message has a header here? Is there many cases > where > > you expect not to have to call getHeader on a MessageWithHeader? If not, then > > maybe you should just always create mHeader and keep it up-to-date with the > base > > message? > > I expect not to need the header each time a method that doesn't have a response > is called. In that case, I need to build the message with the headers, which > will be done by struct#serializeWithHeader, and the result will be passed to > Router#accept, which doesn't need the header as it will just pass it > unconditionally to the connector. Having this done lazily saves this case. > > In those conditions, what would you prefer? How about a second constructor which takes a Message and MessageHeader (and asserts that org.chromium.mojo.bindings.MessageHeader(mBaseMessage).equals(mHeader) in debug mode) ?
https://codereview.chromium.org/371603003/diff/100001/mojo/android/javatests/... File mojo/android/javatests/src/org/chromium/mojo/bindings/AutoCloseableRouterTest.java (right): https://codereview.chromium.org/371603003/diff/100001/mojo/android/javatests/... mojo/android/javatests/src/org/chromium/mojo/bindings/AutoCloseableRouterTest.java:88: System.runFinalization(); On 2014/07/15 18:11:57, rmcilroy wrote: > On 2014/07/15 12:03:15, qsr wrote: > > On 2014/07/14 14:44:57, rmcilroy wrote: > > > Hmm, both System.gc and System.runFinalization are only suggestions to the > VM > > > (and I don't think System.gc will do anything synchronously on newer > versions > > of > > > Android) so this test will probably be very flaky. I can't think of any > > > solution though. > > > > Would you prefer I just remove the test? As you said, I do not have a lot of > > leeway here... > > Yes Done. https://codereview.chromium.org/371603003/diff/100001/mojo/bindings/java/src/... File mojo/bindings/java/src/org/chromium/mojo/bindings/MessageWithHeader.java (right): https://codereview.chromium.org/371603003/diff/100001/mojo/bindings/java/src/... mojo/bindings/java/src/org/chromium/mojo/bindings/MessageWithHeader.java:23: this.mBaseMessage = baseMessage; On 2014/07/15 18:11:57, rmcilroy wrote: > On 2014/07/15 12:03:16, qsr wrote: > > On 2014/07/14 14:44:57, rmcilroy wrote: > > > Can you validate that the message has a header here? Is there many cases > > where > > > you expect not to have to call getHeader on a MessageWithHeader? If not, > then > > > maybe you should just always create mHeader and keep it up-to-date with the > > base > > > message? > > > > I expect not to need the header each time a method that doesn't have a > response > > is called. In that case, I need to build the message with the headers, which > > will be done by struct#serializeWithHeader, and the result will be passed to > > Router#accept, which doesn't need the header as it will just pass it > > unconditionally to the connector. Having this done lazily saves this case. > > > > In those conditions, what would you prefer? > > How about a second constructor which takes a Message and MessageHeader (and > asserts that > org.chromium.mojo.bindings.MessageHeader(mBaseMessage).equals(mHeader) in debug > mode) ? Done.
lgtm when comments are addressed. Thanks. https://codereview.chromium.org/371603003/diff/140001/mojo/android/javatests/... File mojo/android/javatests/src/org/chromium/mojo/bindings/BindingsTestUtils.java (right): https://codereview.chromium.org/371603003/diff/140001/mojo/android/javatests/... mojo/android/javatests/src/org/chromium/mojo/bindings/BindingsTestUtils.java:43: public final List<Pair<MessageWithHeader, MessageReceiver>> messagesWithReceivers = nit - newline above https://codereview.chromium.org/371603003/diff/140001/mojo/bindings/java/src/... File mojo/bindings/java/src/org/chromium/mojo/bindings/AutoCloseableRouter.java (right): https://codereview.chromium.org/371603003/diff/140001/mojo/bindings/java/src/... mojo/bindings/java/src/org/chromium/mojo/bindings/AutoCloseableRouter.java:106: @Override indentation https://codereview.chromium.org/371603003/diff/140001/mojo/bindings/java/src/... File mojo/bindings/java/src/org/chromium/mojo/bindings/MessageHeader.java (right): https://codereview.chromium.org/371603003/diff/140001/mojo/bindings/java/src/... mojo/bindings/java/src/org/chromium/mojo/bindings/MessageHeader.java:48: public MessageHeader(int type, int flags) { I think the flags should are always be zero here - just remove flags parameter from this constructor (we can always add it back again if we ad more flags at some point). https://codereview.chromium.org/371603003/diff/140001/mojo/bindings/java/src/... mojo/bindings/java/src/org/chromium/mojo/bindings/MessageHeader.java:90: public int getSize() { javadoc https://codereview.chromium.org/371603003/diff/140001/mojo/bindings/java/src/... mojo/bindings/java/src/org/chromium/mojo/bindings/MessageHeader.java:196: return true; nit - I think this would be cleaner as: if (object == this) return true; if (object == null) return false; if (getClass() != object.getClass()) return false; MessageHeader other = (MessageHeader) object; return ((mDataHeader == null && other.mDataHeader == null) || (mDataHeader.equals(other.mDataHeader))) && mFlags == other.mFlags && mRequestId == other.mRequestId && mType == other.mType; https://codereview.chromium.org/371603003/diff/140001/mojo/bindings/java/src/... File mojo/bindings/java/src/org/chromium/mojo/bindings/MessageWithHeader.java (right): https://codereview.chromium.org/371603003/diff/140001/mojo/bindings/java/src/... mojo/bindings/java/src/org/chromium/mojo/bindings/MessageWithHeader.java:15: * A wrapper around {@link Message} that handles parsing the message header. I think maybe: Represents a {@link Message} which contains a {@link MessageHeader}. Deals with parsing the {@link MessageHeader} for a message. https://codereview.chromium.org/371603003/diff/140001/mojo/bindings/java/src/... mojo/bindings/java/src/org/chromium/mojo/bindings/MessageWithHeader.java:24: * Reinterpret the given message as a message with header. Reinterpret the given |message| as a message with the given |header|. The |message| must contain the |header| as the start of its raw data. https://codereview.chromium.org/371603003/diff/140001/mojo/bindings/java/src/... mojo/bindings/java/src/org/chromium/mojo/bindings/MessageWithHeader.java:33: * Reinterpret the given message as a message with header. Reinterpret the given |message| as a message with a header. The |message| must contain a header as the start of it's raw data, which will be parsed by this constructor. https://codereview.chromium.org/371603003/diff/140001/mojo/bindings/java/src/... File mojo/bindings/java/src/org/chromium/mojo/bindings/Struct.java (right): https://codereview.chromium.org/371603003/diff/140001/mojo/bindings/java/src/... mojo/bindings/java/src/org/chromium/mojo/bindings/Struct.java:74: return true; ditto with MessageHeader equals() comment.
https://codereview.chromium.org/371603003/diff/140001/mojo/android/javatests/... File mojo/android/javatests/src/org/chromium/mojo/bindings/BindingsTestUtils.java (right): https://codereview.chromium.org/371603003/diff/140001/mojo/android/javatests/... mojo/android/javatests/src/org/chromium/mojo/bindings/BindingsTestUtils.java:43: public final List<Pair<MessageWithHeader, MessageReceiver>> messagesWithReceivers = On 2014/07/16 15:06:44, rmcilroy wrote: > nit - newline above Done. https://codereview.chromium.org/371603003/diff/140001/mojo/bindings/java/src/... File mojo/bindings/java/src/org/chromium/mojo/bindings/AutoCloseableRouter.java (right): https://codereview.chromium.org/371603003/diff/140001/mojo/bindings/java/src/... mojo/bindings/java/src/org/chromium/mojo/bindings/AutoCloseableRouter.java:106: @Override On 2014/07/16 15:06:44, rmcilroy wrote: > indentation Done. https://codereview.chromium.org/371603003/diff/140001/mojo/bindings/java/src/... File mojo/bindings/java/src/org/chromium/mojo/bindings/MessageHeader.java (right): https://codereview.chromium.org/371603003/diff/140001/mojo/bindings/java/src/... mojo/bindings/java/src/org/chromium/mojo/bindings/MessageHeader.java:48: public MessageHeader(int type, int flags) { On 2014/07/16 15:06:44, rmcilroy wrote: > I think the flags should are always be zero here - just remove flags parameter > from this constructor (we can always add it back again if we ad more flags at > some point). Done. https://codereview.chromium.org/371603003/diff/140001/mojo/bindings/java/src/... mojo/bindings/java/src/org/chromium/mojo/bindings/MessageHeader.java:90: public int getSize() { On 2014/07/16 15:06:44, rmcilroy wrote: > javadoc Done. https://codereview.chromium.org/371603003/diff/140001/mojo/bindings/java/src/... mojo/bindings/java/src/org/chromium/mojo/bindings/MessageHeader.java:196: return true; On 2014/07/16 15:06:44, rmcilroy wrote: > nit - I think this would be cleaner as: > > if (object == this) return true; > if (object == null) return false; > if (getClass() != object.getClass()) return false; Didn't remove the new line, because my auto-formatter put it back on 2 lines, and I'd like to be able to continue to use it. > > MessageHeader other = (MessageHeader) object; > return ((mDataHeader == null && other.mDataHeader == null) || > (mDataHeader.equals(other.mDataHeader))) && This will crash is this.mDataHeader is null. You need the double if for this kind of case. Will do the rest of the changes. > mFlags == other.mFlags && > mRequestId == other.mRequestId && > mType == other.mType; https://codereview.chromium.org/371603003/diff/140001/mojo/bindings/java/src/... File mojo/bindings/java/src/org/chromium/mojo/bindings/MessageWithHeader.java (right): https://codereview.chromium.org/371603003/diff/140001/mojo/bindings/java/src/... mojo/bindings/java/src/org/chromium/mojo/bindings/MessageWithHeader.java:15: * A wrapper around {@link Message} that handles parsing the message header. On 2014/07/16 15:06:44, rmcilroy wrote: > I think maybe: > > Represents a {@link Message} which contains a {@link MessageHeader}. Deals with > parsing the {@link MessageHeader} for a message. Done. https://codereview.chromium.org/371603003/diff/140001/mojo/bindings/java/src/... mojo/bindings/java/src/org/chromium/mojo/bindings/MessageWithHeader.java:24: * Reinterpret the given message as a message with header. On 2014/07/16 15:06:44, rmcilroy wrote: > Reinterpret the given |message| as a message with the given |header|. The > |message| must contain the |header| as the start of its raw data. Done. https://codereview.chromium.org/371603003/diff/140001/mojo/bindings/java/src/... mojo/bindings/java/src/org/chromium/mojo/bindings/MessageWithHeader.java:33: * Reinterpret the given message as a message with header. On 2014/07/16 15:06:44, rmcilroy wrote: > Reinterpret the given |message| as a message with a header. The > |message| must contain a header as the start of it's raw data, which will be > parsed by this constructor. Done. https://codereview.chromium.org/371603003/diff/140001/mojo/bindings/java/src/... File mojo/bindings/java/src/org/chromium/mojo/bindings/Struct.java (right): https://codereview.chromium.org/371603003/diff/140001/mojo/bindings/java/src/... mojo/bindings/java/src/org/chromium/mojo/bindings/Struct.java:74: return true; On 2014/07/16 15:06:44, rmcilroy wrote: > ditto with MessageHeader equals() comment. Done, with the same comment about not removing new lines.
https://codereview.chromium.org/371603003/diff/140001/mojo/bindings/java/src/... File mojo/bindings/java/src/org/chromium/mojo/bindings/MessageHeader.java (right): https://codereview.chromium.org/371603003/diff/140001/mojo/bindings/java/src/... mojo/bindings/java/src/org/chromium/mojo/bindings/MessageHeader.java:196: return true; On 2014/07/16 15:58:38, qsr wrote: > On 2014/07/16 15:06:44, rmcilroy wrote: > > nit - I think this would be cleaner as: > > > > if (object == this) return true; > > if (object == null) return false; > > if (getClass() != object.getClass()) return false; > > Didn't remove the new line, because my auto-formatter put it back on 2 lines, > and I'd like to be able to continue to use it. > > > > > MessageHeader other = (MessageHeader) object; > > return ((mDataHeader == null && other.mDataHeader == null) || > > (mDataHeader.equals(other.mDataHeader))) && > > This will crash is this.mDataHeader is null. You need the double if for this > kind of case. Will do the rest of the changes. In that case you can do: Objects.equals(mDataHeader, other.mDataHeader) - which will handle the null check for you. > > > mFlags == other.mFlags && > > mRequestId == other.mRequestId && > > mType == other.mType; > https://codereview.chromium.org/371603003/diff/180001/mojo/bindings/java/src/... File mojo/bindings/java/src/org/chromium/mojo/bindings/MessageHeader.java (right): https://codereview.chromium.org/371603003/diff/180001/mojo/bindings/java/src/... mojo/bindings/java/src/org/chromium/mojo/bindings/MessageHeader.java:186: MessageHeader other = (MessageHeader) object; nit - newline above here
follow review
https://chromiumcodereview.appspot.com/371603003/diff/140001/mojo/bindings/ja... File mojo/bindings/java/src/org/chromium/mojo/bindings/MessageHeader.java (right): https://chromiumcodereview.appspot.com/371603003/diff/140001/mojo/bindings/ja... mojo/bindings/java/src/org/chromium/mojo/bindings/MessageHeader.java:196: return true; On 2014/07/16 16:50:01, rmcilroy wrote: > On 2014/07/16 15:58:38, qsr wrote: > > On 2014/07/16 15:06:44, rmcilroy wrote: > > > nit - I think this would be cleaner as: > > > > > > if (object == this) return true; > > > if (object == null) return false; > > > if (getClass() != object.getClass()) return false; > > > > Didn't remove the new line, because my auto-formatter put it back on 2 lines, > > and I'd like to be able to continue to use it. > > > > > > > > MessageHeader other = (MessageHeader) object; > > > return ((mDataHeader == null && other.mDataHeader == null) || > > > (mDataHeader.equals(other.mDataHeader))) && > > > > This will crash is this.mDataHeader is null. You need the double if for this > > kind of case. Will do the rest of the changes. > > In that case you can do: > Objects.equals(mDataHeader, other.mDataHeader) - which will handle the null > check for you. > > > > > mFlags == other.mFlags && > > > mRequestId == other.mRequestId && > > > mType == other.mType; > > > > Done. https://chromiumcodereview.appspot.com/371603003/diff/180001/mojo/bindings/ja... File mojo/bindings/java/src/org/chromium/mojo/bindings/MessageHeader.java (right): https://chromiumcodereview.appspot.com/371603003/diff/180001/mojo/bindings/ja... mojo/bindings/java/src/org/chromium/mojo/bindings/MessageHeader.java:186: MessageHeader other = (MessageHeader) object; On 2014/07/16 16:50:01, rmcilroy wrote: > nit - newline above here Done.
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/371603003/200001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_aosp on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/9...) android_chromium_gn_compile_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...) android_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...) android_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/20...) chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...) ios_dbg_simulator on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...) ios_rel_device on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...) ios_rel_device_ninja on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...) linux_chromium_chromeos_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) linux_chromium_chromeos_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) linux_chromium_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...) linux_chromium_gn_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...) linux_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...) mac_chromium_compile_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...) mac_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...) win8_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win8_chromium_rel/bui...) win_chromium_compile_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...) win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...) win_chromium_x64_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...) linux_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...) mac_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/29577)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_dbg_simulator on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...) ios_rel_device on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...) ios_rel_device_ninja on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...) mac_chromium_compile_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...) mac_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/29606)
rebase
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/371603003/220001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...) android_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/20...)
The CQ bit was checked by qsr@chromium.org
Try jobs failed on following builders: android_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...) android_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/20...)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/qsr@chromium.org/371603003/240001
Message was sent while issue was closed.
Change committed as 283767
Message was sent while issue was closed.
A revert of this CL has been created in https://codereview.chromium.org/392223004/ by johnme@chromium.org. The reason for reverting is: This appears to have broken mojotest_instrumentation_tests with the errors listed below. http://build.chromium.org/p/chromium.webkit/builders/Android%20Tests%20%28dbg... C 52.086s Main [FAIL] org.chromium.mojo.bindings.ConnectorTest#testErrors: C 52.086s Main java.lang.NoClassDefFoundError: java.util.Objects C 52.086s Main at org.chromium.mojo.bindings.MessageHeader.equals(MessageHeader.java:189) C 52.086s Main at org.chromium.mojo.bindings.MessageWithHeader.<init>(MessageWithHeader.java:29) C 52.086s Main at org.chromium.mojo.bindings.MessageWithHeader.<init>(MessageWithHeader.java:39) C 52.086s Main at org.chromium.mojo.bindings.BindingsTestUtils.newRandomMessageWithHeader(BindingsTestUtils.java:85) C 52.086s Main at org.chromium.mojo.bindings.ConnectorTest.setUp(ConnectorTest.java:53) C 52.086s Main at android.test.AndroidTestRunner.runTest(AndroidTestRunner.java:191) C 52.086s Main at android.test.AndroidTestRunner.runTest(AndroidTestRunner.java:176) C 52.086s Main at android.test.InstrumentationTestRunner.onStart(InstrumentationTestRunner.java:554) C 52.086s Main at android.app.Instrumentation$InstrumentationThread.run(Instrumentation.java:1701) C 52.086s Main [FAIL] org.chromium.mojo.bindings.ConnectorTest#testReceivingMessage: C 52.086s Main java.lang.NoClassDefFoundError: java.util.Objects C 52.086s Main at org.chromium.mojo.bindings.MessageHeader.equals(MessageHeader.java:189) C 52.086s Main at org.chromium.mojo.bindings.MessageWithHeader.<init>(MessageWithHeader.java:29) C 52.086s Main at org.chromium.mojo.bindings.MessageWithHeader.<init>(MessageWithHeader.java:39) C 52.086s Main at org.chromium.mojo.bindings.BindingsTestUtils.newRandomMessageWithHeader(BindingsTestUtils.java:85) C 52.087s Main at org.chromium.mojo.bindings.ConnectorTest.setUp(ConnectorTest.java:53) C 52.087s Main at android.test.AndroidTestRunner.runTest(AndroidTestRunner.java:191) C 52.087s Main at android.test.AndroidTestRunner.runTest(AndroidTestRunner.java:176) C 52.087s Main at android.test.InstrumentationTestRunner.onStart(InstrumentationTestRunner.java:554) C 52.087s Main at android.app.Instrumentation$InstrumentationThread.run(Instrumentation.java:1701) C 52.087s Main [FAIL] org.chromium.mojo.bindings.ConnectorTest#testSendingMessage: C 52.087s Main java.lang.NoClassDefFoundError: java.util.Objects C 52.087s Main at org.chromium.mojo.bindings.MessageHeader.equals(MessageHeader.java:189) C 52.087s Main at org.chromium.mojo.bindings.MessageWithHeader.<init>(MessageWithHeader.java:29) C 52.087s Main at org.chromium.mojo.bindings.MessageWithHeader.<init>(MessageWithHeader.java:39) C 52.087s Main at org.chromium.mojo.bindings.BindingsTestUtils.newRandomMessageWithHeader(BindingsTestUtils.java:85) C 52.087s Main at org.chromium.mojo.bindings.ConnectorTest.setUp(ConnectorTest.java:53) C 52.087s Main at android.test.AndroidTestRunner.runTest(AndroidTestRunner.java:191) C 52.087s Main at android.test.AndroidTestRunner.runTest(AndroidTestRunner.java:176) C 52.087s Main at android.test.InstrumentationTestRunner.onStart(InstrumentationTestRunner.java:554) C 52.087s Main at android.app.Instrumentation$InstrumentationThread.run(Instrumentation.java:1701) C 52.087s Main [FAIL] org.chromium.mojo.bindings.MessageHeaderTest#testMessageWithRequestIdHeader: C 52.087s Main java.lang.NoClassDefFoundError: java.util.Objects C 52.087s Main at org.chromium.mojo.bindings.MessageHeader.equals(MessageHeader.java:189) C 52.088s Main at org.chromium.mojo.bindings.MessageWithHeader.<init>(MessageWithHeader.java:29) C 52.088s Main at org.chromium.mojo.bindings.Struct.serializeWithHeader(Struct.java:116) C 52.088s Main at org.chromium.mojo.bindings.MessageHeaderTest.testMessageWithRequestIdHeader(MessageHeaderTest.java:54) C 52.088s Main at java.lang.reflect.Method.invokeNative(Native Method) C 52.088s Main at android.test.AndroidTestRunner.runTest(AndroidTestRunner.java:191) C 52.088s Main at android.test.AndroidTestRunner.runTest(AndroidTestRunner.java:176) C 52.088s Main at android.test.InstrumentationTestRunner.onStart(InstrumentationTestRunner.java:554) C 52.088s Main at android.app.Instrumentation$InstrumentationThread.run(Instrumentation.java:1701) C 52.088s Main [FAIL] org.chromium.mojo.bindings.MessageHeaderTest#testSimpleMessageHeader: C 52.088s Main java.lang.NoClassDefFoundError: java.util.Objects C 52.088s Main at org.chromium.mojo.bindings.MessageHeader.equals(MessageHeader.java:189) C 52.088s Main at org.chromium.mojo.bindings.MessageWithHeader.<init>(MessageWithHeader.java:29) C 52.088s Main at org.chromium.mojo.bindings.Struct.serializeWithHeader(Struct.java:116) C 52.088s Main at org.chromium.mojo.bindings.MessageHeaderTest.testSimpleMessageHeader(MessageHeaderTest.java:29) C 52.088s Main at java.lang.reflect.Method.invokeNative(Native Method) C 52.088s Main at android.test.AndroidTestRunner.runTest(AndroidTestRunner.java:191) C 52.088s Main at android.test.AndroidTestRunner.runTest(AndroidTestRunner.java:176) C 52.088s Main at android.test.InstrumentationTestRunner.onStart(InstrumentationTestRunner.java:554) C 52.089s Main at android.app.Instrumentation$InstrumentationThread.run(Instrumentation.java:1701) C 52.089s Main [FAIL] org.chromium.mojo.bindings.MessageWithHeaderTest#testReadAndDispatchMessage: C 52.089s Main java.lang.NoClassDefFoundError: java.util.Objects C 52.089s Main at org.chromium.mojo.bindings.MessageHeader.equals(MessageHeader.java:189) C 52.089s Main at org.chromium.mojo.bindings.MessageWithHeader.<init>(MessageWithHeader.java:29) C 52.089s Main at org.chromium.mojo.bindings.MessageWithHeader.<init>(MessageWithHeader.java:39) C 52.089s Main at org.chromium.mojo.bindings.BindingsTestUtils.newRandomMessageWithHeader(BindingsTestUtils.java:85) C 52.089s Main at org.chromium.mojo.bindings.MessageWithHeaderTest.setUp(MessageWithHeaderTest.java:45) C 52.089s Main at android.test.AndroidTestRunner.runTest(AndroidTestRunner.java:191) C 52.089s Main at android.test.AndroidTestRunner.runTest(AndroidTestRunner.java:176) C 52.089s Main at android.test.InstrumentationTestRunner.onStart(InstrumentationTestRunner.java:554) C 52.089s Main at android.app.Instrumentation$InstrumentationThread.run(Instrumentation.java:1701) C 52.089s Main [FAIL] org.chromium.mojo.bindings.MessageWithHeaderTest#testReadAndDispatchMessageOnClosedHandle: C 52.089s Main java.lang.NoClassDefFoundError: java.util.Objects C 52.089s Main at org.chromium.mojo.bindings.MessageHeader.equals(MessageHeader.java:189) C 52.089s Main at org.chromium.mojo.bindings.MessageWithHeader.<init>(MessageWithHeader.java:29) C 52.089s Main at org.chromium.mojo.bindings.MessageWithHeader.<init>(MessageWithHeader.java:39) C 52.089s Main at org.chromium.mojo.bindings.BindingsTestUtils.newRandomMessageWithHeader(BindingsTestUtils.java:85) C 52.089s Main at org.chromium.mojo.bindings.MessageWithHeaderTest.setUp(MessageWithHeaderTest.java:45) C 52.090s Main at android.test.AndroidTestRunner.runTest(AndroidTestRunner.java:191) C 52.090s Main at android.test.AndroidTestRunner.runTest(AndroidTestRunner.java:176) C 52.090s Main at android.test.InstrumentationTestRunner.onStart(InstrumentationTestRunner.java:554) C 52.090s Main at android.app.Instrumentation$InstrumentationThread.run(Instrumentation.java:1701) C 52.090s Main [FAIL] org.chromium.mojo.bindings.MessageWithHeaderTest#testReadAndDispatchMessageOnEmptyHandle: C 52.090s Main java.lang.NoClassDefFoundError: java.util.Objects C 52.090s Main at org.chromium.mojo.bindings.MessageHeader.equals(MessageHeader.java:189) C 52.090s Main at org.chromium.mojo.bindings.MessageWithHeader.<init>(MessageWithHeader.java:29) C 52.090s Main at org.chromium.mojo.bindings.MessageWithHeader.<init>(MessageWithHeader.java:39) C 52.090s Main at org.chromium.mojo.bindings.BindingsTestUtils.newRandomMessageWithHeader(BindingsTestUtils.java:85) C 52.090s Main at org.chromium.mojo.bindings.MessageWithHeaderTest.setUp(MessageWithHeaderTest.java:45) C 52.090s Main at android.test.AndroidTestRunner.runTest(AndroidTestRunner.java:191) C 52.090s Main at android.test.AndroidTestRunner.runTest(AndroidTestRunner.java:176) C 52.090s Main at android.test.InstrumentationTestRunner.onStart(InstrumentationTestRunner.java:554) C 52.090s Main at android.app.Instrumentation$InstrumentationThread.run(Instrumentation.java:1701) C 52.090s Main [FAIL] org.chromium.mojo.bindings.RouterTest#testSendingToRouterWithResponse: C 52.090s Main java.lang.NoClassDefFoundError: java.util.Objects C 52.090s Main at org.chromium.mojo.bindings.MessageHeader.equals(MessageHeader.java:189) C 52.091s Main at org.chromium.mojo.bindings.MessageWithHeader.<init>(MessageWithHeader.java:29) C 52.091s Main at org.chromium.mojo.bindings.MessageWithHeader.<init>(MessageWithHeader.java:39) C 52.091s Main at org.chromium.mojo.bindings.RouterTest.testSendingToRouterWithResponse(RouterTest.java:64) C 52.091s Main at java.lang.reflect.Method.invokeNative(Native Method) C 52.091s Main at android.test.InstrumentationTestCase.runMethod(InstrumentationTestCase.java:214) C 52.091s Main at android.test.InstrumentationTestCase.runTest(InstrumentationTestCase.java:199) C 52.091s Main at android.test.AndroidTestRunner.runTest(AndroidTestRunner.java:191) C 52.091s Main at android.test.AndroidTestRunner.runTest(AndroidTestRunner.java:176) C 52.091s Main at android.test.InstrumentationTestRunner.onStart(InstrumentationTestRunner.java:554) C 52.091s Main at android.app.Instrumentation$InstrumentationThread.run(Instrumentation.java:1701) C 52.091s Main [CRASH] org.chromium.mojo.bindings.RouterTest#testReceivingViaRouterWithResponse: C 52.091s Main Native crash: Segmentation fault.
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/371603003/260001
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/20...) win_chromium_compile_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/20...)
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/371603003/260001
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/20...)
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/371603003/280001
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...)
Message was sent while issue was closed.
Change committed as 284170 |
