Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1492583002:
Add HttpUrlConnection backed implementation of CronetEngine. (Closed)
DescriptionAdd HttpUrlConnection backed implementation of CronetEngine.
When the native implementation is not available (because it wasn't included in the binary), cronet falls back to a HttpUrlConnection based implementation of the async API.
Committed: https://crrev.com/5632bf5e2836df74ca4d92e98c05540d6b4e62f2
Cr-Commit-Position: refs/heads/master@{#368127}
Patch Set 1 #Patch Set 2 : Fix accidental inclusion of comments #Patch Set 3 : Add cancellation-safe disconnection of HttpUrlConnections #Patch Set 4 : Fix tests #
Total comments: 76
Patch Set 5 : Update tests and header ordering. #
Total comments: 38
Patch Set 6 : Addressed comments #Patch Set 7 : Added checks for invalid state transitions. #Patch Set 8 : Rebase on HEAD #
Total comments: 26
Patch Set 9 : Addressed comments #
Total comments: 6
Patch Set 10 : Rebase on head and address comments #Patch Set 11 : Enabled user agent test #
Total comments: 4
Patch Set 12 : Fix broken test #Patch Set 13 : Fix accidental println #
Total comments: 13
Patch Set 14 : Addressed comments #
Total comments: 3
Messages
Total messages: 42 (7 generated)
clm@google.com changed reviewers: + pauljensen@chromium.org
Paul, PTAL
Discussed with Charles offline and asked that he remove the test app and add unit tests. Yesterday I mentioned that if we could run our existing CronetEngine tests against both the native and Java implementations it would be great test coverage and I mentioned using something similar to @CompareDefaultWithCronet
Ready for re-review, PTAL I had to relax a few tests - some were depending on native-only behavior, some were asserting on native-only exceptions. One notable behavior change was that ordering of response headers is no longer guaranteed, since httpurlconnection doesn't provide them I also don't have a way to easily calculate bytes transferred.
Description was changed from ========== Add HttpUrlConnection backed implementation of CronetEngine. BUG= ========== to ========== Add HttpUrlConnection backed implementation of CronetEngine. BUG= ==========
On 2015/12/04 01:14:10, Charles wrote: > Ready for re-review, PTAL > > I had to relax a few tests - some were depending on native-only behavior, some > were asserting on native-only exceptions. One notable behavior change was that > ordering of response headers is no longer guaranteed, since httpurlconnection > doesn't provide them I also don't have a way to easily calculate bytes > transferred. Realized I was missing what is probably an email filter string: Paul, PTAL
I'll take a look, probably Monday. Been discussing some related CronetTestBase changes that may affect this CL.
https://codereview.chromium.org/1492583002/diff/60001/components/cronet/andro... File components/cronet/android/api/src/org/chromium/net/CronetEngine.java (right): https://codereview.chromium.org/1492583002/diff/60001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:670: // implementation is available merge with createCronetFactory. Could you update this comment? At least the "Fallback to stub implementation" part is obsoleted by this CL. I'm not exactly sure what "createCronetFactory" refers to, perhaps createChromiumFactory. https://codereview.chromium.org/1492583002/diff/60001/components/cronet/andro... File components/cronet/android/api/src/org/chromium/net/InputStreamChannel.java (right): https://codereview.chromium.org/1492583002/diff/60001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/InputStreamChannel.java:15: * Adapts an {@link InputStream} into a {@link ReadableByteChannel}, exactly like extra space before "Adapts" https://codereview.chromium.org/1492583002/diff/60001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/InputStreamChannel.java:48: // Android, the only case where it looks like you used "git cl format" to line-wrap your comments. could you rewrap them in a more sane way? https://codereview.chromium.org/1492583002/diff/60001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/InputStreamChannel.java:57: byte[] tmpBuf = new byte[Math.min(reasonableToRead, MAX_TMP_BUFFER_SIZE)]; it's interesting that you marked your ints final but not your byte[]. I get int's are primitive and an array is a reference. Is there a style guide recommendation for this? otherwise it seems inconsistent. https://codereview.chromium.org/1492583002/diff/60001/components/cronet/andro... File components/cronet/android/api/src/org/chromium/net/JavaCronetEngine.java (right): https://codereview.chromium.org/1492583002/diff/60001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/JavaCronetEngine.java:38: THREAD_PRIORITY_BACKGROUND + THREAD_PRIORITY_MORE_FAVORABLE); why one above background? I think Cronet's network thread is just background. https://codereview.chromium.org/1492583002/diff/60001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/JavaCronetEngine.java:56: String url, UrlRequest.Callback callback, Executor executor, int priority) { I see you're ignoring priority. That's probably fine, but perhaps we should document the limitations of this implementation (like ignoring priority, netlog, no bidiStream, no cache, no NQE etc) on line 25. https://codereview.chromium.org/1492583002/diff/60001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/JavaCronetEngine.java:74: return "HttpUrlConnection"; This should really include a version string so we know what version we're dealing with in bug reports. How about: "CronetHttpURLConnection/" + Version.getVersion(); https://codereview.chromium.org/1492583002/diff/60001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/JavaCronetEngine.java:127: return null; perhaps instead of returning null, return: new URLStreamHandler() { @Override URLConnection openConnection(URL u) throws IOException { return u.openConnection(); } @Override URLConnection openConnection(URL u, Proxy proxy) throws IOException { return u.openConnection(proxy); } } https://codereview.chromium.org/1492583002/diff/60001/components/cronet/andro... File components/cronet/android/api/src/org/chromium/net/JavaUrlRequest.java (right): https://codereview.chromium.org/1492583002/diff/60001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/JavaUrlRequest.java:35: @TargetApi(15) Can you add a comment why 15 is necessary and can you replace this with a constant (e.g. Build.VERSION_CODES.JELLY_BEAN_MR1)? https://codereview.chromium.org/1492583002/diff/60001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/JavaUrlRequest.java:37: private static final String X_ANDROID_SELECTED_TRANSPORT = "X-Android-Selected-Transport"; Does this header exist? According to OkHttp changelog it was replace with OkHttp-Selected-Transport which was in turn later replaced with OkHttp-Selected-Protocol: http://androidxref.com/6.0.0_r1/xref/external/okhttp/CHANGELOG.md https://codereview.chromium.org/1492583002/diff/60001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/JavaUrlRequest.java:56: private final int mTrafficStatsTag; Can we add a comment explaining that we retag requests for the thread they were created on, to avoid having them all accounted for on our thread pool? Would be nice if we could also add a test for this. https://codereview.chromium.org/1492583002/diff/60001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/JavaUrlRequest.java:91: * -> COMPLETE This might be easier to read if it contained loops, then again ASCII art is awful: /- REDIRECTED <-\ /---- READING <----\ | | | | \ / \ / NOT_STARTED ---> STARTED ---------> AWAITING_READ ---> COMPLETE https://codereview.chromium.org/1492583002/diff/60001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/JavaUrlRequest.java:104: JavaUrlRequest(Callback callback, final Executor executor, Executor userExecutor, String url, this could really use a comment explaining what arguments are, at least the first two https://codereview.chromium.org/1492583002/diff/60001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/JavaUrlRequest.java:161: "Request is already started. State is: " + mState.get()); I think you might want to change mState.get() to "state" here? https://codereview.chromium.org/1492583002/diff/60001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/JavaUrlRequest.java:235: private long mWrittenBytes = 0; these variables could use some comments, esp the last two https://codereview.chromium.org/1492583002/diff/60001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/JavaUrlRequest.java:256: logv("UploadReadSucceeded"); is this still necessary? ditto for all the following ones https://codereview.chromium.org/1492583002/diff/60001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/JavaUrlRequest.java:366: private void fireOnResponseStarted() { how come we have fireOnResponseStarted() but no other ones (e.g. fireOnSucceeded)? https://codereview.chromium.org/1492583002/diff/60001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/JavaUrlRequest.java:445: if (!entry.getKey().startsWith("X-Android")) { I think the "X-Android" prefix may have changed https://codereview.chromium.org/1492583002/diff/60001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/JavaUrlRequest.java:446: headerList.add(new SimpleEntry<>(entry.getKey(), value)); we could tally up the size of entry.getKey() and |value| here and put the sum into mUrlResponseInfo.setReceivedBytesCount (plus maybe 2 for each header (one for the ':' and one for the '\n')) https://codereview.chromium.org/1492583002/diff/60001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/JavaUrlRequest.java:526: // It could be null because we're cancelled, or it could be null because this I find this comment a little confusing. What is "It" that could be null? I'm guessing oldConnection, but then why are we discussing the case of null oldConnection in a if statement with clause "oldConnection != null"? https://codereview.chromium.org/1492583002/diff/60001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/JavaUrlRequest.java:563: Preconditions.checkHasRemaining(buffer); this will fail inadvertently in certain cases. this call expects distance between position and capacity. position may be equal to limit, in which case checkHasRemaining will fail inadvertently. https://codereview.chromium.org/1492583002/diff/60001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/JavaUrlRequest.java:633: if (read != -1) { seems like we could call mUrlResponseInfo.setReceivedBytesCount(read + mUrlResponseInfo.getReceivedBytesCount()) https://codereview.chromium.org/1492583002/diff/60001/components/cronet/andro... File components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestTest.java (right): https://codereview.chromium.org/1492583002/diff/60001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestTest.java:130: // Length is not supported in pure java Do you mean Content-Length? if so can you clarify the comment. https://codereview.chromium.org/1492583002/diff/60001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestTest.java:152: // We can't currently get bytes transferred Can we clarify "We" in comment? https://codereview.chromium.org/1492583002/diff/60001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestTest.java:152: // We can't currently get bytes transferred Can we disable just the part comparing bytes transferred instead of the whole test? https://codereview.chromium.org/1492583002/diff/60001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestTest.java:457: @OnlyRunNativeCronet // We can't currently get bytes transferred ditto https://codereview.chromium.org/1492583002/diff/60001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestTest.java:504: @OnlyRunNativeCronet // Can't check data length ditto https://codereview.chromium.org/1492583002/diff/60001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestTest.java:1393: System.out.println("Testing " + failureType + " during " + failureStep); you still need this? https://codereview.chromium.org/1492583002/diff/60001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestTest.java:1463: @OnlyRunNativeCronet can we add comment mentioning no setOnDestroyedCallbackForTests()? https://codereview.chromium.org/1492583002/diff/60001/components/cronet/andro... File components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlTest.java (right): https://codereview.chromium.org/1492583002/diff/60001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlTest.java:57: @OnlyRunNativeCronet can we add comment saying "no NetLog from HttpURLConnection"
I did a first review pass. I didn't yet dig too deep into looking at the correctness of the state transitions or threading issues. It's looking really good so far. Thanks again for writing this! I'm not used to your coding patterns so my reviewing is slow, sorry.
Paul, PTAL https://codereview.chromium.org/1492583002/diff/60001/components/cronet/andro... File components/cronet/android/api/src/org/chromium/net/CronetEngine.java (right): https://codereview.chromium.org/1492583002/diff/60001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:670: // implementation is available merge with createCronetFactory. On 2015/12/08 19:49:52, pauljensen wrote: > Could you update this comment? At least the "Fallback to stub implementation" > part is obsoleted by this CL. I'm not exactly sure what "createCronetFactory" > refers to, perhaps createChromiumFactory. Done. https://codereview.chromium.org/1492583002/diff/60001/components/cronet/andro... File components/cronet/android/api/src/org/chromium/net/InputStreamChannel.java (right): https://codereview.chromium.org/1492583002/diff/60001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/InputStreamChannel.java:15: * Adapts an {@link InputStream} into a {@link ReadableByteChannel}, exactly like On 2015/12/08 19:49:52, pauljensen wrote: > extra space before "Adapts" Done. https://codereview.chromium.org/1492583002/diff/60001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/InputStreamChannel.java:48: // Android, the only case where On 2015/12/08 19:49:52, pauljensen wrote: > it looks like you used "git cl format" to line-wrap your comments. could you > rewrap them in a more sane way? Done. https://codereview.chromium.org/1492583002/diff/60001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/InputStreamChannel.java:57: byte[] tmpBuf = new byte[Math.min(reasonableToRead, MAX_TMP_BUFFER_SIZE)]; On 2015/12/08 19:49:52, pauljensen wrote: > it's interesting that you marked your ints final but not your byte[]. I get > int's are primitive and an array is a reference. Is there a style guide > recommendation for this? otherwise it seems inconsistent. The style guide is "don't put final on local variables, unless it's required or it makes the code easier to follow". I tend to interpret that in a way that uses final on variables in three cases: 1. Variable is accessed in an anonymous class (has to be final) 2. Variable is declared, and there's some complex code that initializes it - by declaring it final, the compiler proves that it can only be initialized once, and that it must be initialized. The "final int read" up earlier lets you skip checking that its results might not be populated or might be overwritten. 3. Variable is in a mix of other, similarly typed variables. I could see myself accidentally reassigning possibleToRead or reasonableToRead, since there are a bunch of ints floating around those three lines of code, but that case is a little less defensible. I didn't mark tmpBuf final, because it's the only byte[] in that area of code, and the code from when it's declared to when it goes out of scope is very simple, and there's little opportunity for confusion. https://codereview.chromium.org/1492583002/diff/60001/components/cronet/andro... File components/cronet/android/api/src/org/chromium/net/JavaCronetEngine.java (right): https://codereview.chromium.org/1492583002/diff/60001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/JavaCronetEngine.java:38: THREAD_PRIORITY_BACKGROUND + THREAD_PRIORITY_MORE_FAVORABLE); On 2015/12/08 19:49:52, pauljensen wrote: > why one above background? I think Cronet's network thread is just background. Added comment explaining priority selection. https://codereview.chromium.org/1492583002/diff/60001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/JavaCronetEngine.java:56: String url, UrlRequest.Callback callback, Executor executor, int priority) { On 2015/12/08 19:49:52, pauljensen wrote: > I see you're ignoring priority. That's probably fine, but perhaps we should > document the limitations of this implementation (like ignoring priority, netlog, > no bidiStream, no cache, no NQE etc) on line 25. Done. https://codereview.chromium.org/1492583002/diff/60001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/JavaCronetEngine.java:74: return "HttpUrlConnection"; On 2015/12/08 19:49:52, pauljensen wrote: > This should really include a version string so we know what version we're > dealing with in bug reports. > How about: "CronetHttpURLConnection/" + Version.getVersion(); Done. https://codereview.chromium.org/1492583002/diff/60001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/JavaCronetEngine.java:127: return null; On 2015/12/08 19:49:52, pauljensen wrote: > perhaps instead of returning null, return: > new URLStreamHandler() { > @Override > URLConnection openConnection(URL u) throws IOException { > return u.openConnection(); > } > @Override > URLConnection openConnection(URL u, Proxy proxy) throws IOException { > return u.openConnection(proxy); > } > } That causes infinite recursion. Added a comment. https://codereview.chromium.org/1492583002/diff/60001/components/cronet/andro... File components/cronet/android/api/src/org/chromium/net/JavaUrlRequest.java (right): https://codereview.chromium.org/1492583002/diff/60001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/JavaUrlRequest.java:35: @TargetApi(15) On 2015/12/08 19:49:53, pauljensen wrote: > Can you add a comment why 15 is necessary and can you replace this with a > constant (e.g. Build.VERSION_CODES.JELLY_BEAN_MR1)? Is that the style in chrome? I tend to use integer literals because that's what's specified in the API documentation - "Added in API leve 11" https://codereview.chromium.org/1492583002/diff/60001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/JavaUrlRequest.java:37: private static final String X_ANDROID_SELECTED_TRANSPORT = "X-Android-Selected-Transport"; On 2015/12/08 19:49:53, pauljensen wrote: > Does this header exist? According to OkHttp changelog it was replace with > OkHttp-Selected-Transport which was in turn later replaced with > OkHttp-Selected-Protocol: > http://androidxref.com/6.0.0_r1/xref/external/okhttp/CHANGELOG.md It was in here at some point: https://android.googlesource.com/platform/external/okhttp/+/00834c9a00d53b29c... https://codereview.chromium.org/1492583002/diff/60001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/JavaUrlRequest.java:56: private final int mTrafficStatsTag; On 2015/12/08 19:49:53, pauljensen wrote: > Can we add a comment explaining that we retag requests for the thread they were > created on, to avoid having them all accounted for on our thread pool? > Would be nice if we could also add a test for this. Unfortunately there's no way to externally observe that data info was sent to a specific tag (or I'd be using it to populate the bytes transferred). I had the idea of creating a wrapper HttpUrlConnection that checks the current tag, but that's pretty complex and I'd rather do it in another CL. https://codereview.chromium.org/1492583002/diff/60001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/JavaUrlRequest.java:91: * -> COMPLETE On 2015/12/08 19:49:52, pauljensen wrote: > This might be easier to read if it contained loops, then again ASCII art is > awful: > > /- REDIRECTED <-\ /---- READING <----\ > | | | | > \ / \ / > NOT_STARTED ---> STARTED ---------> AWAITING_READ ---> COMPLETE Done. https://codereview.chromium.org/1492583002/diff/60001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/JavaUrlRequest.java:104: JavaUrlRequest(Callback callback, final Executor executor, Executor userExecutor, String url, On 2015/12/08 19:49:52, pauljensen wrote: > this could really use a comment explaining what arguments are, at least the > first two Done. https://codereview.chromium.org/1492583002/diff/60001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/JavaUrlRequest.java:161: "Request is already started. State is: " + mState.get()); On 2015/12/08 19:49:53, pauljensen wrote: > I think you might want to change mState.get() to "state" here? Done. https://codereview.chromium.org/1492583002/diff/60001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/JavaUrlRequest.java:235: private long mWrittenBytes = 0; On 2015/12/08 19:49:53, pauljensen wrote: > these variables could use some comments, esp the last two Done. https://codereview.chromium.org/1492583002/diff/60001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/JavaUrlRequest.java:256: logv("UploadReadSucceeded"); On 2015/12/08 19:49:53, pauljensen wrote: > is this still necessary? ditto for all the following ones Done. https://codereview.chromium.org/1492583002/diff/60001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/JavaUrlRequest.java:366: private void fireOnResponseStarted() { On 2015/12/08 19:49:53, pauljensen wrote: > how come we have fireOnResponseStarted() but no other ones (e.g. > fireOnSucceeded)? Done. https://codereview.chromium.org/1492583002/diff/60001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/JavaUrlRequest.java:445: if (!entry.getKey().startsWith("X-Android")) { On 2015/12/08 19:49:53, pauljensen wrote: > I think the "X-Android" prefix may have changed On my test device, the synthetic headers are still present, and still have X-Android in them. https://codereview.chromium.org/1492583002/diff/60001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/JavaUrlRequest.java:446: headerList.add(new SimpleEntry<>(entry.getKey(), value)); On 2015/12/08 19:49:53, pauljensen wrote: > we could tally up the size of entry.getKey() and |value| here and put the sum > into mUrlResponseInfo.setReceivedBytesCount (plus maybe 2 for each header (one > for the ':' and one for the '\n')) We could, but I feel a little dishonest doing it, since the platform is free to add its own headers that we don't know about. https://codereview.chromium.org/1492583002/diff/60001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/JavaUrlRequest.java:526: // It could be null because we're cancelled, or it could be null because this On 2015/12/08 19:49:53, pauljensen wrote: > I find this comment a little confusing. What is "It" that could be null? I'm > guessing oldConnection, but then why are we discussing the case of null > oldConnection in a if statement with clause "oldConnection != null"? Done. https://codereview.chromium.org/1492583002/diff/60001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/JavaUrlRequest.java:563: Preconditions.checkHasRemaining(buffer); On 2015/12/08 19:49:53, pauljensen wrote: > this will fail inadvertently in certain cases. this call expects distance > between position and capacity. position may be equal to limit, in which case > checkHasRemaining will fail inadvertently. Good catch. https://codereview.chromium.org/1492583002/diff/60001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/JavaUrlRequest.java:633: if (read != -1) { On 2015/12/08 19:49:52, pauljensen wrote: > seems like we could call mUrlResponseInfo.setReceivedBytesCount(read + > mUrlResponseInfo.getReceivedBytesCount()) Unfortunately, gzip is transparent - we don't know the real count. https://codereview.chromium.org/1492583002/diff/60001/components/cronet/andro... File components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestTest.java (right): https://codereview.chromium.org/1492583002/diff/60001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestTest.java:130: // Length is not supported in pure java On 2015/12/08 19:49:53, pauljensen wrote: > Do you mean Content-Length? if so can you clarify the comment. No longer relevant. https://codereview.chromium.org/1492583002/diff/60001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestTest.java:152: // We can't currently get bytes transferred On 2015/12/08 19:49:53, pauljensen wrote: > Can we clarify "We" in comment? Done. https://codereview.chromium.org/1492583002/diff/60001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestTest.java:152: // We can't currently get bytes transferred On 2015/12/08 19:49:53, pauljensen wrote: > Can we disable just the part comparing bytes transferred instead of the whole > test? Done. https://codereview.chromium.org/1492583002/diff/60001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestTest.java:457: @OnlyRunNativeCronet // We can't currently get bytes transferred On 2015/12/08 19:49:53, pauljensen wrote: > ditto Done. https://codereview.chromium.org/1492583002/diff/60001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestTest.java:504: @OnlyRunNativeCronet // Can't check data length On 2015/12/08 19:49:53, pauljensen wrote: > ditto Done. https://codereview.chromium.org/1492583002/diff/60001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestTest.java:1393: System.out.println("Testing " + failureType + " during " + failureStep); On 2015/12/08 19:49:53, pauljensen wrote: > you still need this? Done. https://codereview.chromium.org/1492583002/diff/60001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestTest.java:1463: @OnlyRunNativeCronet On 2015/12/08 19:49:53, pauljensen wrote: > can we add comment mentioning no setOnDestroyedCallbackForTests()? Done. https://codereview.chromium.org/1492583002/diff/60001/components/cronet/andro... File components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlTest.java (right): https://codereview.chromium.org/1492583002/diff/60001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlTest.java:57: @OnlyRunNativeCronet On 2015/12/08 19:49:53, pauljensen wrote: > can we add comment saying "no NetLog from HttpURLConnection" Done.
https://codereview.chromium.org/1492583002/diff/60001/components/cronet/andro... File components/cronet/android/api/src/org/chromium/net/JavaUrlRequest.java (right): https://codereview.chromium.org/1492583002/diff/60001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/JavaUrlRequest.java:35: @TargetApi(15) On 2015/12/11 16:45:39, Charles wrote: > On 2015/12/08 19:49:53, pauljensen wrote: > > Can you add a comment why 15 is necessary and can you replace this with a > > constant (e.g. Build.VERSION_CODES.JELLY_BEAN_MR1)? > > Is that the style in chrome? I tend to use integer literals because that's > what's specified in the API documentation - "Added in API leve 11" I find a pre-defined constant is always preferable to an integer literal. Please also add a comment about why this is necessary. https://codereview.chromium.org/1492583002/diff/60001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/JavaUrlRequest.java:37: private static final String X_ANDROID_SELECTED_TRANSPORT = "X-Android-Selected-Transport"; On 2015/12/11 16:45:40, Charles wrote: > On 2015/12/08 19:49:53, pauljensen wrote: > > Does this header exist? According to OkHttp changelog it was replace with > > OkHttp-Selected-Transport which was in turn later replaced with > > OkHttp-Selected-Protocol: > > http://androidxref.com/6.0.0_r1/xref/external/okhttp/CHANGELOG.md > > It was in here at some point: > https://android.googlesource.com/platform/external/okhttp/+/00834c9a00d53b29c... I agree it was there at some point, but it isn't anymore AFAIK. https://codereview.chromium.org/1492583002/diff/60001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/JavaUrlRequest.java:56: private final int mTrafficStatsTag; On 2015/12/11 16:45:39, Charles wrote: > On 2015/12/08 19:49:53, pauljensen wrote: > > Can we add a comment explaining that we retag requests for the thread they > were > > created on, to avoid having them all accounted for on our thread pool? > > Would be nice if we could also add a test for this. > > Unfortunately there's no way to externally observe that data info was sent to a > specific tag (or I'd be using it to populate the bytes transferred). > > I had the idea of creating a wrapper HttpUrlConnection that checks the current > tag, but that's pretty complex and I'd rather do it in another CL. Ok, I guess there isn't an easy way to test this, but let's add the comment about why/how this code exists. https://codereview.chromium.org/1492583002/diff/60001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/JavaUrlRequest.java:445: if (!entry.getKey().startsWith("X-Android")) { On 2015/12/11 16:45:39, Charles wrote: > On 2015/12/08 19:49:53, pauljensen wrote: > > I think the "X-Android" prefix may have changed > > On my test device, the synthetic headers are still present, and still have > X-Android in them. I understand it was this way, but AFAIK it's not anymore according to the change-log I linked to. https://codereview.chromium.org/1492583002/diff/60001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/JavaUrlRequest.java:446: headerList.add(new SimpleEntry<>(entry.getKey(), value)); On 2015/12/11 16:45:39, Charles wrote: > On 2015/12/08 19:49:53, pauljensen wrote: > > we could tally up the size of entry.getKey() and |value| here and put the sum > > into mUrlResponseInfo.setReceivedBytesCount (plus maybe 2 for each header (one > > for the ':' and one for the '\n')) > > We could, but I feel a little dishonest doing it, since the platform is free to > add its own headers that we don't know about. How is it dishonest? The received bytes count is just used to so some really simplistic data-usage accounting by embedders AFAIK. https://codereview.chromium.org/1492583002/diff/80001/components/cronet/andro... File components/cronet/android/api/src/org/chromium/net/InputStreamChannel.java (right): https://codereview.chromium.org/1492583002/diff/80001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/InputStreamChannel.java:55: byte[] tmpBuf = new byte[Math.min(reasonableToRead, MAX_TMP_BUFFER_SIZE)]; this last min() seems useless. x = min(a, b); y = min(x, a) should be equivalent to min(a, b) https://codereview.chromium.org/1492583002/diff/80001/components/cronet/andro... File components/cronet/android/api/src/org/chromium/net/JavaCronetEngine.java (right): https://codereview.chromium.org/1492583002/diff/80001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/JavaCronetEngine.java:59: return new JavaUrlRequest(callback, mExecutorService, executor, url, mUserAgent); can we make this call createRequest(blah, blah, blah, blah, REQUEST_PRIORITY_MEDIUM), or better yet, can you make give CronetEngine.createRequest(without priority) an impl that does this? https://codereview.chromium.org/1492583002/diff/80001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/JavaCronetEngine.java:133: // implementation. Hmm this isn't documented anywhere that I can find, but it does appear to be how Android's implementation works. https://codereview.chromium.org/1492583002/diff/80001/components/cronet/andro... File components/cronet/android/api/src/org/chromium/net/JavaUrlRequest.java (right): https://codereview.chromium.org/1492583002/diff/80001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/JavaUrlRequest.java:41: public static final String USER_AGENT = "User-Agent"; why public? https://codereview.chromium.org/1492583002/diff/80001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/JavaUrlRequest.java:46: new TreeMap<>(String.CASE_INSENSITIVE_ORDER); nit: I prefer hash tables as they're more computationally efficient... probably doesn't matter much for headers I suppose. https://codereview.chromium.org/1492583002/diff/80001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/JavaUrlRequest.java:65: * Holds a subset of StatusValues - {@link State#STARTED} can represent indented an extra space https://codereview.chromium.org/1492583002/diff/80001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/JavaUrlRequest.java:71: * risk funny line wrapping https://codereview.chromium.org/1492583002/diff/80001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/JavaUrlRequest.java:109: * can we get rid of this blank line? https://codereview.chromium.org/1492583002/diff/80001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/JavaUrlRequest.java:159: || "TRACE".equalsIgnoreCase(method) || "PATCH".equalsIgnoreCase(method)) { CONNECT? https://codereview.chromium.org/1492583002/diff/80001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/JavaUrlRequest.java:228: mInitialMethod = "POST"; nit: I like calling setHttpMethod() here in case it does something more than just setting mInitialMethod, but this is probably just a personal preference. Either way is fine. https://codereview.chromium.org/1492583002/diff/80001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/JavaUrlRequest.java:241: /** This holds the total bytes to send (the content-length) */ -1 has a special meaning, correct? https://codereview.chromium.org/1492583002/diff/80001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/JavaUrlRequest.java:340: mBuffer = ByteBuffer.allocateDirect((int) mTotalBytes + 1); Why +1 ? https://codereview.chromium.org/1492583002/diff/80001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/JavaUrlRequest.java:347: } else if (mTotalBytes > Integer.MAX_VALUE && Build.VERSION.SDK_INT >= 19) { I'd prefer a constant is used rather than an integer literal. I imagine dev.android.com lists the integer literal next to each API for brevity. https://codereview.chromium.org/1492583002/diff/80001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/JavaUrlRequest.java:405: if (!(state == State.CANCELLED || state == State.ERROR)) { Do we really want to be continuing along if the request is cancelled or failed? For example followRedirect() will set mCurrentUrl to mPendingRedirectUrl which is probably null. Then it will call fireOpenConnection which will probably NPE on some worker thread. Seems like transitionStates() should return the CAS result and callers should bail out (i.e. return early) when false. https://codereview.chromium.org/1492583002/diff/80001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/JavaUrlRequest.java:429: String selectedTransport = "http/1.1"; I'm not sure it's a good idea to make this default to a believable value; this is likely to cover up problems (like X-Android header names changing). https://codereview.chromium.org/1492583002/diff/80001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/JavaUrlRequest.java:622: fireDisconnect(); nit: seems a little silly that fireDisconnect() posts a task to mExecutor, from processReadResult() which is already posted to mExecutor. https://codereview.chromium.org/1492583002/diff/80001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/JavaUrlRequest.java:633: if (connection != null) { seems like this null check could be before the execute() call https://codereview.chromium.org/1492583002/diff/80001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/JavaUrlRequest.java:725: void execute(State currentState, CheckedRunnable runnable) { private?
Paul, PTAL. A few unsettled issues: 1. What should we do about features of Cronet we can't implement in this impl? I'm thinking of Proxy, selected transport, and transferred byte count. 2. What should we do about the synthetic headers from Android? https://codereview.chromium.org/1492583002/diff/60001/components/cronet/andro... File components/cronet/android/api/src/org/chromium/net/JavaUrlRequest.java (right): https://codereview.chromium.org/1492583002/diff/60001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/JavaUrlRequest.java:35: @TargetApi(15) On 2015/12/14 20:02:05, pauljensen wrote: > On 2015/12/11 16:45:39, Charles wrote: > > On 2015/12/08 19:49:53, pauljensen wrote: > > > Can you add a comment why 15 is necessary and can you replace this with a > > > constant (e.g. Build.VERSION_CODES.JELLY_BEAN_MR1)? > > > > Is that the style in chrome? I tend to use integer literals because that's > > what's specified in the API documentation - "Added in API leve 11" > > I find a pre-defined constant is always preferable to an integer literal. > Please also add a comment about why this is necessary. Done. https://codereview.chromium.org/1492583002/diff/60001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/JavaUrlRequest.java:37: private static final String X_ANDROID_SELECTED_TRANSPORT = "X-Android-Selected-Transport"; On 2015/12/14 20:02:05, pauljensen wrote: > On 2015/12/11 16:45:40, Charles wrote: > > On 2015/12/08 19:49:53, pauljensen wrote: > > > Does this header exist? According to OkHttp changelog it was replace with > > > OkHttp-Selected-Transport which was in turn later replaced with > > > OkHttp-Selected-Protocol: > > > http://androidxref.com/6.0.0_r1/xref/external/okhttp/CHANGELOG.md > > > > It was in here at some point: > > > https://android.googlesource.com/platform/external/okhttp/+/00834c9a00d53b29c... > > I agree it was there at some point, but it isn't anymore AFAIK. Old android code doesn't go away, it's still running on users devices for years. https://codereview.chromium.org/1492583002/diff/60001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/JavaUrlRequest.java:56: private final int mTrafficStatsTag; On 2015/12/14 20:02:05, pauljensen wrote: > On 2015/12/11 16:45:39, Charles wrote: > > On 2015/12/08 19:49:53, pauljensen wrote: > > > Can we add a comment explaining that we retag requests for the thread they > > were > > > created on, to avoid having them all accounted for on our thread pool? > > > Would be nice if we could also add a test for this. > > > > Unfortunately there's no way to externally observe that data info was sent to > a > > specific tag (or I'd be using it to populate the bytes transferred). > > > > I had the idea of creating a wrapper HttpUrlConnection that checks the current > > tag, but that's pretty complex and I'd rather do it in another CL. > > Ok, I guess there isn't an easy way to test this, but let's add the comment > about why/how this code exists. Done. https://codereview.chromium.org/1492583002/diff/60001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/JavaUrlRequest.java:445: if (!entry.getKey().startsWith("X-Android")) { On 2015/12/14 20:02:05, pauljensen wrote: > On 2015/12/11 16:45:39, Charles wrote: > > On 2015/12/08 19:49:53, pauljensen wrote: > > > I think the "X-Android" prefix may have changed > > > > On my test device, the synthetic headers are still present, and still have > > X-Android in them. > > I understand it was this way, but AFAIK it's not anymore according to the > change-log I linked to. I'm not sure I follow - it doesn't matter what changes Android makes, the devices users actually have in their hands are still running that old code. We have to handle that behavior. https://codereview.chromium.org/1492583002/diff/60001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/JavaUrlRequest.java:446: headerList.add(new SimpleEntry<>(entry.getKey(), value)); On 2015/12/14 20:02:05, pauljensen wrote: > On 2015/12/11 16:45:39, Charles wrote: > > On 2015/12/08 19:49:53, pauljensen wrote: > > > we could tally up the size of entry.getKey() and |value| here and put the > sum > > > into mUrlResponseInfo.setReceivedBytesCount (plus maybe 2 for each header > (one > > > for the ':' and one for the '\n')) > > > > We could, but I feel a little dishonest doing it, since the platform is free > to > > add its own headers that we don't know about. > > How is it dishonest? The received bytes count is just used to so some really > simplistic data-usage accounting by embedders AFAIK. The simplistic data usage accounting is used to make decisions about what features cost users what amount of money - I don't want to return information that's not correct. https://codereview.chromium.org/1492583002/diff/80001/components/cronet/andro... File components/cronet/android/api/src/org/chromium/net/InputStreamChannel.java (right): https://codereview.chromium.org/1492583002/diff/80001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/InputStreamChannel.java:55: byte[] tmpBuf = new byte[Math.min(reasonableToRead, MAX_TMP_BUFFER_SIZE)]; On 2015/12/14 20:02:05, pauljensen wrote: > this last min() seems useless. x = min(a, b); y = min(x, a) should be > equivalent to min(a, b) Done. https://codereview.chromium.org/1492583002/diff/80001/components/cronet/andro... File components/cronet/android/api/src/org/chromium/net/JavaCronetEngine.java (right): https://codereview.chromium.org/1492583002/diff/80001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/JavaCronetEngine.java:59: return new JavaUrlRequest(callback, mExecutorService, executor, url, mUserAgent); On 2015/12/14 20:02:05, pauljensen wrote: > can we make this call createRequest(blah, blah, blah, blah, > REQUEST_PRIORITY_MEDIUM), or better yet, can you make give > CronetEngine.createRequest(without priority) an impl that does this? Done. https://codereview.chromium.org/1492583002/diff/80001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/JavaCronetEngine.java:133: // implementation. On 2015/12/14 20:02:05, pauljensen wrote: > Hmm this isn't documented anywhere that I can find, but it does appear to be how > Android's implementation works. Acknowledged. https://codereview.chromium.org/1492583002/diff/80001/components/cronet/andro... File components/cronet/android/api/src/org/chromium/net/JavaUrlRequest.java (right): https://codereview.chromium.org/1492583002/diff/80001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/JavaUrlRequest.java:41: public static final String USER_AGENT = "User-Agent"; On 2015/12/14 20:02:05, pauljensen wrote: > why public? Done. https://codereview.chromium.org/1492583002/diff/80001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/JavaUrlRequest.java:46: new TreeMap<>(String.CASE_INSENSITIVE_ORDER); On 2015/12/14 20:02:05, pauljensen wrote: > nit: I prefer hash tables as they're more computationally efficient... probably > doesn't matter much for headers I suppose. Doesn't matter much for headers, and I'd have to implement my own custom Map class to provide the case insensitive behavior I need. https://codereview.chromium.org/1492583002/diff/80001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/JavaUrlRequest.java:65: * Holds a subset of StatusValues - {@link State#STARTED} can represent On 2015/12/14 20:02:05, pauljensen wrote: > indented an extra space Done. https://codereview.chromium.org/1492583002/diff/80001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/JavaUrlRequest.java:71: * risk On 2015/12/14 20:02:05, pauljensen wrote: > funny line wrapping Done. https://codereview.chromium.org/1492583002/diff/80001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/JavaUrlRequest.java:109: * On 2015/12/14 20:02:06, pauljensen wrote: > can we get rid of this blank line? Done. https://codereview.chromium.org/1492583002/diff/80001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/JavaUrlRequest.java:159: || "TRACE".equalsIgnoreCase(method) || "PATCH".equalsIgnoreCase(method)) { On 2015/12/14 20:02:05, pauljensen wrote: > CONNECT? It's not mentioned as a supported method in the Android docs: https://developer.android.com/reference/java/net/HttpURLConnection.html And in getRequestMethod, it says "All possible methods of this HTTP implementation is listed in the class definition" https://codereview.chromium.org/1492583002/diff/80001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/JavaUrlRequest.java:228: mInitialMethod = "POST"; On 2015/12/14 20:02:06, pauljensen wrote: > nit: I like calling setHttpMethod() here in case it does something more than > just setting mInitialMethod, but this is probably just a personal preference. > Either way is fine. I don't create the HttpUrlConnection instance to call this on until start() is called, and I don't want to move that earlier, when it's happening on an arbitrary thread. https://codereview.chromium.org/1492583002/diff/80001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/JavaUrlRequest.java:241: /** This holds the total bytes to send (the content-length) */ On 2015/12/14 20:02:05, pauljensen wrote: > -1 has a special meaning, correct? Done. https://codereview.chromium.org/1492583002/diff/80001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/JavaUrlRequest.java:340: mBuffer = ByteBuffer.allocateDirect((int) mTotalBytes + 1); On 2015/12/14 20:02:05, pauljensen wrote: > Why +1 ? Commented. https://codereview.chromium.org/1492583002/diff/80001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/JavaUrlRequest.java:347: } else if (mTotalBytes > Integer.MAX_VALUE && Build.VERSION.SDK_INT >= 19) { On 2015/12/14 20:02:05, pauljensen wrote: > I'd prefer a constant is used rather than an integer literal. I imagine > http://dev.android.com lists the integer literal next to each API for brevity. Done. https://codereview.chromium.org/1492583002/diff/80001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/JavaUrlRequest.java:405: if (!(state == State.CANCELLED || state == State.ERROR)) { On 2015/12/14 20:02:05, pauljensen wrote: > Do we really want to be continuing along if the request is cancelled or failed? > For example followRedirect() will set mCurrentUrl to mPendingRedirectUrl which > is probably null. Then it will call fireOpenConnection which will probably NPE > on some worker thread. > > Seems like transitionStates() should return the CAS result and callers should > bail out (i.e. return early) when false. Total error on my part. You're correct that we should be returning early. I implemented it with a callback, so that there's no way to forget. https://codereview.chromium.org/1492583002/diff/80001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/JavaUrlRequest.java:429: String selectedTransport = "http/1.1"; On 2015/12/14 20:02:05, pauljensen wrote: > I'm not sure it's a good idea to make this default to a believable value; this > is likely to cover up problems (like X-Android header names changing). All of that behavior is undocumented and best-effort anyway. What would you like the behavior to be? https://codereview.chromium.org/1492583002/diff/80001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/JavaUrlRequest.java:622: fireDisconnect(); On 2015/12/14 20:02:05, pauljensen wrote: > nit: seems a little silly that fireDisconnect() posts a task to mExecutor, from > processReadResult() which is already posted to mExecutor. If it didn't, we'd delay the onSucceeded callback until disconnect() returned. I suppose I could call it after, but it's easier to reason about if we always post to the executor for disconnection, and the overhead isn't high. https://codereview.chromium.org/1492583002/diff/80001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/JavaUrlRequest.java:633: if (connection != null) { On 2015/12/14 20:02:05, pauljensen wrote: > seems like this null check could be before the execute() call Done. https://codereview.chromium.org/1492583002/diff/80001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/JavaUrlRequest.java:725: void execute(State currentState, CheckedRunnable runnable) { On 2015/12/14 20:02:05, pauljensen wrote: > private? No point in adding private to methods on private inner classes. I've removed private from all the variables and methods too.
https://codereview.chromium.org/1492583002/diff/60001/components/cronet/andro... File components/cronet/android/api/src/org/chromium/net/JavaUrlRequest.java (right): https://codereview.chromium.org/1492583002/diff/60001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/JavaUrlRequest.java:37: private static final String X_ANDROID_SELECTED_TRANSPORT = "X-Android-Selected-Transport"; On 2015/12/15 22:32:35, Charles wrote: > On 2015/12/14 20:02:05, pauljensen wrote: > > On 2015/12/11 16:45:40, Charles wrote: > > > On 2015/12/08 19:49:53, pauljensen wrote: > > > > Does this header exist? According to OkHttp changelog it was replace with > > > > OkHttp-Selected-Transport which was in turn later replaced with > > > > OkHttp-Selected-Protocol: > > > > http://androidxref.com/6.0.0_r1/xref/external/okhttp/CHANGELOG.md > > > > > > It was in here at some point: > > > > > > https://android.googlesource.com/platform/external/okhttp/+/00834c9a00d53b29c... > > > > I agree it was there at some point, but it isn't anymore AFAIK. > > Old android code doesn't go away, it's still running on users devices for years. That may be so, but newer Android releases will fail here and need to be fixed. https://codereview.chromium.org/1492583002/diff/60001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/JavaUrlRequest.java:446: headerList.add(new SimpleEntry<>(entry.getKey(), value)); On 2015/12/15 22:32:35, Charles wrote: > On 2015/12/14 20:02:05, pauljensen wrote: > > On 2015/12/11 16:45:39, Charles wrote: > > > On 2015/12/08 19:49:53, pauljensen wrote: > > > > we could tally up the size of entry.getKey() and |value| here and put the > > sum > > > > into mUrlResponseInfo.setReceivedBytesCount (plus maybe 2 for each header > > (one > > > > for the ':' and one for the '\n')) > > > > > > We could, but I feel a little dishonest doing it, since the platform is free > > to > > > add its own headers that we don't know about. > > > > How is it dishonest? The received bytes count is just used to so some really > > simplistic data-usage accounting by embedders AFAIK. > > The simplistic data usage accounting is used to make decisions about what > features cost users what amount of money - I don't want to return information > that's not correct. Returning zero is less information than returning something non-zero. This value is documented to be a lower bound. A higher lower bound is better than a lower one.
Charles and I chatted on two matters: 1. I think the X-Android-blah header is changed on Marshmallow releases. He's going to test on Marshmallow. 2. The compare-and-swap synchronization model seems somewhat brittle to me. If we just encountered a redirect we'll set the state to the next state, but we won't have finished updating the internal state so if the user were to call followRedirect() for example it might crash. Charles agreed to add locks/synchronization blocks to address this.
On 2015/12/18 18:50:35, pauljensen wrote: > Charles and I chatted on two matters: > > 1. I think the X-Android-blah header is changed on Marshmallow releases. He's > going to test on Marshmallow. No extra headers need fixing on marshmallow. > 2. The compare-and-swap synchronization model seems somewhat brittle to me. If > we just encountered a redirect we'll set the state to the next state, but we > won't have finished updating the internal state so if the user were to call > followRedirect() for example it might crash. Charles agreed to add > locks/synchronization blocks to address this. We later discussed and no longer feel that we need locks. An extra state was added to handle redirects.
On 2015/12/17 19:59:49, pauljensen wrote: > https://codereview.chromium.org/1492583002/diff/60001/components/cronet/andro... > File components/cronet/android/api/src/org/chromium/net/JavaUrlRequest.java > (right): > > https://codereview.chromium.org/1492583002/diff/60001/components/cronet/andro... > components/cronet/android/api/src/org/chromium/net/JavaUrlRequest.java:37: > private static final String X_ANDROID_SELECTED_TRANSPORT = > "X-Android-Selected-Transport"; > On 2015/12/15 22:32:35, Charles wrote: > > On 2015/12/14 20:02:05, pauljensen wrote: > > > On 2015/12/11 16:45:40, Charles wrote: > > > > On 2015/12/08 19:49:53, pauljensen wrote: > > > > > Does this header exist? According to OkHttp changelog it was replace > with > > > > > OkHttp-Selected-Transport which was in turn later replaced with > > > > > OkHttp-Selected-Protocol: > > > > > http://androidxref.com/6.0.0_r1/xref/external/okhttp/CHANGELOG.md > > > > > > > > It was in here at some point: > > > > > > > > > > https://android.googlesource.com/platform/external/okhttp/+/00834c9a00d53b29c... > > > > > > I agree it was there at some point, but it isn't anymore AFAIK. > > > > Old android code doesn't go away, it's still running on users devices for > years. > > That may be so, but newer Android releases will fail here and need to be fixed. > > https://codereview.chromium.org/1492583002/diff/60001/components/cronet/andro... > components/cronet/android/api/src/org/chromium/net/JavaUrlRequest.java:446: > headerList.add(new SimpleEntry<>(entry.getKey(), value)); > On 2015/12/15 22:32:35, Charles wrote: > > On 2015/12/14 20:02:05, pauljensen wrote: > > > On 2015/12/11 16:45:39, Charles wrote: > > > > On 2015/12/08 19:49:53, pauljensen wrote: > > > > > we could tally up the size of entry.getKey() and |value| here and put > the > > > sum > > > > > into mUrlResponseInfo.setReceivedBytesCount (plus maybe 2 for each > header > > > (one > > > > > for the ':' and one for the '\n')) > > > > > > > > We could, but I feel a little dishonest doing it, since the platform is > free > > > to > > > > add its own headers that we don't know about. > > > > > > How is it dishonest? The received bytes count is just used to so some > really > > > simplistic data-usage accounting by embedders AFAIK. > > > > The simplistic data usage accounting is used to make decisions about what > > features cost users what amount of money - I don't want to return information > > that's not correct. > > Returning zero is less information than returning something non-zero. This > value is documented to be a lower bound. A higher lower bound is better than a > lower one. That's true, but I can't keep it a lower bound and include the response body in the count. I feel like it's better to just say that it's not supported at all, with 0 or -1 or some other sentinel value.
Paul, PTAL https://codereview.chromium.org/1492583002/diff/60001/components/cronet/andro... File components/cronet/android/api/src/org/chromium/net/JavaUrlRequest.java (right): https://codereview.chromium.org/1492583002/diff/60001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/JavaUrlRequest.java:37: private static final String X_ANDROID_SELECTED_TRANSPORT = "X-Android-Selected-Transport"; On 2015/12/17 19:59:49, pauljensen wrote: > On 2015/12/15 22:32:35, Charles wrote: > > On 2015/12/14 20:02:05, pauljensen wrote: > > > On 2015/12/11 16:45:40, Charles wrote: > > > > On 2015/12/08 19:49:53, pauljensen wrote: > > > > > Does this header exist? According to OkHttp changelog it was replace > with > > > > > OkHttp-Selected-Transport which was in turn later replaced with > > > > > OkHttp-Selected-Protocol: > > > > > http://androidxref.com/6.0.0_r1/xref/external/okhttp/CHANGELOG.md > > > > > > > > It was in here at some point: > > > > > > > > > > https://android.googlesource.com/platform/external/okhttp/+/00834c9a00d53b29c... > > > > > > I agree it was there at some point, but it isn't anymore AFAIK. > > > > Old android code doesn't go away, it's still running on users devices for > years. > > That may be so, but newer Android releases will fail here and need to be fixed. Tested. https://codereview.chromium.org/1492583002/diff/60001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/JavaUrlRequest.java:446: headerList.add(new SimpleEntry<>(entry.getKey(), value)); On 2015/12/17 19:59:49, pauljensen wrote: > On 2015/12/15 22:32:35, Charles wrote: > > On 2015/12/14 20:02:05, pauljensen wrote: > > > On 2015/12/11 16:45:39, Charles wrote: > > > > On 2015/12/08 19:49:53, pauljensen wrote: > > > > > we could tally up the size of entry.getKey() and |value| here and put > the > > > sum > > > > > into mUrlResponseInfo.setReceivedBytesCount (plus maybe 2 for each > header > > > (one > > > > > for the ':' and one for the '\n')) > > > > > > > > We could, but I feel a little dishonest doing it, since the platform is > free > > > to > > > > add its own headers that we don't know about. > > > > > > How is it dishonest? The received bytes count is just used to so some > really > > > simplistic data-usage accounting by embedders AFAIK. > > > > The simplistic data usage accounting is used to make decisions about what > > features cost users what amount of money - I don't want to return information > > that's not correct. > > Returning zero is less information than returning something non-zero. This > value is documented to be a lower bound. A higher lower bound is better than a > lower one. Alas, we don't get either - since I don't know if the response was gzipped or not, I can't say that the body size is a lower bound.
On 2015/12/18 21:52:53, Charles wrote: > On 2015/12/17 19:59:49, pauljensen wrote: > > > https://codereview.chromium.org/1492583002/diff/60001/components/cronet/andro... > > File components/cronet/android/api/src/org/chromium/net/JavaUrlRequest.java > > (right): > > > > > https://codereview.chromium.org/1492583002/diff/60001/components/cronet/andro... > > components/cronet/android/api/src/org/chromium/net/JavaUrlRequest.java:37: > > private static final String X_ANDROID_SELECTED_TRANSPORT = > > "X-Android-Selected-Transport"; > > On 2015/12/15 22:32:35, Charles wrote: > > > On 2015/12/14 20:02:05, pauljensen wrote: > > > > On 2015/12/11 16:45:40, Charles wrote: > > > > > On 2015/12/08 19:49:53, pauljensen wrote: > > > > > > Does this header exist? According to OkHttp changelog it was replace > > with > > > > > > OkHttp-Selected-Transport which was in turn later replaced with > > > > > > OkHttp-Selected-Protocol: > > > > > > http://androidxref.com/6.0.0_r1/xref/external/okhttp/CHANGELOG.md > > > > > > > > > > It was in here at some point: > > > > > > > > > > > > > > > https://android.googlesource.com/platform/external/okhttp/+/00834c9a00d53b29c... > > > > > > > > I agree it was there at some point, but it isn't anymore AFAIK. > > > > > > Old android code doesn't go away, it's still running on users devices for > > years. > > > > That may be so, but newer Android releases will fail here and need to be > fixed. > > > > > https://codereview.chromium.org/1492583002/diff/60001/components/cronet/andro... > > components/cronet/android/api/src/org/chromium/net/JavaUrlRequest.java:446: > > headerList.add(new SimpleEntry<>(entry.getKey(), value)); > > On 2015/12/15 22:32:35, Charles wrote: > > > On 2015/12/14 20:02:05, pauljensen wrote: > > > > On 2015/12/11 16:45:39, Charles wrote: > > > > > On 2015/12/08 19:49:53, pauljensen wrote: > > > > > > we could tally up the size of entry.getKey() and |value| here and put > > the > > > > sum > > > > > > into mUrlResponseInfo.setReceivedBytesCount (plus maybe 2 for each > > header > > > > (one > > > > > > for the ':' and one for the '\n')) > > > > > > > > > > We could, but I feel a little dishonest doing it, since the platform is > > free > > > > to > > > > > add its own headers that we don't know about. > > > > > > > > How is it dishonest? The received bytes count is just used to so some > > really > > > > simplistic data-usage accounting by embedders AFAIK. > > > > > > The simplistic data usage accounting is used to make decisions about what > > > features cost users what amount of money - I don't want to return > information > > > that's not correct. > > > > Returning zero is less information than returning something non-zero. This > > value is documented to be a lower bound. A higher lower bound is better than > a > > lower one. > That's true, but I can't keep it a lower bound and include the response body in > the count. I feel like it's better to just say that it's not supported at all, > with 0 or -1 or some other sentinel value. Ya, 0 is fine as it at least indicates the value is unknown/uncomputed.
https://codereview.chromium.org/1492583002/diff/60001/components/cronet/andro... File components/cronet/android/api/src/org/chromium/net/JavaUrlRequest.java (right): https://codereview.chromium.org/1492583002/diff/60001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/JavaUrlRequest.java:37: private static final String X_ANDROID_SELECTED_TRANSPORT = "X-Android-Selected-Transport"; On 2015/12/18 23:37:51, Charles wrote: > On 2015/12/17 19:59:49, pauljensen wrote: > > On 2015/12/15 22:32:35, Charles wrote: > > > On 2015/12/14 20:02:05, pauljensen wrote: > > > > On 2015/12/11 16:45:40, Charles wrote: > > > > > On 2015/12/08 19:49:53, pauljensen wrote: > > > > > > Does this header exist? According to OkHttp changelog it was replace > > with > > > > > > OkHttp-Selected-Transport which was in turn later replaced with > > > > > > OkHttp-Selected-Protocol: > > > > > > http://androidxref.com/6.0.0_r1/xref/external/okhttp/CHANGELOG.md > > > > > > > > > > It was in here at some point: > > > > > > > > > > > > > > > https://android.googlesource.com/platform/external/okhttp/+/00834c9a00d53b29c... > > > > > > > > I agree it was there at some point, but it isn't anymore AFAIK. > > > > > > Old android code doesn't go away, it's still running on users devices for > > years. > > > > That may be so, but newer Android releases will fail here and need to be > fixed. > > Tested. I assume that means the test passed on Marshmallow? It looks like Marshmallow may override this setting here: http://androidxref.com/6.0.0_r1/xref/external/okhttp/android/main/java/com/sq...
On 2015/12/21 19:31:24, pauljensen wrote: > https://codereview.chromium.org/1492583002/diff/60001/components/cronet/andro... > File components/cronet/android/api/src/org/chromium/net/JavaUrlRequest.java > (right): > > https://codereview.chromium.org/1492583002/diff/60001/components/cronet/andro... > components/cronet/android/api/src/org/chromium/net/JavaUrlRequest.java:37: > private static final String X_ANDROID_SELECTED_TRANSPORT = > "X-Android-Selected-Transport"; > On 2015/12/18 23:37:51, Charles wrote: > > On 2015/12/17 19:59:49, pauljensen wrote: > > > On 2015/12/15 22:32:35, Charles wrote: > > > > On 2015/12/14 20:02:05, pauljensen wrote: > > > > > On 2015/12/11 16:45:40, Charles wrote: > > > > > > On 2015/12/08 19:49:53, pauljensen wrote: > > > > > > > Does this header exist? According to OkHttp changelog it was > replace > > > with > > > > > > > OkHttp-Selected-Transport which was in turn later replaced with > > > > > > > OkHttp-Selected-Protocol: > > > > > > > http://androidxref.com/6.0.0_r1/xref/external/okhttp/CHANGELOG.md > > > > > > > > > > > > It was in here at some point: > > > > > > > > > > > > > > > > > > > > > https://android.googlesource.com/platform/external/okhttp/+/00834c9a00d53b29c... > > > > > > > > > > I agree it was there at some point, but it isn't anymore AFAIK. > > > > > > > > Old android code doesn't go away, it's still running on users devices for > > > years. > > > > > > That may be so, but newer Android releases will fail here and need to be > > fixed. > > > > Tested. > > I assume that means the test passed on Marshmallow? > It looks like Marshmallow may override this setting here: > http://androidxref.com/6.0.0_r1/xref/external/okhttp/android/main/java/com/sq... Yup, passed just fine on Marshmallow.
On 2015/12/21 19:31:24, pauljensen wrote: > https://codereview.chromium.org/1492583002/diff/60001/components/cronet/andro... > File components/cronet/android/api/src/org/chromium/net/JavaUrlRequest.java > (right): > > https://codereview.chromium.org/1492583002/diff/60001/components/cronet/andro... > components/cronet/android/api/src/org/chromium/net/JavaUrlRequest.java:37: > private static final String X_ANDROID_SELECTED_TRANSPORT = > "X-Android-Selected-Transport"; > On 2015/12/18 23:37:51, Charles wrote: > > On 2015/12/17 19:59:49, pauljensen wrote: > > > On 2015/12/15 22:32:35, Charles wrote: > > > > On 2015/12/14 20:02:05, pauljensen wrote: > > > > > On 2015/12/11 16:45:40, Charles wrote: > > > > > > On 2015/12/08 19:49:53, pauljensen wrote: > > > > > > > Does this header exist? According to OkHttp changelog it was > replace > > > with > > > > > > > OkHttp-Selected-Transport which was in turn later replaced with > > > > > > > OkHttp-Selected-Protocol: > > > > > > > http://androidxref.com/6.0.0_r1/xref/external/okhttp/CHANGELOG.md > > > > > > > > > > > > It was in here at some point: > > > > > > > > > > > > > > > > > > > > > https://android.googlesource.com/platform/external/okhttp/+/00834c9a00d53b29c... > > > > > > > > > > I agree it was there at some point, but it isn't anymore AFAIK. > > > > > > > > Old android code doesn't go away, it's still running on users devices for > > > years. > > > > > > That may be so, but newer Android releases will fail here and need to be > > fixed. > > > > Tested. > > I assume that means the test passed on Marshmallow? > It looks like Marshmallow may override this setting here: > http://androidxref.com/6.0.0_r1/xref/external/okhttp/android/main/java/com/sq... Yup, passed just fine on Marshmallow.
https://codereview.chromium.org/1492583002/diff/80001/components/cronet/andro... File components/cronet/android/api/src/org/chromium/net/JavaUrlRequest.java (right): https://codereview.chromium.org/1492583002/diff/80001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/JavaUrlRequest.java:228: mInitialMethod = "POST"; On 2015/12/15 22:32:36, Charles wrote: > On 2015/12/14 20:02:06, pauljensen wrote: > > nit: I like calling setHttpMethod() here in case it does something more than > > just setting mInitialMethod, but this is probably just a personal preference. > > Either way is fine. > > I don't create the HttpUrlConnection instance to call this on until start() is > called, and I don't want to move that earlier, when it's happening on an > arbitrary thread. I was talking about JavaUrlRequest.setHttpMethod() which doesn't create the HttpUrlConnection. https://codereview.chromium.org/1492583002/diff/80001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/JavaUrlRequest.java:429: String selectedTransport = "http/1.1"; On 2015/12/15 22:32:36, Charles wrote: > On 2015/12/14 20:02:05, pauljensen wrote: > > I'm not sure it's a good idea to make this default to a believable value; this > > is likely to cover up problems (like X-Android header names changing). > > All of that behavior is undocumented and best-effort anyway. What would you like > the behavior to be? The API is documented like so: "Returns an empty string if no protocol was negotiated, the protocol is not known, or when using plain HTTP or HTTPS." and considering that the system's HttpURLConnection impl doesn't support QUIC or SPDY (disabled by Android), I think we may always want to just return an empty string always, not just in the default case.
I'm pretty happy with the impl here, my comments are mostly on the tests now. It's unclear to me why some of them are disabled for the Java impl. https://codereview.chromium.org/1492583002/diff/140001/components/cronet/andr... File components/cronet/android/api/src/org/chromium/net/UrlResponseInfo.java (right): https://codereview.chromium.org/1492583002/diff/140001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/UrlResponseInfo.java:210: // Prevent asserting on the contents of this string I don't understand this comment. How does adding code here cause an assert on the contents of this string? https://codereview.chromium.org/1492583002/diff/140001/components/cronet/andr... File components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java (right): https://codereview.chromium.org/1492583002/diff/140001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java:270: Preconditions.checkDirect(buffer); could you add this line on line 234 and adjust the comment on line 259 and error code on line 262? https://codereview.chromium.org/1492583002/diff/140001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java:287: // not a direct ByteBuffer. please update this comment https://codereview.chromium.org/1492583002/diff/140001/components/cronet/andr... File components/cronet/android/test/javatests/src/org/chromium/net/CronetUploadTest.java (right): https://codereview.chromium.org/1492583002/diff/140001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/CronetUploadTest.java:53: @OnlyRunNativeCronet why are all of our upload tests disabled? can we add a comment explaining this? should we allow adding an annotation to the class? this might be useful for QuicTest and SdchTest also. https://codereview.chromium.org/1492583002/diff/140001/components/cronet/andr... File components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java (right): https://codereview.chromium.org/1492583002/diff/140001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java:128: @OnlyRunNativeCronet why is this test disabled? please add a comment https://codereview.chromium.org/1492583002/diff/140001/components/cronet/andr... File components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestTest.java (left): https://codereview.chromium.org/1492583002/diff/140001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestTest.java:1426: assertEquals(callback.mResponseStep, failureStep); why was this removed? https://codereview.chromium.org/1492583002/diff/140001/components/cronet/andr... File components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestTest.java (right): https://codereview.chromium.org/1492583002/diff/140001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestTest.java:515: @OnlyRunNativeCronet // No canonical exception to assert on I think a lot of these comments could be more accurate if they simply said "Java impl doesn't support MockUrlRequestJobFactory" https://codereview.chromium.org/1492583002/diff/140001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestTest.java:1383: @OnlyRunNativeCronet // No canonical exception to assert on can we at least test that some error is generated? perhaps add a boolean like mTestingSystemHttpURLConnection that tests can decide upon whether to test the exact error code https://codereview.chromium.org/1492583002/diff/140001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestTest.java:1407: System.out.println("Testing " + failureType + " during " + failureStep); is this still necessary? https://codereview.chromium.org/1492583002/diff/140001/components/cronet/andr... File components/cronet/android/test/javatests/src/org/chromium/net/PkpTest.java (right): https://codereview.chromium.org/1492583002/diff/140001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/PkpTest.java:287: @OnlyRunNativeCronet shouldn't this test pass as the checks are in CronetEngine.Builder? ditto for the test on line 232 and 300 also
https://codereview.chromium.org/1492583002/diff/140001/components/cronet/andr... File components/cronet/android/api/src/org/chromium/net/UrlResponseInfo.java (right): https://codereview.chromium.org/1492583002/diff/140001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/UrlResponseInfo.java:210: // Prevent asserting on the contents of this string On 2015/12/21 20:27:58, pauljensen wrote: > I don't understand this comment. How does adding code here cause an assert on > the contents of this string? This is a common pattern in java - including the identity hashcode of the object (which is derived from its memory address) means that the toString() of this object is different for each object instance, and is unpredictable. It prevents people from writing the types of brittle tests that I replaced in this CL, which compare an object to a string literal. https://codereview.chromium.org/1492583002/diff/140001/components/cronet/andr... File components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java (right): https://codereview.chromium.org/1492583002/diff/140001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java:270: Preconditions.checkDirect(buffer); On 2015/12/21 20:27:58, pauljensen wrote: > could you add this line on line 234 and adjust the comment on line 259 and error > code on line 262? Done. https://codereview.chromium.org/1492583002/diff/140001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java:287: // not a direct ByteBuffer. On 2015/12/21 20:27:58, pauljensen wrote: > please update this comment Done. https://codereview.chromium.org/1492583002/diff/140001/components/cronet/andr... File components/cronet/android/test/javatests/src/org/chromium/net/CronetUploadTest.java (right): https://codereview.chromium.org/1492583002/diff/140001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/CronetUploadTest.java:53: @OnlyRunNativeCronet On 2015/12/21 20:27:58, pauljensen wrote: > why are all of our upload tests disabled? can we add a comment explaining this? > should we allow adding an annotation to the class? this might be useful for > QuicTest and SdchTest also. Annotation for the class is a matter of preference, although it's easier to do per-method, and also encourages people adding new tests to consider the environment being tested. Also has less chance of losing test coverage by moving tests between classes. These are all disabled because they're testing CronetUploadDatastream, which is a peer for a native object. https://codereview.chromium.org/1492583002/diff/140001/components/cronet/andr... File components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java (right): https://codereview.chromium.org/1492583002/diff/140001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java:128: @OnlyRunNativeCronet On 2015/12/21 20:27:58, pauljensen wrote: > why is this test disabled? please add a comment Done. https://codereview.chromium.org/1492583002/diff/140001/components/cronet/andr... File components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestTest.java (left): https://codereview.chromium.org/1492583002/diff/140001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestTest.java:1426: assertEquals(callback.mResponseStep, failureStep); On 2015/12/21 20:27:58, pauljensen wrote: > why was this removed? Done. https://codereview.chromium.org/1492583002/diff/140001/components/cronet/andr... File components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestTest.java (right): https://codereview.chromium.org/1492583002/diff/140001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestTest.java:515: @OnlyRunNativeCronet // No canonical exception to assert on On 2015/12/21 20:27:58, pauljensen wrote: > I think a lot of these comments could be more accurate if they simply said "Java > impl doesn't support MockUrlRequestJobFactory" Done. https://codereview.chromium.org/1492583002/diff/140001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestTest.java:1383: @OnlyRunNativeCronet // No canonical exception to assert on On 2015/12/21 20:27:58, pauljensen wrote: > can we at least test that some error is generated? perhaps add a boolean like > mTestingSystemHttpURLConnection that tests can decide upon whether to test the > exact error code Done. https://codereview.chromium.org/1492583002/diff/140001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestTest.java:1407: System.out.println("Testing " + failureType + " during " + failureStep); On 2015/12/21 20:27:58, pauljensen wrote: > is this still necessary? Not strictly necessary, but very very helpful for figuring out which of these many tests-in-a-test actually failed. https://codereview.chromium.org/1492583002/diff/140001/components/cronet/andr... File components/cronet/android/test/javatests/src/org/chromium/net/PkpTest.java (right): https://codereview.chromium.org/1492583002/diff/140001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/PkpTest.java:287: @OnlyRunNativeCronet On 2015/12/21 20:27:58, pauljensen wrote: > shouldn't this test pass as the checks are in CronetEngine.Builder? ditto for > the test on line 232 and 300 also Done.
https://codereview.chromium.org/1492583002/diff/140001/components/cronet/andr... File components/cronet/android/api/src/org/chromium/net/UrlResponseInfo.java (right): https://codereview.chromium.org/1492583002/diff/140001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/UrlResponseInfo.java:210: // Prevent asserting on the contents of this string On 2015/12/21 23:36:25, Charles wrote: > On 2015/12/21 20:27:58, pauljensen wrote: > > I don't understand this comment. How does adding code here cause an assert on > > the contents of this string? > > This is a common pattern in java - including the identity hashcode of the object > (which is derived from its memory address) means that the toString() of this > object is different for each object instance, and is unpredictable. It prevents > people from writing the types of brittle tests that I replaced in this CL, which > compare an object to a string literal. Acknowledged. https://codereview.chromium.org/1492583002/diff/140001/components/cronet/andr... File components/cronet/android/test/javatests/src/org/chromium/net/CronetUploadTest.java (right): https://codereview.chromium.org/1492583002/diff/140001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/CronetUploadTest.java:53: @OnlyRunNativeCronet On 2015/12/21 23:36:26, Charles wrote: > On 2015/12/21 20:27:58, pauljensen wrote: > > why are all of our upload tests disabled? can we add a comment explaining > this? > > should we allow adding an annotation to the class? this might be useful for > > QuicTest and SdchTest also. > > Annotation for the class is a matter of preference, although it's easier to do > per-method, and also encourages people adding new tests to consider the > environment being tested. Also has less chance of losing test coverage by moving > tests between classes. SGTM > > These are all disabled because they're testing CronetUploadDatastream, which is > a peer for a native object. SGTM, looks like CronetUrlRequestTest.java tests uploading. https://codereview.chromium.org/1492583002/diff/140001/components/cronet/andr... File components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java (right): https://codereview.chromium.org/1492583002/diff/140001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java:128: @OnlyRunNativeCronet On 2015/12/21 23:36:26, Charles wrote: > On 2015/12/21 20:27:58, pauljensen wrote: > > why is this test disabled? please add a comment > > Done. We really need a test that the user-agent is set properly for the Java impl. This is quite important and has a good chance of failure and completely untested. Hmm. If we re-enliven the legacyMode() plumbing as I mentioned in another comment, we could use that to run this test on both impls. https://codereview.chromium.org/1492583002/diff/140001/components/cronet/andr... File components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestTest.java (right): https://codereview.chromium.org/1492583002/diff/140001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestTest.java:1407: System.out.println("Testing " + failureType + " during " + failureStep); On 2015/12/21 23:36:26, Charles wrote: > On 2015/12/21 20:27:58, pauljensen wrote: > > is this still necessary? > > Not strictly necessary, but very very helpful for figuring out which of these > many tests-in-a-test actually failed. I think Chromium generally likes their tests to not print anything. Can you comment this out or enable it only for verbose logging? https://codereview.chromium.org/1492583002/diff/160001/components/cronet/andr... File components/cronet/android/api/src/org/chromium/net/CronetEngine.java (right): https://codereview.chromium.org/1492583002/diff/160001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:837: cronetEngine = new JavaCronetEngine(builder.getUserAgent()); I'm worried about this automatic silent fallback. I dislike silent fallback because embedders might try out Cronet and think it doesn't perform any better than their existing HttpURLConnection code, but they could silently be using the HttpURLConnection impl of Cronet. Perhaps we should use the legacyMode() plumbing we already have to control this fallback? This code could be something like: if (builder.legacyMode()) { cronetEngine = new JavaCronetEngine(builder.getUserAgent()); } else { cronetEngine = createCronetEngine(builder); } https://codereview.chromium.org/1492583002/diff/160001/components/cronet/andr... File components/cronet/android/test/javatests/src/org/chromium/net/CronetTestBase.java (right): https://codereview.chromium.org/1492583002/diff/160001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/CronetTestBase.java:108: mTestingSystemHttpURLConnection = false; I'm a little uncertain of the lifetimes of these classes, can you either set mTestingJavaImpl to false here or assert that it is false? https://codereview.chromium.org/1492583002/diff/160001/components/cronet/andr... File components/cronet/android/test/javatests/src/org/chromium/net/CronetUploadTest.java (right): https://codereview.chromium.org/1492583002/diff/160001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/CronetUploadTest.java:21: * why'd you add an empty line?
https://codereview.chromium.org/1492583002/diff/140001/components/cronet/andr... File components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestTest.java (right): https://codereview.chromium.org/1492583002/diff/140001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestTest.java:1407: System.out.println("Testing " + failureType + " during " + failureStep); On 2015/12/29 16:35:53, pauljensen wrote: > On 2015/12/21 23:36:26, Charles wrote: > > On 2015/12/21 20:27:58, pauljensen wrote: > > > is this still necessary? > > > > Not strictly necessary, but very very helpful for figuring out which of these > > many tests-in-a-test actually failed. > > I think Chromium generally likes their tests to not print anything. Can you > comment this out or enable it only for verbose logging? Done. https://codereview.chromium.org/1492583002/diff/160001/components/cronet/andr... File components/cronet/android/api/src/org/chromium/net/CronetEngine.java (right): https://codereview.chromium.org/1492583002/diff/160001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:837: cronetEngine = new JavaCronetEngine(builder.getUserAgent()); On 2015/12/29 16:35:53, pauljensen wrote: > I'm worried about this automatic silent fallback. I dislike silent fallback > because embedders might try out Cronet and think it doesn't perform any better > than their existing HttpURLConnection code, but they could silently be using the > HttpURLConnection impl of Cronet. Perhaps we should use the legacyMode() > plumbing we already have to control this fallback? This code could be something > like: > > if (builder.legacyMode()) { > cronetEngine = new JavaCronetEngine(builder.getUserAgent()); > } else { > cronetEngine = createCronetEngine(builder); > } The problem with that is that you now need a code change in order to use the java impl - this means that you have to have two different source files when building, rather than making the decision based on the structure of the build graph alone. If you want, I could add a log statement that provides an explicit warning about the fact that we're loading the legacy version of the stack. Would assuage your concerns? https://codereview.chromium.org/1492583002/diff/160001/components/cronet/andr... File components/cronet/android/test/javatests/src/org/chromium/net/CronetTestBase.java (right): https://codereview.chromium.org/1492583002/diff/160001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/CronetTestBase.java:108: mTestingSystemHttpURLConnection = false; On 2015/12/29 16:35:53, pauljensen wrote: > I'm a little uncertain of the lifetimes of these classes, can you either set > mTestingJavaImpl to false here or assert that it is false? Done. https://codereview.chromium.org/1492583002/diff/160001/components/cronet/andr... File components/cronet/android/test/javatests/src/org/chromium/net/CronetUploadTest.java (right): https://codereview.chromium.org/1492583002/diff/160001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/CronetUploadTest.java:21: * On 2015/12/29 16:35:53, pauljensen wrote: > why'd you add an empty line? Done.
pauljensen@chromium.org changed reviewers: + mef@chromium.org
+Misha Misha, as I mentioned to you earlier, Charles would like to enable the automatic fallback to the Java-impl of the async API. I'm worried about embedders building with cronet_api.jar and not cronet.jar and not seeing any benefits from Cronet, or an inadvertent build system change making the native impl inaccessible. Charles' concern is that this would require embedders to explicitly enable the fallback. I'm wondering if upcoming more centralized Cronet distribution mechanisms will make this moot (because the jars will always be shipped together) and we should just enable the automatic fallback now. I just don't want the first question I have for every Cronet embedder to be "are you using the real native impl?"
On 2016/01/06 14:50:35, pauljensen wrote: > +Misha > > Misha, as I mentioned to you earlier, Charles would like to enable the automatic > fallback to the Java-impl of the async API. I'm worried about embedders > building with cronet_api.jar and not cronet.jar and not seeing any benefits from > Cronet, or an inadvertent build system change making the native impl > inaccessible. Charles' concern is that this would require embedders to > explicitly enable the fallback. Looking at existing handling of legacy mode by legacy api here https://code.google.com/p/chromium/codesearch#chromium/src/components/cronet/... we do silently fallback if native factory cannot be created. There is a log statement showing the chosen stack. In fact the CronetEngine.Builder.enableLegacyMode() comment seems incorrect: https://code.google.com/p/chromium/codesearch#chromium/src/components/cronet/... > I'm wondering if upcoming more centralized Cronet distribution mechanisms will > make this moot (because the jars will always be shipped together) and we should > just enable the automatic fallback now. I think so. > I just don't want the first question I have for every Cronet embedder to be "are > you using the real native impl?" We haven't had too many of those, maybe because our first question usually is 'can we have a net log?', and java fallbacks don't do that...
PTAL. I think this is finally ready for submission. https://codereview.chromium.org/1492583002/diff/140001/components/cronet/andr... File components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java (right): https://codereview.chromium.org/1492583002/diff/140001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java:128: @OnlyRunNativeCronet On 2015/12/29 16:35:53, pauljensen wrote: > On 2015/12/21 23:36:26, Charles wrote: > > On 2015/12/21 20:27:58, pauljensen wrote: > > > why is this test disabled? please add a comment > > > > Done. > > We really need a test that the user-agent is set properly for the Java impl. > This is quite important and has a good chance of failure and completely > untested. Hmm. If we re-enliven the legacyMode() plumbing as I mentioned in > another comment, we could use that to run this test on both impls. Done.
My final set of comments before approving. https://codereview.chromium.org/1492583002/diff/200001/components/cronet/andr... File components/cronet/android/test/javatests/src/org/chromium/net/CronetTestBase.java (right): https://codereview.chromium.org/1492583002/diff/200001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/CronetTestBase.java:190: public @interface OnlyRunNativeCronet {} can you make these braces match those on lines 180 and 185? https://codereview.chromium.org/1492583002/diff/200001/components/cronet/andr... File components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestTest.java (right): https://codereview.chromium.org/1492583002/diff/200001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestTest.java:1387: UrlRequest.Builder builder = new UrlRequest.Builder("http://127.0.0.1:12345", callback, why did you change this and line 1391? I admit the previous code wasn't safe (NativeTestServer probably bound to some random port, and once shutdown another server was probably free to bind to that port, thus breaking the test potentially). But picking port 12345 and assuming it's unused is also a bit unsafe. If you really think using port 12345 is better, please remove lines 1389-1391. https://codereview.chromium.org/1492583002/diff/240001/components/cronet/andr... File components/cronet/android/api/src/org/chromium/net/InputStreamChannel.java (right): https://codereview.chromium.org/1492583002/diff/240001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/InputStreamChannel.java:27: this.mInputStream = inputStream; nit: we generally don't prefix "this." when the member starts with an "m" and the argument doesn't. up to you. https://codereview.chromium.org/1492583002/diff/240001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/InputStreamChannel.java:53: Math.max(mInputStream.available(), MIN_TMP_BUFFER_SIZE), dst.remaining()); Just to verify: the behavior you want here is: if mInputStream.available() is 1 but dst.remaining() is infinity, a buffer of size 4096. if mInputStream.available() is infinity but dst.remaining() is 1, a buffer of size 1. https://codereview.chromium.org/1492583002/diff/240001/components/cronet/andr... File components/cronet/android/api/src/org/chromium/net/JavaUrlRequest.java (right): https://codereview.chromium.org/1492583002/diff/240001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/JavaUrlRequest.java:34: @TargetApi(Build.VERSION_CODES.ICE_CREAM_SANDWICH_MR1) // TrafficStats only available on ICS ICS MR1 is API level 15. TrafficStats was added in API level 8 and pieces of it were added in API level 14. I wouldn't care normally, but I think Cronet is intended to work on ICS MR0, so can we just crank this back to ICS MR0? https://codereview.chromium.org/1492583002/diff/240001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/JavaUrlRequest.java:130: throw new NullPointerException("Executor is required"); nit: Executor->userExecutor so as to make it distinct from exception throw three lines above https://codereview.chromium.org/1492583002/diff/240001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/JavaUrlRequest.java:347: mAdditionalStatusDetails = Status.WAITING_FOR_RESPONSE; can we move this line into fireGetHeaders() as it proceeds both calls to fireGetHeaders()? https://codereview.chromium.org/1492583002/diff/240001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/JavaUrlRequest.java:473: if (!headerKey.startsWith("X-Android")) { Move "X-Android" into a private final static String like X_ANDROID_SELECTED_TRANSPORT
PTAL https://codereview.chromium.org/1492583002/diff/200001/components/cronet/andr... File components/cronet/android/test/javatests/src/org/chromium/net/CronetTestBase.java (right): https://codereview.chromium.org/1492583002/diff/200001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/CronetTestBase.java:190: public @interface OnlyRunNativeCronet {} On 2016/01/07 04:33:38, pauljensen wrote: > can you make these braces match those on lines 180 and 185? Done. https://codereview.chromium.org/1492583002/diff/200001/components/cronet/andr... File components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestTest.java (right): https://codereview.chromium.org/1492583002/diff/200001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestTest.java:1387: UrlRequest.Builder builder = new UrlRequest.Builder("http://127.0.0.1:12345", callback, On 2016/01/07 04:33:38, pauljensen wrote: > why did you change this and line 1391? I admit the previous code wasn't safe > (NativeTestServer probably bound to some random port, and once shutdown another > server was probably free to bind to that port, thus breaking the test > potentially). But picking port 12345 and assuming it's unused is also a bit > unsafe. If you really think using port 12345 is better, please remove lines > 1389-1391. Shutting down the native test server causes getEchoBodyUrl to abort with a CHECK failure the next time this test is called, because the server isn't running. I've now chosen a reserved port that only accepts UDP, so we should be in good shape. https://codereview.chromium.org/1492583002/diff/240001/components/cronet/andr... File components/cronet/android/api/src/org/chromium/net/InputStreamChannel.java (right): https://codereview.chromium.org/1492583002/diff/240001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/InputStreamChannel.java:27: this.mInputStream = inputStream; On 2016/01/07 04:33:38, pauljensen wrote: > nit: we generally don't prefix "this." when the member starts with an "m" and > the argument doesn't. up to you. Done. https://codereview.chromium.org/1492583002/diff/240001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/InputStreamChannel.java:53: Math.max(mInputStream.available(), MIN_TMP_BUFFER_SIZE), dst.remaining()); On 2016/01/07 04:33:38, pauljensen wrote: > Just to verify: the behavior you want here is: > if mInputStream.available() is 1 but dst.remaining() is infinity, a buffer of > size 4096. > if mInputStream.available() is infinity but dst.remaining() is 1, a buffer of > size 1. Correct. That way, we never allocate an enormous buffer (imagine that dst is a 1gb memory-mapped file). https://codereview.chromium.org/1492583002/diff/240001/components/cronet/andr... File components/cronet/android/api/src/org/chromium/net/JavaUrlRequest.java (right): https://codereview.chromium.org/1492583002/diff/240001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/JavaUrlRequest.java:34: @TargetApi(Build.VERSION_CODES.ICE_CREAM_SANDWICH_MR1) // TrafficStats only available on ICS On 2016/01/07 04:33:38, pauljensen wrote: > ICS MR1 is API level 15. TrafficStats was added in API level 8 and pieces of it > were added in API level 14. I wouldn't care normally, but I think Cronet is > intended to work on ICS MR0, so can we just crank this back to ICS MR0? Interesting. There's basically no reason to support MR0, since it's less than 0.1%. In fact, I think only one phone ever launched with it, which was the Galaxy Nexus, and that got an OTA within weeks updating it to MR1. Done. https://codereview.chromium.org/1492583002/diff/240001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/JavaUrlRequest.java:130: throw new NullPointerException("Executor is required"); On 2016/01/07 04:33:38, pauljensen wrote: > nit: Executor->userExecutor > so as to make it distinct from exception throw three lines above Done. https://codereview.chromium.org/1492583002/diff/240001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/JavaUrlRequest.java:347: mAdditionalStatusDetails = Status.WAITING_FOR_RESPONSE; On 2016/01/07 04:33:38, pauljensen wrote: > can we move this line into fireGetHeaders() as it proceeds both calls to > fireGetHeaders()? Done. https://codereview.chromium.org/1492583002/diff/240001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/JavaUrlRequest.java:473: if (!headerKey.startsWith("X-Android")) { On 2016/01/07 04:33:38, pauljensen wrote: > Move "X-Android" into a private final static String like > X_ANDROID_SELECTED_TRANSPORT Done.
lgtm! Thanks again for writing this! nit: you can remove the "BUG=" line from the CL description as it doesn't add anything. nit: it would be nice if the CL description included a description of the Java impl, including when it's used and what its limitations are. FYI: lgtm's in Chrome are always sticky, so feel free to check the CQ box without further approval even after uploading patch sets to address remaining nits. If you make any changes beyond addressing nits feel free to ask me to review them. FYI: There is no CQ trybot for Cronet, but there are Cronet waterfall bots, so the CQ will happily commit something and the Cronet waterfall bots will happily email you if it fails. So I always suggest running the Cronet tests before CQing. https://codereview.chromium.org/1492583002/diff/240001/components/cronet/andr... File components/cronet/android/api/src/org/chromium/net/JavaUrlRequest.java (right): https://codereview.chromium.org/1492583002/diff/240001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/JavaUrlRequest.java:34: @TargetApi(Build.VERSION_CODES.ICE_CREAM_SANDWICH_MR1) // TrafficStats only available on ICS On 2016/01/07 16:10:02, Charles wrote: > On 2016/01/07 04:33:38, pauljensen wrote: > > ICS MR1 is API level 15. TrafficStats was added in API level 8 and pieces of > it > > were added in API level 14. I wouldn't care normally, but I think Cronet is > > intended to work on ICS MR0, so can we just crank this back to ICS MR0? > > Interesting. There's basically no reason to support MR0, since it's less than > 0.1%. In fact, I think only one phone ever launched with it, which was the > Galaxy Nexus, and that got an OTA within weeks updating it to MR1. > > Done. Agreed, I didn't set the MR0 requirement, I just enforce it :) https://codereview.chromium.org/1492583002/diff/260001/components/cronet/andr... File components/cronet/android/api/src/org/chromium/net/JavaCronetEngine.java (right): https://codereview.chromium.org/1492583002/diff/260001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/JavaCronetEngine.java:55: this.mUserAgent = userAgent; nit: this.m -> m https://codereview.chromium.org/1492583002/diff/260001/components/cronet/andr... File components/cronet/android/api/src/org/chromium/net/JavaUrlRequest.java (right): https://codereview.chromium.org/1492583002/diff/260001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/JavaUrlRequest.java:133: this.mCallbackAsync = new AsyncUrlRequestCallback(callback, userExecutor); nit: this.m -> m
Description was changed from ========== Add HttpUrlConnection backed implementation of CronetEngine. BUG= ========== to ========== Add HttpUrlConnection backed implementation of CronetEngine. When the native implementation is not available (because it wasn't included in the binary), cronet falls back to a HttpUrlConnection based implementation of the async API. ==========
https://codereview.chromium.org/1492583002/diff/260001/components/cronet/andr... File components/cronet/android/api/src/org/chromium/net/JavaCronetEngine.java (right): https://codereview.chromium.org/1492583002/diff/260001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/JavaCronetEngine.java:55: this.mUserAgent = userAgent; On 2016/01/07 17:00:00, pauljensen wrote: > nit: this.m -> m Done everywhere
The CQ bit was checked by pauljensen@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1492583002/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1492583002/260001
Message was sent while issue was closed.
Description was changed from ========== Add HttpUrlConnection backed implementation of CronetEngine. When the native implementation is not available (because it wasn't included in the binary), cronet falls back to a HttpUrlConnection based implementation of the async API. ========== to ========== Add HttpUrlConnection backed implementation of CronetEngine. When the native implementation is not available (because it wasn't included in the binary), cronet falls back to a HttpUrlConnection based implementation of the async API. ==========
Message was sent while issue was closed.
Committed patchset #14 (id:260001)
Message was sent while issue was closed.
Description was changed from ========== Add HttpUrlConnection backed implementation of CronetEngine. When the native implementation is not available (because it wasn't included in the binary), cronet falls back to a HttpUrlConnection based implementation of the async API. ========== to ========== Add HttpUrlConnection backed implementation of CronetEngine. When the native implementation is not available (because it wasn't included in the binary), cronet falls back to a HttpUrlConnection based implementation of the async API. Committed: https://crrev.com/5632bf5e2836df74ca4d92e98c05540d6b4e62f2 Cr-Commit-Position: refs/heads/master@{#368127} ==========
Message was sent while issue was closed.
Patchset 14 (id:??) landed as https://crrev.com/5632bf5e2836df74ca4d92e98c05540d6b4e62f2 Cr-Commit-Position: refs/heads/master@{#368127}
Message was sent while issue was closed.
https://codereview.chromium.org/1492583002/diff/60001/components/cronet/andro... File components/cronet/android/api/src/org/chromium/net/InputStreamChannel.java (right): https://codereview.chromium.org/1492583002/diff/60001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/InputStreamChannel.java:57: byte[] tmpBuf = new byte[Math.min(reasonableToRead, MAX_TMP_BUFFER_SIZE)]; On 2015/12/11 16:45:39, Charles wrote: > On 2015/12/08 19:49:52, pauljensen wrote: > > it's interesting that you marked your ints final but not your byte[]. I get > > int's are primitive and an array is a reference. Is there a style guide > > recommendation for this? otherwise it seems inconsistent. > > The style guide is "don't put final on local variables, unless it's required or > it makes the code easier to follow". I tend to interpret that in a way that uses > final on variables in three cases: > > 1. Variable is accessed in an anonymous class (has to be final) > 2. Variable is declared, and there's some complex code that initializes it - by > declaring it final, the compiler proves that it can only be initialized once, > and that it must be initialized. The "final int read" up earlier lets you skip > checking that its results might not be populated or might be overwritten. > 3. Variable is in a mix of other, similarly typed variables. I could see myself > accidentally reassigning possibleToRead or reasonableToRead, since there are a > bunch of ints floating around those three lines of code, but that case is a > little less defensible. > > I didn't mark tmpBuf final, because it's the only byte[] in that area of code, > and the code from when it's declared to when it goes out of scope is very > simple, and there's little opportunity for confusion. Charles, what is "The style guide" you are referring to? |
