Add a ResourceScheduler to ResourceDispatcherHost.

Add a ResourceScheduler to ResourceDispatcherHost.

For this CL, ResourceScheduler mimicks WebKit's ResourceLoadScheduler. That means only JS and CSS are loaded before first paint.

Eventually, we will improve it to do things like:
- Lower priority of background tabs.
- Preconnect for low priority resources.
- Dynamically adapt scheduling to the user's connection.
- Experiment with other scheduling ideas.

BUG=157763
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=183382

[willchan no longer on Chromium, 2012-10-25 19:44:49]

Please update the changelist description to explain what behavior it's mimic'ing
from the WebKit scheduler.

Architecturally, the one thing that makes me nervous here is the duplication of
state for the requests in ResourceScheduler and ResourceDispatcherHost. I'm
worried that there are use-after-frees and what not. For example, when a request
is canceled, are you sure it'll lead to ResourceScheduler::RemoveLoad() getting
called?

I don't think we should plan to get this in for Chrome 24. I think we'll have a
few issues stabilizing this change, so we should do that in Chrome 25.

https://codereview.chromium.org/11270027/diff/1/content/browser/renderer_host...
File content/browser/renderer_host/resource_scheduler.cc (right):

https://codereview.chromium.org/11270027/diff/1/content/browser/renderer_host...
content/browser/renderer_host/resource_scheduler.cc:17: return
static_cast<int64>(child_id) << 32 | route_id;
I don't memorize the order of operations between << and |, please add a
parentheses to clarify :)

