Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1130343006:
Don't share ResourceDispatcherHostImpl's timer for reporting upload progress. (Closed)
DescriptionDon't share ResourceDispatcherHostImpl's timer for reporting upload progress.
We want that timer to run only when needed, which is when a tab is loading,
or currently when there is an upload in progress. This change separates the
upload progress logic to make things simpler and help towards that goal.
BUG=483287
Committed: https://crrev.com/e9b1f93b69d69cd1b4b9971789ee6ac57786c1a8
Cr-Commit-Position: refs/heads/master@{#331228}
Patch Set 1 #
Total comments: 6
Patch Set 2 : #
Total comments: 8
Patch Set 3 : #
Total comments: 12
Patch Set 4 : #
Total comments: 3
Patch Set 5 : Fixes for mmenke #
Total comments: 1
Messages
Total messages: 24 (5 generated)
andresantoso@chromium.org changed reviewers: + mmenke@chromium.org
mmenke, WDYT about this change as an intermediate step towards http://crrev.com/1117923004? I think it helps simplify things by separating load states updates and upload progress.
I'd been thinking of this as a followup step, but I think it's reasonable to start with this. We should probably still test this - if we just test the ResourceLoader part, though, as opposed to more of an integration test (Which is what working with the RDH pretty much forces us to do), that should be pretty easy, though. Just make ResourceHandlerStub record progress in OnUploadProgress, and optionally invoke a callback, then do a real upload to the embedded test server, using a chunked upload. Then just add bytes, wait for callback, add bytes, wait for callback a few times. That doesn't test that we can skip an update, though, which we should also do.
I'm completely unfamiliar with this stuff. Can you explain to me, what is the embedded test server? And can you point me to some code that shows how to setup a chunked upload?
On 2015/05/19 21:46:05, Andre wrote: > I'm completely unfamiliar with this stuff. > Can you explain to me, what is the embedded test server? And can you point me to > some code that shows how to setup a chunked upload? I'd offer to write the test myself, but I'm swamped. I'll give you some pointers tomorrow.
On 2015/05/19 21:51:27, mmenke wrote: > On 2015/05/19 21:46:05, Andre wrote: > > I'm completely unfamiliar with this stuff. > > Can you explain to me, what is the embedded test server? And can you point me > to > > some code that shows how to setup a chunked upload? > > I'd offer to write the test myself, but I'm swamped. I'll give you some > pointers tomorrow. Sorry for slowness - Finding myself kinda of buried in random stuff these last two weeks. Setting up the test server is pretty trivial - see https://code.google.com/p/chromium/codesearch#chromium/src/chrome/test/base/i.... You'll also need to call its Start() method to actually start it. (You can just use content/test/data for the directory). Then to create the request just see how ResourceLoader's test fixture does it https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/lo.... For the URL, you'll need to use the embedded test server's GetURL method (with "title1.html" or some other innocuous file in "content/test/data". You'll need to hand on to the URLRequest pointer, and call its EnableChunkedUpload();. You'll need to add a Transfer-"Encoding: Chunked" header (And maybe a Content-Type header with some random value) using SetExtraRequestHeaderByName on the URLRequest. Then you just start the ResourceLoader (Which you should have passed ownership of the request to), and call URLRequest::AppendChunkToUpload as needed.
https://codereview.chromium.org/1130343006/diff/1/content/browser/loader/reso... File content/browser/loader/resource_loader.cc (right): https://codereview.chromium.org/1130343006/diff/1/content/browser/loader/reso... content/browser/loader/resource_loader.cc:146: return; // Nothing to upload. BUG: A request will return a size of 0 until it actually starts a URLRequestJob (Which the NetworkDelegate can delay). I think you're best off just always running the timer, until OnResponseStarted. https://codereview.chromium.org/1130343006/diff/1/content/browser/loader/reso... content/browser/loader/resource_loader.cc:162: last_upload_position_ = progress.position(); BUG: In the redirect case, we'll start uploading the file again, so will need to start the timer once more. https://codereview.chromium.org/1130343006/diff/1/content/browser/loader/reso... content/browser/loader/resource_loader.cc:349: Why did you remove the code here?
https://codereview.chromium.org/1130343006/diff/1/content/browser/loader/reso... File content/browser/loader/resource_loader.cc (right): https://codereview.chromium.org/1130343006/diff/1/content/browser/loader/reso... content/browser/loader/resource_loader.cc:146: return; // Nothing to upload. Thanks. Changed to use a RepeatingTimer. https://codereview.chromium.org/1130343006/diff/1/content/browser/loader/reso... content/browser/loader/resource_loader.cc:162: last_upload_position_ = progress.position(); Now using a RepeatingTimer until OnResponseStarted, which should happen after redirect (I think?). https://codereview.chromium.org/1130343006/diff/1/content/browser/loader/reso... content/browser/loader/resource_loader.cc:349: Because the way I was posting a delayed task in OnUploadProgressACK should guarantee the final progress message being sent without needing this. But now I've changed it to use a RepeatingTimer, so bringing this back. https://codereview.chromium.org/1130343006/diff/20001/content/browser/loader/... File content/browser/loader/resource_loader.cc (right): https://codereview.chromium.org/1130343006/diff/20001/content/browser/loader/... content/browser/loader/resource_loader.cc:150: return; // Nothing to upload. Do we send upload progress messages for chunked uploads? Wouldn't progress.size() be 0 when chunked? https://codereview.chromium.org/1130343006/diff/20001/content/browser/loader/... File content/browser/loader/resource_loader_unittest.cc (right): https://codereview.chromium.org/1130343006/diff/20001/content/browser/loader/... content/browser/loader/resource_loader_unittest.cc:664: base::RunLoop().RunUntilIdle(); What should we do here? What is the best way to wait for OnUploadProgress to be called? https://codereview.chromium.org/1130343006/diff/20001/content/browser/loader/... File content/browser/loader/resource_request_info_impl.cc (right): https://codereview.chromium.org/1130343006/diff/20001/content/browser/loader/... content/browser/loader/resource_request_info_impl.cc:63: true, // enable upload progress Seems like we would need to plumb this in as an additional parameter to AllocateForTesting().
https://codereview.chromium.org/1130343006/diff/20001/content/browser/loader/... File content/browser/loader/resource_loader.cc (right): https://codereview.chromium.org/1130343006/diff/20001/content/browser/loader/... content/browser/loader/resource_loader.cc:150: return; // Nothing to upload. On 2015/05/20 21:05:12, Andre wrote: > Do we send upload progress messages for chunked uploads? > Wouldn't progress.size() be 0 when chunked? Ahh, very good point! ResourceLoader isn't normally used for chunked uploads, too, so sending them through this path would also be a bit funky.
https://codereview.chromium.org/1130343006/diff/20001/content/browser/loader/... File content/browser/loader/resource_loader_unittest.cc (right): https://codereview.chromium.org/1130343006/diff/20001/content/browser/loader/... content/browser/loader/resource_loader_unittest.cc:664: base::RunLoop().RunUntilIdle(); On 2015/05/20 21:05:12, Andre wrote: > What should we do here? > What is the best way to wait for OnUploadProgress to be called? My suggestion: Add a method to ResourceHandlerStub to wait for the next call to OnUploadProgress. When it's called, have it make a RunLoop, stored in a scoped_ptr, and run it, and then resets it once its done. Have OnUploadProgress checks if it's NULL, and if not, calls its quit method. https://codereview.chromium.org/1130343006/diff/20001/content/browser/loader/... File content/browser/loader/resource_request_info_impl.cc (right): https://codereview.chromium.org/1130343006/diff/20001/content/browser/loader/... content/browser/loader/resource_request_info_impl.cc:63: true, // enable upload progress On 2015/05/20 21:05:12, Andre wrote: > Seems like we would need to plumb this in as an additional parameter to > AllocateForTesting(). You're right. That method makes me sad.
So to drive the upload, I think we need to either subclass either UploadDataStream or UploadElementReader ourselves. We could either make it return some data every 10 milliseconds or so, or drive it ourselves. First route's simpler, though we couldn't do as exact on what we expect to see, but I think it's good enough for our purposes, though if you want to go the latter route, I certainly won't stop you.
Patchset #3 (id:40001) has been deleted
PTAL at patchset 3. The upload setup is based on the code in ResourceDispatcherHostTest.CalculateApproximateMemoryCost, it seems to work well.
https://codereview.chromium.org/1130343006/diff/20001/content/browser/loader/... File content/browser/loader/resource_loader_unittest.cc (right): https://codereview.chromium.org/1130343006/diff/20001/content/browser/loader/... content/browser/loader/resource_loader_unittest.cc:664: base::RunLoop().RunUntilIdle(); Thanks, that works well. https://codereview.chromium.org/1130343006/diff/20001/content/browser/loader/... File content/browser/loader/resource_request_info_impl.cc (right): https://codereview.chromium.org/1130343006/diff/20001/content/browser/loader/... content/browser/loader/resource_request_info_impl.cc:63: true, // enable upload progress Actually, I think I can just use request->has_upload() here. https://codereview.chromium.org/1130343006/diff/60001/content/browser/loader/... File content/browser/loader/resource_loader.cc (right): https://codereview.chromium.org/1130343006/diff/60001/content/browser/loader/... content/browser/loader/resource_loader.cc:360: if (info->is_upload_progress_enabled()) { I think we need to add this check.
https://codereview.chromium.org/1130343006/diff/60001/content/browser/loader/... File content/browser/loader/resource_loader.cc (right): https://codereview.chromium.org/1130343006/diff/60001/content/browser/loader/... content/browser/loader/resource_loader.cc:144: void ResourceLoader::ReportUploadProgress() { DCHECK(info->is_upload_progress_enabled());? https://codereview.chromium.org/1130343006/diff/60001/content/browser/loader/... content/browser/loader/resource_loader.cc:504: if (GetRequestInfo()->is_upload_progress_enabled()) { I'm paranoid. Maybe add "&& request_->get_upload()". Otherwise, if some change in blink ends up in having is_upload_progress_enabled() set for non-uploads, we'll be running a ton of timers. (Note that we never attach an upload body after a request starts...Well, extensions may be able to do it during a redirect or something). https://codereview.chromium.org/1130343006/diff/60001/content/browser/loader/... File content/browser/loader/resource_loader_unittest.cc (right): https://codereview.chromium.org/1130343006/diff/60001/content/browser/loader/... content/browser/loader/resource_loader_unittest.cc:410: void SetUpResourceLoader(scoped_ptr<net::URLRequest> request) { Think this is worth a brief comment. Just something along the lines of "Replaces loader_ with a new one for the passed in request" https://codereview.chromium.org/1130343006/diff/60001/content/browser/loader/... content/browser/loader/resource_loader_unittest.cc:665: TEST_F(ResourceLoaderTest, UploadProgress) { This test doesn't actually make sure the timer works (Which is my main concern) - all it tests is that we get the final message when the upload is done. That's why I was thinking we'd need to make a new class to slowly feed the data along, and then make sure we get a couple intermediary upload progress messages. https://codereview.chromium.org/1130343006/diff/60001/content/browser/loader/... content/browser/loader/resource_loader_unittest.cc:665: TEST_F(ResourceLoaderTest, UploadProgress) { Could you add a TODO about adding a test for the redirect case? https://codereview.chromium.org/1130343006/diff/60001/content/browser/loader/... content/browser/loader/resource_loader_unittest.cc:693: EXPECT_LE(position, upload_content.size()); Should also check that position is monotonically increasing.
Patchset #4 (id:80001) has been deleted
PTAL at patchset 4. https://codereview.chromium.org/1130343006/diff/60001/content/browser/loader/... File content/browser/loader/resource_loader.cc (right): https://codereview.chromium.org/1130343006/diff/60001/content/browser/loader/... content/browser/loader/resource_loader.cc:144: void ResourceLoader::ReportUploadProgress() { On 2015/05/21 15:41:57, mmenke wrote: > DCHECK(info->is_upload_progress_enabled());? Done. https://codereview.chromium.org/1130343006/diff/60001/content/browser/loader/... content/browser/loader/resource_loader.cc:504: if (GetRequestInfo()->is_upload_progress_enabled()) { On 2015/05/21 15:41:57, mmenke wrote: > I'm paranoid. Maybe add "&& request_->get_upload()". Otherwise, if some change > in blink ends up in having is_upload_progress_enabled() set for non-uploads, > we'll be running a ton of timers. > > (Note that we never attach an upload body after a request starts...Well, > extensions may be able to do it during a redirect or something). Done. https://codereview.chromium.org/1130343006/diff/60001/content/browser/loader/... File content/browser/loader/resource_loader_unittest.cc (right): https://codereview.chromium.org/1130343006/diff/60001/content/browser/loader/... content/browser/loader/resource_loader_unittest.cc:410: void SetUpResourceLoader(scoped_ptr<net::URLRequest> request) { On 2015/05/21 15:41:57, mmenke wrote: > Think this is worth a brief comment. Just something along the lines of > "Replaces loader_ with a new one for the passed in request" Done. https://codereview.chromium.org/1130343006/diff/60001/content/browser/loader/... content/browser/loader/resource_loader_unittest.cc:665: TEST_F(ResourceLoaderTest, UploadProgress) { On 2015/05/21 15:41:57, mmenke wrote: > Could you add a TODO about adding a test for the redirect case? Done. I wrapped ChunkedUploadDataStream to make it appear non-chunked. https://codereview.chromium.org/1130343006/diff/60001/content/browser/loader/... content/browser/loader/resource_loader_unittest.cc:693: EXPECT_LE(position, upload_content.size()); On 2015/05/21 15:41:57, mmenke wrote: > Should also check that position is monotonically increasing. Done. Added the check inside OnUploadProgress().
LGTM! https://codereview.chromium.org/1130343006/diff/100001/content/browser/loader... File content/browser/loader/resource_loader_unittest.cc (right): https://codereview.chromium.org/1130343006/diff/100001/content/browser/loader... content/browser/loader/resource_loader_unittest.cc:226: EXPECT_GE(position, upload_position_); This should be GT, right? We never call it with the same value, and we also don't call it when position is 0 (Since ResourceLoader starts with a last_upload_position_ of 0). https://codereview.chromium.org/1130343006/diff/100001/content/browser/loader... content/browser/loader/resource_loader_unittest.cc:375: class NonChunkedUploadDataStream : public net::UploadDataStream { Great idea!
https://codereview.chromium.org/1130343006/diff/100001/content/browser/loader... File content/browser/loader/resource_loader_unittest.cc (right): https://codereview.chromium.org/1130343006/diff/100001/content/browser/loader... content/browser/loader/resource_loader_unittest.cc:226: EXPECT_GE(position, upload_position_); You're right, fixed. https://codereview.chromium.org/1130343006/diff/120001/content/browser/loader... File content/browser/loader/resource_dispatcher_host_impl.cc (left): https://codereview.chromium.org/1130343006/diff/120001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:2184: loader.second->ReportUploadProgress(); Oops, forgot to move this from the other CL.
The CQ bit was checked by andresantoso@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mmenke@chromium.org Link to the patchset: https://codereview.chromium.org/1130343006/#ps120001 (title: "Fixes for mmenke")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1130343006/120001
Message was sent while issue was closed.
Committed patchset #5 (id:120001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/e9b1f93b69d69cd1b4b9971789ee6ac57786c1a8 Cr-Commit-Position: refs/heads/master@{#331228} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
