Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(1553)

Issue 586143002: Initial implementation of Cronet Async API. (Closed)

Created:
6 years, 3 months ago by mef
Modified:
6 years, 1 month ago
Reviewers:
Charles, ebravo, mmenke, xunjieli
CC:
chromium-reviews, cbentzel+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Project:
chromium
Visibility:
Public.

Description

Initial implementation of Cronet Async API. BUG=409926 Committed: https://crrev.com/d190710e56c9399e0bc1e9a88b26093190974c4d Cr-Commit-Position: refs/heads/master@{#303252}

Patch Set 1 #

Total comments: 30

Patch Set 2 : Address clm's comments. #

Patch Set 3 : Add and pass simple async unit test. #

Patch Set 4 : Address clm's comments, populate ResponseInfo. #

Total comments: 27

Patch Set 5 : SYnc #

Patch Set 6 : Address review comments. #

Patch Set 7 : Remove spurious change to ChromiumUrlRequest. #

Patch Set 8 : Make UrlRequestFactory into a class that can create factories. #

Total comments: 17

Patch Set 9 : Address clm's comments, fix legacy unit test. #

Patch Set 10 : Sync #

Patch Set 11 : Move userAgent into config, introduce ExtendedResponseInfo. #

Total comments: 36

Patch Set 12 : Address Matt's comments. #

Total comments: 36

Patch Set 13 : Address review comments. #

Total comments: 67

Patch Set 14 : Moar tests. #

Patch Set 15 : Address Matt's comments. #

Patch Set 16 : Sync #

Patch Set 17 : Added ReportMockErrorFromQuery to URLRequestMockHTTPJob. #

Total comments: 10

Patch Set 18 : Address review comments, add mock errors to CronetUrlRequestTest. #

Total comments: 55

Patch Set 19 : Address review comments, add cancel tests. #

Patch Set 20 : Added GetMockUrlWithFailure method and add tests to cancel outside of listener. #

Patch Set 21 : Sync #

Total comments: 52

Patch Set 22 : Renamed UrlRequestFactory into UrlRequestContext, addressed some comments. #

Total comments: 21

Patch Set 23 : Addressed more comments. #

Patch Set 24 : Sync #

Patch Set 25 : More tests, more comments, more cancel. #

Total comments: 102

Patch Set 26 : Address more comments. #

Total comments: 36

Patch Set 27 : Better cancel and destroy. #

Patch Set 28 : Moar tests and comments. #

Total comments: 20

Patch Set 29 : Address Helen's comments, add CronetUrlRequestContextTest. #

Total comments: 2

Patch Set 30 : Added mUrlRequestAdapterLock, don't create request adapter until start. #

Patch Set 31 : Sync #

Total comments: 28

Patch Set 32 : Address review comments. #

Total comments: 51

Patch Set 33 : Sync. #

Patch Set 34 : Address review comments. #

Patch Set 35 : Delete network thread from java thread, adjust urlchain, check for bad http method. #

Total comments: 16

Patch Set 36 : Address comments, add more shutdown tests. #

Total comments: 137

Patch Set 37 : Extract TestUrlRequestListener, address review comments. #

Total comments: 26

Patch Set 38 : Address comments, moar scoped pointers. #

Total comments: 32

Patch Set 39 : UPPERCASE enum names. #

Total comments: 57

Patch Set 40 : Address more comments. #

Patch Set 41 : Added more tests, removed CronetUrlRequestPriority enum. #

Patch Set 42 : Sync #

Total comments: 134

Patch Set 43 : Address review comments. #

Total comments: 32

Patch Set 44 : Address more comments. #

Total comments: 7

Patch Set 45 : Last comments. #

Patch Set 46 : Sync #

Unified diffs Side-by-side diffs Delta from patch set Stats (+2915 lines, -465 lines) Patch
M components/cronet.gypi View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 6 chunks +33 lines, -9 lines 0 comments Download
M components/cronet/android/cronet_loader.cc View 1 2 2 chunks +4 lines, -0 lines 0 comments Download
A + components/cronet/android/cronet_url_request.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 1 chunk +4 lines, -4 lines 0 comments Download
A components/cronet/android/cronet_url_request.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 1 chunk +269 lines, -0 lines 0 comments Download
A components/cronet/android/cronet_url_request_adapter.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 1 chunk +152 lines, -0 lines 0 comments Download
A components/cronet/android/cronet_url_request_adapter.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 1 chunk +160 lines, -0 lines 0 comments Download
A components/cronet/android/cronet_url_request_context.h View 1 chunk +16 lines, -0 lines 0 comments Download
A components/cronet/android/cronet_url_request_context.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 1 chunk +109 lines, -0 lines 0 comments Download
A components/cronet/android/cronet_url_request_context_adapter.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 1 chunk +72 lines, -0 lines 0 comments Download
A + components/cronet/android/cronet_url_request_context_adapter.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 8 chunks +56 lines, -117 lines 0 comments Download
M components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 2 chunks +562 lines, -9 lines 0 comments Download
A components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 1 chunk +139 lines, -0 lines 0 comments Download
D components/cronet/android/java/src/org/chromium/net/CronetUrlRequestFactory.java View 1 chunk +0 lines, -18 lines 0 comments Download
A components/cronet/android/java/src/org/chromium/net/ExtendedResponseInfo.java View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 1 chunk +23 lines, -0 lines 0 comments Download
M components/cronet/android/java/src/org/chromium/net/HttpUrlRequestFactoryConfig.java View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 1 chunk +3 lines, -160 lines 0 comments Download
M components/cronet/android/java/src/org/chromium/net/ResponseInfo.java View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 2 chunks +19 lines, -20 lines 0 comments Download
M components/cronet/android/java/src/org/chromium/net/UrlRequest.java View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 3 chunks +23 lines, -7 lines 0 comments Download
A components/cronet/android/java/src/org/chromium/net/UrlRequestContext.java View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 1 chunk +109 lines, -0 lines 0 comments Download
A + components/cronet/android/java/src/org/chromium/net/UrlRequestContextConfig.java View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 7 chunks +70 lines, -53 lines 0 comments Download
D components/cronet/android/java/src/org/chromium/net/UrlRequestContextConfig.template View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 1 chunk +0 lines, -14 lines 0 comments Download
A + components/cronet/android/java/src/org/chromium/net/UrlRequestContextConfigList.template View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 1 chunk +2 lines, -2 lines 0 comments Download
M components/cronet/android/java/src/org/chromium/net/UrlRequestException.java View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 1 chunk +19 lines, -1 line 0 comments Download
M components/cronet/android/java/src/org/chromium/net/UrlRequestFactory.java View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 1 chunk +0 lines, -26 lines 0 comments Download
M components/cronet/android/java/src/org/chromium/net/UrlRequestListener.java View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 3 chunks +19 lines, -12 lines 0 comments Download
A + components/cronet/android/test/assets/test/multiredirect.html View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 0 chunks +-1 lines, --1 lines 0 comments Download
A components/cronet/android/test/assets/test/multiredirect.html.mock-http-headers View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 1 chunk +3 lines, -0 lines 0 comments Download
M components/cronet/android/test/assets/test/success.txt.mock-http-headers View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 1 chunk +3 lines, -0 lines 0 comments Download
M components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/CronetTestBase.java View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 37 1 chunk +7 lines, -0 lines 0 comments Download
A components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/CronetUrlRequestContextTest.java View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 1 chunk +182 lines, -0 lines 0 comments Download
A components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/CronetUrlRequestTest.java View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 1 chunk +465 lines, -0 lines 0 comments Download
A components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/TestUrlRequestListener.java View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 1 chunk +193 lines, -0 lines 0 comments Download
M components/cronet/android/test/mock_url_request_job_util.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 2 chunks +20 lines, -0 lines 0 comments Download
M components/cronet/android/test/src/org/chromium/cronet_test_apk/CronetTestActivity.java View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 4 chunks +7 lines, -2 lines 0 comments Download
M components/cronet/android/test/src/org/chromium/cronet_test_apk/MockUrlRequestJobUtil.java View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 2 chunks +35 lines, -0 lines 0 comments Download
M components/cronet/url_request_context_config.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 2 chunks +5 lines, -0 lines 0 comments Download
M components/cronet/url_request_context_config.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 5 chunks +22 lines, -3 lines 0 comments Download
M components/cronet/url_request_context_config_list.h View 1 2 3 4 5 6 7 8 9 10 1 chunk +1 line, -0 lines 0 comments Download
M net/base/request_priority.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 1 chunk +4 lines, -0 lines 0 comments Download
M net/test/url_request/url_request_mock_http_job.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 4 chunks +22 lines, -1 line 0 comments Download
M net/test/url_request/url_request_mock_http_job.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 3 chunks +84 lines, -8 lines 0 comments Download

