[Prerender] Restore request priorities when swapped in

[Prerender] Restore request priorities when swapped in

Requests from prerendered contents have a IDLE priority, in order not to slow
down visible pages.
However, when the prerendered contents become visible, the request priorities
were not reset back to their correct values, leading to bad prerendering
performance.

In this CL, the priority management for prerender requests is moved to the
PrerenderResourceThrottle/PrerendereContents.
The original priorities are stored in the throttle, and restored when the
prerender contents swaps in.
A new public function is added to ResourceDispatcherHost to update a request
priority, and its implementation reuses the existing code that updates the image
priorities.

The prerender contents keeps a list of all the network resources that were
started while the prerender is hidden. If this proves to be too large, pruning
the list when responses are received should be doable.

BUG=705955
Review-Url: https://codereview.chromium.org/2807163002
Cr-Commit-Position: refs/heads/master@{#464728}
Committed: https://chromium.googlesource.com/chromium/src/+/d3bc6148c7f0d2695ec2ca74652d996e2c4f5a6e

[droger, 2017-04-11 11:22:50]

The CQ bit was checked by droger@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-11 11:23:07]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-11 11:25:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-11 11:25:02]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[droger, 2017-04-11 11:44:26]

The CQ bit was checked by droger@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-11 11:44:38]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-11 12:38:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-11 12:38:44]

Dry run: This issue passed the CQ dry run.

[droger, 2017-04-11 13:20:20]

Description was changed from

==========
implementation in resource throttle

BUG=
==========

to

==========
[Prerender] Restore request priorities when swapped in

Requests from prerendered contents have a IDLE priority, in order
not to slow down visible pages.
However, when the prerendered contents become visible, the request
priorities were not reset back to their correct values, leading to
bad prerendering performance.

In this CL, the priority management for prerender requests is moved
to the PrerenderResourceThrottle.
A new public function is added to ResourceDispatcherHost to update
a request priority, and its implementation reuses the existing code
that updates the image priorities.

The prerender contents keeps a list of all the network resources
that were started while the prerender is hidden. If this proves to be
too large, pruning the list as responses are finished should be doable.

BUG=705955
==========

[droger, 2017-04-11 13:22:15]

Description was changed from

==========
[Prerender] Restore request priorities when swapped in

Requests from prerendered contents have a IDLE priority, in order
not to slow down visible pages.
However, when the prerendered contents become visible, the request
priorities were not reset back to their correct values, leading to
bad prerendering performance.

In this CL, the priority management for prerender requests is moved
to the PrerenderResourceThrottle.
A new public function is added to ResourceDispatcherHost to update
a request priority, and its implementation reuses the existing code
that updates the image priorities.

The prerender contents keeps a list of all the network resources
that were started while the prerender is hidden. If this proves to be
too large, pruning the list as responses are finished should be doable.

BUG=705955
==========

to

==========
[Prerender] Restore request priorities when swapped in

Requests from prerendered contents have a IDLE priority, in order
not to slow down visible pages.
However, when the prerendered contents become visible, the request
priorities were not reset back to their correct values, leading to
bad prerendering performance.

In this CL, the priority management for prerender requests is moved
to the PrerenderResourceThrottle/PrerendereContents.
The original priorities are stored in the throttle, and restored when
the prerender contents swaps in.
A new public function is added to ResourceDispatcherHost to update
a request priority, and its implementation reuses the existing code
that updates the image priorities.

The prerender contents keeps a list of all the network resources
that were started while the prerender is hidden. If this proves to be
too large, pruning the list as responses are finished should be doable.

BUG=705955
==========

[droger, 2017-04-11 13:43:18]

droger@chromium.org changed reviewers:
+ rdsmith@chromium.org

[droger, 2017-04-11 13:44:35]

Description was changed from

==========
[Prerender] Restore request priorities when swapped in

