Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
145213003:
Initial upload of cronet for Android. (Closed)
|
Created:
6 years, 11 months ago by mef Modified:
6 years, 9 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Description- Copied existing C++ and Java JNI implementation.
- Fixed compilation errors, added custom cflags and ldflags.
- Added cronet_package target to build libcronet.so and cronet.jar.
- Added cronet_sample_apk target to build CronetSample.apk sample app.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=257131
Patch Set 1 : Initial upload #Patch Set 2 : Move and rename some files #Patch Set 3 : Added cronet_unittests_apk target. #Patch Set 4 : Added cronet sample apk. Fixed startup issues. #Patch Set 5 : Get Cronet Sample apk to complete request. #Patch Set 6 : Better Cronet Sample UI. #Patch Set 7 : Fixed cronet_jni_headers. #Patch Set 8 : Applied clm's cflags and ldflags. #Patch Set 9 : Added cronet_package target to copy cronet.jar and stripped libcronet.so #
Total comments: 2
Patch Set 10 : #Patch Set 11 : #Patch Set 12 : Replaced android_log with chromium VLOG. #Patch Set 13 : Initialize ICU, upstream netjni bugfix. #Patch Set 14 : Sync to r251102 #Patch Set 15 : Fix java and clang compilation errors. #Patch Set 16 : Fix problems with component=shared_library (on android_clang_dbg bot). #Patch Set 17 : NET_EXPORT RequestPriorityToString to avoid buildbot errors. #
Total comments: 174
Patch Set 18 : Address some codereview comments. #Patch Set 19 : Address code review comments. #
Total comments: 16
Patch Set 20 : Reupload to fix xml files. #Patch Set 21 : Fix codereview comments and findbugs warnings. #
Total comments: 1
Patch Set 22 : Store clone of data in UrlRequest mPostBody. #
Total comments: 2
Patch Set 23 : Don't store post data in UrlRequest, pass it to native. #Patch Set 24 : Get testPostData to execute. It gets 411 from test server for now. #
Total comments: 10
Patch Set 25 : Address codereview comments. #Patch Set 26 : Changed copyright year on sample code back to 2012 #Messages
Total messages: 70 (0 generated)
Hi, please take a look. It is 'private' issue, only visible by @chromium and @google accounts. It builds libcrnet.so (whopping 153mb in size) and crnet.jar. I'll appreciate any comments, especially in regards to location and naming. thanks, -m
How big is the Release build? On Thu, Jan 23, 2014 at 7:47 AM, <mef@chromium.org> wrote: > Reviewers: mmenke, szym, cbentzel, dplotnikov_google.com, clm_google.com, > > Message: > Hi, > > please take a look. It is 'private' issue, only visible by @chromium and > @google > accounts. > > It builds libcrnet.so (whopping 153mb in size) and crnet.jar. > > I'll appreciate any comments, especially in regards to location and naming. > > thanks, > -m > > Description: > - Copied existing C++ and Java JNI implementation. > - Added crnet target to net.gyp. > - Fixed compilation errors. > > > Please review this at https://codereview.chromium.org/145213003/ > > SVN Base: svn://svn.chromium.org/chrome/trunk/src > > Affected files (+1773, -0 lines): > A net/crnet/java/src/UrlRequest.java > A net/crnet/java/src/UrlRequestContext.java > A net/crnet/jni/android_log.h > A net/crnet/jni/org_chromium_netjni_UrlRequest.h > A net/crnet/jni/org_chromium_netjni_UrlRequest.cpp > A net/crnet/jni/org_chromium_netjni_UrlRequestContext.h > A net/crnet/jni/org_chromium_netjni_UrlRequestContext.cpp > A net/crnet/jni/url_request_context_peer.h > A net/crnet/jni/url_request_context_peer.cpp > A net/crnet/jni/url_request_peer.h > A net/crnet/jni/url_request_peer.cpp > M net/net.gyp > > > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
153mb is release, OTOH libcontent_shell_content_view.so is 1917mb, so there is clearly something fishy. Interestingly libcontent_shell_content_view.so inside of apk is only 33mb, so the sizes that I see probably contain lots of debug info... On Thu, Jan 23, 2014 at 10:48 AM, Szymon Jakubczak <szym@chromium.org>wrote: > How big is the Release build? > > > On Thu, Jan 23, 2014 at 7:47 AM, <mef@chromium.org> wrote: > >> Reviewers: mmenke, szym, cbentzel, dplotnikov_google.com, clm_google.com, >> >> Message: >> Hi, >> >> please take a look. It is 'private' issue, only visible by @chromium and >> @google >> accounts. >> >> It builds libcrnet.so (whopping 153mb in size) and crnet.jar. >> >> I'll appreciate any comments, especially in regards to location and >> naming. >> >> thanks, >> -m >> >> Description: >> - Copied existing C++ and Java JNI implementation. >> - Added crnet target to net.gyp. >> - Fixed compilation errors. >> >> >> Please review this at https://codereview.chromium.org/145213003/ >> >> SVN Base: svn://svn.chromium.org/chrome/trunk/src >> >> Affected files (+1773, -0 lines): >> A net/crnet/java/src/UrlRequest.java >> A net/crnet/java/src/UrlRequestContext.java >> A net/crnet/jni/android_log.h >> A net/crnet/jni/org_chromium_netjni_UrlRequest.h >> A net/crnet/jni/org_chromium_netjni_UrlRequest.cpp >> A net/crnet/jni/org_chromium_netjni_UrlRequestContext.h >> A net/crnet/jni/org_chromium_netjni_UrlRequestContext.cpp >> A net/crnet/jni/url_request_context_peer.h >> A net/crnet/jni/url_request_context_peer.cpp >> A net/crnet/jni/url_request_peer.h >> A net/crnet/jni/url_request_peer.cpp >> M net/net.gyp >> >> >> > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
May take me a while to dig through all this, but one quick comment: Maybe the directory should be called libcrnet, to try and make its function a little clearer, and to match the target name? On 2014/01/23 15:49:06, szym wrote: > How big is the Release build? > > > On Thu, Jan 23, 2014 at 7:47 AM, <mailto:mef@chromium.org> wrote: > > > Reviewers: mmenke, szym, cbentzel, http://dplotnikov_google.com, http://clm_google.com, > > > > Message: > > Hi, > > > > please take a look. It is 'private' issue, only visible by @chromium and > > @google > > accounts. > > > > It builds libcrnet.so (whopping 153mb in size) and crnet.jar. > > > > I'll appreciate any comments, especially in regards to location and naming. > > > > thanks, > > -m > > > > Description: > > - Copied existing C++ and Java JNI implementation. > > - Added crnet target to net.gyp. > > - Fixed compilation errors. > > > > > > Please review this at https://codereview.chromium.org/145213003/ > > > > SVN Base: svn://svn.chromium.org/chrome/trunk/src > > > > Affected files (+1773, -0 lines): > > A net/crnet/java/src/UrlRequest.java > > A net/crnet/java/src/UrlRequestContext.java > > A net/crnet/jni/android_log.h > > A net/crnet/jni/org_chromium_netjni_UrlRequest.h > > A net/crnet/jni/org_chromium_netjni_UrlRequest.cpp > > A net/crnet/jni/org_chromium_netjni_UrlRequestContext.h > > A net/crnet/jni/org_chromium_netjni_UrlRequestContext.cpp > > A net/crnet/jni/url_request_context_peer.h > > A net/crnet/jni/url_request_context_peer.cpp > > A net/crnet/jni/url_request_peer.h > > A net/crnet/jni/url_request_peer.cpp > > M net/net.gyp > > > > > > > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org.
Maybe have to make buildtype official to get rid of symbols? On 2014/01/23 15:55:31, mef_google.com wrote: > 153mb is release, OTOH libcontent_shell_content_view.so is 1917mb, so there > is clearly something fishy. > > Interestingly libcontent_shell_content_view.so inside of apk is only 33mb, > so the sizes that I see probably contain lots of debug info... > > > On Thu, Jan 23, 2014 at 10:48 AM, Szymon Jakubczak <szym@chromium.org>wrote: > > > How big is the Release build? > > > > > > On Thu, Jan 23, 2014 at 7:47 AM, <mailto:mef@chromium.org> wrote: > > > >> Reviewers: mmenke, szym, cbentzel, http://dplotnikov_google.com, http://clm_google.com, > >> > >> Message: > >> Hi, > >> > >> please take a look. It is 'private' issue, only visible by @chromium and > >> @google > >> accounts. > >> > >> It builds libcrnet.so (whopping 153mb in size) and crnet.jar. > >> > >> I'll appreciate any comments, especially in regards to location and > >> naming. > >> > >> thanks, > >> -m > >> > >> Description: > >> - Copied existing C++ and Java JNI implementation. > >> - Added crnet target to net.gyp. > >> - Fixed compilation errors. > >> > >> > >> Please review this at https://codereview.chromium.org/145213003/ > >> > >> SVN Base: svn://svn.chromium.org/chrome/trunk/src > >> > >> Affected files (+1773, -0 lines): > >> A net/crnet/java/src/UrlRequest.java > >> A net/crnet/java/src/UrlRequestContext.java > >> A net/crnet/jni/android_log.h > >> A net/crnet/jni/org_chromium_netjni_UrlRequest.h > >> A net/crnet/jni/org_chromium_netjni_UrlRequest.cpp > >> A net/crnet/jni/org_chromium_netjni_UrlRequestContext.h > >> A net/crnet/jni/org_chromium_netjni_UrlRequestContext.cpp > >> A net/crnet/jni/url_request_context_peer.h > >> A net/crnet/jni/url_request_context_peer.cpp > >> A net/crnet/jni/url_request_peer.h > >> A net/crnet/jni/url_request_peer.cpp > >> M net/net.gyp > >> > >> > >> > > > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org.
We will definitely need a shippable build target -- that is without symbols. Adding 'buildtype': 'Official' to the target variables should suffice. On Thu, Jan 23, 2014 at 7:57 AM, <mmenke@chromium.org> wrote: > Maybe have to make buildtype official to get rid of symbols? > > > On 2014/01/23 15:55:31, mef_google.com wrote: > >> 153mb is release, OTOH libcontent_shell_content_view.so is 1917mb, so >> there >> is clearly something fishy. >> > > Interestingly libcontent_shell_content_view.so inside of apk is only >> 33mb, >> so the sizes that I see probably contain lots of debug info... >> > > > On Thu, Jan 23, 2014 at 10:48 AM, Szymon Jakubczak <szym@chromium.org >> >wrote: >> > > > How big is the Release build? >> > >> > >> > On Thu, Jan 23, 2014 at 7:47 AM, <mailto:mef@chromium.org> wrote: >> > >> >> Reviewers: mmenke, szym, cbentzel, http://dplotnikov_google.com, >> > http://clm_google.com, > >> >> >> >> Message: >> >> Hi, >> >> >> >> please take a look. It is 'private' issue, only visible by @chromium >> and >> >> @google >> >> accounts. >> >> >> >> It builds libcrnet.so (whopping 153mb in size) and crnet.jar. >> >> >> >> I'll appreciate any comments, especially in regards to location and >> >> naming. >> >> >> >> thanks, >> >> -m >> >> >> >> Description: >> >> - Copied existing C++ and Java JNI implementation. >> >> - Added crnet target to net.gyp. >> >> - Fixed compilation errors. >> >> >> >> >> >> Please review this at https://codereview.chromium.org/145213003/ >> >> >> >> SVN Base: svn://svn.chromium.org/chrome/trunk/src >> >> >> >> Affected files (+1773, -0 lines): >> >> A net/crnet/java/src/UrlRequest.java >> >> A net/crnet/java/src/UrlRequestContext.java >> >> A net/crnet/jni/android_log.h >> >> A net/crnet/jni/org_chromium_netjni_UrlRequest.h >> >> A net/crnet/jni/org_chromium_netjni_UrlRequest.cpp >> >> A net/crnet/jni/org_chromium_netjni_UrlRequestContext.h >> >> A net/crnet/jni/org_chromium_netjni_UrlRequestContext.cpp >> >> A net/crnet/jni/url_request_context_peer.h >> >> A net/crnet/jni/url_request_context_peer.cpp >> >> A net/crnet/jni/url_request_peer.h >> >> A net/crnet/jni/url_request_peer.cpp >> >> M net/net.gyp >> >> >> >> >> >> >> > >> > > To unsubscribe from this group and stop receiving emails from it, send an >> > email > >> to mailto:chromium-reviews+unsubscribe@chromium.org. >> > > > > https://codereview.chromium.org/145213003/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Doesn't look like the code is using URLRequestContextBuilder. Shouldn't worry about it for now, since we know this works, but longer term, we should probably switch to it, so we have less unique code. Quick comments on small things to clean up: * #ifdefs are the top of files use the wrong paths. * We should get rid of android_log, and use standard chrome logging functions. If the base logging functions don't work well when using net as a library, we should figure out a more general solution. * annonymous namespaces are generally preferred over static functions / globals, particularly when there are a lot of them. * No clue about Java style, but at least elsewhere, comments should probably use C++-style rather than C-style, to be consistent with Chromium style.
On 2014/01/23 15:55:56, mmenke wrote: > May take me a while to dig through all this, but one quick comment: Maybe the > directory should be called libcrnet, to try and make its function a little > clearer, and to match the target name? > > On 2014/01/23 15:49:06, szym wrote: > > How big is the Release build? > > > > > > On Thu, Jan 23, 2014 at 7:47 AM, <mailto:mef@chromium.org> wrote: > > > > > Reviewers: mmenke, szym, cbentzel, http://dplotnikov_google.com, > http://clm_google.com, > > > > > > Message: > > > Hi, > > > > > > please take a look. It is 'private' issue, only visible by @chromium and > > > @google > > > accounts. > > > > > > It builds libcrnet.so (whopping 153mb in size) and crnet.jar. > > > > > > I'll appreciate any comments, especially in regards to location and naming. > > > > > > thanks, > > > -m > > > > > > Description: > > > - Copied existing C++ and Java JNI implementation. > > > - Added crnet target to net.gyp. > > > - Fixed compilation errors. > > > > > > > > > Please review this at https://codereview.chromium.org/145213003/ > > > > > > SVN Base: svn://svn.chromium.org/chrome/trunk/src > > > > > > Affected files (+1773, -0 lines): > > > A net/crnet/java/src/UrlRequest.java > > > A net/crnet/java/src/UrlRequestContext.java > > > A net/crnet/jni/android_log.h > > > A net/crnet/jni/org_chromium_netjni_UrlRequest.h > > > A net/crnet/jni/org_chromium_netjni_UrlRequest.cpp > > > A net/crnet/jni/org_chromium_netjni_UrlRequestContext.h > > > A net/crnet/jni/org_chromium_netjni_UrlRequestContext.cpp > > > A net/crnet/jni/url_request_context_peer.h > > > A net/crnet/jni/url_request_context_peer.cpp > > > A net/crnet/jni/url_request_peer.h > > > A net/crnet/jni/url_request_peer.cpp > > > M net/net.gyp > > > > > > > > > > > > > To unsubscribe from this group and stop receiving emails from it, send an > email > > to mailto:chromium-reviews+unsubscribe@chromium.org. I don't think so as there are multiple targets for Android (so, jar, etc) and there will be additional targets for iOS. Speaking about iOS, should java/ and jni/ go under net/crnet/android? Then net/crnet/ios would have framework/, etc?
On 2014/01/23 16:33:52, mef wrote: > On 2014/01/23 15:55:56, mmenke wrote: > > May take me a while to dig through all this, but one quick comment: Maybe the > > directory should be called libcrnet, to try and make its function a little > > clearer, and to match the target name? > > > > On 2014/01/23 15:49:06, szym wrote: > > > How big is the Release build? > > > > > > > > > On Thu, Jan 23, 2014 at 7:47 AM, <mailto:mef@chromium.org> wrote: > > > > > > > Reviewers: mmenke, szym, cbentzel, http://dplotnikov_google.com, > > http://clm_google.com, > > > > > > > > Message: > > > > Hi, > > > > > > > > please take a look. It is 'private' issue, only visible by @chromium and > > > > @google > > > > accounts. > > > > > > > > It builds libcrnet.so (whopping 153mb in size) and crnet.jar. > > > > > > > > I'll appreciate any comments, especially in regards to location and > naming. > > > > > > > > thanks, > > > > -m > > > > > > > > Description: > > > > - Copied existing C++ and Java JNI implementation. > > > > - Added crnet target to net.gyp. > > > > - Fixed compilation errors. > > > > > > > > > > > > Please review this at https://codereview.chromium.org/145213003/ > > > > > > > > SVN Base: svn://svn.chromium.org/chrome/trunk/src > > > > > > > > Affected files (+1773, -0 lines): > > > > A net/crnet/java/src/UrlRequest.java > > > > A net/crnet/java/src/UrlRequestContext.java > > > > A net/crnet/jni/android_log.h > > > > A net/crnet/jni/org_chromium_netjni_UrlRequest.h > > > > A net/crnet/jni/org_chromium_netjni_UrlRequest.cpp > > > > A net/crnet/jni/org_chromium_netjni_UrlRequestContext.h > > > > A net/crnet/jni/org_chromium_netjni_UrlRequestContext.cpp > > > > A net/crnet/jni/url_request_context_peer.h > > > > A net/crnet/jni/url_request_context_peer.cpp > > > > A net/crnet/jni/url_request_peer.h > > > > A net/crnet/jni/url_request_peer.cpp > > > > M net/net.gyp > > > > > > > > > > > > > > > > > > To unsubscribe from this group and stop receiving emails from it, send an > > email > > > to mailto:chromium-reviews+unsubscribe@chromium.org. > > I don't think so as there are multiple targets for Android (so, jar, etc) and > there will be additional targets for iOS. > Speaking about iOS, should java/ and jni/ go under net/crnet/android? > Then net/crnet/ios would have framework/, etc? Seems like a good idea to me.
On Thu, Jan 23, 2014 at 8:30 AM, <mmenke@chromium.org> wrote: > Doesn't look like the code is using URLRequestContextBuilder. Shouldn't > worry > about it for now, since we know this works, but longer term, we should > probably > switch to it, so we have less unique code. > > Quick comments on small things to clean up: > > * #ifdefs are the top of files use the wrong paths. > * We should get rid of android_log, and use standard chrome logging > functions. > If the base logging functions don't work well when using net as a library, > we > should figure out a more general solution. > * annonymous namespaces are generally preferred over static functions / > globals, > particularly when there are a lot of them. > * No clue about Java style, but at least elsewhere, comments should > probably use > C++-style rather than C-style, to be consistent with Chromium style. > And to add to this list: source file extensions should be .cc rather than .cpp > > https://codereview.chromium.org/145213003/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
I noticed that, but a bunch of other similar files use cpp as well, so I wasn't sure if that (And the file names having caps in them) was a JNI thing or not. On Thu, Jan 23, 2014 at 11:42 AM, Szymon Jakubczak <szym@chromium.org>wrote: > > > > On Thu, Jan 23, 2014 at 8:30 AM, <mmenke@chromium.org> wrote: > >> Doesn't look like the code is using URLRequestContextBuilder. Shouldn't >> worry >> about it for now, since we know this works, but longer term, we should >> probably >> switch to it, so we have less unique code. >> >> Quick comments on small things to clean up: >> >> * #ifdefs are the top of files use the wrong paths. >> * We should get rid of android_log, and use standard chrome logging >> functions. >> If the base logging functions don't work well when using net as a >> library, we >> should figure out a more general solution. >> * annonymous namespaces are generally preferred over static functions / >> globals, >> particularly when there are a lot of them. >> * No clue about Java style, but at least elsewhere, comments should >> probably use >> C++-style rather than C-style, to be consistent with Chromium style. >> > > And to add to this list: source file extensions should be .cc rather than > .cpp > > >> >> https://codereview.chromium.org/145213003/ >> > > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
JNI makes no requirement on the file names. The
<fully-qualified-class-name-with-underscores>.{h,cpp} is an Android
convention. I think at the very least the url_request*_peer files should
follow Chromium convention.
On Thu, Jan 23, 2014 at 8:43 AM, Matt Menke <mmenke@chromium.org> wrote:
> I noticed that, but a bunch of other similar files use cpp as well, so I
> wasn't sure if that (And the file names having caps in them) was a JNI
> thing or not.
>
>
> On Thu, Jan 23, 2014 at 11:42 AM, Szymon Jakubczak <szym@chromium.org>wrote:
>
>>
>>
>>
>> On Thu, Jan 23, 2014 at 8:30 AM, <mmenke@chromium.org> wrote:
>>
>>> Doesn't look like the code is using URLRequestContextBuilder. Shouldn't
>>> worry
>>> about it for now, since we know this works, but longer term, we should
>>> probably
>>> switch to it, so we have less unique code.
>>>
>>> Quick comments on small things to clean up:
>>>
>>> * #ifdefs are the top of files use the wrong paths.
>>> * We should get rid of android_log, and use standard chrome logging
>>> functions.
>>> If the base logging functions don't work well when using net as a
>>> library, we
>>> should figure out a more general solution.
>>> * annonymous namespaces are generally preferred over static functions /
>>> globals,
>>> particularly when there are a lot of them.
>>> * No clue about Java style, but at least elsewhere, comments should
>>> probably use
>>> C++-style rather than C-style, to be consistent with Chromium style.
>>>
>>
>> And to add to this list: source file extensions should be .cc rather than
>> .cpp
>>
>>
>>>
>>> https://codereview.chromium.org/145213003/
>>>
>>
>>
>
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.
On 2014/01/23 16:50:30, szym wrote:
> JNI makes no requirement on the file names. The
> <fully-qualified-class-name-with-underscores>.{h,cpp} is an Android
> convention. I think at the very least the url_request*_peer files should
> follow Chromium convention.
>
>
> On Thu, Jan 23, 2014 at 8:43 AM, Matt Menke <mailto:mmenke@chromium.org>
wrote:
>
> > I noticed that, but a bunch of other similar files use cpp as well, so I
> > wasn't sure if that (And the file names having caps in them) was a JNI
> > thing or not.
> >
> >
> > On Thu, Jan 23, 2014 at 11:42 AM, Szymon Jakubczak <szym@chromium.org>wrote:
> >
> >>
> >>
> >>
> >> On Thu, Jan 23, 2014 at 8:30 AM, <mailto:mmenke@chromium.org> wrote:
> >>
> >>> Doesn't look like the code is using URLRequestContextBuilder. Shouldn't
> >>> worry
> >>> about it for now, since we know this works, but longer term, we should
> >>> probably
> >>> switch to it, so we have less unique code.
> >>>
> >>> Quick comments on small things to clean up:
> >>>
> >>> * #ifdefs are the top of files use the wrong paths.
> >>> * We should get rid of android_log, and use standard chrome logging
> >>> functions.
> >>> If the base logging functions don't work well when using net as a
> >>> library, we
> >>> should figure out a more general solution.
> >>> * annonymous namespaces are generally preferred over static functions /
> >>> globals,
> >>> particularly when there are a lot of them.
> >>> * No clue about Java style, but at least elsewhere, comments should
> >>> probably use
> >>> C++-style rather than C-style, to be consistent with Chromium style.
> >>>
> >>
> >> And to add to this list: source file extensions should be .cc rather than
> >> .cpp
> >>
> >>
> >>>
> >>> https://codereview.chromium.org/145213003/
> >>>
> >>
> >>
> >
>
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.
I've moved Android stuff into net/cronet/android, and renamed .cpp/.h files to
match chromium naming convention.
On 2014/01/23 16:01:59, szym wrote: > We will definitely need a shippable build target -- that is without > symbols. Adding 'buildtype': 'Official' to the target variables should > suffice. I was wrong. 'Official' seems to affect win builds only. However, after stripping ($NDK_HOME/toolchains/arm-linux-androideabi-4.7/prebuilt/linux-x86/bin/arm-linux-androideabi-strip out/Release/lib/libcronet.so) the library shrinks from 152MB down to 8.5MB.
Yep. I've added cronet_unittests_apk target and libcronet.so inside of .apk is only 8.5mb (I haven't looked for space optimizations yet). So, we need some explicit stripping as part of build target. Also, do you know whether it is possible to generate something similar to https://x20web.corp.google.com/~andrewhayden/no_crawl/clank-analysis/index.ht... arbitrary .so file? On Fri, Jan 24, 2014 at 10:22 AM, <szym@chromium.org> wrote: > On 2014/01/23 16:01:59, szym wrote: > >> We will definitely need a shippable build target -- that is without >> symbols. Adding 'buildtype': 'Official' to the target variables should >> suffice. >> > > I was wrong. 'Official' seems to affect win builds only. > > However, after stripping > ($NDK_HOME/toolchains/arm-linux-androideabi-4.7/ > prebuilt/linux-x86/bin/arm-linux-androideabi-strip > out/Release/lib/libcronet.so) the library shrinks from 152MB down to 8.5MB. > > https://codereview.chromium.org/145213003/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
tools/binary_size/run_binary_size_analysis.py? The original thread where I got that link is https://groups.google.com/a/google.com/forum/#!topic/chrome-perf/g_ua7iQt5ew, and has a link to a CL where that was added. On Fri, Jan 24, 2014 at 10:28 AM, Misha Efimov <mef@chromium.org> wrote: > Yep. I've added cronet_unittests_apk target and libcronet.so inside of > .apk is only 8.5mb (I haven't looked for space optimizations yet). > > So, we need some explicit stripping as part of build target. > > Also, do you know whether it is possible to generate something similar to > https://x20web.corp.google.com/~andrewhayden/no_crawl/clank-analysis/index.ht... arbitrary .so file? > > > On Fri, Jan 24, 2014 at 10:22 AM, <szym@chromium.org> wrote: > >> On 2014/01/23 16:01:59, szym wrote: >> >>> We will definitely need a shippable build target -- that is without >>> symbols. Adding 'buildtype': 'Official' to the target variables should >>> suffice. >>> >> >> I was wrong. 'Official' seems to affect win builds only. >> >> However, after stripping >> ($NDK_HOME/toolchains/arm-linux-androideabi-4.7/ >> prebuilt/linux-x86/bin/arm-linux-androideabi-strip >> out/Release/lib/libcronet.so) the library shrinks from 152MB down to >> 8.5MB. >> >> https://codereview.chromium.org/145213003/ >> > > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2014/01/24 15:28:17, mef wrote: > Also, do you know whether it is possible to generate something similar to > https://x20web.corp.google.com/%7Eandrewhayden/no_crawl/clank-analysis/index.... > arbitrary .so file? I guess so. That very analysis covers just the native .so file. This technique, developed by Evan Martin (http://neugierig.org/software/chromium/bloat/) relies just on nm.
Nice! https://x20web.corp.google.com/~mef/cronet_size/index.html On Fri, Jan 24, 2014 at 10:37 AM, <szym@chromium.org> wrote: > On 2014/01/24 15:28:17, mef wrote: > >> Also, do you know whether it is possible to generate something similar to >> > > https://x20web.corp.google.com/%7Eandrewhayden/no_crawl/ > clank-analysis/index.htmlfor > >> arbitrary .so file? >> > > I guess so. That very analysis covers just the native .so file. This > technique, > developed by Evan Martin (http://neugierig.org/software/chromium/bloat/) > relies > just on nm. > > > https://codereview.chromium.org/145213003/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2014/01/24 16:20:07, mef wrote: > Nice! https://x20web.corp.google.com/%7Emef/cronet_size/index.html > This is pretty sweet. I'm glad to see that the part of base/ that net/ needs is very small. So why doesn't v8 show up here, but icu does?
That's awesome! Looks like v8's already excluded from the binary, so most of that is icu. FTP also looks to be excluded. Other than registry controlled domains and QUIC (Which we'll want to include eventually), nothing sticks out at me as big obvious wins, though there are a few smaller things (~16k for HTTP pipelining, ~24k for the internal DNS client). On Fri, Jan 24, 2014 at 11:25 AM, <szym@chromium.org> wrote: > On 2014/01/24 16:20:07, mef wrote: > >> Nice! https://x20web.corp.google.com/%7Emef/cronet_size/index.html >> > > > This is pretty sweet. I'm glad to see that the part of base/ that net/ > needs is > very small. > > So why doesn't v8 show up here, but icu does? > > > https://codereview.chromium.org/145213003/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2014/01/24 16:30:57, mmenke wrote: > That's awesome! > > Looks like v8's already excluded from the binary, so most of that is icu. > FTP also looks to be excluded. > > Other than registry controlled domains and QUIC (Which we'll want to > include eventually), nothing sticks out at me as big obvious wins, though > there are a few smaller things (~16k for HTTP pipelining, ~24k for the > internal DNS client). Note: ttuttle just got the DNS probes working on Android, so if they turn out useful, we might have a reason to keep the DNS client code.
Check out the android makefile that lives in google3's version for the flags that got it down from 4.7mb to 3.1. On Jan 24, 2014 8:30 AM, "Matt Menke" <mmenke@chromium.org> wrote: > That's awesome! > > Looks like v8's already excluded from the binary, so most of that is icu. > FTP also looks to be excluded. > > Other than registry controlled domains and QUIC (Which we'll want to > include eventually), nothing sticks out at me as big obvious wins, though > there are a few smaller things (~16k for HTTP pipelining, ~24k for the > internal DNS client). > > > On Fri, Jan 24, 2014 at 11:25 AM, <szym@chromium.org> wrote: > >> On 2014/01/24 16:20:07, mef wrote: >> >>> Nice! https://x20web.corp.google.com/%7Emef/cronet_size/index.html >>> >> >> >> This is pretty sweet. I'm glad to see that the part of base/ that net/ >> needs is >> very small. >> >> So why doesn't v8 show up here, but icu does? >> >> >> https://codereview.chromium.org/145213003/ >> > > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2014/01/24 17:30:32, clm wrote: > Check out the android makefile that lives in google3's version for the > flags that got it down from 4.7mb to 3.1. > On Jan 24, 2014 8:30 AM, "Matt Menke" <mailto:mmenke@chromium.org> wrote: > > > That's awesome! > > > > Looks like v8's already excluded from the binary, so most of that is icu. > > FTP also looks to be excluded. > > > > Other than registry controlled domains and QUIC (Which we'll want to > > include eventually), nothing sticks out at me as big obvious wins, though > > there are a few smaller things (~16k for HTTP pipelining, ~24k for the > > internal DNS client). > > > > > > On Fri, Jan 24, 2014 at 11:25 AM, <mailto:szym@chromium.org> wrote: > > > >> On 2014/01/24 16:20:07, mef wrote: > >> > >>> Nice! https://x20web.corp.google.com/%257Emef/cronet_size/index.html > >>> > >> > >> > >> This is pretty sweet. I'm glad to see that the part of base/ that net/ > >> needs is > >> very small. > >> > >> So why doesn't v8 show up here, but icu does? > >> > >> > >> https://codereview.chromium.org/145213003/ > >> > > > > > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. Are you talking about these: https://cs.corp.google.com/#piper///depot/google3/third_party/android_native_... ? I've tried to remove ICU from net's dependencies (with #ifdef out parts of net_util.cc, ftp_util.cc and http_content_disposition.cc), but that didn't help libcronet.so as ICU is also a dependency of base.gyp:base_i18n and url.gyp. Would we have to go with slimmed ICU (following clm@'s path), or are there alternative solutions?
On 2014/01/24 17:43:03, mef wrote: > On 2014/01/24 17:30:32, clm wrote: > > Check out the android makefile that lives in google3's version for the > > flags that got it down from 4.7mb to 3.1. > > On Jan 24, 2014 8:30 AM, "Matt Menke" <mailto:mmenke@chromium.org> wrote: > > > > > That's awesome! > > > > > > Looks like v8's already excluded from the binary, so most of that is icu. > > > FTP also looks to be excluded. > > > > > > Other than registry controlled domains and QUIC (Which we'll want to > > > include eventually), nothing sticks out at me as big obvious wins, though > > > there are a few smaller things (~16k for HTTP pipelining, ~24k for the > > > internal DNS client). > > > > > > > > > On Fri, Jan 24, 2014 at 11:25 AM, <mailto:szym@chromium.org> wrote: > > > > > >> On 2014/01/24 16:20:07, mef wrote: > > >> > > >>> Nice! https://x20web.corp.google.com/%25257Emef/cronet_size/index.html > > >>> > > >> > > >> > > >> This is pretty sweet. I'm glad to see that the part of base/ that net/ > > >> needs is > > >> very small. > > >> > > >> So why doesn't v8 show up here, but icu does? > > >> > > >> > > >> https://codereview.chromium.org/145213003/ > > >> > > > > > > > > > > To unsubscribe from this group and stop receiving emails from it, send an > email > > to mailto:chromium-reviews+unsubscribe@chromium.org. > > Are you talking about these: > https://cs.corp.google.com/#piper///depot/google3/third_party/android_native_... > ? > > I've tried to remove ICU from net's dependencies (with #ifdef out parts of > net_util.cc, ftp_util.cc and http_content_disposition.cc), but that didn't help > libcronet.so as ICU is also a dependency of base.gyp:base_i18n and url.gyp. > > Would we have to go with slimmed ICU (following clm@'s path), or are there > alternative solutions? For now it seems like we should get an initial version landed which works, and then focus on removing ICU dependency (or other dependencies) rather than trying to solve all of the problems in a single CL.
I agree. The only thing worth doing in this cl is stripping the symbols so that the .so is not 153mb On Jan 26, 2014 3:44 PM, <cbentzel@chromium.org> wrote: > On 2014/01/24 17:43:03, mef wrote: > >> On 2014/01/24 17:30:32, clm wrote: >> > Check out the android makefile that lives in google3's version for the >> > flags that got it down from 4.7mb to 3.1. >> > On Jan 24, 2014 8:30 AM, "Matt Menke" <mailto:mmenke@chromium.org> >> wrote: >> > >> > > That's awesome! >> > > >> > > Looks like v8's already excluded from the binary, so most of that is >> icu. >> > > FTP also looks to be excluded. >> > > >> > > Other than registry controlled domains and QUIC (Which we'll want to >> > > include eventually), nothing sticks out at me as big obvious wins, >> though >> > > there are a few smaller things (~16k for HTTP pipelining, ~24k for the >> > > internal DNS client). >> > > >> > > >> > > On Fri, Jan 24, 2014 at 11:25 AM, <mailto:szym@chromium.org> wrote: >> > > >> > >> On 2014/01/24 16:20:07, mef wrote: >> > >> >> > >>> Nice! https://x20web.corp.google.com/%25257Emef/cronet_size/ >> index.html >> > >>> >> > >> >> > >> >> > >> This is pretty sweet. I'm glad to see that the part of base/ that >> net/ >> > >> needs is >> > >> very small. >> > >> >> > >> So why doesn't v8 show up here, but icu does? >> > >> >> > >> >> > >> https://codereview.chromium.org/145213003/ >> > >> >> > > >> > > >> > >> > To unsubscribe from this group and stop receiving emails from it, send >> an >> email >> > to mailto:chromium-reviews+unsubscribe@chromium.org. >> > > Are you talking about these: >> > > https://cs.corp.google.com/#piper///depot/google3/third_ > party/android_native_libs/chromium_net/Android.mk&l=101 > >> ? >> > > I've tried to remove ICU from net's dependencies (with #ifdef out parts of >> net_util.cc, ftp_util.cc and http_content_disposition.cc), but that didn't >> > help > >> libcronet.so as ICU is also a dependency of base.gyp:base_i18n and >> url.gyp. >> > > Would we have to go with slimmed ICU (following clm@'s path), or are >> there >> alternative solutions? >> > > For now it seems like we should get an initial version landed which works, > and > then focus on removing ICU dependency (or other dependencies) rather than > trying > to solve all of the problems in a single CL. > > https://codereview.chromium.org/145213003/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Specifically, these flags: LOCAL_CFLAGS := \ -DLOGGING=1 \ -ffunction-sections \ -fdata-sections \ -DCHROMIUM_VERSION="\"$(CHROMIUM_VERSION)\"" \ $(CHROMIUM_DEFINES)LOCAL_CPPFLAGS := \ -fno-rtti \ -Wno-sign-promo \ -Wno-missing-field-initializers \ -ffunction-sections \ -fdata-sections \ -fvisibility=hidden \ -fvisibility-inlines-hiddenLOCAL_C_INCLUDES := \ $(LOCAL_PATH)/org/chromium \ $(CHROMIUM_NET_HEADERS_PATH) \ $(CHROMIUM_NET_HEADERS_PATH)/android LOCAL_LDFLAGS := -llog -landroid -Wl,--gc-sections -Wl,--exclude-libs,ALL On Sun, Jan 26, 2014 at 7:29 AM, Szymon Jakubczak <szym@chromium.org> wrote: > I agree. The only thing worth doing in this cl is stripping the symbols so > that the .so is not 153mb > On Jan 26, 2014 3:44 PM, <cbentzel@chromium.org> wrote: > >> On 2014/01/24 17:43:03, mef wrote: >> >>> On 2014/01/24 17:30:32, clm wrote: >>> > Check out the android makefile that lives in google3's version for the >>> > flags that got it down from 4.7mb to 3.1. >>> > On Jan 24, 2014 8:30 AM, "Matt Menke" <mailto:mmenke@chromium.org> >>> wrote: >>> > >>> > > That's awesome! >>> > > >>> > > Looks like v8's already excluded from the binary, so most of that is >>> icu. >>> > > FTP also looks to be excluded. >>> > > >>> > > Other than registry controlled domains and QUIC (Which we'll want to >>> > > include eventually), nothing sticks out at me as big obvious wins, >>> though >>> > > there are a few smaller things (~16k for HTTP pipelining, ~24k for >>> the >>> > > internal DNS client). >>> > > >>> > > >>> > > On Fri, Jan 24, 2014 at 11:25 AM, <mailto:szym@chromium.org> wrote: >>> > > >>> > >> On 2014/01/24 16:20:07, mef wrote: >>> > >> >>> > >>> Nice! https://x20web.corp.google.com/%25257Emef/cronet_size/ >>> index.html >>> > >>> >>> > >> >>> > >> >>> > >> This is pretty sweet. I'm glad to see that the part of base/ that >>> net/ >>> > >> needs is >>> > >> very small. >>> > >> >>> > >> So why doesn't v8 show up here, but icu does? >>> > >> >>> > >> >>> > >> https://codereview.chromium.org/145213003/ >>> > >> >>> > > >>> > > >>> > >>> > To unsubscribe from this group and stop receiving emails from it, send >>> an >>> email >>> > to mailto:chromium-reviews+unsubscribe@chromium.org. >>> >> >> Are you talking about these: >>> >> >> https://cs.corp.google.com/#piper///depot/google3/third_ >> party/android_native_libs/chromium_net/Android.mk&l=101 >> >>> ? >>> >> >> I've tried to remove ICU from net's dependencies (with #ifdef out parts >>> of >>> net_util.cc, ftp_util.cc and http_content_disposition.cc), but that >>> didn't >>> >> help >> >>> libcronet.so as ICU is also a dependency of base.gyp:base_i18n and >>> url.gyp. >>> >> >> Would we have to go with slimmed ICU (following clm@'s path), or are >>> there >>> alternative solutions? >>> >> >> For now it seems like we should get an initial version landed which >> works, and >> then focus on removing ICU dependency (or other dependencies) rather than >> trying >> to solve all of the problems in a single CL. >> >> https://codereview.chromium.org/145213003/ >> > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2014/01/26 14:44:06, cbentzel wrote: > On 2014/01/24 17:43:03, mef wrote: > > On 2014/01/24 17:30:32, clm wrote: > > > Check out the android makefile that lives in google3's version for the > > > flags that got it down from 4.7mb to 3.1. > > > On Jan 24, 2014 8:30 AM, "Matt Menke" <mailto:mmenke@chromium.org> wrote: > > > > > > > That's awesome! > > > > > > > > Looks like v8's already excluded from the binary, so most of that is icu. > > > > FTP also looks to be excluded. > > > > > > > > Other than registry controlled domains and QUIC (Which we'll want to > > > > include eventually), nothing sticks out at me as big obvious wins, though > > > > there are a few smaller things (~16k for HTTP pipelining, ~24k for the > > > > internal DNS client). > > > > > > > > > > > > On Fri, Jan 24, 2014 at 11:25 AM, <mailto:szym@chromium.org> wrote: > > > > > > > >> On 2014/01/24 16:20:07, mef wrote: > > > >> > > > >>> Nice! https://x20web.corp.google.com/%2525257Emef/cronet_size/index.html > > > >>> > > > >> > > > >> > > > >> This is pretty sweet. I'm glad to see that the part of base/ that net/ > > > >> needs is > > > >> very small. > > > >> > > > >> So why doesn't v8 show up here, but icu does? > > > >> > > > >> > > > >> https://codereview.chromium.org/145213003/ > > > >> > > > > > > > > > > > > > > To unsubscribe from this group and stop receiving emails from it, send an > > email > > > to mailto:chromium-reviews+unsubscribe@chromium.org. > > > > Are you talking about these: > > > https://cs.corp.google.com/#piper///depot/google3/third_party/android_native_... > > ? > > > > I've tried to remove ICU from net's dependencies (with #ifdef out parts of > > net_util.cc, ftp_util.cc and http_content_disposition.cc), but that didn't > help > > libcronet.so as ICU is also a dependency of base.gyp:base_i18n and url.gyp. > > > > Would we have to go with slimmed ICU (following clm@'s path), or are there > > alternative solutions? > > For now it seems like we should get an initial version landed which works, and > then focus on removing ICU dependency (or other dependencies) rather than trying > to solve all of the problems in a single CL. I agree. I'd rather have few smaller CLs than one massive CL. I'm just wondering whether we should wait for approval of open source request prior to landing this CL. Also, I'm yet to see the unit test to actually work.
On 2014/01/26 21:08:17, mef wrote: > On 2014/01/26 14:44:06, cbentzel wrote: > > On 2014/01/24 17:43:03, mef wrote: > > > On 2014/01/24 17:30:32, clm wrote: > > > > Check out the android makefile that lives in google3's version for the > > > > flags that got it down from 4.7mb to 3.1. > > > > On Jan 24, 2014 8:30 AM, "Matt Menke" <mailto:mmenke@chromium.org> wrote: > > > > > > > > > That's awesome! > > > > > > > > > > Looks like v8's already excluded from the binary, so most of that is > icu. > > > > > FTP also looks to be excluded. > > > > > > > > > > Other than registry controlled domains and QUIC (Which we'll want to > > > > > include eventually), nothing sticks out at me as big obvious wins, > though > > > > > there are a few smaller things (~16k for HTTP pipelining, ~24k for the > > > > > internal DNS client). > > > > > > > > > > > > > > > On Fri, Jan 24, 2014 at 11:25 AM, <mailto:szym@chromium.org> wrote: > > > > > > > > > >> On 2014/01/24 16:20:07, mef wrote: > > > > >> > > > > >>> Nice! > https://x20web.corp.google.com/%252525257Emef/cronet_size/index.html > > > > >>> > > > > >> > > > > >> > > > > >> This is pretty sweet. I'm glad to see that the part of base/ that net/ > > > > >> needs is > > > > >> very small. > > > > >> > > > > >> So why doesn't v8 show up here, but icu does? > > > > >> > > > > >> > > > > >> https://codereview.chromium.org/145213003/ > > > > >> > > > > > > > > > > > > > > > > > > To unsubscribe from this group and stop receiving emails from it, send an > > > email > > > > to mailto:chromium-reviews+unsubscribe@chromium.org. > > > > > > Are you talking about these: > > > > > > https://cs.corp.google.com/#piper///depot/google3/third_party/android_native_... > > > ? > > > > > > I've tried to remove ICU from net's dependencies (with #ifdef out parts of > > > net_util.cc, ftp_util.cc and http_content_disposition.cc), but that didn't > > help > > > libcronet.so as ICU is also a dependency of base.gyp:base_i18n and url.gyp. > > > > > > Would we have to go with slimmed ICU (following clm@'s path), or are there > > > alternative solutions? > > > > For now it seems like we should get an initial version landed which works, and > > then focus on removing ICU dependency (or other dependencies) rather than > trying > > to solve all of the problems in a single CL. > > I agree. I'd rather have few smaller CLs than one massive CL. > I'm just wondering whether we should wait for approval of open source request > prior to landing this CL. > Also, I'm yet to see the unit test to actually work. Yes, we absolutely should wait for approval of the open source request before landing.
FYI, I'm still working on this CL as open sourcing request is pending. At this point I'm able to build the library (using clm's cflags and ldflags) and a sample app that uses it. My next steps is to add a test to drive sample app and start looking into reducing binary size . https://codereview.chromium.org/145213003/diff/790001/net/cronet/android/org_... File net/cronet/android/org_chromium_net_UrlRequestContext.cc (right): https://codereview.chromium.org/145213003/diff/790001/net/cronet/android/org_... net/cronet/android/org_chromium_net_UrlRequestContext.cc:146: base::android::ScopedJavaLocalRef<jobject> scoped_context(env, context); Setting application context here doesn't seem quite right. Would it make sense to add another method to explicitly perform base::android::InitApplicationContext? https://codereview.chromium.org/145213003/diff/790001/net/net.gyp File net/net.gyp (right): https://codereview.chromium.org/145213003/diff/790001/net/net.gyp#newcode3218 net/net.gyp:3218: '-DCHROMIUM_VERSION=\\"TBD\\"', What's a good way to define CHROMIUM_VERSION? Normally it is generated in chrome.gyp.
Hi, I've got a response from dannyb@ confirming that there are no issues with open-sourcing of G+ netjni and social.net bindings. I've updated this CL with latest code from /depot/google3/third_party/android_native_libs/chromium_net. I'm wondering whether I should land it now to make a base line for further work. WDYT? thanks, -m
Just FYI, there are now (a few) tests for it in the javatests/ social net folder. On Mon, Feb 10, 2014 at 8:29 AM, <mef@chromium.org> wrote: > Hi, > > I've got a response from dannyb@ confirming that there are no issues with > open-sourcing of G+ netjni and social.net bindings. > > I've updated this CL with latest code from > /depot/google3/third_party/android_native_libs/chromium_net. > > I'm wondering whether I should land it now to make a base line for further > work. > > WDYT? > > thanks, > -m > > https://codereview.chromium.org/145213003/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Hi guys, please take a look, I'll appreciate your comments. This CL builds on buildbots, but 'findbugs' step finds some issues: H D IA: Potentially ambiguous invocation of either an outer or inherited method org.chromium.net.UrlRequest.getHttpStatusCode() in org.chromium.cronet_sample_apk.CronetSampleActivity$SampleRequest.onRequestComplete() At CronetSampleActivity.java H D IA: Potentially ambiguous invocation of either an outer or inherited method org.chromium.net.UrlRequest.getUrl() in org.chromium.cronet_sample_apk.CronetSampleActivity$SampleRequest.onRequestComplete() At CronetSampleActivity.java M C CSM: Shouldn't use synchronized method, please narrow down the synchronization scope. At UrlRequest.java M C CST: Shouldn't use synchronized(this), please narrow down the synchronization scope. At UrlRequest.java M M IS: Inconsistent synchronization of org.chromium.net.UrlRequest.mAdditionalHeaders; locked 42% of time Unsynchronized access at UrlRequest.java M M IS: Inconsistent synchronization of org.chromium.net.UrlRequest.mFinished; locked 50% of time Unsynchronized access at UrlRequest.java M M IS: Inconsistent synchronization of org.chromium.net.UrlRequest.mPostBody; locked 60% of time Unsynchronized access at UrlRequest.java M M IS: Inconsistent synchronization of org.chromium.net.UrlRequest.mPostBodyContentType; locked 50% of time Unsynchronized access at UrlRequest.java M V EI2: org.chromium.net.UrlRequest.setUploadData(String, byte[]) may expose internal representation by storing an externally mutable object into UrlRequest.mPostBody At UrlRequest.java
Quick nits...I sometimes just pointed out one or two cases of something in individual files. I have not reviewed the correctness of the code. Thigns to think about: * Should we move files into the net namespace? * Should we move org_chromium_net files into a subdirectory? * Copyright year: We don't normally update it when modifying files. I think we shouldn't be updating it here, either? https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/jav... File net/cronet/android/java/src/org/chromium/net/UrlRequest.java (right): https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/jav... net/cronet/android/java/src/org/chromium/net/UrlRequest.java:1: // Copyright 2014 The Chromium Authors. All rights reserved. Should these be the dates from the original files, or updated dates? I'd assume the former is more consistent with chromium policy. https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/jav... net/cronet/android/java/src/org/chromium/net/UrlRequest.java:28: /* nit: All comments in this file other than this one and the next one use "/**". https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/jav... net/cronet/android/java/src/org/chromium/net/UrlRequest.java:66: private Semaphore mAppendChunkSemaphore; All of these variable names violate the Google Java style guide: "In Google Style special prefixes or suffixes, like those seen in the examples name_, mName, s_name and kName, are not used." (Why this is different from the C++ style guide, I can't say). Suggest keeping them as-is for now, but adding a todo. Mostly refactoring things like "mUrl = url" are easy to mess up. https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/jav... net/cronet/android/java/src/org/chromium/net/UrlRequest.java:82: * @param sink The output channel into which downloaded content will be written. nit: While the Java style guide allows 100 character lines, it also allows 80. I think we should wrap at 80, simply because of how rietveld is configured. https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/jav... net/cronet/android/java/src/org/chromium/net/UrlRequest.java:85: Map<String, String> headers, WritableByteChannel sink) { Suggest indenting lines like this to align with parens, though Java style does not require it. https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/jav... File net/cronet/android/javatests/src/org/chromium/net/UrlRequestTest.java (right): https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/jav... net/cronet/android/javatests/src/org/chromium/net/UrlRequestTest.java:27: UrlRequestContext.LOG_VERBOSE); This should use two space indent. https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/jav... net/cronet/android/javatests/src/org/chromium/net/UrlRequestTest.java:49: assertTrue(request != null); Also two space ident. https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/org... File net/cronet/android/org_chromium_net_UrlRequest.cc (right): https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/org... net/cronet/android/org_chromium_net_UrlRequest.cc:15: #define LOG_TAG "ChromiumNetwork" this isn't used in this file https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/org... net/cronet/android/org_chromium_net_UrlRequest.cc:24: static jfieldID g_request_field; Suggest just putting all these in an anonymous namespace. https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/org... net/cronet/android/org_chromium_net_UrlRequest.cc:24: static jfieldID g_request_field; Most of these names violate the Google C++ style guide. Suggest just renaming them something like g_on_blah_[id | method | method_id]. Be great if we could make them no longer globals, but I don't think we should worry about that for now. https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/org... net/cronet/android/org_chromium_net_UrlRequest.cc:50: || !g_method_write) { While this is the right style for Java, and allowed in C++, the "||" are pretty much always put on the previous line in Chrome. https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/org... net/cronet/android/org_chromium_net_UrlRequest.cc:56: static net::RequestPriority ConvertRequestPriority(jint request_priority) { static is not used very often before functions in chrome, suggest an anonymous namespace instead (And putting it above UrlRequestRegisterJni). https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/org... net/cronet/android/org_chromium_net_UrlRequest.cc:63: default: return net::LOWEST; Put all the returns on the next lines. https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/org... net/cronet/android/org_chromium_net_UrlRequest.cc:69: */ Use C++-style comments. (Goes for entire file) https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/org... net/cronet/android/org_chromium_net_UrlRequest.cc:73: JavaVM* vm_; These should go in a private section at the bottom of the class. https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/org... net/cronet/android/org_chromium_net_UrlRequest.cc:129: * Stores a reference to the request in a java field. Should Java be capitalized in comments? https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/org... net/cronet/android/org_chromium_net_UrlRequest.cc:131: static void SetNativeObject(JNIEnv *env, jobject object, Static methods should be moved to the anonymous namespace up top (And can get rid of the static) https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/org... net/cronet/android/org_chromium_net_UrlRequest.cc:147: Java_org_chromium_net_UrlRequest_nativeInit( Maybe be able to move all these up a line. https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/org... net/cronet/android/org_chromium_net_UrlRequest.cc:152: CHECK(context != NULL); DCHECK? https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/org... net/cronet/android/org_chromium_net_UrlRequest.cc:154: const char *url_utf8 = env->GetStringUTFChars(url_string, NULL); Should be char* url_utf8. https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/org... net/cronet/android/org_chromium_net_UrlRequest.cc:175: CHECK(request != NULL); DCHECK? https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/org... net/cronet/android/org_chromium_net_UrlRequest.cc:189: jstring content_type) { Statics up top, -static https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/org... net/cronet/android/org_chromium_net_UrlRequest.cc:190: CHECK(request != NULL); DCHECK? https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/org... net/cronet/android/org_chromium_net_UrlRequest.cc:197: const char *content_type_utf8 = env->GetStringUTFChars(content_type, NULL); char* content_... https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/org... net/cronet/android/org_chromium_net_UrlRequest.cc:282: case 0: case net::OK: https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/org... net/cronet/android/org_chromium_net_UrlRequest.cc:282: case 0: Please add a TODO about investigating returning success on positive values, too, as they technically indicate success. https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/org... net/cronet/android/org_chromium_net_UrlRequest.cc:289: // treat downloads as complete in both cases, so we follow their lead. Add a TODO about investigating this. The fact is that Chrome does not do this, and this library is not just being used for downloads. https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/org... File net/cronet/android/org_chromium_net_UrlRequest.h (right): https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/org... net/cronet/android/org_chromium_net_UrlRequest.h:10: #ifdef __cplusplus Hmm...Wonder if this is necessary. We're using C++ style comments in this file, so is it ever compiled as string C? https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/org... net/cronet/android/org_chromium_net_UrlRequest.h:15: * Native method implementations of the org.chromium.netjni.UrlRequest class. Should use C++ style comments in C++, since that's what chrome normally uses. https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/org... net/cronet/android/org_chromium_net_UrlRequest.h:35: Java_org_chromium_net_UrlRequest_nativeInit( Think for most of these, you can move the function name up a line. I'm assuming the funky names are necessary. https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/org... File net/cronet/android/org_chromium_net_UrlRequestContext.cc (right): https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/org... net/cronet/android/org_chromium_net_UrlRequestContext.cc:26: static jfieldID g_field_mRequestContext; fix names / anonymous namespace https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/org... net/cronet/android/org_chromium_net_UrlRequestContext.cc:28: static base::AtExitManager* s_at_exit_manager = NULL; Suggest using a g_ prefix for consistency with above. We should see if we can get rid of these globals, but not in this CL. https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/org... net/cronet/android/org_chromium_net_UrlRequestContext.cc:32: */ C++ style comments https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/org... net/cronet/android/org_chromium_net_UrlRequestContext.cc:39: const char* class_name = "org/chromium/net/UrlRequestContext"; Seems like this should be "const char kClassName[]". May also want to move it to the anonymous namespace up top, given its importance. https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/org... net/cronet/android/org_chromium_net_UrlRequestContext.cc:75: == JNI_EDETACHED) { nit: "==" on previous line. https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/org... net/cronet/android/org_chromium_net_UrlRequestContext.cc:83: */ C++-style comment? https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/org... net/cronet/android/org_chromium_net_UrlRequestContext.cc:84: class JniURLRequestContextPeerDelegate : Move into an anonymous namespace? https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/org... net/cronet/android/org_chromium_net_UrlRequestContext.cc:87: JavaVM* vm_; These should go at bottom of class, in aa private section. https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/org... net/cronet/android/org_chromium_net_UrlRequestContext.cc:91: owner_ = env->NewGlobalRef(owner); This can be set in an initializer list. https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/org... net/cronet/android/org_chromium_net_UrlRequestContext.cc:127: static URLRequestContextPeer* GetNativeObject(JNIEnv* env, jobject object) { Move these into an anonymous namespace? https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/org... net/cronet/android/org_chromium_net_UrlRequestContext.cc:157: // Set application context Comments should end in periods, here and below. https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/org... File net/cronet/android/org_chromium_net_UrlRequestContext.h (right): https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/org... net/cronet/android/org_chromium_net_UrlRequestContext.h:22: */ C++ style? https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/sam... File net/cronet/android/sample/javatests/src/org/chromium/cronet_sample_apk/CronetSampleUrlTest.java (right): https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/sam... net/cronet/android/sample/javatests/src/org/chromium/cronet_sample_apk/CronetSampleUrlTest.java:30: assertEquals(200, activity.getHttpStatusCode()); 2-space indent. https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/sam... net/cronet/android/sample/javatests/src/org/chromium/cronet_sample_apk/CronetSampleUrlTest.java:31: } 2-space indent. https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/sam... net/cronet/android/sample/javatests/src/org/chromium/cronet_sample_apk/CronetSampleUrlTest.java:36: CronetSampleActivity activity = launchCronetSampleWithUrl("127.0.0.1:8000"); Wrap line. https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/sam... net/cronet/android/sample/javatests/src/org/chromium/cronet_sample_apk/CronetSampleUrlTest.java:45: } 2-space indent (x2) https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/sam... File net/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java (right): https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/sam... net/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java:19: //import java.io.FileOutputStream; Remove unused line https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/sam... net/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java:29: private static final String TAG = "CronetSampleActivity"; 2 space indent (Goes for most of this file) https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/sam... net/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java:35: int mHttpStatusCode = 0; Again, this naming seems to violate the style guide, but think we should leave it for now. https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/sam... net/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java:39: super(getApplicationContext(), "Cronet Sample", UrlRequestContext.LOG_VERBOSE); Wrap at 80 chars https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/sam... net/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java:122: sink); fix indent https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/sam... File net/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleApplication.java (right): https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/sam... net/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleApplication.java:28: } fix indent https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/sam... File net/cronet/android/sample/src/org/chromium/cronet_sample_apk/LibraryLoader.java (right): https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/sam... net/cronet/android/sample/src/org/chromium/cronet_sample_apk/LibraryLoader.java:22: } fix indent https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/url... File net/cronet/android/url_request_context_peer.cc (right): https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/url... net/cronet/android/url_request_context_peer.cc:16: #include "net/url_request/url_request_job_factory_impl.h" net/base/net_errors.h. https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/url... net/cronet/android/url_request_context_peer.cc:18: class BasicNetworkDelegate : public net::NetworkDelegate { Move into anonymous namespace? https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/url... net/cronet/android/url_request_context_peer.cc:24: virtual int OnBeforeURLRequest(net::URLRequest* request, Comment that all these are the net::NetworkDelegate implementation. https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/url... net/cronet/android/url_request_context_peer.cc:68: } Hmm...All of the functions above this are the same as NetworkDelegate's, so we may not want them. Suggest leaving them for now, but may want to just do the ones we care about (The tree OnCan ones). https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/url... net/cronet/android/url_request_context_peer.cc:100: class BasicURLRequestContext : public net::URLRequestContext { Move into anonymous namespace? https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/url... net/cronet/android/url_request_context_peer.cc:110: net::URLRequestContextStorage storage_; nit: Add blank line. https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/url... net/cronet/android/url_request_context_peer.cc:213: net::URLRequestContext *URLRequestContextPeer::GetURLRequestContext() { nit: net::URLRequestContext* URLRequestContextPeer https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/url... File net/cronet/android/url_request_context_peer.h (right): https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/url... net/cronet/android/url_request_context_peer.h:10: #include "net/url_request/url_request_context_getter.h" base/compiler_specific.h and base/macros.h for DISALLOW_COPY_AND_ASSIGN and OVERRIDE https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/url... net/cronet/android/url_request_context_peer.h:10: #include "net/url_request/url_request_context_getter.h" Add scoped_ptr, ref_counted, NetLog, NetworkChangeNotifier headers https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/url... net/cronet/android/url_request_context_peer.h:14: */ C++ style comment? https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/url... net/cronet/android/url_request_context_peer.h:25: private: Blank line before private. https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/url... net/cronet/android/url_request_context_peer.h:31: */ C++ style comment? https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/url... net/cronet/android/url_request_context_peer.h:50: const std::string &GetUserAgent(const GURL &url) const; std::string& ... https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/url... net/cronet/android/url_request_context_peer.h:54: const char* version() const; When using this naming style, functions must be inlined. https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/url... net/cronet/android/url_request_context_peer.h:56: public: not needed. https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/url... net/cronet/android/url_request_context_peer.h:59: GetNetworkTaskRunner() const OVERRIDE; These two are the "// net::URLRequestContextGetter implementation:" https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/url... net/cronet/android/url_request_context_peer.h:76: DISALLOW_EVIL_CONSTRUCTORS(URLRequestContextPeer); DISALLOW_COPY_AND_ASSIGN? https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/url... File net/cronet/android/url_request_peer.cc (right): https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/url... net/cronet/android/url_request_peer.cc:97: /* static */ C++-style comments https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/url... net/cronet/android/url_request_peer.cc:113: /* static */ C++-style comments https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/url... net/cronet/android/url_request_peer.cc:175: void URLRequestPeer::OnCancelRequestWrapper(URLRequestPeer *self) { URLRequestPeer* self https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/url... net/cronet/android/url_request_peer.cc:196: void URLRequestPeer::OnDestroyRequest(URLRequestPeer *self) { URLRequestPeer* self https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/url... net/cronet/android/url_request_peer.cc:202: void URLRequestPeer::OnResponseStarted(net::URLRequest *request) { net::URLRequest* request https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/url... net/cronet/android/url_request_peer.cc:221: */ C++ style comments https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/url... net/cronet/android/url_request_peer.cc:223: while (1) { while (true) https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/url... net/cronet/android/url_request_peer.cc:318: size_t URLRequestPeer::BytesRead() const { return bytes_read_; } Inline and rename to bytes_read? https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/url... net/cronet/android/url_request_peer.cc:321: return reinterpret_cast<unsigned char *>(read_buffer_->StartOfBuffer()); <unsigned char*> https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/url... File net/cronet/android/url_request_peer.h (right): https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/url... net/cronet/android/url_request_peer.h:11: #include "net/url_request/url_request.h" ref_ptr, request_priority, OVERRIDE, DISALLOW_COPY_AND_ASSIGN headers. https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/url... net/cronet/android/url_request_peer.h:19: */ C++-style comments? https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/url... net/cronet/android/url_request_peer.h:92: typedef Delegate inherited; ? https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/url... net/cronet/android/url_request_peer.h:129: DISALLOW_EVIL_CONSTRUCTORS(URLRequestPeer); DISALLOW_COPY_AND_ASSIGN?
Just for the record, these are almost all just reformatting, unused lines, missing headers, and moving static functions / variables / classes around (And into anonymous namespaces). If you feel like you'd rather take care of some (Or even all) of the issues after landing, I'm fine with that, but I have tried to limit my suggestions to think that are fairly "safe" changes.
On 2014/02/27 23:16:17, mmenke wrote: > Just for the record, these are almost all just reformatting, unused lines, > missing headers, and moving static functions / variables / classes around (And > into anonymous namespaces). > > If you feel like you'd rather take care of some (Or even all) of the issues > after landing, I'm fine with that, but I have tried to limit my suggestions to > think that are fairly "safe" changes. Oh, and a couple files have "A+" by their names instead of "A". It doesn't look like git picked up on a similar files already in the repo...
https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/org... File net/cronet/android/org_chromium_net_UrlRequest.cc (right): https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/org... net/cronet/android/org_chromium_net_UrlRequest.cc:5: #include "net/cronet/android/org_chromium_net_UrlRequest.h" After reading some more on the https://code.google.com/p/chromium/codesearch#chromium/src/base/android/jni_g... , I think you should use it to generate the bindings between UrlRequest.java and the native implementation. The native UrlRequest.h would be: class UrlRequest { public: void Init(JNIEnv* env, jobject, jobject request_context, jstring url, jint priority); void AddHeader(JNIEnv* env, jobject, jstring name, jstring value); ... jint GetErrorCode(JNIEnv* env, jobject obj); ... private: scoped_ptr<net::URLRequest> request_; }; And in UrlRequest.java you would have native void nativeInit(long nativeUrlRequest, Object request_context, String url, int priority); native void nativeAddHeader(long nativeUrlRequest, String name, String value); ... native int nativeGetErrorCode(long nativeUrlRequest); This should reduce a lot of error-prone boiler plate code. That said, it's non-trivial amount of work, so probably ok to punt until subsequent CL. I'd leave a TODO+crbug though. https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/org... File net/cronet/android/org_chromium_net_UrlRequest.h (right): https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/org... net/cronet/android/org_chromium_net_UrlRequest.h:19: #define REQUEST_PRIORITY_IDLE 0 Use enum or const.
PTAL. I've addressed most of the nits. I'd prefer to look into using of generated jni headers as separate CL. I'll move into net namespace at the same time as jni will no longer be relying on name mangling. I get a few java bug warnings about inconsistent synchronization in UrlRequest.java. Dmitri, could you comment on those? thanks, -m https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/jav... File net/cronet/android/java/src/org/chromium/net/UrlRequest.java (right): https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/jav... net/cronet/android/java/src/org/chromium/net/UrlRequest.java:1: // Copyright 2014 The Chromium Authors. All rights reserved. On 2014/02/27 23:06:02, mmenke wrote: > Should these be the dates from the original files, or updated dates? I'd assume > the former is more consistent with chromium policy. Good question. My reasoning was that these files are added to chromium repository this year, but I'm open to changing it back to 2013. https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/jav... net/cronet/android/java/src/org/chromium/net/UrlRequest.java:28: /* On 2014/02/27 23:06:02, mmenke wrote: > nit: All comments in this file other than this one and the next one use "/**". Done. Does /** mean something special for javadoc? If so, should we put more meaningful comments? Also, it would be nice if jni_generator have copied them into _jni.h, but it doesn't seem to do that. https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/jav... net/cronet/android/java/src/org/chromium/net/UrlRequest.java:66: private Semaphore mAppendChunkSemaphore; On 2014/02/27 23:06:02, mmenke wrote: > All of these variable names violate the Google Java style guide: "In Google > Style special prefixes or suffixes, like those seen in the examples name_, > mName, s_name and kName, are not used." (Why this is different from the C++ > style guide, I can't say). Suggest keeping them as-is for now, but adding a > todo. Mostly refactoring things like "mUrl = url" are easy to mess up. Added TODO() https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/jav... net/cronet/android/java/src/org/chromium/net/UrlRequest.java:82: * @param sink The output channel into which downloaded content will be written. On 2014/02/27 23:06:02, mmenke wrote: > nit: While the Java style guide allows 100 character lines, it also allows 80. > I think we should wrap at 80, simply because of how rietveld is configured. Done. https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/jav... net/cronet/android/java/src/org/chromium/net/UrlRequest.java:85: Map<String, String> headers, WritableByteChannel sink) { On 2014/02/27 23:06:02, mmenke wrote: > Suggest indenting lines like this to align with parens, though Java style does > not require it. Done. https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/jav... File net/cronet/android/javatests/src/org/chromium/net/UrlRequestTest.java (right): https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/jav... net/cronet/android/javatests/src/org/chromium/net/UrlRequestTest.java:27: UrlRequestContext.LOG_VERBOSE); On 2014/02/27 23:06:02, mmenke wrote: > This should use two space indent. Done. Deleted the module. :-) https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/jav... net/cronet/android/javatests/src/org/chromium/net/UrlRequestTest.java:49: assertTrue(request != null); On 2014/02/27 23:06:02, mmenke wrote: > Also two space ident. Done. https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/org... File net/cronet/android/org_chromium_net_UrlRequest.cc (right): https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/org... net/cronet/android/org_chromium_net_UrlRequest.cc:5: #include "net/cronet/android/org_chromium_net_UrlRequest.h" On 2014/02/28 00:50:40, szym wrote: > After reading some more on the > https://code.google.com/p/chromium/codesearch#chromium/src/base/android/jni_g... > , I think you should use it to generate the bindings between UrlRequest.java and > the native implementation. > > The native UrlRequest.h would be: > > class UrlRequest { > public: > void Init(JNIEnv* env, jobject, jobject request_context, jstring url, jint > priority); > void AddHeader(JNIEnv* env, jobject, jstring name, jstring value); > ... > jint GetErrorCode(JNIEnv* env, jobject obj); > ... > private: > scoped_ptr<net::URLRequest> request_; > }; > > And in UrlRequest.java you would have > > native void nativeInit(long nativeUrlRequest, Object request_context, String > url, int priority); > native void nativeAddHeader(long nativeUrlRequest, String name, String value); > ... > native int nativeGetErrorCode(long nativeUrlRequest); > > > This should reduce a lot of error-prone boiler plate code. That said, it's > non-trivial amount of work, so probably ok to punt until subsequent CL. I'd > leave a TODO+crbug though. Sounds good. Added TODO for now. https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/org... net/cronet/android/org_chromium_net_UrlRequest.cc:15: #define LOG_TAG "ChromiumNetwork" On 2014/02/27 23:06:02, mmenke wrote: > this isn't used in this file Done. https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/org... net/cronet/android/org_chromium_net_UrlRequest.cc:24: static jfieldID g_request_field; On 2014/02/27 23:06:02, mmenke wrote: > Suggest just putting all these in an anonymous namespace. Done. https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/org... net/cronet/android/org_chromium_net_UrlRequest.cc:24: static jfieldID g_request_field; On 2014/02/27 23:06:02, mmenke wrote: > Most of these names violate the Google C++ style guide. Suggest just renaming > them something like g_on_blah_[id | method | method_id]. > > Be great if we could make them no longer globals, but I don't think we should > worry about that for now. I'll address this as part of migration to generated jni headers. https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/org... net/cronet/android/org_chromium_net_UrlRequest.cc:50: || !g_method_write) { On 2014/02/27 23:06:02, mmenke wrote: > While this is the right style for Java, and allowed in C++, the "||" are pretty > much always put on the previous line in Chrome. Done. https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/org... net/cronet/android/org_chromium_net_UrlRequest.cc:56: static net::RequestPriority ConvertRequestPriority(jint request_priority) { On 2014/02/27 23:06:02, mmenke wrote: > static is not used very often before functions in chrome, suggest an anonymous > namespace instead (And putting it above UrlRequestRegisterJni). Done. https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/org... net/cronet/android/org_chromium_net_UrlRequest.cc:63: default: return net::LOWEST; On 2014/02/27 23:06:02, mmenke wrote: > Put all the returns on the next lines. Done. https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/org... net/cronet/android/org_chromium_net_UrlRequest.cc:69: */ On 2014/02/27 23:06:02, mmenke wrote: > Use C++-style comments. (Goes for entire file) Done. https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/org... net/cronet/android/org_chromium_net_UrlRequest.cc:73: JavaVM* vm_; On 2014/02/27 23:06:02, mmenke wrote: > These should go in a private section at the bottom of the class. Done. https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/org... net/cronet/android/org_chromium_net_UrlRequest.cc:129: * Stores a reference to the request in a java field. On 2014/02/27 23:06:02, mmenke wrote: > Should Java be capitalized in comments? Done. https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/org... net/cronet/android/org_chromium_net_UrlRequest.cc:131: static void SetNativeObject(JNIEnv *env, jobject object, On 2014/02/27 23:06:02, mmenke wrote: > Static methods should be moved to the anonymous namespace up top (And can get > rid of the static) Done. https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/org... net/cronet/android/org_chromium_net_UrlRequest.cc:147: Java_org_chromium_net_UrlRequest_nativeInit( On 2014/02/27 23:06:02, mmenke wrote: > Maybe be able to move all these up a line. Done. https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/org... net/cronet/android/org_chromium_net_UrlRequest.cc:152: CHECK(context != NULL); On 2014/02/27 23:06:02, mmenke wrote: > DCHECK? Done. https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/org... net/cronet/android/org_chromium_net_UrlRequest.cc:154: const char *url_utf8 = env->GetStringUTFChars(url_string, NULL); On 2014/02/27 23:06:02, mmenke wrote: > Should be char* url_utf8. Done. https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/org... net/cronet/android/org_chromium_net_UrlRequest.cc:175: CHECK(request != NULL); On 2014/02/27 23:06:02, mmenke wrote: > DCHECK? Done. https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/org... net/cronet/android/org_chromium_net_UrlRequest.cc:189: jstring content_type) { On 2014/02/27 23:06:02, mmenke wrote: > Statics up top, -static Done. https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/org... net/cronet/android/org_chromium_net_UrlRequest.cc:190: CHECK(request != NULL); On 2014/02/27 23:06:02, mmenke wrote: > DCHECK? Done. https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/org... net/cronet/android/org_chromium_net_UrlRequest.cc:197: const char *content_type_utf8 = env->GetStringUTFChars(content_type, NULL); On 2014/02/27 23:06:02, mmenke wrote: > char* content_... Done. https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/org... net/cronet/android/org_chromium_net_UrlRequest.cc:282: case 0: On 2014/02/27 23:06:02, mmenke wrote: > case net::OK: Done. https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/org... net/cronet/android/org_chromium_net_UrlRequest.cc:282: case 0: On 2014/02/27 23:06:02, mmenke wrote: > Please add a TODO about investigating returning success on positive values, too, > as they technically indicate success. Done. https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/org... net/cronet/android/org_chromium_net_UrlRequest.cc:289: // treat downloads as complete in both cases, so we follow their lead. On 2014/02/27 23:06:02, mmenke wrote: > Add a TODO about investigating this. The fact is that Chrome does not do this, > and this library is not just being used for downloads. Done. https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/org... File net/cronet/android/org_chromium_net_UrlRequest.h (right): https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/org... net/cronet/android/org_chromium_net_UrlRequest.h:10: #ifdef __cplusplus On 2014/02/27 23:06:02, mmenke wrote: > Hmm...Wonder if this is necessary. We're using C++ style comments in this file, > so is it ever compiled as string C? Done. https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/org... net/cronet/android/org_chromium_net_UrlRequest.h:15: * Native method implementations of the org.chromium.netjni.UrlRequest class. On 2014/02/27 23:06:02, mmenke wrote: > Should use C++ style comments in C++, since that's what chrome normally uses. Done. https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/org... net/cronet/android/org_chromium_net_UrlRequest.h:19: #define REQUEST_PRIORITY_IDLE 0 On 2014/02/28 00:50:40, szym wrote: > Use enum or const. Done. https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/org... net/cronet/android/org_chromium_net_UrlRequest.h:35: Java_org_chromium_net_UrlRequest_nativeInit( On 2014/02/27 23:06:02, mmenke wrote: > Think for most of these, you can move the function name up a line. I'm assuming > the funky names are necessary. I think we could and should replace these with generated jni headers. I'll look into that. https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/org... File net/cronet/android/org_chromium_net_UrlRequestContext.cc (right): https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/org... net/cronet/android/org_chromium_net_UrlRequestContext.cc:26: static jfieldID g_field_mRequestContext; On 2014/02/27 23:06:02, mmenke wrote: > fix names / anonymous namespace Done. https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/org... net/cronet/android/org_chromium_net_UrlRequestContext.cc:28: static base::AtExitManager* s_at_exit_manager = NULL; On 2014/02/27 23:06:02, mmenke wrote: > Suggest using a g_ prefix for consistency with above. We should see if we can > get rid of these globals, but not in this CL. Done. https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/org... net/cronet/android/org_chromium_net_UrlRequestContext.cc:32: */ On 2014/02/27 23:06:02, mmenke wrote: > C++ style comments Done. https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/org... net/cronet/android/org_chromium_net_UrlRequestContext.cc:39: const char* class_name = "org/chromium/net/UrlRequestContext"; On 2014/02/27 23:06:02, mmenke wrote: > Seems like this should be "const char kClassName[]". May also want to move it > to the anonymous namespace up top, given its importance. Done. https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/org... net/cronet/android/org_chromium_net_UrlRequestContext.cc:75: == JNI_EDETACHED) { On 2014/02/27 23:06:02, mmenke wrote: > nit: "==" on previous line. Done. https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/org... net/cronet/android/org_chromium_net_UrlRequestContext.cc:83: */ On 2014/02/27 23:06:02, mmenke wrote: > C++-style comment? Done. https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/org... net/cronet/android/org_chromium_net_UrlRequestContext.cc:84: class JniURLRequestContextPeerDelegate : On 2014/02/27 23:06:02, mmenke wrote: > Move into an anonymous namespace? Done. https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/org... net/cronet/android/org_chromium_net_UrlRequestContext.cc:87: JavaVM* vm_; On 2014/02/27 23:06:02, mmenke wrote: > These should go at bottom of class, in aa private section. Done. https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/org... net/cronet/android/org_chromium_net_UrlRequestContext.cc:91: owner_ = env->NewGlobalRef(owner); On 2014/02/27 23:06:02, mmenke wrote: > This can be set in an initializer list. Done. https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/org... net/cronet/android/org_chromium_net_UrlRequestContext.cc:127: static URLRequestContextPeer* GetNativeObject(JNIEnv* env, jobject object) { On 2014/02/27 23:06:02, mmenke wrote: > Move these into an anonymous namespace? Done. https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/org... net/cronet/android/org_chromium_net_UrlRequestContext.cc:157: // Set application context On 2014/02/27 23:06:02, mmenke wrote: > Comments should end in periods, here and below. Done. https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/org... File net/cronet/android/org_chromium_net_UrlRequestContext.h (right): https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/org... net/cronet/android/org_chromium_net_UrlRequestContext.h:22: */ On 2014/02/27 23:06:02, mmenke wrote: > C++ style? Done. https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/sam... File net/cronet/android/sample/javatests/src/org/chromium/cronet_sample_apk/CronetSampleUrlTest.java (right): https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/sam... net/cronet/android/sample/javatests/src/org/chromium/cronet_sample_apk/CronetSampleUrlTest.java:30: assertEquals(200, activity.getHttpStatusCode()); On 2014/02/27 23:06:02, mmenke wrote: > 2-space indent. Done. https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/sam... net/cronet/android/sample/javatests/src/org/chromium/cronet_sample_apk/CronetSampleUrlTest.java:31: } On 2014/02/27 23:06:02, mmenke wrote: > 2-space indent. Done. https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/sam... net/cronet/android/sample/javatests/src/org/chromium/cronet_sample_apk/CronetSampleUrlTest.java:36: CronetSampleActivity activity = launchCronetSampleWithUrl("127.0.0.1:8000"); On 2014/02/27 23:06:02, mmenke wrote: > Wrap line. Done. https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/sam... net/cronet/android/sample/javatests/src/org/chromium/cronet_sample_apk/CronetSampleUrlTest.java:45: } On 2014/02/27 23:06:02, mmenke wrote: > 2-space indent (x2) Done. https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/sam... File net/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java (right): https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/sam... net/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java:19: //import java.io.FileOutputStream; On 2014/02/27 23:06:02, mmenke wrote: > Remove unused line Done. https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/sam... net/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java:29: private static final String TAG = "CronetSampleActivity"; On 2014/02/27 23:06:02, mmenke wrote: > 2 space indent (Goes for most of this file) Done. https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/sam... net/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java:35: int mHttpStatusCode = 0; On 2014/02/27 23:06:02, mmenke wrote: > Again, this naming seems to violate the style guide, but think we should leave > it for now. Ok. https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/sam... net/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java:39: super(getApplicationContext(), "Cronet Sample", UrlRequestContext.LOG_VERBOSE); On 2014/02/27 23:06:02, mmenke wrote: > Wrap at 80 chars Done. https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/sam... net/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java:122: sink); On 2014/02/27 23:06:02, mmenke wrote: > fix indent Done. https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/sam... File net/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleApplication.java (right): https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/sam... net/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleApplication.java:28: } On 2014/02/27 23:06:02, mmenke wrote: > fix indent Done. https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/sam... File net/cronet/android/sample/src/org/chromium/cronet_sample_apk/LibraryLoader.java (right): https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/sam... net/cronet/android/sample/src/org/chromium/cronet_sample_apk/LibraryLoader.java:22: } On 2014/02/27 23:06:02, mmenke wrote: > fix indent Done. https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/url... File net/cronet/android/url_request_context_peer.cc (right): https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/url... net/cronet/android/url_request_context_peer.cc:16: #include "net/url_request/url_request_job_factory_impl.h" On 2014/02/27 23:06:02, mmenke wrote: > net/base/net_errors.h. Done. https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/url... net/cronet/android/url_request_context_peer.cc:18: class BasicNetworkDelegate : public net::NetworkDelegate { On 2014/02/27 23:06:02, mmenke wrote: > Move into anonymous namespace? Done. https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/url... net/cronet/android/url_request_context_peer.cc:24: virtual int OnBeforeURLRequest(net::URLRequest* request, On 2014/02/27 23:06:02, mmenke wrote: > Comment that all these are the net::NetworkDelegate implementation. Done. https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/url... net/cronet/android/url_request_context_peer.cc:68: } On 2014/02/27 23:06:02, mmenke wrote: > Hmm...All of the functions above this are the same as NetworkDelegate's, so we > may not want them. Suggest leaving them for now, but may want to just do the > ones we care about (The tree OnCan ones). Sounds good. https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/url... net/cronet/android/url_request_context_peer.cc:100: class BasicURLRequestContext : public net::URLRequestContext { On 2014/02/27 23:06:02, mmenke wrote: > Move into anonymous namespace? Done. https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/url... net/cronet/android/url_request_context_peer.cc:110: net::URLRequestContextStorage storage_; On 2014/02/27 23:06:02, mmenke wrote: > nit: Add blank line. Done. https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/url... net/cronet/android/url_request_context_peer.cc:213: net::URLRequestContext *URLRequestContextPeer::GetURLRequestContext() { On 2014/02/27 23:06:02, mmenke wrote: > nit: net::URLRequestContext* URLRequestContextPeer Done. https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/url... File net/cronet/android/url_request_context_peer.h (right): https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/url... net/cronet/android/url_request_context_peer.h:10: #include "net/url_request/url_request_context_getter.h" On 2014/02/27 23:06:02, mmenke wrote: > base/compiler_specific.h and base/macros.h for DISALLOW_COPY_AND_ASSIGN and > OVERRIDE Done. https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/url... net/cronet/android/url_request_context_peer.h:10: #include "net/url_request/url_request_context_getter.h" On 2014/02/27 23:06:02, mmenke wrote: > Add scoped_ptr, ref_counted, NetLog, NetworkChangeNotifier headers Done. https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/url... net/cronet/android/url_request_context_peer.h:14: */ On 2014/02/27 23:06:02, mmenke wrote: > C++ style comment? Done. https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/url... net/cronet/android/url_request_context_peer.h:25: private: On 2014/02/27 23:06:02, mmenke wrote: > Blank line before private. Done. https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/url... net/cronet/android/url_request_context_peer.h:31: */ On 2014/02/27 23:06:02, mmenke wrote: > C++ style comment? Done. https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/url... net/cronet/android/url_request_context_peer.h:50: const std::string &GetUserAgent(const GURL &url) const; On 2014/02/27 23:06:02, mmenke wrote: > std::string& ... Done. https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/url... net/cronet/android/url_request_context_peer.h:54: const char* version() const; On 2014/02/27 23:06:02, mmenke wrote: > When using this naming style, functions must be inlined. Done. https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/url... net/cronet/android/url_request_context_peer.h:56: public: On 2014/02/27 23:06:02, mmenke wrote: > not needed. Done. https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/url... net/cronet/android/url_request_context_peer.h:59: GetNetworkTaskRunner() const OVERRIDE; On 2014/02/27 23:06:02, mmenke wrote: > These two are the "// net::URLRequestContextGetter implementation:" Done. https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/url... net/cronet/android/url_request_context_peer.h:76: DISALLOW_EVIL_CONSTRUCTORS(URLRequestContextPeer); On 2014/02/27 23:06:02, mmenke wrote: > DISALLOW_COPY_AND_ASSIGN? Done. https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/url... File net/cronet/android/url_request_peer.cc (right): https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/url... net/cronet/android/url_request_peer.cc:97: /* static */ On 2014/02/27 23:06:02, mmenke wrote: > C++-style comments Done. https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/url... net/cronet/android/url_request_peer.cc:113: /* static */ On 2014/02/27 23:06:02, mmenke wrote: > C++-style comments Done. https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/url... net/cronet/android/url_request_peer.cc:175: void URLRequestPeer::OnCancelRequestWrapper(URLRequestPeer *self) { On 2014/02/27 23:06:02, mmenke wrote: > URLRequestPeer* self Done. https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/url... net/cronet/android/url_request_peer.cc:196: void URLRequestPeer::OnDestroyRequest(URLRequestPeer *self) { On 2014/02/27 23:06:02, mmenke wrote: > URLRequestPeer* self Done. https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/url... net/cronet/android/url_request_peer.cc:202: void URLRequestPeer::OnResponseStarted(net::URLRequest *request) { On 2014/02/27 23:06:02, mmenke wrote: > net::URLRequest* request Done. https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/url... net/cronet/android/url_request_peer.cc:221: */ On 2014/02/27 23:06:02, mmenke wrote: > C++ style comments Done. https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/url... net/cronet/android/url_request_peer.cc:223: while (1) { On 2014/02/27 23:06:02, mmenke wrote: > while (true) Done. https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/url... net/cronet/android/url_request_peer.cc:318: size_t URLRequestPeer::BytesRead() const { return bytes_read_; } On 2014/02/27 23:06:02, mmenke wrote: > Inline and rename to bytes_read? Done. https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/url... net/cronet/android/url_request_peer.cc:321: return reinterpret_cast<unsigned char *>(read_buffer_->StartOfBuffer()); On 2014/02/27 23:06:02, mmenke wrote: > <unsigned char*> Done. https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/url... File net/cronet/android/url_request_peer.h (right): https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/url... net/cronet/android/url_request_peer.h:11: #include "net/url_request/url_request.h" On 2014/02/27 23:06:02, mmenke wrote: > ref_ptr, request_priority, OVERRIDE, DISALLOW_COPY_AND_ASSIGN headers. Done. https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/url... net/cronet/android/url_request_peer.h:19: */ On 2014/02/27 23:06:02, mmenke wrote: > C++-style comments? Done. https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/url... net/cronet/android/url_request_peer.h:92: typedef Delegate inherited; On 2014/02/27 23:06:02, mmenke wrote: > ? Done. https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/url... net/cronet/android/url_request_peer.h:129: DISALLOW_EVIL_CONSTRUCTORS(URLRequestPeer); On 2014/02/27 23:06:02, mmenke wrote: > DISALLOW_COPY_AND_ASSIGN? Done.
Going to make a couple more tiny nits (Smaller than the last set), but two immediate comments: We should rope in some legal-types for the copyright year decision (And not discuss it here). The XML files did not seem to upload correctly.
Just a couple more nits. Once we reach a decision about the copyright dates, I think it's fine to land, and then worry about cleaning it up. https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/jav... File net/cronet/android/java/src/org/chromium/net/UrlRequest.java (right): https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/jav... net/cronet/android/java/src/org/chromium/net/UrlRequest.java:28: /* On 2014/03/03 19:15:13, mef wrote: > On 2014/02/27 23:06:02, mmenke wrote: > > nit: All comments in this file other than this one and the next one use > "/**". > > Done. Does /** mean something special for javadoc? If so, should we put more > meaningful comments? > Also, it would be nice if jni_generator have copied them into _jni.h, but it > doesn't seem to do that. Honestly, I have no clue. https://codereview.chromium.org/145213003/diff/1200001/net/cronet/android/org... File net/cronet/android/org_chromium_net_UrlRequest.cc (right): https://codereview.chromium.org/145213003/diff/1200001/net/cronet/android/org... net/cronet/android/org_chromium_net_UrlRequest.cc:52: static URLRequestPeer* GetNativeObject(JNIEnv* env, jobject object) { nit: --static https://codereview.chromium.org/145213003/diff/1200001/net/cronet/android/org... net/cronet/android/org_chromium_net_UrlRequest.cc:79: (jclass)env->NewGlobalRef(env->FindClass("org/chromium/net/UrlRequest")); nit: C-style casts are forbidden. https://codereview.chromium.org/145213003/diff/1200001/net/cronet/android/org... net/cronet/android/org_chromium_net_UrlRequest.cc:90: (jclass)env->NewGlobalRef(env->FindClass("java/io/OutputStream")); nit: C-style casts are forbidden. https://codereview.chromium.org/145213003/diff/1200001/net/cronet/android/org... net/cronet/android/org_chromium_net_UrlRequest.cc:156: JavaVM* vm_; DISALLOW_COPY_AND_ASSIGN (+include the header for it) https://codereview.chromium.org/145213003/diff/1200001/net/cronet/android/org... File net/cronet/android/org_chromium_net_UrlRequestContext.cc (right): https://codereview.chromium.org/145213003/diff/1200001/net/cronet/android/org... net/cronet/android/org_chromium_net_UrlRequestContext.cc:22: namespace { nit: blank line after namespace { https://codereview.chromium.org/145213003/diff/1200001/net/cronet/android/sam... File net/cronet/android/sample/javatests/AndroidManifest.xml (right): https://codereview.chromium.org/145213003/diff/1200001/net/cronet/android/sam... net/cronet/android/sample/javatests/AndroidManifest.xml:2: doesn't ignore this. --> Should still have a copyright notice here? https://codereview.chromium.org/145213003/diff/1200001/net/cronet/android/url... File net/cronet/android/url_request_context_peer.h (right): https://codereview.chromium.org/145213003/diff/1200001/net/cronet/android/url... net/cronet/android/url_request_context_peer.h:28: int log_level_; DISALLOW_COPY_AND_ASSIGN? https://codereview.chromium.org/145213003/diff/1200001/net/cronet/android/url... File net/cronet/android/url_request_peer.h (right): https://codereview.chromium.org/145213003/diff/1200001/net/cronet/android/url... net/cronet/android/url_request_peer.h:97: // typedef Delegate inherited; Remove this (And the following blank line)
FYI, there are some bugs fixed and tests added in google3 since you grabbed this. https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/jav... File net/cronet/android/java/src/org/chromium/net/UrlRequest.java (right): https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/jav... net/cronet/android/java/src/org/chromium/net/UrlRequest.java:28: /* Yes. The double star means that it's javadoc (as opposed to a regular comment) and will be picked up for generating HTML documentation and in-IDE docs. On 2014/03/04 19:36:20, mmenke wrote: > On 2014/03/03 19:15:13, mef wrote: > > On 2014/02/27 23:06:02, mmenke wrote: > > > nit: All comments in this file other than this one and the next one use > > "/**". > > > > Done. Does /** mean something special for javadoc? If so, should we put more > > meaningful comments? > > Also, it would be nice if jni_generator have copied them into _jni.h, but it > > doesn't seem to do that. > > Honestly, I have no clue. https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/jav... net/cronet/android/java/src/org/chromium/net/UrlRequest.java:66: private Semaphore mAppendChunkSemaphore; They're android-style java. On 2014/03/03 19:15:13, mef wrote: > On 2014/02/27 23:06:02, mmenke wrote: > > All of these variable names violate the Google Java style guide: "In Google > > Style special prefixes or suffixes, like those seen in the examples name_, > > mName, s_name and kName, are not used." (Why this is different from the C++ > > style guide, I can't say). Suggest keeping them as-is for now, but adding a > > todo. Mostly refactoring things like "mUrl = url" are easy to mess up. > > Added TODO()
On 2014/03/04 21:17:39, clm wrote: > FYI, there are some bugs fixed and tests added in google3 since you grabbed > this. Cool, I'll make sure to take them prior to landing this CL. Do you know who could explain threading expectations and usage of 'synchronized' sections in UrlRequest.java? > https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/jav... > net/cronet/android/java/src/org/chromium/net/UrlRequest.java:28: /* > Yes. The double star means that it's javadoc (as opposed to a regular comment) > and will be picked up for generating HTML documentation and in-IDE docs. Nice. Is there a standard step to generate javadoc? > https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/jav... > net/cronet/android/java/src/org/chromium/net/UrlRequest.java:66: private > Semaphore mAppendChunkSemaphore; > They're android-style java. Yep, we've found that android-style java is different from google-style java. :-(
On Tue, Mar 4, 2014 at 2:25 PM, <mef@chromium.org> wrote: > On 2014/03/04 21:17:39, clm wrote: > >> FYI, there are some bugs fixed and tests added in google3 since you >> grabbed >> this. >> > > Cool, I'll make sure to take them prior to landing this CL. > Do you know who could explain threading expectations and usage of > 'synchronized' > sections in UrlRequest.java? > > tonycuadra@google.com > > > https://codereview.chromium.org/145213003/diff/1160001/ > net/cronet/android/java/src/org/chromium/net/UrlRequest.java#newcode28 > >> net/cronet/android/java/src/org/chromium/net/UrlRequest.java:28: /* >> Yes. The double star means that it's javadoc (as opposed to a regular >> comment) >> and will be picked up for generating HTML documentation and in-IDE docs. >> > Nice. Is there a standard step to generate javadoc? > > http://docs.oracle.com/javase/7/docs/technotes/tools/windows/javadoc.html#run... > > > https://codereview.chromium.org/145213003/diff/1160001/ > net/cronet/android/java/src/org/chromium/net/UrlRequest.java#newcode66 > >> net/cronet/android/java/src/org/chromium/net/UrlRequest.java:66: private >> Semaphore mAppendChunkSemaphore; >> They're android-style java. >> > Yep, we've found that android-style java is different from google-style > java. > :-( > > > https://codereview.chromium.org/145213003/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Tony, could you take a look and comment, especially about usage of synchronized sections in UrlRequest.java? clm@, I see most recent changes to netjni/ on 2/14/14, are there any more recent? thanks, -m
On Tue, Mar 4, 2014 at 2:39 PM, <mef@chromium.org> wrote: > Tony, > > could you take a look and comment, especially about usage of synchronized > sections in UrlRequest.java? > > clm@, I see most recent changes to netjni/ on 2/14/14, are there any more > recent? > > There are some other changes in java/com/google/android/libraries/social/net/, with tests. > thanks, > -m > > https://codereview.chromium.org/145213003/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
PTAL. I've addressed all comments and hopefully most of findbugs warnings. What would be a good fix for this warning? M V EI2: org.chromium.net.UrlRequest.setUploadData(String, byte[]) may expose internal representation by storing an externally mutable object into UrlRequest.mPostBody At UrlRequest.java thanks, -m https://codereview.chromium.org/145213003/diff/1200001/net/cronet/android/org... File net/cronet/android/org_chromium_net_UrlRequest.cc (right): https://codereview.chromium.org/145213003/diff/1200001/net/cronet/android/org... net/cronet/android/org_chromium_net_UrlRequest.cc:52: static URLRequestPeer* GetNativeObject(JNIEnv* env, jobject object) { On 2014/03/04 19:36:20, mmenke wrote: > nit: --static Done. https://codereview.chromium.org/145213003/diff/1200001/net/cronet/android/org... net/cronet/android/org_chromium_net_UrlRequest.cc:79: (jclass)env->NewGlobalRef(env->FindClass("org/chromium/net/UrlRequest")); On 2014/03/04 19:36:20, mmenke wrote: > nit: C-style casts are forbidden. Done. https://codereview.chromium.org/145213003/diff/1200001/net/cronet/android/org... net/cronet/android/org_chromium_net_UrlRequest.cc:90: (jclass)env->NewGlobalRef(env->FindClass("java/io/OutputStream")); On 2014/03/04 19:36:20, mmenke wrote: > nit: C-style casts are forbidden. Done. https://codereview.chromium.org/145213003/diff/1200001/net/cronet/android/org... net/cronet/android/org_chromium_net_UrlRequest.cc:156: JavaVM* vm_; On 2014/03/04 19:36:20, mmenke wrote: > DISALLOW_COPY_AND_ASSIGN (+include the header for it) Done. https://codereview.chromium.org/145213003/diff/1200001/net/cronet/android/org... File net/cronet/android/org_chromium_net_UrlRequestContext.cc (right): https://codereview.chromium.org/145213003/diff/1200001/net/cronet/android/org... net/cronet/android/org_chromium_net_UrlRequestContext.cc:22: namespace { On 2014/03/04 19:36:20, mmenke wrote: > nit: blank line after namespace { Done. https://codereview.chromium.org/145213003/diff/1200001/net/cronet/android/sam... File net/cronet/android/sample/javatests/AndroidManifest.xml (right): https://codereview.chromium.org/145213003/diff/1200001/net/cronet/android/sam... net/cronet/android/sample/javatests/AndroidManifest.xml:2: doesn't ignore this. --> On 2014/03/04 19:36:20, mmenke wrote: > Should still have a copyright notice here? Done. https://codereview.chromium.org/145213003/diff/1200001/net/cronet/android/url... File net/cronet/android/url_request_context_peer.h (right): https://codereview.chromium.org/145213003/diff/1200001/net/cronet/android/url... net/cronet/android/url_request_context_peer.h:28: int log_level_; On 2014/03/04 19:36:21, mmenke wrote: > DISALLOW_COPY_AND_ASSIGN? Done. https://codereview.chromium.org/145213003/diff/1200001/net/cronet/android/url... File net/cronet/android/url_request_peer.h (right): https://codereview.chromium.org/145213003/diff/1200001/net/cronet/android/url... net/cronet/android/url_request_peer.h:97: // typedef Delegate inherited; On 2014/03/04 19:36:21, mmenke wrote: > Remove this (And the following blank line) Done.
lgtm
https://codereview.chromium.org/145213003/diff/1260001/net/cronet/android/jav... File net/cronet/android/java/src/org/chromium/net/UrlRequest.java (right): https://codereview.chromium.org/145213003/diff/1260001/net/cronet/android/jav... net/cronet/android/java/src/org/chromium/net/UrlRequest.java:128: mPostBody = data.clone(); Is this absolutely necessary? In same cases we may be uploading reasonably large data this way (like the bytes of a profile photo). Would it be sufficient to add a comment saying that this API takes ownership of the byte[] array and the caller should not modify it after this call?
Are you going to add the tests too? https://codereview.chromium.org/145213003/diff/1260001/net/cronet/android/jav... File net/cronet/android/java/src/org/chromium/net/UrlRequest.java (right): https://codereview.chromium.org/145213003/diff/1260001/net/cronet/android/jav... net/cronet/android/java/src/org/chromium/net/UrlRequest.java:128: mPostBody = data.clone(); Yeah, allocating a new byte[] is wasteful. We shouldn't do that. On 2014/03/05 22:11:33, tonycuadra wrote: > Is this absolutely necessary? In same cases we may be uploading reasonably large > data this way (like the bytes of a profile photo). > > Would it be sufficient to add a comment saying that this API takes ownership of > the byte[] array and the caller should not modify it after this call?
On 2014/03/05 22:29:35, clm wrote: > Are you going to add the tests too? I've added some tests to CL with Java wrappers: https://codereview.chromium.org/183333002/ I haven't (yet) found a way to port existing tests to run on chromium android test bots, but that it is definitely on my list. > https://codereview.chromium.org/145213003/diff/1260001/net/cronet/android/jav... > File net/cronet/android/java/src/org/chromium/net/UrlRequest.java (right): > > https://codereview.chromium.org/145213003/diff/1260001/net/cronet/android/jav... > net/cronet/android/java/src/org/chromium/net/UrlRequest.java:128: mPostBody = > data.clone(); > Yeah, allocating a new byte[] is wasteful. We shouldn't do that. Agreed. Is there a way in Java to pass ownership of data to UrlRequest? Eventually data seems to be copied into upload_data_stream_ by URLRequestPeer::SetPostContent. I wonder if this could be done in setUploadData, so we wouldn't need to keep mPostBody separately? > > Is this absolutely necessary? In same cases we may be uploading reasonably > large > > data this way (like the bytes of a profile photo). > > > > Would it be sufficient to add a comment saying that this API takes ownership > of > > the byte[] array and the caller should not modify it after this call? It triggers failure of a build step. Maybe it can be added to some kind of disabled warnings list, but I'd rather fix warnings than disable them...
I'm happy to sign off on landing this once we get consensus on the copyright date.
On 2014/03/06 17:05:44, mmenke wrote: > I'm happy to sign off on landing this once we get consensus on the copyright > date. Legal has looped back dannyb@ to answer this question. In the meanwhile per discussion with tonycuadra@ I've changed UrlRequest.setUploadXXX to call native methods instead of storing cloned data in mPostXXX. thanks, -m
PTAL. I've updated the copyright, restored |extern "C"| for proper JNI name mangling and got |testPostData| to run. It gets HTTP status 411 from the lighttpd test server, possibly because it is not configured properly. I suggest landing this CL and follow up on that in separate CL. https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/org... File net/cronet/android/org_chromium_net_UrlRequest.h (right): https://codereview.chromium.org/145213003/diff/1160001/net/cronet/android/org... net/cronet/android/org_chromium_net_UrlRequest.h:10: #ifdef __cplusplus On 2014/02/27 23:06:02, mmenke wrote: > Hmm...Wonder if this is necessary. We're using C++ style comments in this file, > so is it ever compiled as string C? It turns out that JNI relies on its own name mangling, which is not compatible with C++ name mangling. We should be able to remove this once we switch to generated JNI headers.
LGTM, but we'll want to update net/BUILD.gn as well, either in this CL or in a followup. https://codereview.chromium.org/145213003/diff/1290001/net/cronet/android/sam... File net/cronet/android/sample/javatests/src/org/chromium/cronet_sample_apk/CriteriaHelper.java (right): https://codereview.chromium.org/145213003/diff/1290001/net/cronet/android/sam... net/cronet/android/sample/javatests/src/org/chromium/cronet_sample_apk/CriteriaHelper.java:1: // Copyright 2014 The Chromium Authors. All rights reserved. Should this be 2012? https://codereview.chromium.org/145213003/diff/1290001/net/cronet/android/sam... File net/cronet/android/sample/javatests/src/org/chromium/cronet_sample_apk/CronetSampleTestBase.java (right): https://codereview.chromium.org/145213003/diff/1290001/net/cronet/android/sam... net/cronet/android/sample/javatests/src/org/chromium/cronet_sample_apk/CronetSampleTestBase.java:1: // Copyright 2014 The Chromium Authors. All rights reserved. 2012? https://codereview.chromium.org/145213003/diff/1290001/net/cronet/android/sam... File net/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java (right): https://codereview.chromium.org/145213003/diff/1290001/net/cronet/android/sam... net/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java:133: Log.i(TAG, "Cronet commandLineArgs[" + i + "]=" + commandLineArgs[i]); nit: Wrap line https://codereview.chromium.org/145213003/diff/1290001/net/cronet/android/sam... File net/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleApplication.java (right): https://codereview.chromium.org/145213003/diff/1290001/net/cronet/android/sam... net/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleApplication.java:30: nit: Remove blank line https://codereview.chromium.org/145213003/diff/1290001/net/net.gyp File net/net.gyp (right): https://codereview.chromium.org/145213003/diff/1290001/net/net.gyp#newcode3200 net/net.gyp:3200: ['OS=="android"', { Wonder if this should go in its own file. Fine for now. Also...what about src/tools/gn/secondary/net/BUILD.gn? I guess we aren't using gn for android currently. Certainly seem to be using it for net/ on Windows.
And, for the record, by "LGTM" I mean fine to land, but we still should do full reviews of this code, once we have tests up and running.
Thanks! I'm going to land this intial CL with understanding that follow up cleanup, test coverage and review will be required. https://codereview.chromium.org/145213003/diff/1290001/net/cronet/android/sam... File net/cronet/android/sample/javatests/src/org/chromium/cronet_sample_apk/CriteriaHelper.java (right): https://codereview.chromium.org/145213003/diff/1290001/net/cronet/android/sam... net/cronet/android/sample/javatests/src/org/chromium/cronet_sample_apk/CriteriaHelper.java:1: // Copyright 2014 The Chromium Authors. All rights reserved. On 2014/03/07 16:24:29, mmenke wrote: > Should this be 2012? This file is new (I've copied it from another Chromium project). https://codereview.chromium.org/145213003/diff/1290001/net/cronet/android/sam... File net/cronet/android/sample/javatests/src/org/chromium/cronet_sample_apk/CronetSampleTestBase.java (right): https://codereview.chromium.org/145213003/diff/1290001/net/cronet/android/sam... net/cronet/android/sample/javatests/src/org/chromium/cronet_sample_apk/CronetSampleTestBase.java:1: // Copyright 2014 The Chromium Authors. All rights reserved. On 2014/03/07 16:24:29, mmenke wrote: > 2012? This file is new (I've copied it from another Chromium project). https://codereview.chromium.org/145213003/diff/1290001/net/cronet/android/sam... File net/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java (right): https://codereview.chromium.org/145213003/diff/1290001/net/cronet/android/sam... net/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java:133: Log.i(TAG, "Cronet commandLineArgs[" + i + "]=" + commandLineArgs[i]); On 2014/03/07 16:24:29, mmenke wrote: > nit: Wrap line Done. https://codereview.chromium.org/145213003/diff/1290001/net/cronet/android/sam... File net/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleApplication.java (right): https://codereview.chromium.org/145213003/diff/1290001/net/cronet/android/sam... net/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleApplication.java:30: On 2014/03/07 16:24:29, mmenke wrote: > nit: Remove blank line Done. https://codereview.chromium.org/145213003/diff/1290001/net/net.gyp File net/net.gyp (right): https://codereview.chromium.org/145213003/diff/1290001/net/net.gyp#newcode3200 net/net.gyp:3200: ['OS=="android"', { On 2014/03/07 16:24:29, mmenke wrote: > Wonder if this should go in its own file. Fine for now. > > Also...what about src/tools/gn/secondary/net/BUILD.gn? I guess we aren't using > gn for android currently. Certainly seem to be using it for net/ on Windows. Added TODO(mef).
The CQ bit was checked by mef@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mef@chromium.org/145213003/1310001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel
Er...wait...when you copy a file, you should keep the original copyright date. Just like a book...Making a copy of it does not extend the copyright period (Modification is different, but Chrome policy is to keep the old date).
The CQ bit was checked by mef@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mef@chromium.org/145213003/1330001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mef@chromium.org/145213003/1330001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on mac_rel for step(s) telemetry_perf_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
We shouldn't be landing this yet - looks like we don't have the privacy signoff yet.
The CQ bit was checked by mef@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mef@chromium.org/145213003/1330001
Message was sent while issue was closed.
Change committed as 257131
Message was sent while issue was closed.
https://codereview.chromium.org/145213003/diff/1230001/net/cronet/android/jav... File net/cronet/android/java/src/org/chromium/net/UrlRequest.java (right): https://codereview.chromium.org/145213003/diff/1230001/net/cronet/android/jav... net/cronet/android/java/src/org/chromium/net/UrlRequest.java:72: private final ContextLock mLock; You don't have to use a new class for a lock. "Object" is traditionally used for locks. |
