Perform navigation policy check on UI thread for --site-per-process.

Perform navigation policy check on UI thread for --site-per-process.

This CL adds a NavigateFrameToURL method, which will allow browser tests to navigate specific (sub)frames and more precise out-of-process iframes tests can be written. 

It also changes how cross-site transition is done for --site-per-process. It used to depend on the Referer header, but if there is a navigation to data URL, it will be empty and there will always be a transfer to new renderer.
I'm changing it to do the check on the UI thread and for now to be a very simplistic one - is the URL in the same SiteInstance as the current renderer.

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

[nasko, 2014-04-24 19:02:34]

Hey Charlie,
I think this CL is ready for review. 
Thanks,
Nasko

[nasko, 2014-04-24 19:03:19]

Adding site-isolation-reviews@.

[Charlie Reis, 2014-04-24 20:37:50]

Cool-- I'm excited to have the new test function working so nicely.  There's a
lot of comments here, but it's mostly cleanup stuff.

https://codereview.chromium.org/248963007/diff/50001/content/browser/loader/c...
File content/browser/loader/cross_site_resource_handler.cc (right):

https://codereview.chromium.org/248963007/diff/50001/content/browser/loader/c...
content/browser/loader/cross_site_resource_handler.cc:89:
RenderFrameHostImpl::FromID(process_id, render_frame_id);
We should check for null here, in case the tab was closed during the hop from IO
to UI thread.

https://codereview.chromium.org/248963007/diff/50001/content/browser/loader/c...
content/browser/loader/cross_site_resource_handler.cc:94: bool transfer =
!SiteInstance::IsSameWebSite(
Why not just return from here?

https://codereview.chromium.org/248963007/diff/50001/content/browser/loader/c...
content/browser/loader/cross_site_resource_handler.cc:154: // WebUI iframes but
don't have referrers.
We can drop the "but don't have referrers" part now.  Maybe say "host multiple
cross-site WebUI iframes" instead, since that's the part we can't support.

https://codereview.chromium.org/248963007/diff/50001/content/browser/loader/c...
content/browser/loader/cross_site_resource_handler.cc:159: GURL
referrer(request()->referrer());
referrer looks unused now.

https://codereview.chromium.org/248963007/diff/50001/content/browser/loader/c...
content/browser/loader/cross_site_resource_handler.cc:160: // Store the
response_ object internally, since it is needed when the flow
Can we move this whole block out to a helper function, like
DeferForNavigationPolicyCheck?  It should have a TODO to merge it into the same
UI thread hop as StartCrossSiteTransition, rather than having two hops.

https://codereview.chromium.org/248963007/diff/50001/content/browser/loader/c...
content/browser/loader/cross_site_resource_handler.cc:162: response_ = response;
Hmm, this is subtle.  It means this value is set in the non-transfer case,
unlike before.  I think it's only right because StartCrossSiteTransition and
ResumeResponse are the only ones that look at it, and we need ResumeResponse in
the non-transfer case here thanks to the defer below.  Maybe it's worth
mentioning in the comment that this is needed because we defer the request
whether we choose to transfer it or not.

https://codereview.chromium.org/248963007/diff/50001/content/browser/loader/c...
content/browser/loader/cross_site_resource_handler.cc:164: // Always send the
navigation to the UI thread to make a policy decision. It
nit: send the navigation to the -> defer to the

https://codereview.chromium.org/248963007/diff/50001/content/browser/loader/c...
content/browser/loader/cross_site_resource_handler.cc:167:
BrowserThread::PostTaskAndReplyWithResult(
This function and the use of WeakPtr here is still a bit magic to me.  Can you
have Albert take a look to make sure it's correct?

https://codereview.chromium.org/248963007/diff/50001/content/browser/loader/c...
content/browser/loader/cross_site_resource_handler.cc:175:
weak_ptr_factory_.GetWeakPtr()));
I'm sad that we need to add a weak pointer factory here, but I agree that it's
possible for this to get deleted in the mean time.  We've gotten by without it
before because ResumeDeferredNavigation has a null ResourceLoader check in
ResourceDispatcherHostImpl.

Can this just be a temporary thing (with a TODO to remove the factory) until we
combine the two UI hops into one?

https://codereview.chromium.org/248963007/diff/50001/content/browser/site_per...
File content/browser/site_per_process_browsertest.cc (left):