Requests from prerendered contents have a IDLE priority, in order
not to slow down visible pages.
However, when the prerendered contents become visible, the request
priorities were not reset back to their correct values, leading to
bad prerendering performance.

In this CL, the priority management for prerender requests is moved
to the PrerenderResourceThrottle/PrerendereContents.
The original priorities are stored in the throttle, and restored when
the prerender contents swaps in.
A new public function is added to ResourceDispatcherHost to update
a request priority, and its implementation reuses the existing code
that updates the image priorities.

The prerender contents keeps a list of all the network resources
that were started while the prerender is hidden. If this proves to be
too large, pruning the list as responses are finished should be doable.

BUG=705955
==========

to

==========
[Prerender] Restore request priorities when swapped in

Requests from prerendered contents have a IDLE priority, in order
not to slow down visible pages.
However, when the prerendered contents become visible, the request
priorities were not reset back to their correct values, leading to
bad prerendering performance.

In this CL, the priority management for prerender requests is moved
to the PrerenderResourceThrottle/PrerendereContents.
The original priorities are stored in the throttle, and restored when
the prerender contents swaps in.
A new public function is added to ResourceDispatcherHost to update
a request priority, and its implementation reuses the existing code
that updates the image priorities.

The prerender contents keeps a list of all the network resources
that were started while the prerender is hidden. If this proves to be
too large, pruning the list when responses are received should be
doable.

BUG=705955
==========

[droger, 2017-04-11 13:46:14]

Description was changed from

==========
[Prerender] Restore request priorities when swapped in

Requests from prerendered contents have a IDLE priority, in order
not to slow down visible pages.
However, when the prerendered contents become visible, the request
priorities were not reset back to their correct values, leading to
bad prerendering performance.

In this CL, the priority management for prerender requests is moved
to the PrerenderResourceThrottle/PrerendereContents.
The original priorities are stored in the throttle, and restored when
the prerender contents swaps in.
A new public function is added to ResourceDispatcherHost to update
a request priority, and its implementation reuses the existing code
that updates the image priorities.

The prerender contents keeps a list of all the network resources
that were started while the prerender is hidden. If this proves to be
too large, pruning the list when responses are received should be
doable.

BUG=705955
==========

to

==========
[Prerender] Restore request priorities when swapped in

Requests from prerendered contents have a IDLE priority, in order not to slow
down visible pages.
However, when the prerendered contents become visible, the request priorities
were not reset back to their correct values, leading to bad prerendering
performance.

In this CL, the priority management for prerender requests is moved to the
PrerenderResourceThrottle/PrerendereContents.
The original priorities are stored in the throttle, and restored when the
prerender contents swaps in.
A new public function is added to ResourceDispatcherHost to update a request
priority, and its implementation reuses the existing code that updates the image
priorities.

The prerender contents keeps a list of all the network resources that were
started while the prerender is hidden. If this proves to be too large, pruning
the list when responses are received should be doable.

BUG=705955
==========

[Randy Smith (Not in Mondays), 2017-04-11 19:47:08]

rdsmith@chromium.org changed reviewers:
+ csharrison@chromium.org
- rdsmith@chromium.org

[Randy Smith (Not in Mondays), 2017-04-11 19:47:08]

I'm trying to keep my plate clear for servicification, so lateralling to
csharrison@ (who probably has a bigger prerenderer clue than I do anyway).

[Charlie Harrison, 2017-04-12 12:37:26]

The CL looks good, but it would be nice to have this behavior covered by some
tests.

https://codereview.chromium.org/2807163002/diff/60001/chrome/browser/prerende...
File chrome/browser/prerender/prerender_resource_throttle.cc (right):

https://codereview.chromium.org/2807163002/diff/60001/chrome/browser/prerende...
chrome/browser/prerender/prerender_resource_throttle.cc:176: // changed.
You could use a base::Optional to make this distinction more clear.

