Make PrerenderTracker's resource_throttle_io_thread_map_ use RenderFrame IDs instead of RenderView …

Make PrerenderTracker's resource_throttle_io_thread_map_ use RenderFrame IDs instead of RenderView IDs.

BUG=304341
R=davidben@chromium.org, tburkard@chromium.org

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

[tburkard, 2014-01-17 11:26:11]

davidben@ should def. review this too since he wrote the resourcethrottle code.

Added a few comments, I think if you intend to move
resource_throttle_io_thread_map_ to use RenderFrame IDs, there are a few pieces
missing. Perhaps you are only doing preparations work and not actually making
that switch yet (in that case you should adjust the title of the change).

Also, assuming the changes that I described were made, I think this could work,
but one issue might be concurrency: what if, in the browser process on the UI
thread, it was determined that the prerender is swapped, while on the IO thread,
we just after that process a request to be delayed that is still tagged as
prerendered visibility. We'd need to "remember" what RenderView ID's had just
been swapped, so that we can adjust the request when it comes in.

Maybe an easier idea would be to inform PrerenderTracker every time a new
RenderFrame is added to the prerender, and keep that on the IO thread (which
seems like we will have to do that regardless). Then, we can look up whether a
request is for a prerender based on the RenderFrame on the IO thread. This is
ultimately duplicating state of the render tree from the UI thread to another
thread, which is I know what you are trying to avoid, but I don't see a good way
around it.

Curious to hear your comments. Maybe I am misunderstanding how you are trying to
make this work.

Thanks.

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

https://codereview.chromium.org/141163002/diff/640002/chrome/browser/prerende...
chrome/browser/prerender/prerender_resource_throttle.cc:172: int route_id =
info->GetRouteID();
So you said transition to RenderFrameID vs. RenderViewID.

But here, when adding the resource to the throttle, it seems like the
RenderViewID is still used to add it?

Did you intend to change this to GetRenderFrameID()?

https://codereview.chromium.org/141163002/diff/640002/chrome/browser/prerende...
File chrome/browser/prerender/prerender_tracker.cc (right):