https://codereview.chromium.org/248963007/diff/50001/content/browser/site_per...
content/browser/site_per_process_browsertest.cc:206: StartFrameAtDataURL();
Interesting.  This isn't required anymore?  Is that because we hear the commit
even in the case of AUTO_SUBFRAME?

https://codereview.chromium.org/248963007/diff/50001/content/browser/site_per...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/248963007/diff/50001/content/browser/site_per...
content/browser/site_per_process_browsertest.cc:235:
child->current_frame_host()->render_view_host()->GetSiteInstance();
nit: Wrong indent.  Also, RenderFrameHostImpl has GetSiteInstance now, so no
need for the render_view_host() call.

https://codereview.chromium.org/248963007/diff/50001/content/browser/site_per...
content/browser/site_per_process_browsertest.cc:261:
child->current_frame_host()->render_view_host()->GetSiteInstance());
nit: Drop render_view_host()

https://codereview.chromium.org/248963007/diff/50001/content/public/test/cont...
File content/public/test/content_browser_test_utils.cc (right):

https://codereview.chromium.org/248963007/diff/50001/content/public/test/cont...
content/public/test/content_browser_test_utils.cc:63: rfh->NavigateToURL(url);
Hmm, I'm not sure we want to use RFH::NavigateToURL, do we?  Seems like it would
be more representative to go through NavigationController::LoadURLWithParams, as
in
https://codereview.chromium.org/209153003/diff/1/content/browser/site_per_pro....
 Does that have problems?

Similarly, I'm uncertain if we want this method to take in a RFH or a
FrameTreeNode.  I don't think we would want people trying to navigate RFHs that
aren't the current RFH, for example.

https://codereview.chromium.org/248963007/diff/50001/content/test/test_frame_...
File content/test/test_frame_navigation_observer.cc (right):