https://codereview.chromium.org/2807163002/diff/60001/chrome/browser/prerende...
chrome/browser/prerender/prerender_resource_throttle.cc:236:
prerender_contents->AddResourceThrottle(throttle);
nit: newline below

https://codereview.chromium.org/2807163002/diff/60001/chrome/browser/prerende...
File chrome/browser/prerender/prerender_resource_throttle.h (left):

https://codereview.chromium.org/2807163002/diff/60001/chrome/browser/prerende...
chrome/browser/prerender/prerender_resource_throttle.h:91: int load_flags_;  //
Load flags to be OR'ed with the existing request flags.
nit: newline below.

https://codereview.chromium.org/2807163002/diff/60001/content/browser/loader/...
File content/browser/loader/resource_scheduler.cc (right):

https://codereview.chromium.org/2807163002/diff/60001/content/browser/loader/...
content/browser/loader/resource_scheduler.cc:1083: void
ResourceScheduler::ReprioritizeRequest(net::URLRequest* request,
Just curious: In practice, is this needed? I couldn't find any caller that
actually uses the intra_priority params.

[Charlie Harrison, 2017-04-12 12:50:33]

Also note that since this throttle is after the ScheduledResourceRequest
throttle, any decisions made by WillStartRequest time are only relevant if the
request is still delayed by the resource scheduler. This could affect behavior.

I feel like it may be ok to just set the priority of the URL request on
construction of your throttle, but I'll need to ponder it.

[Charlie Harrison, 2017-04-12 13:02:45]

On 2017/04/12 12:50:33, Charlie Harrison wrote:
> Also note that since this throttle is after the ScheduledResourceRequest
> throttle, any decisions made by WillStartRequest time are only relevant if the
> request is still delayed by the resource scheduler. This could affect
behavior.
Actually, this is wrong, but it still feels fragile to rely on the ordering.
> 
> I feel like it may be ok to just set the priority of the URL request on
> construction of your throttle, but I'll need to ponder it.

[droger, 2017-04-12 13:20:00]

The CQ bit was checked by droger@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-12 13:20:27]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[droger, 2017-04-12 13:20:55]

Thanks!

Addressed most comments except:
- tests
- moving the SetPriority to the constructor.

I'll look into these and ping the review when I'm done.

https://codereview.chromium.org/2807163002/diff/60001/chrome/browser/prerende...
File chrome/browser/prerender/prerender_resource_throttle.cc (right):

https://codereview.chromium.org/2807163002/diff/60001/chrome/browser/prerende...
chrome/browser/prerender/prerender_resource_throttle.cc:176: // changed.
On 2017/04/12 12:37:26, Charlie Harrison wrote:
> You could use a base::Optional to make this distinction more clear.

Done.

https://codereview.chromium.org/2807163002/diff/60001/chrome/browser/prerende...
chrome/browser/prerender/prerender_resource_throttle.cc:236:
prerender_contents->AddResourceThrottle(throttle);
On 2017/04/12 12:37:26, Charlie Harrison wrote:
> nit: newline below

Done.

https://codereview.chromium.org/2807163002/diff/60001/chrome/browser/prerende...
File chrome/browser/prerender/prerender_resource_throttle.h (left):

https://codereview.chromium.org/2807163002/diff/60001/chrome/browser/prerende...
chrome/browser/prerender/prerender_resource_throttle.h:91: int load_flags_;  //
Load flags to be OR'ed with the existing request flags.
On 2017/04/12 12:37:26, Charlie Harrison wrote:
> nit: newline below.

Done.

https://codereview.chromium.org/2807163002/diff/60001/content/browser/loader/...
File content/browser/loader/resource_scheduler.cc (right):