Messages

Total messages: 131 (6 generated)
mef
Hi, this is very early work in progress, but I would like to confirm that ...
6 years, 3 months ago (2014-09-19 21:12:31 UTC) #2
Charles
https://codereview.chromium.org/586143002/diff/1/components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java File components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java (right): https://codereview.chromium.org/586143002/diff/1/components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java#newcode24 components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java:24: public class CronetUrlRequest implements UrlRequest { Final? https://codereview.chromium.org/586143002/diff/1/components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java#newcode37 components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java:37: ...
6 years, 3 months ago (2014-09-20 00:47:34 UTC) #3
mef
Thanks, so I guess overall threading looks ok? Do we care about thread-pooling and other ...
6 years, 3 months ago (2014-09-22 17:12:13 UTC) #4
Charles
https://codereview.chromium.org/586143002/diff/1/components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java File components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java (right): https://codereview.chromium.org/586143002/diff/1/components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java#newcode119 components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java:119: public void start() { On 2014/09/22 17:12:12, mef wrote: ...
6 years, 3 months ago (2014-09-22 21:22:54 UTC) #5
mef
Thanks! I got basic unit test to pass, and will proceed with completeness and correctness. ...
6 years, 3 months ago (2014-09-23 18:42:14 UTC) #6
xunjieli
https://codereview.chromium.org/586143002/diff/60001/components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java File components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java (right): https://codereview.chromium.org/586143002/diff/60001/components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java#newcode227 components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java:227: mResponseInfo.mUrlChain.add(0, newLocation); It looks like your mUrlChain will append ...
6 years, 3 months ago (2014-09-23 20:53:25 UTC) #7
Charles
I don't actually see any way to get the response body from a CronetUrlRequest. https://codereview.chromium.org/586143002/diff/60001/components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java ...
6 years, 3 months ago (2014-09-23 22:33:13 UTC) #8
mmenke
https://codereview.chromium.org/586143002/diff/60001/components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java File components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java (right): https://codereview.chromium.org/586143002/diff/60001/components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java#newcode295 components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java:295: mListener.onComplete(CronetUrlRequest.this); On 2014/09/23 22:33:12, Charles wrote: > You're giving ...
6 years, 3 months ago (2014-09-23 22:38:22 UTC) #9
Charles
https://codereview.chromium.org/586143002/diff/60001/components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java File components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java (right): https://codereview.chromium.org/586143002/diff/60001/components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java#newcode295 components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java:295: mListener.onComplete(CronetUrlRequest.this); On 2014/09/23 22:38:22, mmenke wrote: > On 2014/09/23 ...
6 years, 3 months ago (2014-09-23 22:50:51 UTC) #10
mef
Thanks! I have one more question - ebravo@ asked to get information about over the ...
6 years, 3 months ago (2014-09-24 18:56:26 UTC) #12
mmenke
https://codereview.chromium.org/586143002/diff/60001/components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java File components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java (right): https://codereview.chromium.org/586143002/diff/60001/components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java#newcode38 components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java:38: final class OnDataReceivedRunnable implements Runnable { On 2014/09/24 18:56:26, ...
6 years, 3 months ago (2014-09-24 19:04:14 UTC) #13
mef
https://codereview.chromium.org/586143002/diff/60001/components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java File components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java (right): https://codereview.chromium.org/586143002/diff/60001/components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java#newcode38 components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java:38: final class OnDataReceivedRunnable implements Runnable { On 2014/09/24 19:04:14, ...
6 years, 2 months ago (2014-09-26 19:23:52 UTC) #15
Charles
https://codereview.chromium.org/586143002/diff/60001/components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java File components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java (right): https://codereview.chromium.org/586143002/diff/60001/components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java#newcode38 components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java:38: final class OnDataReceivedRunnable implements Runnable { On 2014/09/24 19:04:14, ...
6 years, 2 months ago (2014-09-26 22:52:26 UTC) #16
mef
https://codereview.chromium.org/586143002/diff/160001/components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java File components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java (right): https://codereview.chromium.org/586143002/diff/160001/components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java#newcode59 components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java:59: private long mTotalReceivedBytes = 0; On 2014/09/26 22:52:26, Charles ...
6 years, 2 months ago (2014-09-29 17:57:37 UTC) #17
Charles
https://codereview.chromium.org/586143002/diff/160001/components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java File components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java (right): https://codereview.chromium.org/586143002/diff/160001/components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java#newcode59 components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java:59: private long mTotalReceivedBytes = 0; On 2014/09/29 17:57:36, mef ...
6 years, 2 months ago (2014-09-29 18:10:34 UTC) #18
mmenke
https://codereview.chromium.org/586143002/diff/160001/components/cronet/android/java/src/org/chromium/net/UrlRequestListener.java File components/cronet/android/java/src/org/chromium/net/UrlRequestListener.java (right): https://codereview.chromium.org/586143002/diff/160001/components/cronet/android/java/src/org/chromium/net/UrlRequestListener.java#newcode56 components/cronet/android/java/src/org/chromium/net/UrlRequestListener.java:56: public void onComplete(UrlRequest request, ResponseInfo info); On 2014/09/29 18:10:34, ...
6 years, 2 months ago (2014-09-29 19:26:47 UTC) #19
mmenke
https://codereview.chromium.org/586143002/diff/160001/components/cronet/android/java/src/org/chromium/net/UrlRequestListener.java File components/cronet/android/java/src/org/chromium/net/UrlRequestListener.java (right): https://codereview.chromium.org/586143002/diff/160001/components/cronet/android/java/src/org/chromium/net/UrlRequestListener.java#newcode56 components/cronet/android/java/src/org/chromium/net/UrlRequestListener.java:56: public void onComplete(UrlRequest request, ResponseInfo info); On 2014/09/29 19:26:46, ...
6 years, 2 months ago (2014-09-29 19:28:36 UTC) #20
Charles
https://codereview.chromium.org/586143002/diff/160001/components/cronet/android/java/src/org/chromium/net/UrlRequestListener.java File components/cronet/android/java/src/org/chromium/net/UrlRequestListener.java (right): https://codereview.chromium.org/586143002/diff/160001/components/cronet/android/java/src/org/chromium/net/UrlRequestListener.java#newcode56 components/cronet/android/java/src/org/chromium/net/UrlRequestListener.java:56: public void onComplete(UrlRequest request, ResponseInfo info); On 2014/09/29 19:28:36, ...
6 years, 2 months ago (2014-09-29 19:52:50 UTC) #21
mmenke
On 2014/09/29 19:52:50, Charles wrote: > https://codereview.chromium.org/586143002/diff/160001/components/cronet/android/java/src/org/chromium/net/UrlRequestListener.java > File components/cronet/android/java/src/org/chromium/net/UrlRequestListener.java > (right): > > https://codereview.chromium.org/586143002/diff/160001/components/cronet/android/java/src/org/chromium/net/UrlRequestListener.java#newcode56 ...
6 years, 2 months ago (2014-09-29 20:03:05 UTC) #22
mef
On 2014/09/29 20:03:05, mmenke wrote: > On 2014/09/29 19:52:50, Charles wrote: > > > https://codereview.chromium.org/586143002/diff/160001/components/cronet/android/java/src/org/chromium/net/UrlRequestListener.java ...
6 years, 2 months ago (2014-09-29 20:16:30 UTC) #23
mmenke
On 2014/09/29 20:16:30, mef wrote: > On 2014/09/29 20:03:05, mmenke wrote: > > On 2014/09/29 ...
6 years, 2 months ago (2014-09-29 20:50:56 UTC) #24
mef
On 2014/09/29 20:50:56, mmenke wrote: > On 2014/09/29 20:16:30, mef wrote: > > On 2014/09/29 ...
6 years, 2 months ago (2014-09-29 21:01:24 UTC) #25
Charles
On 2014/09/29 21:01:24, mef wrote: > On 2014/09/29 20:50:56, mmenke wrote: > > On 2014/09/29 ...
6 years, 2 months ago (2014-09-29 21:09:56 UTC) #26
mmenke
On 2014/09/29 21:09:56, Charles wrote: > On 2014/09/29 21:01:24, mef wrote: > > On 2014/09/29 ...
6 years, 2 months ago (2014-09-29 21:14:32 UTC) #27
Charles
On 2014/09/29 21:14:32, mmenke wrote: > On 2014/09/29 21:09:56, Charles wrote: > > On 2014/09/29 ...
6 years, 2 months ago (2014-09-29 21:16:22 UTC) #28
mef
https://codereview.chromium.org/586143002/diff/160001/components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java File components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java (right): https://codereview.chromium.org/586143002/diff/160001/components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java#newcode59 components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java:59: private long mTotalReceivedBytes = 0; On 2014/09/29 18:10:34, Charles ...
6 years, 2 months ago (2014-09-29 21:18:21 UTC) #29
Charles
On 2014/09/29 21:18:21, mef wrote: > https://codereview.chromium.org/586143002/diff/160001/components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java > File components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java > (right): > > https://codereview.chromium.org/586143002/diff/160001/components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java#newcode59 ...
6 years, 2 months ago (2014-09-29 21:21:47 UTC) #30
mef
Thanks, PTAL. Per conversation with Matt I've kept UrlRequestListener request parameters. https://codereview.chromium.org/586143002/diff/160001/components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java File components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java (right): ...
6 years, 2 months ago (2014-09-30 20:23:15 UTC) #31
Charles
lgtm
6 years, 2 months ago (2014-09-30 20:27:10 UTC) #32
Charles
lgtm
6 years, 2 months ago (2014-09-30 20:27:11 UTC) #33
mef
On 2014/09/30 20:27:11, Charles wrote: > lgtm woot! Thanks!
6 years, 2 months ago (2014-09-30 20:33:09 UTC) #34
xunjieli
https://codereview.chromium.org/586143002/diff/220001/components/cronet/android/cronet_url_request_context.cc File components/cronet/android/cronet_url_request_context.cc (right): https://codereview.chromium.org/586143002/diff/220001/components/cronet/android/cronet_url_request_context.cc#newcode86 components/cronet/android/cronet_url_request_context.cc:86: base::android::InitApplicationContext(env, scoped_context); Just curious. Will embedders create multiple request ...
6 years, 2 months ago (2014-10-01 14:40:37 UTC) #35
mef
https://codereview.chromium.org/586143002/diff/220001/components/cronet/android/cronet_url_request_context.cc File components/cronet/android/cronet_url_request_context.cc (right): https://codereview.chromium.org/586143002/diff/220001/components/cronet/android/cronet_url_request_context.cc#newcode86 components/cronet/android/cronet_url_request_context.cc:86: base::android::InitApplicationContext(env, scoped_context); On 2014/10/01 14:40:37, xunjieli wrote: > Just ...
6 years, 2 months ago (2014-10-01 20:41:31 UTC) #36
xunjieli
On 2014/10/01 20:41:31, mef wrote: > https://codereview.chromium.org/586143002/diff/220001/components/cronet/android/cronet_url_request_context.cc > File components/cronet/android/cronet_url_request_context.cc (right): > > https://codereview.chromium.org/586143002/diff/220001/components/cronet/android/cronet_url_request_context.cc#newcode86 > ...
6 years, 2 months ago (2014-10-01 20:45:39 UTC) #37
mmenke
Sorry for the long delay - finally started reviewing this yesterday, hope to get back ...
6 years, 2 months ago (2014-10-02 15:01:27 UTC) #38
mmenke
Partial comments...One is a fairly significant refactor, which is what I'm sending them now. Also, ...
6 years, 2 months ago (2014-10-02 15:24:09 UTC) #39
mef
Thanks, Matt! I'll add more tests and appropriate logic from url_request_loader in the next rev. ...
6 years, 2 months ago (2014-10-02 22:07:43 UTC) #40
mmenke
Misha: This isn't really a full pass, just what I have at the moment. Feel ...
6 years, 2 months ago (2014-10-03 20:45:59 UTC) #41
xunjieli
some style nits. https://codereview.chromium.org/586143002/diff/240001/components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java File components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java (right): https://codereview.chromium.org/586143002/diff/240001/components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java#newcode195 components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java:195: if (mResponseInfo != null) It is ...
6 years, 2 months ago (2014-10-03 20:58:39 UTC) #42
mef
Thanks, PTAL. https://codereview.chromium.org/586143002/diff/240001/components/cronet/android/cronet_url_request_adapter.cc File components/cronet/android/cronet_url_request_adapter.cc (right): https://codereview.chromium.org/586143002/diff/240001/components/cronet/android/cronet_url_request_adapter.cc#newcode7 components/cronet/android/cronet_url_request_adapter.cc:7: #include <string.h> On 2014/10/03 20:45:59, mmenke wrote: ...
6 years, 2 months ago (2014-10-06 14:52:43 UTC) #43
mmenke
https://codereview.chromium.org/586143002/diff/300001/components/cronet/android/cronet_url_request.cc File components/cronet/android/cronet_url_request.cc (right): https://codereview.chromium.org/586143002/diff/300001/components/cronet/android/cronet_url_request.cc#newcode37 components/cronet/android/cronet_url_request.cc:37: } nit: Blank line after function. https://codereview.chromium.org/586143002/diff/300001/components/cronet/android/cronet_url_request.cc#newcode109 components/cronet/android/cronet_url_request.cc:109: DCHECK(context ...
6 years, 2 months ago (2014-10-06 18:28:45 UTC) #46
mef
Thanks! I've addressed most of the comments. I would like to better understand usage of ...
6 years, 2 months ago (2014-10-07 00:45:18 UTC) #47
mmenke
I think this looks pretty reasonable. https://codereview.chromium.org/586143002/diff/380001/net/test/url_request/url_request_mock_http_job.cc File net/test/url_request/url_request_mock_http_job.cc (right): https://codereview.chromium.org/586143002/diff/380001/net/test/url_request/url_request_mock_http_job.cc#newcode167 net/test/url_request/url_request_mock_http_job.cc:167: return false; Hrm...I ...
6 years, 2 months ago (2014-10-16 16:39:35 UTC) #48
mef
Matt, thanks! I've tweaked the mock http job, added tests to match, and will appreciate ...
6 years, 2 months ago (2014-10-16 21:36:50 UTC) #49
xunjieli
Some style nits from me. https://codereview.chromium.org/586143002/diff/400001/components/cronet/android/cronet_url_request.cc File components/cronet/android/cronet_url_request.cc (right): https://codereview.chromium.org/586143002/diff/400001/components/cronet/android/cronet_url_request.cc#newcode103 components/cronet/android/cronet_url_request.cc:103: jlong url_request_context_adapter, nit: I ...
6 years, 2 months ago (2014-10-17 00:31:36 UTC) #50
mef
Thanks, PTAL! https://codereview.chromium.org/586143002/diff/300001/components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java File components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java (right): https://codereview.chromium.org/586143002/diff/300001/components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java#newcode153 components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java:153: public void cancel() { On 2014/10/06 18:28:44, ...
6 years, 2 months ago (2014-10-17 20:19:43 UTC) #51
xunjieli
https://codereview.chromium.org/586143002/diff/400001/components/cronet/android/cronet_url_request_adapter.h File components/cronet/android/cronet_url_request_adapter.h (right): https://codereview.chromium.org/586143002/diff/400001/components/cronet/android/cronet_url_request_adapter.h#newcode20 components/cronet/android/cronet_url_request_adapter.h:20: class SingleThreadTaskRunner; On 2014/10/17 20:19:42, mef wrote: > On ...
6 years, 2 months ago (2014-10-17 20:32:22 UTC) #52
mmenke
https://codereview.chromium.org/586143002/diff/400001/components/cronet/android/cronet_url_request_adapter.h File components/cronet/android/cronet_url_request_adapter.h (right): https://codereview.chromium.org/586143002/diff/400001/components/cronet/android/cronet_url_request_adapter.h#newcode20 components/cronet/android/cronet_url_request_adapter.h:20: class SingleThreadTaskRunner; On 2014/10/17 20:32:22, xunjieli wrote: > On ...
6 years, 2 months ago (2014-10-17 20:38:12 UTC) #53
mef
PTAL, I've added GetMockUrlWithFailure method and tests to cancel outside of listener.
6 years, 2 months ago (2014-10-20 15:15:22 UTC) #54
mmenke
Not a full pass, mostly just looked at tests. Want to pick it up first ...
6 years, 2 months ago (2014-10-20 21:07:30 UTC) #55
mmenke
https://codereview.chromium.org/586143002/diff/460001/components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java File components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java (right): https://codereview.chromium.org/586143002/diff/460001/components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java#newcode62 components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java:62: private long mTotalReceivedBytes = 0; I'm still nervous about ...
6 years, 2 months ago (2014-10-20 21:23:05 UTC) #56
mef
Matt, thanks! I'll address your comments tomorrow. https://codereview.chromium.org/586143002/diff/460001/components/cronet/android/cronet_url_request.cc File components/cronet/android/cronet_url_request.cc (right): https://codereview.chromium.org/586143002/diff/460001/components/cronet/android/cronet_url_request.cc#newcode97 components/cronet/android/cronet_url_request.cc:97: jobject owner_; ...
6 years, 2 months ago (2014-10-20 22:30:09 UTC) #57
mmenke
https://codereview.chromium.org/586143002/diff/460001/components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java File components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java (right): https://codereview.chromium.org/586143002/diff/460001/components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java#newcode162 components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java:162: public void cancel() { On 2014/10/20 22:30:09, mef wrote: ...
6 years, 2 months ago (2014-10-21 14:28:02 UTC) #58
mef
https://codereview.chromium.org/586143002/diff/460001/components/cronet/android/cronet_url_request.h File components/cronet/android/cronet_url_request.h (right): https://codereview.chromium.org/586143002/diff/460001/components/cronet/android/cronet_url_request.h#newcode14 components/cronet/android/cronet_url_request.h:14: enum CronetUrlRequestPriority { On 2014/10/20 21:07:29, mmenke wrote: > ...
6 years, 2 months ago (2014-10-21 14:49:11 UTC) #59
mmenke
https://codereview.chromium.org/586143002/diff/460001/components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java File components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java (right): https://codereview.chromium.org/586143002/diff/460001/components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java#newcode162 components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java:162: public void cancel() { On 2014/10/21 14:49:11, mef wrote: ...
6 years, 2 months ago (2014-10-21 15:30:46 UTC) #60
mmenke
A couple more comments. May get back to this later today, may not. https://codereview.chromium.org/586143002/diff/480001/components/cronet/android/cronet_url_request.cc File ...
6 years, 2 months ago (2014-10-21 16:17:54 UTC) #61
mmenke
https://codereview.chromium.org/586143002/diff/480001/components/cronet/android/cronet_url_request.cc File components/cronet/android/cronet_url_request.cc (right): https://codereview.chromium.org/586143002/diff/480001/components/cronet/android/cronet_url_request.cc#newcode143 components/cronet/android/cronet_url_request.cc:143: DCHECK(!request_adapter->IsOnNetworkThread()); On 2014/10/21 16:17:53, mmenke wrote: > Check IsValidHeaderName ...
6 years, 2 months ago (2014-10-21 17:39:18 UTC) #62
mmenke
https://codereview.chromium.org/586143002/diff/480001/components/cronet/android/java/src/org/chromium/net/UrlRequestListener.java File components/cronet/android/java/src/org/chromium/net/UrlRequestListener.java (right): https://codereview.chromium.org/586143002/diff/480001/components/cronet/android/java/src/org/chromium/net/UrlRequestListener.java#newcode56 components/cronet/android/java/src/org/chromium/net/UrlRequestListener.java:56: public void onComplete(UrlRequest request, ExtendedResponseInfo info); Oh, we only ...
6 years, 2 months ago (2014-10-21 20:57:02 UTC) #63
mef
Matt, thanks! I've addressed some of your comments, but there are more tests to add... ...
6 years, 2 months ago (2014-10-21 21:27:48 UTC) #64
mef
PTAL, I believe I've addressed all comments and added more tests. https://codereview.chromium.org/586143002/diff/460001/components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java File components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java (right): ...
6 years, 2 months ago (2014-10-22 20:50:05 UTC) #65
xunjieli
https://codereview.chromium.org/586143002/diff/540001/components/cronet/android/cronet_url_request.cc File components/cronet/android/cronet_url_request.cc (right): https://codereview.chromium.org/586143002/diff/540001/components/cronet/android/cronet_url_request.cc#newcode74 components/cronet/android/cronet_url_request.cc:74: void OnRequestCanceled(CronetURLRequestAdapter* request_adapter) override { unused request_adapter. https://codereview.chromium.org/586143002/diff/540001/components/cronet/android/cronet_url_request.cc#newcode79 components/cronet/android/cronet_url_request.cc:79: ...
6 years, 2 months ago (2014-10-22 21:02:17 UTC) #66
xunjieli
a few more comments from me. https://codereview.chromium.org/586143002/diff/540001/components/cronet/android/cronet_url_request_context_adapter.h File components/cronet/android/cronet_url_request_context_adapter.h (right): https://codereview.chromium.org/586143002/diff/540001/components/cronet/android/cronet_url_request_context_adapter.h#newcode10 components/cronet/android/cronet_url_request_context_adapter.h:10: #include "base/compiler_specific.h" Is ...
6 years, 2 months ago (2014-10-23 14:04:33 UTC) #67
xunjieli
Sorry for the email spam. The CL is too big to review in one setting ...
6 years, 2 months ago (2014-10-23 15:51:22 UTC) #68
mmenke
https://codereview.chromium.org/586143002/diff/540001/components/cronet/android/cronet_url_request.cc File components/cronet/android/cronet_url_request.cc (right): https://codereview.chromium.org/586143002/diff/540001/components/cronet/android/cronet_url_request.cc#newcode69 components/cronet/android/cronet_url_request.cc:69: jenv, jenv->NewDirectByteBuffer(request_adapter->Data(), bytes_read)); I think it makes more sense ...
6 years, 2 months ago (2014-10-23 16:26:02 UTC) #69
mmenke
https://codereview.chromium.org/586143002/diff/540001/components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java File components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java (right): https://codereview.chromium.org/586143002/diff/540001/components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java#newcode125 components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java:125: CronetUrlRequest(CronetUrlRequestContext requestContext, On 2014/10/23 15:51:22, xunjieli wrote: > Just ...
6 years, 2 months ago (2014-10-23 21:36:42 UTC) #70
xunjieli
https://codereview.chromium.org/586143002/diff/540001/components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java File components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java (right): https://codereview.chromium.org/586143002/diff/540001/components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java#newcode125 components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java:125: CronetUrlRequest(CronetUrlRequestContext requestContext, On 2014/10/23 21:36:42, mmenke wrote: > On ...
6 years, 2 months ago (2014-10-23 21:44:46 UTC) #71
mef
Thanks, PTAL. I'll add tests for UserAgent and fix UrlRequestContext lifecycle in next patch. https://codereview.chromium.org/586143002/diff/540001/components/cronet/android/cronet_url_request.cc ...
6 years, 2 months ago (2014-10-24 03:31:45 UTC) #72
mmenke
quick responses https://codereview.chromium.org/586143002/diff/540001/components/cronet/android/cronet_url_request_adapter.cc File components/cronet/android/cronet_url_request_adapter.cc (right): https://codereview.chromium.org/586143002/diff/540001/components/cronet/android/cronet_url_request_adapter.cc#newcode132 components/cronet/android/cronet_url_request_adapter.cc:132: http_status_code_ = request->GetResponseCode(); On 2014/10/24 03:31:43, mef ...
6 years, 2 months ago (2014-10-24 03:46:19 UTC) #73
mmenke
Just looked at the tests - want to wait for cancellation to be fixed up ...
6 years, 2 months ago (2014-10-24 16:34:36 UTC) #74
xunjieli
The new Async API does not explicitly load the native library (via System.load()). We probably ...
6 years, 2 months ago (2014-10-24 19:38:49 UTC) #75
mef
Thanks, PTAL, I've fixed cancellation and destruction of requests and context, and will add more ...
6 years, 2 months ago (2014-10-24 19:57:56 UTC) #76
mmenke
https://codereview.chromium.org/586143002/diff/560001/components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/CronetUrlRequestTest.java File components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/CronetUrlRequestTest.java (right): https://codereview.chromium.org/586143002/diff/560001/components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/CronetUrlRequestTest.java#newcode170 components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/CronetUrlRequestTest.java:170: String methodName = "head"; On 2014/10/24 19:57:55, mef wrote: ...
6 years, 2 months ago (2014-10-24 20:16:22 UTC) #77
mmenke
Misha: One other option, that may be a little simpler. We could make our own ...
6 years, 2 months ago (2014-10-24 21:40:05 UTC) #78
mef
Thanks, PTAL, I believe that I've addressed all comments. https://codereview.chromium.org/586143002/diff/540001/components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java File components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java (right): https://codereview.chromium.org/586143002/diff/540001/components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java#newcode29 components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java:29: ...
6 years, 1 month ago (2014-10-27 21:00:03 UTC) #79
xunjieli
https://codereview.chromium.org/586143002/diff/600001/components/cronet.gypi File components/cronet.gypi (right): https://codereview.chromium.org/586143002/diff/600001/components/cronet.gypi#newcode30 components/cronet.gypi:30: 'includes': [ '../build/android/java_cpp_enum.gypi' ], nit: indent. https://codereview.chromium.org/586143002/diff/600001/components/cronet.gypi#newcode124 components/cronet.gypi:124: 'cronet_url_request_java_enum', ...
6 years, 1 month ago (2014-10-28 14:24:20 UTC) #80
mef
Thanks, PTAL! https://codereview.chromium.org/586143002/diff/600001/components/cronet.gypi File components/cronet.gypi (right): https://codereview.chromium.org/586143002/diff/600001/components/cronet.gypi#newcode30 components/cronet.gypi:30: 'includes': [ '../build/android/java_cpp_enum.gypi' ], On 2014/10/28 14:24:19, ...
6 years, 1 month ago (2014-10-28 16:29:01 UTC) #81
mef
On 2014/10/28 16:29:01, mef wrote: > Thanks, PTAL! > > https://codereview.chromium.org/586143002/diff/600001/components/cronet.gypi > File components/cronet.gypi (right): ...
6 years, 1 month ago (2014-10-28 16:30:23 UTC) #82
mmenke
https://codereview.chromium.org/586143002/diff/620001/components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java File components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java (right): https://codereview.chromium.org/586143002/diff/620001/components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java#newcode257 components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java:257: nativeDestroyRequestAdapter(urlRequestAdapter); Per previous discussions, this isn't threadsafe. I'd really ...
6 years, 1 month ago (2014-10-28 18:07:52 UTC) #83
mef
PTAL. https://chromiumcodereview.appspot.com/586143002/diff/620001/components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java File components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java (right): https://chromiumcodereview.appspot.com/586143002/diff/620001/components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java#newcode257 components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java:257: nativeDestroyRequestAdapter(urlRequestAdapter); On 2014/10/28 18:07:52, mmenke wrote: > Per ...
6 years, 1 month ago (2014-10-28 21:04:25 UTC) #84
mmenke
Plan to do another pass this afternoon. Really focused on thread safety Java-side on this ...
6 years, 1 month ago (2014-10-29 16:20:48 UTC) #85
mmenke
https://codereview.chromium.org/586143002/diff/660001/components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java File components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java (right): https://codereview.chromium.org/586143002/diff/660001/components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java#newcode304 components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java:304: mListener.onError(this, mResponseInfo, requestError); On 2014/10/29 16:20:47, mmenke wrote: > ...
6 years, 1 month ago (2014-10-29 16:24:35 UTC) #86
mef
Thanks, PTAL. https://codereview.chromium.org/586143002/diff/660001/components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java File components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java (right): https://codereview.chromium.org/586143002/diff/660001/components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java#newcode28 components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java:28: private final Object mUrlRequestAdapterLock = new Object(); ...
6 years, 1 month ago (2014-10-29 19:05:01 UTC) #87
mmenke
I think things are really shaping up. Need to do another pass over the C++ ...
6 years, 1 month ago (2014-10-29 21:40:30 UTC) #88
mef
Thanks, PTAL. https://codereview.chromium.org/586143002/diff/680001/components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java File components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java (right): https://codereview.chromium.org/586143002/diff/680001/components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java#newcode28 components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java:28: private final Object mUrlRequestAdapterLock = new Object(); ...
6 years, 1 month ago (2014-10-30 03:01:32 UTC) #89
mef
https://codereview.chromium.org/586143002/diff/680001/components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java File components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java (right): https://codereview.chromium.org/586143002/diff/680001/components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java#newcode171 components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java:171: if (method == null) { On 2014/10/30 03:01:32, mef ...
6 years, 1 month ago (2014-10-30 14:53:17 UTC) #90
mmenke
Note: I have a fair number of other reviews to do today, so probably won't ...
6 years, 1 month ago (2014-10-30 15:15:55 UTC) #91
mmenke
More comments, will get back to this tomorrow. https://codereview.chromium.org/586143002/diff/680001/components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/CronetUrlRequestTest.java File components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/CronetUrlRequestTest.java (right): https://codereview.chromium.org/586143002/diff/680001/components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/CronetUrlRequestTest.java#newcode26 components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/CronetUrlRequestTest.java:26: import ...
6 years, 1 month ago (2014-10-30 21:47:57 UTC) #92
mef
Thanks, PTAL! https://codereview.chromium.org/586143002/diff/730001/components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java File components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java (right): https://codereview.chromium.org/586143002/diff/730001/components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java#newcode465 components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java:465: destroyRequestAdapter(); On 2014/10/30 21:47:56, mmenke wrote: > ...
6 years, 1 month ago (2014-10-30 22:32:56 UTC) #93
mmenke
Still reviewing, though I have a couple other reviews I need to do today, too. ...
6 years, 1 month ago (2014-10-31 15:49:24 UTC) #94
mmenke
More nits (Sorry for all the nits, but minor followup cleanups tend to be rarely ...
6 years, 1 month ago (2014-10-31 19:16:23 UTC) #95
xunjieli
https://codereview.chromium.org/586143002/diff/680001/components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java File components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java (right): https://codereview.chromium.org/586143002/diff/680001/components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java#newcode208 components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java:208: + header.getKey() + "=" + header.getValue()); On 2014/10/30 03:01:31, ...
6 years, 1 month ago (2014-10-31 19:31:23 UTC) #96
xunjieli
A couple more comments.Looks good! https://codereview.chromium.org/586143002/diff/750001/components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java File components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java (right): https://codereview.chromium.org/586143002/diff/750001/components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java#newcode257 components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java:257: * Post task to ...
6 years, 1 month ago (2014-10-31 19:57:21 UTC) #97
mef
Thanks! https://codereview.chromium.org/586143002/diff/750001/components/cronet/android/cronet_url_request.cc File components/cronet/android/cronet_url_request.cc (right): https://codereview.chromium.org/586143002/diff/750001/components/cronet/android/cronet_url_request.cc#newcode48 components/cronet/android/cronet_url_request.cc:48: owner_.Reset(jenv, jowner); On 2014/10/31 19:16:22, mmenke wrote: > ...
6 years, 1 month ago (2014-10-31 20:39:17 UTC) #98
xunjieli
https://codereview.chromium.org/586143002/diff/770001/components/cronet/android/cronet_url_request_adapter.cc File components/cronet/android/cronet_url_request_adapter.cc (right): https://codereview.chromium.org/586143002/diff/770001/components/cronet/android/cronet_url_request_adapter.cc#newcode176 components/cronet/android/cronet_url_request_adapter.cc:176: bool CronetURLRequestAdapter::MaybeReportError(net::URLRequest* request) const { nit: I often find ...
6 years, 1 month ago (2014-10-31 21:01:51 UTC) #99
mmenke
Quick responses to your responses. I don't think we're going to make it to 1000 ...
6 years, 1 month ago (2014-10-31 21:03:25 UTC) #100
mmenke
Oh, something else to test...What happens when we get weird characters in a header, both ...
6 years, 1 month ago (2014-10-31 21:07:17 UTC) #101
mef
Thanks, PTAL! https://codereview.chromium.org/586143002/diff/750001/components/cronet/android/cronet_url_request_adapter.h File components/cronet/android/cronet_url_request_adapter.h (right): https://codereview.chromium.org/586143002/diff/750001/components/cronet/android/cronet_url_request_adapter.h#newcode39 components/cronet/android/cronet_url_request_adapter.h:39: : public base::RefCountedThreadSafe<CronetURLRequestAdapterDelegate> { On 2014/10/31 19:16:23, ...
6 years, 1 month ago (2014-10-31 21:43:16 UTC) #102
xunjieli
https://codereview.chromium.org/586143002/diff/770001/components/cronet/android/cronet_url_request_adapter.cc File components/cronet/android/cronet_url_request_adapter.cc (right): https://codereview.chromium.org/586143002/diff/770001/components/cronet/android/cronet_url_request_adapter.cc#newcode176 components/cronet/android/cronet_url_request_adapter.cc:176: bool CronetURLRequestAdapter::MaybeReportError(net::URLRequest* request) const { On 2014/10/31 21:43:15, mef ...
6 years, 1 month ago (2014-11-03 14:26:06 UTC) #103
mmenke
https://codereview.chromium.org/586143002/diff/770001/components/cronet/android/cronet_url_request_adapter.cc File components/cronet/android/cronet_url_request_adapter.cc (right): https://codereview.chromium.org/586143002/diff/770001/components/cronet/android/cronet_url_request_adapter.cc#newcode176 components/cronet/android/cronet_url_request_adapter.cc:176: bool CronetURLRequestAdapter::MaybeReportError(net::URLRequest* request) const { On 2014/11/03 14:26:05, xunjieli ...
6 years, 1 month ago (2014-11-03 16:26:10 UTC) #104
mef
https://codereview.chromium.org/586143002/diff/770001/components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/TestUrlRequestListener.java File components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/TestUrlRequestListener.java (right): https://codereview.chromium.org/586143002/diff/770001/components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/TestUrlRequestListener.java#newcode71 components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/TestUrlRequestListener.java:71: OnComplete On 2014/11/03 14:26:05, xunjieli wrote: > On 2014/10/31 ...
6 years, 1 month ago (2014-11-03 16:42:19 UTC) #105
mmenke
https://codereview.chromium.org/586143002/diff/790001/components/cronet/android/cronet_url_request_adapter.cc File components/cronet/android/cronet_url_request_adapter.cc (right): https://codereview.chromium.org/586143002/diff/790001/components/cronet/android/cronet_url_request_adapter.cc#newcode36 components/cronet/android/cronet_url_request_adapter.cc:36: CronetURLRequestAdapter::~CronetURLRequestAdapter() { DCHECK(IsOnNetworkThread()); https://codereview.chromium.org/586143002/diff/790001/components/cronet/android/cronet_url_request_adapter.cc#newcode40 components/cronet/android/cronet_url_request_adapter.cc:40: const std::string& value) { ...
6 years, 1 month ago (2014-11-03 17:13:17 UTC) #106
mmenke
Think we're getting to the bottom of the barrel now. https://codereview.chromium.org/586143002/diff/810001/components/cronet/android/cronet_url_request.cc File components/cronet/android/cronet_url_request.cc (right): https://codereview.chromium.org/586143002/diff/810001/components/cronet/android/cronet_url_request.cc#newcode43 ...
6 years, 1 month ago (2014-11-03 20:21:17 UTC) #107
xunjieli
https://codereview.chromium.org/586143002/diff/810001/components/cronet/android/cronet_url_request.cc File components/cronet/android/cronet_url_request.cc (right): https://codereview.chromium.org/586143002/diff/810001/components/cronet/android/cronet_url_request.cc#newcode106 components/cronet/android/cronet_url_request.cc:106: static jlong CreateRequestAdapter(JNIEnv* jenv, On 2014/11/03 20:21:16, mmenke wrote: ...
6 years, 1 month ago (2014-11-03 20:27:49 UTC) #108
mef
850 comments down, 150 to go! https://codereview.chromium.org/586143002/diff/790001/components/cronet/android/cronet_url_request_adapter.cc File components/cronet/android/cronet_url_request_adapter.cc (right): https://codereview.chromium.org/586143002/diff/790001/components/cronet/android/cronet_url_request_adapter.cc#newcode36 components/cronet/android/cronet_url_request_adapter.cc:36: CronetURLRequestAdapter::~CronetURLRequestAdapter() { On ...
6 years, 1 month ago (2014-11-03 21:23:38 UTC) #109
mmenke
Quick responses. https://codereview.chromium.org/586143002/diff/790001/components/cronet/android/java/src/org/chromium/net/UrlRequestContext.java File components/cronet/android/java/src/org/chromium/net/UrlRequestContext.java (right): https://codereview.chromium.org/586143002/diff/790001/components/cronet/android/java/src/org/chromium/net/UrlRequestContext.java#newcode29 components/cronet/android/java/src/org/chromium/net/UrlRequestContext.java:29: * @param executor Executor on which all ...
6 years, 1 month ago (2014-11-03 21:42:06 UTC) #110
mef
Getting there! https://codereview.chromium.org/586143002/diff/790001/components/cronet/android/java/src/org/chromium/net/UrlRequestContext.java File components/cronet/android/java/src/org/chromium/net/UrlRequestContext.java (right): https://codereview.chromium.org/586143002/diff/790001/components/cronet/android/java/src/org/chromium/net/UrlRequestContext.java#newcode29 components/cronet/android/java/src/org/chromium/net/UrlRequestContext.java:29: * @param executor Executor on which all ...
6 years, 1 month ago (2014-11-03 23:27:00 UTC) #111
mmenke
More comments. Was thinking we wouldn't make it to 1000, but now I'm thinking we ...
6 years, 1 month ago (2014-11-06 16:08:28 UTC) #112
mmenke
https://codereview.chromium.org/586143002/diff/870001/components/cronet/android/cronet_url_request.cc File components/cronet/android/cronet_url_request.cc (right): https://codereview.chromium.org/586143002/diff/870001/components/cronet/android/cronet_url_request.cc#newcode39 components/cronet/android/cronet_url_request.cc:39: ConvertUTF8ToJavaString(env, new_location.spec()).Release(), Again, think this is a leak - ...
6 years, 1 month ago (2014-11-06 16:11:01 UTC) #113
xunjieli
just some style nits :) https://codereview.chromium.org/586143002/diff/870001/components/cronet/android/cronet_url_request_adapter.cc File components/cronet/android/cronet_url_request_adapter.cc (right): https://codereview.chromium.org/586143002/diff/870001/components/cronet/android/cronet_url_request_adapter.cc#newcode27 components/cronet/android/cronet_url_request_adapter.cc:27: : context_(context), a totally ...
6 years, 1 month ago (2014-11-06 17:02:37 UTC) #114
mef
Thanks, quick comments. https://codereview.chromium.org/586143002/diff/870001/components/cronet/android/cronet_url_request.cc File components/cronet/android/cronet_url_request.cc (right): https://codereview.chromium.org/586143002/diff/870001/components/cronet/android/cronet_url_request.cc#newcode39 components/cronet/android/cronet_url_request.cc:39: ConvertUTF8ToJavaString(env, new_location.spec()).Release(), On 2014/11/06 16:11:01, mmenke ...
6 years, 1 month ago (2014-11-06 17:07:47 UTC) #115
mmenke
If you respond to all outstanding comments, we'll be just shy of 1000. :) https://codereview.chromium.org/586143002/diff/870001/components/cronet/android/cronet_url_request.cc ...
6 years, 1 month ago (2014-11-06 17:31:29 UTC) #116
mmenke
A couple more minor comments... Done reviewing for the day. https://codereview.chromium.org/586143002/diff/870001/components/cronet/android/cronet_url_request_context_adapter.cc File components/cronet/android/cronet_url_request_context_adapter.cc (right): https://codereview.chromium.org/586143002/diff/870001/components/cronet/android/cronet_url_request_context_adapter.cc#newcode151 ...
6 years, 1 month ago (2014-11-06 20:50:15 UTC) #117
mef
Just 8 more! https://codereview.chromium.org/586143002/diff/870001/components/cronet/android/cronet_url_request.cc File components/cronet/android/cronet_url_request.cc (right): https://codereview.chromium.org/586143002/diff/870001/components/cronet/android/cronet_url_request.cc#newcode21 components/cronet/android/cronet_url_request.cc:21: On 2014/11/06 16:08:26, mmenke wrote: > ...
6 years, 1 month ago (2014-11-06 22:51:47 UTC) #118
mmenke
Think we shouldn't change the interface in this CL, but I've been thinking about Charles' ...
6 years, 1 month ago (2014-11-06 23:05:00 UTC) #119
mef
I thought that as net/ uses refcounted IOBuffer that could live beyond request, it can't ...
6 years, 1 month ago (2014-11-06 23:16:53 UTC) #120
mmenke
https://codereview.chromium.org/586143002/diff/890001/components/cronet/android/cronet_url_request.cc File components/cronet/android/cronet_url_request.cc (right): https://codereview.chromium.org/586143002/diff/890001/components/cronet/android/cronet_url_request.cc#newcode59 components/cronet/android/cronet_url_request.cc:59: base::android::ScopedJavaLocalRef<jobject> java_buffer( On 2014/11/06 23:16:53, mef wrote: > So, ...
6 years, 1 month ago (2014-11-06 23:35:06 UTC) #121
mmenke
I think this is good enough, for the moment, though suppose I am still finding ...
6 years, 1 month ago (2014-11-07 15:19:09 UTC) #122
mef
Achievement unlocked: 1k comments! https://codereview.chromium.org/586143002/diff/890001/components/cronet/android/cronet_url_request.cc File components/cronet/android/cronet_url_request.cc (right): https://codereview.chromium.org/586143002/diff/890001/components/cronet/android/cronet_url_request.cc#newcode56 components/cronet/android/cronet_url_request.cc:56: void OnBytesRead(unsigned char* bytes_buffer, On ...
6 years, 1 month ago (2014-11-07 16:22:26 UTC) #123
mmenke
Think this has gone on enough. LGTM. Can deal with other issues as they come ...
6 years, 1 month ago (2014-11-07 16:25:39 UTC) #124
mmenke
https://codereview.chromium.org/586143002/diff/910001/components/cronet/android/cronet_url_request_adapter.cc File components/cronet/android/cronet_url_request_adapter.cc (right): https://codereview.chromium.org/586143002/diff/910001/components/cronet/android/cronet_url_request_adapter.cc#newcode145 components/cronet/android/cronet_url_request_adapter.cc:145: bool CronetURLRequestAdapter::IsOnNetworkThread() const { definition order must match declaration ...
6 years, 1 month ago (2014-11-07 16:28:07 UTC) #125
xunjieli
lgtm-ed before. two more nits. hope to see this landed today! https://codereview.chromium.org/586143002/diff/910001/components/cronet/android/cronet_url_request.cc File components/cronet/android/cronet_url_request.cc (right): ...
6 years, 1 month ago (2014-11-07 16:28:40 UTC) #126
mef
Yay! Thanks! https://codereview.chromium.org/586143002/diff/910001/components/cronet/android/cronet_url_request.cc File components/cronet/android/cronet_url_request.cc (right): https://codereview.chromium.org/586143002/diff/910001/components/cronet/android/cronet_url_request.cc#newcode23 components/cronet/android/cronet_url_request.cc:23: // between the Java CronetURLRequest and C++ ...
6 years, 1 month ago (2014-11-07 16:33:40 UTC) #127
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/586143002/950001
6 years, 1 month ago (2014-11-07 16:42:44 UTC) #129
commit-bot: I haz the power
Committed patchset #46 (id:950001)
6 years, 1 month ago (2014-11-07 17:47:00 UTC) #130
commit-bot: I haz the power
6 years, 1 month ago (2014-11-07 17:48:18 UTC) #131
Message was sent while issue was closed.
Patchset 46 (id:??) landed as
https://crrev.com/d190710e56c9399e0bc1e9a88b26093190974c4d
Cr-Commit-Position: refs/heads/master@{#303252}

Powered by Google App Engine
This is Rietveld 408576698