https://codereview.chromium.org/11270027/diff/1/content/browser/renderer_host...
content/browser/renderer_host/resource_scheduler.cc:33: if
(!ContainsKey(client_map_, client_id)) {
I don't understand this. If the client doesn't exist, then we always start the
request? Is that because it's the first request for the client and we never want
to hold it back? What if we are loading a bunch of tabs in the background?

https://codereview.chromium.org/11270027/diff/1/content/browser/renderer_host...
content/browser/renderer_host/resource_scheduler.cc:51: if
(!ContainsKey(client_map_, client_id)) {
Why would this ever happen? Is this a bug? Should it be NOTREACHED()?

https://codereview.chromium.org/11270027/diff/1/content/browser/renderer_host...
File content/browser/renderer_host/resource_scheduler.h (right):

https://codereview.chromium.org/11270027/diff/1/content/browser/renderer_host...
content/browser/renderer_host/resource_scheduler.h:38: // TODO(simonjam): This'd
be CONTENT_EXPORT_PRIVATE if we had such a thing.
Why don't we have this? Aren't we trying to write more content unittests?

https://codereview.chromium.org/11270027/diff/1/content/browser/renderer_host...
File content/browser/renderer_host/resource_scheduler_filter.h (right):

https://codereview.chromium.org/11270027/diff/1/content/browser/renderer_host...
content/browser/renderer_host/resource_scheduler_filter.h:20:
ResourceSchedulerFilter(int child_id);
explicit

[James Simonsen, 2012-11-17 02:56:15]

LoadHandles now make sure the ResourceDispatcher releases everything as needed.
Darin says the IPC messages are sufficient for tracking renderer state, because
that's what RenderViewHost relies on.

https://codereview.chromium.org/11270027/diff/1/content/browser/renderer_host...
File content/browser/renderer_host/resource_scheduler.cc (right):

https://codereview.chromium.org/11270027/diff/1/content/browser/renderer_host...
content/browser/renderer_host/resource_scheduler.cc:17: return
static_cast<int64>(child_id) << 32 | route_id;
On 2012/10/25 19:44:49, willchan wrote:
> I don't memorize the order of operations between << and |, please add a
> parentheses to clarify :)

Done.

https://codereview.chromium.org/11270027/diff/1/content/browser/renderer_host...
content/browser/renderer_host/resource_scheduler.cc:33: if
(!ContainsKey(client_map_, client_id)) {
On 2012/10/25 19:44:49, willchan wrote:
> I don't understand this. If the client doesn't exist, then we always start the
> request? Is that because it's the first request for the client and we never
want
> to hold it back? What if we are loading a bunch of tabs in the background?

Turns out these were the <a ping> requests. I added a comment and explicitly
handle them.

https://codereview.chromium.org/11270027/diff/1/content/browser/renderer_host...
content/browser/renderer_host/resource_scheduler.cc:51: if
(!ContainsKey(client_map_, client_id)) {
On 2012/10/25 19:44:49, willchan wrote:
> Why would this ever happen? Is this a bug? Should it be NOTREACHED()?

When a tab goes away suddenly the OnDestroy() comes in first, then the
ResourceDispatcher clears out all of that tab's requests. I've explicitly
handled that case now.

https://codereview.chromium.org/11270027/diff/1/content/browser/renderer_host...
File content/browser/renderer_host/resource_scheduler.h (right):

https://codereview.chromium.org/11270027/diff/1/content/browser/renderer_host...
content/browser/renderer_host/resource_scheduler.h:38: // TODO(simonjam): This'd
be CONTENT_EXPORT_PRIVATE if we had such a thing.
On 2012/10/25 19:44:49, willchan wrote:
> Why don't we have this? Aren't we trying to write more content unittests?

Done.

https://codereview.chromium.org/11270027/diff/1/content/browser/renderer_host...
File content/browser/renderer_host/resource_scheduler_filter.h (right):

https://codereview.chromium.org/11270027/diff/1/content/browser/renderer_host...
content/browser/renderer_host/resource_scheduler_filter.h:20:
ResourceSchedulerFilter(int child_id);
On 2012/10/25 19:44:49, willchan wrote:
> explicit

Done.

[willchan no longer on Chromium, 2012-11-21 09:04:30]

https://codereview.chromium.org/11270027/diff/7001/content/browser/renderer_h...
File content/browser/renderer_host/resource_dispatcher_host_impl.h (right):

https://codereview.chromium.org/11270027/diff/7001/content/browser/renderer_h...
content/browser/renderer_host/resource_dispatcher_host_impl.h:385: typedef
std::vector<linked_ptr<ResourceLoader> > BlockedLoadersList;
This confuses me. Who owns the ResourceLoader? Why is there a linked_ptr to it
here and in ResourceScheduler's load handles?

https://codereview.chromium.org/11270027/diff/7001/content/browser/renderer_h...
content/browser/renderer_host/resource_dispatcher_host_impl.h:421:
scoped_ptr<ResourceScheduler> resource_scheduler_;
Why bother with a scoped_ptr?

https://codereview.chromium.org/11270027/diff/7001/content/browser/renderer_h...
File content/browser/renderer_host/resource_loader.h (right):

https://codereview.chromium.org/11270027/diff/7001/content/browser/renderer_h...
content/browser/renderer_host/resource_loader.h:28: CONTENT_EXPORT_PRIVATE
ResourceLoader(scoped_ptr<net::URLRequest> request,
Don't we generally just export the whole class? I defer to jam@ here.

https://codereview.chromium.org/11270027/diff/7001/content/browser/renderer_h...
File content/browser/renderer_host/resource_scheduler.h (right):

https://codereview.chromium.org/11270027/diff/7001/content/browser/renderer_h...
content/browser/renderer_host/resource_scheduler.h:61: LoadHandle*
ScheduleLoad(int child_id,
Perhaps return a scoped_ptr<LoadHandle> to convey who owns this?

https://codereview.chromium.org/11270027/diff/7001/content/browser/renderer_h...
content/browser/renderer_host/resource_scheduler.h:104: class
ScheduledLoadHandle : public LoadHandle {
Move these subclasses to the .cc file to keep the header file clean.

https://codereview.chromium.org/11270027/diff/7001/content/browser/renderer_h...
content/browser/renderer_host/resource_scheduler.h:112: virtual
linked_ptr<ResourceLoader>& loader() OVERRIDE { return loader_; }
no returning non-const references. probably best just to copy the linked_ptr.

https://codereview.chromium.org/11270027/diff/7001/content/browser/renderer_h...
content/browser/renderer_host/resource_scheduler.h:121: // A load that fires
immediately and is ignored by the ResourceScheduler.
I think this class shouldn't exist and we should just use ScheduledLoadHandle. I
think in the future we may want to schedule it. For example, for mobile, there's
no reason to wake up the radio just to send this silly thing. We should enable
scheduling of "background" requests in the future. I guess it could be argued
that this should be done in the network stack proper though. Let me go to sleep
and think about it tomorrow.

https://codereview.chromium.org/11270027/diff/7001/content/browser/renderer_h...
File content/browser/renderer_host/resource_scheduler_filter.cc (right):

https://codereview.chromium.org/11270027/diff/7001/content/browser/renderer_h...
content/browser/renderer_host/resource_scheduler_filter.cc:1: // Copyright (c)
2012 The Chromium Authors. All rights reserved.
I don't know the browser<=>renderer IPCs very well. This looks reasonable, but
you should get Darin to review this.

https://codereview.chromium.org/11270027/diff/7001/content/browser/renderer_h...
File content/browser/renderer_host/resource_scheduler_unittest.cc (right):

https://codereview.chromium.org/11270027/diff/7001/content/browser/renderer_h...
content/browser/renderer_host/resource_scheduler_unittest.cc:151:
TEST_F(ResourceSchedulerTest, OneIsolatedRequest) {
OneIsolatedLowRequest

[James Simonsen, 2012-11-27 02:20:51]

https://codereview.chromium.org/11270027/diff/7001/content/browser/renderer_h...
File content/browser/renderer_host/resource_dispatcher_host_impl.h (right):

https://codereview.chromium.org/11270027/diff/7001/content/browser/renderer_h...
content/browser/renderer_host/resource_dispatcher_host_impl.h:385: typedef
std::vector<linked_ptr<ResourceLoader> > BlockedLoadersList;
On 2012/11/21 09:04:30, willchan wrote:
> This confuses me. Who owns the ResourceLoader? Why is there a linked_ptr to it
> here and in ResourceScheduler's load handles?

Both, but only one at a time. A loader is either blocked or  pending.

https://codereview.chromium.org/11270027/diff/7001/content/browser/renderer_h...
content/browser/renderer_host/resource_dispatcher_host_impl.h:421:
scoped_ptr<ResourceScheduler> resource_scheduler_;
On 2012/11/21 09:04:30, willchan wrote:
> Why bother with a scoped_ptr?

I thought the leak checker would complain during the unit tests. Maybe not.

https://codereview.chromium.org/11270027/diff/7001/content/browser/renderer_h...
File content/browser/renderer_host/resource_loader.h (right):

https://codereview.chromium.org/11270027/diff/7001/content/browser/renderer_h...
content/browser/renderer_host/resource_loader.h:28: CONTENT_EXPORT_PRIVATE
ResourceLoader(scoped_ptr<net::URLRequest> request,
On 2012/11/21 09:04:30, willchan wrote:
> Don't we generally just export the whole class? I defer to jam@ here.

I saw examples of each. Not sure which is preferred.

https://codereview.chromium.org/11270027/diff/7001/content/browser/renderer_h...
File content/browser/renderer_host/resource_scheduler.h (right):

https://codereview.chromium.org/11270027/diff/7001/content/browser/renderer_h...
content/browser/renderer_host/resource_scheduler.h:61: LoadHandle*
ScheduleLoad(int child_id,
On 2012/11/21 09:04:30, willchan wrote:
> Perhaps return a scoped_ptr<LoadHandle> to convey who owns this?

Done.

https://codereview.chromium.org/11270027/diff/7001/content/browser/renderer_h...
content/browser/renderer_host/resource_scheduler.h:104: class
ScheduledLoadHandle : public LoadHandle {
On 2012/11/21 09:04:30, willchan wrote:
> Move these subclasses to the .cc file to keep the header file clean.

Done.

https://codereview.chromium.org/11270027/diff/7001/content/browser/renderer_h...
content/browser/renderer_host/resource_scheduler.h:112: virtual
linked_ptr<ResourceLoader>& loader() OVERRIDE { return loader_; }
On 2012/11/21 09:04:30, willchan wrote:
> no returning non-const references. probably best just to copy the linked_ptr.

Done.

https://codereview.chromium.org/11270027/diff/7001/content/browser/renderer_h...
content/browser/renderer_host/resource_scheduler.h:121: // A load that fires
immediately and is ignored by the ResourceScheduler.
On 2012/11/21 09:04:30, willchan wrote:
> I think this class shouldn't exist and we should just use ScheduledLoadHandle.
I
> think in the future we may want to schedule it. For example, for mobile,
there's
> no reason to wake up the radio just to send this silly thing. We should enable
> scheduling of "background" requests in the future. I guess it could be argued
> that this should be done in the network stack proper though. Let me go to
sleep
> and think about it tomorrow.

Good point. I prefer to handle the "background" requests here instead of the
network layer. You could imagine a user loading several windows in parallel.
ResourceScheduler is in the best position to determine that and throttle the
background requests.

https://codereview.chromium.org/11270027/diff/7001/content/browser/renderer_h...
File content/browser/renderer_host/resource_scheduler_unittest.cc (right):

https://codereview.chromium.org/11270027/diff/7001/content/browser/renderer_h...
content/browser/renderer_host/resource_scheduler_unittest.cc:151:
TEST_F(ResourceSchedulerTest, OneIsolatedRequest) {
On 2012/11/21 09:04:30, willchan wrote:
> OneIsolatedLowRequest

Done.

[willchan no longer on Chromium, 2012-11-30 10:16:20]

https://codereview.chromium.org/11270027/diff/7001/content/browser/renderer_h...
File content/browser/renderer_host/resource_dispatcher_host_impl.h (right):

https://codereview.chromium.org/11270027/diff/7001/content/browser/renderer_h...
content/browser/renderer_host/resource_dispatcher_host_impl.h:421:
scoped_ptr<ResourceScheduler> resource_scheduler_;
On 2012/11/27 02:20:51, James Simonsen wrote:
> On 2012/11/21 09:04:30, willchan wrote:
> > Why bother with a scoped_ptr?
> 
> I thought the leak checker would complain during the unit tests. Maybe not.

No, I mean, why isn't the whole object just embedded as a member variable. Why
use a pointer?

[willchan no longer on Chromium, 2012-12-03 02:30:16]

The memory ownership stuff is all confusing to me. We need to fix this. Here's
what I propose:

* ResourceDispatcherHostImpl owns the ResourceLoaders.
* ResourceLoader owns a ResourceScheduler::LoadHandle. Probably should add a
ResourceLoader::Schedule(ResourceScheduler*) function.

No more linked_ptrs.

https://codereview.chromium.org/11270027/diff/7001/content/browser/renderer_h...
File content/browser/renderer_host/resource_dispatcher_host_impl.h (right):

https://codereview.chromium.org/11270027/diff/7001/content/browser/renderer_h...
content/browser/renderer_host/resource_dispatcher_host_impl.h:385: typedef
std::vector<linked_ptr<ResourceLoader> > BlockedLoadersList;
On 2012/11/27 02:20:51, James Simonsen wrote:
> On 2012/11/21 09:04:30, willchan wrote:
> > This confuses me. Who owns the ResourceLoader? Why is there a linked_ptr to
it
> > here and in ResourceScheduler's load handles?
> 
> Both, but only one at a time. A loader is either blocked or  pending.

I think we should use ScopedVector instead, and weak_erase()/erase().

https://codereview.chromium.org/11270027/diff/14001/content/browser/renderer_...
File content/browser/renderer_host/resource_dispatcher_host_impl.cc (right):

https://codereview.chromium.org/11270027/diff/14001/content/browser/renderer_...
content/browser/renderer_host/resource_dispatcher_host_impl.cc:711:
ResourceLoader* loader) {
Not obvious to me why you made this change, but OK

https://codereview.chromium.org/11270027/diff/14001/content/browser/renderer_...
content/browser/renderer_host/resource_dispatcher_host_impl.cc:880:
linked_ptr<ResourceLoader> deferred_loader;
All this linked_ptr stuff annoys me. I want scoped_ptrs with well defined
ownership :)

https://codereview.chromium.org/11270027/diff/14001/content/browser/renderer_...
content/browser/renderer_host/resource_dispatcher_host_impl.cc:1295: void
ResourceDispatcherHostImpl::CancelRequestsForProcess(int child_id) {
How does <a ping> work here? AFAICT, it's broken.

https://codereview.chromium.org/11270027/diff/14001/content/browser/renderer_...
File content/browser/renderer_host/resource_dispatcher_host_impl.h (right):

https://codereview.chromium.org/11270027/diff/14001/content/browser/renderer_...
content/browser/renderer_host/resource_dispatcher_host_impl.h:421:
ResourceScheduler* resource_scheduler_;
I commented elsewhere, but yeah, just embed the full object here. No need to use
a pointer.

https://codereview.chromium.org/11270027/diff/14001/content/browser/renderer_...
File content/browser/renderer_host/resource_scheduler.cc (right):

https://codereview.chromium.org/11270027/diff/14001/content/browser/renderer_...
content/browser/renderer_host/resource_scheduler.cc:46: BackgroundRequestId
request_id_;
private:

https://codereview.chromium.org/11270027/diff/14001/content/browser/renderer_...
content/browser/renderer_host/resource_scheduler.cc:93: loader->StartRequest();
I'm not sure if all URLRequestJobs complete asynchronously. If not, then this
may synchronously call back into ResourceDispatcherHostImpl and delete the
loader. I need to read through the code to make sure everything's safe.

https://codereview.chromium.org/11270027/diff/14001/content/browser/renderer_...
File content/browser/renderer_host/resource_scheduler.h (right):

https://codereview.chromium.org/11270027/diff/14001/content/browser/renderer_...
content/browser/renderer_host/resource_scheduler.h:66: const
linked_ptr<ResourceLoader>& loader);
This should take a ResourceLoader* and not increment the refcount (linked_ptr is
a refcounting mechanism) since ResourceScheduler does not own the
ResourceLoader, ResourceDispatcherHostImpl does.

https://codereview.chromium.org/11270027/diff/14001/content/browser/renderer_...
content/browser/renderer_host/resource_scheduler.h:99: Client();
How about declaring the destructor here and defining it in the .cc file while
you're at it, so we don't get compiler generated inlined destructor STL code
bloat.

https://codereview.chromium.org/11270027/diff/14001/content/browser/renderer_...
content/browser/renderer_host/resource_scheduler.h:110: // Called when a
BackgroundLoadHandle is destroyed.
I think we should kill off this function. I think that in the future we may want
to migrate resource loads between normal priority scheduling and background
scheduling. I think RemoveLoad() should search all the maps/sets to figure
remove it, and we shouldn't need different LoadHandle subclasses to specify
which type of load is getting removed.

That's my random thought because I am imagining what we might want to do in
cases like session restore and prioritizing foreground tabs over background tabs
and stuff. I think the LoadHandle shouldn't be aware of the priority level /
load category, that should all be known by the ResourceScheduler.

Also, you're coupling background requests pretty tightly with the lack of a
client_id. Is that desirable? I feel like in the future we may have background
requests that still have a client_id.

[willchan no longer on Chromium, 2012-12-03 02:33:30]

https://codereview.chromium.org/11270027/diff/14001/content/browser/renderer_...
File content/browser/renderer_host/resource_scheduler_unittest.cc (right):

https://codereview.chromium.org/11270027/diff/14001/content/browser/renderer_...
content/browser/renderer_host/resource_scheduler_unittest.cc:10: #include
"content/browser/renderer_host/resource_scheduler.h"
This should go first

https://codereview.chromium.org/11270027/diff/14001/content/browser/renderer_...
content/browser/renderer_host/resource_scheduler_unittest.cc:25: //
ResourceHandler interface
public:

https://codereview.chromium.org/11270027/diff/14001/content/browser/renderer_...
content/browser/renderer_host/resource_scheduler_unittest.cc:133: }  // unnamed
namespace
Why not put everything in the anonymous namespace? TEST_Fs can go in there too.

https://codereview.chromium.org/11270027/diff/14001/content/common/content_ex...
File content/common/content_export.h (right):

https://codereview.chromium.org/11270027/diff/14001/content/common/content_ex...
content/common/content_export.h:13: #define CONTENT_EXPORT_PRIVATE
__declspec(dllexport)
Please pull this out to a separate CL.

[James Simonsen, 2012-12-05 01:52:45]

https://codereview.chromium.org/11270027/diff/14001/content/browser/renderer_...
File content/browser/renderer_host/resource_dispatcher_host_impl.cc (right):

https://codereview.chromium.org/11270027/diff/14001/content/browser/renderer_...
content/browser/renderer_host/resource_dispatcher_host_impl.cc:711:
ResourceLoader* loader) {
On 2012/12/03 02:30:16, willchan wrote:
> Not obvious to me why you made this change, but OK

Done.

https://codereview.chromium.org/11270027/diff/14001/content/browser/renderer_...
content/browser/renderer_host/resource_dispatcher_host_impl.cc:880:
linked_ptr<ResourceLoader> deferred_loader;
On 2012/12/03 02:30:16, willchan wrote:
> All this linked_ptr stuff annoys me. I want scoped_ptrs with well defined
> ownership :)

Done. LoaderMap still uses them. We could conceivably come up with a ScopedMap,
but that seems excessive.

https://codereview.chromium.org/11270027/diff/14001/content/browser/renderer_...
content/browser/renderer_host/resource_dispatcher_host_impl.cc:1295: void
ResourceDispatcherHostImpl::CancelRequestsForProcess(int child_id) {
On 2012/12/03 02:30:16, willchan wrote:
> How does <a ping> work here? AFAICT, it's broken.

How so? They have a child_id, but no route_id. They still end up in the map, and
the -1 will match them.

https://codereview.chromium.org/11270027/diff/14001/content/browser/renderer_...
File content/browser/renderer_host/resource_dispatcher_host_impl.h (right):

https://codereview.chromium.org/11270027/diff/14001/content/browser/renderer_...
content/browser/renderer_host/resource_dispatcher_host_impl.h:421:
ResourceScheduler* resource_scheduler_;
On 2012/12/03 02:30:16, willchan wrote:
> I commented elsewhere, but yeah, just embed the full object here. No need to
use
> a pointer.

Done.

https://codereview.chromium.org/11270027/diff/14001/content/browser/renderer_...
File content/browser/renderer_host/resource_scheduler.cc (right):

https://codereview.chromium.org/11270027/diff/14001/content/browser/renderer_...
content/browser/renderer_host/resource_scheduler.cc:93: loader->StartRequest();
On 2012/12/03 02:30:16, willchan wrote:
> I'm not sure if all URLRequestJobs complete asynchronously. If not, then this
> may synchronously call back into ResourceDispatcherHostImpl and delete the
> loader. I need to read through the code to make sure everything's safe.

No longer an issue. We don't hang on to it any more.

https://codereview.chromium.org/11270027/diff/14001/content/browser/renderer_...
File content/browser/renderer_host/resource_scheduler.h (right):

https://codereview.chromium.org/11270027/diff/14001/content/browser/renderer_...
content/browser/renderer_host/resource_scheduler.h:66: const
linked_ptr<ResourceLoader>& loader);
On 2012/12/03 02:30:16, willchan wrote:
> This should take a ResourceLoader* and not increment the refcount (linked_ptr
is
> a refcounting mechanism) since ResourceScheduler does not own the
> ResourceLoader, ResourceDispatcherHostImpl does.

Done.

https://codereview.chromium.org/11270027/diff/14001/content/browser/renderer_...
content/browser/renderer_host/resource_scheduler.h:99: Client();
On 2012/12/03 02:30:16, willchan wrote:
> How about declaring the destructor here and defining it in the .cc file while
> you're at it, so we don't get compiler generated inlined destructor STL code
> bloat.

Done.

https://codereview.chromium.org/11270027/diff/14001/content/browser/renderer_...
content/browser/renderer_host/resource_scheduler.h:110: // Called when a
BackgroundLoadHandle is destroyed.
On 2012/12/03 02:30:16, willchan wrote:
> I think we should kill off this function. I think that in the future we may
want
> to migrate resource loads between normal priority scheduling and background
> scheduling. I think RemoveLoad() should search all the maps/sets to figure
> remove it, and we shouldn't need different LoadHandle subclasses to specify
> which type of load is getting removed.
> 
> That's my random thought because I am imagining what we might want to do in
> cases like session restore and prioritizing foreground tabs over background
tabs
> and stuff. I think the LoadHandle shouldn't be aware of the priority level /
> load category, that should all be known by the ResourceScheduler.
> 
> Also, you're coupling background requests pretty tightly with the lack of a
> client_id. Is that desirable? I feel like in the future we may have background
> requests that still have a client_id.

Done.

https://codereview.chromium.org/11270027/diff/14001/content/browser/renderer_...
File content/browser/renderer_host/resource_scheduler_unittest.cc (right):

https://codereview.chromium.org/11270027/diff/14001/content/browser/renderer_...
content/browser/renderer_host/resource_scheduler_unittest.cc:10: #include
"content/browser/renderer_host/resource_scheduler.h"
On 2012/12/03 02:33:31, willchan wrote:
> This should go first

Done.

https://codereview.chromium.org/11270027/diff/14001/content/browser/renderer_...
content/browser/renderer_host/resource_scheduler_unittest.cc:133: }  // unnamed
namespace
On 2012/12/03 02:33:31, willchan wrote:
> Why not put everything in the anonymous namespace? TEST_Fs can go in there
too.

Done.

https://codereview.chromium.org/11270027/diff/14001/content/common/content_ex...
File content/common/content_export.h (right):

https://codereview.chromium.org/11270027/diff/14001/content/common/content_ex...
content/common/content_export.h:13: #define CONTENT_EXPORT_PRIVATE
__declspec(dllexport)
On 2012/12/03 02:33:31, willchan wrote:
> Please pull this out to a separate CL.

Done.

[willchan no longer on Chromium, 2012-12-05 03:44:28]

https://codereview.chromium.org/11270027/diff/14001/content/browser/renderer_...
File content/browser/renderer_host/resource_dispatcher_host_impl.cc (right):

https://codereview.chromium.org/11270027/diff/14001/content/browser/renderer_...
content/browser/renderer_host/resource_dispatcher_host_impl.cc:1295: void
ResourceDispatcherHostImpl::CancelRequestsForProcess(int child_id) {
On 2012/12/05 01:52:45, James Simonsen wrote:
> On 2012/12/03 02:30:16, willchan wrote:
> > How does <a ping> work here? AFAICT, it's broken.
> 
> How so? They have a child_id, but no route_id. They still end up in the map,
and
> the -1 will match them.

Are you saying that we *want* to cancel <a ping> requests when the process goes
away? I thought it was supposed to be fire and forget. Do we actually want to
ever cancel it other than on profile shutdown?

[willchan no longer on Chromium, 2012-12-05 04:37:00]

I apologize, eae@ gave me too much scotch tonight. I cannot review any more. I
will review this in person tomorrow.

https://codereview.chromium.org/11270027/diff/24001/content/browser/renderer_...
File content/browser/renderer_host/resource_dispatcher_host_impl.cc (right):

https://codereview.chromium.org/11270027/diff/24001/content/browser/renderer_...
content/browser/renderer_host/resource_dispatcher_host_impl.cc:1195:
params.new_request_id);
indentation is off

[James Simonsen, 2012-12-06 00:11:42]

On 2012/12/05 03:44:28, willchan wrote:
>
https://codereview.chromium.org/11270027/diff/14001/content/browser/renderer_...
> File content/browser/renderer_host/resource_dispatcher_host_impl.cc (right):
> 
>
https://codereview.chromium.org/11270027/diff/14001/content/browser/renderer_...
> content/browser/renderer_host/resource_dispatcher_host_impl.cc:1295: void
> ResourceDispatcherHostImpl::CancelRequestsForProcess(int child_id) {
> On 2012/12/05 01:52:45, James Simonsen wrote:
> > On 2012/12/03 02:30:16, willchan wrote:
> > > How does <a ping> work here? AFAICT, it's broken.
> > 
> > How so? They have a child_id, but no route_id. They still end up in the map,
> and
> > the -1 will match them.
> 
> Are you saying that we *want* to cancel <a ping> requests when the process
goes
> away? I thought it was supposed to be fire and forget. Do we actually want to
> ever cancel it other than on profile shutdown?

OIC. Yeah, it's as broken now as it was before. We should file a bug. I agree it
should be fire and forget.

[James Simonsen, 2012-12-12 04:19:16]

Despite the trybots being flaky tonight (failing to fetch patches), I think all
the tests pass now.

The important realizations/changes:

- We sometimes detect crashed renderers before we've processed all of the IPC
messages from that renderer. Ignore messages from dead renderers.

- We sometimes report a crashed renderer more than once. Be resilient to that.

- The NaCl tests perform a synchronous XHR inside the <head>. These low priority
requests would block forever waiting for a paint. Instead, treat synchronous
requests as high priority.

PTAL

[willchan no longer on Chromium, 2012-12-13 06:22:27]

I don't know why I didn't send this last week, but I just noticed I still had
these drafts. I hope they're still relevant :)

https://codereview.chromium.org/11270027/diff/34013/content/browser/loader/re...
File content/browser/loader/resource_dispatcher_host_impl.cc (right):

https://codereview.chromium.org/11270027/diff/34013/content/browser/loader/re...
content/browser/loader/resource_dispatcher_host_impl.cc:1061:
pending_loaders_[extra_info->GetGlobalRequestID()] = make_linked_ptr(
linked_ptr--

https://codereview.chromium.org/11270027/diff/34013/content/browser/loader/re...
content/browser/loader/resource_dispatcher_host_impl.cc:1508:
pending_loaders_[info->GetGlobalRequestID()] = make_linked_ptr(
linked_ptr--

https://codereview.chromium.org/11270027/diff/34013/content/browser/loader/re...
File content/browser/loader/resource_scheduler.cc (right):

https://codereview.chromium.org/11270027/diff/34013/content/browser/loader/re...
content/browser/loader/resource_scheduler.cc:39: };
DISALLOW_COPY_AND_ASSIGN

https://codereview.chromium.org/11270027/diff/34013/content/browser/loader/re...
File content/browser/loader/resource_scheduler.h (right):

https://codereview.chromium.org/11270027/diff/34013/content/browser/loader/re...
content/browser/loader/resource_scheduler.h:64: virtual const net::URLRequest*
url_request() = 0;
const net::URLRequest&

It can't be NULL, right? Unless the user is doing something buggy.

[willchan no longer on Chromium, 2012-12-13 06:30:43]

OK, I think things look pretty good. You should probably get an OWNERS review
around now.

https://codereview.chromium.org/11270027/diff/45001/content/browser/loader/re...
File content/browser/loader/resource_dispatcher_host_impl.cc (right):

https://codereview.chromium.org/11270027/diff/45001/content/browser/loader/re...
content/browser/loader/resource_dispatcher_host_impl.cc:455:
loaders->weak_erase(loaders->begin(), loaders->end());
Why not weak_clear()?

https://codereview.chromium.org/11270027/diff/45001/content/browser/loader/re...
content/browser/loader/resource_dispatcher_host_impl.cc:1688: delete loaders;
weak_clear?

https://codereview.chromium.org/11270027/diff/45001/content/browser/loader/re...
File content/browser/loader/resource_scheduler.cc (right):

https://codereview.chromium.org/11270027/diff/45001/content/browser/loader/re...
content/browser/loader/resource_scheduler.cc:40: };
DISALLOW_COPY_AND_ASSIGN

https://codereview.chromium.org/11270027/diff/45001/content/browser/loader/re...
File content/browser/loader/resource_scheduler.h (right):

https://codereview.chromium.org/11270027/diff/45001/content/browser/loader/re...
content/browser/loader/resource_scheduler.h:42: // The scheduling behavior
mimicks WebKit's scheduler, which blocks fetching
Let's kill off this comment. We're going to diverge from that scheduler soon
enough.

[James Simonsen, 2012-12-14 20:23:23]

https://codereview.chromium.org/11270027/diff/24001/content/browser/renderer_...
File content/browser/renderer_host/resource_dispatcher_host_impl.cc (right):

https://codereview.chromium.org/11270027/diff/24001/content/browser/renderer_...
content/browser/renderer_host/resource_dispatcher_host_impl.cc:1195:
params.new_request_id);
On 2012/12/05 04:37:00, willchan wrote:
> indentation is off

Done.

https://codereview.chromium.org/11270027/diff/34013/content/browser/loader/re...
File content/browser/loader/resource_dispatcher_host_impl.cc (right):

https://codereview.chromium.org/11270027/diff/34013/content/browser/loader/re...
content/browser/loader/resource_dispatcher_host_impl.cc:1061:
pending_loaders_[extra_info->GetGlobalRequestID()] = make_linked_ptr(
On 2012/12/13 06:22:27, willchan wrote:
> linked_ptr--

Done. Not convinced this one's a win though. You have to be careful any time you
remove something from this map now and there are a couple of places it could
happen.

https://codereview.chromium.org/11270027/diff/45001/content/browser/loader/re...
File content/browser/loader/resource_dispatcher_host_impl.cc (right):

https://codereview.chromium.org/11270027/diff/45001/content/browser/loader/re...
content/browser/loader/resource_dispatcher_host_impl.cc:455:
loaders->weak_erase(loaders->begin(), loaders->end());
On 2012/12/13 06:30:43, willchan wrote:
> Why not weak_clear()?

Didn't know about it. Done.

https://codereview.chromium.org/11270027/diff/45001/content/browser/loader/re...
content/browser/loader/resource_dispatcher_host_impl.cc:1688: delete loaders;
On 2012/12/13 06:30:43, willchan wrote:
> weak_clear?

Done.

https://codereview.chromium.org/11270027/diff/45001/content/browser/loader/re...
File content/browser/loader/resource_scheduler.cc (right):

https://codereview.chromium.org/11270027/diff/45001/content/browser/loader/re...
content/browser/loader/resource_scheduler.cc:40: };
On 2012/12/13 06:30:43, willchan wrote:
> DISALLOW_COPY_AND_ASSIGN

Done.

https://codereview.chromium.org/11270027/diff/45001/content/browser/loader/re...
File content/browser/loader/resource_scheduler.h (right):

https://codereview.chromium.org/11270027/diff/45001/content/browser/loader/re...
content/browser/loader/resource_scheduler.h:42: // The scheduling behavior
mimicks WebKit's scheduler, which blocks fetching
On 2012/12/13 06:30:43, willchan wrote:
> Let's kill off this comment. We're going to diverge from that scheduler soon
> enough.

Done.

[James Simonsen, 2012-12-14 20:26:43]

Darin, are you a good person to review resource loading changes in content? If
not, please suggest someone else.

[willchan no longer on Chromium, 2012-12-14 20:33:31]

https://codereview.chromium.org/11270027/diff/34013/content/browser/loader/re...
File content/browser/loader/resource_dispatcher_host_impl.cc (right):

https://codereview.chromium.org/11270027/diff/34013/content/browser/loader/re...
content/browser/loader/resource_dispatcher_host_impl.cc:1061:
pending_loaders_[extra_info->GetGlobalRequestID()] = make_linked_ptr(
On 2012/12/14 20:23:23, James Simonsen wrote:
> On 2012/12/13 06:22:27, willchan wrote:
> > linked_ptr--
> 
> Done. Not convinced this one's a win though. You have to be careful any time
you
> remove something from this map now and there are a couple of places it could
> happen.

It's a fair point. I think it's a better state than having memory ownership
transfer via linked_ptr, and better than going between scoped_ptr and linked_ptr
as was previously done here. If it's only ever got a single owner, I'd prefer to
explicitly own it. We'll come back and fix this when we get unique_ptr.

[darin (slow to review), 2012-12-14 22:16:28]

https://codereview.chromium.org/11270027/diff/53001/content/browser/loader/re...
File content/browser/loader/resource_dispatcher_host_impl.cc (right):

https://codereview.chromium.org/11270027/diff/53001/content/browser/loader/re...
content/browser/loader/resource_dispatcher_host_impl.cc:784:
STLDeleteContainerPairSecondPointers(pending_loaders_.begin(),
makes me wish we had a ScopedMap type to go along with ScopedVector.

https://codereview.chromium.org/11270027/diff/53001/content/browser/loader/re...
content/browser/loader/resource_dispatcher_host_impl.cc:1065:
pending_loaders_[extra_info->GetGlobalRequestID()] =
nit: seems a little bloaty codesighs-wise to repeat the map lookup.

https://codereview.chromium.org/11270027/diff/53001/content/browser/loader/re...
content/browser/loader/resource_dispatcher_host_impl.cc:1513:
pending_loaders_[info->GetGlobalRequestID()] = loader.release();
same nit

https://codereview.chromium.org/11270027/diff/53001/content/browser/loader/re...
content/browser/loader/resource_dispatcher_host_impl.cc:1514:
pending_loaders_[info->GetGlobalRequestID()]->ScheduleRequest(
This feels a bit awkward to me.  I don't think the ResourceLoader should have to
know about the ResourceScheduler.  It seems like the ResourceDispatcherHostImpl
should just pass the ResourceLoader to the ResourceScheduler.

I think I might also just pass the URLRequest pointer at this time instead of
making the ResourceScheduler get the URLRequest from the ResourceLoader.

It feels like the scheduler is just something that can delay starting a request,
and it would be nice to build on existing infrastructure for delaying a request.
 Adding more infrastructure here adds complexity.

https://codereview.chromium.org/11270027/diff/53001/content/browser/loader/re...
File content/browser/loader/resource_scheduler.h (right):

https://codereview.chromium.org/11270027/diff/53001/content/browser/loader/re...
content/browser/loader/resource_scheduler.h:59: virtual const net::URLRequest&
url_request() = 0;
virtual functions should never use unix_hacker_style as they are not as free as
a member variable dereference.

https://codereview.chromium.org/11270027/diff/53001/content/browser/loader/re...
content/browser/loader/resource_scheduler.h:75: void OnCreate(int child_id, int
route_id);
OnCreate and OnDestroy do not really seem to correspond to IPC messages.  It
looks like the RenderProcessHost is responsible for calling these.  So, it seems
like the names should be more like AddFoo / RemoveFoo.

https://codereview.chromium.org/11270027/diff/53001/content/browser/loader/re...
File content/browser/loader/resource_scheduler_filter.cc (right):

https://codereview.chromium.org/11270027/diff/53001/content/browser/loader/re...
content/browser/loader/resource_scheduler_filter.cc:39:
resource_scheduler_->OnPaint(child_id_, message.routing_id());
OnPaint -> OnFirstPaint?  OnPaint sounds like something that would be called
each time the page gets painted.

https://codereview.chromium.org/11270027/diff/53001/content/browser/renderer_...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/11270027/diff/53001/content/browser/renderer_...
content/browser/renderer_host/render_process_host_impl.cc:1112:
ResourceDispatcherHostImpl::Get()->scheduler()->OnCreate(
Isn't there some potential raciness here?  I could imagine possibly missing the
first request made by the renderer.  Maybe you should be intercepting something
on the IO thread related to RenderView setup?

https://codereview.chromium.org/11270027/diff/53001/content/renderer/render_v...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/11270027/diff/53001/content/renderer/render_v...
content/renderer/render_view_impl.cc:5612: Send(new
ViewHostMsg_DidFirstPaint(routing_id()));
did you consider just watching for the first ViewHostMsg_UpdateRect message
after ViewHostMsg_FrameNavigate?  seems a bit unfortunate to add more IPCs when
there is already a way to observe what you want to observe.

[darin (slow to review), 2012-12-14 22:26:57]

James and I just chatted.  I think the scheduler can be implemented in terms of
a ResourceThrottle.  I showed him where to hook that up so that it can get first
crack at deferring the request.

By the way, I think changing the code to move away from linked_ptr is a mistake.
 It obviously makes the code way more complicated to not use linked_ptr in this
case.  Maybe it could be good if we had a ScopedMap<T> type that worked like
ScopedVector<T>.  At any rate, replacing linked_ptr here with something else is
a different CL.  I wouldn't couple it with the work to add a resource scheduler.

[willchan no longer on Chromium, 2012-12-14 22:30:54]

Hold on before changing anything! :) Will write up a response shortly.

On Fri, Dec 14, 2012 at 2:26 PM,  <darin@chromium.org> wrote:
> James and I just chatted.  I think the scheduler can be implemented in terms
> of
> a ResourceThrottle.  I showed him where to hook that up so that it can get
> first
> crack at deferring the request.
>
> By the way, I think changing the code to move away from linked_ptr is a
> mistake.
>  It obviously makes the code way more complicated to not use linked_ptr in
> this
> case.  Maybe it could be good if we had a ScopedMap<T> type that worked like
> ScopedVector<T>.  At any rate, replacing linked_ptr here with something else
> is
> a different CL.  I wouldn't couple it with the work to add a resource
> scheduler.
>
> https://codereview.chromium.org/11270027/

[willchan no longer on Chromium, 2012-12-14 22:48:03]

OK, I don't know the ResourceThrottle users well, but I think they are
mostly about delaying things at certain points. We should keep in mind
some things:
* We want a true scheduler for performance reasons. I think the
ResourceThrottle interface allows for individual components to add
throttling. For we want a system, the ResourceScheduler, to have
global visibility so it can schedule everything.
* I believe this argues that ResourceScheduler should be a first class
peer in the ResourceDispatcherHostImpl system, rather than just
another member of the ResourceThrottle system. Either that, or we
should introduce layering of ResourceThrottles and have the
ResourceScheduler be the topmost layer (or perhaps the bottom-most, so
we can get all the other throttling done first, before we consider
whether or not to schedule a resource.
* Remember that the ResourceScheduler will want many different events.
Redirects to different origins, foreground/background tab switching,
yada yada. We need to make sure that if it's part of the
ResourceThrottle system, the ResourceThrottle API has the appropriate
entry points for the scheduler to function optimally. I'm a bit
concerned about that since we might radically expand the API surface
only for the ResourceScheduler use case, as opposed to all the other
ResourceThrottle implementations.

Now, onto linked_ptr. I share the desire for a ScopedMap, but didn't
think we had sufficient interest for this. I've chatted with darin@
and jyasskin@ and we'll move forward with that discussion in a
separate venue. I'll say that I prefer the explicit ownership to
linked_ptr. I will highlight why, and if people still disagree, I am
fine with reverting back to linked_ptr. Examine how the code
originally looked with linked_ptr:
https://codereview.chromium.org/11270027/diff/7001/content/browser/renderer_h....
You can see that the LoadHandle takes a const
linked_ptr<ResourceLoader>&, and stores it, and also hands it out.
This completely muddles the ownership of the ResourceLoader (which is
truly owned by ResourceDispatcherHostImpl). I am more willing to allow
linked_ptr to be used internally within ResourceDispatcherHostImpl,
but when the linked_ptr<ResourceLoader> leaks out to multiple APIs, it
makes the lifetime of ResourceLoader difficult to understand and more
bug-prone IMO. If we were to use linked_ptr internally within
ResourceDispatcherHostImpl's member containers (the vector and the
map), then I would argue that we still shouldn't hand out a
linked_ptr<ResourceLoader> to anything external to
ResourceDispatcherHostImpl and use raw pointers, since only
ResourceDispatcherHostImpl should delete a ResourceLoader.

On Fri, Dec 14, 2012 at 2:30 PM, William Chan (陈智昌)
<willchan@chromium.org> wrote:
> Hold on before changing anything! :) Will write up a response shortly.
>
> On Fri, Dec 14, 2012 at 2:26 PM,  <darin@chromium.org> wrote:
>> James and I just chatted.  I think the scheduler can be implemented in terms
>> of
>> a ResourceThrottle.  I showed him where to hook that up so that it can get
>> first
>> crack at deferring the request.
>>
>> By the way, I think changing the code to move away from linked_ptr is a
>> mistake.
>>  It obviously makes the code way more complicated to not use linked_ptr in
>> this
>> case.  Maybe it could be good if we had a ScopedMap<T> type that worked like
>> ScopedVector<T>.  At any rate, replacing linked_ptr here with something else
>> is
>> a different CL.  I wouldn't couple it with the work to add a resource
>> scheduler.
>>
>> https://codereview.chromium.org/11270027/

[James Simonsen, 2012-12-14 23:00:10]

We probably shoul've fully laid out the new idea...

Instead of returning LoadHandles, the ResourceScheduler will return
ResourceThrottles, which it will Suspend() and Resume() as necessary. The
ResourceScheduler itself won't change.

In doing so, we no longer touch ResourceLoaders and that code, along with its
linked_ptrs, can go back to the way it was. Then it can be refactored
separately.

Does that sound better?

[mmenke, 2012-12-14 23:12:34]

On 2012/12/14 23:00:10, James Simonsen wrote:
> We probably shoul've fully laid out the new idea...
> 
> Instead of returning LoadHandles, the ResourceScheduler will return
> ResourceThrottles, which it will Suspend() and Resume() as necessary. The
> ResourceScheduler itself won't change.
> 
> In doing so, we no longer touch ResourceLoaders and that code, along with its
> linked_ptrs, can go back to the way it was. Then it can be refactored
> separately.
> 
> Does that sound better?

We want to be able to re-prioritize resources, right?  Suspending / resuming a
ResourceThrottle can't suspend or reprioritize a resource that's currently
waiting in a socket pool's queue, or the PAC queue, or the DNS queue, and can't
prevent a request from moving from one queue to the next.

[willchan no longer on Chromium, 2012-12-14 23:14:10]

Yeah Matt, I've conveyed this to Darin offline. That'd have to be done
externally to the ResourceThrottle system. The tentative plan is to
move forward as is with Darin's recommendations, but discuss long-term
plans.

On Fri, Dec 14, 2012 at 3:12 PM,  <mmenke@chromium.org> wrote:
> On 2012/12/14 23:00:10, James Simonsen wrote:
>>
>> We probably shoul've fully laid out the new idea...
>
>
>> Instead of returning LoadHandles, the ResourceScheduler will return
>> ResourceThrottles, which it will Suspend() and Resume() as necessary. The
>> ResourceScheduler itself won't change.
>
>
>> In doing so, we no longer touch ResourceLoaders and that code, along with
>> its
>> linked_ptrs, can go back to the way it was. Then it can be refactored
>> separately.
>
>
>> Does that sound better?
>
>
> We want to be able to re-prioritize resources, right?  Suspending / resuming
> a
> ResourceThrottle can't suspend or reprioritize a resource that's currently
> waiting in a socket pool's queue, or the PAC queue, or the DNS queue, and
> can't
> prevent a request from moving from one queue to the next.
>
> https://codereview.chromium.org/11270027/

[darin (slow to review), 2012-12-14 23:19:04]

Yes, you would solve that problem by modifying the priority of the
URLRequest.
Any queues underneath URLRequest can be re-prioritized using this as a
signal.
Simple.

-Darin


On Fri, Dec 14, 2012 at 3:14 PM, William Chan (陈智昌)
<willchan@chromium.org>wrote:

> Yeah Matt, I've conveyed this to Darin offline. That'd have to be done
> externally to the ResourceThrottle system. The tentative plan is to
> move forward as is with Darin's recommendations, but discuss long-term
> plans.
>
> On Fri, Dec 14, 2012 at 3:12 PM,  <mmenke@chromium.org> wrote:
> > On 2012/12/14 23:00:10, James Simonsen wrote:
> >>
> >> We probably shoul've fully laid out the new idea...
> >
> >
> >> Instead of returning LoadHandles, the ResourceScheduler will return
> >> ResourceThrottles, which it will Suspend() and Resume() as necessary.
> The
> >> ResourceScheduler itself won't change.
> >
> >
> >> In doing so, we no longer touch ResourceLoaders and that code, along
> with
> >> its
> >> linked_ptrs, can go back to the way it was. Then it can be refactored
> >> separately.
> >
> >
> >> Does that sound better?
> >
> >
> > We want to be able to re-prioritize resources, right?  Suspending /
> resuming
> > a
> > ResourceThrottle can't suspend or reprioritize a resource that's
> currently
> > waiting in a socket pool's queue, or the PAC queue, or the DNS queue, and
> > can't
> > prevent a request from moving from one queue to the next.
> >
> > https://codereview.chromium.org/11270027/
>

[James Simonsen, 2012-12-15 02:58:57]

Thanks, this looks way better.

Most of the comments aren't applicable now, so I only responded to the ones that
still are...

https://codereview.chromium.org/11270027/diff/53001/content/browser/loader/re...
File content/browser/loader/resource_scheduler.h (right):

https://codereview.chromium.org/11270027/diff/53001/content/browser/loader/re...
content/browser/loader/resource_scheduler.h:75: void OnCreate(int child_id, int
route_id);
On 2012/12/14 22:16:28, darin wrote:
> OnCreate and OnDestroy do not really seem to correspond to IPC messages.  It
> looks like the RenderProcessHost is responsible for calling these.  So, it
seems
> like the names should be more like AddFoo / RemoveFoo.

OnCreate is tied to an event now. And OnDestroy is loosely tied to
ViewHostMsg_RenderViewGone, though that isn't a real IPC. Do you still want me
to rename them?

https://codereview.chromium.org/11270027/diff/53001/content/browser/loader/re...
File content/browser/loader/resource_scheduler_filter.cc (right):

https://codereview.chromium.org/11270027/diff/53001/content/browser/loader/re...
content/browser/loader/resource_scheduler_filter.cc:39:
resource_scheduler_->OnPaint(child_id_, message.routing_id());
On 2012/12/14 22:16:28, darin wrote:
> OnPaint -> OnFirstPaint?  OnPaint sounds like something that would be called
> each time the page gets painted.

Done.

https://codereview.chromium.org/11270027/diff/53001/content/browser/renderer_...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/11270027/diff/53001/content/browser/renderer_...
content/browser/renderer_host/render_process_host_impl.cc:1112:
ResourceDispatcherHostImpl::Get()->scheduler()->OnCreate(
On 2012/12/14 22:16:28, darin wrote:
> Isn't there some potential raciness here?  I could imagine possibly missing
the
> first request made by the renderer.  Maybe you should be intercepting
something
> on the IO thread related to RenderView setup?

Done.

https://codereview.chromium.org/11270027/diff/53001/content/renderer/render_v...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/11270027/diff/53001/content/renderer/render_v...
content/renderer/render_view_impl.cc:5612: Send(new
ViewHostMsg_DidFirstPaint(routing_id()));
On 2012/12/14 22:16:28, darin wrote:
> did you consider just watching for the first ViewHostMsg_UpdateRect message
> after ViewHostMsg_FrameNavigate?  seems a bit unfortunate to add more IPCs
when
> there is already a way to observe what you want to observe.

Yeah, I did. You talked me out of it. :)

Let's see if your argument works again. Basically, you said the one point in
RenderViewImpl where we record the first paint histogram is the authority on
first paint. We know it to be correct regardless of whether we're using GPU or
software. So, it's best to key off of that.

[darin (slow to review), 2012-12-15 05:05:37]

On Fri, Dec 14, 2012 at 6:58 PM, <simonjam@chromium.org> wrote:

> Thanks, this looks way better.
>
> Most of the comments aren't applicable now, so I only responded to the
> ones that
> still are...
>
>
>
> https://codereview.chromium.**org/11270027/diff/53001/**
>
content/browser/loader/**resource_scheduler.h<https://codereview.chromium.org/11270027/diff/53001/content/browser/loader/resource_scheduler.h>
> File content/browser/loader/**resource_scheduler.h (right):
>
> https://codereview.chromium.**org/11270027/diff/53001/**
>
content/browser/loader/**resource_scheduler.h#newcode75<https://codereview.chromium.org/11270027/diff/53001/content/browser/loader/resource_scheduler.h#newcode75>
> content/browser/loader/**resource_scheduler.h:75: void OnCreate(int
> child_id, int route_id);
> On 2012/12/14 22:16:28, darin wrote:
>
>> OnCreate and OnDestroy do not really seem to correspond to IPC
>>
> messages.  It
>
>> looks like the RenderProcessHost is responsible for calling these.
>>
> So, it seems
>
>> like the names should be more like AddFoo / RemoveFoo.
>>
>
> OnCreate is tied to an event now. And OnDestroy is loosely tied to
> ViewHostMsg_RenderViewGone, though that isn't a real IPC. Do you still
> want me to rename them?
>
>
> https://codereview.chromium.**org/11270027/diff/53001/**
>
content/browser/loader/**resource_scheduler_filter.cc<https://codereview.chromium.org/11270027/diff/53001/content/browser/loader/resource_scheduler_filter.cc>
> File content/browser/loader/**resource_scheduler_filter.cc (right):
>
> https://codereview.chromium.**org/11270027/diff/53001/**
>
content/browser/loader/**resource_scheduler_filter.cc#**newcode39<https://codereview.chromium.org/11270027/diff/53001/content/browser/loader/resource_scheduler_filter.cc#newcode39>
> content/browser/loader/**resource_scheduler_filter.cc:**39:
> resource_scheduler_->OnPaint(**child_id_, message.routing_id());
> On 2012/12/14 22:16:28, darin wrote:
>
>> OnPaint -> OnFirstPaint?  OnPaint sounds like something that would be
>>
> called
>
>> each time the page gets painted.
>>
>
> Done.
>
>
> https://codereview.chromium.**org/11270027/diff/53001/**
>
content/browser/renderer_host/**render_process_host_impl.cc<https://codereview.chromium.org/11270027/diff/53001/content/browser/renderer_host/render_process_host_impl.cc>
> File content/browser/renderer_host/**render_process_host_impl.cc (right):
>
> https://codereview.chromium.**org/11270027/diff/53001/**
>
content/browser/renderer_host/**render_process_host_impl.cc#**newcode1112<https://codereview.chromium.org/11270027/diff/53001/content/browser/renderer_host/render_process_host_impl.cc#newcode1112>
> content/browser/renderer_host/**render_process_host_impl.cc:**1112:
> ResourceDispatcherHostImpl::**Get()->scheduler()->OnCreate(
> On 2012/12/14 22:16:28, darin wrote:
>
>> Isn't there some potential raciness here?  I could imagine possibly
>>
> missing the
>
>> first request made by the renderer.  Maybe you should be intercepting
>>
> something
>
>> on the IO thread related to RenderView setup?
>>
>
> Done.
>
>
> https://codereview.chromium.**org/11270027/diff/53001/**
>
content/renderer/render_view_**impl.cc<https://codereview.chromium.org/11270027/diff/53001/content/renderer/render_view_impl.cc>
> File content/renderer/render_view_**impl.cc (right):
>
> https://codereview.chromium.**org/11270027/diff/53001/**
>
content/renderer/render_view_**impl.cc#newcode5612<https://codereview.chromium.org/11270027/diff/53001/content/renderer/render_view_impl.cc#newcode5612>
> content/renderer/render_view_**impl.cc:5612: Send(new
> ViewHostMsg_DidFirstPaint(**routing_id()));
> On 2012/12/14 22:16:28, darin wrote:
>
>> did you consider just watching for the first ViewHostMsg_UpdateRect
>>
> message
>
>> after ViewHostMsg_FrameNavigate?  seems a bit unfortunate to add more
>>
> IPCs when
>
>> there is already a way to observe what you want to observe.
>>
>
> Yeah, I did. You talked me out of it. :)
>
> Let's see if your argument works again. Basically, you said the one
> point in RenderViewImpl where we record the first paint histogram is the
> authority on first paint. We know it to be correct regardless of whether
> we're using GPU or software. So, it's best to key off of that.
>
>
What sage advice!!



>
https://codereview.chromium.**org/11270027/<https://codereview.chromium.org/1...
>

[darin (slow to review), 2012-12-15 05:17:24]

https://codereview.chromium.org/11270027/diff/56007/content/browser/renderer_...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/11270027/diff/56007/content/browser/renderer_...
content/browser/renderer_host/render_process_host_impl.cc:1463:
ResourceDispatcherHostImpl::Get()->scheduler()->OnDestroy(
This looks like it is the main thread, and I don't think
ResourceScheduler::OnDestroy is safe to call off of the IO thread.  You should
probably make ResourceScheduler inherit from NonThreadSafe.  It might be nice to
find some other way to communicate the destruction of a RenderView to the
scheduler.

Also, this is only trapping process destruction.  RenderViews can be closed well
before process destruction.  OnCreate seems to be called when a RenderView is
created, so I assumed OnDestroy was intended to be called when a RenderView is
destroyed.  Seems not to be how it works though.

[darin (slow to review), 2012-12-15 05:20:47]

https://codereview.chromium.org/11270027/diff/56007/content/browser/loader/re...
File content/browser/loader/resource_scheduler.cc (right):

https://codereview.chromium.org/11270027/diff/56007/content/browser/loader/re...
content/browser/loader/resource_scheduler.cc:75:
scoped_ptr<ScheduledResourceThrottle> request(
nit: might be nice to improve the terms here.  ScheduledResourceThrottle is not
so obviously a "request".  Plus, we already use request to refer to a URL
request in so much of the loader code.  Might help to not overload request in
these parts.

I can see why request makes sense though and calling the variable throttle would
just seem odd too.  One idea is to give the class a different name to be more
descriptive of what the class really represents.  Hmm...

[James Simonsen, 2012-12-17 21:31:38]

https://codereview.chromium.org/11270027/diff/56007/content/browser/loader/re...
File content/browser/loader/resource_scheduler.cc (right):

https://codereview.chromium.org/11270027/diff/56007/content/browser/loader/re...
content/browser/loader/resource_scheduler.cc:75:
scoped_ptr<ScheduledResourceThrottle> request(
On 2012/12/15 05:20:47, darin wrote:
> nit: might be nice to improve the terms here.  ScheduledResourceThrottle is
not
> so obviously a "request".  Plus, we already use request to refer to a URL
> request in so much of the loader code.  Might help to not overload request in
> these parts.
> 
> I can see why request makes sense though and calling the variable throttle
would
> just seem odd too.  One idea is to give the class a different name to be more
> descriptive of what the class really represents.  Hmm...

Done. ScheduledResourceRequest. WDYT?

https://codereview.chromium.org/11270027/diff/56007/content/browser/renderer_...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/11270027/diff/56007/content/browser/renderer_...
content/browser/renderer_host/render_process_host_impl.cc:1463:
ResourceDispatcherHostImpl::Get()->scheduler()->OnDestroy(
On 2012/12/15 05:17:24, darin wrote:
> This looks like it is the main thread, and I don't think
> ResourceScheduler::OnDestroy is safe to call off of the IO thread.  You should
> probably make ResourceScheduler inherit from NonThreadSafe.  It might be nice
to
> find some other way to communicate the destruction of a RenderView to the
> scheduler.
> 
> Also, this is only trapping process destruction.  RenderViews can be closed
well
> before process destruction.  OnCreate seems to be called when a RenderView is
> created, so I assumed OnDestroy was intended to be called when a RenderView is
> destroyed.  Seems not to be how it works though.

Done. Let me know if I should separate out or rename
RenderWidgetHelper::OnCancelResourceRequests.

[James Simonsen, 2012-12-18 19:57:57]

Ping.

It looks like the trybot failures are all flakes/unrelated. The reliable bots
are all green.

[darin (slow to review), 2012-12-18 21:09:56]

https://codereview.chromium.org/11270027/diff/65002/content/browser/loader/re...
File content/browser/loader/resource_dispatcher_host_impl.cc (right):

https://codereview.chromium.org/11270027/diff/65002/content/browser/loader/re...
content/browser/loader/resource_dispatcher_host_impl.cc:1056: if
(!throttles.empty()) {
nit: You can delete this line now.

https://codereview.chromium.org/11270027/diff/65002/content/browser/loader/re...
File content/browser/loader/resource_scheduler.cc (right):

https://codereview.chromium.org/11270027/diff/65002/content/browser/loader/re...
content/browser/loader/resource_scheduler.cc:156: return;
nit: What happens if a navigation occurs before OnCreate?  Should we be
asserting or trapping such conditions instead of just silently ignoring
OnNavigate?

https://codereview.chromium.org/11270027/diff/65002/content/browser/loader/re...
content/browser/loader/resource_scheduler.cc:167: return;
ditto

https://codereview.chromium.org/11270027/diff/65002/content/browser/loader/re...
content/browser/loader/resource_scheduler.cc:181: return;
ditto

https://codereview.chromium.org/11270027/diff/65002/content/browser/loader/re...
File content/browser/loader/resource_scheduler.h (right):

https://codereview.chromium.org/11270027/diff/65002/content/browser/loader/re...
content/browser/loader/resource_scheduler.h:27: // There is one
ResourceScheduler, which is owned by the ResourceDispatcherHost.
nit: It would be good to de-couple this logically from the
ResourceDispatcherHost.  I wouldn't talk about the RDH.  Instead, I would focus
on how this class expects to be used.  One shouldn't have to understand RDH to
understand this class.

https://codereview.chromium.org/11270027/diff/65002/content/browser/loader/re...
content/browser/loader/resource_scheduler.h:45: typedef int64 ClientId;
nit: It looks like this typedef can be made private.

https://codereview.chromium.org/11270027/diff/65002/content/browser/loader/re...
File content/browser/loader/resource_scheduler_filter.cc (right):

https://codereview.chromium.org/11270027/diff/65002/content/browser/loader/re...
content/browser/loader/resource_scheduler_filter.cc:37:
resource_scheduler_->OnNavigate(child_id_, message.routing_id());
Perhaps OnNavigate and OnFirstPaint should return bool values to indicate if
they were not preceded by OnCreate?  Then, if we observe that, we might consider
killing the renderer?

[James Simonsen, 2013-01-10 19:47:53]

I've changed the way we track navigation. I had a lot of trouble getting the
ResourceScheduler on the IO thread to properly track all of the tabs',
extensions', plugins', etc. state. There were always flaky edge cases in the
tests.

Instead, now I just use a MRUCache to track the most recent navigations. I think
that's a much safer way to keep track of the state we need.

PTAL.

https://codereview.chromium.org/11270027/diff/65002/content/browser/loader/re...
File content/browser/loader/resource_dispatcher_host_impl.cc (right):

https://codereview.chromium.org/11270027/diff/65002/content/browser/loader/re...
content/browser/loader/resource_dispatcher_host_impl.cc:1056: if
(!throttles.empty()) {
On 2012/12/18 21:09:56, darin wrote:
> nit: You can delete this line now.

Done.

https://codereview.chromium.org/11270027/diff/65002/content/browser/loader/re...
File content/browser/loader/resource_scheduler.h (right):

https://codereview.chromium.org/11270027/diff/65002/content/browser/loader/re...
content/browser/loader/resource_scheduler.h:27: // There is one
ResourceScheduler, which is owned by the ResourceDispatcherHost.
On 2012/12/18 21:09:56, darin wrote:
> nit: It would be good to de-couple this logically from the
> ResourceDispatcherHost.  I wouldn't talk about the RDH.  Instead, I would
focus
> on how this class expects to be used.  One shouldn't have to understand RDH to
> understand this class.

Done.

https://codereview.chromium.org/11270027/diff/65002/content/browser/loader/re...
content/browser/loader/resource_scheduler.h:45: typedef int64 ClientId;
On 2012/12/18 21:09:56, darin wrote:
> nit: It looks like this typedef can be made private.

Done.

[James Simonsen, 2013-01-14 21:21:53]

Ping.

[darin (slow to review), 2013-01-14 22:03:29]

Some superficial feedback.  I haven't studied the main changes in
ResourceScheduler yet.

https://codereview.chromium.org/11270027/diff/90001/content/browser/loader/re...
File content/browser/loader/resource_dispatcher_host_impl.cc (right):

https://codereview.chromium.org/11270027/diff/90001/content/browser/loader/re...
content/browser/loader/resource_dispatcher_host_impl.cc:1055:
throttles.push_back(scheduler_.ScheduleRequest(child_id, route_id,
nit: might read slightly better as:

  throttles.push_back(
      scheduler_.ScheduleRequest(child_id, route_id, *request).release());

https://codereview.chromium.org/11270027/diff/90001/content/browser/loader/re...
File content/browser/loader/resource_scheduler.cc (right):

https://codereview.chromium.org/11270027/diff/90001/content/browser/loader/re...
content/browser/loader/resource_scheduler.cc:15: static const int
MAX_NUM_NAVIGATIONS_TO_TRACK = 5;
nit: constants use kConstantStyle.  not required for file scoped things, but i
usually just put everything in the content namespace unless there is a reason to
do so.

[darin (slow to review), 2013-01-14 22:56:21]

more substantive feedback...

https://codereview.chromium.org/11270027/diff/90001/content/browser/loader/re...
File content/browser/loader/resource_scheduler.cc (right):

https://codereview.chromium.org/11270027/diff/90001/content/browser/loader/re...
content/browser/loader/resource_scheduler.cc:32:
scheduler_->RemoveRequest(this);
how do you know that the scheduler_ is still a valid pointer?

https://codereview.chromium.org/11270027/diff/90001/content/browser/loader/re...
content/browser/loader/resource_scheduler.cc:67: }
Hmm, should you assert that the pending_requests lists of all clients are empty?
 Or, maybe that is not worth worrying about since this class is only destroyed
when the IO thread is going away.  I was just wondering about the possibility of
leaks.

https://codereview.chromium.org/11270027/diff/90001/content/browser/loader/re...
content/browser/loader/resource_scheduler.cc:81: // 1. <a ping> requests don't
have a route_id.
It seems like a bug that <a ping> requests don't have a route id !!

https://codereview.chromium.org/11270027/diff/90001/content/browser/loader/re...
content/browser/loader/resource_scheduler.cc:138: LoadPendingRequests(client);
I'm not sure I understand why you are calling LoadPendingRequests here. 
RemoveRequest could be called because a resource request was simply cancelled.

https://codereview.chromium.org/11270027/diff/90001/content/browser/loader/re...
content/browser/loader/resource_scheduler.cc:177: for (it =
client->pending_requests.begin();
it seems like you are asserting that client->has_painted is true here.  maybe
you should write a DCHECK to indicate that.

that said, it seems like we could reach here from RemoveRequest, which could be
called before client->has_painted becomes true??

NOTE:  Since StartRequest calls ResourceController::Resume(), I worry a bit
about the potential for re-entrancy.  It would be bad if somehow during
StartRequest another call was made to the ResourceScheduler that resulted in
mutation to the pending_requests list.  Hmm...

[James Simonsen, 2013-01-15 01:10:06]

https://codereview.chromium.org/11270027/diff/90001/content/browser/loader/re...
File content/browser/loader/resource_dispatcher_host_impl.cc (right):

https://codereview.chromium.org/11270027/diff/90001/content/browser/loader/re...
content/browser/loader/resource_dispatcher_host_impl.cc:1055:
throttles.push_back(scheduler_.ScheduleRequest(child_id, route_id,
On 2013/01/14 22:03:30, darin wrote:
> nit: might read slightly better as:
> 
>   throttles.push_back(
>       scheduler_.ScheduleRequest(child_id, route_id, *request).release());

Done.

https://codereview.chromium.org/11270027/diff/90001/content/browser/loader/re...
File content/browser/loader/resource_scheduler.cc (right):

https://codereview.chromium.org/11270027/diff/90001/content/browser/loader/re...
content/browser/loader/resource_scheduler.cc:15: static const int
MAX_NUM_NAVIGATIONS_TO_TRACK = 5;
On 2013/01/14 22:03:30, darin wrote:
> nit: constants use kConstantStyle.  not required for file scoped things, but i
> usually just put everything in the content namespace unless there is a reason
to
> do so.

Done.

https://codereview.chromium.org/11270027/diff/90001/content/browser/loader/re...
content/browser/loader/resource_scheduler.cc:32:
scheduler_->RemoveRequest(this);
On 2013/01/14 22:56:22, darin wrote:
> how do you know that the scheduler_ is still a valid pointer?

|this| is owned by the same ResourceDispatcher that owns |scheduler_|.

https://codereview.chromium.org/11270027/diff/90001/content/browser/loader/re...
content/browser/loader/resource_scheduler.cc:67: }
On 2013/01/14 22:56:22, darin wrote:
> Hmm, should you assert that the pending_requests lists of all clients are
empty?
>  Or, maybe that is not worth worrying about since this class is only destroyed
> when the IO thread is going away.  I was just wondering about the possibility
of
> leaks.

Yeah, sounds good. Done.

https://codereview.chromium.org/11270027/diff/90001/content/browser/loader/re...
content/browser/loader/resource_scheduler.cc:81: // 1. <a ping> requests don't
have a route_id.
On 2013/01/14 22:56:22, darin wrote:
> It seems like a bug that <a ping> requests don't have a route id !!

That wouldn't be the only bug with <a ping>. Will pointed out earlier that if we
destroy a renderer process, all of its requests are canceled, including the
pings. I assume those are supposed to finish. Seems like a common case with
cross-renderer navigation.

https://codereview.chromium.org/11270027/diff/90001/content/browser/loader/re...
content/browser/loader/resource_scheduler.cc:138: LoadPendingRequests(client);
On 2013/01/14 22:56:22, darin wrote:
> I'm not sure I understand why you are calling LoadPendingRequests here. 
> RemoveRequest could be called because a resource request was simply cancelled.

Good catch! It's only supposed to do that when that client is idle. Somehow that
got lost in one of the iterations on this CL. Added a test case too.

https://codereview.chromium.org/11270027/diff/90001/content/browser/loader/re...
content/browser/loader/resource_scheduler.cc:177: for (it =
client->pending_requests.begin();
On 2013/01/14 22:56:22, darin wrote:
> it seems like you are asserting that client->has_painted is true here.  maybe
> you should write a DCHECK to indicate that.
> 
> that said, it seems like we could reach here from RemoveRequest, which could
be
> called before client->has_painted becomes true??

Yeah, there are a couple of ways we might decide to start a request:

1. It's high priority.
2. We've already painted.
3. We don't have any other requests in flight.
4. We've blown the MRU cache and can't track this client any more.

> 
> NOTE:  Since StartRequest calls ResourceController::Resume(), I worry a bit
> about the potential for re-entrancy.  It would be bad if somehow during
> StartRequest another call was made to the ResourceScheduler that resulted in
> mutation to the pending_requests list.  Hmm...

Good point. I made the code more robust and wrote a test case to exercise it.

[James Simonsen, 2013-01-15 23:59:21]

Ping. I believe I've addressed all of the feedback to date.

[darin (slow to review), 2013-01-16 06:05:59]

Something is wrong with the diff.  Can you re-upload?

[James Simonsen, 2013-01-16 19:00:21]

Sorry about that. No idea what happened. I rebased and reuploaded and it looks
sane now.

[James Simonsen, 2013-02-16 01:41:27]

PTAL.

I've updated this patch to use the new signals exposed by WebKit. These are
needed to match WebKit's scheduler performance. In fact, we'll be ~3% faster
now, because we're better at enforcing preload priority than WebKit's
timer-based scheduler.

I also updated it to to use the new ResourceMessageDelegate.

Part of this patch overlaps with https://codereview.chromium.org/12243002/. That
was LGTM'd, but wasn't landed because it lacked the new IPC handlers. Those
handlers are (and always have been) in this patch.

[darin (slow to review), 2013-02-19 06:58:33]

LGTM

https://codereview.chromium.org/11270027/diff/113001/content/browser/loader/r...
File content/browser/loader/resource_scheduler.cc (right):

https://codereview.chromium.org/11270027/diff/113001/content/browser/loader/r...
content/browser/loader/resource_scheduler.cc:25: 
nit: nix the new line here

https://codereview.chromium.org/11270027/diff/113001/content/browser/loader/r...
content/browser/loader/resource_scheduler.cc:45: request_->status().status() !=
net::URLRequestStatus::CANCELED) {
what about if the status is FAILED?  why is CANCELED treated differently than
FAILED in this case?

https://codereview.chromium.org/11270027/diff/113001/content/browser/loader/r...
content/browser/loader/resource_scheduler.cc:90: DetachFromThread();  //
Construction happens on the main thread.
ResourceDispatcherHostImpl also has an IO thread initialization and shutdown
phase.  you could arrange for RDHI to heap allocate ResourceScheduler and delete
it all on the IO thread.  Hmm...

https://codereview.chromium.org/11270027/diff/113001/content/browser/loader/r...
File content/browser/loader/resource_scheduler.h (right):

https://codereview.chromium.org/11270027/diff/113001/content/browser/loader/r...
content/browser/loader/resource_scheduler.h:80: struct Client {
nit: it looks like you could get away with defining this struct in the .cc file.
 Scoped, like this:

  struct ResourceScheduler::Client {
    ...
  };

https://codereview.chromium.org/11270027/diff/113001/content/renderer/render_...
File content/renderer/render_view_impl.h (right):

https://codereview.chromium.org/11270027/diff/113001/content/renderer/render_...
content/renderer/render_view_impl.h:638: virtual void
didChangeScrollOffset(WebKit::WebFrame* frame);
nit: it looks like willInsertBody should be inserted here according to where it
appears in WebFrameClient.  please make the .cc file use the same sorting order.

[Cris Neckar, 2013-02-19 19:59:09]

IPC security audit lgtm

[James Simonsen, 2013-02-19 23:08:58]

https://codereview.chromium.org/11270027/diff/113001/content/browser/loader/r...
File content/browser/loader/resource_scheduler.cc (right):

https://codereview.chromium.org/11270027/diff/113001/content/browser/loader/r...
content/browser/loader/resource_scheduler.cc:25: 
On 2013/02/19 06:58:33, darin wrote:
> nit: nix the new line here

Done.

https://codereview.chromium.org/11270027/diff/113001/content/browser/loader/r...
content/browser/loader/resource_scheduler.cc:45: request_->status().status() !=
net::URLRequestStatus::CANCELED) {
On 2013/02/19 06:58:33, darin wrote:
> what about if the status is FAILED?  why is CANCELED treated differently than
> FAILED in this case?

Done. No good reason, just that's all that was needed.

https://codereview.chromium.org/11270027/diff/113001/content/browser/loader/r...
content/browser/loader/resource_scheduler.cc:90: DetachFromThread();  //
Construction happens on the main thread.
On 2013/02/19 06:58:33, darin wrote:
> ResourceDispatcherHostImpl also has an IO thread initialization and shutdown
> phase.  you could arrange for RDHI to heap allocate ResourceScheduler and
delete
> it all on the IO thread.  Hmm... 

Done.

https://codereview.chromium.org/11270027/diff/113001/content/browser/loader/r...
File content/browser/loader/resource_scheduler.h (right):

https://codereview.chromium.org/11270027/diff/113001/content/browser/loader/r...
content/browser/loader/resource_scheduler.h:80: struct Client {
On 2013/02/19 06:58:33, darin wrote:
> nit: it looks like you could get away with defining this struct in the .cc
file.
>  Scoped, like this:
> 
>   struct ResourceScheduler::Client {
>     ...
>   };

Done.

https://codereview.chromium.org/11270027/diff/113001/content/renderer/render_...
File content/renderer/render_view_impl.h (right):

https://codereview.chromium.org/11270027/diff/113001/content/renderer/render_...
content/renderer/render_view_impl.h:638: virtual void
didChangeScrollOffset(WebKit::WebFrame* frame);
On 2013/02/19 06:58:33, darin wrote:
> nit: it looks like willInsertBody should be inserted here according to where
it
> appears in WebFrameClient.  please make the .cc file use the same sorting
order.

Done.

[commit-bot: I haz the power, 2013-02-20 01:14:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/simonjam@chromium.org/11270027/127001

[commit-bot: I haz the power, 2013-02-20 02:32:55]

Change committed as 183382