Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1221513002:
Update the cronet sample app with new async APIs. (Closed)
DescriptionUpdate the cronet sample app with new async APIs.
CronetSampleActivity is modified to use the new async APIs instead of
the deprecated old APIs.
R=xunjieli@chromium.org
BUG=
Committed: https://crrev.com/462c5efdd588eec57086cf0e527eded03a647ed7
Cr-Commit-Position: refs/heads/master@{#339771}
Patch Set 1 #
Total comments: 8
Patch Set 2 : support HTTP POST #
Total comments: 4
Patch Set 3 : update UploadDataProvider class #
Total comments: 12
Patch Set 4 : modify the display of received data #
Total comments: 14
Patch Set 5 : fix byteBuffer read issue #Patch Set 6 : add textview to show received data #
Total comments: 4
Patch Set 7 : update dialog UI to support post #Patch Set 8 : update layout file #
Total comments: 5
Patch Set 9 : update log tag #Patch Set 10 : clean code a bit #Patch Set 11 : fix all issues in ninja build #
Total comments: 14
Patch Set 12 : make test work #
Total comments: 4
Patch Set 13 : clean code and update test #
Total comments: 10
Patch Set 14 : update SampleTest #
Total comments: 15
Patch Set 15 : clean the code a bit #Patch Set 16 : clean code #
Total comments: 2
Patch Set 17 : update code formatting a bit #Patch Set 18 : code formatting #
Total comments: 6
Patch Set 19 : fix some bytebuffer bug #
Total comments: 2
Patch Set 20 : add WritableByteChannel #
Total comments: 2
Messages
Total messages: 45 (5 generated)
feixiong@google.com changed reviewers: + mef@chromium.org
https://codereview.chromium.org/1221513002/diff/1/components/cronet/android/s... File components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java (left): https://codereview.chromium.org/1221513002/diff/1/components/cronet/android/s... components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java:156: String postData = getCommandLineArg(POST_DATA_KEY); hmm.. the equivalent of this in the new API is writing a UploadDataProvider, which is slightly harder to write compared to the listener. But I think we should keep the ability to post data. https://codereview.chromium.org/1221513002/diff/1/components/cronet/android/s... File components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java (right): https://codereview.chromium.org/1221513002/diff/1/components/cronet/android/s... components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java:45: int mHttpStatusCode = 0; add "private" to variables above. https://codereview.chromium.org/1221513002/diff/1/components/cronet/android/s... components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java:67: request.read(ByteBuffer.allocateDirect(READ_BUFFER_SIZE)); It's quite strange that we call in UrlRequest.Read but don't do anything with byteBuffer. Could you save the bytes into a member variable, say mBytesReceived? And convert the bytes to a string and display it in the UI? https://codereview.chromium.org/1221513002/diff/1/components/cronet/android/s... components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java:119: Log.d(TAG, "Cronet onCreate!"); I don't think this log is particularly useful. please remove it.
feixiong@google.com changed reviewers: + junda@google.com, misterikkit@google.com
On 2015/06/26 21:46:46, xunjieli (OOO Jun29 - July05) wrote: > https://codereview.chromium.org/1221513002/diff/1/components/cronet/android/s... > File > components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java > (left): > > https://codereview.chromium.org/1221513002/diff/1/components/cronet/android/s... > components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java:156: > String postData = getCommandLineArg(POST_DATA_KEY); > hmm.. the equivalent of this in the new API is writing a UploadDataProvider, > which is slightly harder to write compared to the listener. But I think we > should keep the ability to post data. > > https://codereview.chromium.org/1221513002/diff/1/components/cronet/android/s... > File > components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java > (right): > > https://codereview.chromium.org/1221513002/diff/1/components/cronet/android/s... > components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java:45: > int mHttpStatusCode = 0; > add "private" to variables above. > > https://codereview.chromium.org/1221513002/diff/1/components/cronet/android/s... > components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java:67: > request.read(ByteBuffer.allocateDirect(READ_BUFFER_SIZE)); > It's quite strange that we call in UrlRequest.Read but don't do anything with > byteBuffer. Could you save the bytes into a member variable, say mBytesReceived? > And convert the bytes to a string and display it in the UI? > > https://codereview.chromium.org/1221513002/diff/1/components/cronet/android/s... > components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java:119: > Log.d(TAG, "Cronet onCreate!"); > I don't think this log is particularly useful. please remove it. modify the code accordingly and a new patch set is submitted.
https://codereview.chromium.org/1221513002/diff/1/components/cronet/android/s... File components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java (left): https://codereview.chromium.org/1221513002/diff/1/components/cronet/android/s... components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java:156: String postData = getCommandLineArg(POST_DATA_KEY); On 2015/06/26 21:46:46, xunjieli (OOO Jun29 - July05) wrote: > hmm.. the equivalent of this in the new API is writing a UploadDataProvider, > which is slightly harder to write compared to the listener. But I think we > should keep the ability to post data. Done. https://codereview.chromium.org/1221513002/diff/1/components/cronet/android/s... File components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java (right): https://codereview.chromium.org/1221513002/diff/1/components/cronet/android/s... components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java:45: int mHttpStatusCode = 0; On 2015/06/26 21:46:46, xunjieli (OOO Jun29 - July05) wrote: > add "private" to variables above. Done. https://codereview.chromium.org/1221513002/diff/1/components/cronet/android/s... components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java:67: request.read(ByteBuffer.allocateDirect(READ_BUFFER_SIZE)); On 2015/06/26 21:46:46, xunjieli (OOO Jun29 - July05) wrote: > It's quite strange that we call in UrlRequest.Read but don't do anything with > byteBuffer. Could you save the bytes into a member variable, say mBytesReceived? > And convert the bytes to a string and display it in the UI? Done. https://codereview.chromium.org/1221513002/diff/1/components/cronet/android/s... components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java:119: Log.d(TAG, "Cronet onCreate!"); On 2015/06/26 21:46:46, xunjieli (OOO Jun29 - July05) wrote: > I don't think this log is particularly useful. please remove it. Done.
https://codereview.chromium.org/1221513002/diff/20001/components/cronet/andro... File components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java (right): https://codereview.chromium.org/1221513002/diff/20001/components/cronet/andro... components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java:147: byteBuffer.put(mReads); I see that you copied some of the logic from TestDataProvider.java, which made the assumption that a single read fits in byteBuffer. However, this is not always true in non-test scenarios. The size of upload data can be larger than what byteBuffer's remaining capacity is. If that's the case, you need to remember how much you've written, and continue writing next time UploadDataProvider#read is invoked. https://codereview.chromium.org/1221513002/diff/20001/components/cronet/andro... components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java:155: completeRunnable.run(); Since you always run the onReadSucceeded/onRewindSucceeded on current thread, you don't need to make it into a Runnable.
https://codereview.chromium.org/1221513002/diff/20001/components/cronet/andro... File components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java (right): https://codereview.chromium.org/1221513002/diff/20001/components/cronet/andro... components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java:147: byteBuffer.put(mReads); On 2015/07/06 15:35:02, xunjieli wrote: > I see that you copied some of the logic from TestDataProvider.java, which made > the assumption that a single read fits in byteBuffer. However, this is not > always true in non-test scenarios. > > The size of upload data can be larger than what byteBuffer's remaining capacity > is. If that's the case, you need to remember how much you've written, and > continue writing next time UploadDataProvider#read is invoked. Done. https://codereview.chromium.org/1221513002/diff/20001/components/cronet/andro... components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java:155: completeRunnable.run(); On 2015/07/06 15:35:02, xunjieli wrote: > Since you always run the onReadSucceeded/onRewindSucceeded on current thread, > you don't need to make it into a Runnable. Done.
https://codereview.chromium.org/1221513002/diff/40001/components/cronet/andro... File components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java (right): https://codereview.chromium.org/1221513002/diff/40001/components/cronet/andro... components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java:42: private static final int READ_BUFFER_SIZE = 32 * 1024; I prefer inline this variable. We don't want to make it looks like that embedders have to allocate 32k. https://codereview.chromium.org/1221513002/diff/40001/components/cronet/andro... components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java:75: CronetSampleActivity.this.runOnUiThread(new Runnable() { This isn't quite right. If the response body is large, onReadCompleted can be called multiple times. In that case, we want to accumulate the results in mBytesReceived, and only display the content when onSucceeded is invoked. https://codereview.chromium.org/1221513002/diff/40001/components/cronet/andro... components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java:131: class SampleUploadDataProvider implements UploadDataProvider { Suggest changing SampleUploadDataProvider to SimpleUploadDataProvider. Same goes to the SampleUrlRequestListener. https://codereview.chromium.org/1221513002/diff/40001/components/cronet/andro... components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java:133: private byte[] mReads; Suggest changing mReads to mUploadData. https://codereview.chromium.org/1221513002/diff/40001/components/cronet/andro... components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java:156: mOffset = mOffset + byteBuffer.remaining(); nit: use "+=". https://codereview.chromium.org/1221513002/diff/40001/components/cronet/andro... components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java:163: Log.i(TAG, "****** UploadDataProvider rewind ******"); I don't see how the log statement is useful. Please remove this line and line 149.
https://codereview.chromium.org/1221513002/diff/40001/components/cronet/andro... File components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java (right): https://codereview.chromium.org/1221513002/diff/40001/components/cronet/andro... components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java:42: private static final int READ_BUFFER_SIZE = 32 * 1024; On 2015/07/06 20:45:42, xunjieli wrote: > I prefer inline this variable. We don't want to make it looks like that > embedders have to allocate 32k. Done. https://codereview.chromium.org/1221513002/diff/40001/components/cronet/andro... components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java:75: CronetSampleActivity.this.runOnUiThread(new Runnable() { On 2015/07/06 20:45:42, xunjieli wrote: > This isn't quite right. If the response body is large, onReadCompleted can be > called multiple times. In that case, we want to accumulate the results in > mBytesReceived, and only display the content when onSucceeded is invoked. Done. https://codereview.chromium.org/1221513002/diff/40001/components/cronet/andro... components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java:131: class SampleUploadDataProvider implements UploadDataProvider { On 2015/07/06 20:45:42, xunjieli wrote: > Suggest changing SampleUploadDataProvider to SimpleUploadDataProvider. Same goes > to the SampleUrlRequestListener. Done. https://codereview.chromium.org/1221513002/diff/40001/components/cronet/andro... components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java:133: private byte[] mReads; On 2015/07/06 20:45:42, xunjieli wrote: > Suggest changing mReads to mUploadData. Done. https://codereview.chromium.org/1221513002/diff/40001/components/cronet/andro... components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java:156: mOffset = mOffset + byteBuffer.remaining(); On 2015/07/06 20:45:42, xunjieli wrote: > nit: use "+=". Done. https://codereview.chromium.org/1221513002/diff/40001/components/cronet/andro... components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java:163: Log.i(TAG, "****** UploadDataProvider rewind ******"); On 2015/07/06 20:45:42, xunjieli wrote: > I don't see how the log statement is useful. Please remove this line and line > 149. Done.
a few quick comments. https://codereview.chromium.org/1221513002/diff/60001/components/cronet/andro... File components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java (right): https://codereview.chromium.org/1221513002/diff/60001/components/cronet/andro... components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java:71: mBytesReceived.append(new String(byteBuffer.array(), Charset.forName("UTF-8"))); hmm.. looks like byteBuffer.array() gets the entire byteBuffer as an array disregarding the position of the byteBuffer. If you have "Hello World" as data in your byteBuffer, and the position is 5, byteBuffer.array() will give "Hello World" instead of "World". https://codereview.chromium.org/1221513002/diff/60001/components/cronet/andro... components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java:72: request.read(ByteBuffer.allocateDirect(32 * 1024)); Instead of allocating a new byte buffer, can we reuse the same one? Maybe something like: byteBuffer.position(0); request.read(byteBuffer); https://codereview.chromium.org/1221513002/diff/60001/components/cronet/andro... components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java:86: startActivity(intent); I don't think we need a new Intent here. Maybe remove line 84, 85, and the one in onFailed? https://codereview.chromium.org/1221513002/diff/60001/components/cronet/andro... components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java:99: successToast.show(); The UI is pretty janky now with a medium sized response body (try google.com). Instead of doing two toasts, could you display the text in a TextView instead? Not a UI expert, so I am open for other suggestions as well. https://codereview.chromium.org/1221513002/diff/60001/components/cronet/andro... components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java:126: class SimpleUploadDataProvider implements UploadDataProvider { this should be a static class.
https://codereview.chromium.org/1221513002/diff/60001/components/cronet/andro... File components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java (right): https://codereview.chromium.org/1221513002/diff/60001/components/cronet/andro... components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java:71: mBytesReceived.append(new String(byteBuffer.array(), Charset.forName("UTF-8"))); On 2015/07/07 15:35:58, xunjieli wrote: > hmm.. looks like byteBuffer.array() gets the entire byteBuffer as an array > disregarding the position of the byteBuffer. If you have "Hello World" as data > in your byteBuffer, and the position is 5, byteBuffer.array() will give "Hello > World" instead of "World". Done. https://codereview.chromium.org/1221513002/diff/60001/components/cronet/andro... components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java:72: request.read(ByteBuffer.allocateDirect(32 * 1024)); On 2015/07/07 15:35:58, xunjieli wrote: > Instead of allocating a new byte buffer, can we reuse the same one? > Maybe something like: > byteBuffer.position(0); > request.read(byteBuffer); change the code to reuse the same butebuffer. https://codereview.chromium.org/1221513002/diff/60001/components/cronet/andro... components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java:86: startActivity(intent); On 2015/07/07 15:35:58, xunjieli wrote: > I don't think we need a new Intent here. Maybe remove line 84, 85, and the one > in onFailed? do we need an intent as a parameter for the startActivity? I change the intent as the outer class instance variable to avoid a new intent every time. https://codereview.chromium.org/1221513002/diff/60001/components/cronet/andro... components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java:99: successToast.show(); On 2015/07/07 15:35:58, xunjieli wrote: > The UI is pretty janky now with a medium sized response body (try http://google.com). > Instead of doing two toasts, could you display the text in a TextView instead? > Not a UI expert, so I am open for other suggestions as well. maybe WebView be better? https://codereview.chromium.org/1221513002/diff/60001/components/cronet/andro... components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java:126: class SimpleUploadDataProvider implements UploadDataProvider { On 2015/07/07 15:35:58, xunjieli wrote: > this should be a static class. I didn't quite get the reason for this. do we also need to change the listener class to static then?
https://codereview.chromium.org/1221513002/diff/60001/components/cronet/andro... File components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java (right): https://codereview.chromium.org/1221513002/diff/60001/components/cronet/andro... components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java:72: request.read(ByteBuffer.allocateDirect(32 * 1024)); On 2015/07/07 15:35:58, xunjieli wrote: > Instead of allocating a new byte buffer, can we reuse the same one? > Maybe something like: > byteBuffer.position(0); > request.read(byteBuffer); Done. https://codereview.chromium.org/1221513002/diff/60001/components/cronet/andro... components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java:86: startActivity(intent); On 2015/07/07 23:41:34, feixiong wrote: > On 2015/07/07 15:35:58, xunjieli wrote: > > I don't think we need a new Intent here. Maybe remove line 84, 85, and the one > > in onFailed? > > do we need an intent as a parameter for the startActivity? I change the intent > as the outer class instance variable to avoid a new intent every time. you are correct. remove these lines. https://codereview.chromium.org/1221513002/diff/60001/components/cronet/andro... components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java:86: startActivity(intent); On 2015/07/07 15:35:58, xunjieli wrote: > I don't think we need a new Intent here. Maybe remove line 84, 85, and the one > in onFailed? Done. https://codereview.chromium.org/1221513002/diff/60001/components/cronet/andro... components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java:99: successToast.show(); On 2015/07/07 23:41:34, feixiong wrote: > On 2015/07/07 15:35:58, xunjieli wrote: > > The UI is pretty janky now with a medium sized response body (try > http://google.com). > > Instead of doing two toasts, could you display the text in a TextView instead? > > Not a UI expert, so I am open for other suggestions as well. > > maybe WebView be better? use a textview to display the received data right now. the UI looks much better than toast.
Looks really good! There isn't a UI hook to do a POST request. Could you add a simple text field in the dialog box for entering POST data? (Then you can get rid of getCommandLineArg). The toast box is very subtle (compared to the dialog prompt which gets shown immediately after success), maybe display the text in the toast above the plain html data in the same text view (or a different text view, whichever you prefer) and get rid of the toast entirely? https://codereview.chromium.org/1221513002/diff/100001/components/cronet/andr... File components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java (right): https://codereview.chromium.org/1221513002/diff/100001/components/cronet/andr... components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java:46: private boolean mLoading = false; I don't think mLoading is used anymore. Could you remove it as well as isLoading() ? https://codereview.chromium.org/1221513002/diff/100001/components/cronet/andr... components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java:106: final CharSequence text = "Failed " + info.getUrl() + " (" + error + ")"; info can be null. Try a weird hostname (eg. https://this-weird-hostname-doesnt-exist.com), then you will have a null pointer exception on line 106. I guess you can get the mUrl from the parent class. Also, could you do error.getMessage() instead of just error?
update the dialog to support POST and remove CommandLineArg. https://codereview.chromium.org/1221513002/diff/100001/components/cronet/andr... File components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java (right): https://codereview.chromium.org/1221513002/diff/100001/components/cronet/andr... components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java:46: private boolean mLoading = false; On 2015/07/08 21:04:07, xunjieli wrote: > I don't think mLoading is used anymore. Could you remove it as well as > isLoading() ? Done. https://codereview.chromium.org/1221513002/diff/100001/components/cronet/andr... components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java:106: final CharSequence text = "Failed " + info.getUrl() + " (" + error + ")"; On 2015/07/08 21:04:07, xunjieli wrote: > info can be null. Try a weird hostname (eg. > https://this-weird-hostname-doesnt-exist.com), then you will have a null pointer > exception on line 106. I guess you can get the mUrl from the parent class. Also, > could you do error.getMessage() instead of just error? Done.
There are a couple of findbugs errors, please fix those (otherwise your change won't get pass the commit queue). To build the app with findbugs on, do the following in your checkout: $ components/cronet/tools/cr_cronet.py gyp $ ninja -C out/Debug cronet_sample_apk The first command initializes the gyp environment with findbugs set to true. The second one actually builds it. $ build/android/adb_install_apk.py --apk=CronetSample.apk After successfully building the sample app, this will get the app on your device. https://codereview.chromium.org/1221513002/diff/140001/components/cronet/andr... File components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java (right): https://codereview.chromium.org/1221513002/diff/140001/components/cronet/andr... components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java:37: private static final String TAG = Log.makeTag("CronetSampleActivity"); Log.makeTag method is removed. Use private static final String TAG = "cr.CronetSampleActivity"; instead Same for the one in CronetSampleApplication.java. https://codereview.chromium.org/1221513002/diff/140001/components/cronet/andr... components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java:109: class SimpleUploadDataProvider implements UploadDataProvider { This should be a nested static class because it does not access member variables of its parent class (whereas, SimpleUrlRequestListener accessed member variables like mUrl, mResltText, etc.). https://docs.oracle.com/javase/tutorial/java/javaOO/nested.html
fix all findbugs errors https://codereview.chromium.org/1221513002/diff/140001/components/cronet/andr... File components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java (right): https://codereview.chromium.org/1221513002/diff/140001/components/cronet/andr... components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java:37: private static final String TAG = Log.makeTag("CronetSampleActivity"); On 2015/07/13 18:47:16, xunjieli wrote: > Log.makeTag method is removed. > Use > private static final String TAG = "cr.CronetSampleActivity"; > instead > > Same for the one in CronetSampleApplication.java. change the log tag to cr.CronetActivity and cr.CronetApplication accordingly. seems java has a limit on the length of log tag as below: java.lang.IllegalArgumentException: Log tag "cr.CronetSampleApplication" exceeds limit of 23 characters https://codereview.chromium.org/1221513002/diff/140001/components/cronet/andr... components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java:37: private static final String TAG = Log.makeTag("CronetSampleActivity"); On 2015/07/13 18:47:16, xunjieli wrote: > Log.makeTag method is removed. > Use > private static final String TAG = "cr.CronetSampleActivity"; > instead > > Same for the one in CronetSampleApplication.java. Done. https://codereview.chromium.org/1221513002/diff/140001/components/cronet/andr... components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java:109: class SimpleUploadDataProvider implements UploadDataProvider { On 2015/07/13 18:47:16, xunjieli wrote: > This should be a nested static class because it does not access member variables > of its parent class (whereas, SimpleUrlRequestListener accessed member variables > like mUrl, mResltText, etc.). > https://docs.oracle.com/javase/tutorial/java/javaOO/nested.html Done.
Sorry, I forgot that there is also a test for this sample app, so we need to keep the test updated as well. The test file is components/cronet/android/sample/javatests/src/org/chromium/cronet_sample_apk/CronetSampleTest.java. To run compile and run the test: 1. ninja -C out/Debug cronet_sample_test_apk (compiles the test target) 2. build/android/adb_install_apk.py --apk=CronetSample.apk (installs the sample app) 3. build/android/test_runner.py instrumentation --test-apk=CronetSampleTest (runs the test) I didn't realize that mLoading is used in test, so we have to add that bit of logic back in the activity file. Could you make sure the test still works? https://codereview.chromium.org/1221513002/diff/200001/components/cronet/andr... File components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java (right): https://codereview.chromium.org/1221513002/diff/200001/components/cronet/andr... components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java:37: private static final String TAG = "cr.CronetActivity"; nit: "cr.CronetActivity" is a bit general. Let's change this to "cr.CronetSample". https://codereview.chromium.org/1221513002/diff/200001/components/cronet/andr... components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java:44: TextView mRcvDataText; nit: add private. https://codereview.chromium.org/1221513002/diff/200001/components/cronet/andr... components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java:83: final String text = "Completed " + responseInfo.getUrl() + " (" nit: s/responseInfo.getUrl()/url https://codereview.chromium.org/1221513002/diff/200001/components/cronet/andr... components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java:158: promptForURL("https://"); In order for the test to work, you need to add back the getUrlFromIntent part. String appUrl = getUrlFromIntent(getIntent()); if (appUrl == null) { promptForURL("https://"); } else { startWithURL(appUrl); } https://codereview.chromium.org/1221513002/diff/200001/components/cronet/andr... components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java:187: request.addHeader("Content-Type", "useless/string"); Content type doesnt look right. Suggest using "application/x-www-form-urlencoded" instead. https://codereview.chromium.org/1221513002/diff/200001/components/cronet/andr... components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java:203: public String getUrl() { Could you comment on getUrl() and getHttpStatusCode() that these two methods are used in testing. https://codereview.chromium.org/1221513002/diff/200001/components/cronet/andr... File components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleApplication.java (right): https://codereview.chromium.org/1221513002/diff/200001/components/cronet/andr... components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleApplication.java:24: initializeApplicationParameters(this); Let's get rid of the initializeApplicationParameters method, which is really an extra detail that Cronet consumers don't need to concern themselves with. It does not affect the test.
The test is just a GET request. You don't need to add an extra test for POST request, since the simple server does not support POST yet (we have a test somewhere else for that). Let me know if you have any questions. Feel free to ping me over gchat.
make all the changes accordingly and pass the test. https://codereview.chromium.org/1221513002/diff/200001/components/cronet/andr... File components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java (right): https://codereview.chromium.org/1221513002/diff/200001/components/cronet/andr... components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java:37: private static final String TAG = "cr.CronetActivity"; On 2015/07/15 16:38:33, xunjieli wrote: > nit: "cr.CronetActivity" is a bit general. Let's change this to > "cr.CronetSample". Done. https://codereview.chromium.org/1221513002/diff/200001/components/cronet/andr... components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java:44: TextView mRcvDataText; On 2015/07/15 16:38:33, xunjieli wrote: > nit: add private. Done. https://codereview.chromium.org/1221513002/diff/200001/components/cronet/andr... components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java:83: final String text = "Completed " + responseInfo.getUrl() + " (" On 2015/07/15 16:38:33, xunjieli wrote: > nit: s/responseInfo.getUrl()/url Done. https://codereview.chromium.org/1221513002/diff/200001/components/cronet/andr... components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java:158: promptForURL("https://"); On 2015/07/15 16:38:33, xunjieli wrote: > In order for the test to work, you need to add back the getUrlFromIntent part. > > > String appUrl = getUrlFromIntent(getIntent()); > if (appUrl == null) { > promptForURL("https://"); > } else { > startWithURL(appUrl); > } Done. https://codereview.chromium.org/1221513002/diff/200001/components/cronet/andr... components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java:187: request.addHeader("Content-Type", "useless/string"); On 2015/07/15 16:38:33, xunjieli wrote: > Content type doesnt look right. Suggest using > "application/x-www-form-urlencoded" instead. Done. https://codereview.chromium.org/1221513002/diff/200001/components/cronet/andr... components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java:203: public String getUrl() { On 2015/07/15 16:38:33, xunjieli wrote: > Could you comment on getUrl() and getHttpStatusCode() that these two methods are > used in testing. Done. https://codereview.chromium.org/1221513002/diff/200001/components/cronet/andr... File components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleApplication.java (right): https://codereview.chromium.org/1221513002/diff/200001/components/cronet/andr... components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleApplication.java:24: initializeApplicationParameters(this); On 2015/07/15 16:38:33, xunjieli wrote: > Let's get rid of the initializeApplicationParameters method, which is really an > extra detail that Cronet consumers don't need to concern themselves with. It > does not affect the test. Done.
https://codereview.chromium.org/1221513002/diff/220001/components/cronet/andr... File components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java (right): https://codereview.chromium.org/1221513002/diff/220001/components/cronet/andr... components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java:40: public static final String COMMAND_LINE_ARGS_KEY = "commandLineArgs"; This variable is referenced from the test. You can remove that reference (since it doesn't do anything), and get rid of this variable here. https://codereview.chromium.org/1221513002/diff/220001/components/cronet/andr... File components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleApplication.java (right): https://codereview.chromium.org/1221513002/diff/220001/components/cronet/andr... components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleApplication.java:15: private static final String PRIVATE_DATA_DIRECTORY_SUFFIX = "cronet_sample"; These two variables are no longer used, right? Please remove.
https://codereview.chromium.org/1221513002/diff/220001/components/cronet/andr... File components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java (right): https://codereview.chromium.org/1221513002/diff/220001/components/cronet/andr... components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java:40: public static final String COMMAND_LINE_ARGS_KEY = "commandLineArgs"; On 2015/07/15 19:37:38, xunjieli wrote: > This variable is referenced from the test. You can remove that reference (since > it doesn't do anything), and get rid of this variable here. Done. https://codereview.chromium.org/1221513002/diff/220001/components/cronet/andr... File components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleApplication.java (right): https://codereview.chromium.org/1221513002/diff/220001/components/cronet/andr... components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleApplication.java:15: private static final String PRIVATE_DATA_DIRECTORY_SUFFIX = "cronet_sample"; On 2015/07/15 19:37:38, xunjieli wrote: > These two variables are no longer used, right? Please remove. Done.
https://codereview.chromium.org/1221513002/diff/240001/components/cronet/andr... File components/cronet/android/sample/javatests/src/org/chromium/cronet_sample_apk/CronetSampleTest.java (right): https://codereview.chromium.org/1221513002/diff/240001/components/cronet/andr... components/cronet/android/sample/javatests/src/org/chromium/cronet_sample_apk/CronetSampleTest.java:57: * CronetSampleActivity.DEFAULT_SHELL_URL. I don't think CronetSampleActivity.DEFAULT_SHELL_URL is there anymore. please remove the second sentence. https://codereview.chromium.org/1221513002/diff/240001/components/cronet/andr... components/cronet/android/sample/javatests/src/org/chromium/cronet_sample_apk/CronetSampleTest.java:63: if (url != null) { We can get rid of the conditional, since the url has to be non-null anyway (there isn't a fallback). https://codereview.chromium.org/1221513002/diff/240001/components/cronet/andr... components/cronet/android/sample/javatests/src/org/chromium/cronet_sample_apk/CronetSampleTest.java:69: setActivityIntent(intent); Invoke waitForActiveShellToBeDoneLoading after this line. An example is here: https://code.google.com/p/chromium/codesearch#chromium/src/components/cronet/.... https://codereview.chromium.org/1221513002/diff/240001/components/cronet/andr... File components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java (right): https://codereview.chromium.org/1221513002/diff/240001/components/cronet/andr... components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java:43: boolean mLoading = false; nit: add private https://codereview.chromium.org/1221513002/diff/240001/components/cronet/andr... components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java:71: byteBuffer.position() + byteBuffer.arrayOffset(), byteBuffer.limit())); I don't think converting partial response body from bytes to string always works correctly, since the boundaries might be off. Suggest to use ByteArrayOutputStream for mBytesReceived instead.
modify the code accordingly. https://codereview.chromium.org/1221513002/diff/240001/components/cronet/andr... File components/cronet/android/sample/javatests/src/org/chromium/cronet_sample_apk/CronetSampleTest.java (right): https://codereview.chromium.org/1221513002/diff/240001/components/cronet/andr... components/cronet/android/sample/javatests/src/org/chromium/cronet_sample_apk/CronetSampleTest.java:57: * CronetSampleActivity.DEFAULT_SHELL_URL. On 2015/07/15 20:21:44, xunjieli wrote: > I don't think CronetSampleActivity.DEFAULT_SHELL_URL is there anymore. please > remove the second sentence. Done. https://codereview.chromium.org/1221513002/diff/240001/components/cronet/andr... components/cronet/android/sample/javatests/src/org/chromium/cronet_sample_apk/CronetSampleTest.java:63: if (url != null) { On 2015/07/15 20:21:43, xunjieli wrote: > We can get rid of the conditional, since the url has to be non-null anyway > (there isn't a fallback). Done. https://codereview.chromium.org/1221513002/diff/240001/components/cronet/andr... components/cronet/android/sample/javatests/src/org/chromium/cronet_sample_apk/CronetSampleTest.java:69: setActivityIntent(intent); On 2015/07/15 20:21:44, xunjieli wrote: > Invoke waitForActiveShellToBeDoneLoading after this line. > An example is here: > https://code.google.com/p/chromium/codesearch#chromium/src/components/cronet/.... Done. https://codereview.chromium.org/1221513002/diff/240001/components/cronet/andr... File components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java (right): https://codereview.chromium.org/1221513002/diff/240001/components/cronet/andr... components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java:43: boolean mLoading = false; On 2015/07/15 20:21:44, xunjieli wrote: > nit: add private Done. https://codereview.chromium.org/1221513002/diff/240001/components/cronet/andr... components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java:71: byteBuffer.position() + byteBuffer.arrayOffset(), byteBuffer.limit())); On 2015/07/15 20:21:44, xunjieli wrote: > I don't think converting partial response body from bytes to string always works > correctly, since the boundaries might be off. > Suggest to use ByteArrayOutputStream for mBytesReceived instead. Done.
https://codereview.chromium.org/1221513002/diff/260001/components/cronet/andr... File components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java (right): https://codereview.chromium.org/1221513002/diff/260001/components/cronet/andr... components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java:72: byteBuffer.position() + byteBuffer.arrayOffset(), byteBuffer.limit()); The third param is the number of bytes to read, so it should = limit - position, which is byteBuffer.remaining().
Looks good! a few additional comments. Will sign off once you fix these. https://codereview.chromium.org/1221513002/diff/260001/components/cronet/andr... File components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java (right): https://codereview.chromium.org/1221513002/diff/260001/components/cronet/andr... components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java:193: private static String getUrlFromIntent(Intent intent) { nit: maybe inline this method. https://codereview.chromium.org/1221513002/diff/260001/components/cronet/andr... components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java:207: public void startWithURL(String url) { nit:private https://codereview.chromium.org/1221513002/diff/260001/components/cronet/andr... components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java:211: public void startWithURL(String url, String postData) { nit:private https://codereview.chromium.org/1221513002/diff/260001/components/cronet/andr... components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java:244: public void startNetLog() { nit:private https://codereview.chromium.org/1221513002/diff/260001/components/cronet/andr... components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java:250: public void stopNetLog() { nit:private
https://codereview.chromium.org/1221513002/diff/260001/components/cronet/andr... File components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java (right): https://codereview.chromium.org/1221513002/diff/260001/components/cronet/andr... components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java:72: byteBuffer.position() + byteBuffer.arrayOffset(), byteBuffer.limit()); On 2015/07/16 17:55:21, xunjieli wrote: > The third param is the number of bytes to read, so it should = limit - position, > which is byteBuffer.remaining(). Done. https://codereview.chromium.org/1221513002/diff/260001/components/cronet/andr... components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java:193: private static String getUrlFromIntent(Intent intent) { On 2015/07/16 18:13:24, xunjieli wrote: > nit: maybe inline this method. Done. https://codereview.chromium.org/1221513002/diff/260001/components/cronet/andr... components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java:207: public void startWithURL(String url) { On 2015/07/16 18:13:24, xunjieli wrote: > nit:private Done. https://codereview.chromium.org/1221513002/diff/260001/components/cronet/andr... components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java:211: public void startWithURL(String url, String postData) { On 2015/07/16 18:13:24, xunjieli wrote: > nit:private Done. https://codereview.chromium.org/1221513002/diff/260001/components/cronet/andr... components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java:244: public void startNetLog() { On 2015/07/16 18:13:24, xunjieli wrote: > nit:private Done. https://codereview.chromium.org/1221513002/diff/260001/components/cronet/andr... components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java:250: public void stopNetLog() { On 2015/07/16 18:13:24, xunjieli wrote: > nit:private Done.
https://codereview.chromium.org/1221513002/diff/260001/components/cronet/andr... File components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java (right): https://codereview.chromium.org/1221513002/diff/260001/components/cronet/andr... components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java:193: private static String getUrlFromIntent(Intent intent) { On 2015/07/16 18:54:44, feixiong wrote: > On 2015/07/16 18:13:24, xunjieli wrote: > > nit: maybe inline this method. > > Done. Not quite what I had in mind. Since this method is pretty small and used only in one place, I meant we could move this piece of logic to its call place instead of having it as a separate method.
https://codereview.chromium.org/1221513002/diff/260001/components/cronet/andr... File components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java (right): https://codereview.chromium.org/1221513002/diff/260001/components/cronet/andr... components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java:193: private static String getUrlFromIntent(Intent intent) { On 2015/07/16 19:00:17, xunjieli wrote: > On 2015/07/16 18:54:44, feixiong wrote: > > On 2015/07/16 18:13:24, xunjieli wrote: > > > nit: maybe inline this method. > > > > Done. > > Not quite what I had in mind. Since this method is pretty small and used only in > one place, I meant we could move this piece of logic to its call place instead > of having it as a separate method. oh, make sense! https://codereview.chromium.org/1221513002/diff/260001/components/cronet/andr... components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java:193: private static String getUrlFromIntent(Intent intent) { On 2015/07/16 19:00:17, xunjieli wrote: > On 2015/07/16 18:54:44, feixiong wrote: > > On 2015/07/16 18:13:24, xunjieli wrote: > > > nit: maybe inline this method. > > > > Done. > > Not quite what I had in mind. Since this method is pretty small and used only in > one place, I meant we could move this piece of logic to its call place instead > of having it as a separate method. Done.
LGTM https://codereview.chromium.org/1221513002/diff/300001/components/cronet/andr... File components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java (right): https://codereview.chromium.org/1221513002/diff/300001/components/cronet/andr... components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java:159: .enableQUIC(true); The formatting is a bit off. Could you do "git cl format" before uploading the patch?
https://codereview.chromium.org/1221513002/diff/300001/components/cronet/andr... File components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java (right): https://codereview.chromium.org/1221513002/diff/300001/components/cronet/andr... components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java:159: .enableQUIC(true); On 2015/07/16 19:23:22, xunjieli wrote: > The formatting is a bit off. Could you do "git cl format" before uploading the > patch? Done.
sorry for the delay, looks pretty good! https://codereview.chromium.org/1221513002/diff/340001/components/cronet/andr... File components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java (right): https://codereview.chromium.org/1221513002/diff/340001/components/cronet/andr... components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java:46: private TextView mRcvDataText; nit: Please use full words in variable names. https://codereview.chromium.org/1221513002/diff/340001/components/cronet/andr... components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java:70: mBytesReceived.write(byteBuffer.array(), according to comment on ByteBuffer.allocateDirect(): Whether or not it has a backing array is unspecified. Can we avoid using array / arrayOffset here? https://codereview.chromium.org/1221513002/diff/340001/components/cronet/andr... components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java:134: mOffset += byteBuffer.remaining(); isn't byteBuffer.remaining() affected by byteBuffer.put()?
https://codereview.chromium.org/1221513002/diff/340001/components/cronet/andr... File components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java (right): https://codereview.chromium.org/1221513002/diff/340001/components/cronet/andr... components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java:46: private TextView mRcvDataText; On 2015/07/20 14:57:25, mef wrote: > nit: Please use full words in variable names. Done. https://codereview.chromium.org/1221513002/diff/340001/components/cronet/andr... components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java:70: mBytesReceived.write(byteBuffer.array(), On 2015/07/20 14:57:25, mef wrote: > according to comment on ByteBuffer.allocateDirect(): Whether or not it has a > backing array is unspecified. > > Can we avoid using array / arrayOffset here? use ByteBuffer.get() method to retrieve the data. https://codereview.chromium.org/1221513002/diff/340001/components/cronet/andr... components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java:134: mOffset += byteBuffer.remaining(); On 2015/07/20 14:57:25, mef wrote: > isn't byteBuffer.remaining() affected by byteBuffer.put()? oh right! fix the bug.
https://codereview.chromium.org/1221513002/diff/350001/components/cronet/andr... File components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java (right): https://codereview.chromium.org/1221513002/diff/350001/components/cronet/andr... components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java:67: public void onReadCompleted(UrlRequest request, ResponseInfo info, ByteBuffer byteBuffer) { Maybe use WritableByteChannel.write(ByteBuffer src)?
https://codereview.chromium.org/1221513002/diff/350001/components/cronet/andr... File components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java (right): https://codereview.chromium.org/1221513002/diff/350001/components/cronet/andr... components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java:67: public void onReadCompleted(UrlRequest request, ResponseInfo info, ByteBuffer byteBuffer) { On 2015/07/20 18:47:25, mef wrote: > Maybe use WritableByteChannel.write(ByteBuffer src)? Done.
lgtm, thanks a lot for doing this!
not lgtm https://codereview.chromium.org/1221513002/diff/370001/components/cronet/andr... File components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java (right): https://codereview.chromium.org/1221513002/diff/370001/components/cronet/andr... components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java:89: final String receivedData = mBytesReceived.toString(); Wait. You never write to mBytesReceived, so this receivedData will always be empty string, right?
https://codereview.chromium.org/1221513002/diff/370001/components/cronet/andr... File components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java (right): https://codereview.chromium.org/1221513002/diff/370001/components/cronet/andr... components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java:89: final String receivedData = mBytesReceived.toString(); On 2015/07/20 22:17:31, xunjieli wrote: > Wait. You never write to mBytesReceived, so this receivedData will always be > empty string, right? oh, there is a trick here: mReceiveChannel = Channels.newChannel(mBytesReceived); so mReceiveChannel.write() would actually write to mBytesReceived directly.
The CQ bit was checked by feixiong@google.com
The CQ bit was unchecked by feixiong@google.com
On 2015/07/20 22:22:49, feixiong wrote: > https://codereview.chromium.org/1221513002/diff/370001/components/cronet/andr... > File > components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java > (right): > > https://codereview.chromium.org/1221513002/diff/370001/components/cronet/andr... > components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java:89: > final String receivedData = mBytesReceived.toString(); > On 2015/07/20 22:17:31, xunjieli wrote: > > Wait. You never write to mBytesReceived, so this receivedData will always be > > empty string, right? > > oh, there is a trick here: > > mReceiveChannel = Channels.newChannel(mBytesReceived); > > so mReceiveChannel.write() would actually write to mBytesReceived directly. Missed that line. Thanks! lgtm.
The CQ bit was checked by feixiong@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1221513002/370001
Message was sent while issue was closed.
Committed patchset #20 (id:370001)
Message was sent while issue was closed.
Patchset 20 (id:??) landed as https://crrev.com/462c5efdd588eec57086cf0e527eded03a647ed7 Cr-Commit-Position: refs/heads/master@{#339771} |