https://codereview.chromium.org/2807163002/diff/60001/content/browser/loader/...
content/browser/loader/resource_scheduler.cc:1083: void
ResourceScheduler::ReprioritizeRequest(net::URLRequest* request,
On 2017/04/12 12:37:26, Charlie Harrison wrote:
> Just curious: In practice, is this needed? I couldn't find any caller that
> actually uses the intra_priority params.

Yeah, I was wondering the same thing initially: intra_priority is almost always
0, so maybe we could remove it?

But there is actually one case where images are prioritized depending on their
"screen area":
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/La...

Not sure if this is really useful, but it does happen.

[Charlie Harrison, 2017-04-12 13:37:04]

https://codereview.chromium.org/2807163002/diff/60001/content/browser/loader/...
File content/browser/loader/resource_scheduler.cc (right):

https://codereview.chromium.org/2807163002/diff/60001/content/browser/loader/...
content/browser/loader/resource_scheduler.cc:1083: void
ResourceScheduler::ReprioritizeRequest(net::URLRequest* request,
On 2017/04/12 13:20:54, droger wrote:
> On 2017/04/12 12:37:26, Charlie Harrison wrote:
> > Just curious: In practice, is this needed? I couldn't find any caller that
> > actually uses the intra_priority params.
> 
> Yeah, I was wondering the same thing initially: intra_priority is almost
always
> 0, so maybe we could remove it?
> 
> But there is actually one case where images are prioritized depending on their
> "screen area":
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/La...
> 
> Not sure if this is really useful, but it does happen.

Ah good catch. Yeah until we address that we should make sure this patch doesn't
mess with them.

[commit-bot: I haz the power, 2017-04-12 14:07:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-12 14:07:23]

Dry run: This issue passed the CQ dry run.

[droger, 2017-04-13 13:41:13]

Patchset #6 (id:100001) has been deleted

[droger, 2017-04-13 13:44:12]

The CQ bit was checked by droger@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-13 13:44:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-13 14:09:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-13 14:09:53]

Dry run: Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[droger, 2017-04-13 14:42:52]

The CQ bit was checked by droger@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-13 14:43:12]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[droger, 2017-04-13 14:46:12]

Added browser tests and now changing the priority in the constructor.

This required changing a bit the ResourceScheduler::ForRequest() implementation
because it was crashing on unknown requests (not sure if it was a bug or not).
An alternative would be to call URLRequest::SetPriority() directly instead of
ResourceDispatcherHost::ReprioritizeRequest().

[droger, 2017-04-13 14:53:55]

droger@chromium.org changed reviewers:
+ mattcary@chromium.org

[droger, 2017-04-13 14:53:56]

+mattcary for prerender code, in particular the browser test which is not
trivial.

[commit-bot: I haz the power, 2017-04-13 15:44:15]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-13 15:44:16]

Dry run: This issue passed the CQ dry run.

[Charlie Harrison, 2017-04-13 15:50:26]

Hm, now I am confused how this works.

PrerenderResourceThrottle gets added before the resource scheduler.

ScheduledResourceRequest sets user data on the request (in its constructor) so
that ForRequest() works later.

So if PrerenderResourceThrottle calls ReprioritizeRequest, I'm not sure how the
ResourceScheduler can successfully look that up? Can you explain how this works
out?

I'm in favor of just setting the priority in the constructor with a comment
about how it needs to be before the ResourceScheduler.

[droger, 2017-04-13 15:54:22]

On 2017/04/13 15:50:26, Charlie Harrison wrote:
> Hm, now I am confused how this works.
> 
> PrerenderResourceThrottle gets added before the resource scheduler.
> 
> ScheduledResourceRequest sets user data on the request (in its constructor) so
> that ForRequest() works later.
> 
> So if PrerenderResourceThrottle calls ReprioritizeRequest, I'm not sure how
the
> ResourceScheduler can successfully look that up? Can you explain how this
works
> out?
> 
> I'm in favor of just setting the priority in the constructor with a comment
> about how it needs to be before the ResourceScheduler.

If the request scheduler does not know about the request, it just calls
SetPriority and returns.