https://codereview.chromium.org/248963007/diff/50001/content/test/test_frame_...
content/test/test_frame_navigation_observer.cc:56: if (frame_id ==
render_frame_host_->GetRoutingID()) {
nit: No braces needed.

https://codereview.chromium.org/248963007/diff/50001/content/test/test_frame_...
File content/test/test_frame_navigation_observer.h (right):

https://codereview.chromium.org/248963007/diff/50001/content/test/test_frame_...
content/test/test_frame_navigation_observer.h:29: // |web_contents|.
nit: There's no |web_contents| parameter.

https://codereview.chromium.org/248963007/diff/50001/content/test/test_frame_...
content/test/test_frame_navigation_observer.h:32: // Like above but waits for
one navigation.
nit: Like -> As

https://codereview.chromium.org/248963007/diff/50001/content/test/test_frame_...
content/test/test_frame_navigation_observer.h:51: 
nit: No blank line, since this is the block of WebContentsObserver overrides.

[nasko, 2014-04-24 22:31:12]

Fixes for all comments.

dcheng@, can you review the usage of PostTaskAndReplyWithResult?

https://codereview.chromium.org/248963007/diff/50001/content/browser/loader/c...
File content/browser/loader/cross_site_resource_handler.cc (right):

https://codereview.chromium.org/248963007/diff/50001/content/browser/loader/c...
content/browser/loader/cross_site_resource_handler.cc:89:
RenderFrameHostImpl::FromID(process_id, render_frame_id);
On 2014/04/24 20:37:50, Charlie Reis wrote:
> We should check for null here, in case the tab was closed during the hop from
IO
> to UI thread.

Do'h, I had a check here.

https://codereview.chromium.org/248963007/diff/50001/content/browser/loader/c...
content/browser/loader/cross_site_resource_handler.cc:94: bool transfer =
!SiteInstance::IsSameWebSite(
On 2014/04/24 20:37:50, Charlie Reis wrote:
> Why not just return from here?

Done.

https://codereview.chromium.org/248963007/diff/50001/content/browser/loader/c...
content/browser/loader/cross_site_resource_handler.cc:154: // WebUI iframes but
don't have referrers.
On 2014/04/24 20:37:50, Charlie Reis wrote:
> We can drop the "but don't have referrers" part now.  Maybe say "host multiple
> cross-site WebUI iframes" instead, since that's the part we can't support.

Done.

https://codereview.chromium.org/248963007/diff/50001/content/browser/loader/c...
content/browser/loader/cross_site_resource_handler.cc:159: GURL
referrer(request()->referrer());
On 2014/04/24 20:37:50, Charlie Reis wrote:
> referrer looks unused now.

Done.

https://codereview.chromium.org/248963007/diff/50001/content/browser/loader/c...
content/browser/loader/cross_site_resource_handler.cc:160: // Store the
response_ object internally, since it is needed when the flow
On 2014/04/24 20:37:50, Charlie Reis wrote:
> Can we move this whole block out to a helper function, like
> DeferForNavigationPolicyCheck?  It should have a TODO to merge it into the
same
> UI thread hop as StartCrossSiteTransition, rather than having two hops.

Done.

https://codereview.chromium.org/248963007/diff/50001/content/browser/loader/c...
content/browser/loader/cross_site_resource_handler.cc:162: response_ = response;
On 2014/04/24 20:37:50, Charlie Reis wrote:
> Hmm, this is subtle.  It means this value is set in the non-transfer case,
> unlike before.  I think it's only right because StartCrossSiteTransition and
> ResumeResponse are the only ones that look at it, and we need ResumeResponse
in
> the non-transfer case here thanks to the defer below.  Maybe it's worth
> mentioning in the comment that this is needed because we defer the request
> whether we choose to transfer it or not.

Done.

https://codereview.chromium.org/248963007/diff/50001/content/browser/loader/c...
content/browser/loader/cross_site_resource_handler.cc:164: // Always send the
navigation to the UI thread to make a policy decision. It
On 2014/04/24 20:37:50, Charlie Reis wrote:
> nit: send the navigation to the -> defer to the

Done.

https://codereview.chromium.org/248963007/diff/50001/content/browser/loader/c...
content/browser/loader/cross_site_resource_handler.cc:167:
BrowserThread::PostTaskAndReplyWithResult(
On 2014/04/24 20:37:50, Charlie Reis wrote:
> This function and the use of WeakPtr here is still a bit magic to me.  Can you
> have Albert take a look to make sure it's correct?

I'll add dcheng@ to review.

https://codereview.chromium.org/248963007/diff/50001/content/browser/loader/c...
content/browser/loader/cross_site_resource_handler.cc:175:
weak_ptr_factory_.GetWeakPtr()));
On 2014/04/24 20:37:50, Charlie Reis wrote:
> I'm sad that we need to add a weak pointer factory here, but I agree that it's
> possible for this to get deleted in the mean time.  We've gotten by without it
> before because ResumeDeferredNavigation has a null ResourceLoader check in
> ResourceDispatcherHostImpl.
> 
> Can this just be a temporary thing (with a TODO to remove the factory) until
we
> combine the two UI hops into one?

I could do away without the factory, but then we risk crashes. I'll put a TODO.

https://codereview.chromium.org/248963007/diff/50001/content/browser/site_per...
File content/browser/site_per_process_browsertest.cc (left):

https://codereview.chromium.org/248963007/diff/50001/content/browser/site_per...
content/browser/site_per_process_browsertest.cc:206: StartFrameAtDataURL();
On 2014/04/24 20:37:50, Charlie Reis wrote:
> Interesting.  This isn't required anymore?  Is that because we hear the commit
> even in the case of AUTO_SUBFRAME?

It isn't needed since we don't listen for NavEntryCommitted, rather listen for
DidCommit.

https://codereview.chromium.org/248963007/diff/50001/content/browser/site_per...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/248963007/diff/50001/content/browser/site_per...
content/browser/site_per_process_browsertest.cc:235:
child->current_frame_host()->render_view_host()->GetSiteInstance();
On 2014/04/24 20:37:50, Charlie Reis wrote:
> nit: Wrong indent.  Also, RenderFrameHostImpl has GetSiteInstance now, so no
> need for the render_view_host() call.

Done.

https://codereview.chromium.org/248963007/diff/50001/content/browser/site_per...
content/browser/site_per_process_browsertest.cc:261:
child->current_frame_host()->render_view_host()->GetSiteInstance());
On 2014/04/24 20:37:50, Charlie Reis wrote:
> nit: Drop render_view_host()

Done.

https://codereview.chromium.org/248963007/diff/50001/content/public/test/cont...
File content/public/test/content_browser_test_utils.cc (right):