https://codereview.chromium.org/141163002/diff/640002/chrome/browser/prerende...
chrome/browser/prerender/prerender_tracker.cc:117: void
PrerenderTracker::AddPrerenderOnIOThread(
Assuming you want to convert resource_throttle_io_thread_map_ to
RenderFrameID's, this would need to change, too. It would have to be called for
all RenderFrameID's, rather than just the RenderViewID.

It would also have to be re-called when a new RenderFrame gets created in a
prerendered RenderView.

[jam, 2014-01-17 17:02:45]

ah thanks for the thorough review, I'll update the cl and let you know. David:
no need to review until then.

[jam, 2014-01-19 23:50:39]

On 2014/01/17 17:02:45, jam wrote:
> ah thanks for the thorough review, I'll update the cl and let you know. David:
> no need to review until then.

ptal. I realized that we don't need to keep this map in PrerenderTracker since
we insert into it based on work that happens on the UI thread. Instead I add
these throttles inside PrerenderContents. The code is simpler as a result.

[tburkard, 2014-01-20 10:01:37]

lgtm, but please wait for David's review as well.

Very elegant!

I don't see any more issues from a correctness point of view now.

I was first concerned that this might add the thread jumping back and forth to
all requests, but it doesn't. It might require additional thread switching
compared to before when a prerender just got swapped in, but that should be
fine.

[davidben, 2014-01-20 20:51:57]

LGTM! I like moving the throttles to the PrerenderContents instead of keying off
a random global. (I think it even removes a thread-hop actually, which is even
better.)

https://codereview.chromium.org/141163002/diff/1010004/chrome/browser/prerend...
File chrome/browser/prerender/prerender_resource_throttle.cc (right):

https://codereview.chromium.org/141163002/diff/1010004/chrome/browser/prerend...
chrome/browser/prerender/prerender_resource_throttle.cc:132:
prerender_contents->AddResourceThrottle(throttle);
This is fine (the assumption the code is making is one I think we shouldn't
break), but I went to verify this, so my articulating it for posterity is
probably worthwhile:

This is relying on the fact that PrerenderContents cannot be in the middle of
being used (i.e. after PrepareForUse is called but before it's destructed), but
that's not a possible state here, and I think we should keep it that way. In
fact, the prerender gets removed from active_prerenders_ before PrepareForUse is
called. Otherwise the throttle would get stuck.

https://codereview.chromium.org/141163002/diff/1010004/content/public/browser...
File content/public/browser/resource_request_info.h (right):

https://codereview.chromium.org/141163002/diff/1010004/content/public/browser...
content/public/browser/resource_request_info.h:93: // Returns the associated
visibility state.
Nit: Maybe make the comment clear that this is the visibility state /at the time
the request was started in the renderer/. (For instance, GetVisibilityState() ==
prerender + invalid request for a prerender isn't sufficient grounds for
canceling the request. Still need to bounce back to the UI thread and get the
canonical state, as this code does.)

[jam, 2014-01-21 00:30:08]

TBR=nasko for a trivial addition to 	 content/common/resource_messages.h

https://codereview.chromium.org/141163002/diff/1010004/chrome/browser/prerend...
File chrome/browser/prerender/prerender_resource_throttle.cc (right):

https://codereview.chromium.org/141163002/diff/1010004/chrome/browser/prerend...
chrome/browser/prerender/prerender_resource_throttle.cc:132:
prerender_contents->AddResourceThrottle(throttle);
On 2014/01/20 20:51:57, David Benjamin wrote:
> This is fine (the assumption the code is making is one I think we shouldn't
> break), but I went to verify this, so my articulating it for posterity is
> probably worthwhile:
> 
> This is relying on the fact that PrerenderContents cannot be in the middle of
> being used (i.e. after PrepareForUse is called but before it's destructed),
but
> that's not a possible state here, and I think we should keep it that way. In
> fact, the prerender gets removed from active_prerenders_ before PrepareForUse
is
> called. Otherwise the throttle would get stuck.

yep precisely, I was worried about this case as well but looking through the
code showed it's not possible

https://codereview.chromium.org/141163002/diff/1010004/content/public/browser...
File content/public/browser/resource_request_info.h (right):

https://codereview.chromium.org/141163002/diff/1010004/content/public/browser...
content/public/browser/resource_request_info.h:93: // Returns the associated
visibility state.
On 2014/01/20 20:51:57, David Benjamin wrote:
> Nit: Maybe make the comment clear that this is the visibility state /at the
time
> the request was started in the renderer/. (For instance, GetVisibilityState()
==
> prerender + invalid request for a prerender isn't sufficient grounds for
> canceling the request. Still need to bounce back to the UI thread and get the
> canonical state, as this code does.)

Done.

[nasko, 2014-01-21 18:19:21]

A comment on the IPC change.

https://codereview.chromium.org/141163002/diff/1010004/content/common/resourc...
File content/common/resource_messages.h (right):

https://codereview.chromium.org/141163002/diff/1010004/content/common/resourc...
content/common/resource_messages.h:144:
IPC_STRUCT_MEMBER(blink::WebPageVisibilityState, visiblity_state)
Since this is an enum, we should add at least IPC_ENUM_TRAITS. Prefered will be
IPC_ENUM_TRAITS_MAX_VALUE, but it doesn't look it has a predefined max value in
the enum itself.

[jam, 2014-01-21 18:54:09]

https://codereview.chromium.org/141163002/diff/1010004/content/common/resourc...
File content/common/resource_messages.h (right):

https://codereview.chromium.org/141163002/diff/1010004/content/common/resourc...
content/common/resource_messages.h:144:
IPC_STRUCT_MEMBER(blink::WebPageVisibilityState, visiblity_state)
On 2014/01/21 18:19:21, nasko wrote:
> Since this is an enum, we should add at least IPC_ENUM_TRAITS. 

I had added it in content_param_traits_macros.h

> Prefered will be
> IPC_ENUM_TRAITS_MAX_VALUE, but it doesn't look it has a predefined max value
in
> the enum itself.

right, so that's why I used IPC_ENUM_TRAITS