Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2591383003:
Implement SetDefersLoading on content::URLLoaderClientImpl (Closed)
DescriptionImplement SetDefersLoading on content::URLLoaderClientImpl
This CL adds the deferred loading support to URLLoaderClientImpl.
BUG=603396
Committed: https://crrev.com/6a0e9c78b3be3955f3abb0d3663f506104845157
Cr-Commit-Position: refs/heads/master@{#440741}
Patch Set 1 : fix #Patch Set 2 : fix #Patch Set 3 : fix #Patch Set 4 : rebase #Patch Set 5 : fix #
Total comments: 16
Patch Set 6 : fix #Patch Set 7 : fix #
Total comments: 5
Patch Set 8 : fix #
Messages
Total messages: 47 (36 generated)
The CQ bit was checked by yhirano@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-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by yhirano@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 yhirano@chromium.org to run a CQ dry run
Patchset #1 (id:1) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #1 (id:20001) has been deleted
The CQ bit was checked by yhirano@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 yhirano@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...
yhirano@chromium.org changed reviewers: + jam@chromium.org, kinuko@chromium.org
PTAL.
Comment in https://codereview.chromium.org/2597873002/: https://codereview.chromium.org/2597873002/diff/40001/content/child/url_loade... File content/child/url_loader_client_impl.cc (right): https://codereview.chromium.org/2597873002/diff/40001/content/child/url_loade... content/child/url_loader_client_impl.cc:151: void URLLoaderClientImpl::SetDefersLoading() { Any reason we don't make this take a boolean flag as ResourceDispatcher::SetDefersLoading does? URLResponseBodyConsumer::UnsetDefersLoading contains FlushDeferredMessages and it's crucial because we need to keep the message ordering between URLLoaderClientImpl and URLResponseBodyConsumer. URLLoaderClientImpl::[Un]SetDefersLoading can be one function with a boolean parameter, though I like two functions more.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by yhirano@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 yhirano@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/2591383003/diff/120001/content/child/resource... File content/child/resource_dispatcher.cc (right): https://codereview.chromium.org/2591383003/diff/120001/content/child/resource... content/child/resource_dispatcher.cc:563: request_info->url_loader_client->FlushDeferredMessages(); Having two pending queues is confusing, it seems if we have url_loader should resource_dispatcher's deferred_message_queue is empty, is that right? If so could we DCHECK it and return here? https://codereview.chromium.org/2591383003/diff/120001/content/child/url_load... File content/child/url_loader_client_impl.cc (right): https://codereview.chromium.org/2591383003/diff/120001/content/child/url_load... content/child/url_loader_client_impl.cc:69: if (is_destroyed->data) Looking into the usage, I think using WeakPtr is probably more common? (Have a WeakPtrFactory, get a WeakPtr of this at the beginning and check if it's null later) https://codereview.chromium.org/2591383003/diff/120001/content/child/url_load... content/child/url_loader_client_impl.cc:88: messages.begin() + index, messages.end()); I'm a little confused, we don't break the for loop (except for returning in the middle), isn't the index always at the end of the messages after the for loop? https://codereview.chromium.org/2591383003/diff/120001/content/child/url_load... content/child/url_loader_client_impl.cc:174: resource_dispatcher_->OnMessageReceived(message); At this point couldn't we directly call ResourceDispatcher::DispatchMessage()? I feel it's less confusing as OnMessageReceived itself has its own deferred message handling (& sanity checks that don't seem necessary in our case). https://codereview.chromium.org/2591383003/diff/120001/content/child/url_load... File content/child/url_loader_client_impl_unittest.cc (right): https://codereview.chromium.org/2591383003/diff/120001/content/child/url_load... content/child/url_loader_client_impl_unittest.cc:466: TEST_F(URLLoaderClientImplTest, CancelRequestDuringFlusingDeferredMessage) { nit: Flusing -> Flushing https://codereview.chromium.org/2591383003/diff/120001/content/child/url_load... content/child/url_loader_client_impl_unittest.cc:514: TEST_F(URLLoaderClientImplTest, SetDeferredDuringFlusingDeferredMessage) { ditto
The CQ bit was checked by yhirano@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/2591383003/diff/120001/content/child/resource... File content/child/resource_dispatcher.cc (right): https://codereview.chromium.org/2591383003/diff/120001/content/child/resource... content/child/resource_dispatcher.cc:563: request_info->url_loader_client->FlushDeferredMessages(); On 2016/12/23 13:11:23, kinuko wrote: > Having two pending queues is confusing, it seems if we have url_loader should > resource_dispatcher's deferred_message_queue is empty, is that right? If so > could we DCHECK it and return here? I've just noticed that |request_info| can be broken here (i.e., the cancelled case). So I'm simply putting a return statement here. https://codereview.chromium.org/2591383003/diff/120001/content/child/url_load... File content/child/url_loader_client_impl.cc (right): https://codereview.chromium.org/2591383003/diff/120001/content/child/url_load... content/child/url_loader_client_impl.cc:69: if (is_destroyed->data) On 2016/12/23 13:11:23, kinuko wrote: > Looking into the usage, I think using WeakPtr is probably more common? (Have a > WeakPtrFactory, get a WeakPtr of this at the beginning and check if it's null > later) Done. https://codereview.chromium.org/2591383003/diff/120001/content/child/url_load... content/child/url_loader_client_impl.cc:88: messages.begin() + index, messages.end()); On 2016/12/23 13:11:23, kinuko wrote: > I'm a little confused, we don't break the for loop (except for returning in the > middle), isn't the index always at the end of the messages after the for loop? Oops, sorry. Fixed. https://codereview.chromium.org/2591383003/diff/120001/content/child/url_load... content/child/url_loader_client_impl.cc:174: resource_dispatcher_->OnMessageReceived(message); On 2016/12/23 13:11:23, kinuko wrote: > At this point couldn't we directly call ResourceDispatcher::DispatchMessage()? > I feel it's less confusing as OnMessageReceived itself has its own deferred > message handling (& sanity checks that don't seem necessary in our case). ResourceDispatcher::DispatchMessage is a private method and I think exposing it also has a risk that someone accidentally calls it. https://codereview.chromium.org/2591383003/diff/120001/content/child/url_load... File content/child/url_loader_client_impl_unittest.cc (right): https://codereview.chromium.org/2591383003/diff/120001/content/child/url_load... content/child/url_loader_client_impl_unittest.cc:466: TEST_F(URLLoaderClientImplTest, CancelRequestDuringFlusingDeferredMessage) { On 2016/12/23 13:11:23, kinuko wrote: > nit: Flusing -> Flushing Done. https://codereview.chromium.org/2591383003/diff/120001/content/child/url_load... content/child/url_loader_client_impl_unittest.cc:514: TEST_F(URLLoaderClientImplTest, SetDeferredDuringFlusingDeferredMessage) { On 2016/12/23 13:11:24, kinuko wrote: > ditto Done.
https://codereview.chromium.org/2591383003/diff/120001/content/child/url_load... File content/child/url_loader_client_impl.cc (right): https://codereview.chromium.org/2591383003/diff/120001/content/child/url_load... content/child/url_loader_client_impl.cc:88: messages.begin() + index, messages.end()); On 2016/12/26 05:08:35, yhirano wrote: > On 2016/12/23 13:11:23, kinuko wrote: > > I'm a little confused, we don't break the for loop (except for returning in > the > > middle), isn't the index always at the end of the messages after the for loop? > > Oops, sorry. Fixed. Thanks. Should we have a test that must have caught a bug like this? Let me make sure... so basically we're inserting ResourceMsg_RequestComplete which must be messages.back(), am I following? I feel just pushing back the last one might be clearer, but what do you think? https://codereview.chromium.org/2591383003/diff/120001/content/child/url_load... content/child/url_loader_client_impl.cc:174: resource_dispatcher_->OnMessageReceived(message); On 2016/12/26 05:08:35, yhirano wrote: > On 2016/12/23 13:11:23, kinuko wrote: > > At this point couldn't we directly call ResourceDispatcher::DispatchMessage()? > > > I feel it's less confusing as OnMessageReceived itself has its own deferred > > message handling (& sanity checks that don't seem necessary in our case). > > ResourceDispatcher::DispatchMessage is a private method and I think exposing it > also has a risk that someone accidentally calls it. But eventually we plan to get rid of OnMessageReceived and the non-mojo code, right? I don't want to leave the possibility that the new code weirdly depends on the old code that's supposed to be go away. If you're uncomfortable could we add another public method with Mojo suffix, or making URLLoaderClientImpl friend of RD?
The CQ bit was checked by yhirano@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/2591383003/diff/120001/content/child/url_load... File content/child/url_loader_client_impl.cc (right): https://codereview.chromium.org/2591383003/diff/120001/content/child/url_load... content/child/url_loader_client_impl.cc:88: messages.begin() + index, messages.end()); On 2016/12/26 07:21:17, kinuko wrote: > On 2016/12/26 05:08:35, yhirano wrote: > > On 2016/12/23 13:11:23, kinuko wrote: > > > I'm a little confused, we don't break the for loop (except for returning in > > the > > > middle), isn't the index always at the end of the messages after the for > loop? > > > > Oops, sorry. Fixed. > > Thanks. Should we have a test that must have caught a bug like this? > > Let me make sure... so basically we're inserting ResourceMsg_RequestComplete > which must be messages.back(), am I following? I feel just pushing back the > last one might be clearer, but what do you think? Done. https://codereview.chromium.org/2591383003/diff/120001/content/child/url_load... content/child/url_loader_client_impl.cc:174: resource_dispatcher_->OnMessageReceived(message); On 2016/12/26 07:21:17, kinuko wrote: > On 2016/12/26 05:08:35, yhirano wrote: > > On 2016/12/23 13:11:23, kinuko wrote: > > > At this point couldn't we directly call > ResourceDispatcher::DispatchMessage()? > > > > > I feel it's less confusing as OnMessageReceived itself has its own deferred > > > message handling (& sanity checks that don't seem necessary in our case). > > > > ResourceDispatcher::DispatchMessage is a private method and I think exposing > it > > also has a risk that someone accidentally calls it. > > But eventually we plan to get rid of OnMessageReceived and the non-mojo code, > right? I don't want to leave the possibility that the new code weirdly depends > on the old code that's supposed to be go away. If you're uncomfortable could we > add another public method with Mojo suffix, or making URLLoaderClientImpl friend > of RD? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks lgtm https://codereview.chromium.org/2591383003/diff/160001/content/child/url_load... File content/child/url_loader_client_impl.cc (right): https://codereview.chromium.org/2591383003/diff/160001/content/child/url_load... content/child/url_loader_client_impl.cc:86: deferred_messages_.emplace_back(std::move(messages.back())); nit: can we add a comment (or documentary DCHECK) to make it clear it's RequestComplete? https://codereview.chromium.org/2591383003/diff/160001/content/child/url_load... File content/child/url_loader_client_impl_unittest.cc (right): https://codereview.chromium.org/2591383003/diff/160001/content/child/url_load... content/child/url_loader_client_impl_unittest.cc:530: SetDeferredDuringFlushingDeferredMessageOnTransferSizeUpdated) { :) https://codereview.chromium.org/2591383003/diff/160001/content/child/url_resp... File content/child/url_response_body_consumer.cc (right): https://codereview.chromium.org/2591383003/diff/160001/content/child/url_resp... content/child/url_response_body_consumer.cc:145: resource_dispatcher_->OnMessageReceived( No need to be in the same change, but I think this can (should) use DispatchMessage too.
https://codereview.chromium.org/2591383003/diff/160001/content/child/url_load... File content/child/url_loader_client_impl.cc (right): https://codereview.chromium.org/2591383003/diff/160001/content/child/url_load... content/child/url_loader_client_impl.cc:86: deferred_messages_.emplace_back(std::move(messages.back())); On 2016/12/27 01:52:21, kinuko wrote: > nit: can we add a comment (or documentary DCHECK) to make it clear it's > RequestComplete? Done. https://codereview.chromium.org/2591383003/diff/160001/content/child/url_resp... File content/child/url_response_body_consumer.cc (right): https://codereview.chromium.org/2591383003/diff/160001/content/child/url_resp... content/child/url_response_body_consumer.cc:145: resource_dispatcher_->OnMessageReceived( On 2016/12/27 01:52:21, kinuko wrote: > No need to be in the same change, but I think this can (should) use > DispatchMessage too. Done.
The CQ bit was checked by yhirano@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kinuko@chromium.org Link to the patchset: https://codereview.chromium.org/2591383003/#ps180001 (title: "fix")
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": 180001, "attempt_start_ts": 1482823983042220,
"parent_rev": "36d78edaf3dfb10555eb8850261f788f389b468c", "commit_rev":
"b9fdd8e40ce74d3c1ee96be538bcdcc0bc7cb099"}
Message was sent while issue was closed.
Description was changed from ========== Implement SetDefersLoading on content::URLLoaderClientImpl This CL adds the deferred loading support to URLLoaderClientImpl. BUG=603396 ========== to ========== Implement SetDefersLoading on content::URLLoaderClientImpl This CL adds the deferred loading support to URLLoaderClientImpl. BUG=603396 Review-Url: https://codereview.chromium.org/2591383003 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== Implement SetDefersLoading on content::URLLoaderClientImpl This CL adds the deferred loading support to URLLoaderClientImpl. BUG=603396 Review-Url: https://codereview.chromium.org/2591383003 ========== to ========== Implement SetDefersLoading on content::URLLoaderClientImpl This CL adds the deferred loading support to URLLoaderClientImpl. BUG=603396 Committed: https://crrev.com/6a0e9c78b3be3955f3abb0d3663f506104845157 Cr-Commit-Position: refs/heads/master@{#440741} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/6a0e9c78b3be3955f3abb0d3663f506104845157 Cr-Commit-Position: refs/heads/master@{#440741} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
