Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1359343005:
Update ResponseInfo to UrlResponseInfo with API review comments. (Closed)
DescriptionUpdate ResponseInfo to UrlResponseInfo with API review comments.
Deprecate ResponseInfo and ExtendedResponseInfo interfaces.
BUG=531538
Committed: https://crrev.com/5f36625b0b053d15ed4d65c7c63feab4ecadfe5c
Cr-Commit-Position: refs/heads/master@{#354515}
Patch Set 1 #Patch Set 2 : Pass the integration tests. #
Total comments: 25
Patch Set 3 : Change UrlRequestListener to take UrlResponseInfo. #Patch Set 4 : Remove ResponseInfo and ExtendedResponseInfo. #Patch Set 5 : Sync #Patch Set 6 : Sync #Patch Set 7 : fix perf_test #Patch Set 8 : Rebase to CronetEngine builders. #
Total comments: 14
Patch Set 9 : Sync #Patch Set 10 : Address Helen's comments. #Patch Set 11 : Updated comment. #Patch Set 12 : Update readBytesCount on redirects, reads and failures. #Patch Set 13 : Accumulate Received Bytes Count on redirects. #
Total comments: 12
Patch Set 14 : Rebase onto HEAD #Patch Set 15 : Address Paul's comments. #
Total comments: 5
Patch Set 16 : Fixed links in comments. #
Total comments: 2
Patch Set 17 : Added receivedBytesCount comment. #
Total comments: 8
Patch Set 18 : Address final comments. #Patch Set 19 : Update README.md #Messages
Total messages: 44 (4 generated)
mef@chromium.org changed reviewers: + pauljensen@chromium.org, xunjieli@chromium.org
PTAL. I've added UrlResponseInfo final class, which implements ResponseInfo interface, which I've marked as deprecated. I haven't changed RequestListener to take UrlResponseInfo as I think it makes sense to be done together with making listener an inner class of UrlRequest. WDYT?
One question for Paul below. https://codereview.chromium.org/1359343005/diff/20001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java (right): https://codereview.chromium.org/1359343005/diff/20001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java:210: mUrlRequestAdapter, header.getKey(), header.getValue())) { The more I looked at this, the more I think this is inefficient. If we have a lot of headers, there are multiple JNI calls. Maybe we can pass a jobjectArray and use base::android::AppendJavaStringArrayToStringVector directly in one single JNI call. Similarly, we can get rid of onAppendResponseHeader, and do the reverse. Paul, what do you think? https://codereview.chromium.org/1359343005/diff/20001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/UrlResponseInfo.java (right): https://codereview.chromium.org/1359343005/diff/20001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/UrlResponseInfo.java:28: private volatile long mReceivedBytesCount; We are only updating this on the network thread. Our embedders will query new values every time using getReceivedBytesCount. Why does this need to be volatile? https://codereview.chromium.org/1359343005/diff/20001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/UrlResponseInfo.java:100: public Map<String, List<String>> getAllHeaders() { It's sad that even though we have Map.Entry, we still do all these extra work to construct a Map because there can be multiple headers of the same name. I wonder how exactly Map.Entry is going to help our embedders. https://codereview.chromium.org/1359343005/diff/20001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/UrlResponseInfo.java:171: void updateReceivedBytesCount(long currentReceivedBytesCount) { Maybe rename this method as "setReceivedBytesCount" to make it clearer that this is setting the variable and not incrementing/decrementing. Need comments to explain why the visibility is package-protected.
https://codereview.chromium.org/1359343005/diff/20001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java (right): https://codereview.chromium.org/1359343005/diff/20001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java:210: mUrlRequestAdapter, header.getKey(), header.getValue())) { On 2015/09/25 13:54:32, xunjieli wrote: > The more I looked at this, the more I think this is inefficient. If we have a > lot of headers, there are multiple JNI calls. Maybe we can pass a jobjectArray > and use base::android::AppendJavaStringArrayToStringVector directly in one > single JNI call. Do you mean make 2 string arrays for keys and values? That might work. > Similarly, we can get rid of onAppendResponseHeader, and do the reverse. IIUIC JNI calls up from C++ to Java are more expensive than down from Java to C++, so getting rid of onAppendResponseHeader sounds like a noble goal. > > Paul, what do you think? https://codereview.chromium.org/1359343005/diff/20001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/UrlResponseInfo.java (right): https://codereview.chromium.org/1359343005/diff/20001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/UrlResponseInfo.java:28: private volatile long mReceivedBytesCount; On 2015/09/25 13:54:32, xunjieli wrote: > We are only updating this on the network thread. Our embedders will query new > values every time using getReceivedBytesCount. Why does this need to be > volatile? Um, what if the app holds on to UrlResponseInfo and accesses it while Cronet is updating value on network thread? https://codereview.chromium.org/1359343005/diff/20001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/UrlResponseInfo.java:100: public Map<String, List<String>> getAllHeaders() { On 2015/09/25 13:54:32, xunjieli wrote: > It's sad that even though we have Map.Entry, we still do all these extra work to > construct a Map because there can be multiple headers of the same name. I wonder > how exactly Map.Entry is going to help our embedders. Theoretically it will help if they want to headers into multimap<String, String> instead of Map<String, List<String>>. https://codereview.chromium.org/1359343005/diff/20001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/UrlResponseInfo.java:171: void updateReceivedBytesCount(long currentReceivedBytesCount) { On 2015/09/25 13:54:32, xunjieli wrote: > Maybe rename this method as "setReceivedBytesCount" to make it clearer that this > is setting the variable and not incrementing/decrementing. > Need comments to explain why the visibility is package-protected. Sounds good, will do. I also wonder whether it should be updated for every onReadCompleted callback.
https://codereview.chromium.org/1359343005/diff/20001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java (right): https://codereview.chromium.org/1359343005/diff/20001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java:210: mUrlRequestAdapter, header.getKey(), header.getValue())) { On 2015/09/25 15:58:27, mef wrote: > On 2015/09/25 13:54:32, xunjieli wrote: > > The more I looked at this, the more I think this is inefficient. If we have a > > lot of headers, there are multiple JNI calls. Maybe we can pass a jobjectArray > > and use base::android::AppendJavaStringArrayToStringVector directly in one > > single JNI call. > Do you mean make 2 string arrays for keys and values? That might work. > > > Similarly, we can get rid of onAppendResponseHeader, and do the reverse. > IIUIC JNI calls up from C++ to Java are more expensive than down from Java to > C++, so getting rid of onAppendResponseHeader sounds like a noble goal. > > > > Paul, what do you think? > > Since key and values are strings, We can use even indices to hold keys and odd indices values, or we can do two arrays. Either way should work to save the Java <-> C++ round trips :) https://codereview.chromium.org/1359343005/diff/20001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/UrlResponseInfo.java (right): https://codereview.chromium.org/1359343005/diff/20001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/UrlResponseInfo.java:28: private volatile long mReceivedBytesCount; On 2015/09/25 15:58:27, mef wrote: > On 2015/09/25 13:54:32, xunjieli wrote: > > We are only updating this on the network thread. Our embedders will query new > > values every time using getReceivedBytesCount. Why does this need to be > > volatile? > > Um, what if the app holds on to UrlResponseInfo and accesses it while Cronet is > updating value on network thread? I see. that volatile keyword might help then. https://codereview.chromium.org/1359343005/diff/20001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/UrlResponseInfo.java:100: public Map<String, List<String>> getAllHeaders() { On 2015/09/25 15:58:27, mef wrote: > On 2015/09/25 13:54:32, xunjieli wrote: > > It's sad that even though we have Map.Entry, we still do all these extra work > to > > construct a Map because there can be multiple headers of the same name. I > wonder > > how exactly Map.Entry is going to help our embedders. > > Theoretically it will help if they want to headers into multimap<String, String> > instead of Map<String, List<String>>. I see. That makes sense. Thanks! https://codereview.chromium.org/1359343005/diff/20001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/UrlResponseInfo.java:171: void updateReceivedBytesCount(long currentReceivedBytesCount) { On 2015/09/25 15:58:27, mef wrote: > On 2015/09/25 13:54:32, xunjieli wrote: > > Maybe rename this method as "setReceivedBytesCount" to make it clearer that > this > > is setting the variable and not incrementing/decrementing. > > Need comments to explain why the visibility is package-protected. > > Sounds good, will do. I also wonder whether it should be updated for every > onReadCompleted callback. I will vote for that. But could also be in a separate CL if the change is big.
https://codereview.chromium.org/1359343005/diff/20001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java (right): https://codereview.chromium.org/1359343005/diff/20001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java:210: mUrlRequestAdapter, header.getKey(), header.getValue())) { On 2015/09/25 16:16:34, xunjieli wrote: > On 2015/09/25 15:58:27, mef wrote: > > On 2015/09/25 13:54:32, xunjieli wrote: > > > The more I looked at this, the more I think this is inefficient. If we have > a > > > lot of headers, there are multiple JNI calls. Maybe we can pass a > jobjectArray > > > and use base::android::AppendJavaStringArrayToStringVector directly in one > > > single JNI call. > > Do you mean make 2 string arrays for keys and values? That might work. > > > > > Similarly, we can get rid of onAppendResponseHeader, and do the reverse. > > IIUIC JNI calls up from C++ to Java are more expensive than down from Java to > > C++, so getting rid of onAppendResponseHeader sounds like a noble goal. > > > > > > Paul, what do you think? > > > > > > Since key and values are strings, We can use even indices to hold keys and odd > indices values, or we can do two arrays. Either way should work to save the Java > <-> C++ round trips :) So here's a great resource: https://docs.google.com/spreadsheets/d/1ulggKOZ7VQ8IytP1hKr-1wvBu70qG2qxKT4vR... That's on a Nexus 4, which is probably 2x slower than a more modern equivalent. JNI is very fast in general. String conversions are slower but still pretty fast. I'm not too worried about the overhead here right now as generally there aren't too many HTTP headers and generally they're not too big. Anyhow this CL isn't changing the number or type of JNI here so lets move this discussion to a separate bug. https://codereview.chromium.org/1359343005/diff/20001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/UrlResponseInfo.java (right): https://codereview.chromium.org/1359343005/diff/20001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/UrlResponseInfo.java:18: // TODO(mef): Remove ResponseInfo after caller updates. This class has pretty crazy mutability requirements. I think it needs a serious comment (not a javadoc) describing how almost all the members are strictly immutable, with the exception of mReceivedBytesCount. Might also be good to describe when we generate a new one (e.g. response begins, redirect followed). https://codereview.chromium.org/1359343005/diff/20001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/UrlResponseInfo.java:28: private volatile long mReceivedBytesCount; On 2015/09/25 16:16:34, xunjieli wrote: > On 2015/09/25 15:58:27, mef wrote: > > On 2015/09/25 13:54:32, xunjieli wrote: > > > We are only updating this on the network thread. Our embedders will query > new > > > values every time using getReceivedBytesCount. Why does this need to be > > > volatile? > > > > Um, what if the app holds on to UrlResponseInfo and accesses it while Cronet > is > > updating value on network thread? > > I see. that volatile keyword might help then. volatile is insufficient for items larger than sig_atomic_t, which is 32 bits on a 32-bit processor. Remember the classic example: Cronet increments from 0xffffffff to 0x100000000, Cronet writes top word as 0x1, embedder reads top word as 0x1, embedder reads bottom word as 0xffffffff, Cronet writes bottom word as 0x0, then embedder thinks value is 0x1ffffffff which is pretty far off. AtomicLong should work. https://codereview.chromium.org/1359343005/diff/20001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/UrlResponseInfo.java:154: * decoding and proxy handling but before gzip and SDCH decompression. Though this sentence is very accurate and precise, it's also very complicated. I think we need to clarify that this is a minimum bytes-over-the-wire count. I think we also need to mention that the count relates just to this request. Can we instead say something like: Returns a minimum count of bytes received from the network to process this request. This count may ignore certain overheads (e.g. IP and TCP/UDP framing, SSL handshake and framing, proxy handling). This count is taken prior to gzip and SDCH decompression. https://codereview.chromium.org/1359343005/diff/20001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/UrlResponseInfo.java:155: * Available on request completion. I don't think "Available on request completion." is sufficient. Aren't we going to update on failure or cancellation too? https://codereview.chromium.org/1359343005/diff/20001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/UrlResponseInfo.java:155: * Available on request completion. I think we need a further comment about how this value can magically change. This is extremely magical (comes from a thread embedder doesn't even know exists) and may come as a big surprise to embedders.
https://codereview.chromium.org/1359343005/diff/20001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java (right): https://codereview.chromium.org/1359343005/diff/20001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java:210: mUrlRequestAdapter, header.getKey(), header.getValue())) { On 2015/09/25 18:30:16, pauljensen wrote: > On 2015/09/25 16:16:34, xunjieli wrote: > > On 2015/09/25 15:58:27, mef wrote: > > > On 2015/09/25 13:54:32, xunjieli wrote: > > > > The more I looked at this, the more I think this is inefficient. If we > have > > a > > > > lot of headers, there are multiple JNI calls. Maybe we can pass a > > jobjectArray > > > > and use base::android::AppendJavaStringArrayToStringVector directly in one > > > > single JNI call. > > > Do you mean make 2 string arrays for keys and values? That might work. > > > > > > > Similarly, we can get rid of onAppendResponseHeader, and do the reverse. > > > IIUIC JNI calls up from C++ to Java are more expensive than down from Java > to > > > C++, so getting rid of onAppendResponseHeader sounds like a noble goal. > > > > > > > > Paul, what do you think? > > > > > > > > > > Since key and values are strings, We can use even indices to hold keys and odd > > indices values, or we can do two arrays. Either way should work to save the > Java > > <-> C++ round trips :) > > So here's a great resource: > https://docs.google.com/spreadsheets/d/1ulggKOZ7VQ8IytP1hKr-1wvBu70qG2qxKT4vR... > That's on a Nexus 4, which is probably 2x slower than a more modern equivalent. > JNI is very fast in general. String conversions are slower but still pretty > fast. Cool, thanks for the pointer. > I'm not too worried about the overhead here right now as generally there aren't > too many HTTP headers and generally they're not too big. > Anyhow this CL isn't changing the number or type of JNI here so lets move this > discussion to a separate bug. Yea, I just thought it worth bringing up while we are here. I mentioned this a while ago, but it got lost. I will file a bug to track it.
https://codereview.chromium.org/1359343005/diff/20001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/UrlResponseInfo.java (right): https://codereview.chromium.org/1359343005/diff/20001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/UrlResponseInfo.java:28: private volatile long mReceivedBytesCount; On 2015/09/25 18:30:16, pauljensen wrote: > On 2015/09/25 16:16:34, xunjieli wrote: > > On 2015/09/25 15:58:27, mef wrote: > > > On 2015/09/25 13:54:32, xunjieli wrote: > > > > We are only updating this on the network thread. Our embedders will query > > new > > > > values every time using getReceivedBytesCount. Why does this need to be > > > > volatile? > > > > > > Um, what if the app holds on to UrlResponseInfo and accesses it while Cronet > > is > > > updating value on network thread? > > > > I see. that volatile keyword might help then. > > volatile is insufficient for items larger than sig_atomic_t, which is 32 bits on > a 32-bit processor. Remember the classic example: Cronet increments from > 0xffffffff to 0x100000000, Cronet writes top word as 0x1, embedder reads top > word as 0x1, embedder reads bottom word as 0xffffffff, Cronet writes bottom word > as 0x0, then embedder thinks value is 0x1ffffffff which is pretty far off. > AtomicLong should work. Arghh! Thanks for catching this. Done. https://codereview.chromium.org/1359343005/diff/20001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/UrlResponseInfo.java:154: * decoding and proxy handling but before gzip and SDCH decompression. On 2015/09/25 18:30:17, pauljensen wrote: > Though this sentence is very accurate and precise, it's also very complicated. > I think we need to clarify that this is a minimum bytes-over-the-wire count. I > think we also need to mention that the count relates just to this request. Can > we instead say something like: > > Returns a minimum count of bytes received from the network to process this > request. > This count may ignore certain overheads (e.g. IP and TCP/UDP framing, SSL > handshake and framing, proxy handling). > This count is taken prior to gzip and SDCH decompression. Done. https://codereview.chromium.org/1359343005/diff/20001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/UrlResponseInfo.java:155: * Available on request completion. On 2015/09/25 18:30:16, pauljensen wrote: > I think we need a further comment about how this value can magically change. > This is extremely magical (comes from a thread embedder doesn't even know > exists) and may come as a big surprise to embedders. In order to reduce magic nature, would it make sense to keep it 0 until completion? Actually, come to think about it, would it make sense to create another copy of ResponseInfo on success/failure/cancel, with actual value in it? That way we don't need a setter. The drawback is memory churn. https://codereview.chromium.org/1359343005/diff/20001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/UrlResponseInfo.java:171: void updateReceivedBytesCount(long currentReceivedBytesCount) { On 2015/09/25 16:16:34, xunjieli wrote: > On 2015/09/25 15:58:27, mef wrote: > > On 2015/09/25 13:54:32, xunjieli wrote: > > > Maybe rename this method as "setReceivedBytesCount" to make it clearer that > > this > > > is setting the variable and not incrementing/decrementing. > > > Need comments to explain why the visibility is package-protected. > > > > Sounds good, will do. I also wonder whether it should be updated for every > > onReadCompleted callback. > > I will vote for that. But could also be in a separate CL if the change is big. Acknowledged.
https://codereview.chromium.org/1359343005/diff/20001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/UrlResponseInfo.java (right): https://codereview.chromium.org/1359343005/diff/20001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/UrlResponseInfo.java:171: void updateReceivedBytesCount(long currentReceivedBytesCount) { On 2015/09/25 13:54:32, xunjieli wrote: > Maybe rename this method as "setReceivedBytesCount" to make it clearer that this > is setting the variable and not incrementing/decrementing. > Need comments to explain why the visibility is package-protected. Done.
PTAL. I've rebased this on top of Paul's CronetEngine CL.
https://codereview.chromium.org/1359343005/diff/140001/components/cronet/andr... File components/cronet/android/java/src/org/chromium/net/UrlResponseInfo.java (right): https://codereview.chromium.org/1359343005/diff/140001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/UrlResponseInfo.java:20: * gets a different copy of UrlResponseInfo describing particular redirect response. Suggest the following changes: "Sent on every call to the {@link UrlRequestListener}" -> "Included in {@link UrlRequestListener} callbacks" "Each call to {@link UrlRequestListener#onReceivedRedirect UrlRequestListener.onReceivedRedirect()} gets" -> "Each {@link UrlRequestListener#onReceivedRedirect UrlRequestListener.onReceivedRedirect()} callback gets" "describing particular" ->"describing a particular" https://codereview.chromium.org/1359343005/diff/140001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/UrlResponseInfo.java:21: * The received bytes count is not valid until request has completion or cancelation. Suggest moving this comment to mReceivedBytesCount's definition, since this is more like an implementation detail. https://codereview.chromium.org/1359343005/diff/140001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/UrlResponseInfo.java:151: * Available on request completion or cancelation. Maybe add a TODO here for implementating it for cancellation, and the bug number. https://codereview.chromium.org/1359343005/diff/140001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/UrlResponseInfo.java:157: public String toString() { add @Override https://codereview.chromium.org/1359343005/diff/140001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/UrlResponseInfo.java:166: // Set ReceivedBytesCount up requirest completion or cancelation. nit: there is a typo in this comment. https://codereview.chromium.org/1359343005/diff/140001/components/cronet/andr... File components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestTest.java (right): https://codereview.chromium.org/1359343005/diff/140001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestTest.java:71: assertTrue(responseInfo.toString().length() > 0); Instead of doing this, maybe add a check for testSimpleGet to assert on the full string format of the responseInfo. https://codereview.chromium.org/1359343005/diff/140001/components/cronet/andr... File components/cronet/android/test/javatests/src/org/chromium/net/QuicTest.java (right): https://codereview.chromium.org/1359343005/diff/140001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/QuicTest.java:94: assertTrue(listener.mResponseInfo.getReceivedBytesCount() > expectedContent.length()); Hmm interesting. I didn't know they changed QUIC's implementation to include headers in ReceivedBytesCount. https://codereview.chromium.org/1360063002
Thanks! https://codereview.chromium.org/1359343005/diff/140001/components/cronet/andr... File components/cronet/android/java/src/org/chromium/net/UrlResponseInfo.java (right): https://codereview.chromium.org/1359343005/diff/140001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/UrlResponseInfo.java:20: * gets a different copy of UrlResponseInfo describing particular redirect response. On 2015/10/05 20:39:02, xunjieli wrote: > Suggest the following changes: > > "Sent on every call to the {@link UrlRequestListener}" > -> > "Included in {@link UrlRequestListener} callbacks" > > "Each call to {@link UrlRequestListener#onReceivedRedirect > UrlRequestListener.onReceivedRedirect()} gets" > -> > "Each {@link UrlRequestListener#onReceivedRedirect > UrlRequestListener.onReceivedRedirect()} callback > gets" > > "describing particular" ->"describing a particular" Done. https://codereview.chromium.org/1359343005/diff/140001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/UrlResponseInfo.java:21: * The received bytes count is not valid until request has completion or cancelation. On 2015/10/05 20:39:02, xunjieli wrote: > Suggest moving this comment to mReceivedBytesCount's definition, since this is > more like an implementation detail. Done. https://codereview.chromium.org/1359343005/diff/140001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/UrlResponseInfo.java:151: * Available on request completion or cancelation. On 2015/10/05 20:39:02, xunjieli wrote: > Maybe add a TODO here for implementating it for cancellation, and the bug > number. Done. https://codereview.chromium.org/1359343005/diff/140001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/UrlResponseInfo.java:157: public String toString() { On 2015/10/05 20:39:02, xunjieli wrote: > add @Override Done. https://codereview.chromium.org/1359343005/diff/140001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/UrlResponseInfo.java:166: // Set ReceivedBytesCount up requirest completion or cancelation. On 2015/10/05 20:39:02, xunjieli wrote: > nit: there is a typo in this comment. Done. https://codereview.chromium.org/1359343005/diff/140001/components/cronet/andr... File components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestTest.java (right): https://codereview.chromium.org/1359343005/diff/140001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestTest.java:71: assertTrue(responseInfo.toString().length() > 0); On 2015/10/05 20:39:02, xunjieli wrote: > Instead of doing this, maybe add a check for testSimpleGet to assert on the full > string format of the responseInfo. Done. https://codereview.chromium.org/1359343005/diff/140001/components/cronet/andr... File components/cronet/android/test/javatests/src/org/chromium/net/QuicTest.java (right): https://codereview.chromium.org/1359343005/diff/140001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/QuicTest.java:94: assertTrue(listener.mResponseInfo.getReceivedBytesCount() > expectedContent.length()); On 2015/10/05 20:39:02, xunjieli wrote: > Hmm interesting. I didn't know they changed QUIC's implementation to include > headers in ReceivedBytesCount. https://codereview.chromium.org/1360063002 Apparently I was wrong and headers ARE included in received bytes count even with HTTP/1.1
https://codereview.chromium.org/1359343005/diff/20001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/UrlResponseInfo.java (right): https://codereview.chromium.org/1359343005/diff/20001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/UrlResponseInfo.java:155: * Available on request completion. On 2015/09/25 20:08:50, mef wrote: > On 2015/09/25 18:30:16, pauljensen wrote: > > I think we need a further comment about how this value can magically change. > > This is extremely magical (comes from a thread embedder doesn't even know > > exists) and may come as a big surprise to embedders. > > In order to reduce magic nature, would it make sense to keep it 0 until > completion? > Actually, come to think about it, would it make sense to create another copy of > ResponseInfo on success/failure/cancel, with actual value in it? > That way we don't need a setter. The drawback is memory churn. So I'd actually like to live in a world where it gets updated even more frequently, like every time we pass it to the Listener, like for every onReadCompleted() and all the other Listener methods. It seems like simple information that we can cheaply update. That's why I was pushing for keeping the description here fairly broad and just indicating that it's a minimum value which gives us the leeway to improve its accuracy as we see fit. Anyhow, perhaps we should clarify the "Available on request completion." to something like "This value may change (even for one {@code UrlResponseInfo} instance) as the request progresses until completion, when {@link UrlRequestListener#onSuccess} or {@link UrlRequestListener#onFailure} is called." I just don't want to paint us into a corner. If tomorrow an embedder says they'd find it useful to get this value updated very frequently, I'd like us to have a cheap and easy path to enabling that. Anyhow, what I'm getting at is I'm fine with this class having some funny mutability if it prevents more copying/allocation of instances of this class, which in Java means more GC. Than again, this is just my crazy opinion. I guess I love my data and want constant updates :) I don't feel too strongly on this matter and I'd be fine with just about any solution to this issue.
On 2015/10/06 12:55:41, pauljensen wrote: > https://codereview.chromium.org/1359343005/diff/20001/components/cronet/andro... > File components/cronet/android/java/src/org/chromium/net/UrlResponseInfo.java > (right): > > https://codereview.chromium.org/1359343005/diff/20001/components/cronet/andro... > components/cronet/android/java/src/org/chromium/net/UrlResponseInfo.java:155: * > Available on request completion. > On 2015/09/25 20:08:50, mef wrote: > > On 2015/09/25 18:30:16, pauljensen wrote: > > > I think we need a further comment about how this value can magically change. > > > > This is extremely magical (comes from a thread embedder doesn't even know > > > exists) and may come as a big surprise to embedders. > > > > In order to reduce magic nature, would it make sense to keep it 0 until > > completion? > > Actually, come to think about it, would it make sense to create another copy > of > > ResponseInfo on success/failure/cancel, with actual value in it? > > That way we don't need a setter. The drawback is memory churn. > > So I'd actually like to live in a world where it gets updated even more > frequently, like every time we pass it to the Listener, like for every > onReadCompleted() and all the other Listener methods. It seems like simple > information that we can cheaply update. That's why I was pushing for keeping > the description here fairly broad and just indicating that it's a minimum value > which gives us the leeway to improve its accuracy as we see fit. > > Anyhow, perhaps we should clarify the "Available on request completion." to > something like "This value may change (even for one {@code UrlResponseInfo} > instance) as the request progresses until completion, when {@link > UrlRequestListener#onSuccess} or {@link UrlRequestListener#onFailure} is > called." sgtm, I've updated the comment. > I just don't want to paint us into a corner. If tomorrow an embedder says > they'd find it useful to get this value updated very frequently, I'd like us to > have a cheap and easy path to enabling that. I actually wonder whether we should extract receivedBytesCount into separate class along side with sent bytes count, time to first byte, time to completion, etc. We can have ResponseInfo have a reference to it, and it will have clear separation of static and mutable data. Also, this structure could be used in BidirectionalStream, for which UrlResponseInfo doesn't really make sense.. Also, I was wrong claiming that headers are not included into the count, so it could make sense to have those values updated for redirects as well. > > Anyhow, what I'm getting at is I'm fine with this class having some funny > mutability if it prevents more copying/allocation of instances of this class, > which in Java means more GC. > > Than again, this is just my crazy opinion. I guess I love my data and want > constant updates :) I don't feel too strongly on this matter and I'd be fine > with just about any solution to this issue.
On 2015/10/06 14:39:35, mef wrote: > I actually wonder whether we should extract receivedBytesCount into separate > class along side with sent bytes count, time to first byte, time to completion, > etc. > We can have ResponseInfo have a reference to it, and it will have clear > separation of static and mutable data. > Also, this structure could be used in BidirectionalStream, for which > UrlResponseInfo doesn't really make sense.. > > Also, I was wrong claiming that headers are not included into the count, so it > could make sense to have those values updated for redirects as well. One issue I see is that according to this comment: https://code.google.com/p/chromium/codesearch#chromium/src/net/url_request/ur... The value is reset on redirects. I think this may be unexpected for embedders who expect this to be a monotonically increasing count of bytes sent across the wire. I think we may want to accumulate it across redirects.
On 2015/10/06 14:43:42, pauljensen wrote: > On 2015/10/06 14:39:35, mef wrote: > > I actually wonder whether we should extract receivedBytesCount into separate > > class along side with sent bytes count, time to first byte, time to > completion, > > etc. > > We can have ResponseInfo have a reference to it, and it will have clear > > separation of static and mutable data. > > Also, this structure could be used in BidirectionalStream, for which > > UrlResponseInfo doesn't really make sense.. > > > > Also, I was wrong claiming that headers are not included into the count, so it > > could make sense to have those values updated for redirects as well. > > One issue I see is that according to this comment: > https://code.google.com/p/chromium/codesearch#chromium/src/net/url_request/ur... > The value is reset on redirects. I think this may be unexpected for embedders > who expect this to be a monotonically increasing count of bytes sent across the > wire. I think we may want to accumulate it across redirects. Currently each onRedirect gets its own info ResponseInfo with redirect-specific headers, url and url chain. I think it would make sense if it also got redirect-specific bytes count (along side with timing info in the future).
On 2015/10/06 14:48:40, mef wrote: > On 2015/10/06 14:43:42, pauljensen wrote: > > On 2015/10/06 14:39:35, mef wrote: > > > I actually wonder whether we should extract receivedBytesCount into separate > > > class along side with sent bytes count, time to first byte, time to > > completion, > > > etc. > > > We can have ResponseInfo have a reference to it, and it will have clear > > > separation of static and mutable data. > > > Also, this structure could be used in BidirectionalStream, for which > > > UrlResponseInfo doesn't really make sense.. > > > > > > Also, I was wrong claiming that headers are not included into the count, so > it > > > could make sense to have those values updated for redirects as well. > > > > One issue I see is that according to this comment: > > > https://code.google.com/p/chromium/codesearch#chromium/src/net/url_request/ur... > > The value is reset on redirects. I think this may be unexpected for embedders > > who expect this to be a monotonically increasing count of bytes sent across > the > > wire. I think we may want to accumulate it across redirects. > > Currently each onRedirect gets its own info ResponseInfo with redirect-specific > headers, url and url chain. > I think it would make sense if it also got redirect-specific bytes count (along > side with timing info in the future). Hmm so the value given back in onSuccess does not include any bytes received prior to the last redirect? This is a little weird/unexpected to me. If we want this to be how it works, then we should document it as such.
On 2015/10/06 17:47:27, pauljensen wrote: > On 2015/10/06 14:48:40, mef wrote: > > On 2015/10/06 14:43:42, pauljensen wrote: > > > On 2015/10/06 14:39:35, mef wrote: > > > > I actually wonder whether we should extract receivedBytesCount into > separate > > > > class along side with sent bytes count, time to first byte, time to > > > completion, > > > > etc. > > > > We can have ResponseInfo have a reference to it, and it will have clear > > > > separation of static and mutable data. > > > > Also, this structure could be used in BidirectionalStream, for which > > > > UrlResponseInfo doesn't really make sense.. > > > > > > > > Also, I was wrong claiming that headers are not included into the count, > so > > it > > > > could make sense to have those values updated for redirects as well. > > > > > > One issue I see is that according to this comment: > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/net/url_request/ur... > > > The value is reset on redirects. I think this may be unexpected for > embedders > > > who expect this to be a monotonically increasing count of bytes sent across > > the > > > wire. I think we may want to accumulate it across redirects. > > > > Currently each onRedirect gets its own info ResponseInfo with > redirect-specific > > headers, url and url chain. > > I think it would make sense if it also got redirect-specific bytes count > (along > > side with timing info in the future). > > Hmm so the value given back in onSuccess does not include any bytes received > prior to the last redirect? This is a little weird/unexpected to me. If we > want this to be how it works, then we should document it as such. Hrm, I've made false claims before, so I guess it needs more clarification.
On 2015/10/06 18:20:26, mef wrote: > On 2015/10/06 17:47:27, pauljensen wrote: > > On 2015/10/06 14:48:40, mef wrote: > > > On 2015/10/06 14:43:42, pauljensen wrote: > > > > On 2015/10/06 14:39:35, mef wrote: > > > > > I actually wonder whether we should extract receivedBytesCount into > > separate > > > > > class along side with sent bytes count, time to first byte, time to > > > > completion, > > > > > etc. > > > > > We can have ResponseInfo have a reference to it, and it will have clear > > > > > separation of static and mutable data. > > > > > Also, this structure could be used in BidirectionalStream, for which > > > > > UrlResponseInfo doesn't really make sense.. > > > > > > > > > > Also, I was wrong claiming that headers are not included into the count, > > so > > > it > > > > > could make sense to have those values updated for redirects as well. > > > > > > > > One issue I see is that according to this comment: > > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/net/url_request/ur... > > > > The value is reset on redirects. I think this may be unexpected for > > embedders > > > > who expect this to be a monotonically increasing count of bytes sent > across > > > the > > > > wire. I think we may want to accumulate it across redirects. > > > > > > Currently each onRedirect gets its own info ResponseInfo with > > redirect-specific > > > headers, url and url chain. > > > I think it would make sense if it also got redirect-specific bytes count > > (along > > > side with timing info in the future). > > > > Hmm so the value given back in onSuccess does not include any bytes received > > prior to the last redirect? This is a little weird/unexpected to me. If we > > want this to be how it works, then we should document it as such. > > Hrm, I've made false claims before, so I guess it needs more clarification. To clear things up, it'll be good to add an assertion in CronetUrlRequestTest#testRedirectAsync to check the total received bytes.
On 2015/10/06 18:26:01, xunjieli wrote: > On 2015/10/06 18:20:26, mef wrote: > > On 2015/10/06 17:47:27, pauljensen wrote: > > > On 2015/10/06 14:48:40, mef wrote: > > > > On 2015/10/06 14:43:42, pauljensen wrote: > > > > > On 2015/10/06 14:39:35, mef wrote: > > > > > > I actually wonder whether we should extract receivedBytesCount into > > > separate > > > > > > class along side with sent bytes count, time to first byte, time to > > > > > completion, > > > > > > etc. > > > > > > We can have ResponseInfo have a reference to it, and it will have > clear > > > > > > separation of static and mutable data. > > > > > > Also, this structure could be used in BidirectionalStream, for which > > > > > > UrlResponseInfo doesn't really make sense.. > > > > > > > > > > > > Also, I was wrong claiming that headers are not included into the > count, > > > so > > > > it > > > > > > could make sense to have those values updated for redirects as well. > > > > > > > > > > One issue I see is that according to this comment: > > > > > > > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/net/url_request/ur... > > > > > The value is reset on redirects. I think this may be unexpected for > > > embedders > > > > > who expect this to be a monotonically increasing count of bytes sent > > across > > > > the > > > > > wire. I think we may want to accumulate it across redirects. > > > > > > > > Currently each onRedirect gets its own info ResponseInfo with > > > redirect-specific > > > > headers, url and url chain. > > > > I think it would make sense if it also got redirect-specific bytes count > > > (along > > > > side with timing info in the future). > > > > > > Hmm so the value given back in onSuccess does not include any bytes received > > > prior to the last redirect? This is a little weird/unexpected to me. If we > > > want this to be how it works, then we should document it as such. > > > > Hrm, I've made false claims before, so I guess it needs more clarification. > > To clear things up, it'll be good to add an assertion in > CronetUrlRequestTest#testRedirectAsync to check the total received bytes. Added. Each redirect gets its own receivedBytesCount, not included into the final one.
On 2015/10/06 18:20:26, mef wrote: > > Hmm so the value given back in onSuccess does not include any bytes received > > prior to the last redirect? This is a little weird/unexpected to me. If we > > want this to be how it works, then we should document it as such. > > Hrm, I've made false claims before, so I guess it needs more clarification. I'm awaiting this comment.
On 2015/10/07 13:11:26, pauljensen wrote: > On 2015/10/06 18:20:26, mef wrote: > > > Hmm so the value given back in onSuccess does not include any bytes received > > > prior to the last redirect? This is a little weird/unexpected to me. If we > > > want this to be how it works, then we should document it as such. > > > > Hrm, I've made false claims before, so I guess it needs more clarification. > > I'm awaiting this comment. Should I add a comment like this: "Each redirect gets its own receivedBytesCount, not included into the final one." or could you suggest a better wording?
On 2015/10/07 13:20:59, mef wrote: > On 2015/10/07 13:11:26, pauljensen wrote: > > On 2015/10/06 18:20:26, mef wrote: > > > > Hmm so the value given back in onSuccess does not include any bytes > received > > > > prior to the last redirect? This is a little weird/unexpected to me. If > we > > > > want this to be how it works, then we should document it as such. > > > > > > Hrm, I've made false claims before, so I guess it needs more clarification. > > > > I'm awaiting this comment. > > Should I add a comment like this: > "Each redirect gets its own receivedBytesCount, not included into the > final one." or could you suggest a better wording? Frankly I don't think we should reset it on each redirect. The one embedder trying to use this value didn't seem to understand that it was reset on each redirect, and they're going to have to incur the pain of storing and accumulating this value across redirects. I think Cronet should be the one storing and accumulating this value across redirects. If the embedder wants to know the byte count for a particular redirect, they can look at the difference. I think we may even want to consider a default implementation of onReceivedRedirect() that simply calls followRedirect(), perhaps with some security limitations. I note that HttpURLConnection follows redirects by default: http://developer.android.com/reference/java/net/HttpURLConnection.html#instan... (though I also note that it claims to have a limit of 5 and won't follow redirects from HTTPS->HTTP or vice versa.) And browsers and wget also do by default (with limitations like no HTTPS->HTTP and watching out for loops). The correlary to this however is that if we offered a default implementation that followed redirects then it would a fair bit of extra work for embedders to determine byte count if we reset upon each redirect.
On 2015/10/07 15:12:05, pauljensen wrote: > On 2015/10/07 13:20:59, mef wrote: > > On 2015/10/07 13:11:26, pauljensen wrote: > > > On 2015/10/06 18:20:26, mef wrote: > > > > > Hmm so the value given back in onSuccess does not include any bytes > > received > > > > > prior to the last redirect? This is a little weird/unexpected to me. > If > > we > > > > > want this to be how it works, then we should document it as such. > > > > > > > > Hrm, I've made false claims before, so I guess it needs more > clarification. > > > > > > I'm awaiting this comment. > > > > Should I add a comment like this: > > "Each redirect gets its own receivedBytesCount, not included into the > > final one." or could you suggest a better wording? > > Frankly I don't think we should reset it on each redirect. The one embedder > trying to use this value didn't seem to understand that it was reset on each > redirect, and they're going to have to incur the pain of storing and > accumulating this value across redirects. I think Cronet should be the one > storing and accumulating this value across redirects. Discussed, agreed, done. PTAL. > If the embedder wants to > know the byte count for a particular redirect, they can look at the difference. > I think we may even want to consider a default implementation of > onReceivedRedirect() that simply calls followRedirect(), perhaps with some > security limitations. > I note that HttpURLConnection follows redirects by default: > http://developer.android.com/reference/java/net/HttpURLConnection.html#instan... > (though I also note that it claims to have a limit of 5 and won't follow > redirects from HTTPS->HTTP or vice versa.) > And browsers and wget also do by default (with limitations like no HTTPS->HTTP > and watching out for loops). > The correlary to this however is that if we offered a default implementation > that followed redirects then it would a fair bit of extra work for embedders to > determine byte count if we reset upon each redirect.
https://codereview.chromium.org/1359343005/diff/240001/components/cronet/andr... File components/cronet/android/java/src/org/chromium/net/UrlRequestListener.java (right): https://codereview.chromium.org/1359343005/diff/240001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/UrlRequestListener.java:89: * @param info Response information. May be null if no response was received. null->{@code null} https://codereview.chromium.org/1359343005/diff/240001/components/cronet/andr... File components/cronet/android/java/src/org/chromium/net/UrlResponseInfo.java (right): https://codereview.chromium.org/1359343005/diff/240001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/UrlResponseInfo.java:72: * request has a "HTTP/1.1 200 OK" response, this method returns "OK". has->received https://codereview.chromium.org/1359343005/diff/240001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/UrlResponseInfo.java:124: * Returns the protocol (e.g. "quic/1+spdy/3") negotiated with the server. The quote mark here is breaking javadoc summary line for this function. Perhaps change quotes to " https://codereview.chromium.org/1359343005/diff/240001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/UrlResponseInfo.java:147: * decompression and includes headers and data from all redirects. "gzip and SDCH decompression"->"decompression (e.g. gzip and SDCH)" https://codereview.chromium.org/1359343005/diff/240001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/UrlResponseInfo.java:149: * This value may change (even for one {@code UrlResponseInfo} instance) as the request could maybe change the @code to @link, up to you https://codereview.chromium.org/1359343005/diff/240001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/UrlResponseInfo.java:167: // Set ReceivedBytesCount up request completion or cancelation. I don't understand this comment. Perhaps reword: Set ReceivedBytesCount. Must not be called after request completion or cancellation.
Thanks, PTAL. https://codereview.chromium.org/1359343005/diff/240001/components/cronet/andr... File components/cronet/android/java/src/org/chromium/net/UrlRequestListener.java (right): https://codereview.chromium.org/1359343005/diff/240001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/UrlRequestListener.java:89: * @param info Response information. May be null if no response was received. On 2015/10/08 14:31:46, pauljensen wrote: > null->{@code null} Done. https://codereview.chromium.org/1359343005/diff/240001/components/cronet/andr... File components/cronet/android/java/src/org/chromium/net/UrlResponseInfo.java (right): https://codereview.chromium.org/1359343005/diff/240001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/UrlResponseInfo.java:72: * request has a "HTTP/1.1 200 OK" response, this method returns "OK". On 2015/10/08 14:31:46, pauljensen wrote: > has->received Done. https://codereview.chromium.org/1359343005/diff/240001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/UrlResponseInfo.java:124: * Returns the protocol (e.g. "quic/1+spdy/3") negotiated with the server. On 2015/10/08 14:31:46, pauljensen wrote: > The quote mark here is breaking javadoc summary line for this function. Perhaps > change quotes to " It turns out that javadoc treats period followed by space as end of sentence. Rephrased. https://codereview.chromium.org/1359343005/diff/240001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/UrlResponseInfo.java:147: * decompression and includes headers and data from all redirects. On 2015/10/08 14:31:46, pauljensen wrote: > "gzip and SDCH decompression"->"decompression (e.g. gzip and SDCH)" Done. https://codereview.chromium.org/1359343005/diff/240001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/UrlResponseInfo.java:149: * This value may change (even for one {@code UrlResponseInfo} instance) as the request On 2015/10/08 14:31:46, pauljensen wrote: > could maybe change the @code to @link, up to you Done. https://codereview.chromium.org/1359343005/diff/240001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/UrlResponseInfo.java:167: // Set ReceivedBytesCount up request completion or cancelation. On 2015/10/08 14:31:46, pauljensen wrote: > I don't understand this comment. Perhaps reword: > Set ReceivedBytesCount. Must not be called after request completion or > cancellation. Done.
https://codereview.chromium.org/1359343005/diff/280001/components/cronet/andr... File components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java (right): https://codereview.chromium.org/1359343005/diff/280001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java:65: private long mReceivedBytesCountFromRedirects; Might be cleaner to keep a mReceivedBytesCount variable and increment it in both onReceivedRedirect, and onReadCompleted/onSucceeded/onError. https://codereview.chromium.org/1359343005/diff/280001/components/cronet/andr... File components/cronet/android/java/src/org/chromium/net/UrlResponseInfo.java (right): https://codereview.chromium.org/1359343005/diff/280001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/UrlResponseInfo.java:151: * {@link UrlRequestListener#onFailure} is called. nit: broken links. Should be #onSucceeded and #onFailed
Thanks! https://codereview.chromium.org/1359343005/diff/280001/components/cronet/andr... File components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java (right): https://codereview.chromium.org/1359343005/diff/280001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java:65: private long mReceivedBytesCountFromRedirects; On 2015/10/08 19:33:55, xunjieli wrote: > Might be cleaner to keep a mReceivedBytesCount variable and increment it in both > onReceivedRedirect, and onReadCompleted/onSucceeded/onError. Given that it is close-to-wire value, it can't be calculated based on number of bytes in onReadCompleted. This variable is essentially an accumulation of data received before final request received its headers. https://codereview.chromium.org/1359343005/diff/280001/components/cronet/andr... File components/cronet/android/java/src/org/chromium/net/UrlResponseInfo.java (right): https://codereview.chromium.org/1359343005/diff/280001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/UrlResponseInfo.java:151: * {@link UrlRequestListener#onFailure} is called. On 2015/10/08 19:33:55, xunjieli wrote: > nit: broken links. Should be #onSucceeded and #onFailed Done.
https://codereview.chromium.org/1359343005/diff/300001/components/cronet/andr... File components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java (right): https://codereview.chromium.org/1359343005/diff/300001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java:494: * @param httpStatusCode from redirect response @param receivedBytesCount
Thanks, PTAL. https://codereview.chromium.org/1359343005/diff/300001/components/cronet/andr... File components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java (right): https://codereview.chromium.org/1359343005/diff/300001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java:494: * @param httpStatusCode from redirect response On 2015/10/13 18:59:30, pauljensen wrote: > @param receivedBytesCount Done.
LGTM https://codereview.chromium.org/1359343005/diff/280001/components/cronet/andr... File components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java (right): https://codereview.chromium.org/1359343005/diff/280001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java:65: private long mReceivedBytesCountFromRedirects; On 2015/10/08 20:54:01, mef wrote: > On 2015/10/08 19:33:55, xunjieli wrote: > > Might be cleaner to keep a mReceivedBytesCount variable and increment it in > both > > onReceivedRedirect, and onReadCompleted/onSucceeded/onError. > > Given that it is close-to-wire value, it can't be calculated based on number of > bytes in onReadCompleted. This variable is essentially an accumulation of data > received before final request received its headers. Acknowledged. I see. I misunderstood the number in the callbacks. https://codereview.chromium.org/1359343005/diff/320001/components/cronet/andr... File components/cronet/android/java/src/org/chromium/net/UrlResponseInfo.java (right): https://codereview.chromium.org/1359343005/diff/320001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/UrlResponseInfo.java:29: private final AtomicLong mReceivedBytesCount = new AtomicLong(); nit: Maybe initialize this in the constructor to match the rest of the variables? Final fields should go above non-final ones. https://codereview.chromium.org/1359343005/diff/320001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/UrlResponseInfo.java:167: // Set ReceivedBytesCount. Must not be called after request completion or cancellation. nit: maybe s/Set/Sets and s/ReceivedBytesCount/mReceivedBytesCount. https://codereview.chromium.org/1359343005/diff/320001/components/cronet/andr... File components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestTest.java (right): https://codereview.chromium.org/1359343005/diff/320001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestTest.java:499: assertEquals(337, mResponseInfo.getReceivedBytesCount()); The numbers are a bit magical. It will be nice to derive them from the contents of the file somehow. Not sure if it's worth the efforts though.
lgtm https://codereview.chromium.org/1359343005/diff/320001/components/cronet/andr... File components/cronet/android/java/src/org/chromium/net/UrlResponseInfo.java (right): https://codereview.chromium.org/1359343005/diff/320001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/UrlResponseInfo.java:172: ; what's this semicolon doing here?
Oh wait, please update readme.md
PTAL, I've updated the README.md https://codereview.chromium.org/1359343005/diff/320001/components/cronet/andr... File components/cronet/android/java/src/org/chromium/net/UrlResponseInfo.java (right): https://codereview.chromium.org/1359343005/diff/320001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/UrlResponseInfo.java:29: private final AtomicLong mReceivedBytesCount = new AtomicLong(); On 2015/10/13 22:31:14, xunjieli wrote: > nit: Maybe initialize this in the constructor to match the rest of the > variables? Final fields should go above non-final ones. Done. https://codereview.chromium.org/1359343005/diff/320001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/UrlResponseInfo.java:167: // Set ReceivedBytesCount. Must not be called after request completion or cancellation. On 2015/10/13 22:31:14, xunjieli wrote: > nit: maybe s/Set/Sets and s/ReceivedBytesCount/mReceivedBytesCount. Done. https://codereview.chromium.org/1359343005/diff/320001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/UrlResponseInfo.java:172: ; On 2015/10/15 17:05:52, pauljensen wrote: > what's this semicolon doing here? Excellent question, I dunno. Done. https://codereview.chromium.org/1359343005/diff/320001/components/cronet/andr... File components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestTest.java (right): https://codereview.chromium.org/1359343005/diff/320001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestTest.java:499: assertEquals(337, mResponseInfo.getReceivedBytesCount()); On 2015/10/13 22:31:14, xunjieli wrote: > The numbers are a bit magical. It will be nice to derive them from the contents > of the file somehow. Not sure if it's worth the efforts though. Acknowledged. I don't like magic, but the best idea I have ATM is to define constants, which doesn't seem to dispel much of the magic.
lgtm. Might want to change "Update ResponseInfo and UrlResponseInfo" to "Update ResponseInfo to UrlResponseInfo" in the CL description.
On 2015/10/16 02:49:38, pauljensen wrote: > lgtm. Might want to change "Update ResponseInfo and UrlResponseInfo" to "Update > ResponseInfo to UrlResponseInfo" in the CL description. LGTM. Good catch on the readme!
The CQ bit was checked by mef@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1359343005/360001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1359343005/360001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by mef@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1359343005/360001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1359343005/360001
Message was sent while issue was closed.
Committed patchset #19 (id:360001)
Message was sent while issue was closed.
Patchset 19 (id:??) landed as https://crrev.com/5f36625b0b053d15ed4d65c7c63feab4ecadfe5c Cr-Commit-Position: refs/heads/master@{#354515} |
