Navigation transitions: Block first response until after transitions have run.

Navigation transitions: Block first response until after transitions have run.

Overall idea was to use the CSRH to pause a navigation while the transition page gets a chance to load and run, then resume the navigation afterwards. If there's no navigation transition, the response is started right away.

There are no callers for this yet, transition elements are sent via IPC to the TransitionNavigationManager. The CSRH will see this in OnResponseStarted, block the request immediately, and post a task to the UI thread to check if the embedder wants to handle the transition.

Navigation Transitions: 

Design doc: https://docs.google.com/a/chromium.org/document/d/17jg1RRL3RI969cLwbKBIcoGDsPwqaEdBxafGNYGwiY4/edit# 
Implementation details: https://docs.google.com/a/chromium.org/document/d/1kREPtFJaeLoDKwrfmrYTD7DHCdxX1RzFBga2gNY8lyE/edit#heading=h.bng2kpmyvxq5
BUG=370696
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=278926

[shatch, 2014-05-23 20:48:14]

ptal!

I've included changes to observers/delegates to receive notifications about the
response being deferred, but I could move those into a new cl if you'd prefer.

[shatch, 2014-05-28 22:22:53]

On 2014/05/23 20:48:14, shatch wrote:
> ptal!
> 
> I've included changes to observers/delegates to receive notifications about
the
> response being deferred, but I could move those into a new cl if you'd prefer.

ping

[nasko, 2014-05-28 22:58:59]

It is a good start, but needs some more work. I would suggest running git cl
format to ensure the code is conformant to the style guide.
The biggest item is that we shouldn't be adding any code to RenderViewHost,
since it doesn't perform navigations anymore. It is all handled by
RenderFrameHost.

https://codereview.chromium.org/297973002/diff/100001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_delegate.h (right):

https://codereview.chromium.org/297973002/diff/100001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_delegate.h:58: // Navigation is
blocked waiting for transition to occur.
nit: Add a comment that this is called only for the main frame.

https://codereview.chromium.org/297973002/diff/100001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_manager.h (right):

https://codereview.chromium.org/297973002/diff/100001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.h:221: // Recieved a
response from cross site resource handler. If
nit: CrossSiteResourceHandler
Also, single "If" looks like missing the rest of the comment.

https://codereview.chromium.org/297973002/diff/100001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.h:235: void
ResumeResponseDeferredAtStart();
nit: Move this definition up, so it is grouped together with other calls coming
from the IO thread.