https://codereview.chromium.org/248963007/diff/50001/content/public/test/cont...
content/public/test/content_browser_test_utils.cc:63: rfh->NavigateToURL(url);
On 2014/04/24 20:37:50, Charlie Reis wrote:
> Hmm, I'm not sure we want to use RFH::NavigateToURL, do we?  Seems like it
would
> be more representative to go through NavigationController::LoadURLWithParams,
as
> in
>
https://codereview.chromium.org/209153003/diff/1/content/browser/site_per_pro....
>  Does that have problems?
> 
> Similarly, I'm uncertain if we want this method to take in a RFH or a
> FrameTreeNode.  I don't think we would want people trying to navigate RFHs
that
> aren't the current RFH, for example.

Done.

https://codereview.chromium.org/248963007/diff/50001/content/test/test_frame_...
File content/test/test_frame_navigation_observer.cc (right):

https://codereview.chromium.org/248963007/diff/50001/content/test/test_frame_...
content/test/test_frame_navigation_observer.cc:56: if (frame_id ==
render_frame_host_->GetRoutingID()) {
On 2014/04/24 20:37:50, Charlie Reis wrote:
> nit: No braces needed.

Done.

https://codereview.chromium.org/248963007/diff/50001/content/test/test_frame_...
File content/test/test_frame_navigation_observer.h (right):

https://codereview.chromium.org/248963007/diff/50001/content/test/test_frame_...
content/test/test_frame_navigation_observer.h:29: // |web_contents|.
On 2014/04/24 20:37:50, Charlie Reis wrote:
> nit: There's no |web_contents| parameter.

Done.

https://codereview.chromium.org/248963007/diff/50001/content/test/test_frame_...
content/test/test_frame_navigation_observer.h:32: // Like above but waits for
one navigation.
On 2014/04/24 20:37:50, Charlie Reis wrote:
> nit: Like -> As

Done.

https://codereview.chromium.org/248963007/diff/50001/content/test/test_frame_...
content/test/test_frame_navigation_observer.h:51: 
On 2014/04/24 20:37:50, Charlie Reis wrote:
> nit: No blank line, since this is the block of WebContentsObserver overrides.

Done.

[awong, 2014-04-24 23:00:35]

LGTM for PTAR usage.

It's pretty much a textbook invocation of PTAR to execute a function on some
other thread and then process the result on the originating one, using a
WeakPtr<> to allow for the case were the source object is deleted during the
asynchronous call.

On 2014/04/24 22:31:12, nasko wrote:
> Fixes for all comments.
> 
> dcheng@, can you review the usage of PostTaskAndReplyWithResult?
> 
>
https://codereview.chromium.org/248963007/diff/50001/content/browser/loader/c...
> File content/browser/loader/cross_site_resource_handler.cc (right):
> 
>
https://codereview.chromium.org/248963007/diff/50001/content/browser/loader/c...
> content/browser/loader/cross_site_resource_handler.cc:89:
> RenderFrameHostImpl::FromID(process_id, render_frame_id);
> On 2014/04/24 20:37:50, Charlie Reis wrote:
> > We should check for null here, in case the tab was closed during the hop
from
> IO
> > to UI thread.
> 
> Do'h, I had a check here.
> 
>
https://codereview.chromium.org/248963007/diff/50001/content/browser/loader/c...
> content/browser/loader/cross_site_resource_handler.cc:94: bool transfer =
> !SiteInstance::IsSameWebSite(
> On 2014/04/24 20:37:50, Charlie Reis wrote:
> > Why not just return from here?
> 
> Done.
> 
>
https://codereview.chromium.org/248963007/diff/50001/content/browser/loader/c...
> content/browser/loader/cross_site_resource_handler.cc:154: // WebUI iframes
but
> don't have referrers.
> On 2014/04/24 20:37:50, Charlie Reis wrote:
> > We can drop the "but don't have referrers" part now.  Maybe say "host
multiple
> > cross-site WebUI iframes" instead, since that's the part we can't support.
> 
> Done.
> 
>
https://codereview.chromium.org/248963007/diff/50001/content/browser/loader/c...
> content/browser/loader/cross_site_resource_handler.cc:159: GURL
> referrer(request()->referrer());
> On 2014/04/24 20:37:50, Charlie Reis wrote:
> > referrer looks unused now.
> 
> Done.
> 
>
https://codereview.chromium.org/248963007/diff/50001/content/browser/loader/c...
> content/browser/loader/cross_site_resource_handler.cc:160: // Store the
> response_ object internally, since it is needed when the flow
> On 2014/04/24 20:37:50, Charlie Reis wrote:
> > Can we move this whole block out to a helper function, like
> > DeferForNavigationPolicyCheck?  It should have a TODO to merge it into the
> same
> > UI thread hop as StartCrossSiteTransition, rather than having two hops.
> 
> Done.
> 
>
https://codereview.chromium.org/248963007/diff/50001/content/browser/loader/c...
> content/browser/loader/cross_site_resource_handler.cc:162: response_ =
response;
> On 2014/04/24 20:37:50, Charlie Reis wrote:
> > Hmm, this is subtle.  It means this value is set in the non-transfer case,
> > unlike before.  I think it's only right because StartCrossSiteTransition and
> > ResumeResponse are the only ones that look at it, and we need ResumeResponse
> in
> > the non-transfer case here thanks to the defer below.  Maybe it's worth
> > mentioning in the comment that this is needed because we defer the request
> > whether we choose to transfer it or not.
> 
> Done.
> 
>
https://codereview.chromium.org/248963007/diff/50001/content/browser/loader/c...
> content/browser/loader/cross_site_resource_handler.cc:164: // Always send the
> navigation to the UI thread to make a policy decision. It
> On 2014/04/24 20:37:50, Charlie Reis wrote:
> > nit: send the navigation to the -> defer to the
> 
> Done.
> 
>
https://codereview.chromium.org/248963007/diff/50001/content/browser/loader/c...
> content/browser/loader/cross_site_resource_handler.cc:167:
> BrowserThread::PostTaskAndReplyWithResult(
> On 2014/04/24 20:37:50, Charlie Reis wrote:
> > This function and the use of WeakPtr here is still a bit magic to me.  Can
you
> > have Albert take a look to make sure it's correct?
> 
> I'll add dcheng@ to review.
> 
>
https://codereview.chromium.org/248963007/diff/50001/content/browser/loader/c...
> content/browser/loader/cross_site_resource_handler.cc:175:
> weak_ptr_factory_.GetWeakPtr()));
> On 2014/04/24 20:37:50, Charlie Reis wrote:
> > I'm sad that we need to add a weak pointer factory here, but I agree that
it's
> > possible for this to get deleted in the mean time.  We've gotten by without
it
> > before because ResumeDeferredNavigation has a null ResourceLoader check in
> > ResourceDispatcherHostImpl.
> > 
> > Can this just be a temporary thing (with a TODO to remove the factory) until
> we
> > combine the two UI hops into one?
> 
> I could do away without the factory, but then we risk crashes. I'll put a
TODO.
> 
>
https://codereview.chromium.org/248963007/diff/50001/content/browser/site_per...
> File content/browser/site_per_process_browsertest.cc (left):
> 
>
https://codereview.chromium.org/248963007/diff/50001/content/browser/site_per...
> content/browser/site_per_process_browsertest.cc:206: StartFrameAtDataURL();
> On 2014/04/24 20:37:50, Charlie Reis wrote:
> > Interesting.  This isn't required anymore?  Is that because we hear the
commit
> > even in the case of AUTO_SUBFRAME?
> 
> It isn't needed since we don't listen for NavEntryCommitted, rather listen for
> DidCommit.
> 
>
https://codereview.chromium.org/248963007/diff/50001/content/browser/site_per...
> File content/browser/site_per_process_browsertest.cc (right):
> 
>
https://codereview.chromium.org/248963007/diff/50001/content/browser/site_per...
> content/browser/site_per_process_browsertest.cc:235:
> child->current_frame_host()->render_view_host()->GetSiteInstance();
> On 2014/04/24 20:37:50, Charlie Reis wrote:
> > nit: Wrong indent.  Also, RenderFrameHostImpl has GetSiteInstance now, so no
> > need for the render_view_host() call.
> 
> Done.
> 
>
https://codereview.chromium.org/248963007/diff/50001/content/browser/site_per...
> content/browser/site_per_process_browsertest.cc:261:
> child->current_frame_host()->render_view_host()->GetSiteInstance());
> On 2014/04/24 20:37:50, Charlie Reis wrote:
> > nit: Drop render_view_host()
> 
> Done.
> 
>
https://codereview.chromium.org/248963007/diff/50001/content/public/test/cont...
> File content/public/test/content_browser_test_utils.cc (right):
> 
>
https://codereview.chromium.org/248963007/diff/50001/content/public/test/cont...
> content/public/test/content_browser_test_utils.cc:63: rfh->NavigateToURL(url);
> On 2014/04/24 20:37:50, Charlie Reis wrote:
> > Hmm, I'm not sure we want to use RFH::NavigateToURL, do we?  Seems like it
> would
> > be more representative to go through
NavigationController::LoadURLWithParams,
> as
> > in
> >
>
https://codereview.chromium.org/209153003/diff/1/content/browser/site_per_pro....
> >  Does that have problems?
> > 
> > Similarly, I'm uncertain if we want this method to take in a RFH or a
> > FrameTreeNode.  I don't think we would want people trying to navigate RFHs
> that
> > aren't the current RFH, for example.
> 
> Done.
> 
>
https://codereview.chromium.org/248963007/diff/50001/content/test/test_frame_...
> File content/test/test_frame_navigation_observer.cc (right):
> 
>
https://codereview.chromium.org/248963007/diff/50001/content/test/test_frame_...
> content/test/test_frame_navigation_observer.cc:56: if (frame_id ==
> render_frame_host_->GetRoutingID()) {
> On 2014/04/24 20:37:50, Charlie Reis wrote:
> > nit: No braces needed.
> 
> Done.
> 
>
https://codereview.chromium.org/248963007/diff/50001/content/test/test_frame_...
> File content/test/test_frame_navigation_observer.h (right):
> 
>
https://codereview.chromium.org/248963007/diff/50001/content/test/test_frame_...
> content/test/test_frame_navigation_observer.h:29: // |web_contents|.
> On 2014/04/24 20:37:50, Charlie Reis wrote:
> > nit: There's no |web_contents| parameter.
> 
> Done.
> 
>
https://codereview.chromium.org/248963007/diff/50001/content/test/test_frame_...
> content/test/test_frame_navigation_observer.h:32: // Like above but waits for
> one navigation.
> On 2014/04/24 20:37:50, Charlie Reis wrote:
> > nit: Like -> As
> 
> Done.
> 
>
https://codereview.chromium.org/248963007/diff/50001/content/test/test_frame_...
> content/test/test_frame_navigation_observer.h:51: 
> On 2014/04/24 20:37:50, Charlie Reis wrote:
> > nit: No blank line, since this is the block of WebContentsObserver
overrides.
> 
> Done.

[dcheng, 2014-04-24 23:04:11]

PostTaskAndReplyWithResult usage lgtm

[Charlie Reis, 2014-04-24 23:17:49]

Thanks!  Looks like there's a merge conflict, but otherwise LGTM with the nits
below.

https://codereview.chromium.org/248963007/diff/70001/content/browser/frame_ho...
File content/browser/frame_host/navigator.h (right):

https://codereview.chromium.org/248963007/diff/70001/content/browser/frame_ho...
content/browser/frame_host/navigator.h:38: virtual NavigationController*
GetController();
We should probably implement this in InterstitialPageNavigatorImpl as well, to
head off bugs for future callers of this.

https://codereview.chromium.org/248963007/diff/70001/content/browser/loader/c...
File content/browser/loader/cross_site_resource_handler.h (right):

https://codereview.chromium.org/248963007/diff/70001/content/browser/loader/c...
content/browser/loader/cross_site_resource_handler.h:64: // or not.
nit: Add "Currently only used in --site-per-process."

[nasko, 2014-04-24 23:29:34]

Fixed nits and merge conflict.

https://codereview.chromium.org/248963007/diff/70001/content/browser/frame_ho...
File content/browser/frame_host/navigator.h (right):

https://codereview.chromium.org/248963007/diff/70001/content/browser/frame_ho...
content/browser/frame_host/navigator.h:38: virtual NavigationController*
GetController();
On 2014/04/24 23:17:50, Charlie Reis wrote:
> We should probably implement this in InterstitialPageNavigatorImpl as well, to
> head off bugs for future callers of this.

Done.

https://codereview.chromium.org/248963007/diff/70001/content/browser/loader/c...
File content/browser/loader/cross_site_resource_handler.h (right):

https://codereview.chromium.org/248963007/diff/70001/content/browser/loader/c...
content/browser/loader/cross_site_resource_handler.h:64: // or not.
On 2014/04/24 23:17:50, Charlie Reis wrote:
> nit: Add "Currently only used in --site-per-process."

Done.

[nasko, 2014-04-24 23:29:39]

The CQ bit was checked by nasko@chromium.org

[commit-bot: I haz the power, 2014-04-24 23:31:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nasko@chromium.org/248963007/110001

[commit-bot: I haz the power, 2014-04-25 00:36:57]

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

[commit-bot: I haz the power, 2014-04-25 00:36:58]

Retried try job too often on android_dbg for step(s) slave_steps
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...

[Charlie Reis, 2014-04-25 00:54:26]

The CQ bit was checked by creis@chromium.org

[commit-bot: I haz the power, 2014-04-25 00:57:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nasko@chromium.org/248963007/110001

[commit-bot: I haz the power, 2014-04-25 02:51:46]

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

[commit-bot: I haz the power, 2014-04-25 02:51:47]

Retried try job too often on android_dbg for step(s) slave_steps
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...

[nasko, 2014-04-25 03:51:37]

Looking at this change, it is probably good to keep NavigateFrameToURL within
content, as it accepts FrameTreeNode as input, which isn't in the public
content/ API. Unfortunately, I don't see a suitable file to put this in. Should
I create a new one or am I missing some obvious location?

[nasko, 2014-04-25 17:06:08]

Charlie, 
Can you take another look at the CL? I've moved the test method to a content/
internal file.
Thanks!

[Charlie Reis, 2014-04-25 17:16:12]

LGTM with nits, assuming John's happy with the new internal test utils class.

https://codereview.chromium.org/248963007/diff/140001/content/browser/frame_h...
File content/browser/frame_host/interstitial_page_navigator_impl.cc (right):

https://codereview.chromium.org/248963007/diff/140001/content/browser/frame_h...
content/browser/frame_host/interstitial_page_navigator_impl.cc:16:
NavigationControllerImpl* navigation_controller)
That's weird.  Why did we have this parameter before if we didn't use it?

https://codereview.chromium.org/248963007/diff/140001/content/browser/frame_h...
File content/browser/frame_host/interstitial_page_navigator_impl.h (right):

https://codereview.chromium.org/248963007/diff/140001/content/browser/frame_h...
content/browser/frame_host/interstitial_page_navigator_impl.h:38: // The
NavigationController associated wit this navigator.
nit: with

https://codereview.chromium.org/248963007/diff/140001/content/browser/site_pe...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/248963007/diff/140001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:265: 
nit: No blank line here.

[nasko, 2014-04-25 17:20:13]

Fixed nits. 
I discussed the move of the code with jam@ beforehand and he suggested using
this name.
Thanks!

https://codereview.chromium.org/248963007/diff/140001/content/browser/frame_h...
File content/browser/frame_host/interstitial_page_navigator_impl.cc (right):

https://codereview.chromium.org/248963007/diff/140001/content/browser/frame_h...
content/browser/frame_host/interstitial_page_navigator_impl.cc:16:
NavigationControllerImpl* navigation_controller)
On 2014/04/25 17:16:13, Charlie Reis wrote:
> That's weird.  Why did we have this parameter before if we didn't use it?

Good question. I think for consistency with NavigatorImpl, but I don't recall
anything specific.

https://codereview.chromium.org/248963007/diff/140001/content/browser/frame_h...
File content/browser/frame_host/interstitial_page_navigator_impl.h (right):

https://codereview.chromium.org/248963007/diff/140001/content/browser/frame_h...
content/browser/frame_host/interstitial_page_navigator_impl.h:38: // The
NavigationController associated wit this navigator.
On 2014/04/25 17:16:13, Charlie Reis wrote:
> nit: with

Done.

https://codereview.chromium.org/248963007/diff/140001/content/browser/site_pe...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/248963007/diff/140001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:265: 
On 2014/04/25 17:16:13, Charlie Reis wrote:
> nit: No blank line here.

Done.

[nasko, 2014-04-25 17:20:20]

The CQ bit was checked by nasko@chromium.org

[nasko, 2014-04-25 19:14:19]

The CQ bit was checked by nasko@chromium.org

[commit-bot: I haz the power, 2014-04-25 21:47:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nasko@chromium.org/248963007/180001

[commit-bot: I haz the power, 2014-04-25 23:01:08]

Change committed as 266267