See code in ReprioritizeRequest:
https://cs.chromium.org/chromium/src/content/browser/loader/resource_schedule...

So this is effectively the same as calling SetPriority directly, but I thought
it would be more robust to go through the ResourceScheduler (that way the
ResourceScheduler is the only place where priorities are managed, and hopefully
this is less likely to break if the ordering of events changes in the future).

[Charlie Harrison, 2017-04-13 15:58:21]

On 2017/04/13 15:54:22, droger wrote:
> On 2017/04/13 15:50:26, Charlie Harrison wrote:
> > Hm, now I am confused how this works.
> > 
> > PrerenderResourceThrottle gets added before the resource scheduler.
> > 
> > ScheduledResourceRequest sets user data on the request (in its constructor)
so
> > that ForRequest() works later.
> > 
> > So if PrerenderResourceThrottle calls ReprioritizeRequest, I'm not sure how
> the
> > ResourceScheduler can successfully look that up? Can you explain how this
> works
> > out?
> > 
> > I'm in favor of just setting the priority in the constructor with a comment
> > about how it needs to be before the ResourceScheduler.
> 
> If the request scheduler does not know about the request, it just calls
> SetPriority and returns.
> 
> See code in ReprioritizeRequest:
>
https://cs.chromium.org/chromium/src/content/browser/loader/resource_schedule...
> 
> So this is effectively the same as calling SetPriority directly, but I thought
> it would be more robust to go through the ResourceScheduler (that way the
> ResourceScheduler is the only place where priorities are managed, and
hopefully
> this is less likely to break if the ordering of events changes in the future).

Ah, right. Did not read down further. Actually I kind of like this solution as
long as it is documented that some of these requests will be tracked by the
ResourceScheduler, but we call into it before the they get scheduled.

[Charlie Harrison, 2017-04-14 00:00:26]

c/b/l LGTM but you'll need someone else for content/public.

[droger, 2017-04-14 08:48:06]

On 2017/04/13 15:58:21, Charlie Harrison wrote:
> On 2017/04/13 15:54:22, droger wrote:
> > On 2017/04/13 15:50:26, Charlie Harrison wrote:
> > > Hm, now I am confused how this works.
> > > 
> > > PrerenderResourceThrottle gets added before the resource scheduler.
> > > 
> > > ScheduledResourceRequest sets user data on the request (in its
constructor)
> so
> > > that ForRequest() works later.
> > > 
> > > So if PrerenderResourceThrottle calls ReprioritizeRequest, I'm not sure
how
> > the
> > > ResourceScheduler can successfully look that up? Can you explain how this
> > works
> > > out?
> > > 
> > > I'm in favor of just setting the priority in the constructor with a
comment
> > > about how it needs to be before the ResourceScheduler.
> > 
> > If the request scheduler does not know about the request, it just calls
> > SetPriority and returns.
> > 
> > See code in ReprioritizeRequest:
> >
>
https://cs.chromium.org/chromium/src/content/browser/loader/resource_schedule...
> > 
> > So this is effectively the same as calling SetPriority directly, but I
thought
> > it would be more robust to go through the ResourceScheduler (that way the
> > ResourceScheduler is the only place where priorities are managed, and
> hopefully
> > this is less likely to break if the ordering of events changes in the
future).
> 
> Ah, right. Did not read down further. Actually I kind of like this solution as
> long as it is documented that some of these requests will be tracked by the
> ResourceScheduler, but we call into it before the they get scheduled.

Thanks. Added a couple more comments in PrerenderResourceThrottle constructor
and resource_scheduler.h.

[droger, 2017-04-14 09:06:10]

+clamy for the new public method in
content/public/browser/resource_dispatcher_host.h

[droger, 2017-04-14 09:06:39]

droger@chromium.org changed reviewers:
+ clamy@chromium.org

[droger, 2017-04-14 09:06:39]