https://codereview.chromium.org/297973002/diff/100001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.h:347: struct
ResponseStartedParams {
Do we really need a struct just to wrap a single struct?

https://codereview.chromium.org/297973002/diff/100001/content/browser/loader/...
File content/browser/loader/cross_site_resource_handler.cc (right):

https://codereview.chromium.org/297973002/diff/100001/content/browser/loader/...
content/browser/loader/cross_site_resource_handler.cc:89: const GlobalRequestID&
global_request_id, int render_frame_id) {
style: parameters need to be on line of their own.

https://codereview.chromium.org/297973002/diff/100001/content/browser/loader/...
content/browser/loader/cross_site_resource_handler.cc:94: if (rfh) {
style: no need for braces here

https://codereview.chromium.org/297973002/diff/100001/content/browser/loader/...
content/browser/loader/cross_site_resource_handler.cc:245: BrowserThread::UI,
style: wrong indent

https://codereview.chromium.org/297973002/diff/100001/content/browser/loader/...
content/browser/loader/cross_site_resource_handler.cc:251: *defer = true;
Shouldn't there be a call to OnDidDefer()?

https://codereview.chromium.org/297973002/diff/100001/content/browser/loader/...
File content/browser/loader/cross_site_resource_handler.h (right):

https://codereview.chromium.org/297973002/diff/100001/content/browser/loader/...
content/browser/loader/cross_site_resource_handler.h:56: // Navigations are
deferred at response started to parse out any navigation
nit: OnResponseStarted

https://codereview.chromium.org/297973002/diff/100001/content/browser/loader/...
content/browser/loader/cross_site_resource_handler.h:74: bool
OnNavigationTransitionResponseStarted(int request_id,
style: parameters should be on separate lines.

https://codereview.chromium.org/297973002/diff/100001/content/browser/loader/...
content/browser/loader/cross_site_resource_handler.h:77: bool
OnNormalResponseStarted(int request_id, ResourceResponse* response,
style: parameters should be on separate lines.

https://codereview.chromium.org/297973002/diff/100001/content/browser/rendere...
File content/browser/renderer_host/render_view_host_impl.h (right):

https://codereview.chromium.org/297973002/diff/100001/content/browser/rendere...
content/browser/renderer_host/render_view_host_impl.h:336: void
SetHasPendingTransitionRequest(bool has_pending_request);
Please don't add any more code to RenderViewHost, as it is actively being
slimmed down and scheduled for extinction. This code belongs to RenderFrameHost.

https://codereview.chromium.org/297973002/diff/100001/content/browser/transit...
File content/browser/transition_request_manager.cc (right):

https://codereview.chromium.org/297973002/diff/100001/content/browser/transit...
content/browser/transition_request_manager.cc:1: // Copyright (c) 2014 The
Chromium Authors. All rights reserved.
No "(c)" needed.

https://codereview.chromium.org/297973002/diff/100001/content/browser/transit...
File content/browser/transition_request_manager.h (right):

https://codereview.chromium.org/297973002/diff/100001/content/browser/transit...
content/browser/transition_request_manager.h:31: bool
HasPendingTransitionRequest(int renderer_id, int render_view_id);
What is renderer_id? Is it the process id or something else? It should be named
in a self-describing way.

https://codereview.chromium.org/297973002/diff/100001/content/browser/transit...
content/browser/transition_request_manager.h:33: // Sets whether the
RenderViewHost specified by the given IDs currently has a
Navigations happen through RenderFrameHost these days, so we should stay away
from using RenderViewHosts here.

https://codereview.chromium.org/297973002/diff/100001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.h (right):

https://codereview.chromium.org/297973002/diff/100001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.h:319: 
nit: extra blank line

https://codereview.chromium.org/297973002/diff/100001/content/public/browser/...
File content/public/browser/web_contents.h (right):

https://codereview.chromium.org/297973002/diff/100001/content/public/browser/...
content/public/browser/web_contents.h:608: virtual void
ResumeResponseDeferredAtStart() = 0;
This needs comment.

[shatch, 2014-05-29 21:41:22]

New snapshot uploaded, ptal!

https://codereview.chromium.org/297973002/diff/100001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_delegate.h (right):

https://codereview.chromium.org/297973002/diff/100001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_delegate.h:58: // Navigation is
blocked waiting for transition to occur.
On 2014/05/28 22:59:00, nasko wrote:
> nit: Add a comment that this is called only for the main frame.

Done.

https://codereview.chromium.org/297973002/diff/100001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_manager.h (right):

https://codereview.chromium.org/297973002/diff/100001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.h:221: // Recieved a
response from cross site resource handler. If
On 2014/05/28 22:59:00, nasko wrote:
> nit: CrossSiteResourceHandler
> Also, single "If" looks like missing the rest of the comment.

Done.

https://codereview.chromium.org/297973002/diff/100001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.h:235: void
ResumeResponseDeferredAtStart();
On 2014/05/28 22:59:00, nasko wrote:
> nit: Move this definition up, so it is grouped together with other calls
coming
> from the IO thread.

Done.

https://codereview.chromium.org/297973002/diff/100001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.h:347: struct
ResponseStartedParams {
On 2014/05/28 22:59:00, nasko wrote:
> Do we really need a struct just to wrap a single struct?

Changed the response_started_params_ to a scoped_ptr to the request id.

https://codereview.chromium.org/297973002/diff/100001/content/browser/loader/...
File content/browser/loader/cross_site_resource_handler.cc (right):

https://codereview.chromium.org/297973002/diff/100001/content/browser/loader/...
content/browser/loader/cross_site_resource_handler.cc:89: const GlobalRequestID&
global_request_id, int render_frame_id) {
On 2014/05/28 22:59:00, nasko wrote:
> style: parameters need to be on line of their own.

Done.

https://codereview.chromium.org/297973002/diff/100001/content/browser/loader/...
content/browser/loader/cross_site_resource_handler.cc:94: if (rfh) {
On 2014/05/28 22:59:00, nasko wrote:
> style: no need for braces here

Done.

https://codereview.chromium.org/297973002/diff/100001/content/browser/loader/...
content/browser/loader/cross_site_resource_handler.cc:245: BrowserThread::UI,
On 2014/05/28 22:59:00, nasko wrote:
> style: wrong indent

Done.

https://codereview.chromium.org/297973002/diff/100001/content/browser/loader/...
content/browser/loader/cross_site_resource_handler.cc:251: *defer = true;
On 2014/05/28 22:59:00, nasko wrote:
> Shouldn't there be a call to OnDidDefer()?

Done.

https://codereview.chromium.org/297973002/diff/100001/content/browser/loader/...
File content/browser/loader/cross_site_resource_handler.h (right):

https://codereview.chromium.org/297973002/diff/100001/content/browser/loader/...
content/browser/loader/cross_site_resource_handler.h:56: // Navigations are
deferred at response started to parse out any navigation
On 2014/05/28 22:59:00, nasko wrote:
> nit: OnResponseStarted

Done.

https://codereview.chromium.org/297973002/diff/100001/content/browser/loader/...
content/browser/loader/cross_site_resource_handler.h:74: bool
OnNavigationTransitionResponseStarted(int request_id,
On 2014/05/28 22:59:00, nasko wrote:
> style: parameters should be on separate lines.

Done.

https://codereview.chromium.org/297973002/diff/100001/content/browser/loader/...
content/browser/loader/cross_site_resource_handler.h:77: bool
OnNormalResponseStarted(int request_id, ResourceResponse* response,
On 2014/05/28 22:59:00, nasko wrote:
> style: parameters should be on separate lines.

Done.

https://codereview.chromium.org/297973002/diff/100001/content/browser/rendere...
File content/browser/renderer_host/render_view_host_impl.h (right):

https://codereview.chromium.org/297973002/diff/100001/content/browser/rendere...
content/browser/renderer_host/render_view_host_impl.h:336: void
SetHasPendingTransitionRequest(bool has_pending_request);
On 2014/05/28 22:59:00, nasko wrote:
> Please don't add any more code to RenderViewHost, as it is actively being
> slimmed down and scheduled for extinction. This code belongs to
RenderFrameHost.

Done.

https://codereview.chromium.org/297973002/diff/100001/content/browser/transit...
File content/browser/transition_request_manager.cc (right):

https://codereview.chromium.org/297973002/diff/100001/content/browser/transit...
content/browser/transition_request_manager.cc:1: // Copyright (c) 2014 The
Chromium Authors. All rights reserved.
On 2014/05/28 22:59:00, nasko wrote:
> No "(c)" needed.

Done.

https://codereview.chromium.org/297973002/diff/100001/content/browser/transit...
File content/browser/transition_request_manager.h (right):

https://codereview.chromium.org/297973002/diff/100001/content/browser/transit...
content/browser/transition_request_manager.h:31: bool
HasPendingTransitionRequest(int renderer_id, int render_view_id);
On 2014/05/28 22:59:00, nasko wrote:
> What is renderer_id? Is it the process id or something else? It should be
named
> in a self-describing way.

Done.

https://codereview.chromium.org/297973002/diff/100001/content/browser/transit...
content/browser/transition_request_manager.h:33: // Sets whether the
RenderViewHost specified by the given IDs currently has a
On 2014/05/28 22:59:00, nasko wrote:
> Navigations happen through RenderFrameHost these days, so we should stay away
> from using RenderViewHosts here.

Changed the comment to reflect being called by the RFH instead. I was just using
the RVH's id since I could get at that from both the CSRH and from the
RenderFrameHostManager, which seems to be the way the CrossSiteRequestManager
tracks requests too. Is there something else I could use here instead?

https://codereview.chromium.org/297973002/diff/100001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.h (right):

https://codereview.chromium.org/297973002/diff/100001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.h:319: 
On 2014/05/28 22:59:00, nasko wrote:
> nit: extra blank line

Done.

https://codereview.chromium.org/297973002/diff/100001/content/public/browser/...
File content/public/browser/web_contents.h (right):

https://codereview.chromium.org/297973002/diff/100001/content/public/browser/...
content/public/browser/web_contents.h:608: virtual void
ResumeResponseDeferredAtStart() = 0;
On 2014/05/28 22:59:00, nasko wrote:
> This needs comment.

Done.

[nasko, 2014-06-02 21:59:33]

Few more comments, but it is getting there!

https://codereview.chromium.org/297973002/diff/100001/content/browser/transit...
File content/browser/transition_request_manager.h (right):

https://codereview.chromium.org/297973002/diff/100001/content/browser/transit...
content/browser/transition_request_manager.h:33: // Sets whether the
RenderViewHost specified by the given IDs currently has a
On 2014/05/29 21:41:22, shatch wrote:
> On 2014/05/28 22:59:00, nasko wrote:
> > Navigations happen through RenderFrameHost these days, so we should stay
away
> > from using RenderViewHosts here.
> 
> Changed the comment to reflect being called by the RFH instead. I was just
using
> the RVH's id since I could get at that from both the CSRH and from the
> RenderFrameHostManager, which seems to be the way the CrossSiteRequestManager
> tracks requests too. Is there something else I could use here instead?

I can see this called only from
RenderFrameHostImpl::SetHasPendingTransitionRequest. Where do we need the
RenderFrameHost routing ID that we can't get it?

https://codereview.chromium.org/297973002/diff/160001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/297973002/diff/160001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:904: void
RenderFrameHostImpl::SetHasPendingTransitionRequest(
Does this method ever get called with value of true? If not, let's call it
Reset* instead of Set*.

https://codereview.chromium.org/297973002/diff/160001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:908:
render_view_host_->GetRoutingID(),
Why use the routing id of the RVH and not the RFH one?

https://codereview.chromium.org/297973002/diff/160001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_manager.h (right):

https://codereview.chromium.org/297973002/diff/160001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.h:222: // is not a
navigation transition, this is not called. If this is called,
nit: "does not require navigation transition". The navigation itself is going to
happen regardless if there is transition specified or not.

https://codereview.chromium.org/297973002/diff/160001/content/browser/transit...
File content/browser/transition_request_manager.h (right):

https://codereview.chromium.org/297973002/diff/160001/content/browser/transit...
content/browser/transition_request_manager.h:29: // Returns whether the
RenderViewHost specified by the given IDs currently
Let's use RenderFrameHost, RenderViewHost doesn't perform navigations anymore.

[shatch, 2014-06-02 23:40:39]

New snapshot uploaded, ptal

https://codereview.chromium.org/297973002/diff/100001/content/browser/transit...
File content/browser/transition_request_manager.h (right):

https://codereview.chromium.org/297973002/diff/100001/content/browser/transit...
content/browser/transition_request_manager.h:33: // Sets whether the
RenderViewHost specified by the given IDs currently has a
On 2014/06/02 21:59:33, nasko wrote:
> On 2014/05/29 21:41:22, shatch wrote:
> > On 2014/05/28 22:59:00, nasko wrote:
> > > Navigations happen through RenderFrameHost these days, so we should stay
> away
> > > from using RenderViewHosts here.
> > 
> > Changed the comment to reflect being called by the RFH instead. I was just
> using
> > the RVH's id since I could get at that from both the CSRH and from the
> > RenderFrameHostManager, which seems to be the way the
CrossSiteRequestManager
> > tracks requests too. Is there something else I could use here instead?
> 
> I can see this called only from
> RenderFrameHostImpl::SetHasPendingTransitionRequest. Where do we need the
> RenderFrameHost routing ID that we can't get it?

Moved everything to RFH per offline discussion.

https://codereview.chromium.org/297973002/diff/160001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/297973002/diff/160001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:904: void
RenderFrameHostImpl::SetHasPendingTransitionRequest(
On 2014/06/02 21:59:33, nasko wrote:
> Does this method ever get called with value of true? If not, let's call it
> Reset* instead of Set*.

Yeah, this will get called eventually by the embedder.

https://codereview.chromium.org/297973002/diff/160001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:908:
render_view_host_->GetRoutingID(),
On 2014/06/02 21:59:33, nasko wrote:
> Why use the routing id of the RVH and not the RFH one?

Changed to RFH routing id per offline discussion.

https://codereview.chromium.org/297973002/diff/160001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_manager.h (right):

https://codereview.chromium.org/297973002/diff/160001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.h:222: // is not a
navigation transition, this is not called. If this is called,
On 2014/06/02 21:59:33, nasko wrote:
> nit: "does not require navigation transition". The navigation itself is going
to
> happen regardless if there is transition specified or not.

Reworded it a bit, ptal.

https://codereview.chromium.org/297973002/diff/160001/content/browser/transit...
File content/browser/transition_request_manager.h (right):

https://codereview.chromium.org/297973002/diff/160001/content/browser/transit...
content/browser/transition_request_manager.h:29: // Returns whether the
RenderViewHost specified by the given IDs currently
On 2014/06/02 21:59:33, nasko wrote:
> Let's use RenderFrameHost, RenderViewHost doesn't perform navigations anymore.

Done.

[nasko, 2014-06-03 16:07:43]

I think the code is looking good. One last thing I would like to see is a test
to ensure this new machinery is working properly. Since we don't have an
embedder in the chrome/ code that exercises this, it will be good to add a test
that drives the transition.

[shatch, 2014-06-04 20:53:05]

New snapshot uploaded, ptal

[nasko, 2014-06-09 22:51:27]

I think this is looking good. Time to loop an OWNERS reviewer and since you are
changing content/public, I would suggest sending it to jam@.

https://codereview.chromium.org/297973002/diff/220001/content/browser/loader/...
File content/browser/loader/cross_site_resource_handler.h (right):

https://codereview.chromium.org/297973002/diff/220001/content/browser/loader/...
content/browser/loader/cross_site_resource_handler.h:61: // Returns whether the
handler is deferred. This is designed for the
nit: When a method is used only by tests, it should get a ForTesting suffix.

[shatch, 2014-06-09 23:27:31]

Hi jam, nasko recommended you as another reviewer for this cl, ptal!

https://codereview.chromium.org/297973002/diff/220001/content/browser/loader/...
File content/browser/loader/cross_site_resource_handler.h (right):

https://codereview.chromium.org/297973002/diff/220001/content/browser/loader/...
content/browser/loader/cross_site_resource_handler.h:61: // Returns whether the
handler is deferred. This is designed for the
On 2014/06/09 22:51:26, nasko wrote:
> nit: When a method is used only by tests, it should get a ForTesting suffix.

Done.

[jam, 2014-06-10 05:54:56]

https://codereview.chromium.org/297973002/diff/240001/content/public/browser/...
File content/public/browser/web_contents.h (right):

https://codereview.chromium.org/297973002/diff/240001/content/public/browser/...
content/public/browser/web_contents.h:352: virtual void
SetHasPendingTransitionRequest(bool has_pending_transition) = 0;
is the only caller of this the test? if so, it should have a ForTesting suffix.

also, since these methods aren't called by chrome, they shouldn't be on the
public interface. i.e. they should only be on WebContentsImpl. see
http://www.chromium.org/developers/content-module/content-api

https://codereview.chromium.org/297973002/diff/240001/content/public/browser/...
File content/public/browser/web_contents_observer.h (right):

https://codereview.chromium.org/297973002/diff/240001/content/public/browser/...
content/public/browser/web_contents_observer.h:251: virtual void
DidDeferAfterResponseStarted() {}
similarly, since this notification is only consumed inside content, no need to
add this to the public WebContentsObserver. have the code which calls this
callback instead call the one impl of this directly

[shatch, 2014-06-10 20:20:24]

New snapshot uploaded, ptal!

https://codereview.chromium.org/297973002/diff/240001/content/public/browser/...
File content/public/browser/web_contents.h (right):

https://codereview.chromium.org/297973002/diff/240001/content/public/browser/...
content/public/browser/web_contents.h:352: virtual void
SetHasPendingTransitionRequest(bool has_pending_transition) = 0;
On 2014/06/10 05:54:56, jam wrote:
> is the only caller of this the test? if so, it should have a ForTesting
suffix.
> 
> also, since these methods aren't called by chrome, they shouldn't be on the
> public interface. i.e. they should only be on WebContentsImpl. see
> http://www.chromium.org/developers/content-module/content-api

The intention was for the java side to control the transition, and call
SetHasPendingTransitionRequest through ContentViewCore (which looks like I
missed, added to the CL).

Cool, removed these from WebContents, only exist on WebContentsImpl now.

https://codereview.chromium.org/297973002/diff/240001/content/public/browser/...
File content/public/browser/web_contents_observer.h (right):

https://codereview.chromium.org/297973002/diff/240001/content/public/browser/...
content/public/browser/web_contents_observer.h:251: virtual void
DidDeferAfterResponseStarted() {}
On 2014/06/10 05:54:56, jam wrote:
> similarly, since this notification is only consumed inside content, no need to
> add this to the public WebContentsObserver. have the code which calls this
> callback instead call the one impl of this directly

I thought this was needed this to make its way to the java
WebContentsObserverAndroid so that the transition helper can listen for the
event. This gets sent through
RenderFrameHostImpl->WebContentsImpl->WebContentsObserverAndroid->java. Is there
another way to do that?

[jam, 2014-06-11 02:00:21]

https://codereview.chromium.org/297973002/diff/240001/content/public/browser/...
File content/public/browser/web_contents_observer.h (right):

https://codereview.chromium.org/297973002/diff/240001/content/public/browser/...
content/public/browser/web_contents_observer.h:251: virtual void
DidDeferAfterResponseStarted() {}
On 2014/06/10 20:20:24, shatch wrote:
> On 2014/06/10 05:54:56, jam wrote:
> > similarly, since this notification is only consumed inside content, no need
to
> > add this to the public WebContentsObserver. have the code which calls this
> > callback instead call the one impl of this directly
> 
> I thought this was needed this to make its way to the java
> WebContentsObserverAndroid so that the transition helper can listen for the
> event. This gets sent through
> RenderFrameHostImpl->WebContentsImpl->WebContentsObserverAndroid->java. Is
there
> another way to do that?

WebContentsImpl can call the ContentViewCoreImpl method directly (getting it
through ContentViewCoreImpl::FromWebContents).

https://codereview.chromium.org/297973002/diff/260001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/297973002/diff/260001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:907: GetProcess()->GetID(),
GetRoutingID(), has_pending_request);
nit: routing_id_ instead of GetRoutingID, which is what we use in this file

https://codereview.chromium.org/297973002/diff/260001/content/browser/loader/...
File content/browser/loader/cross_site_resource_handler.h (right):

https://codereview.chromium.org/297973002/diff/260001/content/browser/loader/...
content/browser/loader/cross_site_resource_handler.h:63: bool
is_deferred_for_testing() const { return did_defer_; }
nit: name should be did_defer_for_testing. also no need for the second sentence
in the comment, if someone wants to find callers they can search for them

https://codereview.chromium.org/297973002/diff/260001/content/browser/rendere...
File content/browser/renderer_host/render_process_host_impl.h (right):

https://codereview.chromium.org/297973002/diff/260001/content/browser/rendere...
content/browser/renderer_host/render_process_host_impl.h:130: virtual void
ResumeDeferredNavigation(const GlobalRequestID& request_id)
i think you copied the pattern for this one by putting it in the virtual
section. i see that the only caller to this is inside content. if you can move
this to RenderProcessHostImpl as a method and out of RenderProcessHost interface
with the change below, that would be great

https://codereview.chromium.org/297973002/diff/260001/content/browser/rendere...
content/browser/renderer_host/render_process_host_impl.h:133: const
GlobalRequestID& request_id) OVERRIDE;
this isn't overridding a method from the interface (which makes sense, since
it's not called by chrome), so put this as a non-virtual method

https://codereview.chromium.org/297973002/diff/260001/content/browser/transit...
File content/browser/transition_request_manager.h (right):

https://codereview.chromium.org/297973002/diff/260001/content/browser/transit...
content/browser/transition_request_manager.h:55: RenderFrameSet
pending_transition_frames_;
cross thread maps in general are bad for performance and maintainability. see
http://dev.chromium.org/developers/design-documents/threading#Overview. 

how often does CrossSiteResourceHandler need this? For every request? For every
main frame? Only for every cross-site request?

Either way, sine navigation is moving to the browser process as part of site
isolation and the navigation-in-browser project by the folks in the paris
office, i would presume that more of navigation will be on the UI thread?

[shatch, 2014-06-14 00:41:37]

New snapshot uploaded, ptal.

https://codereview.chromium.org/297973002/diff/240001/content/public/browser/...
File content/public/browser/web_contents_observer.h (right):

https://codereview.chromium.org/297973002/diff/240001/content/public/browser/...
content/public/browser/web_contents_observer.h:251: virtual void
DidDeferAfterResponseStarted() {}
On 2014/06/11 02:00:21, jam wrote:
> On 2014/06/10 20:20:24, shatch wrote:
> > On 2014/06/10 05:54:56, jam wrote:
> > > similarly, since this notification is only consumed inside content, no
need
> to
> > > add this to the public WebContentsObserver. have the code which calls this
> > > callback instead call the one impl of this directly
> > 
> > I thought this was needed this to make its way to the java
> > WebContentsObserverAndroid so that the transition helper can listen for the
> > event. This gets sent through
> > RenderFrameHostImpl->WebContentsImpl->WebContentsObserverAndroid->java. Is
> there
> > another way to do that?
> 
> WebContentsImpl can call the ContentViewCoreImpl method directly (getting it
> through ContentViewCoreImpl::FromWebContents).

Ah great, thanks! Moved this to call ContentViewCoreImpl directly and
reimplemented the java side as a listener on ContentViewCore instead.

https://codereview.chromium.org/297973002/diff/260001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/297973002/diff/260001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:907: GetProcess()->GetID(),
GetRoutingID(), has_pending_request);
On 2014/06/11 02:00:21, jam wrote:
> nit: routing_id_ instead of GetRoutingID, which is what we use in this file

Done.

https://codereview.chromium.org/297973002/diff/260001/content/browser/loader/...
File content/browser/loader/cross_site_resource_handler.h (right):

https://codereview.chromium.org/297973002/diff/260001/content/browser/loader/...
content/browser/loader/cross_site_resource_handler.h:63: bool
is_deferred_for_testing() const { return did_defer_; }
On 2014/06/11 02:00:21, jam wrote:
> nit: name should be did_defer_for_testing. also no need for the second
sentence
> in the comment, if someone wants to find callers they can search for them

That variable is used internally to check if the request was already deferred
before resuming, the function was added to expose it for tests.

Removed the second sentence in the comment.

https://codereview.chromium.org/297973002/diff/260001/content/browser/rendere...
File content/browser/renderer_host/render_process_host_impl.h (right):

https://codereview.chromium.org/297973002/diff/260001/content/browser/rendere...
content/browser/renderer_host/render_process_host_impl.h:130: virtual void
ResumeDeferredNavigation(const GlobalRequestID& request_id)
On 2014/06/11 02:00:21, jam wrote:
> i think you copied the pattern for this one by putting it in the virtual
> section. i see that the only caller to this is inside content. if you can move
> this to RenderProcessHostImpl as a method and out of RenderProcessHost
interface
> with the change below, that would be great

Done.

https://codereview.chromium.org/297973002/diff/260001/content/browser/rendere...
content/browser/renderer_host/render_process_host_impl.h:133: const
GlobalRequestID& request_id) OVERRIDE;
On 2014/06/11 02:00:21, jam wrote:
> this isn't overridding a method from the interface (which makes sense, since
> it's not called by chrome), so put this as a non-virtual method

Done.

https://codereview.chromium.org/297973002/diff/260001/content/browser/transit...
File content/browser/transition_request_manager.h (right):

https://codereview.chromium.org/297973002/diff/260001/content/browser/transit...
content/browser/transition_request_manager.h:55: RenderFrameSet
pending_transition_frames_;
On 2014/06/11 02:00:21, jam wrote:
> cross thread maps in general are bad for performance and maintainability. see
> http://dev.chromium.org/developers/design-documents/threading#Overview. 
> 
> how often does CrossSiteResourceHandler need this? For every request? For
every
> main frame? Only for every cross-site request?
> 
> Either way, sine navigation is moving to the browser process as part of site
> isolation and the navigation-in-browser project by the folks in the paris
> office, i would presume that more of navigation will be on the UI thread?

Ok, I've reworked it and I think I have a much better solution! Instead of the
cross thread map, the plan is to have an IPC sent to the IO thread that gets
caught by the TransitionRequestMessageFilter and communicates with the
TransitionRequestManager to deliver the transition elements. When we reach
CSRH::OnResponseStarted and there are transition elements, a task is posted to
the UI thread to see if the embedder will handle the transition. If not, the
navigation is resumed. Under normal navigations, there's no additional cost
beyond the lookup in the TransitionRequestManager.

Had a meeting with Darin and Nasko a few weeks ago to go over the CSRH changes
here, and make sure there was nothing in these changes that wasn't compatible
with the new navigation plans. IIRC the CSRH may move/disappear but being able
to defer right at the start of the response didn't seem to be a problem.

https://codereview.chromium.org/297973002/diff/280001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.h (right):

https://codereview.chromium.org/297973002/diff/280001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.h:643:
did_defer_callback_for_testing_ = callback;
Wanted a way to get a callback for the browsertests so that we could see that
DidDeferAfterResponseStarted was being called correctly, and resume the
navigation. Is there a better way to do this?

[jam, 2014-06-16 03:46:10]

https://codereview.chromium.org/297973002/diff/260001/content/browser/loader/...
File content/browser/loader/cross_site_resource_handler.h (right):

https://codereview.chromium.org/297973002/diff/260001/content/browser/loader/...
content/browser/loader/cross_site_resource_handler.h:63: bool
is_deferred_for_testing() const { return did_defer_; }
On 2014/06/14 00:41:37, shatch wrote:
> On 2014/06/11 02:00:21, jam wrote:
> > nit: name should be did_defer_for_testing. also no need for the second
> sentence
> > in the comment, if someone wants to find callers they can search for them
> 
> That variable is used internally to check if the request was already deferred
> before resuming, the function was added to expose it for tests.
> 
> Removed the second sentence in the comment.

per
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Names,
this should be did_defer_for_testing()

https://codereview.chromium.org/297973002/diff/260001/content/browser/transit...
File content/browser/transition_request_manager.h (right):

https://codereview.chromium.org/297973002/diff/260001/content/browser/transit...
content/browser/transition_request_manager.h:55: RenderFrameSet
pending_transition_frames_;
great that you got rid of the cross thread map, thanks.

one question, who sends TransitionRequestHostMsg_SetHasPendingTransitionRequest?

https://codereview.chromium.org/297973002/diff/320001/content/browser/transit...
File content/browser/transition_browsertest.cc (right):

https://codereview.chromium.org/297973002/diff/320001/content/browser/transit...
content/browser/transition_browsertest.cc:67:
static_cast<WebContentsImpl*>(shell_->web_contents())->
WebContents isn't thread safe, so it shouldn't be called on anything but the UI
thread

https://codereview.chromium.org/297973002/diff/320001/content/browser/transit...
File content/browser/transition_request_message_filter.cc (right):

https://codereview.chromium.org/297973002/diff/320001/content/browser/transit...
content/browser/transition_request_message_filter.cc:26: &message, &param)) {
hmm, since this is just dispatching one IPC, do this in RenderMessageFilter. no
point in adding another BMF just for this, unless you forsee a class of IPCs for
this feature (and if so, we can do split it out then). same for the IPC message
class and separate messages files. just put this in frame_messages.h

also when you move it, always use the standard IPC macros to crack and dispatch
IPCs, i.e. IPC_MESSAGE_HANDLER

https://codereview.chromium.org/297973002/diff/320001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/297973002/diff/320001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:3494: return true;
why are you stopping the code below from running in tests?

https://codereview.chromium.org/297973002/diff/320001/content/common/transiti...
File content/common/transition_request_messages.h (right):

https://codereview.chromium.org/297973002/diff/320001/content/common/transiti...
content/common/transition_request_messages.h:15:
IPC_MESSAGE_ROUTED1(TransitionRequestHostMsg_SetHasPendingTransitionRequest,
since the ipc is dispatched in one class, it should be a CONTROL ipc. the first
parameter can be render_frame_id

[shatch, 2014-06-16 22:32:20]

New snapshot uploaded, ptal.

https://codereview.chromium.org/297973002/diff/260001/content/browser/loader/...
File content/browser/loader/cross_site_resource_handler.h (right):

https://codereview.chromium.org/297973002/diff/260001/content/browser/loader/...
content/browser/loader/cross_site_resource_handler.h:63: bool
is_deferred_for_testing() const { return did_defer_; }
On 2014/06/16 03:46:10, jam wrote:
> On 2014/06/14 00:41:37, shatch wrote:
> > On 2014/06/11 02:00:21, jam wrote:
> > > nit: name should be did_defer_for_testing. also no need for the second
> > sentence
> > > in the comment, if someone wants to find callers they can search for them
> > 
> > That variable is used internally to check if the request was already
deferred
> > before resuming, the function was added to expose it for tests.
> > 
> > Removed the second sentence in the comment.
> 
> per
> http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Names,
> this should be did_defer_for_testing()

Done.

https://codereview.chromium.org/297973002/diff/260001/content/browser/transit...
File content/browser/transition_request_manager.h (right):

https://codereview.chromium.org/297973002/diff/260001/content/browser/transit...
content/browser/transition_request_manager.h:55: RenderFrameSet
pending_transition_frames_;
On 2014/06/16 03:46:10, jam wrote:
> great that you got rid of the cross thread map, thanks.
> 
> one question, who sends
TransitionRequestHostMsg_SetHasPendingTransitionRequest?

Right now the plan is to have it sent by blink when initiating a navigation.
This CL: https://codereview.chromium.org/319573008/ gets the serialized elements
from the document. Once that lands, the RFH can send the IPC.

https://codereview.chromium.org/297973002/diff/320001/content/browser/transit...
File content/browser/transition_browsertest.cc (right):

https://codereview.chromium.org/297973002/diff/320001/content/browser/transit...
content/browser/transition_browsertest.cc:67:
static_cast<WebContentsImpl*>(shell_->web_contents())->
On 2014/06/16 03:46:10, jam wrote:
> WebContents isn't thread safe, so it shouldn't be called on anything but the
UI
> thread

Done.

https://codereview.chromium.org/297973002/diff/320001/content/browser/transit...
File content/browser/transition_request_message_filter.cc (right):

https://codereview.chromium.org/297973002/diff/320001/content/browser/transit...
content/browser/transition_request_message_filter.cc:26: &message, &param)) {
On 2014/06/16 03:46:10, jam wrote:
> hmm, since this is just dispatching one IPC, do this in RenderMessageFilter.
no
> point in adding another BMF just for this, unless you forsee a class of IPCs
for
> this feature (and if so, we can do split it out then). same for the IPC
message
> class and separate messages files. just put this in frame_messages.h
> 
> also when you move it, always use the standard IPC macros to crack and
dispatch
> IPCs, i.e. IPC_MESSAGE_HANDLER

Done.

https://codereview.chromium.org/297973002/diff/320001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/297973002/diff/320001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:3494: return true;
On 2014/06/16 03:46:10, jam wrote:
> why are you stopping the code below from running in tests?

Figured for the test we already know we want to the response to defer, so we can
just return true from here immediately. The WillHandleDeferAfterResponseStarted
will just return true/false on whether the response should be deferred, but in
the case of the test that's always true but since there's no embedder here to
actually implement the transition, it'll return false and we won't get the call
to DidDeferAfterResponseStarted.

https://codereview.chromium.org/297973002/diff/320001/content/common/transiti...
File content/common/transition_request_messages.h (right):

https://codereview.chromium.org/297973002/diff/320001/content/common/transiti...
content/common/transition_request_messages.h:15:
IPC_MESSAGE_ROUTED1(TransitionRequestHostMsg_SetHasPendingTransitionRequest,
On 2014/06/16 03:46:10, jam wrote:
> since the ipc is dispatched in one class, it should be a CONTROL ipc. the
first
> parameter can be render_frame_id

Done.

[jam, 2014-06-16 23:08:33]

https://codereview.chromium.org/297973002/diff/320001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/297973002/diff/320001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:3494: return true;
On 2014/06/16 22:32:19, shatch wrote:
> On 2014/06/16 03:46:10, jam wrote:
> > why are you stopping the code below from running in tests?
> 
> Figured for the test we already know we want to the response to defer, so we
can
> just return true from here immediately. The
WillHandleDeferAfterResponseStarted
> will just return true/false on whether the response should be deferred, but in
> the case of the test that's always true but since there's no embedder here to
> actually implement the transition,

Can you explain this to me some more? I think you're talking about the java side
which I'm not very familiar with.

Can the test set up a test embedder that replies true? That way we can avoid
adding test-specific code in production code.

> it'll return false and we won't get the call
> to DidDeferAfterResponseStarted.

[shatch, 2014-06-18 00:07:37]

New snapshot uploaded, ptal!

Per-offline chat, setup a test embedder that replies true to transition queries.
Refactored browsertest to just test for response being deferred.

[jam, 2014-06-18 00:34:25]

lgtm

https://codereview.chromium.org/297973002/diff/400001/content/browser/rendere...
File content/browser/renderer_host/render_message_filter.h (right):

https://codereview.chromium.org/297973002/diff/400001/content/browser/rendere...
content/browser/renderer_host/render_message_filter.h:283: void
OnSetHasPendingTransitionRequest(int frame_id, bool is_transition);
nit: rendre_frame_id here as well, and in the cc file

https://codereview.chromium.org/297973002/diff/400001/content/common/frame_me...
File content/common/frame_messages.h (right):

https://codereview.chromium.org/297973002/diff/400001/content/common/frame_me...
content/common/frame_messages.h:613: int /* frame_id */,
nit: render_frame_id for consistency

https://codereview.chromium.org/297973002/diff/400001/ipc/ipc_message_start.h
File ipc/ipc_message_start.h (right):

https://codereview.chromium.org/297973002/diff/400001/ipc/ipc_message_start.h...
ipc/ipc_message_start.h:107: TransitionRequestMsgStart,
nit: not needed anymore

[nasko, 2014-06-18 17:39:18]

I think it is good, but I want to see how this whole machinery is getting kicked
off and I can't find it.

https://codereview.chromium.org/297973002/diff/420001/content/common/frame_me...
File content/common/frame_messages.h (right):

https://codereview.chromium.org/297973002/diff/420001/content/common/frame_me...
content/common/frame_messages.h:612:
IPC_MESSAGE_CONTROL2(FrameHostMsg_SetHasPendingTransitionRequest,
Where is this IPC sent from? I don't see it in this CL.

[shatch, 2014-06-18 17:42:26]

On 2014/06/18 17:39:18, nasko wrote:
> I think it is good, but I want to see how this whole machinery is getting
kicked
> off and I can't find it.
> 
>
https://codereview.chromium.org/297973002/diff/420001/content/common/frame_me...
> File content/common/frame_messages.h (right):
> 
>
https://codereview.chromium.org/297973002/diff/420001/content/common/frame_me...
> content/common/frame_messages.h:612:
> IPC_MESSAGE_CONTROL2(FrameHostMsg_SetHasPendingTransitionRequest,
> Where is this IPC sent from? I don't see it in this CL.

Right now the plan is to have it sent by blink when initiating a navigation.
This CL: https://codereview.chromium.org/319573008/ gets the serialized elements
from the document. Once that lands, the RFH can send the IPC.

[nasko, 2014-06-18 18:00:43]

LGTM

[shatch, 2014-06-20 16:40:51]

The CQ bit was checked by simonhatch@chromium.org

[commit-bot: I haz the power, 2014-06-20 16:43:17]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/simonhatch@chromium.org/297973002/440001

[commit-bot: I haz the power, 2014-06-20 17:33:16]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_aosp on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/8...)
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)
  linux_chromium_chromeos_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/19603)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/23235)

[commit-bot: I haz the power, 2014-06-20 17:41:44]

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

[commit-bot: I haz the power, 2014-06-20 17:41:45]

Try jobs failed on following builders:
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/19639)

[shatch, 2014-06-20 23:42:41]

The CQ bit was checked by simonhatch@chromium.org

[commit-bot: I haz the power, 2014-06-20 23:45:24]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/simonhatch@chromium.org/297973002/460001

[commit-bot: I haz the power, 2014-06-21 05:29:20]

Change committed as 278926