Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1412243012:
Initial implementation of CronetBidirectionalStream. (Closed)
DescriptionInitial implementation of CronetBidirectionalStream.
BUG=516342
Committed: https://crrev.com/72e10340f0078cc85d10ae9654cec3e91b46b128
Cr-Commit-Position: refs/heads/master@{#372486}
Patch Set 1 #Patch Set 2 : Rebase onto net::BidirectionalStream CL #Patch Set 3 : Compile with net::BidirectionalStream #Patch Set 4 : Sync #Patch Set 5 : Simple case mostly works. #Patch Set 6 : Minor self-review #Patch Set 7 : net::BidirectionalStream properly establishes the session. #Patch Set 8 : Sync #Patch Set 9 : Use netty-based test server on the host. #Patch Set 10 : Sync #Patch Set 11 : Add moar test against local H2 server. #Patch Set 12 : Added testEchoTrailers method, but it is failing. #Patch Set 13 : Sync and make trailers work. #Patch Set 14 : Sync #Patch Set 15 : Change flags to states, add cancel and throw tests. #Patch Set 16 : Little cleanup. #
Total comments: 25
Patch Set 17 : Sync, address comments, use GetTotalReceivedBytes(). #Patch Set 18 : Sync and add simple 'HEAD' test. #Patch Set 19 : Use inproc Netty Http2 server #Patch Set 20 : Added engine shutdown tests. #Patch Set 21 : Sync #
Total comments: 48
Patch Set 22 : Implement BidirectionalStream priority. #Patch Set 23 : Sync #Patch Set 24 : Recognize enable_bidirectional_stream gyp variable. #Patch Set 25 : Sync, use BUILDFLAG(ENABLE_BIDIRECTIONAL_STREAM). #Patch Set 26 : Address Helen's comments. #
Total comments: 57
Patch Set 27 : Sync and fix build errors. #Patch Set 28 : Address Helen's and Paul's comments. #Patch Set 29 : Added isDoneLocked(). #Patch Set 30 : Update BUILD.gn to support enable_bidirectional_stream flag. #Patch Set 31 : Sync #
Total comments: 19
Patch Set 32 : Address Paul's comments. #Patch Set 33 : Address Paul's comments. #
Total comments: 35
Patch Set 34 : Add Http2 Test Server based in Netty4.1 #Patch Set 35 : Address Paul's comments, add more tests. #Patch Set 36 : Destroy the native adapter instead of cancel if can't post task to executor. #
Total comments: 97
Patch Set 37 : Address Paul's comments. #Patch Set 38 : Self review. #
Total comments: 85
Patch Set 39 : Address Helen's comments. #
Total comments: 74
Patch Set 40 : More Helen's and Andrei's comments. #
Total comments: 6
Patch Set 41 : Removed State.WRITING_END_OF_STREAM #
Total comments: 38
Patch Set 42 : Address Andrei's and Helen's comments. #Patch Set 43 : camelCase. #Patch Set 44 : Log the exception. #
Total comments: 38
Patch Set 45 : Address Helen's comments. #
Total comments: 16
Patch Set 46 : Address Andrei's comments. #Patch Set 47 : Make gn work. #Patch Set 48 : Introducing RequestResponder! #
Total comments: 18
Patch Set 49 : Sync, address Helen's comment. #Patch Set 50 : Address Andrei's comments. #Patch Set 51 : Sync #Patch Set 52 : Fix javadoc link. #Messages
Total messages: 75 (11 generated)
Patchset #2 (id:20001) has been deleted
mef@chromium.org changed reviewers: + pauljensen@chromium.org, xunjieli@chromium.org
FYI, it is NOT ready for the review, just sharing my excitement to see a working prototype.
On 2015/11/10 22:21:48, mef wrote: > FYI, it is NOT ready for the review, just sharing my excitement to see a working > prototype. Great! This came at the right time. Was scratching my head why the connection establishment doesn't work (couldn't get it to repro using the unit test). Now I can use this to debug. Thanks!
On 2015/11/10 22:25:06, xunjieli wrote: > On 2015/11/10 22:21:48, mef wrote: > > FYI, it is NOT ready for the review, just sharing my excitement to see a > working > > prototype. > > Great! This came at the right time. Was scratching my head why the connection > establishment doesn't work (couldn't get it to repro using the unit test). Now I > can use this to debug. Thanks! I fixed the session establish bug that you observed earlier. The cause is that I didn't copy out the ALPN and NPN protos into SSLConfig (The tls handshake is mocked out in tests, so it wasn't caught). It should work now. Could you try again?
On 2015/11/13 23:43:49, xunjieli wrote: > On 2015/11/10 22:25:06, xunjieli wrote: > > On 2015/11/10 22:21:48, mef wrote: > > > FYI, it is NOT ready for the review, just sharing my excitement to see a > > working > > > prototype. > > > > Great! This came at the right time. Was scratching my head why the connection > > establishment doesn't work (couldn't get it to repro using the unit test). Now > I > > can use this to debug. Thanks! > > I fixed the session establish bug that you observed earlier. The cause is that I > didn't copy out the ALPN and NPN protos into SSLConfig (The tls handshake is > mocked out in tests, so it wasn't caught). It should work now. Could you try > again? Thanks, it works as expected now!
PTAL when you have a chance. It is missing received bytes count, negotiated protocol, ping and window update, but it is pretty functional. It needs more tests (I'm open for ideas) and will depend on a separate CL that sets up the test server. Not a priority, but I will definitely appreciate any comments. https://codereview.chromium.org/1412243012/diff/320001/components/cronet/andr... File components/cronet/android/test/mock_cert_verifier.cc (right): https://codereview.chromium.org/1412243012/diff/320001/components/cronet/andr... components/cronet/android/test/mock_cert_verifier.cc:78: mock_cert_verifier->set_default_result(net::OK); This will likely be unnecessary if we can run server on the device and use regular test certs.
https://codereview.chromium.org/1412243012/diff/320001/components/cronet/andr... File components/cronet/android/cronet_bidirectional_stream.h (right): https://codereview.chromium.org/1412243012/diff/320001/components/cronet/andr... components/cronet/android/cronet_bidirectional_stream.h:25: class SingleThreadTaskRunner; I don't see this being used. https://codereview.chromium.org/1412243012/diff/320001/components/cronet/andr... components/cronet/android/cronet_bidirectional_stream.h:29: enum Error; I believe the style guide recommends against forward declaring enums. https://codereview.chromium.org/1412243012/diff/320001/components/cronet/andr... components/cronet/android/cronet_bidirectional_stream.h:34: class UploadDataStream; UploadDataStream is not used. https://codereview.chromium.org/1412243012/diff/320001/components/cronet/andr... components/cronet/android/cronet_bidirectional_stream.h:48: // on any thread except PopulateResponseHeaders and Get* methods, which can only nit: outdated comment. There isn't a PopulateResponseHeaders. https://codereview.chromium.org/1412243012/diff/320001/components/cronet/andr... components/cronet/android/cronet_bidirectional_stream.h:132: // Java object that owns this CronetURLRequestContextAdapter. nit: s/CronetURLRequestContextAdapter/CrnetBidirectionalStream https://codereview.chromium.org/1412243012/diff/320001/components/cronet/andr... File components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java (right): https://codereview.chromium.org/1412243012/diff/320001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:70: private String mInitialMethod; nit: final field. https://codereview.chromium.org/1412243012/diff/320001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:162: onSucceeded(); maybe consider inline onSucceeded? since the method is only used here. Should we also talk to gRPC on whether they need this onSucceeded callback? According to ejona@ in the last discussion, they might not need it. https://codereview.chromium.org/1412243012/diff/320001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:308: mWriteState = STATE_WRITING_DONE; After applying your latest suggested edit, client now needs to sendData with END_STREAM (even for GETs) to signal writing done. This is done to match the receive data case.
Sorry about sending comments in batches. Here are a few more. https://codereview.chromium.org/1412243012/diff/320001/components/cronet/andr... File components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java (right): https://codereview.chromium.org/1412243012/diff/320001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:33: @GuardedBy("mNativeStreamLock") private long mNativeStream; nit: Maybe reorder these fields. Static fields before none-static. Final ones before non-final. https://codereview.chromium.org/1412243012/diff/320001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:53: private static final int STATE_WRITING_DONE = 13; Need a comment for each of these states. It's unclear why writing has a END_OF_STREAM, but reading does not. https://codereview.chromium.org/1412243012/diff/320001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:57: * WRITING_END_OF_STREAM -> WRITING_DONE -> SUCCESS Maybe consider combining the read state flow with the write state flow. Not sure what is the best way. Maybe we could use some pseudo grammar. Something like: NOT_STARTED -> STARTED -> ((WAITING_ON_READ -> READING -> WAITING_ON_READ -> READING -> READING_DONE)* (WAITING_ON_WRITE -> WRITING -> WAITING_ON_WRITE -> WRITING_END_OF_STREAM -> WRITING_DONE)*)* -> SUCCESS where arrows indicates state transition, brackets indicate groupings, and * indicate repetitions of any number of times. We need to also include STATE_CANCELLED and STATE_ERROR in the transition. https://codereview.chromium.org/1412243012/diff/320001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:97: "OnReadCompletedRunnable:" + mByteBuffer.toString()); nit: This probably shouldn't be Log.e. Maybe we should get rid of it entirely.
Thanks, Helen! PTAL. https://codereview.chromium.org/1412243012/diff/320001/components/cronet/andr... File components/cronet/android/cronet_bidirectional_stream.h (right): https://codereview.chromium.org/1412243012/diff/320001/components/cronet/andr... components/cronet/android/cronet_bidirectional_stream.h:25: class SingleThreadTaskRunner; On 2015/12/08 17:59:55, xunjieli wrote: > I don't see this being used. Done. https://codereview.chromium.org/1412243012/diff/320001/components/cronet/andr... components/cronet/android/cronet_bidirectional_stream.h:29: enum Error; On 2015/12/08 17:59:55, xunjieli wrote: > I believe the style guide recommends against forward declaring enums. Done. https://codereview.chromium.org/1412243012/diff/320001/components/cronet/andr... components/cronet/android/cronet_bidirectional_stream.h:34: class UploadDataStream; On 2015/12/08 17:59:55, xunjieli wrote: > UploadDataStream is not used. Done. https://codereview.chromium.org/1412243012/diff/320001/components/cronet/andr... components/cronet/android/cronet_bidirectional_stream.h:48: // on any thread except PopulateResponseHeaders and Get* methods, which can only On 2015/12/08 17:59:55, xunjieli wrote: > nit: outdated comment. There isn't a PopulateResponseHeaders. Done. https://codereview.chromium.org/1412243012/diff/320001/components/cronet/andr... components/cronet/android/cronet_bidirectional_stream.h:132: // Java object that owns this CronetURLRequestContextAdapter. On 2015/12/08 17:59:55, xunjieli wrote: > nit: s/CronetURLRequestContextAdapter/CrnetBidirectionalStream Done. https://codereview.chromium.org/1412243012/diff/320001/components/cronet/andr... File components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java (right): https://codereview.chromium.org/1412243012/diff/320001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:33: @GuardedBy("mNativeStreamLock") private long mNativeStream; On 2015/12/08 19:07:20, xunjieli wrote: > nit: Maybe reorder these fields. Static fields before none-static. Final ones > before non-final. Done. https://codereview.chromium.org/1412243012/diff/320001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:53: private static final int STATE_WRITING_DONE = 13; On 2015/12/08 19:07:19, xunjieli wrote: > Need a comment for each of these states. It's unclear why writing has a > END_OF_STREAM, but reading does not. Done. https://codereview.chromium.org/1412243012/diff/320001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:57: * WRITING_END_OF_STREAM -> WRITING_DONE -> SUCCESS On 2015/12/08 19:07:20, xunjieli wrote: > Maybe consider combining the read state flow with the write state flow. Not sure > what is the best way. Maybe we could use some pseudo grammar. Something like: > > NOT_STARTED -> STARTED -> ((WAITING_ON_READ -> READING -> WAITING_ON_READ -> > READING -> READING_DONE)* (WAITING_ON_WRITE -> WRITING -> WAITING_ON_WRITE -> > WRITING_END_OF_STREAM -> WRITING_DONE)*)* -> SUCCESS > > where arrows indicates state transition, brackets indicate groupings, and * > indicate repetitions of any number of times. > > We need to also include STATE_CANCELLED and STATE_ERROR in the transition. Yeah, I'm not sure what's a good way to express the flow. I've tried to describe it in the comment. https://codereview.chromium.org/1412243012/diff/320001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:70: private String mInitialMethod; On 2015/12/08 17:59:56, xunjieli wrote: > nit: final field. Done. https://codereview.chromium.org/1412243012/diff/320001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:97: "OnReadCompletedRunnable:" + mByteBuffer.toString()); On 2015/12/08 19:07:19, xunjieli wrote: > nit: This probably shouldn't be Log.e. Maybe we should get rid of it entirely. Done. Used those for debugging. https://codereview.chromium.org/1412243012/diff/320001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:162: onSucceeded(); On 2015/12/08 17:59:56, xunjieli wrote: > maybe consider inline onSucceeded? since the method is only used here. Done. > Should we also talk to gRPC on whether they need this onSucceeded callback? > According to ejona@ in the last discussion, they might not need it. sgtm, will do. https://codereview.chromium.org/1412243012/diff/320001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:308: mWriteState = STATE_WRITING_DONE; On 2015/12/08 17:59:56, xunjieli wrote: > After applying your latest suggested edit, client now needs to sendData with > END_STREAM (even for GETs) to signal writing done. This is done to match the > receive data case. Per our discussion requiring at least one write is wrong.
Patchset #18 (id:360001) has been deleted
mostly nits for now.. https://codereview.chromium.org/1412243012/diff/440001/components/cronet/andr... File components/cronet/android/cronet_bidirectional_stream.cc (right): https://codereview.chromium.org/1412243012/diff/440001/components/cronet/andr... components/cronet/android/cronet_bidirectional_stream.cc:7: #include <limits> nit: not used? https://codereview.chromium.org/1412243012/diff/440001/components/cronet/andr... components/cronet/android/cronet_bidirectional_stream.cc:8: #include <vector> nit: also #include <string> https://codereview.chromium.org/1412243012/diff/440001/components/cronet/andr... components/cronet/android/cronet_bidirectional_stream.cc:67: jobject jbyte_buffer, nit: the new recommendation is not to pass plain jobject, but to use "const JavaParamRef<jobject>&". Same goes to other instances. https://codereview.chromium.org/1412243012/diff/440001/components/cronet/andr... components/cronet/android/cronet_bidirectional_stream.cc:112: request_info->url = GURL(base::android::ConvertJavaStringToUTF8(env, jurl)); nit: We have "using base::android::ConvertUTF8ToJavaString;", but not for ConvertJavaStringToUTF8. We should not somehow standardize. https://codereview.chromium.org/1412243012/diff/440001/components/cronet/andr... components/cronet/android/cronet_bidirectional_stream.cc:147: *request_info, net::DEFAULT_PRIORITY, context_->GetURLRequestContext() Priority is now folded into RequestInfo. https://codereview.chromium.org/1412243012/diff/440001/components/cronet/andr... File components/cronet/android/cronet_bidirectional_stream.h (right): https://codereview.chromium.org/1412243012/diff/440001/components/cronet/andr... components/cronet/android/cronet_bidirectional_stream.h:15: #include "base/callback.h" nit: not used? https://codereview.chromium.org/1412243012/diff/440001/components/cronet/andr... components/cronet/android/cronet_bidirectional_stream.h:16: #include "base/location.h" nit: not used? https://codereview.chromium.org/1412243012/diff/440001/components/cronet/andr... File components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java (right): https://codereview.chromium.org/1412243012/diff/440001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:7: import android.util.Log; we should avoid using android.util.Log in new files. See https://code.google.com/p/chromium/issues/detail?id=488547 https://codereview.chromium.org/1412243012/diff/440001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:107: private Runnable mOnDestroyedCallbackForTests; nit: maybe using ForTesting. I searched in cronet/, there are more instances of ForTesting, though ForTests is shorter. We should pick one and change the rest. https://codereview.chromium.org/1412243012/diff/440001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:111: boolean mEndOfStream; nit: need documentation on these two fields. https://codereview.chromium.org/1412243012/diff/440001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:143: ByteBuffer mByteBuffer; nit: need doc on this mByteBuffer field. https://codereview.chromium.org/1412243012/diff/440001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:206: CronetBidirectionalStream(CronetUrlRequestContext requestContext, long urlRequestContextAdapter, nit: move constructor to the top. At least before the two private methods. https://codereview.chromium.org/1412243012/diff/440001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:214: mRequestHeaders = new ArrayList<Map.Entry<String, String>>(requestHeaders); Is this a deep copy? https://codereview.chromium.org/1412243012/diff/440001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:300: // TODO(mef): May be last thing to be implemented on Android. nit: throw UnsupportedOperation with a not yet supported message for this and below. https://codereview.chromium.org/1412243012/diff/440001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:320: public void cancel() { nit: add @override, and remove documentation. https://codereview.chromium.org/1412243012/diff/440001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:483: public void setOnDestroyedCallbackForTests(Runnable onDestroyedCallbackForTests) { nit: ForTests vs ForTesting. Should we standardize on one form? https://codereview.chromium.org/1412243012/diff/440001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:503: private ArrayList<Map.Entry<String, String>> headersListFromStrings(String[] headers) { nit: private static. https://codereview.chromium.org/1412243012/diff/440001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:513: private String[] stringsFromHeaderList(ArrayList<Map.Entry<String, String>> headersList) { nit: private static. https://codereview.chromium.org/1412243012/diff/440001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:533: // safe to preserve and use urlRequestAdapter outside the lock. nit: need to update comment. s/mUrlRequestAdapter/mNativeStream
https://codereview.chromium.org/1412243012/diff/440001/components/cronet/andr... File components/cronet/android/cronet_bidirectional_stream.cc (right): https://codereview.chromium.org/1412243012/diff/440001/components/cronet/andr... components/cronet/android/cronet_bidirectional_stream.cc:49: CronetBidirectionalStream* adapter = nit: maybe s/adapter/stream, or rename this class to CronetBidirectionalStreamAdapter. https://codereview.chromium.org/1412243012/diff/440001/components/cronet/andr... File components/cronet/android/cronet_bidirectional_stream.h (right): https://codereview.chromium.org/1412243012/diff/440001/components/cronet/andr... components/cronet/android/cronet_bidirectional_stream.h:42: class CronetBidirectionalStream : public net::BidirectionalStream::Delegate { I think this should be named CronetBidirectionalStreamAdapter, since it is not a subclass of BidirectionalStream, but instead it owns a BidirectionalStream and implements the delegate interface. Might be more consistent to other classes (CronetUrlRequestAdapter, CronetUploadDataStreamAdapter) https://codereview.chromium.org/1412243012/diff/440001/components/cronet/andr... components/cronet/android/cronet_bidirectional_stream.h:74: // issued to indicate when no more callbacks will be issued. nit: s/indicate when/indicate that https://codereview.chromium.org/1412243012/diff/440001/components/cronet/andr... components/cronet/android/cronet_bidirectional_stream.h:85: // Called when the request headers have been sent. nit: override methods do not need documentation. The empty lines between them could be removed. https://codereview.chromium.org/1412243012/diff/440001/components/cronet/andr... File components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java (right): https://codereview.chromium.org/1412243012/diff/440001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:84: @GuardedBy("mNativeStreamLock") private int mStreamState = STATE_NOT_STARTED; nit: mReadState?
Paul - ping! Helen - thanks for your comments! https://codereview.chromium.org/1412243012/diff/440001/components/cronet/andr... File components/cronet/android/cronet_bidirectional_stream.cc (right): https://codereview.chromium.org/1412243012/diff/440001/components/cronet/andr... components/cronet/android/cronet_bidirectional_stream.cc:7: #include <limits> On 2015/12/18 19:11:11, xunjieli (OOO1223-0118) wrote: > nit: not used? Done. https://codereview.chromium.org/1412243012/diff/440001/components/cronet/andr... components/cronet/android/cronet_bidirectional_stream.cc:8: #include <vector> On 2015/12/18 19:11:11, xunjieli (OOO1223-0118) wrote: > nit: also #include <string> Done. https://codereview.chromium.org/1412243012/diff/440001/components/cronet/andr... components/cronet/android/cronet_bidirectional_stream.cc:49: CronetBidirectionalStream* adapter = On 2015/12/18 19:49:25, xunjieli (OOO1223-0118) wrote: > nit: maybe s/adapter/stream, or rename this class to > CronetBidirectionalStreamAdapter. I'll rename CronetBidirectionalStream -> CronetBidirectionalStreamAdapter. https://codereview.chromium.org/1412243012/diff/440001/components/cronet/andr... components/cronet/android/cronet_bidirectional_stream.cc:67: jobject jbyte_buffer, On 2015/12/18 19:11:11, xunjieli (OOO1223-0118) wrote: > nit: the new recommendation is not to pass plain jobject, but to use "const > JavaParamRef<jobject>&". Same goes to other instances. Done. https://codereview.chromium.org/1412243012/diff/440001/components/cronet/andr... components/cronet/android/cronet_bidirectional_stream.cc:112: request_info->url = GURL(base::android::ConvertJavaStringToUTF8(env, jurl)); On 2015/12/18 19:11:11, xunjieli (OOO1223-0118) wrote: > nit: We have "using base::android::ConvertUTF8ToJavaString;", but not for > ConvertJavaStringToUTF8. We should not somehow standardize. Done. https://codereview.chromium.org/1412243012/diff/440001/components/cronet/andr... components/cronet/android/cronet_bidirectional_stream.cc:147: *request_info, net::DEFAULT_PRIORITY, context_->GetURLRequestContext() On 2015/12/18 19:11:11, xunjieli (OOO1223-0118) wrote: > Priority is now folded into RequestInfo. Done. https://codereview.chromium.org/1412243012/diff/440001/components/cronet/andr... File components/cronet/android/cronet_bidirectional_stream.h (right): https://codereview.chromium.org/1412243012/diff/440001/components/cronet/andr... components/cronet/android/cronet_bidirectional_stream.h:15: #include "base/callback.h" On 2015/12/18 19:11:11, xunjieli (OOO1223-0118) wrote: > nit: not used? Done. https://codereview.chromium.org/1412243012/diff/440001/components/cronet/andr... components/cronet/android/cronet_bidirectional_stream.h:16: #include "base/location.h" On 2015/12/18 19:11:11, xunjieli (OOO1223-0118) wrote: > nit: not used? Done. https://codereview.chromium.org/1412243012/diff/440001/components/cronet/andr... components/cronet/android/cronet_bidirectional_stream.h:42: class CronetBidirectionalStream : public net::BidirectionalStream::Delegate { On 2015/12/18 19:49:25, xunjieli (OOO1223-0118) wrote: > I think this should be named CronetBidirectionalStreamAdapter, since it is not a > subclass of BidirectionalStream, but instead it owns a BidirectionalStream and > implements the delegate interface. Might be more consistent to other classes > (CronetUrlRequestAdapter, CronetUploadDataStreamAdapter) Acknowledged. Sounds like a good idea, will do in next patch. https://codereview.chromium.org/1412243012/diff/440001/components/cronet/andr... components/cronet/android/cronet_bidirectional_stream.h:74: // issued to indicate when no more callbacks will be issued. On 2015/12/18 19:49:25, xunjieli (OOO1223-0118) wrote: > nit: s/indicate when/indicate that Done. https://codereview.chromium.org/1412243012/diff/440001/components/cronet/andr... components/cronet/android/cronet_bidirectional_stream.h:85: // Called when the request headers have been sent. On 2015/12/18 19:49:25, xunjieli (OOO1223-0118) wrote: > nit: override methods do not need documentation. The empty lines between them > could be removed. Done. https://codereview.chromium.org/1412243012/diff/440001/components/cronet/andr... File components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java (right): https://codereview.chromium.org/1412243012/diff/440001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:7: import android.util.Log; On 2015/12/18 19:11:12, xunjieli (OOO1223-0118) wrote: > we should avoid using android.util.Log in new files. See > https://code.google.com/p/chromium/issues/detail?id=488547 Done. https://codereview.chromium.org/1412243012/diff/440001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:84: @GuardedBy("mNativeStreamLock") private int mStreamState = STATE_NOT_STARTED; On 2015/12/18 19:49:25, xunjieli (OOO1223-0118) wrote: > nit: mReadState? I'm not sure. This is both read and common stream state. https://codereview.chromium.org/1412243012/diff/440001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:107: private Runnable mOnDestroyedCallbackForTests; On 2015/12/18 19:11:11, xunjieli (OOO1223-0118) wrote: > nit: maybe using ForTesting. I searched in cronet/, there are more instances of > ForTesting, though ForTests is shorter. We should pick one and change the rest. Done. https://codereview.chromium.org/1412243012/diff/440001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:111: boolean mEndOfStream; On 2015/12/18 19:11:12, xunjieli (OOO1223-0118) wrote: > nit: need documentation on these two fields. Done. https://codereview.chromium.org/1412243012/diff/440001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:143: ByteBuffer mByteBuffer; On 2015/12/18 19:11:12, xunjieli (OOO1223-0118) wrote: > nit: need doc on this mByteBuffer field. Done. https://codereview.chromium.org/1412243012/diff/440001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:206: CronetBidirectionalStream(CronetUrlRequestContext requestContext, long urlRequestContextAdapter, On 2015/12/18 19:11:11, xunjieli (OOO1223-0118) wrote: > nit: move constructor to the top. At least before the two private methods. Done. https://codereview.chromium.org/1412243012/diff/440001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:214: mRequestHeaders = new ArrayList<Map.Entry<String, String>>(requestHeaders); On 2015/12/18 19:11:11, xunjieli (OOO1223-0118) wrote: > Is this a deep copy? It constructs a list containing elements of passed collection, so I'd say it is shallow. https://codereview.chromium.org/1412243012/diff/440001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:300: // TODO(mef): May be last thing to be implemented on Android. On 2015/12/18 19:11:11, xunjieli (OOO1223-0118) wrote: > nit: throw UnsupportedOperation with a not yet supported message for this and > below. Done. https://codereview.chromium.org/1412243012/diff/440001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:320: public void cancel() { On 2015/12/18 19:11:11, xunjieli (OOO1223-0118) wrote: > nit: add @override, and remove documentation. Done. https://codereview.chromium.org/1412243012/diff/440001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:483: public void setOnDestroyedCallbackForTests(Runnable onDestroyedCallbackForTests) { On 2015/12/18 19:11:11, xunjieli (OOO1223-0118) wrote: > nit: ForTests vs ForTesting. Should we standardize on one form? Yes, and ForTesting is more consistent with @VisibleForTesting, so I'd vote for that. Done. https://codereview.chromium.org/1412243012/diff/440001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:503: private ArrayList<Map.Entry<String, String>> headersListFromStrings(String[] headers) { On 2015/12/18 19:11:12, xunjieli (OOO1223-0118) wrote: > nit: private static. Done. https://codereview.chromium.org/1412243012/diff/440001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:513: private String[] stringsFromHeaderList(ArrayList<Map.Entry<String, String>> headersList) { On 2015/12/18 19:11:12, xunjieli (OOO1223-0118) wrote: > nit: private static. Done. https://codereview.chromium.org/1412243012/diff/440001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:533: // safe to preserve and use urlRequestAdapter outside the lock. On 2015/12/18 19:11:11, xunjieli (OOO1223-0118) wrote: > nit: need to update comment. s/mUrlRequestAdapter/mNativeStream Done.
This is looking in pretty good shape. I've got a lot more reading to do to fully review it. It's a shame we couldn't share more logic between BiDiStream and UrlRequest. Seems almost like a UrlRequest is just using a subset of BiDiStream states/APIs. https://codereview.chromium.org/1412243012/diff/540001/components/cronet/andr... File components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java (right): https://codereview.chromium.org/1412243012/diff/540001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:50: private static final int STATE_CANCELED = 5; no action required side-note: this state is set but never checked for, ditto for STATE_ERROR. https://codereview.chromium.org/1412243012/diff/540001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:89: * WRITING_END_OF_STREAM -> WRITING_DONE -> SUCCESS does mWriteState ever proceed to SUCCESS? https://codereview.chromium.org/1412243012/diff/540001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:132: // Null out mByteBuffer, out of paranoia. Has to be done before nit: "out of paranoia" seems more like to facilitate garbage collection of large buffers. By setting it to null it's explicitly transferring ownership. https://codereview.chromium.org/1412243012/diff/540001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:231: String headers[] = stringsFromHeaderList(mRequestHeaders); no action required side-note: interesting that you consolidated adding all headers at once. Helen made this change for response headers but not request headers, see comment https://code.google.com/p/chromium/issues/detail?id=536119#c4 https://codereview.chromium.org/1412243012/diff/540001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:412: if (byteBuffer.position() != initialPosition) { no action required side-note: Kinda weird to me that we verify position() hasn't changed, even though we're about to overwrite position(). I guess a change to position() is a common indicator of aberrant behavior. Seems like verifying limit() hasn't changed might not be as good an indicator of aberrant behavior but does a better job of verifying things are going to keep working. https://codereview.chromium.org/1412243012/diff/540001/components/cronet/andr... File components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java (right): https://codereview.chromium.org/1412243012/diff/540001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java:152: } I imagine there will be demand for metrics collection for bidi streams. Are you planning this for a future CL? https://codereview.chromium.org/1412243012/diff/540001/components/cronet/andr... File components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamTest.java (right): https://codereview.chromium.org/1412243012/diff/540001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamTest.java:42: assertTrue(Http2TestServer.startHttp2TestServer(getContext(), CERT_USED, KEY_USED)); Hmm so will Andrei's work add Http2TestServer? https://codereview.chromium.org/1412243012/diff/540001/components/cronet/tool... File components/cronet/tools/cr_cronet.py (right): https://codereview.chromium.org/1412243012/diff/540001/components/cronet/tool... components/cronet/tools/cr_cronet.py:71: 'enable_bidirectional_stream=1"' hmm so buildbots won't be building this I guess?
Paul, thanks a lot for your comments! I agree that it could share more code with UrlRequest (maybe by extracting some utility classes), but I don't think that one should be a subclass of another given totally separate underlying net/ implementation. https://codereview.chromium.org/1412243012/diff/540001/components/cronet/andr... File components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java (right): https://codereview.chromium.org/1412243012/diff/540001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:50: private static final int STATE_CANCELED = 5; On 2016/01/04 19:56:00, pauljensen wrote: > no action required side-note: this state is set but never checked for, ditto for > STATE_ERROR. Good point, not sure where to check them. I think those are useful for debugging (maybe I should add a toString() method?). Should I add a checkStateForTesting() or something like that? https://codereview.chromium.org/1412243012/diff/540001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:89: * WRITING_END_OF_STREAM -> WRITING_DONE -> SUCCESS On 2016/01/04 19:56:00, pauljensen wrote: > does mWriteState ever proceed to SUCCESS? Currently it stops at WRITING_DONE. I could fix the comment or fix the mWriteState to be also set to SUCCESS / ERROR / CANCELED. Helen has suggested to rename mStreamState into mReadState to match mWriteState. WDYT? https://codereview.chromium.org/1412243012/diff/540001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:132: // Null out mByteBuffer, out of paranoia. Has to be done before On 2016/01/04 19:56:00, pauljensen wrote: > nit: "out of paranoia" seems more like to facilitate garbage collection of large > buffers. By setting it to null it's explicitly transferring ownership. Good point. The comment hasn't changed since times when mByteBuffer was pointing to internal buffer allocated by UrlRequest. Will change here and below. https://codereview.chromium.org/1412243012/diff/540001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:231: String headers[] = stringsFromHeaderList(mRequestHeaders); On 2016/01/04 19:56:00, pauljensen wrote: > no action required side-note: interesting that you consolidated adding all > headers at once. Helen made this change for response headers but not request > headers, see comment > https://code.google.com/p/chromium/issues/detail?id=536119#c4 She didn't change request headers because we didn't think of a good way to report an error in particular header. I came up with this solution, which is a bit ugly, so I'm open for criticism. https://codereview.chromium.org/1412243012/diff/540001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:412: if (byteBuffer.position() != initialPosition) { On 2016/01/04 19:56:00, pauljensen wrote: > no action required side-note: Kinda weird to me that we verify position() > hasn't changed, even though we're about to overwrite position(). I guess a > change to position() is a common indicator of aberrant behavior. Seems like > verifying limit() hasn't changed might not be as good an indicator of aberrant > behavior but does a better job of verifying things are going to keep working. Good point, I will preserve and check limits as well. https://codereview.chromium.org/1412243012/diff/540001/components/cronet/andr... File components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java (right): https://codereview.chromium.org/1412243012/diff/540001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java:152: } On 2016/01/04 19:56:00, pauljensen wrote: > I imagine there will be demand for metrics collection for bidi streams. Are you > planning this for a future CL? Yes, but not in this CL. https://codereview.chromium.org/1412243012/diff/540001/components/cronet/andr... File components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamTest.java (right): https://codereview.chromium.org/1412243012/diff/540001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamTest.java:42: assertTrue(Http2TestServer.startHttp2TestServer(getContext(), CERT_USED, KEY_USED)); On 2016/01/04 19:56:00, pauljensen wrote: > Hmm so will Andrei's work add Http2TestServer? It is added in my CL https://codereview.chromium.org/1529643002/ that is based on Andrei's CL to build netty-tcnative. https://codereview.chromium.org/1412243012/diff/540001/components/cronet/tool... File components/cronet/tools/cr_cronet.py (right): https://codereview.chromium.org/1412243012/diff/540001/components/cronet/tool... components/cronet/tools/cr_cronet.py:71: 'enable_bidirectional_stream=1"' On 2016/01/04 19:56:01, pauljensen wrote: > hmm so buildbots won't be building this I guess? Chrome build bots won't be building this as Chrome has no use for bidirectional stream. Cronet build bots have this flag set to 1.
https://codereview.chromium.org/1412243012/diff/540001/components/cronet/andr... File components/cronet/android/cronet_bidirectional_stream.cc (right): https://codereview.chromium.org/1412243012/diff/540001/components/cronet/andr... components/cronet/android/cronet_bidirectional_stream.cc:57: // into separate module. when do you think de-duping IOBufferWithByteBuffer will get done? https://codereview.chromium.org/1412243012/diff/540001/components/cronet/andr... components/cronet/android/cronet_bidirectional_stream.cc:158: const JavaParamRef<jobject>& jcaller, how come this definition is "const JavaParamRef<jobject>& jcaller" but the declaration is "jobject jcaller"? can we make them match? ditto for other functions. https://codereview.chromium.org/1412243012/diff/540001/components/cronet/andr... components/cronet/android/cronet_bidirectional_stream.cc:161: jint jcapacity) { jcapacity->jlimit capacity has a very different connotation for ByteBuffers. please also do this in the .java file "native" declarations. https://codereview.chromium.org/1412243012/diff/540001/components/cronet/andr... components/cronet/android/cronet_bidirectional_stream.cc:186: jint jcapacity, jcapacity->jlimit capacity has a very different connotation for ByteBuffers. https://codereview.chromium.org/1412243012/diff/540001/components/cronet/andr... File components/cronet/android/cronet_bidirectional_stream.h (right): https://codereview.chromium.org/1412243012/diff/540001/components/cronet/andr... components/cronet/android/cronet_bidirectional_stream.h:47: // Starts the request. This function needs more of a comment, esp WRT the return value. https://codereview.chromium.org/1412243012/diff/540001/components/cronet/andr... File components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java (right): https://codereview.chromium.org/1412243012/diff/540001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:40: private static final int STATE_NOT_STARTED = 0; let's change these states to an enum (they're allowed (and preferred for type-safety) internally, just not as external APIs) https://codereview.chromium.org/1412243012/diff/540001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:50: private static final int STATE_CANCELED = 5; On 2016/01/04 22:27:07, mef wrote: > On 2016/01/04 19:56:00, pauljensen wrote: > > no action required side-note: this state is set but never checked for, ditto > for > > STATE_ERROR. > > Good point, not sure where to check them. I think those are useful for debugging > (maybe I should add a toString() method?). > Should I add a checkStateForTesting() or something like that? I don't think we need to take any action, or at least I don't see any action as being very useful. https://codereview.chromium.org/1412243012/diff/540001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:64: /* can we move these four lines down to line 75 so the lock sits beside the guarded https://codereview.chromium.org/1412243012/diff/540001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:89: * WRITING_END_OF_STREAM -> WRITING_DONE -> SUCCESS On 2016/01/04 22:27:07, mef wrote: > On 2016/01/04 19:56:00, pauljensen wrote: > > does mWriteState ever proceed to SUCCESS? > > Currently it stops at WRITING_DONE. I could fix the comment or fix the > mWriteState to be also set to SUCCESS / ERROR / CANCELED. > Helen has suggested to rename mStreamState into mReadState to match mWriteState. > WDYT? As we discussed offline, I think for consistency and for easier readability/understandability, renaming mStreamState to mReadState and making mWriteState progress through STARTED and going to SUCCESS/ERROR/CANCELED would be an improvement. Not a big improvement but an improvement. https://codereview.chromium.org/1412243012/diff/540001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:93: private UrlResponseInfo mResponseInfo; should this be marked volatile? it's written on one thread and read by other threads https://codereview.chromium.org/1412243012/diff/540001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:234: mInitialMethod, headers, !doesMethodAllowWriteData(mInitialMethod)); Can we combine nativeStart() and nativeCreateBidirectionalStream()? https://codereview.chromium.org/1412243012/diff/540001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:326: public boolean isDone() { This function is only called from within "synchronized (mNativeStreamLock) {" or immediately prior to code that enters such a block. I think the code could be more performant if the synchronized statement here was removed, @GuardedBy was added, and callsites immediately before entering the sychronized blocks were moved into the synchronized blocks. This will also make the code more consistent with maybeSucceeded(). https://codereview.chromium.org/1412243012/diff/540001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:500: new ArrayList<Map.Entry<String, String>>(); ()->(headers.length/2) https://codereview.chromium.org/1412243012/diff/540001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:572: mNativeStream = 0; can we swap this line with the line above? I'd rather decrease the lifetime of dangling pointers
Thanks, PTAL. https://codereview.chromium.org/1412243012/diff/540001/components/cronet/andr... File components/cronet/android/cronet_bidirectional_stream.cc (right): https://codereview.chromium.org/1412243012/diff/540001/components/cronet/andr... components/cronet/android/cronet_bidirectional_stream.cc:57: // into separate module. On 2016/01/06 17:18:33, pauljensen wrote: > when do you think de-duping IOBufferWithByteBuffer will get done? In a separate CL as I've just added an initial_limit_ member, so it needs changes in consumer code. https://codereview.chromium.org/1412243012/diff/540001/components/cronet/andr... components/cronet/android/cronet_bidirectional_stream.cc:158: const JavaParamRef<jobject>& jcaller, On 2016/01/06 17:18:33, pauljensen wrote: > how come this definition is "const JavaParamRef<jobject>& jcaller" but the > declaration is "jobject jcaller"? can we make them match? ditto for other > functions. Done. https://codereview.chromium.org/1412243012/diff/540001/components/cronet/andr... components/cronet/android/cronet_bidirectional_stream.cc:161: jint jcapacity) { On 2016/01/06 17:18:33, pauljensen wrote: > jcapacity->jlimit > capacity has a very different connotation for ByteBuffers. > please also do this in the .java file "native" declarations. Done. https://codereview.chromium.org/1412243012/diff/540001/components/cronet/andr... components/cronet/android/cronet_bidirectional_stream.cc:186: jint jcapacity, On 2016/01/06 17:18:33, pauljensen wrote: > jcapacity->jlimit > capacity has a very different connotation for ByteBuffers. Done. https://codereview.chromium.org/1412243012/diff/540001/components/cronet/andr... File components/cronet/android/cronet_bidirectional_stream.h (right): https://codereview.chromium.org/1412243012/diff/540001/components/cronet/andr... components/cronet/android/cronet_bidirectional_stream.h:47: // Starts the request. On 2016/01/06 17:18:33, pauljensen wrote: > This function needs more of a comment, esp WRT the return value. Done. https://codereview.chromium.org/1412243012/diff/540001/components/cronet/andr... File components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java (right): https://codereview.chromium.org/1412243012/diff/540001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:40: private static final int STATE_NOT_STARTED = 0; On 2016/01/06 17:18:33, pauljensen wrote: > let's change these states to an enum (they're allowed (and preferred for > type-safety) internally, just not as external APIs) Done. https://codereview.chromium.org/1412243012/diff/540001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:64: /* On 2016/01/06 17:18:33, pauljensen wrote: > can we move these four lines down to line 75 so the lock sits beside the guarded Done. https://codereview.chromium.org/1412243012/diff/540001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:89: * WRITING_END_OF_STREAM -> WRITING_DONE -> SUCCESS On 2016/01/06 17:18:33, pauljensen wrote: > On 2016/01/04 22:27:07, mef wrote: > > On 2016/01/04 19:56:00, pauljensen wrote: > > > does mWriteState ever proceed to SUCCESS? > > > > Currently it stops at WRITING_DONE. I could fix the comment or fix the > > mWriteState to be also set to SUCCESS / ERROR / CANCELED. > > Helen has suggested to rename mStreamState into mReadState to match > mWriteState. > > WDYT? > > As we discussed offline, I think for consistency and for easier > readability/understandability, renaming mStreamState to mReadState and making > mWriteState progress through STARTED and going to SUCCESS/ERROR/CANCELED would > be an improvement. Not a big improvement but an improvement. Done. I wonder whether we need all 3 - Stream State, Read State and Write State. https://codereview.chromium.org/1412243012/diff/540001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:93: private UrlResponseInfo mResponseInfo; On 2016/01/06 17:18:33, pauljensen wrote: > should this be marked volatile? it's written on one thread and read by other > threads mResponseInfo doesn't change after creation in onResponseHeadersReceived, only its setReceivedBytesCount() gets called, but it uses AtomicLong. https://codereview.chromium.org/1412243012/diff/540001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:132: // Null out mByteBuffer, out of paranoia. Has to be done before On 2016/01/04 22:27:07, mef wrote: > On 2016/01/04 19:56:00, pauljensen wrote: > > nit: "out of paranoia" seems more like to facilitate garbage collection of > large > > buffers. By setting it to null it's explicitly transferring ownership. > > Good point. The comment hasn't changed since times when mByteBuffer was pointing > to internal buffer allocated by UrlRequest. > Will change here and below. Done. https://codereview.chromium.org/1412243012/diff/540001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:234: mInitialMethod, headers, !doesMethodAllowWriteData(mInitialMethod)); On 2016/01/06 17:18:33, pauljensen wrote: > Can we combine nativeStart() and nativeCreateBidirectionalStream()? We could, but then how do we report back faulty method or headers? https://codereview.chromium.org/1412243012/diff/540001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:326: public boolean isDone() { On 2016/01/06 17:18:33, pauljensen wrote: > This function is only called from within "synchronized (mNativeStreamLock) {" or > immediately prior to code that enters such a block. I think the code could be > more performant if the synchronized statement here was removed, @GuardedBy was > added, and callsites immediately before entering the sychronized blocks were > moved into the synchronized blocks. This will also make the code more > consistent with maybeSucceeded(). It is also public API, so it has to be synchronized. https://codereview.chromium.org/1412243012/diff/540001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:412: if (byteBuffer.position() != initialPosition) { On 2016/01/04 22:27:07, mef wrote: > On 2016/01/04 19:56:00, pauljensen wrote: > > no action required side-note: Kinda weird to me that we verify position() > > hasn't changed, even though we're about to overwrite position(). I guess a > > change to position() is a common indicator of aberrant behavior. Seems like > > verifying limit() hasn't changed might not be as good an indicator of aberrant > > behavior but does a better job of verifying things are going to keep working. > > Good point, I will preserve and check limits as well. Done. https://codereview.chromium.org/1412243012/diff/540001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:500: new ArrayList<Map.Entry<String, String>>(); On 2016/01/06 17:18:33, pauljensen wrote: > ()->(headers.length/2) Done. https://codereview.chromium.org/1412243012/diff/540001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:572: mNativeStream = 0; On 2016/01/06 17:18:33, pauljensen wrote: > can we swap this line with the line above? I'd rather decrease the lifetime of > dangling pointers Done.
Thanks, PTAL.
https://codereview.chromium.org/1412243012/diff/540001/components/cronet/andr... File components/cronet/android/cronet_bidirectional_stream.cc (right): https://codereview.chromium.org/1412243012/diff/540001/components/cronet/andr... components/cronet/android/cronet_bidirectional_stream.cc:57: // into separate module. On 2016/01/06 21:28:30, mef wrote: > On 2016/01/06 17:18:33, pauljensen wrote: > > when do you think de-duping IOBufferWithByteBuffer will get done? > > In a separate CL as I've just added an initial_limit_ member, so it needs > changes in consumer code. when do you think such a CL would land? https://codereview.chromium.org/1412243012/diff/540001/components/cronet/andr... File components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java (right): https://codereview.chromium.org/1412243012/diff/540001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:234: mInitialMethod, headers, !doesMethodAllowWriteData(mInitialMethod)); On 2016/01/06 21:28:30, mef wrote: > On 2016/01/06 17:18:33, pauljensen wrote: > > Can we combine nativeStart() and nativeCreateBidirectionalStream()? > > We could, but then how do we report back faulty method or headers? Yuck. JNI is awful sometimes. All we need is a pointer to an int...but that's too much to ask. There are a few ways but they're all awful: http://stackoverflow.com/questions/29043872/android-jni-return-multiple-varia... I guess the way we have it is fine then. https://codereview.chromium.org/1412243012/diff/540001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:326: public boolean isDone() { On 2016/01/06 21:28:30, mef wrote: > On 2016/01/06 17:18:33, pauljensen wrote: > > This function is only called from within "synchronized (mNativeStreamLock) {" > or > > immediately prior to code that enters such a block. I think the code could be > > more performant if the synchronized statement here was removed, @GuardedBy was > > added, and callsites immediately before entering the sychronized blocks were > > moved into the synchronized blocks. This will also make the code more > > consistent with maybeSucceeded(). > > It is also public API, so it has to be synchronized. how about: isDone() { synchronized (mNativeStreamLock) { isDoneLocked(); } } @GuardedBy isDoneLocked() { return mStreamState != STATE_NOT_STARTED && mNativeStream == 0; } and then change all the internal call sites to call isDoneLocked. maybe this is too much micro-optimization.
https://codereview.chromium.org/1412243012/diff/540001/components/cronet/andr... File components/cronet/android/cronet_bidirectional_stream.cc (right): https://codereview.chromium.org/1412243012/diff/540001/components/cronet/andr... components/cronet/android/cronet_bidirectional_stream.cc:57: // into separate module. On 2016/01/07 02:56:34, pauljensen wrote: > On 2016/01/06 21:28:30, mef wrote: > > On 2016/01/06 17:18:33, pauljensen wrote: > > > when do you think de-duping IOBufferWithByteBuffer will get done? > > > > In a separate CL as I've just added an initial_limit_ member, so it needs > > changes in consumer code. > > when do you think such a CL would land? Soon after. I can create it now, I just don't want to touch unrelated files in this CL to avoid snowballing its size. I think few things that we discuss here are applicable to UrlRequest[Adapter], but I'd rather have that refactored separately. https://codereview.chromium.org/1412243012/diff/540001/components/cronet/andr... File components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java (right): https://codereview.chromium.org/1412243012/diff/540001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:234: mInitialMethod, headers, !doesMethodAllowWriteData(mInitialMethod)); On 2016/01/07 02:56:34, pauljensen wrote: > On 2016/01/06 21:28:30, mef wrote: > > On 2016/01/06 17:18:33, pauljensen wrote: > > > Can we combine nativeStart() and nativeCreateBidirectionalStream()? > > > > We could, but then how do we report back faulty method or headers? > > Yuck. JNI is awful sometimes. All we need is a pointer to an int...but that's > too much to ask. There are a few ways but they're all awful: > http://stackoverflow.com/questions/29043872/android-jni-return-multiple-varia... > I guess the way we have it is fine then. Acknowledged. https://codereview.chromium.org/1412243012/diff/540001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:326: public boolean isDone() { On 2016/01/07 02:56:34, pauljensen wrote: > On 2016/01/06 21:28:30, mef wrote: > > On 2016/01/06 17:18:33, pauljensen wrote: > > > This function is only called from within "synchronized (mNativeStreamLock) > {" > > or > > > immediately prior to code that enters such a block. I think the code could > be > > > more performant if the synchronized statement here was removed, @GuardedBy > was > > > added, and callsites immediately before entering the sychronized blocks were > > > moved into the synchronized blocks. This will also make the code more > > > consistent with maybeSucceeded(). > > > > It is also public API, so it has to be synchronized. > > how about: > isDone() { > synchronized (mNativeStreamLock) { > isDoneLocked(); > } > } > @GuardedBy > isDoneLocked() { > return mStreamState != STATE_NOT_STARTED && mNativeStream == 0; > } > and then change all the internal call sites to call isDoneLocked. > maybe this is too much micro-optimization. I think it is micro-optimization when lock is already acquired, but maybe worthy when isDone() was called outside of synchronized section. Anyway, Done.
https://codereview.chromium.org/1412243012/diff/540001/components/cronet/andr... File components/cronet/android/cronet_bidirectional_stream.cc (right): https://codereview.chromium.org/1412243012/diff/540001/components/cronet/andr... components/cronet/android/cronet_bidirectional_stream.cc:57: // into separate module. On 2016/01/07 03:22:15, mef wrote: > On 2016/01/07 02:56:34, pauljensen wrote: > > On 2016/01/06 21:28:30, mef wrote: > > > On 2016/01/06 17:18:33, pauljensen wrote: > > > > when do you think de-duping IOBufferWithByteBuffer will get done? > > > > > > In a separate CL as I've just added an initial_limit_ member, so it needs > > > changes in consumer code. > > > > when do you think such a CL would land? > > Soon after. I can create it now, I just don't want to touch unrelated files in > this CL to avoid snowballing its size. > I think few things that we discuss here are applicable to UrlRequest[Adapter], > but I'd rather have that refactored separately. I agree with not wanting it in this CL...I just wanted to know that it will land soon. https://codereview.chromium.org/1412243012/diff/540001/components/cronet/andr... File components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java (right): https://codereview.chromium.org/1412243012/diff/540001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:234: mInitialMethod, headers, !doesMethodAllowWriteData(mInitialMethod)); On 2016/01/07 03:22:15, mef wrote: > On 2016/01/07 02:56:34, pauljensen wrote: > > On 2016/01/06 21:28:30, mef wrote: > > > On 2016/01/06 17:18:33, pauljensen wrote: > > > > Can we combine nativeStart() and nativeCreateBidirectionalStream()? > > > > > > We could, but then how do we report back faulty method or headers? > > > > Yuck. JNI is awful sometimes. All we need is a pointer to an int...but > that's > > too much to ask. There are a few ways but they're all awful: > > > http://stackoverflow.com/questions/29043872/android-jni-return-multiple-varia... > > I guess the way we have it is fine then. > > Acknowledged. Another idea is to throw the exception from native code: newExcCls = (*env)->FindClass(env, "java/lang/IllegalArgumentException"); if (newExcCls == 0) /* Unable to find the new exception class, give up. */ return; (*env)->ThrowNew(env, newExcCls, "Invalid HTTP method"); Probably not worth implementing this, but it would speed up the general case by having only one JNI call. https://codereview.chromium.org/1412243012/diff/640001/components/cronet/andr... File components/cronet/android/cronet_bidirectional_stream_adapter.h (right): https://codereview.chromium.org/1412243012/diff/640001/components/cronet/andr... components/cronet/android/cronet_bidirectional_stream_adapter.h:119: CronetURLRequestContextAdapter* context_; nit: CronetURLRequestContextAdapter* const context_ https://codereview.chromium.org/1412243012/diff/640001/components/cronet/andr... File components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java (right): https://codereview.chromium.org/1412243012/diff/640001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:127: if (isDoneLocked() || mNativeStream == 0) { how could mNativeStream == 0 here but isDoneLocked() return false? I think the stream would have to be in the NOT_STARTED state, which I think is impossible considering we're issuing a read-completed callback. https://codereview.chromium.org/1412243012/diff/640001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:132: if (maybeSucceeded()) return; our style isn't to put return on the same line; we use curly braces, see line 127, 197, 260 for example https://codereview.chromium.org/1412243012/diff/640001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:138: ByteBuffer buffer = mByteBuffer; I think there is a race here because after line 136 (when the sync block ends) a misbehaving BiDiStream user could call read() which could result in a onReadCompleted() getting called which could race to set mByteBuffer while this line races to read mByteBuffer. If the wrong side loses the race we could call mCallback.onReadCompleted() with the new buffer rather than the old one. I wonder if moving this read up as I describe on the next line might fix this. This kind of a race is a byproduct of reusing mOnReadCompletedCallback. This kind of reuse kinda goes against the Java pattern of new'ing all the time (like how people tend to write execute(new Runable() { blah }). https://codereview.chromium.org/1412243012/diff/640001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:139: mByteBuffer = null; this line should be moved up above all the "return" statements in this function, so that if the BiDiStream is cancelled the buffer is freed. https://codereview.chromium.org/1412243012/diff/640001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:159: mWriteState = State.WRITING_DONE; did we necessarily write all of the buffer's data here? it'd be nice to have some kind of an assertion check here to make sure we wrote all of it (i.e. mByteBuffer.remaining() == 0) https://codereview.chromium.org/1412243012/diff/640001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:160: if (maybeSucceeded()) return; our style isn't to put return on the same line; we use curly braces, see line 127, 197, 260 for example https://codereview.chromium.org/1412243012/diff/640001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:167: mByteBuffer = null; this line should be moved up above all the "return" statements in this function, so that if the BiDiStream is cancelled the buffer is freed. https://codereview.chromium.org/1412243012/diff/640001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:596: } should we insert "mReadState = mWriteState = State.ERROR;"? if we do, should we then dedup this code with that in the next function?
Thanks, PTAL. https://codereview.chromium.org/1412243012/diff/540001/components/cronet/andr... File components/cronet/android/cronet_bidirectional_stream.cc (right): https://codereview.chromium.org/1412243012/diff/540001/components/cronet/andr... components/cronet/android/cronet_bidirectional_stream.cc:57: // into separate module. On 2016/01/11 20:05:45, pauljensen wrote: > On 2016/01/07 03:22:15, mef wrote: > > On 2016/01/07 02:56:34, pauljensen wrote: > > > On 2016/01/06 21:28:30, mef wrote: > > > > On 2016/01/06 17:18:33, pauljensen wrote: > > > > > when do you think de-duping IOBufferWithByteBuffer will get done? > > > > > > > > In a separate CL as I've just added an initial_limit_ member, so it needs > > > > changes in consumer code. > > > > > > when do you think such a CL would land? > > > > Soon after. I can create it now, I just don't want to touch unrelated files in > > this CL to avoid snowballing its size. > > I think few things that we discuss here are applicable to UrlRequest[Adapter], > > but I'd rather have that refactored separately. > > I agree with not wanting it in this CL...I just wanted to know that it will land > soon. Acknowledged. https://codereview.chromium.org/1412243012/diff/540001/components/cronet/andr... File components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java (right): https://codereview.chromium.org/1412243012/diff/540001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:234: mInitialMethod, headers, !doesMethodAllowWriteData(mInitialMethod)); On 2016/01/11 20:05:45, pauljensen wrote: > On 2016/01/07 03:22:15, mef wrote: > > On 2016/01/07 02:56:34, pauljensen wrote: > > > On 2016/01/06 21:28:30, mef wrote: > > > > On 2016/01/06 17:18:33, pauljensen wrote: > > > > > Can we combine nativeStart() and nativeCreateBidirectionalStream()? > > > > > > > > We could, but then how do we report back faulty method or headers? > > > > > > Yuck. JNI is awful sometimes. All we need is a pointer to an int...but > > that's > > > too much to ask. There are a few ways but they're all awful: > > > > > > http://stackoverflow.com/questions/29043872/android-jni-return-multiple-varia... > > > I guess the way we have it is fine then. > > > > Acknowledged. > > Another idea is to throw the exception from native code: > newExcCls = (*env)->FindClass(env, "java/lang/IllegalArgumentException"); > if (newExcCls == 0) /* Unable to find the new exception class, give up. */ > return; > (*env)->ThrowNew(env, newExcCls, "Invalid HTTP method"); > Probably not worth implementing this, but it would speed up the general case by > having only one JNI call. Would it make sense to add a @CalledByNative metod onStartFailure(String invalidMethod, String invalidHeader) that would post the error to an executor, and combine create/start to return 0 in failure case? https://codereview.chromium.org/1412243012/diff/640001/components/cronet/andr... File components/cronet/android/cronet_bidirectional_stream_adapter.h (right): https://codereview.chromium.org/1412243012/diff/640001/components/cronet/andr... components/cronet/android/cronet_bidirectional_stream_adapter.h:119: CronetURLRequestContextAdapter* context_; On 2016/01/11 20:05:45, pauljensen wrote: > nit: CronetURLRequestContextAdapter* const context_ Done. https://codereview.chromium.org/1412243012/diff/640001/components/cronet/andr... File components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java (right): https://codereview.chromium.org/1412243012/diff/640001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:127: if (isDoneLocked() || mNativeStream == 0) { On 2016/01/11 20:05:45, pauljensen wrote: > how could mNativeStream == 0 here but isDoneLocked() return false? I think the > stream would have to be in the NOT_STARTED state, which I think is impossible > considering we're issuing a read-completed callback. Good point. Done. https://codereview.chromium.org/1412243012/diff/640001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:132: if (maybeSucceeded()) return; On 2016/01/11 20:05:45, pauljensen wrote: > our style isn't to put return on the same line; we use curly braces, see line > 127, 197, 260 for example Done. https://codereview.chromium.org/1412243012/diff/640001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:138: ByteBuffer buffer = mByteBuffer; On 2016/01/11 20:05:45, pauljensen wrote: > I think there is a race here because after line 136 (when the sync block ends) a > misbehaving BiDiStream user could call read() which could result in a > onReadCompleted() getting called which could race to set mByteBuffer while this > line races to read mByteBuffer. If the wrong side loses the race we could call > mCallback.onReadCompleted() with the new buffer rather than the old one. I > wonder if moving this read up as I describe on the next line might fix this. > > This kind of a race is a byproduct of reusing mOnReadCompletedCallback. This > kind of reuse kinda goes against the Java pattern of new'ing all the time (like > how people tend to write execute(new Runable() { blah }). Done. https://codereview.chromium.org/1412243012/diff/640001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:139: mByteBuffer = null; On 2016/01/11 20:05:45, pauljensen wrote: > this line should be moved up above all the "return" statements in this function, > so that if the BiDiStream is cancelled the buffer is freed. Done. https://codereview.chromium.org/1412243012/diff/640001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:159: mWriteState = State.WRITING_DONE; On 2016/01/11 20:05:45, pauljensen wrote: > did we necessarily write all of the buffer's data here? it'd be nice to have > some kind of an assertion check here to make sure we wrote all of it (i.e. > mByteBuffer.remaining() == 0) Current implementation always writes the complete buffer (see onWriteCompleted() line 428). https://codereview.chromium.org/1412243012/diff/640001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:160: if (maybeSucceeded()) return; On 2016/01/11 20:05:45, pauljensen wrote: > our style isn't to put return on the same line; we use curly braces, see line > 127, 197, 260 for example Done. https://codereview.chromium.org/1412243012/diff/640001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:167: mByteBuffer = null; On 2016/01/11 20:05:45, pauljensen wrote: > this line should be moved up above all the "return" statements in this function, > so that if the BiDiStream is cancelled the buffer is freed. Done. https://codereview.chromium.org/1412243012/diff/640001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:596: } On 2016/01/11 20:05:45, pauljensen wrote: > should we insert "mReadState = mWriteState = State.ERROR;"? > if we do, should we then dedup this code with that in the next function? Done.
https://codereview.chromium.org/1412243012/diff/540001/components/cronet/andr... File components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java (right): https://codereview.chromium.org/1412243012/diff/540001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:234: mInitialMethod, headers, !doesMethodAllowWriteData(mInitialMethod)); On 2016/01/11 23:22:50, mef wrote: > On 2016/01/11 20:05:45, pauljensen wrote: > > On 2016/01/07 03:22:15, mef wrote: > > > On 2016/01/07 02:56:34, pauljensen wrote: > > > > On 2016/01/06 21:28:30, mef wrote: > > > > > On 2016/01/06 17:18:33, pauljensen wrote: > > > > > > Can we combine nativeStart() and nativeCreateBidirectionalStream()? > > > > > > > > > > We could, but then how do we report back faulty method or headers? > > > > > > > > Yuck. JNI is awful sometimes. All we need is a pointer to an int...but > > > that's > > > > too much to ask. There are a few ways but they're all awful: > > > > > > > > > > http://stackoverflow.com/questions/29043872/android-jni-return-multiple-varia... > > > > I guess the way we have it is fine then. > > > > > > Acknowledged. > > > > Another idea is to throw the exception from native code: > > newExcCls = (*env)->FindClass(env, "java/lang/IllegalArgumentException"); > > if (newExcCls == 0) /* Unable to find the new exception class, give up. */ > > return; > > (*env)->ThrowNew(env, newExcCls, "Invalid HTTP method"); > > Probably not worth implementing this, but it would speed up the general case > by > > having only one JNI call. > > Would it make sense to add a @CalledByNative metod onStartFailure(String > invalidMethod, String invalidHeader) that would post the error to an executor, > and combine create/start to return 0 in failure case? > I don't think there is any need to involve an Executor or more JNI. WDYT about my throw-from-native idea? Frankly I'm fine leaving it the way it is for now. https://codereview.chromium.org/1412243012/diff/640001/components/cronet/andr... File components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java (right): https://codereview.chromium.org/1412243012/diff/640001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:139: mByteBuffer = null; On 2016/01/11 23:22:50, mef wrote: > On 2016/01/11 20:05:45, pauljensen wrote: > > this line should be moved up above all the "return" statements in this > function, > > so that if the BiDiStream is cancelled the buffer is freed. > > Done. Can we make this change to CronetUrlRequest.java too? https://codereview.chromium.org/1412243012/diff/680001/components/cronet/andr... File components/cronet/android/cronet_bidirectional_stream_adapter.cc (right): https://codereview.chromium.org/1412243012/diff/680001/components/cronet/andr... components/cronet/android/cronet_bidirectional_stream_adapter.cc:99: DCHECK(!context_->IsOnNetworkThread()); This could fail if user implements a direct executor and creates a new BiDiStream, I'd say remove it https://codereview.chromium.org/1412243012/diff/680001/components/cronet/andr... components/cronet/android/cronet_bidirectional_stream_adapter.cc:115: DCHECK(!context_->IsOnNetworkThread()); This could fail if user implements a direct executor, I'd say remove it https://codereview.chromium.org/1412243012/diff/680001/components/cronet/andr... components/cronet/android/cronet_bidirectional_stream_adapter.cc:156: ->http_transaction_factory() the auto-formatting here is rather hideous. I'd rather we put context_->... on the next line https://codereview.chromium.org/1412243012/diff/680001/components/cronet/andr... components/cronet/android/cronet_bidirectional_stream_adapter.cc:167: DCHECK(!context_->IsOnNetworkThread()); This could fail if user implements a direct executor, I'd say remove it https://codereview.chromium.org/1412243012/diff/680001/components/cronet/andr... components/cronet/android/cronet_bidirectional_stream_adapter.cc:193: DCHECK(!context_->IsOnNetworkThread()); This could fail if user implements a direct executor, I'd say remove it https://codereview.chromium.org/1412243012/diff/680001/components/cronet/andr... components/cronet/android/cronet_bidirectional_stream_adapter.cc:229: CronetBidirectionalStreamAdapter::GetNegotiatedProtocol( can we get rid of this function and simply pass this as an argument to onResponseHeadersReceived()? this would halve the size of prepareResponseInfoOnNetworkThread() and remove a synchonizing block. This is like what I did here: https://codereview.chromium.org/1100923002/ https://codereview.chromium.org/1412243012/diff/680001/components/cronet/andr... components/cronet/android/cronet_bidirectional_stream_adapter.cc:280: read_buffer_ = nullptr; nit: it'd be nice if we Release()'d the Java ByteBuffer to the onReadComplete() callback, rather than afterwards. This would facilitate freeing the ByteBuffer from within that callback versus the current behavior where we hold it until the callback returns (though I guess the callback should just post to the Executor so it should be a non-issue). https://codereview.chromium.org/1412243012/diff/680001/components/cronet/andr... File components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java (right): https://codereview.chromium.org/1412243012/diff/680001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:502: cancel(); I don't think we want to trigger an onCancelled callback here. I think that would be very unexpected for the user considering we document onCancelled to result only from calls to cancel(). I think attempting to post onFailed() after destroying the native adapter should be fine. I could imagine an Executor that sometimes rejects execute() requests but sometimes does not, e.g. a MessageQueue that gets full (and rejects) then empties itself over time. Let's fix this in CronetUrlRequest too. https://codereview.chromium.org/1412243012/diff/680001/components/cronet/andr... File components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamTest.java (right): https://codereview.chromium.org/1412243012/diff/680001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamTest.java:23: public class BidirectionalStreamTest extends CronetTestBase { can we add a bidirectional test? like send "marko1" and get back "polo1" and then send "marko2" and get back "polo2"? https://codereview.chromium.org/1412243012/diff/680001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamTest.java:194: callback.mResponseInfo.toString()); This isn't going to work anymore, see https://codereview.chromium.org/1492583002/diff/260001/components/cronet/andr... and Charles' description of why here: https://codereview.chromium.org/1492583002/diff/140001/components/cronet/andr... https://codereview.chromium.org/1412243012/diff/680001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamTest.java:240: .addHeader("Content-Type", "zebra") do these headers get verified anywhere? https://codereview.chromium.org/1412243012/diff/680001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamTest.java:249: assertEquals("woot!", callback.mResponseAsString); how come only the last 5 bytes get put in the reponse? https://codereview.chromium.org/1412243012/diff/680001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamTest.java:273: assertEquals("Put This Data!", callback.mResponseAsString); is there any verification here that we used the PUT method? https://codereview.chromium.org/1412243012/diff/680001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamTest.java:398: builder.addHeader(userAgentName, userAgentValue); can we test CronetEngine.Builder.setUserAgent? I think this is the more common way of setting the user-agent https://codereview.chromium.org/1412243012/diff/680001/components/cronet/cron... File components/cronet/cronet_static.gypi (right): https://codereview.chromium.org/1412243012/diff/680001/components/cronet/cron... components/cronet/cronet_static.gypi:76: # If Bidirectional Stream support is enabled, add the following following->following sources. https://codereview.chromium.org/1412243012/diff/680001/components/cronet/tool... File components/cronet/tools/cr_cronet.py (right): https://codereview.chromium.org/1412243012/diff/680001/components/cronet/tool... components/cronet/tools/cr_cronet.py:75: 'enable_bidirectional_stream=true use_goma=true' does this work without Goma? I never use Goma
Thanks, PTAL. https://codereview.chromium.org/1412243012/diff/540001/components/cronet/andr... File components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java (right): https://codereview.chromium.org/1412243012/diff/540001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:234: mInitialMethod, headers, !doesMethodAllowWriteData(mInitialMethod)); On 2016/01/12 16:55:41, pauljensen wrote: > On 2016/01/11 23:22:50, mef wrote: > > On 2016/01/11 20:05:45, pauljensen wrote: > > > On 2016/01/07 03:22:15, mef wrote: > > > > On 2016/01/07 02:56:34, pauljensen wrote: > > > > > On 2016/01/06 21:28:30, mef wrote: > > > > > > On 2016/01/06 17:18:33, pauljensen wrote: > > > > > > > Can we combine nativeStart() and nativeCreateBidirectionalStream()? > > > > > > > > > > > > We could, but then how do we report back faulty method or headers? > > > > > > > > > > Yuck. JNI is awful sometimes. All we need is a pointer to an int...but > > > > that's > > > > > too much to ask. There are a few ways but they're all awful: > > > > > > > > > > > > > > > http://stackoverflow.com/questions/29043872/android-jni-return-multiple-varia... > > > > > I guess the way we have it is fine then. > > > > > > > > Acknowledged. > > > > > > Another idea is to throw the exception from native code: > > > newExcCls = (*env)->FindClass(env, > "java/lang/IllegalArgumentException"); > > > if (newExcCls == 0) /* Unable to find the new exception class, give up. > */ > > > return; > > > (*env)->ThrowNew(env, newExcCls, "Invalid HTTP method"); > > > Probably not worth implementing this, but it would speed up the general case > > by > > > having only one JNI call. > > > > Would it make sense to add a @CalledByNative metod onStartFailure(String > > invalidMethod, String invalidHeader) that would post the error to an executor, > > and combine create/start to return 0 in failure case? > > > > I don't think there is any need to involve an Executor or more JNI. WDYT about > my throw-from-native idea? Frankly I'm fine leaving it the way it is for now. sg, let's keep it this way and refactor both UrlRequest and BidirectionalStream in consistent matter as separate CL. https://codereview.chromium.org/1412243012/diff/680001/components/cronet/andr... File components/cronet/android/cronet_bidirectional_stream_adapter.cc (right): https://codereview.chromium.org/1412243012/diff/680001/components/cronet/andr... components/cronet/android/cronet_bidirectional_stream_adapter.cc:99: DCHECK(!context_->IsOnNetworkThread()); On 2016/01/12 16:55:41, pauljensen wrote: > This could fail if user implements a direct executor and creates a new > BiDiStream, I'd say remove it Done. https://codereview.chromium.org/1412243012/diff/680001/components/cronet/andr... components/cronet/android/cronet_bidirectional_stream_adapter.cc:115: DCHECK(!context_->IsOnNetworkThread()); On 2016/01/12 16:55:41, pauljensen wrote: > This could fail if user implements a direct executor, I'd say remove it Done. https://codereview.chromium.org/1412243012/diff/680001/components/cronet/andr... components/cronet/android/cronet_bidirectional_stream_adapter.cc:156: ->http_transaction_factory() On 2016/01/12 16:55:41, pauljensen wrote: > the auto-formatting here is rather hideous. I'd rather we put context_->... on > the next line Ack. I've tried, but cl format insists. https://codereview.chromium.org/1412243012/diff/680001/components/cronet/andr... components/cronet/android/cronet_bidirectional_stream_adapter.cc:167: DCHECK(!context_->IsOnNetworkThread()); On 2016/01/12 16:55:41, pauljensen wrote: > This could fail if user implements a direct executor, I'd say remove it Done. https://codereview.chromium.org/1412243012/diff/680001/components/cronet/andr... components/cronet/android/cronet_bidirectional_stream_adapter.cc:193: DCHECK(!context_->IsOnNetworkThread()); On 2016/01/12 16:55:41, pauljensen wrote: > This could fail if user implements a direct executor, I'd say remove it Done. https://codereview.chromium.org/1412243012/diff/680001/components/cronet/andr... components/cronet/android/cronet_bidirectional_stream_adapter.cc:229: CronetBidirectionalStreamAdapter::GetNegotiatedProtocol( On 2016/01/12 16:55:41, pauljensen wrote: > can we get rid of this function and simply pass this as an argument to > onResponseHeadersReceived()? > this would halve the size of prepareResponseInfoOnNetworkThread() and remove a > synchonizing block. This is like what I did here: > https://codereview.chromium.org/1100923002/ Done. https://codereview.chromium.org/1412243012/diff/680001/components/cronet/andr... components/cronet/android/cronet_bidirectional_stream_adapter.cc:280: read_buffer_ = nullptr; On 2016/01/12 16:55:41, pauljensen wrote: > nit: it'd be nice if we Release()'d the Java ByteBuffer to the onReadComplete() > callback, rather than afterwards. This would facilitate freeing the ByteBuffer > from within that callback versus the current behavior where we hold it until the > callback returns (though I guess the callback should just post to the Executor > so it should be a non-issue). Acknowledged. As you've said, callback only posts the task, so I don't think it is a problem. Moreover, I think there could be a chance when we are holding the only reference to the read buffer, so releasing it prior to callback could free it. https://codereview.chromium.org/1412243012/diff/680001/components/cronet/andr... File components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java (right): https://codereview.chromium.org/1412243012/diff/680001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:502: cancel(); On 2016/01/12 16:55:41, pauljensen wrote: > I don't think we want to trigger an onCancelled callback here. I think that > would be very unexpected for the user considering we document onCancelled to > result only from calls to cancel(). I think attempting to post onFailed() after > destroying the native adapter should be fine. I could imagine an Executor that > sometimes rejects execute() requests but sometimes does not, e.g. a MessageQueue > that gets full (and rejects) then empties itself over time. > Let's fix this in CronetUrlRequest too. Hrm, I disagree. :) Let's chat about it, but what if executor IS always rejecting? https://codereview.chromium.org/1412243012/diff/680001/components/cronet/andr... File components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamTest.java (right): https://codereview.chromium.org/1412243012/diff/680001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamTest.java:23: public class BidirectionalStreamTest extends CronetTestBase { On 2016/01/12 16:55:42, pauljensen wrote: > can we add a bidirectional test? like send "marko1" and get back "polo1" and > then send "marko2" and get back "polo2"? Done. https://codereview.chromium.org/1412243012/diff/680001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamTest.java:194: callback.mResponseInfo.toString()); On 2016/01/12 16:55:42, pauljensen wrote: > This isn't going to work anymore, see > https://codereview.chromium.org/1492583002/diff/260001/components/cronet/andr... > and Charles' description of why here: > https://codereview.chromium.org/1492583002/diff/140001/components/cronet/andr... Done. https://codereview.chromium.org/1412243012/diff/680001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamTest.java:240: .addHeader("Content-Type", "zebra") On 2016/01/12 16:55:41, pauljensen wrote: > do these headers get verified anywhere? Done. https://codereview.chromium.org/1412243012/diff/680001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamTest.java:249: assertEquals("woot!", callback.mResponseAsString); On 2016/01/12 16:55:42, pauljensen wrote: > how come only the last 5 bytes get put in the reponse? Done. https://codereview.chromium.org/1412243012/diff/680001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamTest.java:273: assertEquals("Put This Data!", callback.mResponseAsString); On 2016/01/12 16:55:41, pauljensen wrote: > is there any verification here that we used the PUT method? Done. https://codereview.chromium.org/1412243012/diff/680001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamTest.java:398: builder.addHeader(userAgentName, userAgentValue); On 2016/01/12 16:55:41, pauljensen wrote: > can we test CronetEngine.Builder.setUserAgent? I think this is the more common > way of setting the user-agent Per offline discussion it is unclear, whether bidirectional stream should support that. https://codereview.chromium.org/1412243012/diff/680001/components/cronet/cron... File components/cronet/cronet_static.gypi (right): https://codereview.chromium.org/1412243012/diff/680001/components/cronet/cron... components/cronet/cronet_static.gypi:76: # If Bidirectional Stream support is enabled, add the following On 2016/01/12 16:55:42, pauljensen wrote: > following->following sources. Done. https://codereview.chromium.org/1412243012/diff/680001/components/cronet/tool... File components/cronet/tools/cr_cronet.py (right): https://codereview.chromium.org/1412243012/diff/680001/components/cronet/tool... components/cronet/tools/cr_cronet.py:75: 'enable_bidirectional_stream=true use_goma=true' On 2016/01/12 16:55:42, pauljensen wrote: > does this work without Goma? I never use Goma You should give it a try, it is great. :) Removed.
https://codereview.chromium.org/1412243012/diff/680001/components/cronet/andr... File components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java (right): https://codereview.chromium.org/1412243012/diff/680001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:502: cancel(); On 2016/01/14 21:07:54, mef wrote: > On 2016/01/12 16:55:41, pauljensen wrote: > > I don't think we want to trigger an onCancelled callback here. I think that > > would be very unexpected for the user considering we document onCancelled to > > result only from calls to cancel(). I think attempting to post onFailed() > after > > destroying the native adapter should be fine. I could imagine an Executor > that > > sometimes rejects execute() requests but sometimes does not, e.g. a > MessageQueue > > that gets full (and rejects) then empties itself over time. > > Let's fix this in CronetUrlRequest too. > > Hrm, I disagree. :) Let's chat about it, but what if executor IS always > rejecting? Discussed. Changed. Done.
https://codereview.chromium.org/1412243012/diff/680001/components/cronet/andr... File components/cronet/android/api/src/org/chromium/net/BidirectionalStream.java (right): https://codereview.chromium.org/1412243012/diff/680001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/BidirectionalStream.java:225: * necessarily equal to its limit. To continue writing, call is there any reason to require all callers to handle the case of partial writes? if our impl is known to always send it all, I feel like that is adding unnecessary complexity to callers. do we even envision changing this requirement? I imagine we could always implement this interface on an impl that may do partial writes, but simply adding a condition to the OnWriteCompleted callback such that is automatically calls write() again if not all data was written. I guess the question is whether embedders are going to benefit from knowing that not all bytes were written and taking action on this. Anyhow probably something to kick down the road to another CL. https://codereview.chromium.org/1412243012/diff/740001/components/cronet/andr... File components/cronet/android/BUILD.gn (right): https://codereview.chromium.org/1412243012/diff/740001/components/cronet/andr... components/cronet/android/BUILD.gn:701: flags = [ "ENABLE_BIDIRECTIONAL_STREAM=$enable_bidirectional_stream" ] Could we instead do this like we do DATA_REDUCTION_PROXY_SUPPORT on line 155? Seems a bit simpler, and having the flag undefined when BiDiStream support is missing seems less error prone (e.g. less likely someone accidentally writes "#ifdef ENABLE_BIDI_STREAM") https://codereview.chromium.org/1412243012/diff/740001/components/cronet/andr... File components/cronet/android/cronet_bidirectional_stream_adapter.cc (right): https://codereview.chromium.org/1412243012/diff/740001/components/cronet/andr... components/cronet/android/cronet_bidirectional_stream_adapter.cc:77: byte_buffer_.Reset(env, jbyte_buffer); How about moving this up to initializer list like all the other members? https://codereview.chromium.org/1412243012/diff/740001/components/cronet/andr... components/cronet/android/cronet_bidirectional_stream_adapter.cc:99: owner_.Reset(env, jbidi_stream); ditto https://codereview.chromium.org/1412243012/diff/740001/components/cronet/andr... components/cronet/android/cronet_bidirectional_stream_adapter.cc:253: cronet::Java_CronetBidirectionalStream_onResponseHeadersReceived( should we pass up the received byte count? https://codereview.chromium.org/1412243012/diff/740001/components/cronet/andr... components/cronet/android/cronet_bidirectional_stream_adapter.cc:267: received_bytes_count); nit: how about "received_bytes_count"->"bidi_stream_->GetTotalReceivedBytes()" and get rid of the variable? https://codereview.chromium.org/1412243012/diff/740001/components/cronet/andr... components/cronet/android/cronet_bidirectional_stream_adapter.cc:300: received_bytes_count); ditto https://codereview.chromium.org/1412243012/diff/740001/components/cronet/andr... File components/cronet/android/cronet_bidirectional_stream_adapter.h (right): https://codereview.chromium.org/1412243012/diff/740001/components/cronet/andr... components/cronet/android/cronet_bidirectional_stream_adapter.h:27: using base::android::JavaParamRef; This is not allowed in style guide: "Don't put an alias in your public API just to save typing in the implementation; do so only if you intend it to be used by your clients." https://google.github.io/styleguide/cppguide.html#Aliases https://codereview.chromium.org/1412243012/diff/740001/components/cronet/andr... components/cronet/android/cronet_bidirectional_stream_adapter.h:33: bool CronetBidirectionalStreamAdapterRegisterJni(JNIEnv* env); nit: what about moving to a static member CronetBidirectionalStreamAdapter::RegisterJni so it's clearer what this is associated with https://codereview.chromium.org/1412243012/diff/740001/components/cronet/andr... components/cronet/android/cronet_bidirectional_stream_adapter.h:36: // Created and configured from a Java thread. Start, ReadData, and Destroy are This doesn't seem true. ReadData() is called on some client thread, ReadDataOnNetworkThread() is posted to network thread. https://codereview.chromium.org/1412243012/diff/740001/components/cronet/andr... components/cronet/android/cronet_bidirectional_stream_adapter.h:40: // on any thread except Get* methods, which can only be called on the network nit: I only see one Get* method; could just say "GetHeadersArray" instead of "Get* methods" https://codereview.chromium.org/1412243012/diff/740001/components/cronet/andr... components/cronet/android/cronet_bidirectional_stream_adapter.h:56: // not valid. isn't it position+1? https://codereview.chromium.org/1412243012/diff/740001/components/cronet/andr... File components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java (right): https://codereview.chromium.org/1412243012/diff/740001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:24: * BidirectionalStream implementation using Chromium network stack. BidirectionalStream ->{@link BidirectionalStream} https://codereview.chromium.org/1412243012/diff/740001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:25: * All @CallByNative methods are called on native network thread CallByNative->CalledByNative https://codereview.chromium.org/1412243012/diff/740001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:25: * All @CallByNative methods are called on native network thread native->the native https://codereview.chromium.org/1412243012/diff/740001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:26: * and post tasks with callback calls onto Executor. Upon return from callback native callback->callback, https://codereview.chromium.org/1412243012/diff/740001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:27: * stream is called on executor thread and posts native tasks to native network thread. native->the native https://codereview.chromium.org/1412243012/diff/740001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:27: * stream is called on executor thread and posts native tasks to native network thread. executor->Executor https://codereview.chromium.org/1412243012/diff/740001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:33: * The write state is separated out as it changes independently of the stream state. nit: might want to update reference to "stream state" https://codereview.chromium.org/1412243012/diff/740001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:34: * There is one initial state - State.NOT_STARTED. There is one final state - State.SUCCESS, Might want to mention that CANCELED and ERROR states are final, i.e. they cannot be left. https://codereview.chromium.org/1412243012/diff/740001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:42: /* Stream started, request headers are sent. */ sent->being sent https://codereview.chromium.org/1412243012/diff/740001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:179: CronetBidirectionalStream(CronetUrlRequestContext requestContext, long urlRequestContextAdapter, remove unused urlRequestContextAdapater argument https://codereview.chromium.org/1412243012/diff/740001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:192: private boolean maybeSucceeded() { nit: add "Locked" suffix to maybeSucceeded https://codereview.chromium.org/1412243012/diff/740001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:258: if (!buffer.hasRemaining()) { replace this with Preconditions.checkHasRemaining(buffer); https://codereview.chromium.org/1412243012/diff/740001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:260: } add Preconditions.checkDirect(buffer); https://codereview.chromium.org/1412243012/diff/740001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:284: } add Preconditions.checkDirect(buffer); https://codereview.chromium.org/1412243012/diff/740001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:405: if (bytesRead < 0 || initialPosition + bytesRead > byteBuffer.limit()) { do one of these for consistency: 1. initialPosition->byteBuffer.position() 2. byteBuffer.limit()->initialLimit https://codereview.chromium.org/1412243012/diff/740001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:414: mOnReadCompletedTask.mEndOfStream = (bytesRead == 0); no action required nit: it'd be a performance boost if client's didn't have to call read() just to get back an end-of-stream (EOS) signal. I imagine the last data packet sent has some EOS marker. If onReadCompleted was called with a boolean conveying this EOS marker, we could have mOnReadCompletedTask queue the onSucceeded() callback (if writing is also done, which is likely). I imagine this saving some thread hops, but it may be ugly to implement so I assume we shouldn't even think about it for this change. https://codereview.chromium.org/1412243012/diff/740001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:456: postTaskToExecutor(task); nit: any particular reason we do this: Runnable task = blah...; postTaskToExecutor(task); versus: postTaskToExecutor(blah...); It seems like more code and hence takes me longer to read. https://codereview.chromium.org/1412243012/diff/740001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:504: mReadState = mWriteState = State.ERROR; seems like this is unlocked access to mReadState and mWriteState? https://codereview.chromium.org/1412243012/diff/740001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:548: String httpStatusText = ""; there's no status line for BiDiStreams? I would have expected there was if there is a response code, but maybe HTTP2 got rid of this formality https://codereview.chromium.org/1412243012/diff/740001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:570: return RequestPriority.MEDIUM; shouldn't this throw IllegalArgumentException? https://codereview.chromium.org/1412243012/diff/740001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:634: private native long nativeCreateBidirectionalStream(long urlRequestContextAdapter); static? https://codereview.chromium.org/1412243012/diff/740001/components/cronet/andr... File components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamTest.java (right): https://codereview.chromium.org/1412243012/diff/740001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamTest.java:30: private static final String KEY_USED = "quic_test.example.com.key"; Please move these to a shared location, perhaps QuicTestServer https://codereview.chromium.org/1412243012/diff/740001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamTest.java:35: private static final String TEST_URL = "https://127.0.0.1:8443"; I wonder if this should be constructed via some Http2TestServer function like QuicTestServer.getServerUrl()? https://codereview.chromium.org/1412243012/diff/740001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamTest.java:44: builder.setMockCertVerifierForTesting(MockCertVerifier.createMockCertVerifier(CERTS_USED)); I feel like this should be moved to a convenience function in whatever class CERT_USED is moved to https://codereview.chromium.org/1412243012/diff/740001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamTest.java:355: mTestFramework.stopNetLog(); what's the netLog for? https://codereview.chromium.org/1412243012/diff/740001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamTest.java:457: mTestFramework.stopNetLog(); ditto https://codereview.chromium.org/1412243012/diff/740001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamTest.java:471: // Since themethod is "GET", the expected response body is also "GET". themethod->the method https://codereview.chromium.org/1412243012/diff/740001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamTest.java:804: } it doesn't look like we call shutdown() after cancel() here like the test name implies? https://codereview.chromium.org/1412243012/diff/740001/components/cronet/andr... File components/cronet/android/test/javatests/src/org/chromium/net/CronetTestBase.java (right): https://codereview.chromium.org/1412243012/diff/740001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/CronetTestBase.java:137: if (!getClass().isAnnotationPresent(OnlyRunNativeCronet.class) Charles wanted to avoid this as it makes it too easy to make changes just for the native impl. I somewhat agree with him, and accept that it will require adding lots of annotations, one for each test in the case of this CL. https://codereview.chromium.org/1412243012/diff/740001/components/cronet/andr... File components/cronet/android/test/src/org/chromium/net/Http2TestHandler.java (right): https://codereview.chromium.org/1412243012/diff/740001/components/cronet/andr... components/cronet/android/test/src/org/chromium/net/Http2TestHandler.java:1: // Copyright 2015 The Chromium Authors. All rights reserved. I see you've added 400+ lines since the patch-set I last reviewed :) I have zero experience with Netty, would Andrei be a better reviewer as he may have some familiarity? From my review it sounds like this is doing reasonable things and since it's only used as a test apparatus I'm not sure it can do much harm. https://codereview.chromium.org/1412243012/diff/740001/components/cronet/andr... components/cronet/android/test/src/org/chromium/net/Http2TestHandler.java:45: frameLogger(sLogger); what does logging to sLogger get us? is this unused? https://codereview.chromium.org/1412243012/diff/740001/components/cronet/andr... components/cronet/android/test/src/org/chromium/net/Http2TestHandler.java:78: if (!header.getKey().toString().startsWith(":")) { is this attempting to ignore HTTP2-specific headers? https://codereview.chromium.org/1412243012/diff/740001/components/cronet/andr... File components/cronet/android/test/src/org/chromium/net/Http2TestServer.java (right): https://codereview.chromium.org/1412243012/diff/740001/components/cronet/andr... components/cronet/android/test/src/org/chromium/net/Http2TestServer.java:49: (new Thread( I don't think the extra parenthesis around "new Thread(blah)" are needed https://codereview.chromium.org/1412243012/diff/740001/components/cronet/andr... components/cronet/android/test/src/org/chromium/net/Http2TestServer.java:82: return getBaseUrl() + "echomethod"; these strings (e.g. "echomethod") seem duplicated between here and the server; can they be moved into a shared location? https://codereview.chromium.org/1412243012/diff/740001/components/cronet/andr... components/cronet/android/test/src/org/chromium/net/Http2TestServer.java:94: Log.i(TAG, "Http2 server started."); any reason onServerStarted is in a seperate function and not inlined? The Log line in here looks duplicated with the log line before the call to onServerStarted() https://codereview.chromium.org/1412243012/diff/740001/components/cronet/andr... components/cronet/android/test/src/org/chromium/net/Http2TestServer.java:99: File mCertFile; private final? https://codereview.chromium.org/1412243012/diff/740001/components/cronet/andr... components/cronet/android/test/src/org/chromium/net/Http2TestServer.java:100: File mKeyFile; ditto
mef@chromium.org changed reviewers: + kapishnikov@chromium.org
Paul, thanks, PTAL. Andrei, could you take a look @ Http2Test Server & Handler? https://codereview.chromium.org/1412243012/diff/680001/components/cronet/andr... File components/cronet/android/api/src/org/chromium/net/BidirectionalStream.java (right): https://codereview.chromium.org/1412243012/diff/680001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/BidirectionalStream.java:225: * necessarily equal to its limit. To continue writing, call On 2016/01/19 16:03:39, pauljensen wrote: > is there any reason to require all callers to handle the case of partial writes? > if our impl is known to always send it all, I feel like that is adding > unnecessary complexity to callers. do we even envision changing this > requirement? I imagine we could always implement this interface on an impl that > may do partial writes, but simply adding a condition to the OnWriteCompleted > callback such that is automatically calls write() again if not all data was > written. I guess the question is whether embedders are going to benefit from > knowing that not all bytes were written and taking action on this. Anyhow > probably something to kick down the road to another CL. Acknowledged. I agree, but I would wait until QUIC implementation and gRPC integration before removing this. https://codereview.chromium.org/1412243012/diff/740001/components/cronet/andr... File components/cronet/android/BUILD.gn (right): https://codereview.chromium.org/1412243012/diff/740001/components/cronet/andr... components/cronet/android/BUILD.gn:701: flags = [ "ENABLE_BIDIRECTIONAL_STREAM=$enable_bidirectional_stream" ] On 2016/01/19 16:03:39, pauljensen wrote: > Could we instead do this like we do DATA_REDUCTION_PROXY_SUPPORT on line 155? > Seems a bit simpler, and having the flag undefined when BiDiStream support is > missing seems less error prone (e.g. less likely someone accidentally writes > "#ifdef ENABLE_BIDI_STREAM") Build flags are the new hotness": https://groups.google.com/a/chromium.org/forum/#!searchin/chromium-dev/build_... The benefit is compile-time check that flag is defined one way or another. https://codereview.chromium.org/1412243012/diff/740001/components/cronet/andr... File components/cronet/android/cronet_bidirectional_stream_adapter.cc (right): https://codereview.chromium.org/1412243012/diff/740001/components/cronet/andr... components/cronet/android/cronet_bidirectional_stream_adapter.cc:77: byte_buffer_.Reset(env, jbyte_buffer); On 2016/01/19 16:03:39, pauljensen wrote: > How about moving this up to initializer list like all the other members? Done. https://codereview.chromium.org/1412243012/diff/740001/components/cronet/andr... components/cronet/android/cronet_bidirectional_stream_adapter.cc:99: owner_.Reset(env, jbidi_stream); On 2016/01/19 16:03:39, pauljensen wrote: > ditto Done. https://codereview.chromium.org/1412243012/diff/740001/components/cronet/andr... components/cronet/android/cronet_bidirectional_stream_adapter.cc:253: cronet::Java_CronetBidirectionalStream_onResponseHeadersReceived( On 2016/01/19 16:03:39, pauljensen wrote: > should we pass up the received byte count? Done. https://codereview.chromium.org/1412243012/diff/740001/components/cronet/andr... components/cronet/android/cronet_bidirectional_stream_adapter.cc:267: received_bytes_count); On 2016/01/19 16:03:39, pauljensen wrote: > nit: how about "received_bytes_count"->"bidi_stream_->GetTotalReceivedBytes()" > and get rid of the variable? Done. https://codereview.chromium.org/1412243012/diff/740001/components/cronet/andr... components/cronet/android/cronet_bidirectional_stream_adapter.cc:300: received_bytes_count); On 2016/01/19 16:03:39, pauljensen wrote: > ditto Done. https://codereview.chromium.org/1412243012/diff/740001/components/cronet/andr... File components/cronet/android/cronet_bidirectional_stream_adapter.h (right): https://codereview.chromium.org/1412243012/diff/740001/components/cronet/andr... components/cronet/android/cronet_bidirectional_stream_adapter.h:27: using base::android::JavaParamRef; On 2016/01/19 16:03:39, pauljensen wrote: > This is not allowed in style guide: "Don't put an alias in your public API just > to save typing in the implementation; do so only if you intend it to be used by > your clients." > https://google.github.io/styleguide/cppguide.html#Aliases Argh! I could argue that this class is NOT public API, but I won't. Done. https://codereview.chromium.org/1412243012/diff/740001/components/cronet/andr... components/cronet/android/cronet_bidirectional_stream_adapter.h:33: bool CronetBidirectionalStreamAdapterRegisterJni(JNIEnv* env); On 2016/01/19 16:03:39, pauljensen wrote: > nit: what about moving to a static member > CronetBidirectionalStreamAdapter::RegisterJni so it's clearer what this is > associated with Done. https://codereview.chromium.org/1412243012/diff/740001/components/cronet/andr... components/cronet/android/cronet_bidirectional_stream_adapter.h:36: // Created and configured from a Java thread. Start, ReadData, and Destroy are On 2016/01/19 16:03:39, pauljensen wrote: > This doesn't seem true. ReadData() is called on some client thread, > ReadDataOnNetworkThread() is posted to network thread. Done. https://codereview.chromium.org/1412243012/diff/740001/components/cronet/andr... components/cronet/android/cronet_bidirectional_stream_adapter.h:40: // on any thread except Get* methods, which can only be called on the network On 2016/01/19 16:03:40, pauljensen wrote: > nit: I only see one Get* method; could just say "GetHeadersArray" instead of > "Get* methods" GetHeadersArray is now just a private helper method. https://codereview.chromium.org/1412243012/diff/740001/components/cronet/andr... components/cronet/android/cronet_bidirectional_stream_adapter.h:56: // not valid. On 2016/01/19 16:03:39, pauljensen wrote: > isn't it position+1? position of header value == position of header name + 1. https://codereview.chromium.org/1412243012/diff/740001/components/cronet/andr... File components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java (right): https://codereview.chromium.org/1412243012/diff/740001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:24: * BidirectionalStream implementation using Chromium network stack. On 2016/01/19 16:03:40, pauljensen wrote: > BidirectionalStream ->{@link BidirectionalStream} Done. https://codereview.chromium.org/1412243012/diff/740001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:25: * All @CallByNative methods are called on native network thread On 2016/01/19 16:03:40, pauljensen wrote: > native->the native Done. https://codereview.chromium.org/1412243012/diff/740001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:25: * All @CallByNative methods are called on native network thread On 2016/01/19 16:03:40, pauljensen wrote: > CallByNative->CalledByNative Done. https://codereview.chromium.org/1412243012/diff/740001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:26: * and post tasks with callback calls onto Executor. Upon return from callback native On 2016/01/19 16:03:40, pauljensen wrote: > callback->callback, Hrm. I think done, but I'm not sure. https://codereview.chromium.org/1412243012/diff/740001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:27: * stream is called on executor thread and posts native tasks to native network thread. On 2016/01/19 16:03:40, pauljensen wrote: > native->the native Done. https://codereview.chromium.org/1412243012/diff/740001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:27: * stream is called on executor thread and posts native tasks to native network thread. On 2016/01/19 16:03:40, pauljensen wrote: > executor->Executor Done. https://codereview.chromium.org/1412243012/diff/740001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:33: * The write state is separated out as it changes independently of the stream state. On 2016/01/19 16:03:40, pauljensen wrote: > nit: might want to update reference to "stream state" Done. https://codereview.chromium.org/1412243012/diff/740001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:34: * There is one initial state - State.NOT_STARTED. There is one final state - State.SUCCESS, On 2016/01/19 16:03:40, pauljensen wrote: > Might want to mention that CANCELED and ERROR states are final, i.e. they cannot > be left. Done. https://codereview.chromium.org/1412243012/diff/740001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:42: /* Stream started, request headers are sent. */ On 2016/01/19 16:03:40, pauljensen wrote: > sent->being sent Done. https://codereview.chromium.org/1412243012/diff/740001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:179: CronetBidirectionalStream(CronetUrlRequestContext requestContext, long urlRequestContextAdapter, On 2016/01/19 16:03:40, pauljensen wrote: > remove unused urlRequestContextAdapater argument Done. https://codereview.chromium.org/1412243012/diff/740001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:192: private boolean maybeSucceeded() { On 2016/01/19 16:03:40, pauljensen wrote: > nit: add "Locked" suffix to maybeSucceeded Done. https://codereview.chromium.org/1412243012/diff/740001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:258: if (!buffer.hasRemaining()) { On 2016/01/19 16:03:40, pauljensen wrote: > replace this with Preconditions.checkHasRemaining(buffer); Done. https://codereview.chromium.org/1412243012/diff/740001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:260: } On 2016/01/19 16:03:40, pauljensen wrote: > add Preconditions.checkDirect(buffer); Done. https://codereview.chromium.org/1412243012/diff/740001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:284: } On 2016/01/19 16:03:40, pauljensen wrote: > add Preconditions.checkDirect(buffer); Done. https://codereview.chromium.org/1412243012/diff/740001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:405: if (bytesRead < 0 || initialPosition + bytesRead > byteBuffer.limit()) { On 2016/01/19 16:03:40, pauljensen wrote: > do one of these for consistency: > 1. initialPosition->byteBuffer.position() > 2. byteBuffer.limit()->initialLimit Done. https://codereview.chromium.org/1412243012/diff/740001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:414: mOnReadCompletedTask.mEndOfStream = (bytesRead == 0); On 2016/01/19 16:03:40, pauljensen wrote: > no action required nit: it'd be a performance boost if client's didn't have to > call read() just to get back an end-of-stream (EOS) signal. I imagine the last > data packet sent has some EOS marker. If onReadCompleted was called with a > boolean conveying this EOS marker, we could have mOnReadCompletedTask queue the > onSucceeded() callback (if writing is also done, which is likely). I imagine > this saving some thread hops, but it may be ugly to implement so I assume we > shouldn't even think about it for this change. Acknowledged. This is also current contract with underlying net:: implementation. https://codereview.chromium.org/1412243012/diff/740001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:456: postTaskToExecutor(task); On 2016/01/19 16:03:40, pauljensen wrote: > nit: any particular reason we do this: > Runnable task = blah...; > postTaskToExecutor(task); > versus: > postTaskToExecutor(blah...); > It seems like more code and hence takes me longer to read. Historical reasons. :) Done. https://codereview.chromium.org/1412243012/diff/740001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:504: mReadState = mWriteState = State.ERROR; On 2016/01/19 16:03:40, pauljensen wrote: > seems like this is unlocked access to mReadState and mWriteState? Done. https://codereview.chromium.org/1412243012/diff/740001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:548: String httpStatusText = ""; On 2016/01/19 16:03:40, pauljensen wrote: > there's no status line for BiDiStreams? I would have expected there was if > there is a response code, but maybe HTTP2 got rid of this formality Correct. Http2 doesn't have special header for http status text, only status code. https://codereview.chromium.org/1412243012/diff/740001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:570: return RequestPriority.MEDIUM; On 2016/01/19 16:03:40, pauljensen wrote: > shouldn't this throw IllegalArgumentException? Done. https://codereview.chromium.org/1412243012/diff/740001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:634: private native long nativeCreateBidirectionalStream(long urlRequestContextAdapter); On 2016/01/19 16:03:40, pauljensen wrote: > static? Why? It gets |this| and stores as java owner for callbacks. https://codereview.chromium.org/1412243012/diff/740001/components/cronet/andr... File components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamTest.java (right): https://codereview.chromium.org/1412243012/diff/740001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamTest.java:30: private static final String KEY_USED = "quic_test.example.com.key"; On 2016/01/19 16:03:40, pauljensen wrote: > Please move these to a shared location, perhaps QuicTestServer Done. https://codereview.chromium.org/1412243012/diff/740001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamTest.java:35: private static final String TEST_URL = "https://127.0.0.1:8443"; On 2016/01/19 16:03:40, pauljensen wrote: > I wonder if this should be constructed via some Http2TestServer function like > QuicTestServer.getServerUrl()? Done. https://codereview.chromium.org/1412243012/diff/740001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamTest.java:44: builder.setMockCertVerifierForTesting(MockCertVerifier.createMockCertVerifier(CERTS_USED)); On 2016/01/19 16:03:40, pauljensen wrote: > I feel like this should be moved to a convenience function in whatever class > CERT_USED is moved to Done. https://codereview.chromium.org/1412243012/diff/740001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamTest.java:355: mTestFramework.stopNetLog(); On 2016/01/19 16:03:40, pauljensen wrote: > what's the netLog for? To see the trailers? Gone. https://codereview.chromium.org/1412243012/diff/740001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamTest.java:457: mTestFramework.stopNetLog(); On 2016/01/19 16:03:40, pauljensen wrote: > ditto Done. https://codereview.chromium.org/1412243012/diff/740001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamTest.java:471: // Since themethod is "GET", the expected response body is also "GET". On 2016/01/19 16:03:41, pauljensen wrote: > themethod->the method Done. https://codereview.chromium.org/1412243012/diff/740001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamTest.java:804: } On 2016/01/19 16:03:40, pauljensen wrote: > it doesn't look like we call shutdown() after cancel() here like the test name > implies? We do in tearDown(), but I've added onCanceled() override to ShutdownTestBidirectionalStreamCallback and call shutdown from there. https://codereview.chromium.org/1412243012/diff/740001/components/cronet/andr... File components/cronet/android/test/javatests/src/org/chromium/net/CronetTestBase.java (right): https://codereview.chromium.org/1412243012/diff/740001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/CronetTestBase.java:137: if (!getClass().isAnnotationPresent(OnlyRunNativeCronet.class) On 2016/01/19 16:03:41, pauljensen wrote: > Charles wanted to avoid this as it makes it too easy to make changes just for > the native impl. I somewhat agree with him, and accept that it will require > adding lots of annotations, one for each test in the case of this CL. Done. We'll also need another annotation 'RunQuicAndHttp2' at some point. https://codereview.chromium.org/1412243012/diff/740001/components/cronet/andr... File components/cronet/android/test/src/org/chromium/net/Http2TestHandler.java (right): https://codereview.chromium.org/1412243012/diff/740001/components/cronet/andr... components/cronet/android/test/src/org/chromium/net/Http2TestHandler.java:1: // Copyright 2015 The Chromium Authors. All rights reserved. On 2016/01/19 16:03:41, pauljensen wrote: > I see you've added 400+ lines since the patch-set I last reviewed :) > I have zero experience with Netty, would Andrei be a better reviewer as he may > have some familiarity? > From my review it sounds like this is doing reasonable things and since it's > only used as a test apparatus I'm not sure it can do much harm. Acknowledged. I've added these into the CL to make it more cohesive. They were previously in the separate CL to avoid dependency on adding netty to third_party, but at this point it looks like netty addition should be able to land first anyway. Adding Andrei to review Http2Test*.java code. https://codereview.chromium.org/1412243012/diff/740001/components/cronet/andr... components/cronet/android/test/src/org/chromium/net/Http2TestHandler.java:45: frameLogger(sLogger); On 2016/01/19 16:03:41, pauljensen wrote: > what does logging to sLogger get us? is this unused? It is conveniently logging received frames into system log. Very pretty and convenient. https://codereview.chromium.org/1412243012/diff/740001/components/cronet/andr... components/cronet/android/test/src/org/chromium/net/Http2TestHandler.java:78: if (!header.getKey().toString().startsWith(":")) { On 2016/01/19 16:03:41, pauljensen wrote: > is this attempting to ignore HTTP2-specific headers? Yes, to avoid constructing header name like 'echo-:method'. https://codereview.chromium.org/1412243012/diff/740001/components/cronet/andr... File components/cronet/android/test/src/org/chromium/net/Http2TestServer.java (right): https://codereview.chromium.org/1412243012/diff/740001/components/cronet/andr... components/cronet/android/test/src/org/chromium/net/Http2TestServer.java:49: (new Thread( On 2016/01/19 16:03:41, pauljensen wrote: > I don't think the extra parenthesis around "new Thread(blah)" are needed Done. https://codereview.chromium.org/1412243012/diff/740001/components/cronet/andr... components/cronet/android/test/src/org/chromium/net/Http2TestServer.java:82: return getBaseUrl() + "echomethod"; On 2016/01/19 16:03:41, pauljensen wrote: > these strings (e.g. "echomethod") seem duplicated between here and the server; > can they be moved into a shared location? They sure can. I think it would make sense to unify native test server, quic test server and http2 test server urls. Maybe in a separate CL though. WDYT? https://codereview.chromium.org/1412243012/diff/740001/components/cronet/andr... components/cronet/android/test/src/org/chromium/net/Http2TestServer.java:94: Log.i(TAG, "Http2 server started."); On 2016/01/19 16:03:41, pauljensen wrote: > any reason onServerStarted is in a seperate function and not inlined? The Log > line in here looks duplicated with the log line before the call to > onServerStarted() Done. https://codereview.chromium.org/1412243012/diff/740001/components/cronet/andr... components/cronet/android/test/src/org/chromium/net/Http2TestServer.java:99: File mCertFile; On 2016/01/19 16:03:41, pauljensen wrote: > private final? Done. https://codereview.chromium.org/1412243012/diff/740001/components/cronet/andr... components/cronet/android/test/src/org/chromium/net/Http2TestServer.java:100: File mKeyFile; On 2016/01/19 16:03:41, pauljensen wrote: > ditto Done.
A few comments first. Still need time to look closer at the code. There's lots of code! :) The server support looks great. Thanks for all the work went into it. https://codereview.chromium.org/1412243012/diff/780001/components/cronet/andr... File components/cronet/android/cronet_bidirectional_stream_adapter.cc (right): https://codereview.chromium.org/1412243012/diff/780001/components/cronet/andr... components/cronet/android/cronet_bidirectional_stream_adapter.cc:28: #include "net/url_request/redirect_info.h" not used. https://codereview.chromium.org/1412243012/diff/780001/components/cronet/andr... components/cronet/android/cronet_bidirectional_stream_adapter.cc:29: #include "net/url_request/url_request_context.h" not used? https://codereview.chromium.org/1412243012/diff/780001/components/cronet/andr... components/cronet/android/cronet_bidirectional_stream_adapter.cc:60: // to the memory backed by the ByteBuffer, and position is the location to nit: s/position/|position|. Maybe also mention |limit|. https://codereview.chromium.org/1412243012/diff/780001/components/cronet/andr... components/cronet/android/cronet_bidirectional_stream_adapter.cc:78: jobject byte_buffer() const { return byte_buffer_.obj(); } s/jobject/const JavaParamRef<jobject>& ? https://codereview.chromium.org/1412243012/diff/780001/components/cronet/andr... components/cronet/android/cronet_bidirectional_stream_adapter.cc:131: nit: maybe remove this blank line. https://codereview.chromium.org/1412243012/diff/780001/components/cronet/andr... components/cronet/android/cronet_bidirectional_stream_adapter.cc:149: DCHECK_LT(jposition, jlimit); Should we add a check to make sure we are not on the network thread? (and for WriteData) https://codereview.chromium.org/1412243012/diff/780001/components/cronet/andr... components/cronet/android/cronet_bidirectional_stream_adapter.cc:208: // net::BidirectionalStream::Delegate overrides (called on network thread). nit: I haven't seen this annotation in cc file. Should this only be in the header file? https://codereview.chromium.org/1412243012/diff/780001/components/cronet/andr... components/cronet/android/cronet_bidirectional_stream_adapter.cc:211: VLOG(1) << "OnHeadersSent"; Debugging statements here and below. We might want to remove them since the CL is getting really close to landing, or change to DVLOG. https://codereview.chromium.org/1412243012/diff/780001/components/cronet/andr... components/cronet/android/cronet_bidirectional_stream_adapter.cc:288: DCHECK(context_->IsOnNetworkThread()); Maybe also DCHECK(!bidi_stream_) https://codereview.chromium.org/1412243012/diff/780001/components/cronet/andr... File components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java (right): https://codereview.chromium.org/1412243012/diff/780001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:34: * There is one initial state - State.NOT_STARTED. There is one normal final state - nit: single dash reads very much like a minus sign to me. Maybe consider using double dash (--) or comma. https://codereview.chromium.org/1412243012/diff/780001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:45: WAITING_ON_READ, Should this say WAITING_FOR_READ instead of ON? Since we are waiting for read() to be called. https://codereview.chromium.org/1412243012/diff/780001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:54: /* Reading and writing is done, and the stream is closed successfully. */ nit: s/is/are. Reading and writing are done https://codereview.chromium.org/1412243012/diff/780001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:57: WAITING_ON_WRITE, Should this say WAITING_FOR_WRITE instead of ON? Since we are waiting for write() to be called. https://codereview.chromium.org/1412243012/diff/780001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:75: * Synchronize access to mNativeStream, mReadState and mWriteState. nit: s/Synchronize/Synchronizes https://codereview.chromium.org/1412243012/diff/780001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:87: * NOT_STARTED -> STARTED --> WAITING_ON_READ -> READING_DONE -> SUCCESS Great flow diagram! This makes things much easier to understand. Have you checked out the generated JavaDoc? Not sure if JavaDoc generator will mess this up. https://codereview.chromium.org/1412243012/diff/780001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:192: private boolean maybeSucceededLocked() { - Suggest making this method return void. (Early return when not done). - I am trying to think of a better name for this method. The "Locked" suffix makes it hard to read. Maybe just simply "maybeSucceed" ? - Maybe also move this private method to where other private methods are. https://codereview.chromium.org/1412243012/diff/780001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:204: // Destroy native stream first, so request context could be shut nit: maybe spell out the "request context" as UrlRequestContext? a bit clearer that way. https://codereview.chromium.org/1412243012/diff/780001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:245: // If there's an exception, cleanup and then throw the nit: s/cleanup/clean up. (Cleanup is the noun) https://codereview.chromium.org/1412243012/diff/780001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:329: private boolean isDoneLocked() { I believe grabbing the same lock is essentially no-op and allowed in Java. Maybe get rid of this method, and just use isDone(). https://codereview.chromium.org/1412243012/diff/780001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:561: private void destroyNativeStreamLocked(boolean sendOnCanceled) { Having the "Locked" suffix is confusing. Suggest getting rid of it and grab the lock. Grabbing lock twice is okay. https://codereview.chromium.org/1412243012/diff/780001/components/cronet/andr... File components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamTest.java (right): https://codereview.chromium.org/1412243012/diff/780001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamTest.java:422: public void testEchoStreamStepByStep() throws Exception { Maybe add a test where a second writeData is called before the first write completes. Same goes to readData. Just want to make sure there can't be two reads or two writes in flight. Maybe also test the case where one read and one write are in flight. That should be okay/allowed. https://codereview.chromium.org/1412243012/diff/780001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamTest.java:648: public void testThrowOnSucceeded() { Could you document this test? It isn't clear what the expectation is and what this test is for. https://codereview.chromium.org/1412243012/diff/780001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamTest.java:689: // Shutdown the executor, so posting the task will throw an exception. nit: s/Shutdown/shut down. https://codereview.chromium.org/1412243012/diff/780001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamTest.java:693: // Callback will never be called again because executor is shutdown, nit: s/shutdown/shut down. https://codereview.chromium.org/1412243012/diff/780001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamTest.java:705: class ShutdownTestBidirectionalStreamCallback extends TestBidirectionalStreamCallback { nit: add private? https://codereview.chromium.org/1412243012/diff/780001/components/cronet/andr... File components/cronet/android/test/javatests/src/org/chromium/net/TestBidirectionalStreamCallback.java (right): https://codereview.chromium.org/1412243012/diff/780001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/TestBidirectionalStreamCallback.java:139: assertTrue(mResponseStep == ResponseStep.NOTHING); nit: assertEquals https://codereview.chromium.org/1412243012/diff/780001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/TestBidirectionalStreamCallback.java:151: assertEquals(mExecutorThread, Thread.currentThread()); nit: assertEquals(Thread.currentThread(), mExecutorThread); expected result is first param https://codereview.chromium.org/1412243012/diff/780001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/TestBidirectionalStreamCallback.java:196: BidirectionalStream stream, UrlResponseInfo info, ByteBuffer buffer) { Need also update mResponseStep. https://codereview.chromium.org/1412243012/diff/780001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/TestBidirectionalStreamCallback.java:212: UrlResponseInfo.HeaderBlock trailers) { Need also update mResponseStep. https://codereview.chromium.org/1412243012/diff/780001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/TestBidirectionalStreamCallback.java:239: public void onFailed(BidirectionalStream stream, UrlResponseInfo info, CronetException error) { need to update mResponseStep. https://codereview.chromium.org/1412243012/diff/780001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/TestBidirectionalStreamCallback.java:256: public void onCanceled(BidirectionalStream stream, UrlResponseInfo info) { need to update mResponseStep. https://codereview.chromium.org/1412243012/diff/780001/components/cronet/andr... File components/cronet/android/test/src/org/chromium/net/Http2TestServer.java (right): https://codereview.chromium.org/1412243012/diff/780001/components/cronet/andr... components/cronet/android/test/src/org/chromium/net/Http2TestServer.java:40: public final class Http2TestServer { maybe add a private constructor? https://codereview.chromium.org/1412243012/diff/780001/components/cronet/andr... components/cronet/android/test/src/org/chromium/net/Http2TestServer.java:45: static final int PORT = 8443; add private? https://codereview.chromium.org/1412243012/diff/780001/components/cronet/andr... components/cronet/android/test/src/org/chromium/net/Http2TestServer.java:89: static String getEchoTrailersUrl() { Why are these methods some public and some package private? https://codereview.chromium.org/1412243012/diff/780001/components/cronet/andr... components/cronet/android/test/src/org/chromium/net/Http2TestServer.java:112: private static void runHttp2TestServer(File certFile, File keyFile) throws Exception { maybe move this method to Http2TestServerRunnable?
I'm pretty happy with this CL as it stands now. I don't have any more pressing comments. The non-netty stuff lgtm. https://codereview.chromium.org/1412243012/diff/740001/components/cronet/andr... File components/cronet/android/test/src/org/chromium/net/Http2TestServer.java (right): https://codereview.chromium.org/1412243012/diff/740001/components/cronet/andr... components/cronet/android/test/src/org/chromium/net/Http2TestServer.java:82: return getBaseUrl() + "echomethod"; On 2016/01/20 15:37:41, mef wrote: > On 2016/01/19 16:03:41, pauljensen wrote: > > these strings (e.g. "echomethod") seem duplicated between here and the server; > > can they be moved into a shared location? > > They sure can. I think it would make sense to unify native test server, quic > test server and http2 test server urls. > Maybe in a separate CL though. WDYT? The more unification the better :) It may be harder to unify the other servers, e.g. I think the native server gets the URLs from C++ code. I think we may want to ensure this CL isn't making things worse though, and at least unify the URLs in this CL. https://codereview.chromium.org/1412243012/diff/780001/components/cronet/andr... File components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java (right): https://codereview.chromium.org/1412243012/diff/780001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:329: private boolean isDoneLocked() { On 2016/01/21 19:13:08, xunjieli wrote: > I believe grabbing the same lock is essentially no-op and allowed in Java. Maybe > get rid of this method, and just use isDone(). Locks in Java are re-entrant but I imagine there may be a significant cost to any synchronization operations. My previous investigations into Cronet performance saw significant fractions of our time in Java locking. https://codereview.chromium.org/1412243012/diff/780001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:561: private void destroyNativeStreamLocked(boolean sendOnCanceled) { On 2016/01/21 19:13:08, xunjieli wrote: > Having the "Locked" suffix is confusing. Suggest getting rid of it and grab the > lock. Grabbing lock twice is okay. I thought the "Locked" suffix was a common pattern. There are more then 1,000 instances of "Locked(" in Chromium and more than 10,000 instances in Android. I think the performance impact will be worth it here.
https://codereview.chromium.org/1412243012/diff/780001/components/cronet/andr... File components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java (right): https://codereview.chromium.org/1412243012/diff/780001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:561: private void destroyNativeStreamLocked(boolean sendOnCanceled) { On 2016/01/22 03:55:31, pauljensen wrote: > On 2016/01/21 19:13:08, xunjieli wrote: > > Having the "Locked" suffix is confusing. Suggest getting rid of it and grab > the > > lock. Grabbing lock twice is okay. > > I thought the "Locked" suffix was a common pattern. There are more then 1,000 > instances of "Locked(" in Chromium and more than 10,000 instances in Android. I > think the performance impact will be worth it here. I see. Learned of it for the first time. SGTM then!
Thanks a lot for your comments! I've addressed most of Helen's comments. I'll add suggested tests and try to unify test server interface in next patch. https://codereview.chromium.org/1412243012/diff/780001/components/cronet/andr... File components/cronet/android/cronet_bidirectional_stream_adapter.cc (right): https://codereview.chromium.org/1412243012/diff/780001/components/cronet/andr... components/cronet/android/cronet_bidirectional_stream_adapter.cc:28: #include "net/url_request/redirect_info.h" On 2016/01/21 19:13:07, xunjieli wrote: > not used. Done. https://codereview.chromium.org/1412243012/diff/780001/components/cronet/andr... components/cronet/android/cronet_bidirectional_stream_adapter.cc:29: #include "net/url_request/url_request_context.h" On 2016/01/21 19:13:07, xunjieli wrote: > not used? It is needed in cronet::CronetBidirectionalStreamAdapter::StartOnNetworkThread() https://codereview.chromium.org/1412243012/diff/780001/components/cronet/andr... components/cronet/android/cronet_bidirectional_stream_adapter.cc:60: // to the memory backed by the ByteBuffer, and position is the location to On 2016/01/21 19:13:07, xunjieli wrote: > nit: s/position/|position|. Maybe also mention |limit|. Done. https://codereview.chromium.org/1412243012/diff/780001/components/cronet/andr... components/cronet/android/cronet_bidirectional_stream_adapter.cc:78: jobject byte_buffer() const { return byte_buffer_.obj(); } On 2016/01/21 19:13:07, xunjieli wrote: > s/jobject/const JavaParamRef<jobject>& ? Hrm, I'm not sure, JavaParamRef seems to suggest the use for parameters. In fact comment says 'Do not create instances manually'. https://codereview.chromium.org/1412243012/diff/780001/components/cronet/andr... components/cronet/android/cronet_bidirectional_stream_adapter.cc:131: On 2016/01/21 19:13:07, xunjieli wrote: > nit: maybe remove this blank line. Done. https://codereview.chromium.org/1412243012/diff/780001/components/cronet/andr... components/cronet/android/cronet_bidirectional_stream_adapter.cc:149: DCHECK_LT(jposition, jlimit); On 2016/01/21 19:13:07, xunjieli wrote: > Should we add a check to make sure we are not on the network thread? (and for > WriteData) Per discussion with Paul it could happen that they use direct executor, and call will come from network thread, so I've removed all those checks. https://codereview.chromium.org/1412243012/diff/780001/components/cronet/andr... components/cronet/android/cronet_bidirectional_stream_adapter.cc:208: // net::BidirectionalStream::Delegate overrides (called on network thread). On 2016/01/21 19:13:07, xunjieli wrote: > nit: I haven't seen this annotation in cc file. Should this only be in the > header file? Done. https://codereview.chromium.org/1412243012/diff/780001/components/cronet/andr... components/cronet/android/cronet_bidirectional_stream_adapter.cc:211: VLOG(1) << "OnHeadersSent"; On 2016/01/21 19:13:07, xunjieli wrote: > Debugging statements here and below. We might want to remove them since the CL > is getting really close to landing, or change to DVLOG. Done. https://codereview.chromium.org/1412243012/diff/780001/components/cronet/andr... components/cronet/android/cronet_bidirectional_stream_adapter.cc:288: DCHECK(context_->IsOnNetworkThread()); On 2016/01/21 19:13:07, xunjieli wrote: > Maybe also DCHECK(!bidi_stream_) Done. https://codereview.chromium.org/1412243012/diff/780001/components/cronet/andr... File components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java (right): https://codereview.chromium.org/1412243012/diff/780001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:34: * There is one initial state - State.NOT_STARTED. There is one normal final state - On 2016/01/21 19:13:08, xunjieli wrote: > nit: single dash reads very much like a minus sign to me. Maybe consider using > double dash (--) or comma. Interesting. Double dash seems unusual to me. :) I've changed dash to semicolumn. https://codereview.chromium.org/1412243012/diff/780001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:45: WAITING_ON_READ, On 2016/01/21 19:13:08, xunjieli wrote: > Should this say WAITING_FOR_READ instead of ON? Since we are waiting for > read() to be called. Excellent suggestion! Done. https://codereview.chromium.org/1412243012/diff/780001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:54: /* Reading and writing is done, and the stream is closed successfully. */ On 2016/01/21 19:13:08, xunjieli wrote: > nit: s/is/are. Reading and writing are done Done. https://codereview.chromium.org/1412243012/diff/780001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:57: WAITING_ON_WRITE, On 2016/01/21 19:13:08, xunjieli wrote: > Should this say WAITING_FOR_WRITE instead of ON? Since we are waiting for > write() to be called. Done. https://codereview.chromium.org/1412243012/diff/780001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:75: * Synchronize access to mNativeStream, mReadState and mWriteState. On 2016/01/21 19:13:08, xunjieli wrote: > nit: s/Synchronize/Synchronizes Done. https://codereview.chromium.org/1412243012/diff/780001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:87: * NOT_STARTED -> STARTED --> WAITING_ON_READ -> READING_DONE -> SUCCESS On 2016/01/21 19:13:08, xunjieli wrote: > Great flow diagram! This makes things much easier to understand. Have you > checked out the generated JavaDoc? Not sure if JavaDoc generator will mess this > up. Kudos to Paul for suggestion! I don't think it gets into javadoc as it is just a private member. https://codereview.chromium.org/1412243012/diff/780001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:192: private boolean maybeSucceededLocked() { On 2016/01/21 19:13:08, xunjieli wrote: > - Suggest making this method return void. (Early return when not done). I'm not sure I understand, the result is important to callers. > - I am trying to think of a better name for this method. The "Locked" suffix > makes it hard to read. Maybe just simply "maybeSucceed" ? It used to be called 'maybeSucceed', I've renamed it to maybeSucceedLocked (among other xyzLocked methods) to signify that they must be guarded externally. It was Paul's suggestion that I agree with. > - Maybe also move this private method to where other private methods are. Done. https://codereview.chromium.org/1412243012/diff/780001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:204: // Destroy native stream first, so request context could be shut On 2016/01/21 19:13:08, xunjieli wrote: > nit: maybe spell out the "request context" as UrlRequestContext? a bit clearer > that way. Done. https://codereview.chromium.org/1412243012/diff/780001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:245: // If there's an exception, cleanup and then throw the On 2016/01/21 19:13:08, xunjieli wrote: > nit: s/cleanup/clean up. (Cleanup is the noun) Done. https://codereview.chromium.org/1412243012/diff/780001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:329: private boolean isDoneLocked() { On 2016/01/21 19:13:08, xunjieli wrote: > I believe grabbing the same lock is essentially no-op and allowed in Java. Maybe > get rid of this method, and just use isDone(). See previous discussion with Paul. I would imagine that already-grabbed lock is cheap, but not quite free. https://codereview.chromium.org/1412243012/diff/780001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:561: private void destroyNativeStreamLocked(boolean sendOnCanceled) { On 2016/01/21 19:13:08, xunjieli wrote: > Having the "Locked" suffix is confusing. Suggest getting rid of it and grab the > lock. Grabbing lock twice is okay. Acknowledged. https://codereview.chromium.org/1412243012/diff/780001/components/cronet/andr... File components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamTest.java (right): https://codereview.chromium.org/1412243012/diff/780001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamTest.java:422: public void testEchoStreamStepByStep() throws Exception { On 2016/01/21 19:13:08, xunjieli wrote: > Maybe add a test where a second writeData is called before the first write > completes. Same goes to readData. Just want to make sure there can't be two > reads or two writes in flight. Maybe also test the case where one read and one > write are in flight. That should be okay/allowed. Good ideas! I'll add that. https://codereview.chromium.org/1412243012/diff/780001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamTest.java:648: public void testThrowOnSucceeded() { On 2016/01/21 19:13:08, xunjieli wrote: > Could you document this test? It isn't clear what the expectation is and what > this test is for. Done. https://codereview.chromium.org/1412243012/diff/780001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamTest.java:689: // Shutdown the executor, so posting the task will throw an exception. On 2016/01/21 19:13:08, xunjieli wrote: > nit: s/Shutdown/shut down. Done. https://codereview.chromium.org/1412243012/diff/780001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamTest.java:693: // Callback will never be called again because executor is shutdown, On 2016/01/21 19:13:08, xunjieli wrote: > nit: s/shutdown/shut down. Done. https://codereview.chromium.org/1412243012/diff/780001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamTest.java:705: class ShutdownTestBidirectionalStreamCallback extends TestBidirectionalStreamCallback { On 2016/01/21 19:13:08, xunjieli wrote: > nit: add private? Done. https://codereview.chromium.org/1412243012/diff/780001/components/cronet/andr... File components/cronet/android/test/javatests/src/org/chromium/net/TestBidirectionalStreamCallback.java (right): https://codereview.chromium.org/1412243012/diff/780001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/TestBidirectionalStreamCallback.java:139: assertTrue(mResponseStep == ResponseStep.NOTHING); On 2016/01/21 19:13:08, xunjieli wrote: > nit: assertEquals Done. https://codereview.chromium.org/1412243012/diff/780001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/TestBidirectionalStreamCallback.java:151: assertEquals(mExecutorThread, Thread.currentThread()); On 2016/01/21 19:13:08, xunjieli wrote: > nit: assertEquals(Thread.currentThread(), mExecutorThread); expected result is > first param Hrm, I think that we are expecting to be called on |mExecutorThread|, so this is correct. https://codereview.chromium.org/1412243012/diff/780001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/TestBidirectionalStreamCallback.java:196: BidirectionalStream stream, UrlResponseInfo info, ByteBuffer buffer) { On 2016/01/21 19:13:08, xunjieli wrote: > Need also update mResponseStep. Done. https://codereview.chromium.org/1412243012/diff/780001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/TestBidirectionalStreamCallback.java:212: UrlResponseInfo.HeaderBlock trailers) { On 2016/01/21 19:13:08, xunjieli wrote: > Need also update mResponseStep. Done. https://codereview.chromium.org/1412243012/diff/780001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/TestBidirectionalStreamCallback.java:239: public void onFailed(BidirectionalStream stream, UrlResponseInfo info, CronetException error) { On 2016/01/21 19:13:08, xunjieli wrote: > need to update mResponseStep. Done. https://codereview.chromium.org/1412243012/diff/780001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/TestBidirectionalStreamCallback.java:256: public void onCanceled(BidirectionalStream stream, UrlResponseInfo info) { On 2016/01/21 19:13:08, xunjieli wrote: > need to update mResponseStep. Done. https://codereview.chromium.org/1412243012/diff/780001/components/cronet/andr... File components/cronet/android/test/src/org/chromium/net/Http2TestServer.java (right): https://codereview.chromium.org/1412243012/diff/780001/components/cronet/andr... components/cronet/android/test/src/org/chromium/net/Http2TestServer.java:40: public final class Http2TestServer { On 2016/01/21 19:13:08, xunjieli wrote: > maybe add a private constructor? Done. https://codereview.chromium.org/1412243012/diff/780001/components/cronet/andr... components/cronet/android/test/src/org/chromium/net/Http2TestServer.java:45: static final int PORT = 8443; On 2016/01/21 19:13:08, xunjieli wrote: > add private? Done. https://codereview.chromium.org/1412243012/diff/780001/components/cronet/andr... components/cronet/android/test/src/org/chromium/net/Http2TestServer.java:89: static String getEchoTrailersUrl() { On 2016/01/21 19:13:08, xunjieli wrote: > Why are these methods some public and some package private? Done. https://codereview.chromium.org/1412243012/diff/780001/components/cronet/andr... components/cronet/android/test/src/org/chromium/net/Http2TestServer.java:112: private static void runHttp2TestServer(File certFile, File keyFile) throws Exception { On 2016/01/21 19:13:08, xunjieli wrote: > maybe move this method to Http2TestServerRunnable? Done.
https://codereview.chromium.org/1412243012/diff/780001/components/cronet/andr... File components/cronet/android/cronet_bidirectional_stream_adapter.cc (right): https://codereview.chromium.org/1412243012/diff/780001/components/cronet/andr... components/cronet/android/cronet_bidirectional_stream_adapter.cc:29: #include "net/url_request/url_request_context.h" On 2016/01/22 14:33:43, mef wrote: > On 2016/01/21 19:13:07, xunjieli wrote: > > not used? > > It is needed in cronet::CronetBidirectionalStreamAdapter::StartOnNetworkThread() > Acknowledged. https://codereview.chromium.org/1412243012/diff/780001/components/cronet/andr... components/cronet/android/cronet_bidirectional_stream_adapter.cc:78: jobject byte_buffer() const { return byte_buffer_.obj(); } On 2016/01/22 14:33:43, mef wrote: > On 2016/01/21 19:13:07, xunjieli wrote: > > s/jobject/const JavaParamRef<jobject>& ? > > Hrm, I'm not sure, JavaParamRef seems to suggest the use for parameters. > In fact comment says 'Do not create instances manually'. Last time I talked to torne@, he said that we should never see a raw jobject. I guess we will just leave this as it is, since the refactoring is still in progress. https://codereview.chromium.org/1412243012/diff/780001/components/cronet/andr... components/cronet/android/cronet_bidirectional_stream_adapter.cc:149: DCHECK_LT(jposition, jlimit); On 2016/01/22 14:33:43, mef wrote: > On 2016/01/21 19:13:07, xunjieli wrote: > > Should we add a check to make sure we are not on the network thread? (and for > > WriteData) > > Per discussion with Paul it could happen that they use direct executor, and call > will come from network thread, so I've removed all those checks. Acknowledged. https://codereview.chromium.org/1412243012/diff/780001/components/cronet/andr... File components/cronet/android/cronet_bidirectional_stream_adapter.h (right): https://codereview.chromium.org/1412243012/diff/780001/components/cronet/andr... components/cronet/android/cronet_bidirectional_stream_adapter.h:18: #include "net/base/request_priority.h" not used? https://codereview.chromium.org/1412243012/diff/780001/components/cronet/andr... File components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java (right): https://codereview.chromium.org/1412243012/diff/780001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:34: * There is one initial state - State.NOT_STARTED. There is one normal final state - On 2016/01/22 14:33:44, mef wrote: > On 2016/01/21 19:13:08, xunjieli wrote: > > nit: single dash reads very much like a minus sign to me. Maybe consider using > > double dash (--) or comma. > > Interesting. Double dash seems unusual to me. :) I've changed dash to > semicolumn. Acknowledged. https://codereview.chromium.org/1412243012/diff/780001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:87: * NOT_STARTED -> STARTED --> WAITING_ON_READ -> READING_DONE -> SUCCESS On 2016/01/22 14:33:44, mef wrote: > On 2016/01/21 19:13:08, xunjieli wrote: > > Great flow diagram! This makes things much easier to understand. Have you > > checked out the generated JavaDoc? Not sure if JavaDoc generator will mess > this > > up. > > Kudos to Paul for suggestion! I don't think it gets into javadoc as it is just a > private member. Acknowledged. Yep you are right. Didn't realize that. https://codereview.chromium.org/1412243012/diff/780001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:192: private boolean maybeSucceededLocked() { On 2016/01/22 14:33:44, mef wrote: > On 2016/01/21 19:13:08, xunjieli wrote: > > - Suggest making this method return void. (Early return when not done). > I'm not sure I understand, the result is important to callers. I think I missed a part of the logic in the first pass. Could you add documentation to this method? (specially the return value). > > > - I am trying to think of a better name for this method. The "Locked" suffix > > makes it hard to read. Maybe just simply "maybeSucceed" ? > It used to be called 'maybeSucceed', I've renamed it to maybeSucceedLocked > (among other xyzLocked methods) to signify that they must be guarded externally. > It was Paul's suggestion that I agree with. > > > - Maybe also move this private method to where other private methods are. > Done. > Right now the method is named as maybeSucceededLocked(). There are an extra "ed" after Succeed. Could you remove that?
https://codereview.chromium.org/1412243012/diff/780001/components/cronet/andr... File components/cronet/android/cronet_bidirectional_stream_adapter.cc (right): https://codereview.chromium.org/1412243012/diff/780001/components/cronet/andr... components/cronet/android/cronet_bidirectional_stream_adapter.cc:149: DCHECK_LT(jposition, jlimit); On 2016/01/22 14:33:43, mef wrote: > On 2016/01/21 19:13:07, xunjieli wrote: > > Should we add a check to make sure we are not on the network thread? (and for > > WriteData) > > Per discussion with Paul it could happen that they use direct executor, and call > will come from network thread, so I've removed all those checks. Why would they want to use a direct executor? Are we encouraging the use of direct executor by not having the check? In the case of UrlRequest, it is understandable, since they might want to emulate the single threaded behavior of HttpURLConnection.
On 2016/01/22 14:55:40, xunjieli wrote: > https://codereview.chromium.org/1412243012/diff/780001/components/cronet/andr... > File components/cronet/android/cronet_bidirectional_stream_adapter.cc (right): > > https://codereview.chromium.org/1412243012/diff/780001/components/cronet/andr... > components/cronet/android/cronet_bidirectional_stream_adapter.cc:149: > DCHECK_LT(jposition, jlimit); > On 2016/01/22 14:33:43, mef wrote: > > On 2016/01/21 19:13:07, xunjieli wrote: > > > Should we add a check to make sure we are not on the network thread? (and > for > > > WriteData) > > > > Per discussion with Paul it could happen that they use direct executor, and > call > > will come from network thread, so I've removed all those checks. > > Why would they want to use a direct executor? Are we encouraging the use of > direct executor by not having the check? In the case of UrlRequest, it is > understandable, since they might want to emulate the single threaded behavior of > HttpURLConnection. Basically to provide bigger footgun. :) Per conversation with grpc team they really want to be invoked on the network thread and handle posting in grpc. They already do that with Netty-based implementation. It requires more attention, but theoretically it could reduce the number of thread hops and thread contention, which is good for performance.
On Fri, Jan 22, 2016 at 10:30 AM, <mef@chromium.org> wrote: > On 2016/01/22 14:55:40, xunjieli wrote: > > > https://codereview.chromium.org/1412243012/diff/780001/components/cronet/andr... > >> File components/cronet/android/cronet_bidirectional_stream_adapter.cc >> (right): >> > > > > https://codereview.chromium.org/1412243012/diff/780001/components/cronet/andr... > >> components/cronet/android/cronet_bidirectional_stream_adapter.cc:149: >> DCHECK_LT(jposition, jlimit); >> On 2016/01/22 14:33:43, mef wrote: >> > On 2016/01/21 19:13:07, xunjieli wrote: >> > > Should we add a check to make sure we are not on the network thread? >> (and >> for >> > > WriteData) >> > >> > Per discussion with Paul it could happen that they use direct executor, >> and >> call >> > will come from network thread, so I've removed all those checks. >> > > Why would they want to use a direct executor? Are we encouraging the use of >> direct executor by not having the check? In the case of UrlRequest, it is >> understandable, since they might want to emulate the single threaded >> behavior >> > of > >> HttpURLConnection. >> > > Basically to provide bigger footgun. :) > Per conversation with grpc team they really want to be invoked on the > network > thread and handle posting in grpc. > They already do that with Netty-based implementation. > It requires more attention, but theoretically it could reduce the number of > thread hops and thread contention, which is good for performance. > > I see. If that's their use case, we might consider doing the task directly if already on network thread. That would save us 7 us for every unnecessary same-thread PostTask call. Good as it is now I guess. > https://codereview.chromium.org/1412243012/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/1412243012/diff/800001/components/cronet/andr... File components/cronet/android/cronet_bidirectional_stream_adapter.cc (right): https://codereview.chromium.org/1412243012/diff/800001/components/cronet/andr... components/cronet/android/cronet_bidirectional_stream_adapter.cc:60: // |position| is the offset of the data inside of the buffer. |limit| is the offsite implies that it might be negative, but |position| can never be negative. Maybe consider rephrasing? sth like: the index of the next element to be read or written https://codereview.chromium.org/1412243012/diff/800001/components/cronet/andr... components/cronet/android/cronet_bidirectional_stream_adapter.cc:61: // maximum count of bytes in the buffer, preserved to verify that buffer is I think the documentation of |limit| is a bit off. |limit| is not the count of bytes but the index of the first element that should not be read or written. https://codereview.chromium.org/1412243012/diff/800001/components/cronet/andr... File components/cronet/android/cronet_bidirectional_stream_adapter.h (right): https://codereview.chromium.org/1412243012/diff/800001/components/cronet/andr... components/cronet/android/cronet_bidirectional_stream_adapter.h:32: // Created and configured from a Java thread. Start, ReadData, WriteData and Could you mention at the end of this sentence that |this| is destroyed on the network thread. https://codereview.chromium.org/1412243012/diff/800001/components/cronet/andr... components/cronet/android/cronet_bidirectional_stream_adapter.h:113: // Gets headers as Java array. nit: s/Java array/a Java array. https://codereview.chromium.org/1412243012/diff/800001/components/cronet/andr... File components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java (right): https://codereview.chromium.org/1412243012/diff/800001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:512: private static String[] stringsFromHeaderList(List<Map.Entry<String, String>> headersList) { Although there is no official rule on class member ordering (https://google.github.io/styleguide/javaguide.html#s3.4.2-class-member-ordering), suggest grouping these four static methods together and moving them before non-static private methods. https://codereview.chromium.org/1412243012/diff/800001/components/cronet/andr... File components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamTest.java (right): https://codereview.chromium.org/1412243012/diff/800001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamTest.java:468: public void testSimpleGetBufferUpdates() throws Exception { Suggest: in this test or another test, test that ResponseStep.ON_TRAILERS happens before ON_READ_COMPLETED if we do not auto advance read. https://codereview.chromium.org/1412243012/diff/800001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamTest.java:668: public void testExecutorShutdown() { Suggest renaming this test. Maybe testStreamDestroyedWhenRequestDone or sth to make it clearer that this tests that the stream is destroyed when request is done? https://codereview.chromium.org/1412243012/diff/800001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamTest.java:682: final ConditionVariable requestDestroyed = new ConditionVariable(false); suggest s/requestDestroyed/streamDestroyed. https://codereview.chromium.org/1412243012/diff/800001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamTest.java:710: // Clear mCronetEngine so it doesn't get shutdown second time in tearDown(). nit: s/shutdown/shut down. shutdown happens to be exclusively a noun. Same for line 720 and 727. https://codereview.chromium.org/1412243012/diff/800001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamTest.java:736: public void testShutdown() throws Exception { Hard to see the distinction between this and testExecutorShutdown. Suggest adding a documentation here saying that this tests that the executor cannot be shut down if there are active requests? https://codereview.chromium.org/1412243012/diff/800001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamTest.java:783: public void testShutdownAfterError() throws Exception { Suggest adding a documentation to this test. It's not clear what the use of this test is. https://codereview.chromium.org/1412243012/diff/800001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamTest.java:812: mTestFramework.mCronetEngine.shutdown(); This test is confusing. The callback class calls shutdown(), and here shutdown() is called again. Not sure which shutdown it is mentioned in the test name. Maybe add some comment? https://codereview.chromium.org/1412243012/diff/800001/components/cronet/andr... File components/cronet/android/test/javatests/src/org/chromium/net/TestBidirectionalStreamCallback.java (right): https://codereview.chromium.org/1412243012/diff/800001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/TestBidirectionalStreamCallback.java:42: // When false, the consumer is responsible for all calls into the request nit: s/request/stream https://codereview.chromium.org/1412243012/diff/800001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/TestBidirectionalStreamCallback.java:50: // Signals when request is done either successfully or not. nit: s/request/stream https://codereview.chromium.org/1412243012/diff/800001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/TestBidirectionalStreamCallback.java:91: // Same as above, but continues to advance the request after posting nit: s/request/stream https://codereview.chromium.org/1412243012/diff/800001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/TestBidirectionalStreamCallback.java:183: // This restores |byteBuffer.position()| to its value on entrance to nit: maybe remove pipes on 181 and 183. https://codereview.chromium.org/1412243012/diff/800001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/TestBidirectionalStreamCallback.java:218: mTrailers = trailers; need to update mResponseStep. https://codereview.chromium.org/1412243012/diff/800001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/TestBidirectionalStreamCallback.java:247: // Should happen at most once for a single request. nit: s/request/stream https://codereview.chromium.org/1412243012/diff/800001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/TestBidirectionalStreamCallback.java:263: // Should happen at most once for a single request. nit: s/request/stream https://codereview.chromium.org/1412243012/diff/800001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/TestBidirectionalStreamCallback.java:303: * request. nit: s/request/stream https://codereview.chromium.org/1412243012/diff/800001/components/cronet/andr... File components/cronet/android/test/src/org/chromium/net/Http2TestHandler.java (right): https://codereview.chromium.org/1412243012/diff/800001/components/cronet/andr... components/cronet/android/test/src/org/chromium/net/Http2TestHandler.java:95: public int onDataRead(ChannelHandlerContext ctx, int streamId, ByteBuf data, int padding, Suggest grouping public methods together.
A few additional comments. I will wait for tests now :) https://codereview.chromium.org/1412243012/diff/800001/components/cronet/andr... File components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java (right): https://codereview.chromium.org/1412243012/diff/800001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:26: * and post tasks with callback calls onto Executor. Upon return from callback, the native s/Upon return/Upon returning https://codereview.chromium.org/1412243012/diff/800001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:163: if (mWriteState == State.WRITING_END_OF_STREAM) { Is it possible to get rid of WRITING_END_OF_STREAM state and just add a mEndOfStream to the runnable? That way write states will be closer to the read states. https://codereview.chromium.org/1412243012/diff/800001/components/cronet/andr... File components/cronet/android/test/src/org/chromium/net/Http2TestHandler.java (right): https://codereview.chromium.org/1412243012/diff/800001/components/cronet/andr... components/cronet/android/test/src/org/chromium/net/Http2TestHandler.java:34: final class Http2TestHandler extends Http2ConnectionHandler implements Http2FrameListener { Suggest adding public modifier to the package private classes and methods. All other classes in test/src/org/chromium/net/ have. It might be better to be consistent. https://codereview.chromium.org/1412243012/diff/800001/components/cronet/andr... components/cronet/android/test/src/org/chromium/net/Http2TestHandler.java:39: private boolean mEchoStream = false; Suggest avoid initializing boolean explicitly here. (Once recommeneded by Paul). https://codereview.chromium.org/1412243012/diff/800001/components/cronet/andr... components/cronet/android/test/src/org/chromium/net/Http2TestHandler.java:87: private void sendResponse(ChannelHandlerContext ctx, int streamId, ByteBuf payload) { Suggest adding a "do" prefix and group this together with other do* methods https://codereview.chromium.org/1412243012/diff/800001/components/cronet/andr... components/cronet/android/test/src/org/chromium/net/Http2TestHandler.java:125: private String getEchoAllHeadersResponse(Http2Headers headers) { static helper method? https://codereview.chromium.org/1412243012/diff/800001/components/cronet/andr... components/cronet/android/test/src/org/chromium/net/Http2TestHandler.java:133: private String getEchoHeaderResponse(Http2Headers headers) { static helper method? https://codereview.chromium.org/1412243012/diff/800001/components/cronet/andr... File components/cronet/android/test/src/org/chromium/net/Http2TestServer.java (right): https://codereview.chromium.org/1412243012/diff/800001/components/cronet/andr... components/cronet/android/test/src/org/chromium/net/Http2TestServer.java:40: final class Http2TestServer { Suggest adding public modifiers (see a previous comment).
Some comments so far: https://codereview.chromium.org/1412243012/diff/800001/components/cronet/andr... File components/cronet/android/api/src/org/chromium/net/BidirectionalStream.java (right): https://codereview.chromium.org/1412243012/diff/800001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/BidirectionalStream.java:157: public Builder setPriority(@StreamPriority int priority) { Not related to this CL but I noticed that @StreamPriority annotation is deprecated/hidden. Since this is the public API we should find a better alternative. https://codereview.chromium.org/1412243012/diff/800001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/BidirectionalStream.java:179: public abstract static class Callback { I wonder why the Callback class is a class and not an interface. Most of its methods are abstract. The ones that are not abstract are empty. Since Java does not support multiple inheritance, this may force the implementers to create an inner class which is not always desirable. If needed, we can have a DefaultCallback class that implements Callback to provide default implementation with no-op methods. https://codereview.chromium.org/1412243012/diff/800001/components/cronet/andr... File components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java (right): https://codereview.chromium.org/1412243012/diff/800001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:299: return mReadState != State.NOT_STARTED && mNativeStream == 0; Should we protect the read with synchronized(mNativeStreamLock) here? Since mReadState is not an atomic variable, I am not sure whether it can produce inconsistent result if the variable modified concurrently by another thread. https://codereview.chromium.org/1412243012/diff/800001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:336: prepareResponseInfoOnNetworkThread(httpStatusCode, negotiatedProtocol, headers); prepareResponseInfoOnNetworkThread() method can return null, so the subsequent line can cause NullPointerException. I think we should handle this case and generate a better exception/'error message'. https://codereview.chromium.org/1412243012/diff/800001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:504: new ArrayList<Map.Entry<String, String>>(headers.length / 2); Can we use 'diamond' syntax here, i.e. instead of new ArrayList<Map.Entry<String, String>> just use ArrayList<>? https://codereview.chromium.org/1412243012/diff/800001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:506: headersList.add(new AbstractMap.SimpleImmutableEntry<String, String>( Same here. https://codereview.chromium.org/1412243012/diff/800001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:530: ArrayList<String> urlChain = new ArrayList<String>(); Same here. https://codereview.chromium.org/1412243012/diff/800001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:538: headersListFromStrings(headers), wasCached, negotiatedProtocol, proxyServer); wasCached is always 'false'. httpStatusText is always "". proxyServer is always 'null'. Should we just pass 'false', "", & 'null' and get rid of the variables? Is it done for readability?
Thanks! I'm still thinking about the test. https://codereview.chromium.org/1412243012/diff/780001/components/cronet/andr... File components/cronet/android/cronet_bidirectional_stream_adapter.cc (right): https://codereview.chromium.org/1412243012/diff/780001/components/cronet/andr... components/cronet/android/cronet_bidirectional_stream_adapter.cc:78: jobject byte_buffer() const { return byte_buffer_.obj(); } On 2016/01/22 14:52:22, xunjieli wrote: > On 2016/01/22 14:33:43, mef wrote: > > On 2016/01/21 19:13:07, xunjieli wrote: > > > s/jobject/const JavaParamRef<jobject>& ? > > > > Hrm, I'm not sure, JavaParamRef seems to suggest the use for parameters. > > In fact comment says 'Do not create instances manually'. > > Last time I talked to torne@, he said that we should never see a raw jobject. I > guess we will just leave this as it is, since the refactoring is still in > progress. Acknowledged. https://codereview.chromium.org/1412243012/diff/780001/components/cronet/andr... File components/cronet/android/cronet_bidirectional_stream_adapter.h (right): https://codereview.chromium.org/1412243012/diff/780001/components/cronet/andr... components/cronet/android/cronet_bidirectional_stream_adapter.h:18: #include "net/base/request_priority.h" On 2016/01/22 14:52:22, xunjieli wrote: > not used? Done. https://codereview.chromium.org/1412243012/diff/780001/components/cronet/andr... File components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java (right): https://codereview.chromium.org/1412243012/diff/780001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:192: private boolean maybeSucceededLocked() { On 2016/01/22 14:52:23, xunjieli wrote: > On 2016/01/22 14:33:44, mef wrote: > > On 2016/01/21 19:13:08, xunjieli wrote: > > > - Suggest making this method return void. (Early return when not done). > > I'm not sure I understand, the result is important to callers. > I think I missed a part of the logic in the first pass. Could you add > documentation to this method? (specially the return value). > > > > > - I am trying to think of a better name for this method. The "Locked" suffix > > > makes it hard to read. Maybe just simply "maybeSucceed" ? > > It used to be called 'maybeSucceed', I've renamed it to maybeSucceedLocked > > (among other xyzLocked methods) to signify that they must be guarded > externally. > > It was Paul's suggestion that I agree with. > > > > > - Maybe also move this private method to where other private methods are. > > Done. > > > > Right now the method is named as maybeSucceededLocked(). There are an extra "ed" > after Succeed. Could you remove that? Done. https://codereview.chromium.org/1412243012/diff/800001/components/cronet/andr... File components/cronet/android/api/src/org/chromium/net/BidirectionalStream.java (right): https://codereview.chromium.org/1412243012/diff/800001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/BidirectionalStream.java:157: public Builder setPriority(@StreamPriority int priority) { On 2016/01/22 16:58:14, kapishnikov wrote: > Not related to this CL but I noticed that @StreamPriority annotation is > deprecated/hidden. Since this is the public API we should find a better > alternative. Yeah, we need to re-work the whole priority API, so we didn't want to publicly expose this yet. https://codereview.chromium.org/1412243012/diff/800001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/BidirectionalStream.java:179: public abstract static class Callback { On 2016/01/22 16:58:14, kapishnikov wrote: > I wonder why the Callback class is a class and not an interface. Most of its > methods are abstract. The ones that are not abstract are empty. Since Java does > not support multiple inheritance, this may force the implementers to create an > inner class which is not always desirable. If needed, we can have a > DefaultCallback class that implements Callback to provide default implementation > with no-op methods. Per Android API guidelines. https://codereview.chromium.org/1412243012/diff/800001/components/cronet/andr... File components/cronet/android/cronet_bidirectional_stream_adapter.cc (right): https://codereview.chromium.org/1412243012/diff/800001/components/cronet/andr... components/cronet/android/cronet_bidirectional_stream_adapter.cc:60: // |position| is the offset of the data inside of the buffer. |limit| is the On 2016/01/22 16:12:18, xunjieli wrote: > offsite implies that it might be negative, but |position| can never be negative. > Maybe consider rephrasing? sth like: > the index of the next element to be read or written Done. offset -> index. https://codereview.chromium.org/1412243012/diff/800001/components/cronet/andr... components/cronet/android/cronet_bidirectional_stream_adapter.cc:61: // maximum count of bytes in the buffer, preserved to verify that buffer is On 2016/01/22 16:12:18, xunjieli wrote: > I think the documentation of |limit| is a bit off. |limit| is not the count of > bytes but the index of the first element that should not be read or written. Done. https://codereview.chromium.org/1412243012/diff/800001/components/cronet/andr... File components/cronet/android/cronet_bidirectional_stream_adapter.h (right): https://codereview.chromium.org/1412243012/diff/800001/components/cronet/andr... components/cronet/android/cronet_bidirectional_stream_adapter.h:32: // Created and configured from a Java thread. Start, ReadData, WriteData and On 2016/01/22 16:12:18, xunjieli wrote: > Could you mention at the end of this sentence that |this| is destroyed on the > network thread. Done. https://codereview.chromium.org/1412243012/diff/800001/components/cronet/andr... components/cronet/android/cronet_bidirectional_stream_adapter.h:113: // Gets headers as Java array. On 2016/01/22 16:12:18, xunjieli wrote: > nit: s/Java array/a Java array. Done. https://codereview.chromium.org/1412243012/diff/800001/components/cronet/andr... File components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java (right): https://codereview.chromium.org/1412243012/diff/800001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:26: * and post tasks with callback calls onto Executor. Upon return from callback, the native On 2016/01/22 16:45:20, xunjieli wrote: > s/Upon return/Upon returning Done. https://codereview.chromium.org/1412243012/diff/800001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:163: if (mWriteState == State.WRITING_END_OF_STREAM) { On 2016/01/22 16:45:20, xunjieli wrote: > Is it possible to get rid of WRITING_END_OF_STREAM state and just add a > mEndOfStream to the runnable? That way write states will be closer to the read > states. Hrm, that could work if we create OnWriteCompletedRunnable in 'write'. Should probably do the same to OnReadCompletedRunnable. WDYT? https://codereview.chromium.org/1412243012/diff/800001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:299: return mReadState != State.NOT_STARTED && mNativeStream == 0; On 2016/01/22 16:58:14, kapishnikov wrote: > Should we protect the read with synchronized(mNativeStreamLock) here? Since > mReadState is not an atomic variable, I am not sure whether it can produce > inconsistent result if the variable modified concurrently by another thread. Yep, it is guarded by mNativeStreamLock and always called from synchronized(mNativeStreamLock) sections. https://codereview.chromium.org/1412243012/diff/800001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:336: prepareResponseInfoOnNetworkThread(httpStatusCode, negotiatedProtocol, headers); On 2016/01/22 16:58:14, kapishnikov wrote: > prepareResponseInfoOnNetworkThread() method can return null, so the subsequent > line can cause NullPointerException. I think we should handle this case and > generate a better exception/'error message'. Done. https://codereview.chromium.org/1412243012/diff/800001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:504: new ArrayList<Map.Entry<String, String>>(headers.length / 2); On 2016/01/22 16:58:14, kapishnikov wrote: > Can we use 'diamond' syntax here, i.e. instead of new > ArrayList<Map.Entry<String, String>> just use ArrayList<>? Done. Cute! https://codereview.chromium.org/1412243012/diff/800001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:506: headersList.add(new AbstractMap.SimpleImmutableEntry<String, String>( On 2016/01/22 16:58:14, kapishnikov wrote: > Same here. Done. https://codereview.chromium.org/1412243012/diff/800001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:512: private static String[] stringsFromHeaderList(List<Map.Entry<String, String>> headersList) { On 2016/01/22 16:12:18, xunjieli wrote: > Although there is no official rule on class member ordering > (https://google.github.io/styleguide/javaguide.html#s3.4.2-class-member-ordering), > suggest grouping these four static methods together and moving them before > non-static private methods. Done. https://codereview.chromium.org/1412243012/diff/800001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:530: ArrayList<String> urlChain = new ArrayList<String>(); On 2016/01/22 16:58:14, kapishnikov wrote: > Same here. Done. https://codereview.chromium.org/1412243012/diff/800001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:538: headersListFromStrings(headers), wasCached, negotiatedProtocol, proxyServer); On 2016/01/22 16:58:14, kapishnikov wrote: > wasCached is always 'false'. httpStatusText is always "". proxyServer is always > 'null'. Should we just pass 'false', "", & 'null' and get rid of the variables? > Is it done for readability? Done. https://codereview.chromium.org/1412243012/diff/800001/components/cronet/andr... File components/cronet/android/test/javatests/src/org/chromium/net/TestBidirectionalStreamCallback.java (right): https://codereview.chromium.org/1412243012/diff/800001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/TestBidirectionalStreamCallback.java:42: // When false, the consumer is responsible for all calls into the request On 2016/01/22 16:12:19, xunjieli wrote: > nit: s/request/stream Done. https://codereview.chromium.org/1412243012/diff/800001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/TestBidirectionalStreamCallback.java:50: // Signals when request is done either successfully or not. On 2016/01/22 16:12:19, xunjieli wrote: > nit: s/request/stream Done. https://codereview.chromium.org/1412243012/diff/800001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/TestBidirectionalStreamCallback.java:91: // Same as above, but continues to advance the request after posting On 2016/01/22 16:12:19, xunjieli wrote: > nit: s/request/stream Done. https://codereview.chromium.org/1412243012/diff/800001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/TestBidirectionalStreamCallback.java:183: // This restores |byteBuffer.position()| to its value on entrance to On 2016/01/22 16:12:19, xunjieli wrote: > nit: maybe remove pipes on 181 and 183. Done. https://codereview.chromium.org/1412243012/diff/800001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/TestBidirectionalStreamCallback.java:218: mTrailers = trailers; On 2016/01/22 16:12:19, xunjieli wrote: > need to update mResponseStep. Done. https://codereview.chromium.org/1412243012/diff/800001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/TestBidirectionalStreamCallback.java:247: // Should happen at most once for a single request. On 2016/01/22 16:12:19, xunjieli wrote: > nit: s/request/stream Done. https://codereview.chromium.org/1412243012/diff/800001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/TestBidirectionalStreamCallback.java:263: // Should happen at most once for a single request. On 2016/01/22 16:12:19, xunjieli wrote: > nit: s/request/stream Done. https://codereview.chromium.org/1412243012/diff/800001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/TestBidirectionalStreamCallback.java:303: * request. On 2016/01/22 16:12:19, xunjieli wrote: > nit: s/request/stream Done. https://codereview.chromium.org/1412243012/diff/800001/components/cronet/andr... File components/cronet/android/test/src/org/chromium/net/Http2TestHandler.java (right): https://codereview.chromium.org/1412243012/diff/800001/components/cronet/andr... components/cronet/android/test/src/org/chromium/net/Http2TestHandler.java:34: final class Http2TestHandler extends Http2ConnectionHandler implements Http2FrameListener { On 2016/01/22 16:45:20, xunjieli wrote: > Suggest adding public modifier to the package private classes and methods. All > other classes in test/src/org/chromium/net/ have. It might be better to be > consistent. Done. https://codereview.chromium.org/1412243012/diff/800001/components/cronet/andr... components/cronet/android/test/src/org/chromium/net/Http2TestHandler.java:39: private boolean mEchoStream = false; On 2016/01/22 16:45:21, xunjieli wrote: > Suggest avoid initializing boolean explicitly here. (Once recommeneded by Paul). Done. https://codereview.chromium.org/1412243012/diff/800001/components/cronet/andr... components/cronet/android/test/src/org/chromium/net/Http2TestHandler.java:87: private void sendResponse(ChannelHandlerContext ctx, int streamId, ByteBuf payload) { On 2016/01/22 16:45:20, xunjieli wrote: > Suggest adding a "do" prefix and group this together with other do* methods Done. https://codereview.chromium.org/1412243012/diff/800001/components/cronet/andr... components/cronet/android/test/src/org/chromium/net/Http2TestHandler.java:95: public int onDataRead(ChannelHandlerContext ctx, int streamId, ByteBuf data, int padding, On 2016/01/22 16:12:19, xunjieli wrote: > Suggest grouping public methods together. Done. https://codereview.chromium.org/1412243012/diff/800001/components/cronet/andr... components/cronet/android/test/src/org/chromium/net/Http2TestHandler.java:125: private String getEchoAllHeadersResponse(Http2Headers headers) { On 2016/01/22 16:45:20, xunjieli wrote: > static helper method? Done. https://codereview.chromium.org/1412243012/diff/800001/components/cronet/andr... components/cronet/android/test/src/org/chromium/net/Http2TestHandler.java:133: private String getEchoHeaderResponse(Http2Headers headers) { On 2016/01/22 16:45:20, xunjieli wrote: > static helper method? Done.
https://codereview.chromium.org/1412243012/diff/800001/components/cronet/andr... File components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java (right): https://codereview.chromium.org/1412243012/diff/800001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:163: if (mWriteState == State.WRITING_END_OF_STREAM) { On 2016/01/22 17:36:07, mef wrote: > On 2016/01/22 16:45:20, xunjieli wrote: > > Is it possible to get rid of WRITING_END_OF_STREAM state and just add a > > mEndOfStream to the runnable? That way write states will be closer to the read > > states. > > Hrm, that could work if we create OnWriteCompletedRunnable in 'write'. > Should probably do the same to OnReadCompletedRunnable. > WDYT? I don't see why not from the first glance, though I might be missing something. https://codereview.chromium.org/1412243012/diff/840001/components/cronet/andr... File components/cronet/android/cronet_bidirectional_stream_adapter.cc (right): https://codereview.chromium.org/1412243012/diff/840001/components/cronet/andr... components/cronet/android/cronet_bidirectional_stream_adapter.cc:62: // |limit| is the mthe index of the first element that should not be read or nit: there is an extra "m". https://codereview.chromium.org/1412243012/diff/840001/components/cronet/andr... components/cronet/android/cronet_bidirectional_stream_adapter.cc:64: // networking operation. nit: s/operation/operations. https://codereview.chromium.org/1412243012/diff/840001/components/cronet/andr... File components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java (right): https://codereview.chromium.org/1412243012/diff/840001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:344: } catch (Exception e) { I think Andrei might meant that a null check is needed here. if (mResponseInfo == null) { // throw an exception with a descriptive message. } https://codereview.chromium.org/1412243012/diff/840001/components/cronet/andr... File components/cronet/android/test/src/org/chromium/net/Http2TestHandler.java (right): https://codereview.chromium.org/1412243012/diff/840001/components/cronet/andr... components/cronet/android/test/src/org/chromium/net/Http2TestHandler.java:42: static final class Builder nit: consider adding public. https://codereview.chromium.org/1412243012/diff/840001/components/cronet/andr... File components/cronet/android/test/src/org/chromium/net/Http2TestServer.java (right): https://codereview.chromium.org/1412243012/diff/840001/components/cronet/andr... components/cronet/android/test/src/org/chromium/net/Http2TestServer.java:47: static boolean startHttp2TestServer(Context context, String certFileName, String keyFileName) consider adding public here and below.
Some more comments: https://codereview.chromium.org/1412243012/diff/820001/components/cronet/andr... File components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java (right): https://codereview.chromium.org/1412243012/diff/820001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:379: mOnReadCompletedTask.mByteBuffer = byteBuffer; Can we add an assertion that "mOnReadCompletedTask.mByteBuffer == null" before assigning the new value. The logic assumes that onReadCompleted() cannot be called twice before postTaskToExecutor started the task execution on a different thread; otherwise, mOnReadCompletedTask may contain invalid data. If in the future we introduce a bug that breaks this assumption, the assert will fail. https://codereview.chromium.org/1412243012/diff/820001/components/cronet/andr... File components/cronet/android/test/src/org/chromium/net/Http2TestServer.java (right): https://codereview.chromium.org/1412243012/diff/820001/components/cronet/andr... components/cronet/android/test/src/org/chromium/net/Http2TestServer.java:57: static boolean shutdownHttp2TestServer() throws Exception { This method does not do anything. Should we gracefully shutdown the server? https://codereview.chromium.org/1412243012/diff/820001/components/cronet/andr... components/cronet/android/test/src/org/chromium/net/Http2TestServer.java:66: return "127.0.0.1"; We should define 127.0.0.1 as a static constant like we did with the port. https://codereview.chromium.org/1412243012/diff/840001/components/cronet/andr... File components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java (right): https://codereview.chromium.org/1412243012/diff/840001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:473: headersArray[i++] = requestHeader.getValue(); How will 'null' header values be interpreted? From the API point of view, I would assume that if the value is not set, it is absent and should be converted to an empty string. I guess the C layer will treat it as invalid. https://codereview.chromium.org/1412243012/diff/840001/components/cronet/andr... File components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamTest.java (right): https://codereview.chromium.org/1412243012/diff/840001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamTest.java:262: builder.addHeader("header:name", "headervalue"); Can we add couple of valid headers around the invalid one to check that the index is calculated correctly? https://codereview.chromium.org/1412243012/diff/840001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamTest.java:269: Can we add a method to check what happens if a header value is absent? We can use the echo API to see what the server receives. https://codereview.chromium.org/1412243012/diff/840001/components/cronet/andr... File components/cronet/android/test/src/org/chromium/net/Http2TestHandler.java (right): https://codereview.chromium.org/1412243012/diff/840001/components/cronet/andr... components/cronet/android/test/src/org/chromium/net/Http2TestHandler.java:70: cause.printStackTrace(); It should be better to log the exception. https://codereview.chromium.org/1412243012/diff/840001/components/cronet/andr... components/cronet/android/test/src/org/chromium/net/Http2TestHandler.java:74: private static Http2Headers responseHeadersFromRequestHeaders(Http2Headers requestHeaders) { Should we rename this method to something like createEchoResponseHeadersFromRequestHeaders? Otherwise, it is not clear what the method is doing without looking inside. https://codereview.chromium.org/1412243012/diff/840001/components/cronet/andr... components/cronet/android/test/src/org/chromium/net/Http2TestHandler.java:135: encoder().writeData(ctx, streamId, data.retain(), 0, endOfStream, ctx.newPromise()); Do we need to retain data here? And if we do, should release it? https://codereview.chromium.org/1412243012/diff/840001/components/cronet/andr... components/cronet/android/test/src/org/chromium/net/Http2TestHandler.java:138: doSendResponse(ctx, streamId, data.retain()); In what scenario/tests this 'else-if' statement is going to be executed? https://codereview.chromium.org/1412243012/diff/840001/components/cronet/andr... components/cronet/android/test/src/org/chromium/net/Http2TestHandler.java:156: if (path.startsWith("/echoallheaders")) { We should externalize all path constants like "echoallheaders" and reference them from here and Http2TestServer.java, so they are always in sync. https://codereview.chromium.org/1412243012/diff/840001/components/cronet/andr... components/cronet/android/test/src/org/chromium/net/Http2TestHandler.java:171: } Should we add an error response if none of the conditions are true; I assume otherwise, the test will wait till the timeout? https://codereview.chromium.org/1412243012/diff/840001/components/cronet/andr... File components/cronet/android/test/src/org/chromium/net/Http2TestServer.java (right): https://codereview.chromium.org/1412243012/diff/840001/components/cronet/andr... components/cronet/android/test/src/org/chromium/net/Http2TestServer.java:78: return getServerUrl() + "echoheader?" + headerName; Can we change the API to something like: return getServerUrl() + "echoheader?headerToEcho=" + headerName; This will simplify Http2TestHandler.getEchoHeaderResponse() logic. https://codereview.chromium.org/1412243012/diff/840001/components/cronet/andr... components/cronet/android/test/src/org/chromium/net/Http2TestServer.java:166: private Http2TestServer() {} Maybe we should move this constructor to the top closer to startHttp2TestServer() method.
Thanks for your comments and suggestions! It took me some time, but I think I've addressed all of them. PTAL, thanks! https://codereview.chromium.org/1412243012/diff/780001/components/cronet/andr... File components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamTest.java (right): https://codereview.chromium.org/1412243012/diff/780001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamTest.java:422: public void testEchoStreamStepByStep() throws Exception { On 2016/01/22 14:33:44, mef wrote: > On 2016/01/21 19:13:08, xunjieli wrote: > > Maybe add a test where a second writeData is called before the first write > > completes. Same goes to readData. Just want to make sure there can't be two > > reads or two writes in flight. Maybe also test the case where one read and one > > write are in flight. That should be okay/allowed. > > Good ideas! I'll add that. Done. https://codereview.chromium.org/1412243012/diff/800001/components/cronet/andr... File components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamTest.java (right): https://codereview.chromium.org/1412243012/diff/800001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamTest.java:668: public void testExecutorShutdown() { On 2016/01/22 16:12:18, xunjieli wrote: > Suggest renaming this test. Maybe testStreamDestroyedWhenRequestDone or sth to > make it clearer that this tests that the stream is destroyed when request is > done? This test verifies that stream is destroyed even if executor is shutdown and rejects posting tasks. https://codereview.chromium.org/1412243012/diff/800001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamTest.java:682: final ConditionVariable requestDestroyed = new ConditionVariable(false); On 2016/01/22 16:12:19, xunjieli wrote: > suggest s/requestDestroyed/streamDestroyed. Done. https://codereview.chromium.org/1412243012/diff/800001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamTest.java:710: // Clear mCronetEngine so it doesn't get shutdown second time in tearDown(). On 2016/01/22 16:12:19, xunjieli wrote: > nit: s/shutdown/shut down. shutdown happens to be exclusively a noun. Same for > line 720 and 727. Done. https://codereview.chromium.org/1412243012/diff/800001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamTest.java:736: public void testShutdown() throws Exception { On 2016/01/22 16:12:19, xunjieli wrote: > Hard to see the distinction between this and testExecutorShutdown. > Suggest adding a documentation here saying that this tests that the executor > cannot be shut down if there are active requests? Actually, executor CAN be shutdown, but CronetEngine can not. https://codereview.chromium.org/1412243012/diff/800001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamTest.java:783: public void testShutdownAfterError() throws Exception { On 2016/01/22 16:12:19, xunjieli wrote: > Suggest adding a documentation to this test. It's not clear what the use of this > test is. Done. https://codereview.chromium.org/1412243012/diff/800001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamTest.java:812: mTestFramework.mCronetEngine.shutdown(); On 2016/01/22 16:12:19, xunjieli wrote: > This test is confusing. The callback class calls shutdown(), and here shutdown() > is called again. Not sure which shutdown it is mentioned in the test name. Maybe > add some comment? Done. https://codereview.chromium.org/1412243012/diff/800001/components/cronet/andr... File components/cronet/android/test/src/org/chromium/net/Http2TestServer.java (right): https://codereview.chromium.org/1412243012/diff/800001/components/cronet/andr... components/cronet/android/test/src/org/chromium/net/Http2TestServer.java:40: final class Http2TestServer { On 2016/01/22 16:45:21, xunjieli wrote: > Suggest adding public modifiers (see a previous comment). Done. https://codereview.chromium.org/1412243012/diff/820001/components/cronet/andr... File components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java (right): https://codereview.chromium.org/1412243012/diff/820001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:379: mOnReadCompletedTask.mByteBuffer = byteBuffer; On 2016/01/22 23:19:33, kapishnikov wrote: > Can we add an assertion that "mOnReadCompletedTask.mByteBuffer == null" before > assigning the new value. The logic assumes that onReadCompleted() cannot be > called twice before postTaskToExecutor started the task execution on a different > thread; otherwise, mOnReadCompletedTask may contain invalid data. If in the > future we introduce a bug that breaks this assumption, the assert will fail. Done. https://codereview.chromium.org/1412243012/diff/820001/components/cronet/andr... File components/cronet/android/test/src/org/chromium/net/Http2TestServer.java (right): https://codereview.chromium.org/1412243012/diff/820001/components/cronet/andr... components/cronet/android/test/src/org/chromium/net/Http2TestServer.java:57: static boolean shutdownHttp2TestServer() throws Exception { On 2016/01/22 23:19:33, kapishnikov wrote: > This method does not do anything. Should we gracefully shutdown the server? Done. https://codereview.chromium.org/1412243012/diff/820001/components/cronet/andr... components/cronet/android/test/src/org/chromium/net/Http2TestServer.java:66: return "127.0.0.1"; On 2016/01/22 23:19:33, kapishnikov wrote: > We should define 127.0.0.1 as a static constant like we did with the port. Done. https://codereview.chromium.org/1412243012/diff/840001/components/cronet/andr... File components/cronet/android/cronet_bidirectional_stream_adapter.cc (right): https://codereview.chromium.org/1412243012/diff/840001/components/cronet/andr... components/cronet/android/cronet_bidirectional_stream_adapter.cc:62: // |limit| is the mthe index of the first element that should not be read or On 2016/01/22 22:22:38, xunjieli wrote: > nit: there is an extra "m". Done. https://codereview.chromium.org/1412243012/diff/840001/components/cronet/andr... components/cronet/android/cronet_bidirectional_stream_adapter.cc:64: // networking operation. On 2016/01/22 22:22:38, xunjieli wrote: > nit: s/operation/operations. Done. https://codereview.chromium.org/1412243012/diff/840001/components/cronet/andr... File components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java (right): https://codereview.chromium.org/1412243012/diff/840001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:344: } catch (Exception e) { On 2016/01/22 22:22:38, xunjieli wrote: > I think Andrei might meant that a null check is needed here. > if (mResponseInfo == null) { > // throw an exception with a descriptive message. > } prepareResponseInfoOnNetworkThread could return null without exception if native stream is destroyed, but that will be handled by isDoneLocked() check in the posted task. Am I missing something? https://codereview.chromium.org/1412243012/diff/840001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:473: headersArray[i++] = requestHeader.getValue(); On 2016/01/22 23:19:33, kapishnikov wrote: > How will 'null' header values be interpreted? From the API point of view, I > would assume that if the value is not set, it is absent and should be converted > to an empty string. I guess the C layer will treat it as invalid. BidirectionalStream.Builder.addHeader doesn't allow null header names or values. https://codereview.chromium.org/1412243012/diff/840001/components/cronet/andr... File components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamTest.java (right): https://codereview.chromium.org/1412243012/diff/840001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamTest.java:262: builder.addHeader("header:name", "headervalue"); On 2016/01/22 23:19:33, kapishnikov wrote: > Can we add couple of valid headers around the invalid one to check that the > index is calculated correctly? Done. https://codereview.chromium.org/1412243012/diff/840001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamTest.java:269: On 2016/01/22 23:19:33, kapishnikov wrote: > Can we add a method to check what happens if a header value is absent? We can > use the echo API to see what the server receives. I've added empty header value to testSimplePost, seems to work fine. Null values are checked by builder (I've added tests to testBuilderChecks). https://codereview.chromium.org/1412243012/diff/840001/components/cronet/andr... File components/cronet/android/test/src/org/chromium/net/Http2TestHandler.java (right): https://codereview.chromium.org/1412243012/diff/840001/components/cronet/andr... components/cronet/android/test/src/org/chromium/net/Http2TestHandler.java:42: static final class Builder On 2016/01/22 22:22:38, xunjieli wrote: > nit: consider adding public. Done. https://codereview.chromium.org/1412243012/diff/840001/components/cronet/andr... components/cronet/android/test/src/org/chromium/net/Http2TestHandler.java:70: cause.printStackTrace(); On 2016/01/22 23:19:34, kapishnikov wrote: > It should be better to log the exception. Actually, for test usage it is probably better to re-throw, so it crashes and burns. https://codereview.chromium.org/1412243012/diff/840001/components/cronet/andr... components/cronet/android/test/src/org/chromium/net/Http2TestHandler.java:74: private static Http2Headers responseHeadersFromRequestHeaders(Http2Headers requestHeaders) { On 2016/01/22 23:19:34, kapishnikov wrote: > Should we rename this method to something like > createEchoResponseHeadersFromRequestHeaders? Otherwise, it is not clear what the > method is doing without looking inside. Done. https://codereview.chromium.org/1412243012/diff/840001/components/cronet/andr... components/cronet/android/test/src/org/chromium/net/Http2TestHandler.java:135: encoder().writeData(ctx, streamId, data.retain(), 0, endOfStream, ctx.newPromise()); On 2016/01/22 23:19:33, kapishnikov wrote: > Do we need to retain data here? And if we do, should release it? IIUIC according to http://netty.io/wiki/reference-counted-objects.html outbound messages are released by Netty. https://codereview.chromium.org/1412243012/diff/840001/components/cronet/andr... components/cronet/android/test/src/org/chromium/net/Http2TestHandler.java:138: doSendResponse(ctx, streamId, data.retain()); On 2016/01/22 23:19:34, kapishnikov wrote: > In what scenario/tests this 'else-if' statement is going to be executed? If it is NOT echo-stream, then response is sent when end-of-stream flag is set on request. https://codereview.chromium.org/1412243012/diff/840001/components/cronet/andr... components/cronet/android/test/src/org/chromium/net/Http2TestHandler.java:156: if (path.startsWith("/echoallheaders")) { On 2016/01/22 23:19:33, kapishnikov wrote: > We should externalize all path constants like "echoallheaders" and reference > them from here and Http2TestServer.java, so they are always in sync. Done. https://codereview.chromium.org/1412243012/diff/840001/components/cronet/andr... components/cronet/android/test/src/org/chromium/net/Http2TestHandler.java:171: } On 2016/01/22 23:19:34, kapishnikov wrote: > Should we add an error response if none of the conditions are true; I assume > otherwise, the test will wait till the timeout? It would just echo last received data buffer. https://codereview.chromium.org/1412243012/diff/840001/components/cronet/andr... File components/cronet/android/test/src/org/chromium/net/Http2TestServer.java (right): https://codereview.chromium.org/1412243012/diff/840001/components/cronet/andr... components/cronet/android/test/src/org/chromium/net/Http2TestServer.java:47: static boolean startHttp2TestServer(Context context, String certFileName, String keyFileName) On 2016/01/22 22:22:38, xunjieli wrote: > consider adding public here and below. Done. https://codereview.chromium.org/1412243012/diff/840001/components/cronet/andr... components/cronet/android/test/src/org/chromium/net/Http2TestServer.java:78: return getServerUrl() + "echoheader?" + headerName; On 2016/01/22 23:19:34, kapishnikov wrote: > Can we change the API to something like: > return getServerUrl() + "echoheader?headerToEcho=" + headerName; > This will simplify Http2TestHandler.getEchoHeaderResponse() logic. Could you elaborate? I don't mind, I'm just not sure how would it simplify anything. https://codereview.chromium.org/1412243012/diff/840001/components/cronet/andr... components/cronet/android/test/src/org/chromium/net/Http2TestServer.java:166: private Http2TestServer() {} On 2016/01/22 23:19:34, kapishnikov wrote: > Maybe we should move this constructor to the top closer to > startHttp2TestServer() method. Done.
https://codereview.chromium.org/1412243012/diff/840001/components/cronet/andr... File components/cronet/android/test/src/org/chromium/net/Http2TestHandler.java (right): https://codereview.chromium.org/1412243012/diff/840001/components/cronet/andr... components/cronet/android/test/src/org/chromium/net/Http2TestHandler.java:70: cause.printStackTrace(); On 2016/01/25 18:11:26, mef wrote: > On 2016/01/22 23:19:34, kapishnikov wrote: > > It should be better to log the exception. > > Actually, for test usage it is probably better to re-throw, so it crashes and > burns. I agree. I was referring that we should use |sLogger| instead of cause.printStackTrace(). https://codereview.chromium.org/1412243012/diff/840001/components/cronet/andr... File components/cronet/android/test/src/org/chromium/net/Http2TestServer.java (right): https://codereview.chromium.org/1412243012/diff/840001/components/cronet/andr... components/cronet/android/test/src/org/chromium/net/Http2TestServer.java:78: return getServerUrl() + "echoheader?" + headerName; On 2016/01/25 18:11:26, mef wrote: > On 2016/01/22 23:19:34, kapishnikov wrote: > > Can we change the API to something like: > > return getServerUrl() + "echoheader?headerToEcho=" + headerName; > > This will simplify Http2TestHandler.getEchoHeaderResponse() logic. > > Could you elaborate? I don't mind, I'm just not sure how would it simplify > anything. I am okay with the current implementation but I was thinking something like this: private static String getEchoHeaderResponse(Http2Headers headers) { CharSequence headerName = headers.get("header_to_echo"); if (headerName != null) { CharSequence headerValue = headers.get(headerName); return headerValue != null ? headerValue.toString() : "Header not found: " + headerName; } else { return "Specify the header to echo"; } } Here we don't need to parse the headers ourselves and the header_to_echo header can be anywhere in the request, i.e. it should not necessarily be the first header. The current implementation has: String[] split_path = headers.path().toString().split("\\?"); if (split_path.length <= 1) return "Header name not found."; Btw, the method uses local variable names with underscores instead of lowerCamelCase: https://engdoc.corp.google.com/eng/doc/devguide/java/styleguide.shtml?cl=head...
https://codereview.chromium.org/1412243012/diff/840001/components/cronet/andr... File components/cronet/android/test/src/org/chromium/net/Http2TestHandler.java (right): https://codereview.chromium.org/1412243012/diff/840001/components/cronet/andr... components/cronet/android/test/src/org/chromium/net/Http2TestHandler.java:70: cause.printStackTrace(); On 2016/01/25 19:47:09, kapishnikov wrote: > On 2016/01/25 18:11:26, mef wrote: > > On 2016/01/22 23:19:34, kapishnikov wrote: > > > It should be better to log the exception. > > > > Actually, for test usage it is probably better to re-throw, so it crashes and > > burns. > > I agree. I was referring that we should use |sLogger| instead of > cause.printStackTrace(). Hrm, |sLogger| is Http2FrameLogger, not a general logger. https://codereview.chromium.org/1412243012/diff/840001/components/cronet/andr... File components/cronet/android/test/src/org/chromium/net/Http2TestServer.java (right): https://codereview.chromium.org/1412243012/diff/840001/components/cronet/andr... components/cronet/android/test/src/org/chromium/net/Http2TestServer.java:78: return getServerUrl() + "echoheader?" + headerName; On 2016/01/25 19:47:09, kapishnikov wrote: > On 2016/01/25 18:11:26, mef wrote: > > On 2016/01/22 23:19:34, kapishnikov wrote: > > > Can we change the API to something like: > > > return getServerUrl() + "echoheader?headerToEcho=" + headerName; > > > This will simplify Http2TestHandler.getEchoHeaderResponse() logic. > > > > Could you elaborate? I don't mind, I'm just not sure how would it simplify > > anything. > > I am okay with the current implementation but I was thinking something like > this: > > private static String getEchoHeaderResponse(Http2Headers headers) { > CharSequence headerName = headers.get("header_to_echo"); That would require passing header name in pre-defined header, not as part of url. > if (headerName != null) { > CharSequence headerValue = headers.get(headerName); > return headerValue != null ? headerValue.toString() : "Header not found: > " + headerName; > } else { > return "Specify the header to echo"; > } > } > > Here we don't need to parse the headers ourselves and the header_to_echo header > can be anywhere in the request, i.e. it should not necessarily be the first > header. > > The current implementation has: > String[] split_path = headers.path().toString().split("\\?"); > if (split_path.length <= 1) return "Header name not found."; > > Btw, the method uses local variable names with underscores instead of > lowerCamelCase: > https://engdoc.corp.google.com/eng/doc/devguide/java/styleguide.shtml?cl=head... > Oops, Done.
https://codereview.chromium.org/1412243012/diff/840001/components/cronet/andr... File components/cronet/android/test/src/org/chromium/net/Http2TestHandler.java (right): https://codereview.chromium.org/1412243012/diff/840001/components/cronet/andr... components/cronet/android/test/src/org/chromium/net/Http2TestHandler.java:70: cause.printStackTrace(); On 2016/01/25 22:31:39, mef wrote: > On 2016/01/25 19:47:09, kapishnikov wrote: > > On 2016/01/25 18:11:26, mef wrote: > > > On 2016/01/22 23:19:34, kapishnikov wrote: > > > > It should be better to log the exception. > > > > > > Actually, for test usage it is probably better to re-throw, so it crashes > and > > > burns. > > > > I agree. I was referring that we should use |sLogger| instead of > > cause.printStackTrace(). > > Hrm, |sLogger| is Http2FrameLogger, not a general logger. I think the exception will be logged anyways but instead of cause.printStackTrace(); we can call: android.util.Log.e("Http2TestHandler", "An exception was caught", cause);
Thanks, PTAL. https://codereview.chromium.org/1412243012/diff/840001/components/cronet/andr... File components/cronet/android/test/src/org/chromium/net/Http2TestHandler.java (right): https://codereview.chromium.org/1412243012/diff/840001/components/cronet/andr... components/cronet/android/test/src/org/chromium/net/Http2TestHandler.java:70: cause.printStackTrace(); On 2016/01/26 00:00:25, kapishnikov wrote: > On 2016/01/25 22:31:39, mef wrote: > > On 2016/01/25 19:47:09, kapishnikov wrote: > > > On 2016/01/25 18:11:26, mef wrote: > > > > On 2016/01/22 23:19:34, kapishnikov wrote: > > > > > It should be better to log the exception. > > > > > > > > Actually, for test usage it is probably better to re-throw, so it crashes > > and > > > > burns. > > > > > > I agree. I was referring that we should use |sLogger| instead of > > > cause.printStackTrace(). > > > > Hrm, |sLogger| is Http2FrameLogger, not a general logger. > > I think the exception will be logged anyways but > instead of > cause.printStackTrace(); > we can call: > android.util.Log.e("Http2TestHandler", "An exception was caught", cause); > > Done.
https://codereview.chromium.org/1412243012/diff/900001/components/cronet/andr... File components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamTest.java (right): https://codereview.chromium.org/1412243012/diff/900001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamTest.java:448: new BidirectionalStream.Builder(url, callback, callback.getBlockingDirectExecutor(), I am not sure that executing the callbacks on the network thread fully exercises our API contract, since processing the OnWrite callback means pausing the network thread (What we really want is to pause the executor thread, but keep calling Write from the Java test thread which will then successfully post the write task to the network thread). Can we use the executor that you made, but instead running the task on current thread (which is the network thread), and make the task run another thread? (like creating a new thread and stored it in mExecutorThread, and execute the runnable on mExecutorThread?) https://codereview.chromium.org/1412243012/diff/900001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamTest.java:458: // Second write may be allowed because onWriteCompleted callback is posted after state This doesn't seem very intuitive. We should be able to report the error on the second write.. https://codereview.chromium.org/1412243012/diff/900001/components/cronet/andr... File components/cronet/android/test/javatests/src/org/chromium/net/TestBidirectionalStreamCallback.java (right): https://codereview.chromium.org/1412243012/diff/900001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/TestBidirectionalStreamCallback.java:330: * Returns {@code false} if the listener should continue to advance the nit: there is no longer a listener.
https://codereview.chromium.org/1412243012/diff/900001/components/cronet/andr... File components/cronet/android/cronet_bidirectional_stream_adapter.cc (right): https://codereview.chromium.org/1412243012/diff/900001/components/cronet/andr... components/cronet/android/cronet_bidirectional_stream_adapter.cc:19: #include "net/cert/cert_status_flags.h" nit: not used. https://codereview.chromium.org/1412243012/diff/900001/components/cronet/andr... File components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamTest.java (right): https://codereview.chromium.org/1412243012/diff/900001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamTest.java:52: private TestBidirectionalStreamCallback startAndWaitForComplete(String url) throws Exception { Only used in two tests. Don't think we will use in more tests? Suggest inline. Easier to see that way. https://codereview.chromium.org/1412243012/diff/900001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamTest.java:78: private static String makeLongString(String base, int repetition) { nit: consider s/make/create to be consistent with other method names. https://codereview.chromium.org/1412243012/diff/900001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamTest.java:97: return unknown; nit: I am not sure why the variable is called "unknown." Maybe simply name it as urlRequestInfo. The method is straightforward, but the variable name made me think twice. https://codereview.chromium.org/1412243012/diff/900001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamTest.java:229: public void testSetHttpMethod() throws Exception { Suggest naming this test as testSimplePut. And move the setHttpMethod(null) part to testBuilderChecks. https://codereview.chromium.org/1412243012/diff/900001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamTest.java:366: assertEquals(headerValue, callback.mTrailers.getAsMap().get("echo-header-name").get(0)); I am not sure what "echo-header-name" came from, I was expectating |headerValue|. Might use a comment. https://codereview.chromium.org/1412243012/diff/900001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamTest.java:390: public void testEchoStream() throws Exception { This test seems to be a combination of some other tests. Is it redundant and same as testSimplePost? https://codereview.chromium.org/1412243012/diff/900001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamTest.java:770: // Test that stream is destroyed even if executor is shutdown and rejects posting tasks. nit: s/shutdown/shut down https://codereview.chromium.org/1412243012/diff/900001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamTest.java:853: assertEquals("Cannot shutdown with active requests.", e.getMessage()); nit: s/shutdown/shut down, or "Cannot call shutdown". https://codereview.chromium.org/1412243012/diff/900001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamTest.java:923: stream.cancel(); testCronetEngineShutdownAfterStreamFailure and testCronetEngineShutdownAfterStreamCancel are hard to reason about. I guess your goal here is to make sure that when the active stream is failed or canceled, we can shut down the engine. However, the shutdown logic is buried in the onFailed and onCanceled callback, so it is hard to see. Suggest adding a AssertNull(mTestFramework.mCronetEngine) at the end of these two tests to make sure the corresponding callback is invoked. https://codereview.chromium.org/1412243012/diff/900001/components/cronet/andr... File components/cronet/android/test/javatests/src/org/chromium/net/TestBidirectionalStreamCallback.java (right): https://codereview.chromium.org/1412243012/diff/900001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/TestBidirectionalStreamCallback.java:26: class TestBidirectionalStreamCallback extends BidirectionalStream.Callback { nit: add public? https://codereview.chromium.org/1412243012/diff/900001/components/cronet/andr... File components/cronet/android/test/src/org/chromium/net/Http2TestHandler.java (right): https://codereview.chromium.org/1412243012/diff/900001/components/cronet/andr... components/cronet/android/test/src/org/chromium/net/Http2TestHandler.java:37: // Some Url Paths have special meaning. nit: s/have/that. https://codereview.chromium.org/1412243012/diff/900001/components/cronet/andr... components/cronet/android/test/src/org/chromium/net/Http2TestHandler.java:52: static final class Builder nit: public? https://codereview.chromium.org/1412243012/diff/900001/components/cronet/andr... components/cronet/android/test/src/org/chromium/net/Http2TestHandler.java:132: private void doEchoTrailers(ChannelHandlerContext ctx, int streamId, Http2Headers headers, hmm. |header| from the args is not used. Should it be used? https://codereview.chromium.org/1412243012/diff/900001/components/cronet/andr... File components/cronet/android/test/src/org/chromium/net/Http2TestServer.java (right): https://codereview.chromium.org/1412243012/diff/900001/components/cronet/andr... components/cronet/android/test/src/org/chromium/net/Http2TestServer.java:82: public static String getEchoStreamUrl() { nit: need docmentation. https://codereview.chromium.org/1412243012/diff/900001/components/cronet/andr... components/cronet/android/test/src/org/chromium/net/Http2TestServer.java:86: public static String getEchoTrailersUrl() { nit: need documentation to explain what this does. Does it echo the request headers as trailers?
Thanks, Helen! PTAL. https://codereview.chromium.org/1412243012/diff/900001/components/cronet/andr... File components/cronet/android/cronet_bidirectional_stream_adapter.cc (right): https://codereview.chromium.org/1412243012/diff/900001/components/cronet/andr... components/cronet/android/cronet_bidirectional_stream_adapter.cc:19: #include "net/cert/cert_status_flags.h" On 2016/01/27 16:16:38, xunjieli wrote: > nit: not used. Done. https://codereview.chromium.org/1412243012/diff/900001/components/cronet/andr... File components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamTest.java (right): https://codereview.chromium.org/1412243012/diff/900001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamTest.java:52: private TestBidirectionalStreamCallback startAndWaitForComplete(String url) throws Exception { On 2016/01/27 16:16:38, xunjieli wrote: > Only used in two tests. Don't think we will use in more tests? Suggest inline. > Easier to see that way. Done. https://codereview.chromium.org/1412243012/diff/900001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamTest.java:78: private static String makeLongString(String base, int repetition) { On 2016/01/27 16:16:38, xunjieli wrote: > nit: consider s/make/create to be consistent with other method names. Done. https://codereview.chromium.org/1412243012/diff/900001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamTest.java:97: return unknown; On 2016/01/27 16:16:38, xunjieli wrote: > nit: I am not sure why the variable is called "unknown." Maybe simply name it as > urlRequestInfo. The method is straightforward, but the variable name made me > think twice. Done. https://codereview.chromium.org/1412243012/diff/900001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamTest.java:229: public void testSetHttpMethod() throws Exception { On 2016/01/27 16:16:38, xunjieli wrote: > Suggest naming this test as testSimplePut. And move the setHttpMethod(null) part > to testBuilderChecks. Done. https://codereview.chromium.org/1412243012/diff/900001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamTest.java:366: assertEquals(headerValue, callback.mTrailers.getAsMap().get("echo-header-name").get(0)); On 2016/01/27 16:16:38, xunjieli wrote: > I am not sure what "echo-header-name" came from, I was expectating > |headerValue|. Might use a comment. Done. https://codereview.chromium.org/1412243012/diff/900001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamTest.java:390: public void testEchoStream() throws Exception { On 2016/01/27 16:16:38, xunjieli wrote: > This test seems to be a combination of some other tests. Is it redundant and > same as testSimplePost? This was THE original test of bidirectional streaming. Among other things it is useful for verifying that data split between multiple frames is sent / received properly. https://codereview.chromium.org/1412243012/diff/900001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamTest.java:448: new BidirectionalStream.Builder(url, callback, callback.getBlockingDirectExecutor(), On 2016/01/26 21:41:10, xunjieli wrote: > I am not sure that executing the callbacks on the network thread fully exercises > our API contract, since processing the OnWrite callback means pausing the > network thread (What we really want is to pause the executor thread, but keep > calling Write from the Java test thread which will then successfully post the > write task to the network thread). Can we use the executor that you made, but > instead running the task on current thread (which is the network thread), and > make the task run another thread? (like creating a new thread and stored it in > mExecutorThread, and execute the runnable on mExecutorThread?) Done. Initially I've used direct executor to prevent callback posted on another thread from switching state to 'waiting_for_write'. Your comment made me realize that calling read or write second time from the callback invoked on executor thread will be guaranteed to throw because even if first operation completes on network thread, the onComplete callback is posted to the executor and will not get invoked until completion of previous callback task. https://codereview.chromium.org/1412243012/diff/900001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamTest.java:458: // Second write may be allowed because onWriteCompleted callback is posted after state On 2016/01/26 21:41:10, xunjieli wrote: > This doesn't seem very intuitive. We should be able to report the error on the > second write.. Done. https://codereview.chromium.org/1412243012/diff/900001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamTest.java:770: // Test that stream is destroyed even if executor is shutdown and rejects posting tasks. On 2016/01/27 16:16:38, xunjieli wrote: > nit: s/shutdown/shut down Done. https://codereview.chromium.org/1412243012/diff/900001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamTest.java:853: assertEquals("Cannot shutdown with active requests.", e.getMessage()); On 2016/01/27 16:16:38, xunjieli wrote: > nit: s/shutdown/shut down, or "Cannot call shutdown". That's the exception message, untouched by this CL. https://codereview.chromium.org/1412243012/diff/900001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamTest.java:923: stream.cancel(); On 2016/01/27 16:16:38, xunjieli wrote: > testCronetEngineShutdownAfterStreamFailure and > testCronetEngineShutdownAfterStreamCancel are hard to reason about. I guess your > goal here is to make sure that when the active stream is failed or canceled, we > can shut down the engine. However, the shutdown logic is buried in the onFailed > and onCanceled callback, so it is hard to see. > > Suggest adding a AssertNull(mTestFramework.mCronetEngine) at the end of these > two tests to make sure the corresponding callback is invoked. Done. https://codereview.chromium.org/1412243012/diff/900001/components/cronet/andr... File components/cronet/android/test/javatests/src/org/chromium/net/TestBidirectionalStreamCallback.java (right): https://codereview.chromium.org/1412243012/diff/900001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/TestBidirectionalStreamCallback.java:26: class TestBidirectionalStreamCallback extends BidirectionalStream.Callback { On 2016/01/27 16:16:38, xunjieli wrote: > nit: add public? Done. https://codereview.chromium.org/1412243012/diff/900001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/TestBidirectionalStreamCallback.java:330: * Returns {@code false} if the listener should continue to advance the On 2016/01/26 21:41:10, xunjieli wrote: > nit: there is no longer a listener. Done. https://codereview.chromium.org/1412243012/diff/900001/components/cronet/andr... File components/cronet/android/test/src/org/chromium/net/Http2TestHandler.java (right): https://codereview.chromium.org/1412243012/diff/900001/components/cronet/andr... components/cronet/android/test/src/org/chromium/net/Http2TestHandler.java:37: // Some Url Paths have special meaning. On 2016/01/27 16:16:38, xunjieli wrote: > nit: s/have/that. Done. https://codereview.chromium.org/1412243012/diff/900001/components/cronet/andr... components/cronet/android/test/src/org/chromium/net/Http2TestHandler.java:52: static final class Builder On 2016/01/27 16:16:38, xunjieli wrote: > nit: public? Done. https://codereview.chromium.org/1412243012/diff/900001/components/cronet/andr... components/cronet/android/test/src/org/chromium/net/Http2TestHandler.java:132: private void doEchoTrailers(ChannelHandlerContext ctx, int streamId, Http2Headers headers, On 2016/01/27 16:16:38, xunjieli wrote: > hmm. |header| from the args is not used. Should it be used? Done. https://codereview.chromium.org/1412243012/diff/900001/components/cronet/andr... File components/cronet/android/test/src/org/chromium/net/Http2TestServer.java (right): https://codereview.chromium.org/1412243012/diff/900001/components/cronet/andr... components/cronet/android/test/src/org/chromium/net/Http2TestServer.java:82: public static String getEchoStreamUrl() { On 2016/01/27 16:16:38, xunjieli wrote: > nit: need docmentation. Done. https://codereview.chromium.org/1412243012/diff/900001/components/cronet/andr... components/cronet/android/test/src/org/chromium/net/Http2TestServer.java:86: public static String getEchoTrailersUrl() { On 2016/01/27 16:16:38, xunjieli wrote: > nit: need documentation to explain what this does. Does it echo the request > headers as trailers? Done.
I tried to run GN build to test Netty and found the following issues: https://codereview.chromium.org/1412243012/diff/920001/components/cronet/andr... File components/cronet/android/BUILD.gn (right): https://codereview.chromium.org/1412243012/diff/920001/components/cronet/andr... components/cronet/android/BUILD.gn:162: if (enable_bidirectional_stream) { We should define the default value for |enable_bidirectional_stream|. Otherwise, it is necessary to provide the value explicitly as an argument whenever Cronet is built. https://codereview.chromium.org/1412243012/diff/920001/components/cronet/andr... components/cronet/android/BUILD.gn:438: sources += [ Here should be 'java_files' instead of 'sources'. https://codereview.chromium.org/1412243012/diff/920001/components/cronet/andr... components/cronet/android/BUILD.gn:548: sources += [ Same here: 'java_files' instead of 'sources'.
https://codereview.chromium.org/1412243012/diff/920001/components/cronet/andr... File components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamTest.java (right): https://codereview.chromium.org/1412243012/diff/920001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamTest.java:508: } Can we have a test where we call .read() when there is an inflight write()? Just a sanity check to make sure that one of each works. Can we have a test where we call read after two writes? In that case, I'd expect that the read would return data from two data frames instead of one. https://codereview.chromium.org/1412243012/diff/920001/components/cronet/andr... File components/cronet/android/test/src/org/chromium/net/Http2TestHandler.java (right): https://codereview.chromium.org/1412243012/diff/920001/components/cronet/andr... components/cronet/android/test/src/org/chromium/net/Http2TestHandler.java:49: private boolean mEchoStream; Seems like this handler can only handle one active stream at a time. Is this a known limitation? If we are done with the first active stream, I am guessing the second request will work. Is this right? https://codereview.chromium.org/1412243012/diff/920001/components/cronet/andr... components/cronet/android/test/src/org/chromium/net/Http2TestHandler.java:127: private void doEchoStream(ChannelHandlerContext ctx, int streamId, Http2Headers headers, This method is confusingly named. I was expecting it to be used on line 149 too. Maybe generalize it so it can be used for both header and data frame? https://codereview.chromium.org/1412243012/diff/920001/components/cronet/andr... components/cronet/android/test/src/org/chromium/net/Http2TestHandler.java:149: encoder().writeData(ctx, streamId, data.retain(), 0, endOfStream, ctx.newPromise()); use doEchoStream? see previous comment.
https://codereview.chromium.org/1412243012/diff/920001/components/cronet/andr... File components/cronet/android/test/src/org/chromium/net/Http2TestHandler.java (right): https://codereview.chromium.org/1412243012/diff/920001/components/cronet/andr... components/cronet/android/test/src/org/chromium/net/Http2TestHandler.java:49: private boolean mEchoStream; On 2016/01/27 22:17:10, xunjieli wrote: > Seems like this handler can only handle one active stream at a time. Is this a > known limitation? If we are done with the first active stream, I am guessing the > second request will work. Is this right? Also bug here. If first url is echo stream, and the second url is some other, mEchoStream will still be true! Similar issue with mResponseHeaders below.
This is the change needed in the cronet gn script to build the CL successfully
with Netty:
diff --git a/components/cronet/android/BUILD.gn
b/components/cronet/android/BUILD.gn
index 59d075c..a0e138c 100644
--- a/components/cronet/android/BUILD.gn
+++ b/components/cronet/android/BUILD.gn
@@ -432,10 +432,11 @@ android_library("cronet_test_apk_java") {
":cronet_java",
"//base:base_java",
"//net/android:net_java_test_support",
+ "//third_party/netty4:netty_all",
]
if (enable_bidirectional_stream) {
- sources += [
+ java_files += [
"test/src/org/chromium/net/Http2TestHandler.java",
"test/src/org/chromium/net/Http2TestServer.java",
]
@@ -492,7 +493,6 @@ android_apk("cronet_test_apk") {
":cronet_tests",
"//base:base_java",
"//third_party/netty-tcnative:netty-tcnative_all",
- "//third_party/netty4:netty_all",
]
run_findbugs = true
@@ -548,7 +548,7 @@ instrumentation_test_apk("cronet_test_instrumentation_apk")
{
run_findbugs = true
if (enable_bidirectional_stream) {
- sources += [
+ java_files += [
"test/javatests/src/org/chromium/net/BidirectionalStreamTest.java",
"test/javatests/src/org/chromium/net/TestBidirectionalStreamCallback.java",
]
Patchset #47 (id:960001) has been deleted
https://codereview.chromium.org/1412243012/diff/920001/components/cronet/andr... File components/cronet/android/BUILD.gn (right): https://codereview.chromium.org/1412243012/diff/920001/components/cronet/andr... components/cronet/android/BUILD.gn:162: if (enable_bidirectional_stream) { On 2016/01/27 21:50:29, kapishnikov wrote: > We should define the default value for |enable_bidirectional_stream|. Otherwise, > it is necessary to provide the value explicitly as an argument whenever Cronet > is built. Done. https://codereview.chromium.org/1412243012/diff/920001/components/cronet/andr... components/cronet/android/BUILD.gn:438: sources += [ On 2016/01/27 21:50:29, kapishnikov wrote: > Here should be 'java_files' instead of 'sources'. Done. https://codereview.chromium.org/1412243012/diff/920001/components/cronet/andr... components/cronet/android/BUILD.gn:548: sources += [ On 2016/01/27 21:50:29, kapishnikov wrote: > Same here: 'java_files' instead of 'sources'. Done. https://codereview.chromium.org/1412243012/diff/920001/components/cronet/andr... File components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamTest.java (right): https://codereview.chromium.org/1412243012/diff/920001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamTest.java:508: } On 2016/01/27 22:17:10, xunjieli wrote: > Can we have a test where we call .read() when there is an inflight write()? Just > a sanity check to make sure that one of each works. Done. > Can we have a test where we call read after two writes? In that case, I'd expect > that the read would return data from two data frames instead of one. This is a bit tricky, as I'm not sure how to guarantee what data is buffered by the time the read is called. I've added a test that does a few writes before calling read, would that work? https://codereview.chromium.org/1412243012/diff/920001/components/cronet/andr... File components/cronet/android/test/src/org/chromium/net/Http2TestHandler.java (right): https://codereview.chromium.org/1412243012/diff/920001/components/cronet/andr... components/cronet/android/test/src/org/chromium/net/Http2TestHandler.java:49: private boolean mEchoStream; On 2016/01/27 22:17:10, xunjieli wrote: > Seems like this handler can only handle one active stream at a time. Is this a > known limitation? If we are done with the first active stream, I am guessing the > second request will work. Is this right? Done. https://codereview.chromium.org/1412243012/diff/920001/components/cronet/andr... components/cronet/android/test/src/org/chromium/net/Http2TestHandler.java:49: private boolean mEchoStream; On 2016/01/27 22:23:24, xunjieli wrote: > On 2016/01/27 22:17:10, xunjieli wrote: > > Seems like this handler can only handle one active stream at a time. Is this a > > known limitation? If we are done with the first active stream, I am guessing > the > > second request will work. Is this right? > > Also bug here. If first url is echo stream, and the second url is some other, > mEchoStream will still be true! Similar issue with mResponseHeaders below. Great catch! Currently there are no tests that use multiple requests in parallel, but yeah. I've refactored the handler to introduce RequestResponders keyed by streamId. https://codereview.chromium.org/1412243012/diff/920001/components/cronet/andr... components/cronet/android/test/src/org/chromium/net/Http2TestHandler.java:127: private void doEchoStream(ChannelHandlerContext ctx, int streamId, Http2Headers headers, On 2016/01/27 22:17:10, xunjieli wrote: > This method is confusingly named. I was expecting it to be used on line 149 too. > Maybe generalize it so it can be used for both header and data frame? Done. https://codereview.chromium.org/1412243012/diff/920001/components/cronet/andr... components/cronet/android/test/src/org/chromium/net/Http2TestHandler.java:149: encoder().writeData(ctx, streamId, data.retain(), 0, endOfStream, ctx.newPromise()); On 2016/01/27 22:17:10, xunjieli wrote: > use doEchoStream? see previous comment. Done.
Looks good! Last comment before I sign off :) https://codereview.chromium.org/1412243012/diff/1000001/components/cronet/and... File components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamTest.java (right): https://codereview.chromium.org/1412243012/diff/1000001/components/cronet/and... components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamTest.java:573: assertEquals("", callback.mResponseAsString); This is a bit surprising. Will this be flaky if the data *is* buffered when calling read? How about calling read, and check if the callback returned any data? If there isn't any data, continue calling read and check?
https://codereview.chromium.org/1412243012/diff/1000001/components/cronet/and... File components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamTest.java (right): https://codereview.chromium.org/1412243012/diff/1000001/components/cronet/and... components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamTest.java:573: assertEquals("", callback.mResponseAsString); On 2016/01/29 15:08:04, xunjieli wrote: > This is a bit surprising. Will this be flaky if the data *is* buffered when > calling read? > > How about calling read, and check if the callback returned any data? If there > isn't any data, continue calling read and check? That's the point. waitForNextReadStep() just waits for stream to get into WAITING_FOR_READ state, but doesn't issue any reads yet, so the "mResponseAsString" is still empty. I've added another startNextRead() to verify that very first read gets some data.
lgtm!
https://codereview.chromium.org/1412243012/diff/1000001/components/cronet/and... File components/cronet/android/api/src/org/chromium/net/BidirectionalStream.java (right): https://codereview.chromium.org/1412243012/diff/1000001/components/cronet/and... components/cronet/android/api/src/org/chromium/net/BidirectionalStream.java:311: * BidirectionalStream.Builder#BidirectionalStream.Builder BidirectionalStream.Builder()}. Should be BidirectionalStream.Builder#Builder instead of BidirectionalStream.Builder#BidirectionalStream.Builder. https://codereview.chromium.org/1412243012/diff/1000001/components/cronet/and... File components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java (right): https://codereview.chromium.org/1412243012/diff/1000001/components/cronet/and... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:135: if (maybeSucceedLocked()) { I think the API would be friendlier if we always signal the client that the end of stream reached, regardless whether State.WRITING_DONE is set or not. This would allow the client to place some logic related to end of reading in one place knowing that it will be reliably called. https://codereview.chromium.org/1412243012/diff/1000001/components/cronet/and... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:167: if (maybeSucceedLocked()) { Same here. I think that every write() should be followed by onWriteCompleted() or onFailed() regardless whether the read stream is closed or not. This would also simplify the javadoc of write() method. https://codereview.chromium.org/1412243012/diff/1000001/components/cronet/and... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:436: "Exception in BidirectionalStream: " + errorString, nativeError)); The constructor of CronetException ignores the passed |nativeError| and defaults it to 0. https://codereview.chromium.org/1412243012/diff/1000001/components/cronet/and... File components/cronet/android/test/javatests/src/org/chromium/net/TestBidirectionalStreamCallback.java (right): https://codereview.chromium.org/1412243012/diff/1000001/components/cronet/and... components/cronet/android/test/javatests/src/org/chromium/net/TestBidirectionalStreamCallback.java:167: public void onReadCompleted( Do we have any test that does partial read and reuses the existing buffer? E.g., the read buffer contains 10 bytes. onReadCompleted() reads 5 bytes and calls stream.read() with the original buffer. https://codereview.chromium.org/1412243012/diff/1000001/components/cronet/and... components/cronet/android/test/javatests/src/org/chromium/net/TestBidirectionalStreamCallback.java:182: lastDataReceivedAsBytes = new byte[bytesRead]; Since |lastDataReceivedAsBytes| is final it is better to keep the declaration and the value assignment together, i.e. replace it with final byte[] lastDataReceivedAsBytes = new byte[bytesRead]; https://codereview.chromium.org/1412243012/diff/1000001/components/cronet/and... components/cronet/android/test/javatests/src/org/chromium/net/TestBidirectionalStreamCallback.java:246: public void onFailed(BidirectionalStream stream, UrlResponseInfo info, CronetException error) { Can we add a test that checks that the callback is called with an error that contains the correct error code? I suspect that its value is always 0. https://codereview.chromium.org/1412243012/diff/1000001/components/cronet/and... components/cronet/android/test/javatests/src/org/chromium/net/TestBidirectionalStreamCallback.java:264: public void onCanceled(BidirectionalStream stream, UrlResponseInfo info) { Do we have any test that checks that onCanceled() is ever called?
On 2016/01/29 20:08:58, kapishnikov wrote: > https://codereview.chromium.org/1412243012/diff/1000001/components/cronet/and... > components/cronet/android/test/javatests/src/org/chromium/net/TestBidirectionalStreamCallback.java:264: > public void onCanceled(BidirectionalStream stream, UrlResponseInfo info) { > Do we have any test that checks that onCanceled() is ever called? Found it in BidirectionalStreamTest, sorry.
Helen, thanks! Andrei, PTAL! https://codereview.chromium.org/1412243012/diff/1000001/components/cronet/and... File components/cronet/android/api/src/org/chromium/net/BidirectionalStream.java (right): https://codereview.chromium.org/1412243012/diff/1000001/components/cronet/and... components/cronet/android/api/src/org/chromium/net/BidirectionalStream.java:311: * BidirectionalStream.Builder#BidirectionalStream.Builder BidirectionalStream.Builder()}. On 2016/01/29 20:08:58, kapishnikov wrote: > Should be BidirectionalStream.Builder#Builder instead of > BidirectionalStream.Builder#BidirectionalStream.Builder. Done. https://codereview.chromium.org/1412243012/diff/1000001/components/cronet/and... File components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java (right): https://codereview.chromium.org/1412243012/diff/1000001/components/cronet/and... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:135: if (maybeSucceedLocked()) { On 2016/01/29 20:08:58, kapishnikov wrote: > I think the API would be friendlier if we always signal the client that the end > of stream reached, regardless whether State.WRITING_DONE is set or not. This > would allow the client to place some logic related to end of reading in one > place knowing that it will be reliably called. Acknowledged. We should consider that, currently end of reading is signaled to the client by callbacks to onReadCompleted with 0 bytes read. https://codereview.chromium.org/1412243012/diff/1000001/components/cronet/and... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:167: if (maybeSucceedLocked()) { On 2016/01/29 20:08:58, kapishnikov wrote: > Same here. I think that every write() should be followed by onWriteCompleted() > or onFailed() regardless whether the read stream is closed or not. This would > also simplify the javadoc of write() method. Acknowledged. https://codereview.chromium.org/1412243012/diff/1000001/components/cronet/and... components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:436: "Exception in BidirectionalStream: " + errorString, nativeError)); On 2016/01/29 20:08:58, kapishnikov wrote: > The constructor of CronetException ignores the passed |nativeError| and defaults > it to 0. Whoa! Good catch, Done. https://codereview.chromium.org/1412243012/diff/1000001/components/cronet/and... File components/cronet/android/test/javatests/src/org/chromium/net/TestBidirectionalStreamCallback.java (right): https://codereview.chromium.org/1412243012/diff/1000001/components/cronet/and... components/cronet/android/test/javatests/src/org/chromium/net/TestBidirectionalStreamCallback.java:167: public void onReadCompleted( On 2016/01/29 20:08:58, kapishnikov wrote: > Do we have any test that does partial read and reuses the existing buffer? E.g., > the read buffer contains 10 bytes. onReadCompleted() reads 5 bytes and calls > stream.read() with the original buffer. testSimpleGetBufferUpdates() https://codereview.chromium.org/1412243012/diff/1000001/components/cronet/and... components/cronet/android/test/javatests/src/org/chromium/net/TestBidirectionalStreamCallback.java:182: lastDataReceivedAsBytes = new byte[bytesRead]; On 2016/01/29 20:08:58, kapishnikov wrote: > Since |lastDataReceivedAsBytes| is final it is better to keep the declaration > and the value assignment together, i.e. replace it with > final byte[] lastDataReceivedAsBytes = new byte[bytesRead]; Done. https://codereview.chromium.org/1412243012/diff/1000001/components/cronet/and... components/cronet/android/test/javatests/src/org/chromium/net/TestBidirectionalStreamCallback.java:246: public void onFailed(BidirectionalStream stream, UrlResponseInfo info, CronetException error) { On 2016/01/29 20:08:58, kapishnikov wrote: > Can we add a test that checks that the callback is called with an error that > contains the correct error code? I suspect that its value is always 0. Good point, done, see testFailPlainHttp(). https://codereview.chromium.org/1412243012/diff/1000001/components/cronet/and... components/cronet/android/test/javatests/src/org/chromium/net/TestBidirectionalStreamCallback.java:264: public void onCanceled(BidirectionalStream stream, UrlResponseInfo info) { On 2016/01/29 20:08:58, kapishnikov wrote: > Do we have any test that checks that onCanceled() is ever called? testFailures() -> throwOrCancel(FailureType.CANCEL_SYNC, ResponseStep.ON_REQUEST_HEADERS_SENT, false, false); -> assertEquals(failureType == FailureType.CANCEL_SYNC || failureType == FailureType.CANCEL_ASYNC || failureType == FailureType.CANCEL_ASYNC_WITHOUT_PAUSE, callback.mOnCanceledCalled);
lgtm
The CQ bit was checked by mef@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pauljensen@chromium.org, xunjieli@chromium.org, kapishnikov@chromium.org Link to the patchset: https://codereview.chromium.org/1412243012/#ps1060001 (title: "Sync")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1412243012/1060001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1412243012/1060001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_chromium_gn_compile_rel on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...)
The CQ bit was checked by mef@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pauljensen@chromium.org, kapishnikov@chromium.org, xunjieli@chromium.org Link to the patchset: https://codereview.chromium.org/1412243012/#ps1080001 (title: "Fix javadoc link.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1412243012/1080001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1412243012/1080001
Message was sent while issue was closed.
Committed patchset #52 (id:1080001)
Message was sent while issue was closed.
Description was changed from ========== Initial implementation of CronetBidirectionalStream. BUG=516342 ========== to ========== Initial implementation of CronetBidirectionalStream. BUG=516342 Committed: https://crrev.com/72e10340f0078cc85d10ae9654cec3e91b46b128 Cr-Commit-Position: refs/heads/master@{#372486} ==========
Message was sent while issue was closed.
Patchset 52 (id:??) landed as https://crrev.com/72e10340f0078cc85d10ae9654cec3e91b46b128 Cr-Commit-Position: refs/heads/master@{#372486} |