+clamy for real
for new function in 	content/public/browser/resource_dispatcher_host.h

[mattcary, 2017-04-14 09:28:46]

lgtm

https://codereview.chromium.org/2807163002/diff/160001/chrome/browser/prerend...
File chrome/browser/prerender/prerender_browsertest.cc (right):

https://codereview.chromium.org/2807163002/diff/160001/chrome/browser/prerend...
chrome/browser/prerender/prerender_browsertest.cc:3305: GURL after_swap_url =
embedded_test_server()->GetURL("/prerender/image.png");
nit: if this gets in after cl/2819523002 (which is likely as it's being
submitted now) then there is a new GetURLWithReplacement utility function to use
instead of doing all the replacement text stuff yourself.

https://codereview.chromium.org/2807163002/diff/160001/chrome/browser/prerend...
chrome/browser/prerender/prerender_browsertest.cc:3323:
base::Bind(get_priority_lambda, &before_swap_priority));
So before_swap_priority doesn't need to be base::Unretained because it's a basic
type and not a class? The documentation doesn't seem to be clear; I would have
thought that it would need to be unretained regardless.

https://codereview.chromium.org/2807163002/diff/160001/chrome/browser/prerend...
chrome/browser/prerender/prerender_browsertest.cc:3336: EXPECT_NE(net::IDLE,
before_swap_priority);
why not kDefaultImagePriority, as below?

https://codereview.chromium.org/2807163002/diff/160001/chrome/browser/prerend...
chrome/browser/prerender/prerender_browsertest.cc:3412: // Check priority after
swap.
Clarify that the test will timeout if the priority isn't changed as expected.

[droger, 2017-04-14 11:32:33]

The CQ bit was checked by droger@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-14 11:32:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[droger, 2017-04-14 11:38:36]

https://codereview.chromium.org/2807163002/diff/160001/chrome/browser/prerend...
File chrome/browser/prerender/prerender_browsertest.cc (right):

https://codereview.chromium.org/2807163002/diff/160001/chrome/browser/prerend...
chrome/browser/prerender/prerender_browsertest.cc:3305: GURL after_swap_url =
embedded_test_server()->GetURL("/prerender/image.png");
On 2017/04/14 09:28:46, mattcary wrote:
> nit: if this gets in after cl/2819523002 (which is likely as it's being
> submitted now) then there is a new GetURLWithReplacement utility function to
use
> instead of doing all the replacement text stuff yourself.

Done.

https://codereview.chromium.org/2807163002/diff/160001/chrome/browser/prerend...
chrome/browser/prerender/prerender_browsertest.cc:3323:
base::Bind(get_priority_lambda, &before_swap_priority));
On 2017/04/14 09:28:46, mattcary wrote:
> So before_swap_priority doesn't need to be base::Unretained because it's a
basic
> type and not a class? The documentation doesn't seem to be clear; I would have
> thought that it would need to be unretained regardless.

Done.

Unretained is often not used in these cases, but probably mostly because:

- Unretained actally does nothing, as opposed to other helpers such as Owned().

- the compiler will only enforce it for classes (and only for the callee object,
it will not even complain for the function parameters).

If you do:
Bind(&MyClass::MyFunction, my_instance, my_param), compilation will fail if you
omit the Unretained(my_instance). It does not check my_param though, and you can
get away without the Unretained there.


I'm not completely sure what is the best though. I made the change because I
could find a couple examples in the codebase, so it's probably a valid thing.

https://codereview.chromium.org/2807163002/diff/160001/chrome/browser/prerend...
chrome/browser/prerender/prerender_browsertest.cc:3336: EXPECT_NE(net::IDLE,
before_swap_priority);
On 2017/04/14 09:28:46, mattcary wrote:
> why not kDefaultImagePriority, as below?

Done.

