Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(239)

Issue 1285863003: ResourceScheduler: remove dependency on ResourceRequestInfo and request_id (Closed)

Created:
5 years, 4 months ago by Adam Rice
Modified:
5 years, 4 months ago
Reviewers:
davidben, mmenke
CC:
chromium-reviews, darin-cc_chromium.org, jam
Base URL:
https://chromium.googlesource.com/chromium/src.git@lkgr
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

ResourceScheduler: remove dependency on ResourceRequestInfo and request_id For async revalidations to work independently of ResourceDispatcherHostImpl after creation, it is necessary that they have no ResourceRequestInfoImpl objects attached to them. However, we still need ResourceScheduler to be able to schedule these requests. This CL makes ScheduledResourceRequest a public interface which is returned by ResourceScheduler::ScheduleResource(). The code to handle ResourceHostMsg_DidChangePriority is now inside ResourceDispatcherHostImpl instead of being delegated to ResourceMessageDelegates. A pointer to the ScheduledResourceRequest is attached to the URLRequest so that ResourceDispatcherHostImpl can retrieve it when a DidChangePriority message is received. BUG=348877 TEST=content_unittests Committed: https://crrev.com/6e001fc816a85f6815e286398e870b8e86e09703 Cr-Commit-Position: refs/heads/master@{#343442}

Patch Set 1 #

Patch Set 2 : Fix tests. #

Total comments: 8

Patch Set 3 : Add test that DidChangePriority messages work. #

Patch Set 4 : Remove ScheduledResourceRequest pointer from ResourceRequestInfoImpl. #

Patch Set 5 : Minor fixes. #

Total comments: 13

Patch Set 6 : Changes suggested by davidben@ #

Total comments: 8

Patch Set 7 : Make ResourceScheduler::ReprioritizeRequest public and take a URLRequest. #

Patch Set 8 : Rename resource_throttle variables back to throttle. #

Total comments: 2

Patch Set 9 : DCHECK that there isn't an existing ScheduledResourceRequest attached to the URLRequest. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+190 lines, -149 lines) Patch
M content/browser/loader/resource_dispatcher_host_impl.h View 2 chunks +4 lines, -0 lines 0 comments Download
M content/browser/loader/resource_dispatcher_host_impl.cc View 1 2 3 4 5 6 3 chunks +19 lines, -2 lines 0 comments Download
M content/browser/loader/resource_dispatcher_host_unittest.cc View 1 2 3 4 5 3 chunks +54 lines, -0 lines 0 comments Download
M content/browser/loader/resource_scheduler.h View 1 2 3 4 5 6 5 chunks +16 lines, -16 lines 0 comments Download
M content/browser/loader/resource_scheduler.cc View 1 2 3 4 5 6 7 8 11 chunks +62 lines, -39 lines 0 comments Download
M content/browser/loader/resource_scheduler_unittest.cc View 1 2 3 4 5 6 7 10 chunks +35 lines, -92 lines 0 comments Download

Messages

