Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2574143003:
Implement upload progress handling in Mojo loading (Closed)
|
Created:
4 years ago by tzik Modified:
3 years, 11 months ago CC:
chromium-reviews, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org, kinuko+watch, jsbell+serviceworker_chromium.org, jam, abarth-chromium, darin-cc_chromium.org, loading-reviews_chromium.org, blink-worker-reviews_chromium.org, mlamouri+watch-content_chromium.org, Randy Smith (Not in Mondays), nhiroki, michaeln, shimazu+serviceworker_chromium.org, serviceworker-reviews, Aaron Boodman, kinuko+serviceworker, horo+watch_chromium.org, darin (slow to review), mmenke Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionImplement upload progress handling in Mojo loading
This CL integrates upload progress events from the browser side loading
stack in Mojo to the renderer.
BUG=603396
Review-Url: https://codereview.chromium.org/2574143003
Cr-Commit-Position: refs/heads/master@{#445727}
Committed: https://chromium.googlesource.com/chromium/src/+/bedcbce4a048c2e6c6eaf7a3cef82eecf85543e9
Patch Set 1 #Patch Set 2 : +ResourceDispatcher #Patch Set 3 : update TestExpectations #
Total comments: 4
Patch Set 4 : . #Patch Set 5 : DEPS & #include #Patch Set 6 : rebase #
Total comments: 6
Patch Set 7 : rebase, +comment, +#include #Patch Set 8 : update TestExpectations #
Total comments: 6
Patch Set 9 : rebase on the unittest CL #Patch Set 10 : rebase #Patch Set 11 : +test #Patch Set 12 : rebase #Patch Set 13 : remove unused fake #Patch Set 14 : +content_browsertests #
Total comments: 10
Patch Set 15 : wip #Patch Set 16 : +test case for UploadProgressHandler passes #Patch Set 17 : fix #Patch Set 18 : +sanity check. +rewind case test. #Patch Set 19 : android build fix #
Total comments: 12
Patch Set 20 : +#include. +forward decl. +DISALLOW_COPY_AND_ASSIGN. #
Total comments: 6
Patch Set 21 : s/OnMesnsageReceived/Dispatch/ #
Total comments: 2
Patch Set 22 : +comment #Messages
Total messages: 119 (90 generated)
The CQ bit was checked by tzik@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by tzik@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Implement upload progress handling in Mojo loading TODO: Comment on .mojom TODO: ResourceDispatcher TODO: Test BUG= ========== to ========== Implement upload progress handling in Mojo loading TODO: Comment on .mojom TODO: Test BUG= ==========
The CQ bit was checked by tzik@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by tzik@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
tzik@chromium.org changed reviewers: + yhirano@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2574143003/diff/40001/content/browser/loader/... File content/browser/loader/DEPS (right): https://codereview.chromium.org/2574143003/diff/40001/content/browser/loader/... content/browser/loader/DEPS:97: "+content/browser/loader/test_url_loader_client.h", Hmm, I think mojo_async_resource_handler.(cc|h) should not include test_url_loader_client.h and it doesn't. Can you remove this entry? https://codereview.chromium.org/2574143003/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/TestExpectations (left): https://codereview.chromium.org/2574143003/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/TestExpectations:667: crbug.com/280342 [ Linux Win ] virtual/mojo-loading/http/tests/media/progress-events-generated-correctly.html [ Failure ] I think these are unrelated given the non-virtual one is also failing.
The CQ bit was checked by tzik@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by tzik@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Implement upload progress handling in Mojo loading TODO: Comment on .mojom TODO: Test BUG= ========== to ========== Implement upload progress handling in Mojo loading BUG= ==========
The CQ bit was checked by tzik@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2574143003/diff/40001/content/browser/loader/... File content/browser/loader/DEPS (right): https://codereview.chromium.org/2574143003/diff/40001/content/browser/loader/... content/browser/loader/DEPS:97: "+content/browser/loader/test_url_loader_client.h", On 2016/12/15 06:21:25, yhirano wrote: > Hmm, I think mojo_async_resource_handler.(cc|h) should not include > test_url_loader_client.h and it doesn't. Can you remove this entry? Done. https://codereview.chromium.org/2574143003/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/TestExpectations (left): https://codereview.chromium.org/2574143003/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/TestExpectations:667: crbug.com/280342 [ Linux Win ] virtual/mojo-loading/http/tests/media/progress-events-generated-correctly.html [ Failure ] On 2016/12/15 06:21:25, yhirano wrote: > I think these are unrelated given the non-virtual one is also failing. Done.
Description was changed from ========== Implement upload progress handling in Mojo loading BUG= ========== to ========== Implement upload progress handling in Mojo loading This CL integrates upload progress events from the browser side loading stack in Mojo to the renderer. BUG= ==========
Talked offline w/tzik@ about testing. He will write unittests for UploadProgressTracker separately. MojoAsyncResourceHandler implementation is covered by layout tests. LGTM. Please provide a bug number. https://codereview.chromium.org/2574143003/diff/100001/content/browser/loader... File content/browser/loader/mojo_async_resource_handler.cc (right): https://codereview.chromium.org/2574143003/diff/100001/content/browser/loader... content/browser/loader/mojo_async_resource_handler.cc:7: #include <algorithm> Why is this needed? https://codereview.chromium.org/2574143003/diff/100001/content/browser/loader... content/browser/loader/mojo_async_resource_handler.cc:13: #include "base/macros.h" +base/memory/ptr_util.h https://codereview.chromium.org/2574143003/diff/100001/content/child/resource... File content/child/resource_dispatcher.cc (right): https://codereview.chromium.org/2574143003/diff/100001/content/child/resource... content/child/resource_dispatcher.cc:241: if (!request_info->url_loader) { Please leave some comments here.
Description was changed from ========== Implement upload progress handling in Mojo loading This CL integrates upload progress events from the browser side loading stack in Mojo to the renderer. BUG= ========== to ========== Implement upload progress handling in Mojo loading This CL integrates upload progress events from the browser side loading stack in Mojo to the renderer. BUG=670562 ==========
Description was changed from ========== Implement upload progress handling in Mojo loading This CL integrates upload progress events from the browser side loading stack in Mojo to the renderer. BUG=670562 ========== to ========== Implement upload progress handling in Mojo loading This CL integrates upload progress events from the browser side loading stack in Mojo to the renderer. BUG=603396 ==========
The CQ bit was checked by tzik@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2574143003/diff/100001/content/browser/loader... File content/browser/loader/mojo_async_resource_handler.cc (right): https://codereview.chromium.org/2574143003/diff/100001/content/browser/loader... content/browser/loader/mojo_async_resource_handler.cc:7: #include <algorithm> On 2016/12/16 08:47:56, yhirano wrote: > Why is this needed? This is for std::min(). https://codereview.chromium.org/2574143003/diff/100001/content/browser/loader... content/browser/loader/mojo_async_resource_handler.cc:13: #include "base/macros.h" On 2016/12/16 08:47:56, yhirano wrote: > +base/memory/ptr_util.h Done. https://codereview.chromium.org/2574143003/diff/100001/content/child/resource... File content/child/resource_dispatcher.cc (right): https://codereview.chromium.org/2574143003/diff/100001/content/child/resource... content/child/resource_dispatcher.cc:241: if (!request_info->url_loader) { On 2016/12/16 08:47:56, yhirano wrote: > Please leave some comments here. Done.
The CQ bit was checked by tzik@chromium.org to run a CQ dry run
tzik@chromium.org changed reviewers: + mmenke@chromium.org
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
tzik@chromium.org changed reviewers: + nhiroki@chromium.org
Adding mmenke and nhiroki. PTAL to nhiroki: service_worker code. mmenke: //content/browser/loader
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Seems like we should have tests for this, otherwise, looks really good. https://codereview.chromium.org/2574143003/diff/140001/content/browser/loader... File content/browser/loader/test_url_loader_client.cc (right): https://codereview.chromium.org/2574143003/diff/140001/content/browser/loader... content/browser/loader/test_url_loader_client.cc:56: ack_callback.Run(); No tests using this?
service worker lgtm https://codereview.chromium.org/2574143003/diff/140001/content/browser/servic... File content/browser/service_worker/service_worker_fetch_dispatcher.cc (right): https://codereview.chromium.org/2574143003/diff/140001/content/browser/servic... content/browser/service_worker/service_worker_fetch_dispatcher.cc:87: void OnUploadProgress(int64_t current_position, nit: Can you move this between OnDataDownloaded and OnTransferSizeUpdated to align the definition order with the declaration order in content/child/url_loader_client_impl.h? https://codereview.chromium.org/2574143003/diff/140001/content/renderer/servi... File content/renderer/service_worker/service_worker_context_client.cc (right): https://codereview.chromium.org/2574143003/diff/140001/content/renderer/servi... content/renderer/service_worker/service_worker_context_client.cc:297: void OnUploadProgress(int64_t current_position, ditto.
The CQ bit was checked by tzik@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by tzik@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by tzik@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
The CQ bit was checked by tzik@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by tzik@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by tzik@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2574143003/diff/140001/content/browser/loader... File content/browser/loader/test_url_loader_client.cc (right): https://codereview.chromium.org/2574143003/diff/140001/content/browser/loader... content/browser/loader/test_url_loader_client.cc:56: ack_callback.Run(); On 2017/01/05 16:31:20, mmenke wrote: > No tests using this? Now, it's used! https://codereview.chromium.org/2574143003/diff/140001/content/browser/servic... File content/browser/service_worker/service_worker_fetch_dispatcher.cc (right): https://codereview.chromium.org/2574143003/diff/140001/content/browser/servic... content/browser/service_worker/service_worker_fetch_dispatcher.cc:87: void OnUploadProgress(int64_t current_position, On 2017/01/06 05:30:09, nhiroki wrote: > nit: Can you move this between OnDataDownloaded and OnTransferSizeUpdated to > align the definition order with the declaration order in > content/child/url_loader_client_impl.h? Done. https://codereview.chromium.org/2574143003/diff/140001/content/renderer/servi... File content/renderer/service_worker/service_worker_context_client.cc (right): https://codereview.chromium.org/2574143003/diff/140001/content/renderer/servi... content/renderer/service_worker/service_worker_context_client.cc:297: void OnUploadProgress(int64_t current_position, On 2017/01/06 05:30:09, nhiroki wrote: > ditto. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2574143003/diff/260001/content/browser/loader... File content/browser/loader/async_resource_handler_browsertest.cc (right): https://codereview.chromium.org/2574143003/diff/260001/content/browser/loader... content/browser/loader/async_resource_handler_browsertest.cc:74: "LoadingWithMojo"); nit: Add braces. (And good job noticing we can now do this!) https://codereview.chromium.org/2574143003/diff/260001/content/browser/loader... File content/browser/loader/mojo_async_resource_handler_unittest.cc (right): https://codereview.chromium.org/2574143003/diff/260001/content/browser/loader... content/browser/loader/mojo_async_resource_handler_unittest.cc:419: SetUpRequest(); Can we do this in the constructor instead, and make SetUpRequestWithUpload just overwrite the default objects? https://codereview.chromium.org/2574143003/diff/260001/content/browser/loader... content/browser/loader/mojo_async_resource_handler_unittest.cc:596: url_loader_client_.completion_status().encoded_data_length); Should we test somewhere for the non-upload cases that we don't get upload progress? Could stick it at the end of a couple of the more complete tests, or make it part of the destructor (Though in the latter case, you'd need to figure out if the request has an upload or not - which I guess isn't hard, since we have access to request_). https://codereview.chromium.org/2574143003/diff/260001/content/browser/loader... content/browser/loader/mojo_async_resource_handler_unittest.cc:897: ASSERT_TRUE(CallOnWillStartAndOnResponseStarted()); So there are three paths to update the status, so I suggest three tests total, one for each path. We of course don't need to fully test the UploadProgressTracker, but we should test the three places it's hooked up. One where there's a redirect, make sure we get the new position without calling ResponseStarted. One where enough time passes that we get an update without the ResponseStarted call (Could set the position to something less than 1000 in that one). https://codereview.chromium.org/2574143003/diff/260001/content/browser/loader... File content/browser/loader/test_url_loader_client.cc (right): https://codereview.chromium.org/2574143003/diff/260001/content/browser/loader... content/browser/loader/test_url_loader_client.cc:67: const base::Closure& ack_callback) { Have sanity checks here? Haven't received redirects / started loading response body / isn't completed, current_position is not 0, is > current_upload_position_. and is <= total_size? Some of these are overkill for the current set of tests, just thinking they'd be nice to have if we ever add more upload tests (Due to a bug or something).
The CQ bit was checked by tzik@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by tzik@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
The CQ bit was checked by tzik@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by tzik@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2574143003/diff/260001/content/browser/loader... File content/browser/loader/async_resource_handler_browsertest.cc (right): https://codereview.chromium.org/2574143003/diff/260001/content/browser/loader... content/browser/loader/async_resource_handler_browsertest.cc:74: "LoadingWithMojo"); On 2017/01/18 17:00:46, mmenke wrote: > nit: Add braces. (And good job noticing we can now do this!) Done. https://codereview.chromium.org/2574143003/diff/260001/content/browser/loader... File content/browser/loader/mojo_async_resource_handler_unittest.cc (right): https://codereview.chromium.org/2574143003/diff/260001/content/browser/loader... content/browser/loader/mojo_async_resource_handler_unittest.cc:419: SetUpRequest(); On 2017/01/18 17:00:46, mmenke wrote: > Can we do this in the constructor instead, and make SetUpRequestWithUpload just > overwrite the default objects? Done. https://codereview.chromium.org/2574143003/diff/260001/content/browser/loader... content/browser/loader/mojo_async_resource_handler_unittest.cc:596: url_loader_client_.completion_status().encoded_data_length); On 2017/01/18 17:00:46, mmenke wrote: > Should we test somewhere for the non-upload cases that we don't get upload > progress? Could stick it at the end of a couple of the more complete tests, or > make it part of the destructor (Though in the latter case, you'd need to figure > out if the request has an upload or not - which I guess isn't hard, since we > have access to request_). Done. https://codereview.chromium.org/2574143003/diff/260001/content/browser/loader... content/browser/loader/mojo_async_resource_handler_unittest.cc:897: ASSERT_TRUE(CallOnWillStartAndOnResponseStarted()); On 2017/01/18 17:00:46, mmenke wrote: > So there are three paths to update the status, so I suggest three tests total, > one for each path. We of course don't need to fully test the > UploadProgressTracker, but we should test the three places it's hooked up. > > One where there's a redirect, make sure we get the new position without calling > ResponseStarted. > > One where enough time passes that we get an update without the ResponseStarted > call (Could set the position to something less than 1000 in that one). Done. https://codereview.chromium.org/2574143003/diff/260001/content/browser/loader... File content/browser/loader/test_url_loader_client.cc (right): https://codereview.chromium.org/2574143003/diff/260001/content/browser/loader... content/browser/loader/test_url_loader_client.cc:67: const base::Closure& ack_callback) { On 2017/01/18 17:00:46, mmenke wrote: > Have sanity checks here? Haven't received redirects / started loading response > body / isn't completed, current_position is not 0, is > > current_upload_position_. and is <= total_size? Some of these are overkill for > the current set of tests, just thinking they'd be nice to have if we ever add > more upload tests (Due to a bug or something). Done. https://codereview.chromium.org/2574143003/diff/360001/content/browser/loader... File content/browser/loader/upload_progress_tracker_unittest.cc (right): https://codereview.chromium.org/2574143003/diff/360001/content/browser/loader... content/browser/loader/upload_progress_tracker_unittest.cc:193: base::TimeDelta::FromSeconds(5)); Note: 1s is not enough to trigger the upload progress report. We need (1 + epsilon)s. Increasing this to 5s to make sure it's long enough.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM https://codereview.chromium.org/2574143003/diff/360001/content/browser/loader... File content/browser/loader/mojo_async_resource_handler.cc (right): https://codereview.chromium.org/2574143003/diff/360001/content/browser/loader... content/browser/loader/mojo_async_resource_handler.cc:205: FROM_HERE, nit: Pre-existing issue, but should include location.h for FROM_HERE. https://codereview.chromium.org/2574143003/diff/360001/content/browser/loader... File content/browser/loader/mojo_async_resource_handler.h (right): https://codereview.chromium.org/2574143003/diff/360001/content/browser/loader... content/browser/loader/mojo_async_resource_handler.h:106: const tracked_objects::Location& from_here, Should forward declare tracked_objects::Location https://codereview.chromium.org/2574143003/diff/360001/content/browser/loader... File content/browser/loader/mojo_async_resource_handler_unittest.cc (right): https://codereview.chromium.org/2574143003/diff/360001/content/browser/loader... content/browser/loader/mojo_async_resource_handler_unittest.cc:73: void ResetInternal() override { NOTREACHED(); } nit: include base/logging.h https://codereview.chromium.org/2574143003/diff/360001/content/browser/loader... content/browser/loader/mojo_async_resource_handler_unittest.cc:76: DISALLOW_COPY_AND_ASSIGN(DummyUploadDataStream); include base/macros.h https://codereview.chromium.org/2574143003/diff/360001/content/browser/loader... content/browser/loader/mojo_async_resource_handler_unittest.cc:89: base::TimeTicks current_time_; private: DISALLOW_COPY_AND_ASSIGN? https://codereview.chromium.org/2574143003/diff/360001/content/browser/loader... content/browser/loader/mojo_async_resource_handler_unittest.cc:917: // progress. Oops, sorry...When I said rewind, I actually mean a check for for both the OnResponseStarted and OnResponseCompleted OnUploadCompleted() calls (So you will need another test, or another upload in this test, to cover them body). Not that I object to the extra coverage in this test - looks great!
The CQ bit was checked by tzik@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2574143003/diff/360001/content/browser/loader... File content/browser/loader/mojo_async_resource_handler.cc (right): https://codereview.chromium.org/2574143003/diff/360001/content/browser/loader... content/browser/loader/mojo_async_resource_handler.cc:205: FROM_HERE, On 2017/01/19 16:06:49, mmenke wrote: > nit: Pre-existing issue, but should include location.h for FROM_HERE. Done. https://codereview.chromium.org/2574143003/diff/360001/content/browser/loader... File content/browser/loader/mojo_async_resource_handler.h (right): https://codereview.chromium.org/2574143003/diff/360001/content/browser/loader... content/browser/loader/mojo_async_resource_handler.h:106: const tracked_objects::Location& from_here, On 2017/01/19 16:06:49, mmenke wrote: > Should forward declare tracked_objects::Location Done. https://codereview.chromium.org/2574143003/diff/360001/content/browser/loader... File content/browser/loader/mojo_async_resource_handler_unittest.cc (right): https://codereview.chromium.org/2574143003/diff/360001/content/browser/loader... content/browser/loader/mojo_async_resource_handler_unittest.cc:73: void ResetInternal() override { NOTREACHED(); } On 2017/01/19 16:06:49, mmenke wrote: > nit: include base/logging.h Done. https://codereview.chromium.org/2574143003/diff/360001/content/browser/loader... content/browser/loader/mojo_async_resource_handler_unittest.cc:76: DISALLOW_COPY_AND_ASSIGN(DummyUploadDataStream); On 2017/01/19 16:06:49, mmenke wrote: > include base/macros.h Done. https://codereview.chromium.org/2574143003/diff/360001/content/browser/loader... content/browser/loader/mojo_async_resource_handler_unittest.cc:89: base::TimeTicks current_time_; On 2017/01/19 16:06:49, mmenke wrote: > private: > DISALLOW_COPY_AND_ASSIGN? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
+dcheng & kinuko for OWNER review. PTAL to: kinuko: //content/child dcheng: *.mojom
On 2017/01/23 07:53:16, tzik wrote: > +dcheng & kinuko for OWNER review. PTAL to: > kinuko: //content/child > dcheng: *.mojom Just in case, the reviewers list has not been updated.
kinuko@chromium.org changed reviewers: + kinuko@chromium.org
https://codereview.chromium.org/2574143003/diff/380001/content/child/url_load... File content/child/url_loader_client_impl.cc (right): https://codereview.chromium.org/2574143003/diff/380001/content/child/url_load... content/child/url_loader_client_impl.cc:184: resource_dispatcher_->OnMessageReceived( Could we / should we use Dispatch() instead?
https://codereview.chromium.org/2574143003/diff/380001/content/browser/loader... File content/browser/loader/mojo_async_resource_handler.h (right): https://codereview.chromium.org/2574143003/diff/380001/content/browser/loader... content/browser/loader/mojo_async_resource_handler.h:107: // These function can be overriden only for tests. nit: These function's' https://codereview.chromium.org/2574143003/diff/380001/content/common/url_loa... File content/common/url_loader.mojom (right): https://codereview.chromium.org/2574143003/diff/380001/content/common/url_loa... content/common/url_loader.mojom:53: // Call the response closure when the client is ready to receive the next nit: Call -> Calls ?
On 2017/01/23 07:55:32, yhirano wrote: > On 2017/01/23 07:53:16, tzik wrote: > > +dcheng & kinuko for OWNER review. PTAL to: > > kinuko: //content/child > > dcheng: *.mojom > > Just in case, the reviewers list has not been updated. ...!
The CQ bit was checked by tzik@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
tzik@chromium.org changed reviewers: + dcheng@chromium.org
+dcheng for *.mojom OWNER review. PTAL. https://codereview.chromium.org/2574143003/diff/380001/content/browser/loader... File content/browser/loader/mojo_async_resource_handler.h (right): https://codereview.chromium.org/2574143003/diff/380001/content/browser/loader... content/browser/loader/mojo_async_resource_handler.h:107: // These function can be overriden only for tests. On 2017/01/23 08:28:09, kinuko wrote: > nit: These function's' Done. https://codereview.chromium.org/2574143003/diff/380001/content/child/url_load... File content/child/url_loader_client_impl.cc (right): https://codereview.chromium.org/2574143003/diff/380001/content/child/url_load... content/child/url_loader_client_impl.cc:184: resource_dispatcher_->OnMessageReceived( On 2017/01/23 08:23:03, kinuko wrote: > Could we / should we use Dispatch() instead? Done. https://codereview.chromium.org/2574143003/diff/380001/content/common/url_loa... File content/common/url_loader.mojom (right): https://codereview.chromium.org/2574143003/diff/380001/content/common/url_loa... content/common/url_loader.mojom:53: // Call the response closure when the client is ready to receive the next On 2017/01/23 08:28:09, kinuko wrote: > nit: Call -> Calls ? Updated the comment. This is a request to the implementation of the interface.
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
mojo lgtm (question is just to help me understand the code) https://codereview.chromium.org/2574143003/diff/400001/content/browser/loader... File content/browser/loader/mojo_async_resource_handler.cc (right): https://codereview.chromium.org/2574143003/diff/400001/content/browser/loader... content/browser/loader/mojo_async_resource_handler.cc:365: upload_progress_tracker_ = nullptr; This might be a dumb question, but why do we need to check here and in OnResponseStarted? Can we skip OnResponseStarted sometimes?
https://codereview.chromium.org/2574143003/diff/400001/content/browser/loader... File content/browser/loader/mojo_async_resource_handler.cc (right): https://codereview.chromium.org/2574143003/diff/400001/content/browser/loader... content/browser/loader/mojo_async_resource_handler.cc:365: upload_progress_tracker_ = nullptr; On 2017/01/24 10:38:58, dcheng wrote: > This might be a dumb question, but why do we need to check here and in > OnResponseStarted? Can we skip OnResponseStarted sometimes? Yes, we skip it on cancellation and error cases. Added comment here.
The CQ bit was checked by tzik@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yhirano@chromium.org, nhiroki@chromium.org, mmenke@chromium.org, kinuko@chromium.org, dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/2574143003/#ps420001 (title: "+comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by yhirano@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by yhirano@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by tzik@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 420001, "attempt_start_ts": 1485270243823420,
"parent_rev": "d6eb8548aca00c0743e5d315e41f481436360dbd", "commit_rev":
"bedcbce4a048c2e6c6eaf7a3cef82eecf85543e9"}
Message was sent while issue was closed.
Description was changed from ========== Implement upload progress handling in Mojo loading This CL integrates upload progress events from the browser side loading stack in Mojo to the renderer. BUG=603396 ========== to ========== Implement upload progress handling in Mojo loading This CL integrates upload progress events from the browser side loading stack in Mojo to the renderer. BUG=603396 Review-Url: https://codereview.chromium.org/2574143003 Cr-Commit-Position: refs/heads/master@{#445727} Committed: https://chromium.googlesource.com/chromium/src/+/bedcbce4a048c2e6c6eaf7a3cef8... ==========
Message was sent while issue was closed.
Committed patchset #22 (id:420001) as https://chromium.googlesource.com/chromium/src/+/bedcbce4a048c2e6c6eaf7a3cef8... |