https://codereview.chromium.org/2807163002/diff/160001/chrome/browser/prerend...
chrome/browser/prerender/prerender_browsertest.cc:3412: // Check priority after
swap.
On 2017/04/14 09:28:46, mattcary wrote:
> Clarify that the test will timeout if the priority isn't changed as expected.

Done, and changed the test a bit so that it should be harder to timeout. It will
now timeout if the priority is never updated. No longer timeout if the priority
is updated to a wrong value.

[commit-bot: I haz the power, 2017-04-14 13:07:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-14 13:07:05]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[droger, 2017-04-14 13:45:47]

Patchset #10 (id:200001) has been deleted

[clamy, 2017-04-14 13:46:37]

Thanks! content/public lgtm.

[droger, 2017-04-14 13:48:21]

The CQ bit was checked by droger@chromium.org to run a CQ dry run

[droger, 2017-04-14 13:48:29]

Patchset #10 (id:220001) has been deleted

[commit-bot: I haz the power, 2017-04-14 13:48:31]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-14 14:45:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-14 14:45:50]

Dry run: This issue passed the CQ dry run.

[droger, 2017-04-14 14:50:52]

The CQ bit was checked by droger@chromium.org

[droger, 2017-04-14 14:50:53]

The patchset sent to the CQ was uploaded after l-g-t-m from
csharrison@chromium.org, mattcary@chromium.org, clamy@chromium.org
Link to the patchset: https://codereview.chromium.org/2807163002/#ps240001
(title: "Fix test: default image priority can be LOWEST or MEDIUM")

[commit-bot: I haz the power, 2017-04-14 14:51:02]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-14 14:56:11]

CQ is committing da patch.

Bot data: {"patchset_id": 240001, "attempt_start_ts": 1492181452817830,
"parent_rev": "9352bae3ebd8ae0dcfe70b115f9ad78af9f82e12", "commit_rev":
"d3bc6148c7f0d2695ec2ca74652d996e2c4f5a6e"}

[commit-bot: I haz the power, 2017-04-14 14:57:02]

Description was changed from

==========
[Prerender] Restore request priorities when swapped in

Requests from prerendered contents have a IDLE priority, in order not to slow
down visible pages.
However, when the prerendered contents become visible, the request priorities
were not reset back to their correct values, leading to bad prerendering
performance.

In this CL, the priority management for prerender requests is moved to the
PrerenderResourceThrottle/PrerendereContents.
The original priorities are stored in the throttle, and restored when the
prerender contents swaps in.
A new public function is added to ResourceDispatcherHost to update a request
priority, and its implementation reuses the existing code that updates the image
priorities.

The prerender contents keeps a list of all the network resources that were
started while the prerender is hidden. If this proves to be too large, pruning
the list when responses are received should be doable.

BUG=705955
==========

to

==========
[Prerender] Restore request priorities when swapped in

Requests from prerendered contents have a IDLE priority, in order not to slow
down visible pages.
However, when the prerendered contents become visible, the request priorities
were not reset back to their correct values, leading to bad prerendering
performance.

In this CL, the priority management for prerender requests is moved to the
PrerenderResourceThrottle/PrerendereContents.
The original priorities are stored in the throttle, and restored when the
prerender contents swaps in.
A new public function is added to ResourceDispatcherHost to update a request
priority, and its implementation reuses the existing code that updates the image
priorities.

The prerender contents keeps a list of all the network resources that were
started while the prerender is hidden. If this proves to be too large, pruning
the list when responses are received should be doable.

BUG=705955

Review-Url: https://codereview.chromium.org/2807163002
Cr-Commit-Position: refs/heads/master@{#464728}
Committed:
https://chromium.googlesource.com/chromium/src/+/d3bc6148c7f0d2695ec2ca74652d...
==========

[commit-bot: I haz the power, 2017-04-14 14:57:04]

Committed patchset #10 (id:240001) as
https://chromium.googlesource.com/chromium/src/+/d3bc6148c7f0d2695ec2ca74652d...