Total messages: 17 (4 generated)
davidben
Thanks! I like that the tests can now be detached from the RDH too. :-) ...
5 years, 4 months ago (2015-08-12 23:58:11 UTC) #2
Adam Rice
https://codereview.chromium.org/1285863003/diff/20001/content/browser/loader/resource_dispatcher_host_impl.cc File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/1285863003/diff/20001/content/browser/loader/resource_dispatcher_host_impl.cc#newcode1444 content/browser/loader/resource_dispatcher_host_impl.cc:1444: info->set_scheduled_resource_request(scheduled_resource_request.get()); On 2015/08/12 23:58:11, David Benjamin wrote: > There ...
5 years, 4 months ago (2015-08-13 21:21:02 UTC) #3
davidben
Some comments below, but lgtm! https://codereview.chromium.org/1285863003/diff/20001/content/browser/loader/resource_dispatcher_host_impl.cc File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/1285863003/diff/20001/content/browser/loader/resource_dispatcher_host_impl.cc#newcode1444 content/browser/loader/resource_dispatcher_host_impl.cc:1444: info->set_scheduled_resource_request(scheduled_resource_request.get()); On 2015/08/13 21:21:02, ...
5 years, 4 months ago (2015-08-13 23:08:53 UTC) #4
Adam Rice
https://codereview.chromium.org/1285863003/diff/20001/content/browser/loader/resource_dispatcher_host_impl.cc File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/1285863003/diff/20001/content/browser/loader/resource_dispatcher_host_impl.cc#newcode1444 content/browser/loader/resource_dispatcher_host_impl.cc:1444: info->set_scheduled_resource_request(scheduled_resource_request.get()); On 2015/08/13 23:08:52, David Benjamin wrote: > On ...
5 years, 4 months ago (2015-08-13 23:42:28 UTC) #5
Adam Rice
+mmenke, PTAL.
5 years, 4 months ago (2015-08-13 23:45:56 UTC) #7
davidben
https://codereview.chromium.org/1285863003/diff/80001/content/browser/loader/resource_dispatcher_host_unittest.cc File content/browser/loader/resource_dispatcher_host_unittest.cc (right): https://codereview.chromium.org/1285863003/diff/80001/content/browser/loader/resource_dispatcher_host_unittest.cc#newcode3372 content/browser/loader/resource_dispatcher_host_unittest.cc:3372: On 2015/08/13 23:42:28, Adam Rice wrote: > On 2015/08/13 ...
5 years, 4 months ago (2015-08-14 02:14:28 UTC) #8
mmenke
https://codereview.chromium.org/1285863003/diff/100001/content/browser/loader/resource_scheduler.cc File content/browser/loader/resource_scheduler.cc (right): https://codereview.chromium.org/1285863003/diff/100001/content/browser/loader/resource_scheduler.cc#newcode258 content/browser/loader/resource_scheduler.cc:258: UnownedPointer(ScheduledResourceRequestImpl* pointer) : pointer(pointer) {} explicit https://codereview.chromium.org/1285863003/diff/100001/content/browser/loader/resource_scheduler.cc#newcode260 content/browser/loader/resource_scheduler.cc:260: ScheduledResourceRequestImpl* ...
5 years, 4 months ago (2015-08-14 15:23:15 UTC) #9
Adam Rice
https://codereview.chromium.org/1285863003/diff/100001/content/browser/loader/resource_scheduler.cc File content/browser/loader/resource_scheduler.cc (right): https://codereview.chromium.org/1285863003/diff/100001/content/browser/loader/resource_scheduler.cc#newcode258 content/browser/loader/resource_scheduler.cc:258: UnownedPointer(ScheduledResourceRequestImpl* pointer) : pointer(pointer) {} On 2015/08/14 15:23:14, mmenke ...
5 years, 4 months ago (2015-08-14 17:17:23 UTC) #10
mmenke
LGTM https://codereview.chromium.org/1285863003/diff/140001/content/browser/loader/resource_scheduler.cc File content/browser/loader/resource_scheduler.cc (right): https://codereview.chromium.org/1285863003/diff/140001/content/browser/loader/resource_scheduler.cc#newcode182 content/browser/loader/resource_scheduler.cc:182: request_->SetUserData(kUserDataKey, new UnownedPointer(this)); Maybe DCHECK that it doesn't ...
5 years, 4 months ago (2015-08-14 17:24:26 UTC) #11
Adam Rice
https://codereview.chromium.org/1285863003/diff/140001/content/browser/loader/resource_scheduler.cc File content/browser/loader/resource_scheduler.cc (right): https://codereview.chromium.org/1285863003/diff/140001/content/browser/loader/resource_scheduler.cc#newcode182 content/browser/loader/resource_scheduler.cc:182: request_->SetUserData(kUserDataKey, new UnownedPointer(this)); On 2015/08/14 17:24:25, mmenke wrote: > ...
5 years, 4 months ago (2015-08-14 17:30:03 UTC) #12
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1285863003/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1285863003/160001
5 years, 4 months ago (2015-08-14 17:30:34 UTC) #15
commit-bot: I haz the power
Committed patchset #9 (id:160001)
5 years, 4 months ago (2015-08-14 18:25:37 UTC) #16
commit-bot: I haz the power
5 years, 4 months ago (2015-08-14 18:26:33 UTC) #17
Message was sent while issue was closed.
Patchset 9 (id:??) landed as
https://crrev.com/6e001fc816a85f6815e286398e870b8e86e09703
Cr-Commit-Position: refs/heads/master@{#343442}

Powered by Google App Engine
This is Rietveld 408576698