Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
849903002:
[Cronet] Upload support for async APIs (Closed)
DescriptionUpload support for async APIs
This CL implements upload support for the new async APIs.
Most of this CL is patched from mmenke@'s CL 740673003.
BUG=412942
Committed: https://crrev.com/a57830437741d839456f92f1bb8c69d088984842
Cr-Commit-Position: refs/heads/master@{#317185}
Patch Set 1 : #
Total comments: 34
Patch Set 2 : addressed Matt's comments #
Total comments: 34
Patch Set 3 : Addressed Matt's comments #
Total comments: 16
Patch Set 4 : mRewindPending/mReadPending using a lock #Patch Set 5 : Combined with Matt's CL #
Total comments: 35
Patch Set 6 : Addressed Misha and Paul's comments #
Total comments: 2
Patch Set 7 : Addressed Matt's comments #Patch Set 8 : Remove the need to flip byte buffer #
Total comments: 4
Patch Set 9 : Addressed Misha's comments #
Total comments: 6
Patch Set 10 : Comments and added a test #
Total comments: 24
Patch Set 11 : Addressed Misha's comments #
Total comments: 4
Patch Set 12 : Calculate remaining #Patch Set 13 : Removed unnecessary argument #
Total comments: 22
Patch Set 14 : Addressed Misha's comments #Patch Set 15 : Addressed Misha's comments #Patch Set 16 : Rebased and changed destroyAdapter() to destroyDelegate() #
Total comments: 7
Patch Set 17 : Move mDataProvider.read() outside of lock #
Total comments: 16
Patch Set 18 : Addressed Matt's comments #
Total comments: 4
Patch Set 19 : Removed unnecessary check #Patch Set 20 : Addressed Misha's comments #
Total comments: 40
Patch Set 21 : Addressed Paul's comments #Patch Set 22 : Rebased #
Total comments: 14
Patch Set 23 : javadoc changes #Patch Set 24 : Added throws IOException to UploadDataProvider #
Total comments: 44
Patch Set 25 : Addressed Matt's comments #Patch Set 26 : Rebased #Patch Set 27 : updated chunk not supported test #Patch Set 28 : Move delegate destructor to implementation file to avoid chromium style error #Messages
Total messages: 96 (25 generated)
Patchset #1 (id:1) has been deleted
Patchset #2 (id:40001) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:60001) has been deleted
xunjieli@chromium.org changed reviewers: + mmenke@chromium.org
Hi Matt, here's an initial draft for the upload tests. The tests are passing. Could you take a brief look? Just want to make sure there isn't any major structural/logic problem. PTAL Thanks!
xunjieli@chromium.org changed reviewers: + mef@chromium.org
On 2015/01/15 22:29:46, xunjieli wrote: > Hi Matt, here's an initial draft for the upload tests. The tests are passing. > Could you take a brief look? Just want to make sure there isn't any major > structural/logic problem. PTAL > > Thanks! I'll (finally) have time to dig through this tomorrow, and should (even more finally?) have time to get to your other CL as well - sorry it's taken me ridiculously long on that one.
These look good! Just what I had in mind. https://codereview.chromium.org/849903002/diff/80001/components/cronet/androi... File components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/CronetUploadTest.java (right): https://codereview.chromium.org/849903002/diff/80001/components/cronet/androi... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/CronetUploadTest.java:28: mHandler = new UploadDataStreamHandler(mUploadDataStream.createAdapter()); Need to destroy the handler's C-side stuff in tearDown, too (That should take care of cleaning up the uploadDataStream, too, or at least should post a task to the executor do it). https://codereview.chromium.org/849903002/diff/80001/components/cronet/androi... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/CronetUploadTest.java:34: assertTrue(mHandler.init()); // make sure init is synchronous. These should be capitalized. Also suggest to put comments above the line they're commenting on, in most cases. It's more common in Chrome. https://codereview.chromium.org/849903002/diff/80001/components/cronet/androi... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/CronetUploadTest.java:38: assertFalse(mHandler.checkIfReadCallbackInvoked()); Suggest some line breaks in here to make these easier to read. Maybe after the last assert of each chain (Do stuff, assert stuff, line break, etc). https://codereview.chromium.org/849903002/diff/80001/components/cronet/androi... File components/cronet/android/test/src/org/chromium/cronet_test_apk/UploadDataStreamHandler.java (right): https://codereview.chromium.org/849903002/diff/80001/components/cronet/androi... components/cronet/android/test/src/org/chromium/cronet_test_apk/UploadDataStreamHandler.java:60: public boolean checkIfInitCallbackInvoked() { Suggest making this and the next method voids, and calling them checkInitCallbackNotInvoked (Or Pending or NotComplete - should also use similar names in the CC file - read_complete_ / init_complete_ should have similar names to whatever this is called), and do the asserting here. They're only intended for use to check if something hasn't happened yet. If we want to wait for something to actually happen, there are other methods to use for that. https://codereview.chromium.org/849903002/diff/80001/components/cronet/androi... components/cronet/android/test/src/org/chromium/cronet_test_apk/UploadDataStreamHandler.java:62: mCheckInit = false; This should be initialized to the opposite of what we assert on, out of paranoia. https://codereview.chromium.org/849903002/diff/80001/components/cronet/androi... File components/cronet/android/test/upload_data_stream_handler.cc (right): https://codereview.chromium.org/849903002/diff/80001/components/cronet/androi... components/cronet/android/test/upload_data_stream_handler.cc:32: bool UploadDataStreamHandlerRegisterJni(JNIEnv* env) { Declaration order should match definition order - this should be at the bottom of the file. https://codereview.chromium.org/849903002/diff/80001/components/cronet/androi... components/cronet/android/test/upload_data_stream_handler.cc:98: network_thread_ = new base::Thread("network"); Problem: Once we start mucking with weak references on some thread, the weak factory can only be used (And deleted) on that thread. But this class also owns the network thread, and threads can't be deleted from a method called on that thread. This makes clean deletion a bit of a problem. Currently, it looks like we aren't cleaning things up. One way to do that is a method that moves network_thread_ into a local scoped_ptr, posts a task to delete the Handler on that thread, and then deletes the method. There are a couple alternatives: The simplest, and the one I recommend: Get rid of the weak factory, and just use base::Unretained(this). Since the handler owns the network thread, which is the only place we can dereference the weak pointers anyways, this should be safe. https://codereview.chromium.org/849903002/diff/80001/components/cronet/androi... components/cronet/android/test/upload_data_stream_handler.cc:110: // On data provider's executor. Method level comments should go above the method declarations, not their definitions. https://codereview.chromium.org/849903002/diff/80001/components/cronet/androi... components/cronet/android/test/upload_data_stream_handler.cc:125: init_complete_ = false; This shouldn't be touched on the IO thread. May make life simpler/safer to make this purely a network thread class, with the possible exception of grabbing the network thread and destruction. https://codereview.chromium.org/849903002/diff/80001/components/cronet/androi... components/cronet/android/test/upload_data_stream_handler.cc:142: read_complete_ = false; This shouldn't be touched on the IO thread https://codereview.chromium.org/849903002/diff/80001/components/cronet/androi... components/cronet/android/test/upload_data_stream_handler.cc:145: weak_factory_.GetWeakPtr())); The GetWeakPtr calls aren't safe. According to the WeakPtrFactory docs, once a weak pointer is dereferenced, the factory and the weak pointers can only be dereferenced on that thread. So you need to grab a weak pointer once, and then just use it for all these calls. https://codereview.chromium.org/849903002/diff/80001/components/cronet/androi... components/cronet/android/test/upload_data_stream_handler.cc:181: void UploadDataStreamHandler::CheckIfInitCallbackInvokedOnNetworkThread() { DCHECK(GetTaskRunner()->BelongsToCurrentThread()); https://codereview.chromium.org/849903002/diff/80001/components/cronet/androi... components/cronet/android/test/upload_data_stream_handler.cc:191: void UploadDataStreamHandler::CheckIfReadCallbackInvokedOnNetworkThread() { DCHECK(GetTaskRunner()->BelongsToCurrentThread()); https://codereview.chromium.org/849903002/diff/80001/components/cronet/androi... components/cronet/android/test/upload_data_stream_handler.cc:195: std::string UploadDataStreamHandler::Data() const { DCHECK(GetTaskRunner()->BelongsToCurrentThread()); https://codereview.chromium.org/849903002/diff/80001/components/cronet/androi... components/cronet/android/test/upload_data_stream_handler.cc:239: return base::android::ConvertUTF8ToJavaString(env, handler->Data()).Release(); This doesn't seem threadsafe. Should probably pass it along in OnReadComplete https://codereview.chromium.org/849903002/diff/80001/components/cronet/androi... File components/cronet/android/test/upload_data_stream_handler.h (right): https://codereview.chromium.org/849903002/diff/80001/components/cronet/androi... components/cronet/android/test/upload_data_stream_handler.h:20: class UploadDataStreamHandler { This doesn't need to be in the header. Can be in an anonymous namespace in the cc file. https://codereview.chromium.org/849903002/diff/80001/components/cronet/androi... components/cronet/android/test/upload_data_stream_handler.h:22: class Delegate { You don't need both this and UploadDataStreamHandlerDelegate. Since there's only one implementation, and both are in the same file, separating them doesn't get us anything. Actually, beyond that, I don't think we really need the Delegate - the methods are simple enough that I suggest just inlining them.
Patchset #2 (id:100001) has been deleted
Matt, thanks for the detailed review! Glad that this what you had in mind. I think I understand a little better now. I uploaded a new patch and did some code cleanup. PTAL. Thanks! https://codereview.chromium.org/849903002/diff/80001/components/cronet/androi... File components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/CronetUploadTest.java (right): https://codereview.chromium.org/849903002/diff/80001/components/cronet/androi... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/CronetUploadTest.java:28: mHandler = new UploadDataStreamHandler(mUploadDataStream.createAdapter()); On 2015/01/21 18:48:58, mmenke wrote: > Need to destroy the handler's C-side stuff in tearDown, too (That should take > care of cleaning up the uploadDataStream, too, or at least should post a task to > the executor do it). Done. https://codereview.chromium.org/849903002/diff/80001/components/cronet/androi... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/CronetUploadTest.java:34: assertTrue(mHandler.init()); // make sure init is synchronous. On 2015/01/21 18:48:58, mmenke wrote: > These should be capitalized. Also suggest to put comments above the line > they're commenting on, in most cases. It's more common in Chrome. Done. https://codereview.chromium.org/849903002/diff/80001/components/cronet/androi... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/CronetUploadTest.java:38: assertFalse(mHandler.checkIfReadCallbackInvoked()); On 2015/01/21 18:48:58, mmenke wrote: > Suggest some line breaks in here to make these easier to read. Maybe after the > last assert of each chain (Do stuff, assert stuff, line break, etc). Done. https://codereview.chromium.org/849903002/diff/80001/components/cronet/androi... File components/cronet/android/test/src/org/chromium/cronet_test_apk/UploadDataStreamHandler.java (right): https://codereview.chromium.org/849903002/diff/80001/components/cronet/androi... components/cronet/android/test/src/org/chromium/cronet_test_apk/UploadDataStreamHandler.java:60: public boolean checkIfInitCallbackInvoked() { On 2015/01/21 18:48:58, mmenke wrote: > Suggest making this and the next method voids, and calling them > checkInitCallbackNotInvoked (Or Pending or NotComplete - should also use similar > names in the CC file - read_complete_ / init_complete_ should have similar names > to whatever this is called), and do the asserting here. They're only intended > for use to check if something hasn't happened yet. If we want to wait for > something to actually happen, there are other methods to use for that. Done. https://codereview.chromium.org/849903002/diff/80001/components/cronet/androi... components/cronet/android/test/src/org/chromium/cronet_test_apk/UploadDataStreamHandler.java:62: mCheckInit = false; On 2015/01/21 18:48:58, mmenke wrote: > This should be initialized to the opposite of what we assert on, out of > paranoia. Done. https://codereview.chromium.org/849903002/diff/80001/components/cronet/androi... File components/cronet/android/test/upload_data_stream_handler.cc (right): https://codereview.chromium.org/849903002/diff/80001/components/cronet/androi... components/cronet/android/test/upload_data_stream_handler.cc:32: bool UploadDataStreamHandlerRegisterJni(JNIEnv* env) { On 2015/01/21 18:48:58, mmenke wrote: > Declaration order should match definition order - this should be at the bottom > of the file. Done. https://codereview.chromium.org/849903002/diff/80001/components/cronet/androi... components/cronet/android/test/upload_data_stream_handler.cc:98: network_thread_ = new base::Thread("network"); On 2015/01/21 18:48:58, mmenke wrote: > Problem: Once we start mucking with weak references on some thread, the weak > factory can only be used (And deleted) on that thread. But this class also owns > the network thread, and threads can't be deleted from a method called on that > thread. > > This makes clean deletion a bit of a problem. Currently, it looks like we > aren't cleaning things up. One way to do that is a method that moves > network_thread_ into a local scoped_ptr, posts a task to delete the Handler on > that thread, and then deletes the method. > > > There are a couple alternatives: The simplest, and the one I recommend: Get > rid of the weak factory, and just use base::Unretained(this). Since the handler > owns the network thread, which is the only place we can dereference the weak > pointers anyways, this should be safe. Got it. Thanks for the detailed explanation! https://codereview.chromium.org/849903002/diff/80001/components/cronet/androi... components/cronet/android/test/upload_data_stream_handler.cc:110: // On data provider's executor. On 2015/01/21 18:48:58, mmenke wrote: > Method level comments should go above the method declarations, not their > definitions. Done. Right, haven't cleaned this up. For my own reference mostly, as I often lose track about which method is called by whom :) https://codereview.chromium.org/849903002/diff/80001/components/cronet/androi... components/cronet/android/test/upload_data_stream_handler.cc:125: init_complete_ = false; On 2015/01/21 18:48:58, mmenke wrote: > This shouldn't be touched on the IO thread. May make life simpler/safer to make > this purely a network thread class, with the possible exception of grabbing the > network thread and destruction. Done. Agreed! https://codereview.chromium.org/849903002/diff/80001/components/cronet/androi... components/cronet/android/test/upload_data_stream_handler.cc:142: read_complete_ = false; On 2015/01/21 18:48:58, mmenke wrote: > This shouldn't be touched on the IO thread Done. https://codereview.chromium.org/849903002/diff/80001/components/cronet/androi... components/cronet/android/test/upload_data_stream_handler.cc:145: weak_factory_.GetWeakPtr())); On 2015/01/21 18:48:58, mmenke wrote: > The GetWeakPtr calls aren't safe. According to the WeakPtrFactory docs, once a > weak pointer is dereferenced, the factory and the weak pointers can only be > dereferenced on that thread. So you need to grab a weak pointer once, and then > just use it for all these calls. Ahh I see. That's how it is used. Thanks! https://codereview.chromium.org/849903002/diff/80001/components/cronet/androi... components/cronet/android/test/upload_data_stream_handler.cc:181: void UploadDataStreamHandler::CheckIfInitCallbackInvokedOnNetworkThread() { On 2015/01/21 18:48:58, mmenke wrote: > DCHECK(GetTaskRunner()->BelongsToCurrentThread()); Done. https://codereview.chromium.org/849903002/diff/80001/components/cronet/androi... components/cronet/android/test/upload_data_stream_handler.cc:191: void UploadDataStreamHandler::CheckIfReadCallbackInvokedOnNetworkThread() { On 2015/01/21 18:48:58, mmenke wrote: > DCHECK(GetTaskRunner()->BelongsToCurrentThread()); Done. https://codereview.chromium.org/849903002/diff/80001/components/cronet/androi... components/cronet/android/test/upload_data_stream_handler.cc:195: std::string UploadDataStreamHandler::Data() const { On 2015/01/21 18:48:58, mmenke wrote: > DCHECK(GetTaskRunner()->BelongsToCurrentThread()); Done. https://codereview.chromium.org/849903002/diff/80001/components/cronet/androi... components/cronet/android/test/upload_data_stream_handler.cc:239: return base::android::ConvertUTF8ToJavaString(env, handler->Data()).Release(); On 2015/01/21 18:48:58, mmenke wrote: > This doesn't seem threadsafe. Should probably pass it along in OnReadComplete Done. You are right! this is not thread-safe. https://codereview.chromium.org/849903002/diff/80001/components/cronet/androi... File components/cronet/android/test/upload_data_stream_handler.h (right): https://codereview.chromium.org/849903002/diff/80001/components/cronet/androi... components/cronet/android/test/upload_data_stream_handler.h:20: class UploadDataStreamHandler { On 2015/01/21 18:48:58, mmenke wrote: > This doesn't need to be in the header. Can be in an anonymous namespace in the > cc file. Done. https://codereview.chromium.org/849903002/diff/80001/components/cronet/androi... components/cronet/android/test/upload_data_stream_handler.h:22: class Delegate { On 2015/01/21 18:48:58, mmenke wrote: > You don't need both this and UploadDataStreamHandlerDelegate. Since there's > only one implementation, and both are in the same file, separating them doesn't > get us anything. > > Actually, beyond that, I don't think we really need the Delegate - the methods > are simple enough that I suggest just inlining them. Done. Good idea! I have inlined the methods.
Patchset #2 (id:120001) has been deleted
Still reviewing, but I thought I'd send you what I have, in case I don't have time to get back to this today. https://codereview.chromium.org/849903002/diff/140001/components/cronet/andro... File components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/CronetUploadTest.java (right): https://codereview.chromium.org/849903002/diff/140001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/CronetUploadTest.java:60: assertEquals(0, mDataProvider.getNumRewindCalls()); Suggesting the number of read calls, too (Especially at the end of tests - we don't have any checks for that). https://codereview.chromium.org/849903002/diff/140001/components/cronet/andro... File components/cronet/android/test/upload_data_stream_handler.cc (right): https://codereview.chromium.org/849903002/diff/140001/components/cronet/andro... components/cronet/android/test/upload_data_stream_handler.cc:1: // Copyright 2014 The Chromium Authors. All rights reserved. 2015, for all of these files (No idea if you should keep the 2014 on the files I handed off to you or not) https://codereview.chromium.org/849903002/diff/140001/components/cronet/andro... components/cronet/android/test/upload_data_stream_handler.cc:29: class UploadDataStreamHandler { Should have a description of this class. https://codereview.chromium.org/849903002/diff/140001/components/cronet/andro... components/cronet/android/test/upload_data_stream_handler.cc:37: void Destroy(); Suggest separating this from the other methods with a blank line, and giving it its own comment. https://codereview.chromium.org/849903002/diff/140001/components/cronet/andro... components/cronet/android/test/upload_data_stream_handler.cc:38: void Init(); Suggest a comment before this block of methods, and one before the second block, telling when/where they're called...Or may even want to comment them individually. https://codereview.chromium.org/849903002/diff/140001/components/cronet/andro... components/cronet/android/test/upload_data_stream_handler.cc:41: scoped_refptr<base::SingleThreadTaskRunner> GetTaskRunner() const; I don't think this is needed - can just access the task runner directly everywhere. https://codereview.chromium.org/849903002/diff/140001/components/cronet/andro... components/cronet/android/test/upload_data_stream_handler.cc:43: void CheckReadCallbackNotInvoked(); Declaration order should match definition order. https://codereview.chromium.org/849903002/diff/140001/components/cronet/andro... components/cronet/android/test/upload_data_stream_handler.cc:57: int bytes_read_; Should include descriptions of these (When set to true/false, in particular, and where they live. Same goes for the other member variables) https://codereview.chromium.org/849903002/diff/140001/components/cronet/andro... components/cronet/android/test/upload_data_stream_handler.cc:59: base::Thread* network_thread_; Should mention lifetime / creation / destruction of this object, in particular. https://codereview.chromium.org/849903002/diff/140001/components/cronet/andro... components/cronet/android/test/upload_data_stream_handler.cc:67: } // namespace This namespace should extend down to the last method of UploadDataStreamHandler. https://codereview.chromium.org/849903002/diff/140001/components/cronet/andro... components/cronet/android/test/upload_data_stream_handler.cc:77: network_thread_ = new base::Thread("network"); nit: Think this can be in the initializer list. https://codereview.chromium.org/849903002/diff/140001/components/cronet/andro... components/cronet/android/test/upload_data_stream_handler.cc:90: // network thread before delete network_thread is called. nit: "as" -> "so", "before delete network_thread is called." -> "before the network thread is destroyed." https://codereview.chromium.org/849903002/diff/140001/components/cronet/andro... components/cronet/android/test/upload_data_stream_handler.cc:91: base::Thread* network_thread = network_thread_; Ownership is a bit weird, but I suggest making both of these scoped_ptrs, and using Pass here. https://codereview.chromium.org/849903002/diff/140001/components/cronet/andro... components/cronet/android/test/upload_data_stream_handler.cc:112: base::android::ConvertUTF8ToJavaString(env, this->Data()).Release()); This callback is idental to the one in ReadOnNetworkThread. Suggest a common subroutine (Also suggest getting rid of the Data() method and just inlining the code to get it) https://codereview.chromium.org/849903002/diff/140001/components/cronet/andro... components/cronet/android/test/upload_data_stream_handler.cc:125: read_buffer_ = NULL; nit: nullptr https://codereview.chromium.org/849903002/diff/140001/components/cronet/andro... components/cronet/android/test/upload_data_stream_handler.cc:270: static void Destroy(JNIEnv* env, This should probably be after CreateUploadDataStreamHandler.
Thanks for the review! Sending the comments in batches work for me :) https://codereview.chromium.org/849903002/diff/140001/components/cronet/andro... File components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/CronetUploadTest.java (right): https://codereview.chromium.org/849903002/diff/140001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/CronetUploadTest.java:60: assertEquals(0, mDataProvider.getNumRewindCalls()); On 2015/01/22 20:19:07, mmenke wrote: > Suggesting the number of read calls, too (Especially at the end of tests - we > don't have any checks for that). Done. https://codereview.chromium.org/849903002/diff/140001/components/cronet/andro... File components/cronet/android/test/upload_data_stream_handler.cc (right): https://codereview.chromium.org/849903002/diff/140001/components/cronet/andro... components/cronet/android/test/upload_data_stream_handler.cc:1: // Copyright 2014 The Chromium Authors. All rights reserved. On 2015/01/22 20:19:07, mmenke wrote: > 2015, for all of these files (No idea if you should keep the 2014 on the files I > handed off to you or not) Done. 2015 it is! :) https://codereview.chromium.org/849903002/diff/140001/components/cronet/andro... components/cronet/android/test/upload_data_stream_handler.cc:29: class UploadDataStreamHandler { On 2015/01/22 20:19:07, mmenke wrote: > Should have a description of this class. Done. https://codereview.chromium.org/849903002/diff/140001/components/cronet/andro... components/cronet/android/test/upload_data_stream_handler.cc:37: void Destroy(); On 2015/01/22 20:19:08, mmenke wrote: > Suggest separating this from the other methods with a blank line, and giving it > its own comment. Done. https://codereview.chromium.org/849903002/diff/140001/components/cronet/andro... components/cronet/android/test/upload_data_stream_handler.cc:38: void Init(); On 2015/01/22 20:19:07, mmenke wrote: > Suggest a comment before this block of methods, and one before the second block, > telling when/where they're called...Or may even want to comment them > individually. Done. https://codereview.chromium.org/849903002/diff/140001/components/cronet/andro... components/cronet/android/test/upload_data_stream_handler.cc:41: scoped_refptr<base::SingleThreadTaskRunner> GetTaskRunner() const; On 2015/01/22 20:19:07, mmenke wrote: > I don't think this is needed - can just access the task runner directly > everywhere. Done. https://codereview.chromium.org/849903002/diff/140001/components/cronet/andro... components/cronet/android/test/upload_data_stream_handler.cc:43: void CheckReadCallbackNotInvoked(); On 2015/01/22 20:19:07, mmenke wrote: > Declaration order should match definition order. Done. https://codereview.chromium.org/849903002/diff/140001/components/cronet/andro... components/cronet/android/test/upload_data_stream_handler.cc:57: int bytes_read_; On 2015/01/22 20:19:08, mmenke wrote: > Should include descriptions of these (When set to true/false, in particular, and > where they live. Same goes for the other member variables) Done. https://codereview.chromium.org/849903002/diff/140001/components/cronet/andro... components/cronet/android/test/upload_data_stream_handler.cc:57: int bytes_read_; On 2015/01/22 20:19:08, mmenke wrote: > Should include descriptions of these (When set to true/false, in particular, and > where they live. Same goes for the other member variables) Done. https://codereview.chromium.org/849903002/diff/140001/components/cronet/andro... components/cronet/android/test/upload_data_stream_handler.cc:59: base::Thread* network_thread_; On 2015/01/22 20:19:07, mmenke wrote: > Should mention lifetime / creation / destruction of this object, in particular. Done. https://codereview.chromium.org/849903002/diff/140001/components/cronet/andro... components/cronet/android/test/upload_data_stream_handler.cc:67: } // namespace On 2015/01/22 20:19:08, mmenke wrote: > This namespace should extend down to the last method of UploadDataStreamHandler. Done. https://codereview.chromium.org/849903002/diff/140001/components/cronet/andro... components/cronet/android/test/upload_data_stream_handler.cc:77: network_thread_ = new base::Thread("network"); On 2015/01/22 20:19:07, mmenke wrote: > nit: Think this can be in the initializer list. Done. https://codereview.chromium.org/849903002/diff/140001/components/cronet/andro... components/cronet/android/test/upload_data_stream_handler.cc:90: // network thread before delete network_thread is called. On 2015/01/22 20:19:07, mmenke wrote: > nit: "as" -> "so", "before delete network_thread is called." -> "before the > network thread is destroyed." Done. https://codereview.chromium.org/849903002/diff/140001/components/cronet/andro... components/cronet/android/test/upload_data_stream_handler.cc:91: base::Thread* network_thread = network_thread_; On 2015/01/22 20:19:07, mmenke wrote: > Ownership is a bit weird, but I suggest making both of these scoped_ptrs, and > using Pass here. Done. Copied from other code from elsewhere. Why do we need to delete |this| on network thread? https://codereview.chromium.org/849903002/diff/140001/components/cronet/andro... components/cronet/android/test/upload_data_stream_handler.cc:112: base::android::ConvertUTF8ToJavaString(env, this->Data()).Release()); On 2015/01/22 20:19:07, mmenke wrote: > This callback is idental to the one in ReadOnNetworkThread. Suggest a common > subroutine (Also suggest getting rid of the Data() method and just inlining the > code to get it) Done. https://codereview.chromium.org/849903002/diff/140001/components/cronet/andro... components/cronet/android/test/upload_data_stream_handler.cc:125: read_buffer_ = NULL; On 2015/01/22 20:19:08, mmenke wrote: > nit: nullptr Done. https://codereview.chromium.org/849903002/diff/140001/components/cronet/andro... components/cronet/android/test/upload_data_stream_handler.cc:270: static void Destroy(JNIEnv* env, On 2015/01/22 20:19:07, mmenke wrote: > This should probably be after CreateUploadDataStreamHandler. Done.
This is looking really good. https://codereview.chromium.org/849903002/diff/140001/components/cronet/andro... File components/cronet/android/test/upload_data_stream_handler.cc (right): https://codereview.chromium.org/849903002/diff/140001/components/cronet/andro... components/cronet/android/test/upload_data_stream_handler.cc:91: base::Thread* network_thread = network_thread_; On 2015/01/23 16:39:48, xunjieli wrote: > On 2015/01/22 20:19:07, mmenke wrote: > > Ownership is a bit weird, but I suggest making both of these scoped_ptrs, and > > using Pass here. > > Done. Copied from other code from elsewhere. Why do we need to delete |this| on > network thread? Weak pointer factories must be destroyed on the thread the weak pointer is bound to. The handler no longer has a weak pointer itself, but the CronetUploadDataStream[Adapter?] has one, for callbacks from the Java side. In general, it's safest to only use objects from a single thread, and destroy them on it, too. We could even create an object owned by Java, and have it have own the network thread and have a pointer to the handle, and then never touch the network thread (Other than passing around its pointer and creating it) on the Java thread, if we wanted to be even more strict about that. Doesn't seem worth the effort, in this case, but it's a common pattern. https://codereview.chromium.org/849903002/diff/160001/components/cronet/andro... File components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/CronetUploadTest.java (right): https://codereview.chromium.org/849903002/diff/160001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/CronetUploadTest.java:69: assertTrue(mDataProvider.checkIfRewindPending()); I don't think we need this "assertTrue(mDataProvider.checkIfRewindPending())" or the other two, either, as the "waitForRewindRequest" just above should be sufficient. In fact, I suggest that we replace "mDataProvider.checkIfRewindPending()" with "mDataProvider.assertRewindNotPending()". assertTrue(checkIfRewindPending) is currently flaky, *unless* it's called after a waitForRewindRequest() anyways, which ensures a rewind is pending. We also don't have to worry about a rewind becoming no longer pending without calling into the data provider. I suggest the same for reads as well. This is just defensive programming against new tests being flaky. https://codereview.chromium.org/849903002/diff/160001/components/cronet/andro... File components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/TestDrivenDataProvider.java (right): https://codereview.chromium.org/849903002/diff/160001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/TestDrivenDataProvider.java:25: private boolean mRewindPending = false; Should mention that mReadPending and mRewindPending are only accessed on the executor thread (Or if you take my suggestion below, while holding a lock) https://codereview.chromium.org/849903002/diff/160001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/TestDrivenDataProvider.java:33: mWaitForRewindRequest = new ConditionVariable(); nit: Can just inline the initialization of the ConditionVariables, and make them final. https://codereview.chromium.org/849903002/diff/160001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/TestDrivenDataProvider.java:39: public void addRead(byte[] read) { All of the tests that use this class only use a single read...Suggest just baking that in to the class, and making it a public constant. Can also get rid of mStarted, then, too. https://codereview.chromium.org/849903002/diff/160001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/TestDrivenDataProvider.java:83: private boolean mRewindPendingCopy = false; Think all member variables should go at the top, for Java files...Or at least that's what we seem to be doing in CroNet. Actually, maybe just protect access to mRewindPending and mReadPending with a lock, and just thread hop and back, and then read the value? https://codereview.chromium.org/849903002/diff/160001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/TestDrivenDataProvider.java:85: // Called on test thread. Check happens on executor thread. Think it's worth mentioning that thread hopping here, while not strictly necessary, is more likely to come out on the wrong side of races than just using a lock. https://codereview.chromium.org/849903002/diff/160001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/TestDrivenDataProvider.java:136: mReadPending = false; I'd like to fail if mReadPending isn't true, and the same in the rewind case. This protects against duplicate callbacks. https://codereview.chromium.org/849903002/diff/160001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/TestDrivenDataProvider.java:143: public void rewind(final UploadDataSink uploadDataSink) { Think this class is complicated enough that we should annotate all methods as being one of: 1) Called by test fixture on main thread (You've annotated most of these already). 2) Called by UploadDataSink on executor thread. 3) Called by UploadDataStreamHandler on network thread. Also, should 3) have @CalledByNative annotations?
Thanks for the review! I have changed the code to protect access to mReadPending/mRewindPending using a lock. Not sure if I've put the defensive checks in the right places though. PTAL. thanks! https://codereview.chromium.org/849903002/diff/160001/components/cronet/andro... File components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/CronetUploadTest.java (right): https://codereview.chromium.org/849903002/diff/160001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/CronetUploadTest.java:69: assertTrue(mDataProvider.checkIfRewindPending()); On 2015/01/23 21:31:41, mmenke wrote: > I don't think we need this "assertTrue(mDataProvider.checkIfRewindPending())" or > the other two, either, as the "waitForRewindRequest" just above should be > sufficient. > > In fact, I suggest that we replace "mDataProvider.checkIfRewindPending()" with > "mDataProvider.assertRewindNotPending()". assertTrue(checkIfRewindPending) is > currently flaky, *unless* it's called after a waitForRewindRequest() anyways, > which ensures a rewind is pending. We also don't have to worry about a rewind > becoming no longer pending without calling into the data provider. > > I suggest the same for reads as well. This is just defensive programming > against new tests being flaky. Done. Makes sense. Thank you! https://codereview.chromium.org/849903002/diff/160001/components/cronet/andro... File components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/TestDrivenDataProvider.java (right): https://codereview.chromium.org/849903002/diff/160001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/TestDrivenDataProvider.java:25: private boolean mRewindPending = false; On 2015/01/23 21:31:42, mmenke wrote: > Should mention that mReadPending and mRewindPending are only accessed on the > executor thread (Or if you take my suggestion below, while holding a lock) Done. https://codereview.chromium.org/849903002/diff/160001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/TestDrivenDataProvider.java:33: mWaitForRewindRequest = new ConditionVariable(); On 2015/01/23 21:31:42, mmenke wrote: > nit: Can just inline the initialization of the ConditionVariables, and make > them final. Done. https://codereview.chromium.org/849903002/diff/160001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/TestDrivenDataProvider.java:39: public void addRead(byte[] read) { On 2015/01/23 21:31:42, mmenke wrote: > All of the tests that use this class only use a single read...Suggest just > baking that in to the class, and making it a public constant. Can also get rid > of mStarted, then, too. Done. https://codereview.chromium.org/849903002/diff/160001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/TestDrivenDataProvider.java:83: private boolean mRewindPendingCopy = false; On 2015/01/23 21:31:41, mmenke wrote: > Think all member variables should go at the top, for Java files...Or at least > that's what we seem to be doing in CroNet. > > Actually, maybe just protect access to mRewindPending and mReadPending with a > lock, and just thread hop and back, and then read the value? Done. Good idea! thanks https://codereview.chromium.org/849903002/diff/160001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/TestDrivenDataProvider.java:85: // Called on test thread. Check happens on executor thread. On 2015/01/23 21:31:42, mmenke wrote: > Think it's worth mentioning that thread hopping here, while not strictly > necessary, is more likely to come out on the wrong side of races than just using > a lock. Done. https://codereview.chromium.org/849903002/diff/160001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/TestDrivenDataProvider.java:136: mReadPending = false; On 2015/01/23 21:31:42, mmenke wrote: > I'd like to fail if mReadPending isn't true, and the same in the rewind case. > This protects against duplicate callbacks. Done. https://codereview.chromium.org/849903002/diff/160001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/TestDrivenDataProvider.java:143: public void rewind(final UploadDataSink uploadDataSink) { On 2015/01/23 21:31:41, mmenke wrote: > Think this class is complicated enough that we should annotate all methods as > being one of: > > 1) Called by test fixture on main thread (You've annotated most of these > already). > 2) Called by UploadDataSink on executor thread. > 3) Called by UploadDataStreamHandler on network thread. > > Also, should 3) have @CalledByNative annotations? Hmm.. there're no 3) cases. 3) should all be in UploadDataStreamHandler.java. I have annotated the rest.
Pinging for review. Seems Paul's GMS core integration is blocked on this. Matt could you take a look? Should I combine this with your CL?
On 2015/02/02 15:39:08, xunjieli wrote: > Pinging for review. Seems Paul's GMS core integration is blocked on this. Matt > could you take a look? Should I combine this with your CL? Yes, please combine this with my CL. There's one thing I may want changed on my CL, which is maybe a 2 or 3 line change. I'll want to take another look over the whole thing, but think I can sign off on it pretty promptly, though should also get Misha's signoff for this one.
On 2015/02/02 15:48:26, mmenke wrote: > On 2015/02/02 15:39:08, xunjieli wrote: > > Pinging for review. Seems Paul's GMS core integration is blocked on this. Matt > > could you take a look? Should I combine this with your CL? > > Yes, please combine this with my CL. There's one thing I may want changed on my > CL, which is maybe a 2 or 3 line change. I'll want to take another look over > the whole thing, but think I can sign off on it pretty promptly, though should > also get Misha's signoff for this one. I'll take a look at this ASAP, but it would help if I could also see Matt's CL to understand what are we testing.
Patchset #5 (id:200001) has been deleted
Patchset #5 (id:220001) has been deleted
On 2015/02/02 16:01:34, mef wrote: > On 2015/02/02 15:48:26, mmenke wrote: > > On 2015/02/02 15:39:08, xunjieli wrote: > > > Pinging for review. Seems Paul's GMS core integration is blocked on this. > Matt > > > could you take a look? Should I combine this with your CL? > > > > Yes, please combine this with my CL. There's one thing I may want changed on > my > > CL, which is maybe a 2 or 3 line change. I'll want to take another look over > > the whole thing, but think I can sign off on it pretty promptly, though should > > also get Misha's signoff for this one. > > I'll take a look at this ASAP, but it would help if I could also see Matt's CL > to understand what are we testing. Sounds good. Combined with Matt's CL. PTAL. Thanks!
Happy reviewing! looks like there's a lot of code. https://codereview.chromium.org/849903002/diff/240001/components/cronet/andro... File components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/CronetUploadTest.java (right): https://codereview.chromium.org/849903002/diff/240001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/CronetUploadTest.java:214: mDataProvider.assertReadNotPending(); There is a race condition here. assertReadNotPending() might get called before onReadSucceeded task is executed on the executor. I think I will just remove line 214, so test is not flaky.
pauljensen@chromium.org changed reviewers: + pauljensen@chromium.org
https://codereview.chromium.org/849903002/diff/240001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java (right): https://codereview.chromium.org/849903002/diff/240001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java:289: if (header.first == "Content-Type" && !header.second.isEmpty()) { Should use equalsIgnoreCase() to compare header names. https://codereview.chromium.org/849903002/diff/240001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java:301: "Requests with body must have a Content-Type."); Certain HTTP stacks (e.g. OkHttp) default to application/x-www-form-urlencoded, as the HTML spec states for POSTs. We may want to do that, though perhaps at the HttpURLConnection level.
https://codereview.chromium.org/849903002/diff/240001/components/cronet/andro... File components/cronet/android/cronet_upload_data_stream.cc (right): https://codereview.chromium.org/849903002/diff/240001/components/cronet/andro... components/cronet/android/cronet_upload_data_stream.cc:182: static jlong CreateDelegate(JNIEnv* env, jobject jupload_data_stream) { nit: Should this be called 'CreateDelegateForTesting'? https://codereview.chromium.org/849903002/diff/240001/components/cronet/andro... components/cronet/android/cronet_upload_data_stream.cc:189: static jlong CreateAdapter(JNIEnv* env, nit: Should this be called 'CreateAdapterForTesting'? https://codereview.chromium.org/849903002/diff/240001/components/cronet/andro... File components/cronet/android/cronet_url_request_adapter.cc (right): https://codereview.chromium.org/849903002/diff/240001/components/cronet/andro... components/cronet/android/cronet_url_request_adapter.cc:63: net::UploadDataStream* CronetURLRequestAdapter::GetUpload() { nit: GetUploadForTesting? https://codereview.chromium.org/849903002/diff/240001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java (right): https://codereview.chromium.org/849903002/diff/240001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java:298: if (mUploadDataStream != null) { Would it make sense to make Content-Type an argument of setUploadDataProvider? https://codereview.chromium.org/849903002/diff/240001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java:305: } catch (RuntimeException e) { should this be just Exception? https://codereview.chromium.org/849903002/diff/240001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java:447: /** empty comment? https://codereview.chromium.org/849903002/diff/240001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/UploadDataProvider.java (right): https://codereview.chromium.org/849903002/diff/240001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/UploadDataProvider.java:41: * Reads upload data into byteBuffer. Each call must be followed be a single copy/paste comment? https://codereview.chromium.org/849903002/diff/240001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/UploadDataSink.java (right): https://codereview.chromium.org/849903002/diff/240001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/UploadDataSink.java:14: * @param done For chunked uploads, true if this is the final read. Ignored @param finalChunk https://codereview.chromium.org/849903002/diff/240001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/UploadDataSink.java:31: * Called by UploadDataProvider when a rewind fails. or if rewiding uploads sp: rewinding https://codereview.chromium.org/849903002/diff/240001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/UrlRequest.java (right): https://codereview.chromium.org/849903002/diff/240001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/UrlRequest.java:52: * set. Starting the request will throw an exception it a content-type is if a Content-Type header.
https://codereview.chromium.org/849903002/diff/240001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/UploadDataSink.java (right): https://codereview.chromium.org/849903002/diff/240001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/UploadDataSink.java:15: * for non-chunked uploads. This is not ignored for non-chunked uploads, it instead throws an exception if true.
Thanks for the review! PTAL https://codereview.chromium.org/849903002/diff/240001/components/cronet/andro... File components/cronet/android/cronet_upload_data_stream.cc (right): https://codereview.chromium.org/849903002/diff/240001/components/cronet/andro... components/cronet/android/cronet_upload_data_stream.cc:182: static jlong CreateDelegate(JNIEnv* env, jobject jupload_data_stream) { On 2015/02/02 17:45:10, mef wrote: > nit: Should this be called 'CreateDelegateForTesting'? Done. Good idea thanks! https://codereview.chromium.org/849903002/diff/240001/components/cronet/andro... components/cronet/android/cronet_upload_data_stream.cc:189: static jlong CreateAdapter(JNIEnv* env, On 2015/02/02 17:45:10, mef wrote: > nit: Should this be called 'CreateAdapterForTesting'? Done. https://codereview.chromium.org/849903002/diff/240001/components/cronet/andro... File components/cronet/android/cronet_url_request_adapter.cc (right): https://codereview.chromium.org/849903002/diff/240001/components/cronet/andro... components/cronet/android/cronet_url_request_adapter.cc:63: net::UploadDataStream* CronetURLRequestAdapter::GetUpload() { On 2015/02/02 17:45:11, mef wrote: > nit: GetUploadForTesting? Done. https://codereview.chromium.org/849903002/diff/240001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java (right): https://codereview.chromium.org/849903002/diff/240001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java:289: if (header.first == "Content-Type" && !header.second.isEmpty()) { On 2015/02/02 17:40:29, pauljensen wrote: > Should use equalsIgnoreCase() to compare header names. Done. https://codereview.chromium.org/849903002/diff/240001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java:301: "Requests with body must have a Content-Type."); On 2015/02/02 17:40:29, pauljensen wrote: > Certain HTTP stacks (e.g. OkHttp) default to application/x-www-form-urlencoded, > as the HTML spec states for POSTs. We may want to do that, though perhaps at > the HttpURLConnection level. Thanks for the heads up! Will keep this in mind. After the basic support is in, I will work on it. https://codereview.chromium.org/849903002/diff/240001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java:447: /** On 2015/02/02 17:45:11, mef wrote: > empty comment? Done. https://codereview.chromium.org/849903002/diff/240001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/UploadDataProvider.java (right): https://codereview.chromium.org/849903002/diff/240001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/UploadDataProvider.java:41: * Reads upload data into byteBuffer. Each call must be followed be a single On 2015/02/02 17:45:11, mef wrote: > copy/paste comment? Done. https://codereview.chromium.org/849903002/diff/240001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/UploadDataSink.java (right): https://codereview.chromium.org/849903002/diff/240001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/UploadDataSink.java:14: * @param done For chunked uploads, true if this is the final read. Ignored On 2015/02/02 17:45:11, mef wrote: > @param finalChunk Done. https://codereview.chromium.org/849903002/diff/240001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/UploadDataSink.java:15: * for non-chunked uploads. On 2015/02/02 18:11:24, pauljensen wrote: > This is not ignored for non-chunked uploads, it instead throws an exception if > true. Done. https://codereview.chromium.org/849903002/diff/240001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/UploadDataSink.java:31: * Called by UploadDataProvider when a rewind fails. or if rewiding uploads On 2015/02/02 17:45:11, mef wrote: > sp: rewinding Done. https://codereview.chromium.org/849903002/diff/240001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/UrlRequest.java (right): https://codereview.chromium.org/849903002/diff/240001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/UrlRequest.java:52: * set. Starting the request will throw an exception it a content-type is On 2015/02/02 17:45:11, mef wrote: > if a Content-Type header. Done. https://codereview.chromium.org/849903002/diff/240001/components/cronet/andro... File components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/CronetUploadTest.java (right): https://codereview.chromium.org/849903002/diff/240001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/CronetUploadTest.java:214: mDataProvider.assertReadNotPending(); On 2015/02/02 17:06:01, xunjieli wrote: > There is a race condition here. assertReadNotPending() might get called before > onReadSucceeded task is executed on the executor. I think I will just remove > line 214, so test is not flaky. Done.
https://codereview.chromium.org/849903002/diff/240001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java (right): https://codereview.chromium.org/849903002/diff/240001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java:298: if (mUploadDataStream != null) { On 2015/02/02 17:45:11, mef wrote: > Would it make sense to make Content-Type an argument of setUploadDataProvider? The problem with this is if the header is also set manually, you have two ways to set a header duking it out. https://codereview.chromium.org/849903002/diff/240001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java:305: } catch (RuntimeException e) { On 2015/02/02 17:45:11, mef wrote: > should this be just Exception? All other exceptions have to be declared, no? Since no methods we call declare an exception type being throw, and neither do we, there can be no other exceptions. Note that if we caught and re-threw all exceptions, we'd also have to declare this method as throwing exceptions. https://codereview.chromium.org/849903002/diff/240001/components/cronet/andro... File components/cronet/android/test/cronet_test_jni.cc (right): https://codereview.chromium.org/849903002/diff/240001/components/cronet/andro... components/cronet/android/test/cronet_test_jni.cc:23: cronet::UploadDataStreamHandlerRegisterJni}, These should be indented two more (Hrm...I don't remember changing that) https://codereview.chromium.org/849903002/diff/240001/components/cronet/andro... File components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/CronetUploadTest.java (right): https://codereview.chromium.org/849903002/diff/240001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/CronetUploadTest.java:214: mDataProvider.assertReadNotPending(); On 2015/02/02 17:06:01, xunjieli wrote: > There is a race condition here. assertReadNotPending() might get called before > onReadSucceeded task is executed on the executor. I think I will just remove > line 214, so test is not flaky. Right, that's why we had the asserts jumping over to the DataProvider thread. https://codereview.chromium.org/849903002/diff/240001/components/cronet/andro... File components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/TestDrivenDataProvider.java (right): https://codereview.chromium.org/849903002/diff/240001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/TestDrivenDataProvider.java:140: return mNumReadCalls; Suggest a "synchronized (mLock)" here and in the next method, and moving the two variables into the proected by the lock section up top. https://codereview.chromium.org/849903002/diff/240001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/TestDrivenDataProvider.java:185: private void assertIdle() { Think this can now just be: assertRewindNotPending(); assertRewindNotPending(); (Nested synchronized statements on the same lock work in Java)
https://codereview.chromium.org/849903002/diff/260001/components/cronet/andro... File components/cronet/android/cronet_upload_data_stream.cc (right): https://codereview.chromium.org/849903002/diff/260001/components/cronet/andro... components/cronet/android/cronet_upload_data_stream.cc:34: // has completed. I thought I'd hooked this up, but doesn't look like I had gotten that far. You'll need to make CronetUploadDataStream::destroyAdapter delay destruction of the adapted if a read is pending, and only delete the adapter after it has completed. Deleting during a rewind isn't a problem.
Thanks for the review! I have addressed all comments I believe. PTAL. https://codereview.chromium.org/849903002/diff/240001/components/cronet/andro... File components/cronet/android/test/cronet_test_jni.cc (right): https://codereview.chromium.org/849903002/diff/240001/components/cronet/andro... components/cronet/android/test/cronet_test_jni.cc:23: cronet::UploadDataStreamHandlerRegisterJni}, On 2015/02/02 18:26:12, mmenke wrote: > These should be indented two more (Hrm...I don't remember changing that) Done. My bad. Thanks! git cl format makes me to leave 4 space indentation, let me know if you'd like me to change to the original 2 space indentation. https://codereview.chromium.org/849903002/diff/240001/components/cronet/andro... File components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/TestDrivenDataProvider.java (right): https://codereview.chromium.org/849903002/diff/240001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/TestDrivenDataProvider.java:140: return mNumReadCalls; On 2015/02/02 18:26:12, mmenke wrote: > Suggest a "synchronized (mLock)" here and in the next method, and moving the two > variables into the proected by the lock section up top. Done. https://codereview.chromium.org/849903002/diff/240001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/TestDrivenDataProvider.java:185: private void assertIdle() { On 2015/02/02 18:26:12, mmenke wrote: > Think this can now just be: > > assertRewindNotPending(); > assertRewindNotPending(); > > (Nested synchronized statements on the same lock work in Java) Done. https://codereview.chromium.org/849903002/diff/260001/components/cronet/andro... File components/cronet/android/cronet_upload_data_stream.cc (right): https://codereview.chromium.org/849903002/diff/260001/components/cronet/andro... components/cronet/android/cronet_upload_data_stream.cc:34: // has completed. On 2015/02/02 18:35:13, mmenke wrote: > I thought I'd hooked this up, but doesn't look like I had gotten that far. > You'll need to make CronetUploadDataStream::destroyAdapter delay destruction of > the adapted if a read is pending, and only delete the adapter after it has > completed. > > Deleting during a rewind isn't a problem. Done.
Per email discussion, I removed the bytebuffer.flip() part. PTAL. thanks!
https://codereview.chromium.org/849903002/diff/300001/components/cronet.gypi File components/cronet.gypi (right): https://codereview.chromium.org/849903002/diff/300001/components/cronet.gypi#... components/cronet.gypi:126: 'cronet/android/cronet_upload_data_stream_adapter.cc', nit: these should go below cronet_loader. https://codereview.chromium.org/849903002/diff/300001/components/cronet.gypi#... components/cronet.gypi:328: 'cronet/android/test/upload_test_server.cc', is it still called upload_test_server?
https://codereview.chromium.org/849903002/diff/300001/components/cronet.gypi File components/cronet.gypi (right): https://codereview.chromium.org/849903002/diff/300001/components/cronet.gypi#... components/cronet.gypi:126: 'cronet/android/cronet_upload_data_stream_adapter.cc', On 2015/02/03 22:08:23, mef wrote: > nit: these should go below cronet_loader. Done. https://codereview.chromium.org/849903002/diff/300001/components/cronet.gypi#... components/cronet.gypi:328: 'cronet/android/test/upload_test_server.cc', On 2015/02/03 22:08:23, mef wrote: > is it still called upload_test_server? Oops. Will change it in a separate CL.
https://codereview.chromium.org/849903002/diff/320001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/CronetUploadDataStream.java (right): https://codereview.chromium.org/849903002/diff/320001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/CronetUploadDataStream.java:121: synchronized (mLock) { Maybe throw an exception if !mReading and !mRewinding? https://codereview.chromium.org/849903002/diff/320001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/CronetUploadDataStream.java:131: mRequest.onUploadException(exception); This should be safe, even if we just deleted the adapter, because in that case, the request has already been cancelled... Maybe a comment along those lines? https://codereview.chromium.org/849903002/diff/320001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/CronetUploadDataStream.java:210: mDestroyAdapterPostponed = true; Should probably have a test for this - using your test fixture, start an upload, wait for the read to be pending, and then during the read, delete the C++ object. Then make the read complete. The test is racy, in that the read could complete either before or after destroyAdapter() is called. It should pass either way, but we're really only interested in the latter case. I don't see a way around the race, unfortunately, without digging into the guts of the UploadDataStream.
Patchset #10 (id:340001) has been deleted
Patchset #10 (id:360001) has been deleted
PTAL https://codereview.chromium.org/849903002/diff/320001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/CronetUploadDataStream.java (right): https://codereview.chromium.org/849903002/diff/320001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/CronetUploadDataStream.java:52: // Native adapter object, owned by the CronetUploadDataStream. It's only The name is confusing. Can I refer to mUploadDataStreamDelegate as the delegate (rather than the adapter) in the comment above? and rename destroyAdapter() to destroyDelegate() ? WDYT? https://codereview.chromium.org/849903002/diff/320001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/CronetUploadDataStream.java:121: synchronized (mLock) { On 2015/02/04 16:46:00, mmenke wrote: > Maybe throw an exception if !mReading and !mRewinding? Done. https://codereview.chromium.org/849903002/diff/320001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/CronetUploadDataStream.java:131: mRequest.onUploadException(exception); On 2015/02/04 16:46:00, mmenke wrote: > This should be safe, even if we just deleted the adapter, because in that case, > the request has already been cancelled... Maybe a comment along those lines? Done.
https://codereview.chromium.org/849903002/diff/380001/components/cronet/andro... File components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/TestDataProvider.java (right): https://codereview.chromium.org/849903002/diff/380001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/TestDataProvider.java:14: class TestUploadDataProvider implements UploadDataProvider { class comment? https://codereview.chromium.org/849903002/diff/380001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/TestDataProvider.java:91: // @returns the cumulative length of all data added by calls to addRead. javadoc? https://codereview.chromium.org/849903002/diff/380001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/TestDataProvider.java:127: throw new IllegalStateException( hmm, why doesn't it throw if it is chunked but over mReads.size()? https://codereview.chromium.org/849903002/diff/380001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/TestDataProvider.java:135: uploadDataSink.onReadSucceeded(false); what if it IS final chunk? https://codereview.chromium.org/849903002/diff/380001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/TestDataProvider.java:153: nit: nl https://codereview.chromium.org/849903002/diff/380001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/TestDataProvider.java:165: @Override nit: tab https://codereview.chromium.org/849903002/diff/380001/components/cronet/andro... File components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/TestDrivenDataProvider.java (right): https://codereview.chromium.org/849903002/diff/380001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/TestDrivenDataProvider.java:54: public long getLength() { Would it make sense to take a 'isChunked' flag in constructor, and return -1 here if it is true? https://codereview.chromium.org/849903002/diff/380001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/TestDrivenDataProvider.java:67: int currentReadCall = mNumReadCalls; unused? https://codereview.chromium.org/849903002/diff/380001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/TestDrivenDataProvider.java:73: if (byteBuffer.capacity() < mReads.get(mNextRead).length) { are we always expecting byteBuffer.position() to be 0 here? https://codereview.chromium.org/849903002/diff/380001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/TestDrivenDataProvider.java:104: public void onReadSucceeded(final UploadDataSink uploadDataSink) { should this take 'finalChunk' as a parameter? https://codereview.chromium.org/849903002/diff/380001/components/cronet/andro... File components/cronet/android/test/src/org/chromium/cronet_test_apk/UploadDataStreamHandler.java (right): https://codereview.chromium.org/849903002/diff/380001/components/cronet/andro... components/cronet/android/test/src/org/chromium/cronet_test_apk/UploadDataStreamHandler.java:30: private int mInitSync = -1; can mInitSync be boolean? Also, maybe a bit more descriptive name?
Thanks for the review! PTAL https://codereview.chromium.org/849903002/diff/380001/components/cronet/andro... File components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/TestDataProvider.java (right): https://codereview.chromium.org/849903002/diff/380001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/TestDataProvider.java:14: class TestUploadDataProvider implements UploadDataProvider { On 2015/02/04 21:41:05, mef wrote: > class comment? Done. https://codereview.chromium.org/849903002/diff/380001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/TestDataProvider.java:91: // @returns the cumulative length of all data added by calls to addRead. On 2015/02/04 21:41:05, mef wrote: > javadoc? Done. https://codereview.chromium.org/849903002/diff/380001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/TestDataProvider.java:127: throw new IllegalStateException( On 2015/02/04 21:41:05, mef wrote: > hmm, why doesn't it throw if it is chunked but over mReads.size()? Done. https://codereview.chromium.org/849903002/diff/380001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/TestDataProvider.java:135: uploadDataSink.onReadSucceeded(false); On 2015/02/04 21:41:05, mef wrote: > what if it IS final chunk? Done. https://codereview.chromium.org/849903002/diff/380001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/TestDataProvider.java:153: On 2015/02/04 21:41:05, mef wrote: > nit: nl Done. https://codereview.chromium.org/849903002/diff/380001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/TestDataProvider.java:165: @Override On 2015/02/04 21:41:05, mef wrote: > nit: tab Done. https://codereview.chromium.org/849903002/diff/380001/components/cronet/andro... File components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/TestDrivenDataProvider.java (right): https://codereview.chromium.org/849903002/diff/380001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/TestDrivenDataProvider.java:54: public long getLength() { On 2015/02/04 21:41:05, mef wrote: > Would it make sense to take a 'isChunked' flag in constructor, and return -1 > here if it is true? This data provider only does not test chunked mode. This data provider is mainly for making sure init/read/rewind call order is correct. https://codereview.chromium.org/849903002/diff/380001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/TestDrivenDataProvider.java:67: int currentReadCall = mNumReadCalls; On 2015/02/04 21:41:05, mef wrote: > unused? Done. https://codereview.chromium.org/849903002/diff/380001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/TestDrivenDataProvider.java:73: if (byteBuffer.capacity() < mReads.get(mNextRead).length) { On 2015/02/04 21:41:05, mef wrote: > are we always expecting byteBuffer.position() to be 0 here? I believe so, since we are not reusing java buffers in the JNI code (there's a TODO to change that, but we are not there yet). Matt correct me if I am wrong. https://codereview.chromium.org/849903002/diff/380001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/TestDrivenDataProvider.java:104: public void onReadSucceeded(final UploadDataSink uploadDataSink) { On 2015/02/04 21:41:05, mef wrote: > should this take 'finalChunk' as a parameter? This test class does not test chunked mode. https://codereview.chromium.org/849903002/diff/380001/components/cronet/andro... File components/cronet/android/test/src/org/chromium/cronet_test_apk/UploadDataStreamHandler.java (right): https://codereview.chromium.org/849903002/diff/380001/components/cronet/andro... components/cronet/android/test/src/org/chromium/cronet_test_apk/UploadDataStreamHandler.java:30: private int mInitSync = -1; On 2015/02/04 21:41:05, mef wrote: > can mInitSync be boolean? Also, maybe a bit more descriptive name? Done.
https://codereview.chromium.org/849903002/diff/380001/components/cronet/andro... File components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/TestDrivenDataProvider.java (right): https://codereview.chromium.org/849903002/diff/380001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/TestDrivenDataProvider.java:73: if (byteBuffer.capacity() < mReads.get(mNextRead).length) { On 2015/02/04 22:24:33, xunjieli wrote: > On 2015/02/04 21:41:05, mef wrote: > > are we always expecting byteBuffer.position() to be 0 here? > > I believe so, since we are not reusing java buffers in the JNI code (there's a > TODO to change that, but we are not there yet). Matt correct me if I am wrong. It is always 0, and I don't expect that to change, but Misha's right: It makes more sense to check remaining here, and in the other DataProvider.
https://codereview.chromium.org/849903002/diff/400001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/CronetUploadDataStream.java (right): https://codereview.chromium.org/849903002/diff/400001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/CronetUploadDataStream.java:150: int limit = mByteBuffer.limit(); Don't need the limit any more, and suggest renaming the position to something along the lines of "bytesRead".
https://codereview.chromium.org/849903002/diff/400001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/CronetUploadDataStream.java (right): https://codereview.chromium.org/849903002/diff/400001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/CronetUploadDataStream.java:65: mLength = mDataProvider.getLength(); While using the upload API I found it rather unexpected that when I called setUploadDataProvider(), it immediately turned around and called my UploadDataProvider's getLength() function. I mistakenly assumed it would wait to call getLength() until UrlRequest.start() was called, similar to when read() is called. I don't feel too strongly about it but from my limited use of the new API I'd appreciate if either: 1. getLength() was removed and instead there was a |length| argument to setUploadDataProvider(), or 2. getLength() wasn't called until UrlRequest.start() was called
On 2015/02/05 15:53:39, pauljensen wrote: > https://codereview.chromium.org/849903002/diff/400001/components/cronet/andro... > File > components/cronet/android/java/src/org/chromium/net/CronetUploadDataStream.java > (right): > > https://codereview.chromium.org/849903002/diff/400001/components/cronet/andro... > components/cronet/android/java/src/org/chromium/net/CronetUploadDataStream.java:65: > mLength = mDataProvider.getLength(); > While using the upload API I found it rather unexpected that when I called > setUploadDataProvider(), it immediately turned around and called my > UploadDataProvider's getLength() function. I mistakenly assumed it would wait > to call getLength() until UrlRequest.start() was called, similar to when read() > is called. I don't feel too strongly about it but from my limited use of the > new API I'd appreciate if either: > 1. getLength() was removed and instead there was a |length| argument to > setUploadDataProvider(), or > 2. getLength() wasn't called until UrlRequest.start() was called To reiterate, I don't feel strongly about this; if you just want to ack my concern that's fine.
PTAL! Thanks! https://codereview.chromium.org/849903002/diff/380001/components/cronet/andro... File components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/TestDrivenDataProvider.java (right): https://codereview.chromium.org/849903002/diff/380001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/TestDrivenDataProvider.java:73: if (byteBuffer.capacity() < mReads.get(mNextRead).length) { On 2015/02/05 15:45:47, mmenke wrote: > On 2015/02/04 22:24:33, xunjieli wrote: > > On 2015/02/04 21:41:05, mef wrote: > > > are we always expecting byteBuffer.position() to be 0 here? > > > > I believe so, since we are not reusing java buffers in the JNI code (there's a > > TODO to change that, but we are not there yet). Matt correct me if I am wrong. > > It is always 0, and I don't expect that to change, but Misha's right: It makes > more sense to check remaining here, and in the other DataProvider. Done. https://codereview.chromium.org/849903002/diff/400001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/CronetUploadDataStream.java (right): https://codereview.chromium.org/849903002/diff/400001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/CronetUploadDataStream.java:65: mLength = mDataProvider.getLength(); On 2015/02/05 15:53:39, pauljensen wrote: > While using the upload API I found it rather unexpected that when I called > setUploadDataProvider(), it immediately turned around and called my > UploadDataProvider's getLength() function. I mistakenly assumed it would wait > to call getLength() until UrlRequest.start() was called, similar to when read() > is called. I don't feel too strongly about it but from my limited use of the > new API I'd appreciate if either: > 1. getLength() was removed and instead there was a |length| argument to > setUploadDataProvider(), or > 2. getLength() wasn't called until UrlRequest.start() was called I tried doing #1. but I think getLength() is nice as it makes mLength contained in the data provider. #2 involves another thread hop. So I'd keep it as it is since you don't feel strongly about it. Thanks! https://codereview.chromium.org/849903002/diff/400001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/CronetUploadDataStream.java:150: int limit = mByteBuffer.limit(); On 2015/02/05 15:52:29, mmenke wrote: > Don't need the limit any more, and suggest renaming the position to something > along the lines of "bytesRead". Done.
xunjieli@chromium.org changed reviewers: + miloslav@google.com
Adding Milo who might have some suggestions from consumers' perspective.
https://codereview.chromium.org/849903002/diff/440001/components/cronet/andro... File components/cronet/android/cronet_upload_data_stream.cc (right): https://codereview.chromium.org/849903002/diff/440001/components/cronet/andro... components/cronet/android/cronet_upload_data_stream.cc:67: scoped_refptr<base::MessageLoopProxy> network_message_loop_; can this be base::SingleThreadTaskRunner? https://codereview.chromium.org/849903002/diff/440001/components/cronet/andro... File components/cronet/android/cronet_upload_data_stream_adapter.h (right): https://codereview.chromium.org/849903002/diff/440001/components/cronet/andro... components/cronet/android/cronet_upload_data_stream_adapter.h:61: // Failure is handled at the Java layer. I would elaborate that these 2 are called from Java layer upon completion of operation, maybe even mention the Sink. https://codereview.chromium.org/849903002/diff/440001/components/cronet/andro... components/cronet/android/cronet_upload_data_stream_adapter.h:65: // net::UploadDataStream implementation: Does it have to be public?
Patchset #14 (id:460001) has been deleted
PTAL https://codereview.chromium.org/849903002/diff/440001/components/cronet/andro... File components/cronet/android/cronet_upload_data_stream.cc (right): https://codereview.chromium.org/849903002/diff/440001/components/cronet/andro... components/cronet/android/cronet_upload_data_stream.cc:67: scoped_refptr<base::MessageLoopProxy> network_message_loop_; On 2015/02/06 19:46:09, mef wrote: > can this be base::SingleThreadTaskRunner? Done. Looked at other cronet code, I think we should be consistent and use SingleThreadTaskRunner. Matt, let me know if you think otherwise. thanks https://codereview.chromium.org/849903002/diff/440001/components/cronet/andro... File components/cronet/android/cronet_upload_data_stream_adapter.h (right): https://codereview.chromium.org/849903002/diff/440001/components/cronet/andro... components/cronet/android/cronet_upload_data_stream_adapter.h:61: // Failure is handled at the Java layer. On 2015/02/06 19:46:09, mef wrote: > I would elaborate that these 2 are called from Java layer upon completion of > operation, maybe even mention the Sink. Done. https://codereview.chromium.org/849903002/diff/440001/components/cronet/andro... components/cronet/android/cronet_upload_data_stream_adapter.h:65: // net::UploadDataStream implementation: On 2015/02/06 19:46:09, mef wrote: > Does it have to be public? Done.
https://codereview.chromium.org/849903002/diff/440001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/CronetUploadDataStream.java (right): https://codereview.chromium.org/849903002/diff/440001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/CronetUploadDataStream.java:45: private ByteBuffer mByteBuffer = null; I think it is worth commenting that mBuffer is passed from native and used for reading. https://codereview.chromium.org/849903002/diff/440001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/CronetUploadDataStream.java:249: // TODO(mmenke): Add tests and remove this line. do we still need it? https://codereview.chromium.org/849903002/diff/440001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java (right): https://codereview.chromium.org/849903002/diff/440001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java:302: "Requests with body must have a Content-Type."); suggest: 'Requests with upload data' to match setUploadData naming. https://codereview.chromium.org/849903002/diff/440001/components/cronet/andro... File components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/CronetUploadTest.java (right): https://codereview.chromium.org/849903002/diff/440001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/CronetUploadTest.java:253: mAdapter = 0; bug: Setting mAdapter = 0 does not destoy the C++ object, you have to call native method to destroy it. https://codereview.chromium.org/849903002/diff/440001/components/cronet/andro... File components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/CronetUrlRequestTest.java (right): https://codereview.chromium.org/849903002/diff/440001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/CronetUrlRequestTest.java:815: public void testUploadChunkedNotSupported() throws Exception { What's wrong with chunked? https://codereview.chromium.org/849903002/diff/440001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/CronetUrlRequestTest.java:834: Can we add tests that cancel requests in the middle of upload?
I believe I have address all the comments. PTAL. https://codereview.chromium.org/849903002/diff/440001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/CronetUploadDataStream.java (right): https://codereview.chromium.org/849903002/diff/440001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/CronetUploadDataStream.java:45: private ByteBuffer mByteBuffer = null; On 2015/02/06 21:55:21, mef wrote: > I think it is worth commenting that mBuffer is passed from native and used for > reading. Done. https://codereview.chromium.org/849903002/diff/440001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/CronetUploadDataStream.java:249: // TODO(mmenke): Add tests and remove this line. On 2015/02/06 21:55:21, mef wrote: > do we still need it? Yes. Although chunked uploads are supported, but our test server currently does not support chunked mode. Therefore, Matt has disabled chunked support for now, util we improve our test server. This is gonna be in a follow-up CL. https://codereview.chromium.org/849903002/diff/440001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java (right): https://codereview.chromium.org/849903002/diff/440001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java:302: "Requests with body must have a Content-Type."); On 2015/02/06 21:55:21, mef wrote: > suggest: 'Requests with upload data' to match setUploadData naming. Done. https://codereview.chromium.org/849903002/diff/440001/components/cronet/andro... File components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/CronetUploadTest.java (right): https://codereview.chromium.org/849903002/diff/440001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/CronetUploadTest.java:253: mAdapter = 0; On 2015/02/06 21:55:21, mef wrote: > bug: Setting mAdapter = 0 does not destoy the C++ object, you have to call > native method to destroy it. But setting mAdapter = 0 does trigger onAdapterDestroyed() which in return triggers destoryAdapter() in CronetUploadDataStream.java. Should I call destroyAdapter() directly instead? https://codereview.chromium.org/849903002/diff/440001/components/cronet/andro... File components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/CronetUrlRequestTest.java (right): https://codereview.chromium.org/849903002/diff/440001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/CronetUrlRequestTest.java:815: public void testUploadChunkedNotSupported() throws Exception { On 2015/02/06 21:55:21, mef wrote: > What's wrong with chunked? Our test server does not supported chunked right now, so we have disabled chunked support until we fix the test server. https://codereview.chromium.org/849903002/diff/440001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/CronetUrlRequestTest.java:834: On 2015/02/06 21:55:21, mef wrote: > Can we add tests that cancel requests in the middle of upload? Yes, they are in CronetUploadTest.java, where we manually drive init(), reset(), rewind() and read() to simulate cancel in the middle of a read.
Patchset #16 (id:520001) has been deleted
https://codereview.chromium.org/849903002/diff/440001/components/cronet/andro... File components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/CronetUploadTest.java (right): https://codereview.chromium.org/849903002/diff/440001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/CronetUploadTest.java:253: mAdapter = 0; On 2015/02/09 15:59:36, xunjieli wrote: > On 2015/02/06 21:55:21, mef wrote: > > bug: Setting mAdapter = 0 does not destoy the C++ object, you have to call > > native method to destroy it. > > But setting mAdapter = 0 does trigger onAdapterDestroyed() which in return > triggers destoryAdapter() in CronetUploadDataStream.java. Hrm, mAdapter is a 'long'. Why setting it to 0 would trigger onAdapterDestroyed() > Should I call destroyAdapter() directly instead? I think so. https://codereview.chromium.org/849903002/diff/440001/components/cronet/andro... File components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/CronetUrlRequestTest.java (right): https://codereview.chromium.org/849903002/diff/440001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/CronetUrlRequestTest.java:834: On 2015/02/09 15:59:36, xunjieli wrote: > On 2015/02/06 21:55:21, mef wrote: > > Can we add tests that cancel requests in the middle of upload? > > Yes, they are in CronetUploadTest.java, where we manually drive init(), reset(), > rewind() and read() to simulate cancel in the middle of a read. That's good, but what about calling 'urlRequest.cancel()" in the middle of the read?
Maybe take a look at the latest patch? onAdapterDestroyed() was not used in Matt's original code, so that might be the confusing part as to when it was invoked. I have invoked it in the destructor of the cronet_upload_data_stream_adapter https://codereview.chromium.org/849903002/diff/440001/components/cronet/andro... File components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/CronetUploadTest.java (right): https://codereview.chromium.org/849903002/diff/440001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/CronetUploadTest.java:253: mAdapter = 0; On 2015/02/09 21:53:43, mef wrote: > On 2015/02/09 15:59:36, xunjieli wrote: > > On 2015/02/06 21:55:21, mef wrote: > > > bug: Setting mAdapter = 0 does not destoy the C++ object, you have to call > > > native method to destroy it. > > > > But setting mAdapter = 0 does trigger onAdapterDestroyed() which in return > > triggers destoryAdapter() in CronetUploadDataStream.java. > Hrm, mAdapter is a 'long'. Why setting it to 0 would trigger > onAdapterDestroyed() > > > Should I call destroyAdapter() directly instead? > I think so. mAdapter native object is owned by this test, so when we set it to 0, we invoke C++ destructor which in return calls onAdapterDestroyed(), which then call destroyDelegate(). (destroyDelegate() was formerly called destroyAdapter(), which is confusing, since it is really destroying the adapter, so I renamed it in the last patch). https://codereview.chromium.org/849903002/diff/440001/components/cronet/andro... File components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/CronetUrlRequestTest.java (right): https://codereview.chromium.org/849903002/diff/440001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/CronetUrlRequestTest.java:834: On 2015/02/09 21:53:43, mef wrote: > On 2015/02/09 15:59:36, xunjieli wrote: > > On 2015/02/06 21:55:21, mef wrote: > > > Can we add tests that cancel requests in the middle of upload? > > > > Yes, they are in CronetUploadTest.java, where we manually drive init(), > reset(), > > rewind() and read() to simulate cancel in the middle of a read. > > That's good, but what about calling 'urlRequest.cancel()" in the middle of the > read? I am a little confused as to how to achieve that here. If I just after I do urlRequest.start(), I immediately do "urlRequest.cancel()", then I can be at any stage (not necessarily during a read()). I thought that was the limitation, so we made the other tests. Let me know if you have any suggestion. I might be missing sth since i am not the original author of the code.
https://codereview.chromium.org/849903002/diff/540001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/CronetUploadDataStream.java (right): https://codereview.chromium.org/849903002/diff/540001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/CronetUploadDataStream.java:14: * Pass an upload body to a UrlRequest using an UploadDataProvider. nit: an UrlRequest. https://codereview.chromium.org/849903002/diff/540001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/CronetUploadDataStream.java:46: // UploadDataProvider for reading. I would add that it is valid from call to mDataProvider.read until onError or onReadSucceeded is called. https://codereview.chromium.org/849903002/diff/540001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/CronetUploadDataStream.java:97: try { Is there a particular reason why mDataProvider.read is called outside the lock, and mDataProvider.rewind is called under the lock?
Hey Matt, I have made some changes in response to Misha's comments. I moved mDataProvide.read() outside of the synchronized block, and removed try and catch block in readData() to match the rest of the code. Let me know if you'd like me to revert. Thanks! https://codereview.chromium.org/849903002/diff/540001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/CronetUploadDataStream.java (right): https://codereview.chromium.org/849903002/diff/540001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/CronetUploadDataStream.java:14: * Pass an upload body to a UrlRequest using an UploadDataProvider. On 2015/02/09 23:06:41, mef wrote: > nit: an UrlRequest. I believe Matt's right, it is "a UrlRequest", since the first sound in Url is consonant. An umbrella, but a universe :) See http://itknowledgeexchange.techtarget.com/writing-for-business/which-is-corre.... https://codereview.chromium.org/849903002/diff/540001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/CronetUploadDataStream.java:46: // UploadDataProvider for reading. On 2015/02/09 23:06:41, mef wrote: > I would add that it is valid from call to mDataProvider.read until onError or > onReadSucceeded is called. Done. https://codereview.chromium.org/849903002/diff/540001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/CronetUploadDataStream.java:97: try { On 2015/02/09 23:06:41, mef wrote: > Is there a particular reason why mDataProvider.read is called outside the lock, > and mDataProvider.rewind is called under the lock? I think both should be outside of the lock. Let me verify with Matt.
https://codereview.chromium.org/849903002/diff/540001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/CronetUploadDataStream.java (right): https://codereview.chromium.org/849903002/diff/540001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/CronetUploadDataStream.java:14: * Pass an upload body to a UrlRequest using an UploadDataProvider. On 2015/02/10 14:37:16, xunjieli wrote: > On 2015/02/09 23:06:41, mef wrote: > > nit: an UrlRequest. > > I believe Matt's right, it is "a UrlRequest", since the first sound in Url is > consonant. An umbrella, but a universe :) See > http://itknowledgeexchange.techtarget.com/writing-for-business/which-is-corre.... Cool, I stand corrected.
Pinging Matt for review! On 2015/02/10 14:37:17, xunjieli wrote: > Hey Matt, I have made some changes in response to Misha's comments. I moved > mDataProvide.read() outside of the synchronized block, and removed try and catch > block in readData() to match the rest of the code. Let me know if you'd like me > to revert. Thanks! > > https://codereview.chromium.org/849903002/diff/540001/components/cronet/andro... > File > components/cronet/android/java/src/org/chromium/net/CronetUploadDataStream.java > (right): > > https://codereview.chromium.org/849903002/diff/540001/components/cronet/andro... > components/cronet/android/java/src/org/chromium/net/CronetUploadDataStream.java:14: > * Pass an upload body to a UrlRequest using an UploadDataProvider. > On 2015/02/09 23:06:41, mef wrote: > > nit: an UrlRequest. > > I believe Matt's right, it is "a UrlRequest", since the first sound in Url is > consonant. An umbrella, but a universe :) See > http://itknowledgeexchange.techtarget.com/writing-for-business/which-is-corre.... > > https://codereview.chromium.org/849903002/diff/540001/components/cronet/andro... > components/cronet/android/java/src/org/chromium/net/CronetUploadDataStream.java:46: > // UploadDataProvider for reading. > On 2015/02/09 23:06:41, mef wrote: > > I would add that it is valid from call to mDataProvider.read until onError or > > onReadSucceeded is called. > > Done. > > https://codereview.chromium.org/849903002/diff/540001/components/cronet/andro... > components/cronet/android/java/src/org/chromium/net/CronetUploadDataStream.java:97: > try { > On 2015/02/09 23:06:41, mef wrote: > > Is there a particular reason why mDataProvider.read is called outside the > lock, > > and mDataProvider.rewind is called under the lock? > > I think both should be outside of the lock. Let me verify with Matt.
I'm pretty happy with this. https://codereview.chromium.org/849903002/diff/560001/components/cronet/andro... File components/cronet/android/cronet_url_request_adapter.cc (right): https://codereview.chromium.org/849903002/diff/560001/components/cronet/andro... components/cronet/android/cronet_url_request_adapter.cc:64: DCHECK(!upload_.get()); nit: This should probably just be DCHECK(!upload_); https://codereview.chromium.org/849903002/diff/560001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/CronetUploadDataStream.java (right): https://codereview.chromium.org/849903002/diff/560001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/CronetUploadDataStream.java:64: public CronetUploadDataStream(UploadDataProvider dataProvider, Most of these methods should have a little documentation https://codereview.chromium.org/849903002/diff/560001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/CronetUploadDataStream.java:157: } This can no longer happen. Throw if it does? https://codereview.chromium.org/849903002/diff/560001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/CronetUploadDataStream.java:170: if (mUploadDataStreamDelegate == 0) { This can no longer happen. Throw if it does? https://codereview.chromium.org/849903002/diff/560001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/CronetUploadDataStream.java:233: if (mReading) { Should put this in a synchronized block, I believe (Mostly for the mReading check, but I'd include the entire method) https://codereview.chromium.org/849903002/diff/560001/components/cronet/andro... File components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/CronetUploadTest.java (right): https://codereview.chromium.org/849903002/diff/560001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/CronetUploadTest.java:239: * we are interested in the latter case. Suggest a comparable rewind test (I'm not as concerned about it, but best to be thorough) https://codereview.chromium.org/849903002/diff/560001/components/cronet/andro... File components/cronet/android/test/upload_data_stream_handler.cc (right): https://codereview.chromium.org/849903002/diff/560001/components/cronet/andro... components/cronet/android/test/upload_data_stream_handler.cc:234: env, jupload_data_stream_handler_.obj(), init_callback_invoked_ == false); nit: Suggest !init_callback_invoked_, here and in the next method.
Matt, I have one question below. PTAL. https://codereview.chromium.org/849903002/diff/560001/components/cronet/andro... File components/cronet/android/cronet_url_request_adapter.cc (right): https://codereview.chromium.org/849903002/diff/560001/components/cronet/andro... components/cronet/android/cronet_url_request_adapter.cc:64: DCHECK(!upload_.get()); On 2015/02/10 19:37:09, mmenke wrote: > nit: This should probably just be DCHECK(!upload_); Done. https://codereview.chromium.org/849903002/diff/560001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/CronetUploadDataStream.java (right): https://codereview.chromium.org/849903002/diff/560001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/CronetUploadDataStream.java:64: public CronetUploadDataStream(UploadDataProvider dataProvider, On 2015/02/10 19:37:09, mmenke wrote: > Most of these methods should have a little documentation Done. https://codereview.chromium.org/849903002/diff/560001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/CronetUploadDataStream.java:157: } On 2015/02/10 19:37:09, mmenke wrote: > This can no longer happen. Throw if it does? Why this can't happen? destroyDelegateIfPostponed() can be invoked, and mUploadDataStreamDelegate is set to 0 right? So if that is 0, we want to exit rather than calling nativeOnReadSucceed. Why this can no longer happen? https://codereview.chromium.org/849903002/diff/560001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/CronetUploadDataStream.java:170: if (mUploadDataStreamDelegate == 0) { On 2015/02/10 19:37:09, mmenke wrote: > This can no longer happen. Throw if it does? Done. https://codereview.chromium.org/849903002/diff/560001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/CronetUploadDataStream.java:233: if (mReading) { On 2015/02/10 19:37:09, mmenke wrote: > Should put this in a synchronized block, I believe (Mostly for the mReading > check, but I'd include the entire method) Done. The caller aquired mLock. I'll use nested acquire of the same lock to be more specific. https://codereview.chromium.org/849903002/diff/560001/components/cronet/andro... File components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/CronetUploadTest.java (right): https://codereview.chromium.org/849903002/diff/560001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/CronetUploadTest.java:239: * we are interested in the latter case. On 2015/02/10 19:37:09, mmenke wrote: > Suggest a comparable rewind test (I'm not as concerned about it, but best to be > thorough) Done. https://codereview.chromium.org/849903002/diff/560001/components/cronet/andro... File components/cronet/android/test/upload_data_stream_handler.cc (right): https://codereview.chromium.org/849903002/diff/560001/components/cronet/andro... components/cronet/android/test/upload_data_stream_handler.cc:234: env, jupload_data_stream_handler_.obj(), init_callback_invoked_ == false); On 2015/02/10 19:37:09, mmenke wrote: > nit: Suggest !init_callback_invoked_, here and in the next method. Done.
https://codereview.chromium.org/849903002/diff/560001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/CronetUploadDataStream.java (right): https://codereview.chromium.org/849903002/diff/560001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/CronetUploadDataStream.java:157: } On 2015/02/10 20:28:30, xunjieli wrote: > On 2015/02/10 19:37:09, mmenke wrote: > > This can no longer happen. Throw if it does? > > Why this can't happen? destroyDelegateIfPostponed() can be invoked, and > mUploadDataStreamDelegate is set to 0 right? So if that is 0, we want to exit > rather than calling nativeOnReadSucceed. Why this can no longer happen? Oops - was thinking this was before the new destruction call. Fine as-is (Not worth checking it's not 0 before the destroyDelegateIfPostponed call, and may legitimately be 0 when we call into destroyIfPostponed call).
I believe I have addressed all comments. PTAL. Thanks! https://codereview.chromium.org/849903002/diff/560001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/CronetUploadDataStream.java (right): https://codereview.chromium.org/849903002/diff/560001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/CronetUploadDataStream.java:157: } On 2015/02/10 20:44:07, mmenke wrote: > On 2015/02/10 20:28:30, xunjieli wrote: > > On 2015/02/10 19:37:09, mmenke wrote: > > > This can no longer happen. Throw if it does? > > > > Why this can't happen? destroyDelegateIfPostponed() can be invoked, and > > mUploadDataStreamDelegate is set to 0 right? So if that is 0, we want to exit > > rather than calling nativeOnReadSucceed. Why this can no longer happen? > > Oops - was thinking this was before the new destruction call. Fine as-is (Not > worth checking it's not 0 before the destroyDelegateIfPostponed call, and may > legitimately be 0 when we call into destroyIfPostponed call). Acknowledged, I also removed two unnecessary checks before destroyDelegateIfPostponed call ( which is called by onError() in onReadError and onRewindError).
lgtm https://codereview.chromium.org/849903002/diff/580001/components/cronet/andro... File components/cronet/android/cronet_library_loader.cc (right): https://codereview.chromium.org/849903002/diff/580001/components/cronet/andro... components/cronet/android/cronet_library_loader.cc:43: {"CronetUploadDataStream", CronetUploadDataStreamRegisterJni}, nit: Upload should go above Url. https://codereview.chromium.org/849903002/diff/580001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/CronetUploadDataStream.java (right): https://codereview.chromium.org/849903002/diff/580001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/CronetUploadDataStream.java:58: private long mUploadDataStreamDelegate; I'm not sure about java conventions, but I would explicitly initialize it to 0 here as it is not initialized in constructor.
New patchsets have been uploaded after l-g-t-m from mef@chromium.org
Woohoo! thanks for the review. I will wait for Paul to take a look before I land this one, since he's on my reviewers list. But Matt, could you take a look? https://codereview.chromium.org/849903002/diff/580001/components/cronet/andro... File components/cronet/android/cronet_library_loader.cc (right): https://codereview.chromium.org/849903002/diff/580001/components/cronet/andro... components/cronet/android/cronet_library_loader.cc:43: {"CronetUploadDataStream", CronetUploadDataStreamRegisterJni}, On 2015/02/11 20:04:26, mef wrote: > nit: Upload should go above Url. Done. https://codereview.chromium.org/849903002/diff/580001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/CronetUploadDataStream.java (right): https://codereview.chromium.org/849903002/diff/580001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/CronetUploadDataStream.java:58: private long mUploadDataStreamDelegate; On 2015/02/11 20:04:26, mef wrote: > I'm not sure about java conventions, but I would explicitly initialize it to 0 > here as it is not initialized in constructor. Done.
I took a brief look over it all and it seemed fine. I didn't dwell too much on possible threading or state machine problems. Most of my comments are typos or optional suggestions/questions. I could think more about the threading and state machine problems, but it's such a large CL it would take me quite a while and I dunno if others have already. https://codereview.chromium.org/849903002/diff/620001/components/cronet.gypi File components/cronet.gypi (right): https://codereview.chromium.org/849903002/diff/620001/components/cronet.gypi#... components/cronet.gypi:18: 'cronet/android/java/src/org/chromium/net/CronetUploadDataStream.java', nit: not quite in alphabetical order https://codereview.chromium.org/849903002/diff/620001/components/cronet/andro... File components/cronet/android/cronet_upload_data_stream.cc (right): https://codereview.chromium.org/849903002/diff/620001/components/cronet/andro... components/cronet/android/cronet_upload_data_stream.cc:5: #include "components/cronet/android/cronet_upload_data_stream.h" nit: Would seem like this file would be better named cronet_upload_data_stream_delegate? https://codereview.chromium.org/849903002/diff/620001/components/cronet/andro... components/cronet/android/cronet_upload_data_stream.cc:198: jupload_data_stream_delegate); I'm no JNI expert, but did you consider using @NativeClassQualifiedName? Seems like it would auto-generate these static call-thru functions for you. https://codereview.chromium.org/849903002/diff/620001/components/cronet/andro... File components/cronet/android/cronet_upload_data_stream_adapter.h (right): https://codereview.chromium.org/849903002/diff/620001/components/cronet/andro... components/cronet/android/cronet_upload_data_stream_adapter.h:22: // UploadDataStream's callbacks, and ensuring only only one read/rewind request too many "only"'s https://codereview.chromium.org/849903002/diff/620001/components/cronet/andro... components/cronet/android/cronet_upload_data_stream_adapter.h:23: // send to Java is outstanding at a time. The main complexity is around send->sent https://codereview.chromium.org/849903002/diff/620001/components/cronet/andro... components/cronet/android/cronet_upload_data_stream_adapter.h:78: // Size of the upload. -1 if chunked. extra space after period https://codereview.chromium.org/849903002/diff/620001/components/cronet/andro... components/cronet/android/cronet_upload_data_stream_adapter.h:90: // True if ReadInternal has been called, the rewind hasn't completed, and Should this say InitInternal instead of ReadInternal? https://codereview.chromium.org/849903002/diff/620001/components/cronet/andro... components/cronet/android/cronet_upload_data_stream_adapter.h:103: Delegate* delegate_; const https://codereview.chromium.org/849903002/diff/620001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/CronetUploadDataStream.java (right): https://codereview.chromium.org/849903002/diff/620001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/CronetUploadDataStream.java:17: public class CronetUploadDataStream implements UploadDataSink { Why is this public? Should it be final like CronetUrlRequest? https://codereview.chromium.org/849903002/diff/620001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/CronetUploadDataStream.java:106: } nit: you have a new line on line 36 but not here. https://codereview.chromium.org/849903002/diff/620001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/UploadDataProvider.java (right): https://codereview.chromium.org/849903002/diff/620001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/UploadDataProvider.java:26: * Reads upload data into byteBuffer. Upon completion, the buffer's position byteBuffer should probably be in {@code } https://codereview.chromium.org/849903002/diff/620001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/UploadDataProvider.java:30: * onReadSucceeded on success or onError on failure. Neither read nor rewind onError->onReadError onReadSucceeded and onReadError should probably be in {@link } too. https://codereview.chromium.org/849903002/diff/620001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/UploadDataProvider.java:32: * the associated UrlRequest is cancelled, one or the other must still be UrlRequest should probably be in {@link } https://codereview.chromium.org/849903002/diff/620001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/UploadDataProvider.java:45: * onRewindSucceeded on success or onError on failure. Neither read nor onError->onRewindError onRewindSucceeded and onRewindError should probably be in {@link } too. https://codereview.chromium.org/849903002/diff/620001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/UploadDataProvider.java:47: * Even if the associated UrlRequest is cancelled, one or the other must UrlRequest should probably be in {@link } https://codereview.chromium.org/849903002/diff/620001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/UploadDataProvider.java:52: * If rewinding is not supported, this should call onError. Note that onError->onRewindError should probably be in {@code } too. https://codereview.chromium.org/849903002/diff/620001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/UploadDataSink.java (right): https://codereview.chromium.org/849903002/diff/620001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/UploadDataSink.java:13: * Called by UploadDataProvider when a read succeeds. UploadDataProvider should probably be in {@link } true and false and Exception should probably be in {@code } https://codereview.chromium.org/849903002/diff/620001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/UrlRequest.java (right): https://codereview.chromium.org/849903002/diff/620001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/UrlRequest.java:56: * @param executor All UploadDataProvider methods will be called using this I think UploadDataProvider should not have the first letter capitalized. I think it should also be in {@code } braces. https://codereview.chromium.org/849903002/diff/620001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/UrlRequest.java:57: * executor. May optionally be the same executor the request itself is I think both copies of "executor" on this line should have the first letter capitalized and be in {@code } braces. https://codereview.chromium.org/849903002/diff/620001/components/cronet/andro... File components/cronet/android/test/upload_data_stream_handler.cc (right): https://codereview.chromium.org/849903002/diff/620001/components/cronet/andro... components/cronet/android/test/upload_data_stream_handler.cc:290: handler->CheckInitCallbackNotInvoked(); These functions seem like another good place to use @NativeClassQualifiedName
Patchset #21 (id:640001) has been deleted
Thanks for the review! I will file a bug to use "@NativeClassQualifiedName" for existing code in cronet. Didn't know about it. Very good to know. Thanks! I believe I have addressed all comments. Matt, I have changed quite a bit after Paul's review. https://codereview.chromium.org/849903002/diff/620001/components/cronet.gypi File components/cronet.gypi (right): https://codereview.chromium.org/849903002/diff/620001/components/cronet.gypi#... components/cronet.gypi:18: 'cronet/android/java/src/org/chromium/net/CronetUploadDataStream.java', On 2015/02/12 17:15:39, pauljensen wrote: > nit: not quite in alphabetical order Done. https://codereview.chromium.org/849903002/diff/620001/components/cronet/andro... File components/cronet/android/cronet_upload_data_stream.cc (right): https://codereview.chromium.org/849903002/diff/620001/components/cronet/andro... components/cronet/android/cronet_upload_data_stream.cc:5: #include "components/cronet/android/cronet_upload_data_stream.h" On 2015/02/12 17:15:39, pauljensen wrote: > nit: Would seem like this file would be better named > cronet_upload_data_stream_delegate? Done. SGTM https://codereview.chromium.org/849903002/diff/620001/components/cronet/andro... components/cronet/android/cronet_upload_data_stream.cc:198: jupload_data_stream_delegate); On 2015/02/12 17:15:39, pauljensen wrote: > I'm no JNI expert, but did you consider using @NativeClassQualifiedName? Seems > like it would auto-generate these static call-thru functions for you. Done. Good to know. That's very convenient! https://codereview.chromium.org/849903002/diff/620001/components/cronet/andro... File components/cronet/android/cronet_upload_data_stream_adapter.h (right): https://codereview.chromium.org/849903002/diff/620001/components/cronet/andro... components/cronet/android/cronet_upload_data_stream_adapter.h:22: // UploadDataStream's callbacks, and ensuring only only one read/rewind request On 2015/02/12 17:15:39, pauljensen wrote: > too many "only"'s Done. https://codereview.chromium.org/849903002/diff/620001/components/cronet/andro... components/cronet/android/cronet_upload_data_stream_adapter.h:23: // send to Java is outstanding at a time. The main complexity is around On 2015/02/12 17:15:39, pauljensen wrote: > send->sent Done. https://codereview.chromium.org/849903002/diff/620001/components/cronet/andro... components/cronet/android/cronet_upload_data_stream_adapter.h:78: // Size of the upload. -1 if chunked. On 2015/02/12 17:15:39, pauljensen wrote: > extra space after period Done. https://codereview.chromium.org/849903002/diff/620001/components/cronet/andro... components/cronet/android/cronet_upload_data_stream_adapter.h:90: // True if ReadInternal has been called, the rewind hasn't completed, and On 2015/02/12 17:15:39, pauljensen wrote: > Should this say InitInternal instead of ReadInternal? Done. Good catch! https://codereview.chromium.org/849903002/diff/620001/components/cronet/andro... components/cronet/android/cronet_upload_data_stream_adapter.h:103: Delegate* delegate_; On 2015/02/12 17:15:39, pauljensen wrote: > const Done. https://codereview.chromium.org/849903002/diff/620001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/CronetUploadDataStream.java (right): https://codereview.chromium.org/849903002/diff/620001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/CronetUploadDataStream.java:17: public class CronetUploadDataStream implements UploadDataSink { On 2015/02/12 17:15:40, pauljensen wrote: > Why is this public? > Should it be final like CronetUrlRequest? I think it is public is because we used it in tests. The test package is named differently, which require this to be public. However, I have a CL at https://codereview.chromium.org/884003004/ to change this. https://codereview.chromium.org/849903002/diff/620001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/CronetUploadDataStream.java:106: } On 2015/02/12 17:15:40, pauljensen wrote: > nit: you have a new line on line 36 but not here. Done. https://codereview.chromium.org/849903002/diff/620001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/UploadDataProvider.java (right): https://codereview.chromium.org/849903002/diff/620001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/UploadDataProvider.java:26: * Reads upload data into byteBuffer. Upon completion, the buffer's position On 2015/02/12 17:15:40, pauljensen wrote: > byteBuffer should probably be in {@code } Done. https://codereview.chromium.org/849903002/diff/620001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/UploadDataProvider.java:30: * onReadSucceeded on success or onError on failure. Neither read nor rewind On 2015/02/12 17:15:40, pauljensen wrote: > onError->onReadError > onReadSucceeded and onReadError should probably be in {@link } too. Done. https://codereview.chromium.org/849903002/diff/620001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/UploadDataProvider.java:32: * the associated UrlRequest is cancelled, one or the other must still be On 2015/02/12 17:15:40, pauljensen wrote: > UrlRequest should probably be in {@link } Done. https://codereview.chromium.org/849903002/diff/620001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/UploadDataProvider.java:47: * Even if the associated UrlRequest is cancelled, one or the other must On 2015/02/12 17:15:40, pauljensen wrote: > UrlRequest should probably be in {@link } Done. https://codereview.chromium.org/849903002/diff/620001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/UploadDataProvider.java:52: * If rewinding is not supported, this should call onError. Note that On 2015/02/12 17:15:40, pauljensen wrote: > onError->onRewindError > should probably be in {@code } too. Done. https://codereview.chromium.org/849903002/diff/620001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/UploadDataSink.java (right): https://codereview.chromium.org/849903002/diff/620001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/UploadDataSink.java:13: * Called by UploadDataProvider when a read succeeds. On 2015/02/12 17:15:40, pauljensen wrote: > UploadDataProvider should probably be in {@link } > true and false and Exception should probably be in {@code } Done. https://codereview.chromium.org/849903002/diff/620001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/UrlRequest.java (right): https://codereview.chromium.org/849903002/diff/620001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/UrlRequest.java:56: * @param executor All UploadDataProvider methods will be called using this On 2015/02/12 17:15:40, pauljensen wrote: > I think UploadDataProvider should not have the first letter capitalized. I > think it should also be in {@code } braces. Done. https://codereview.chromium.org/849903002/diff/620001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/UrlRequest.java:57: * executor. May optionally be the same executor the request itself is On 2015/02/12 17:15:40, pauljensen wrote: > I think both copies of "executor" on this line should have the first letter > capitalized and be in {@code } braces. Done. https://codereview.chromium.org/849903002/diff/620001/components/cronet/andro... File components/cronet/android/test/upload_data_stream_handler.cc (right): https://codereview.chromium.org/849903002/diff/620001/components/cronet/andro... components/cronet/android/test/upload_data_stream_handler.cc:290: handler->CheckInitCallbackNotInvoked(); On 2015/02/12 17:15:40, pauljensen wrote: > These functions seem like another good place to use @NativeClassQualifiedName Done.
https://codereview.chromium.org/849903002/diff/620001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/CronetUploadDataStream.java (right): https://codereview.chromium.org/849903002/diff/620001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/CronetUploadDataStream.java:17: public class CronetUploadDataStream implements UploadDataSink { On 2015/02/12 20:56:27, xunjieli wrote: > On 2015/02/12 17:15:40, pauljensen wrote: > > Why is this public? > > Should it be final like CronetUrlRequest? > > I think it is public is because we used it in tests. The test package is named > differently, which require this to be public. However, I have a CL at > https://codereview.chromium.org/884003004/ to change this. Alright, my renaming CL is landed. I have rebased, and removed "public" modifier :)
https://codereview.chromium.org/849903002/diff/680001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/UploadDataProvider.java (right): https://codereview.chromium.org/849903002/diff/680001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/UploadDataProvider.java:30: * {@code uploadDataSink}: {@link #onReadSucceeded} on success or Do these @links work? I thought you could drop the class name (i.e. {@link #blah} instead of {@link thing#blah} ) only when the function was in the same class? https://codereview.chromium.org/849903002/diff/680001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/UploadDataSink.java (right): https://codereview.chromium.org/849903002/diff/680001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/UploadDataSink.java:14: * @param finalChunk For chunked uploads, true if this is the final read. true->{@code true} https://codereview.chromium.org/849903002/diff/680001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/UploadDataSink.java:15: * It must be false for non-chunked uploads. false->{@code false}
Thanks for the review! PTAL https://codereview.chromium.org/849903002/diff/680001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/UploadDataProvider.java (right): https://codereview.chromium.org/849903002/diff/680001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/UploadDataProvider.java:30: * {@code uploadDataSink}: {@link #onReadSucceeded} on success or On 2015/02/17 15:55:40, pauljensen wrote: > Do these @links work? I thought you could drop the class name (i.e. {@link > #blah} instead of {@link thing#blah} ) only when the function was in the same > class? Done. You are right, It should be prefixed by class. I just generated javadoc and confirmed that {@link UploadDataProvider#onReadSucceeded} correctly links. https://codereview.chromium.org/849903002/diff/680001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/UploadDataSink.java (right): https://codereview.chromium.org/849903002/diff/680001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/UploadDataSink.java:14: * @param finalChunk For chunked uploads, true if this is the final read. On 2015/02/17 15:55:40, pauljensen wrote: > true->{@code true} Done. https://codereview.chromium.org/849903002/diff/680001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/UploadDataSink.java:15: * It must be false for non-chunked uploads. On 2015/02/17 15:55:40, pauljensen wrote: > false->{@code false} Done.
https://codereview.chromium.org/849903002/diff/680001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/UploadDataProvider.java (right): https://codereview.chromium.org/849903002/diff/680001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/UploadDataProvider.java:15: * An upload is either always chunked, across multiple uploads if the data ends Need to insert <p> in the beginning of paragraphs. https://codereview.chromium.org/849903002/diff/680001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/UploadDataProvider.java:42: public void read(UploadDataSink uploadDataSink, ByteBuffer byteBuffer); Maybe returning an int code instead of having to call onReadSucceeded, etc, would be safer? Could also be declared "throws IOException". This way, Cronet will not have to rely on implementors actually calling the appropriate method and the compiler will help clients do the right thing. https://codereview.chromium.org/849903002/diff/680001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/UploadDataProvider.java:54: * If rewinding is not supported, this should call {@link #onRewindError}. <p> https://codereview.chromium.org/849903002/diff/680001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/UploadDataProvider.java:61: public void rewind(UploadDataSink uploadDataSink); Similarly, maybe throws an IOException and returning some value, so that people don't just forget and leave it with empty body.
I am going to defer the two API design questions to Matt. Matt, any thoughts? On 2015/02/17 18:14:01, miloslav wrote: > https://codereview.chromium.org/849903002/diff/680001/components/cronet/andro... > File components/cronet/android/java/src/org/chromium/net/UploadDataProvider.java > (right): > > https://codereview.chromium.org/849903002/diff/680001/components/cronet/andro... > components/cronet/android/java/src/org/chromium/net/UploadDataProvider.java:15: > * An upload is either always chunked, across multiple uploads if the data ends > Need to insert <p> in the beginning of paragraphs. > Done. > https://codereview.chromium.org/849903002/diff/680001/components/cronet/andro... > components/cronet/android/java/src/org/chromium/net/UploadDataProvider.java:42: > public void read(UploadDataSink uploadDataSink, ByteBuffer byteBuffer); > Maybe returning an int code instead of having to call onReadSucceeded, etc, > would be safer? Could also be declared "throws IOException". This way, Cronet > will not have to rely on implementors actually calling the appropriate method > and the compiler will help clients do the right thing. > > https://codereview.chromium.org/849903002/diff/680001/components/cronet/andro... > components/cronet/android/java/src/org/chromium/net/UploadDataProvider.java:54: > * If rewinding is not supported, this should call {@link #onRewindError}. > <p> > Done. > https://codereview.chromium.org/849903002/diff/680001/components/cronet/andro... > components/cronet/android/java/src/org/chromium/net/UploadDataProvider.java:61: > public void rewind(UploadDataSink uploadDataSink); > Similarly, maybe throws an IOException and returning some value, so that people > don't just forget and leave it with empty body.
https://codereview.chromium.org/849903002/diff/680001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/UploadDataProvider.java (right): https://codereview.chromium.org/849903002/diff/680001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/UploadDataProvider.java:42: public void read(UploadDataSink uploadDataSink, ByteBuffer byteBuffer); On 2015/02/17 18:14:00, miloslav wrote: > Maybe returning an int code instead of having to call onReadSucceeded, etc, > would be safer? Could also be declared "throws IOException". This way, Cronet > will not have to rely on implementors actually calling the appropriate method > and the compiler will help clients do the right thing. I'm very reluctant to force embedders to use a blocking API to obtain data they aren't ready to give us immediately.
https://codereview.chromium.org/849903002/diff/680001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/UploadDataProvider.java (right): https://codereview.chromium.org/849903002/diff/680001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/UploadDataProvider.java:42: public void read(UploadDataSink uploadDataSink, ByteBuffer byteBuffer); On 2015/02/17 20:25:49, mmenke wrote: > On 2015/02/17 18:14:00, miloslav wrote: > > Maybe returning an int code instead of having to call onReadSucceeded, etc, > > would be safer? Could also be declared "throws IOException". This way, Cronet > > will not have to rely on implementors actually calling the appropriate method > > and the compiler will help clients do the right thing. > > I'm very reluctant to force embedders to use a blocking API to obtain data they > aren't ready to give us immediately. Oh, and I'm fine with adding "throws IOException". We already catch runtime exceptions that it throws, anyways. Allowing IOExceptions, too, makes sense.
On 2015/02/17 20:29:33, mmenke wrote: > https://codereview.chromium.org/849903002/diff/680001/components/cronet/andro... > File components/cronet/android/java/src/org/chromium/net/UploadDataProvider.java > (right): > > https://codereview.chromium.org/849903002/diff/680001/components/cronet/andro... > components/cronet/android/java/src/org/chromium/net/UploadDataProvider.java:42: > public void read(UploadDataSink uploadDataSink, ByteBuffer byteBuffer); > On 2015/02/17 20:25:49, mmenke wrote: > > On 2015/02/17 18:14:00, miloslav wrote: > > > Maybe returning an int code instead of having to call onReadSucceeded, etc, > > > would be safer? Could also be declared "throws IOException". This way, > Cronet > > > will not have to rely on implementors actually calling the appropriate > method > > > and the compiler will help clients do the right thing. > > > > I'm very reluctant to force embedders to use a blocking API to obtain data > they > > aren't ready to give us immediately. > > Oh, and I'm fine with adding "throws IOException". We already catch runtime > exceptions that it throws, anyways. Allowing IOExceptions, too, makes sense. Done. Added the throws IOException clauses. Milo, please refer to Matt's comment above, and let us know what you think.
On 2015/02/17 20:51:48, xunjieli wrote: > On 2015/02/17 20:29:33, mmenke wrote: > > > https://codereview.chromium.org/849903002/diff/680001/components/cronet/andro... > > File > components/cronet/android/java/src/org/chromium/net/UploadDataProvider.java > > (right): > > > > > https://codereview.chromium.org/849903002/diff/680001/components/cronet/andro... > > > components/cronet/android/java/src/org/chromium/net/UploadDataProvider.java:42: > > public void read(UploadDataSink uploadDataSink, ByteBuffer byteBuffer); > > On 2015/02/17 20:25:49, mmenke wrote: > > > On 2015/02/17 18:14:00, miloslav wrote: > > > > Maybe returning an int code instead of having to call onReadSucceeded, > etc, > > > > would be safer? Could also be declared "throws IOException". This way, > > Cronet > > > > will not have to rely on implementors actually calling the appropriate > > method > > > > and the compiler will help clients do the right thing. > > > > > > I'm very reluctant to force embedders to use a blocking API to obtain data > > they > > > aren't ready to give us immediately. > > > > Oh, and I'm fine with adding "throws IOException". We already catch runtime > > exceptions that it throws, anyways. Allowing IOExceptions, too, makes sense. > > Done. Added the throws IOException clauses. Milo, please refer to Matt's comment > above, and let us know what you think. Actually, Milo's comments were based on my inability to explain the need for UploadDataSink, and Matt's explanation about avoiding blocking calls is completely reasonable.
I've reviewed all the non-test code, and just found nits. I want to carefully go over the tests again before signing off. May do that later today, may do it tomorrow. https://codereview.chromium.org/849903002/diff/720001/components/cronet/andro... File components/cronet/android/cronet_library_loader.cc (right): https://codereview.chromium.org/849903002/diff/720001/components/cronet/andro... components/cronet/android/cronet_library_loader.cc:42: CronetUploadDataStreamDelegateRegisterJni}, Should match indentation in components/cronet/android/test/cronet_test_jni.cc (I don't care which style you use, just be consistent) https://codereview.chromium.org/849903002/diff/720001/components/cronet/andro... File components/cronet/android/cronet_upload_data_stream_adapter.cc (right): https://codereview.chromium.org/849903002/diff/720001/components/cronet/andro... components/cronet/android/cronet_upload_data_stream_adapter.cc:79: // Emedder is not waiting on any operation. Note that the active operation, "Emedder" is confusing here. Maybe "The consumer" https://codereview.chromium.org/849903002/diff/720001/components/cronet/andro... File components/cronet/android/cronet_upload_data_stream_adapter.h (right): https://codereview.chromium.org/849903002/diff/720001/components/cronet/andro... components/cronet/android/cronet_upload_data_stream_adapter.h:21: // afterwards, lives on the network thread. It's responsible for invoking nit: "lives" -> "lives and is deleted", just to be a little clearer https://codereview.chromium.org/849903002/diff/720001/components/cronet/andro... components/cronet/android/cronet_upload_data_stream_adapter.h:67: nit: Remove blank line after private: https://codereview.chromium.org/849903002/diff/720001/components/cronet/andro... components/cronet/android/cronet_upload_data_stream_adapter.h:73: nit: Remove extra blank line. https://codereview.chromium.org/849903002/diff/720001/components/cronet/andro... File components/cronet/android/cronet_upload_data_stream_delegate.cc (right): https://codereview.chromium.org/849903002/diff/720001/components/cronet/andro... components/cronet/android/cronet_upload_data_stream_delegate.cc:93: // Explicitly register static JNI functions. nit: Remove comment, or move to the declaration in the header. https://codereview.chromium.org/849903002/diff/720001/components/cronet/andro... File components/cronet/android/cronet_upload_data_stream_delegate.h (right): https://codereview.chromium.org/849903002/diff/720001/components/cronet/andro... components/cronet/android/cronet_upload_data_stream_delegate.h:29: // destroys the adapter and the CronetUploadDataStream has no operation pending, "has no operation pending" -> "has no read operation pending" https://codereview.chromium.org/849903002/diff/720001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/CronetUploadDataStream.java (right): https://codereview.chromium.org/849903002/diff/720001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/CronetUploadDataStream.java:45: // ByteBuffer created in the native code and is passed to nit: -is https://codereview.chromium.org/849903002/diff/720001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/CronetUploadDataStream.java:267: } This block should now be moved to the constructor, where we populate mLength. https://codereview.chromium.org/849903002/diff/720001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/CronetUploadDataStream.java:276: * @return the address of the native CronetUploadDataStreamAdapter object nit: +. https://codereview.chromium.org/849903002/diff/720001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/UploadDataProvider.java (right): https://codereview.chromium.org/849903002/diff/720001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/UploadDataProvider.java:37: * cancelled. Maybe "request being cancelled" -> "request failing" or "request being errored out"? Same for the rewind docs. https://codereview.chromium.org/849903002/diff/720001/components/cronet/andro... File components/cronet/android/test/src/org/chromium/net/UploadDataStreamHandler.java (right): https://codereview.chromium.org/849903002/diff/720001/components/cronet/andro... components/cronet/android/test/src/org/chromium/net/UploadDataStreamHandler.java:20: public final class UploadDataStreamHandler { optional: Maybe TestUploadDataStreamHandler? Think making class names clearer that they're only for tests makes reading the code that actually uses them easier. https://codereview.chromium.org/849903002/diff/720001/components/cronet/andro... components/cronet/android/test/src/org/chromium/net/UploadDataStreamHandler.java:29: private ConditionVariable mWaitCheckReadCallback; optional: Suggest removing the "Callback" from these two variable names - "callback" is often used, Chrome side, for the methods themselves that are called back, so it's easy to misinterpret those names. https://codereview.chromium.org/849903002/diff/720001/components/cronet/andro... components/cronet/android/test/src/org/chromium/net/UploadDataStreamHandler.java:41: mData = ""; Should probably inline all these initializations above - my feeling is should either do it wherever possible, or not at all, and we seem to be going the former route in Cronet.
A few more comments. Haven't made my way through all 1,600 lines of the tests yet. https://codereview.chromium.org/849903002/diff/720001/components/cronet/andro... File components/cronet/android/test/src/org/chromium/net/UploadDataStreamHandler.java (right): https://codereview.chromium.org/849903002/diff/720001/components/cronet/andro... components/cronet/android/test/src/org/chromium/net/UploadDataStreamHandler.java:96: public void resetWaitForInitComplete() { Rather than have these close() methods, couldn't your checkBlah methods have the close at the end, not the start, and your waitForBlah also have the close right after them? Then you can also replace the current resetBlah() calls with checkBlahCalbackNotInvoked(), which seems clearer. https://codereview.chromium.org/849903002/diff/720001/components/cronet/andro... File components/cronet/android/test/upload_data_stream_handler.cc (right): https://codereview.chromium.org/849903002/diff/720001/components/cronet/andro... components/cronet/android/test/upload_data_stream_handler.cc:140: read_callback_invoked_ = false; Should we be resetting these here? If we don't expect to get a callback, but we do, could see this incorrectly clearing it. Tests should probably fail anyways, since we'll get the extra onBlahCompleted callback, but I'm paranoid. https://codereview.chromium.org/849903002/diff/720001/components/cronet/andro... File components/cronet/android/test/upload_data_stream_handler.h (right): https://codereview.chromium.org/849903002/diff/720001/components/cronet/andro... components/cronet/android/test/upload_data_stream_handler.h:22: * CronetUploadDataStreamAdapter. This comment seems a bit misleading... Maybe something like "This class allows a CronetUploadDataStreamAdapter to be driven directly from Java, for use in tests." https://codereview.chromium.org/849903002/diff/720001/components/cronet/andro... components/cronet/android/test/upload_data_stream_handler.h:26: UploadDataStreamHandler(CronetUploadDataStreamAdapter* adapter, scoped_ptr<CronetUploadDataStreamAdapter>, to make ownership semantics clear. https://codereview.chromium.org/849903002/diff/720001/components/cronet/andro... components/cronet/android/test/upload_data_stream_handler.h:26: UploadDataStreamHandler(CronetUploadDataStreamAdapter* adapter, Actually, can we just make this an UploadDataStream everywhere in this file (And switch includes as well)? I don't think we need to know what type it is, as we only depend on the parent interface. https://codereview.chromium.org/849903002/diff/720001/components/cronet/andro... components/cronet/android/test/upload_data_stream_handler.h:68: // when init or reset is called again. Created on a Java thread, but are only are -> is https://codereview.chromium.org/849903002/diff/720001/components/cronet/andro... components/cronet/android/test/upload_data_stream_handler.h:72: // when init or reset is called again. Created on a Java thread, but are only are -> is https://codereview.chromium.org/849903002/diff/720001/components/cronet/andro... components/cronet/android/test/upload_data_stream_handler.h:75: // Indicates the number of bytes read. It is reset to 0 when init, reset, or are -> is
Patchset #25 (id:740001) has been deleted
Thanks for the review! https://codereview.chromium.org/849903002/diff/680001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/UploadDataProvider.java (right): https://codereview.chromium.org/849903002/diff/680001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/UploadDataProvider.java:15: * An upload is either always chunked, across multiple uploads if the data ends On 2015/02/17 18:14:01, miloslav wrote: > Need to insert <p> in the beginning of paragraphs. Done. https://codereview.chromium.org/849903002/diff/680001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/UploadDataProvider.java:54: * If rewinding is not supported, this should call {@link #onRewindError}. On 2015/02/17 18:14:01, miloslav wrote: > <p> Done. https://codereview.chromium.org/849903002/diff/720001/components/cronet/andro... File components/cronet/android/cronet_library_loader.cc (right): https://codereview.chromium.org/849903002/diff/720001/components/cronet/andro... components/cronet/android/cronet_library_loader.cc:42: CronetUploadDataStreamDelegateRegisterJni}, On 2015/02/18 17:08:40, mmenke wrote: > Should match indentation in components/cronet/android/test/cronet_test_jni.cc (I > don't care which style you use, just be consistent) Done. I will choose the style used by "git cl format". https://codereview.chromium.org/849903002/diff/720001/components/cronet/andro... File components/cronet/android/cronet_upload_data_stream_adapter.cc (right): https://codereview.chromium.org/849903002/diff/720001/components/cronet/andro... components/cronet/android/cronet_upload_data_stream_adapter.cc:79: // Emedder is not waiting on any operation. Note that the active operation, On 2015/02/18 17:08:40, mmenke wrote: > "Emedder" is confusing here. Maybe "The consumer" Done. https://codereview.chromium.org/849903002/diff/720001/components/cronet/andro... File components/cronet/android/cronet_upload_data_stream_adapter.h (right): https://codereview.chromium.org/849903002/diff/720001/components/cronet/andro... components/cronet/android/cronet_upload_data_stream_adapter.h:21: // afterwards, lives on the network thread. It's responsible for invoking On 2015/02/18 17:08:40, mmenke wrote: > nit: "lives" -> "lives and is deleted", just to be a little clearer Done. https://codereview.chromium.org/849903002/diff/720001/components/cronet/andro... components/cronet/android/cronet_upload_data_stream_adapter.h:67: On 2015/02/18 17:08:40, mmenke wrote: > nit: Remove blank line after private: Done. https://codereview.chromium.org/849903002/diff/720001/components/cronet/andro... components/cronet/android/cronet_upload_data_stream_adapter.h:73: On 2015/02/18 17:08:40, mmenke wrote: > nit: Remove extra blank line. Done. https://codereview.chromium.org/849903002/diff/720001/components/cronet/andro... File components/cronet/android/cronet_upload_data_stream_delegate.cc (right): https://codereview.chromium.org/849903002/diff/720001/components/cronet/andro... components/cronet/android/cronet_upload_data_stream_delegate.cc:93: // Explicitly register static JNI functions. On 2015/02/18 17:08:40, mmenke wrote: > nit: Remove comment, or move to the declaration in the header. Done. https://codereview.chromium.org/849903002/diff/720001/components/cronet/andro... File components/cronet/android/cronet_upload_data_stream_delegate.h (right): https://codereview.chromium.org/849903002/diff/720001/components/cronet/andro... components/cronet/android/cronet_upload_data_stream_delegate.h:29: // destroys the adapter and the CronetUploadDataStream has no operation pending, On 2015/02/18 17:08:40, mmenke wrote: > "has no operation pending" -> "has no read operation pending" Done. https://codereview.chromium.org/849903002/diff/720001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/CronetUploadDataStream.java (right): https://codereview.chromium.org/849903002/diff/720001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/CronetUploadDataStream.java:45: // ByteBuffer created in the native code and is passed to On 2015/02/18 17:08:40, mmenke wrote: > nit: -is Done. https://codereview.chromium.org/849903002/diff/720001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/CronetUploadDataStream.java:267: } On 2015/02/18 17:08:41, mmenke wrote: > This block should now be moved to the constructor, where we populate mLength. Done. https://codereview.chromium.org/849903002/diff/720001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/CronetUploadDataStream.java:276: * @return the address of the native CronetUploadDataStreamAdapter object On 2015/02/18 17:08:40, mmenke wrote: > nit: +. Done. https://codereview.chromium.org/849903002/diff/720001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/UploadDataProvider.java (right): https://codereview.chromium.org/849903002/diff/720001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/UploadDataProvider.java:37: * cancelled. On 2015/02/18 17:08:41, mmenke wrote: > Maybe "request being cancelled" -> "request failing" or "request being errored > out"? Same for the rewind docs. Done. https://codereview.chromium.org/849903002/diff/720001/components/cronet/andro... File components/cronet/android/test/src/org/chromium/net/UploadDataStreamHandler.java (right): https://codereview.chromium.org/849903002/diff/720001/components/cronet/andro... components/cronet/android/test/src/org/chromium/net/UploadDataStreamHandler.java:20: public final class UploadDataStreamHandler { On 2015/02/18 17:08:41, mmenke wrote: > optional: Maybe TestUploadDataStreamHandler? Think making class names clearer > that they're only for tests makes reading the code that actually uses them > easier. Done. Good idea! https://codereview.chromium.org/849903002/diff/720001/components/cronet/andro... components/cronet/android/test/src/org/chromium/net/UploadDataStreamHandler.java:29: private ConditionVariable mWaitCheckReadCallback; On 2015/02/18 17:08:41, mmenke wrote: > optional: Suggest removing the "Callback" from these two variable names - > "callback" is often used, Chrome side, for the methods themselves that are > called back, so it's easy to misinterpret those names. Done. https://codereview.chromium.org/849903002/diff/720001/components/cronet/andro... components/cronet/android/test/src/org/chromium/net/UploadDataStreamHandler.java:41: mData = ""; On 2015/02/18 17:08:41, mmenke wrote: > Should probably inline all these initializations above - my feeling is should > either do it wherever possible, or not at all, and we seem to be going the > former route in Cronet. Done. https://codereview.chromium.org/849903002/diff/720001/components/cronet/andro... components/cronet/android/test/src/org/chromium/net/UploadDataStreamHandler.java:96: public void resetWaitForInitComplete() { On 2015/02/18 21:06:23, mmenke wrote: > Rather than have these close() methods, couldn't your checkBlah methods have the > close at the end, not the start, and your waitForBlah also have the close right > after them? > > Then you can also replace the current resetBlah() calls with > checkBlahCalbackNotInvoked(), which seems clearer. Done. Great idea! Thanks for the suggestion. Definitely has made the code easier to understand. https://codereview.chromium.org/849903002/diff/720001/components/cronet/andro... File components/cronet/android/test/upload_data_stream_handler.cc (right): https://codereview.chromium.org/849903002/diff/720001/components/cronet/andro... components/cronet/android/test/upload_data_stream_handler.cc:140: read_callback_invoked_ = false; On 2015/02/18 21:06:23, mmenke wrote: > Should we be resetting these here? If we don't expect to get a callback, but we > do, could see this incorrectly clearing it. Tests should probably fail anyways, > since we'll get the extra onBlahCompleted callback, but I'm paranoid. Done. https://codereview.chromium.org/849903002/diff/720001/components/cronet/andro... File components/cronet/android/test/upload_data_stream_handler.h (right): https://codereview.chromium.org/849903002/diff/720001/components/cronet/andro... components/cronet/android/test/upload_data_stream_handler.h:22: * CronetUploadDataStreamAdapter. On 2015/02/18 21:06:24, mmenke wrote: > This comment seems a bit misleading... Maybe something like "This class allows > a CronetUploadDataStreamAdapter to be driven directly from Java, for use in > tests." Done. https://codereview.chromium.org/849903002/diff/720001/components/cronet/andro... components/cronet/android/test/upload_data_stream_handler.h:26: UploadDataStreamHandler(CronetUploadDataStreamAdapter* adapter, On 2015/02/18 21:06:24, mmenke wrote: > Actually, can we just make this an UploadDataStream everywhere in this file (And > switch includes as well)? I don't think we need to know what type it is, as we > only depend on the parent interface. Done. https://codereview.chromium.org/849903002/diff/720001/components/cronet/andro... components/cronet/android/test/upload_data_stream_handler.h:26: UploadDataStreamHandler(CronetUploadDataStreamAdapter* adapter, On 2015/02/18 21:06:24, mmenke wrote: > scoped_ptr<CronetUploadDataStreamAdapter>, to make ownership semantics clear. Done. https://codereview.chromium.org/849903002/diff/720001/components/cronet/andro... components/cronet/android/test/upload_data_stream_handler.h:68: // when init or reset is called again. Created on a Java thread, but are only On 2015/02/18 21:06:24, mmenke wrote: > are -> is Done. https://codereview.chromium.org/849903002/diff/720001/components/cronet/andro... components/cronet/android/test/upload_data_stream_handler.h:72: // when init or reset is called again. Created on a Java thread, but are only On 2015/02/18 21:06:24, mmenke wrote: > are -> is Done. https://codereview.chromium.org/849903002/diff/720001/components/cronet/andro... components/cronet/android/test/upload_data_stream_handler.h:75: // Indicates the number of bytes read. It is reset to 0 when init, reset, or On 2015/02/18 21:06:24, mmenke wrote: > are -> is Done.
LGTM
New patchsets have been uploaded after l-g-t-m from mmenke@chromium.org
On 2015/02/19 21:26:45, mmenke wrote: > LGTM Yay!! Thanks for the reviews! Just rebased, going to check CQ now.
The CQ bit was checked by xunjieli@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/849903002/780001
The CQ bit was unchecked by xunjieli@chromium.org
The CQ bit was checked by xunjieli@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/849903002/800001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_clang_dbg_recipe on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
The CQ bit was checked by xunjieli@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/849903002/820001
Message was sent while issue was closed.
Committed patchset #28 (id:820001)
Message was sent while issue was closed.
Patchset 28 (id:??) landed as https://crrev.com/a57830437741d839456f92f1bb8c69d088984842 Cr-Commit-Position: refs/heads/master@{#317185} |
