Remove dependency on NavigationEntry to get a SiteInstance

Remove dependency on NavigationEntry to get a SiteInstance

Remove the dependency on a NavigationEntry in
RenderFrameHostManager::ShouldSwapBrowsingInstancesForNavigation and
RenderFrameHostManager::GetSiteInstanceForEntry. This is needed by the
PlzNavigate project, as the SiteInstance to use is determined when the
navigation is about to commit. At this time, there is no convinient access to a
SNavigationEntry.

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

[clamy, 2014-08-13 16:06:16]

@ creis, nasko: PTAL
@ carlosk: FYI

Here is a CL that removes the dependency on a NavigationEntry in the functions
used to get a SiteInstance in RenderFrameHostManger. Note that it conflicts with
https://chromiumcodereview.appspot.com/379143002/.

[Charlie Reis, 2014-08-14 06:28:09]

Cool.  It's a bit more awkward to call these now, but I'm a big fan of not
having them depend on NavEntry.  I think it's a step in the right direction.

It'll let us avoid storing a NavEntry in the other CL, correct?

https://chromiumcodereview.appspot.com/471603002/diff/1/content/browser/frame...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://chromiumcodereview.appspot.com/471603002/diff/1/content/browser/frame...
content/browser/frame_host/render_frame_host_manager.cc:1342: if
(!is_guest_scheme && (ShouldTransitionCrossSite() || force_swap))
This needs braces if the body is more than one line.

https://chromiumcodereview.appspot.com/471603002/diff/1/content/browser/frame...
File content/browser/frame_host/render_frame_host_manager.h (right):

https://chromiumcodereview.appspot.com/471603002/diff/1/content/browser/frame...
content/browser/frame_host/render_frame_host_manager.h:397: const
NavigationEntry* current_entry,
Looks like we can eliminate |current_entry| from this method as well, which
would make it a more consistent set of parameters (even if we can't yet remove
it from UpdateStateForNavigate).

https://chromiumcodereview.appspot.com/471603002/diff/1/content/browser/frame...
content/browser/frame_host/render_frame_host_manager.h:399: const GURL& new_url,
It's very important that this is the result of GetEffectiveURL, and not just the
entry's URL (for hosted apps).  Let's call it |new_effective_url| and document
the requirement.

https://chromiumcodereview.appspot.com/471603002/diff/1/content/browser/frame...
content/browser/frame_host/render_frame_host_manager.h:414: SiteInstance*
navigation_instance,
We should be clearer about which parameters are for the destination, by grouping
them together and giving them a common prefix:

dest_url, dest_site_instance, dest_transition, dest_is_restore,
dest_is_view_source_mode, current_instance, force_browsing_instance_swap

https://chromiumcodereview.appspot.com/471603002/diff/1/content/browser/frame...
content/browser/frame_host/render_frame_host_manager.h:417:
NavigationEntryImpl::RestoreType restore_type,
We can make this a bool |dest_is_restore| to avoid having to include
navigation_entry_impl.h in this header file.

[clamy, 2014-08-14 10:03:00]

Thanks! Yes, we should not have to store the NavigationEntry in the other CL. I
will rebase it on top of this one to see what it looks like.

https://chromiumcodereview.appspot.com/471603002/diff/1/content/browser/frame...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://chromiumcodereview.appspot.com/471603002/diff/1/content/browser/frame...
content/browser/frame_host/render_frame_host_manager.cc:1342: if
(!is_guest_scheme && (ShouldTransitionCrossSite() || force_swap))
On 2014/08/14 06:28:08, Charlie Reis wrote:
> This needs braces if the body is more than one line.

Done.

https://chromiumcodereview.appspot.com/471603002/diff/1/content/browser/frame...
File content/browser/frame_host/render_frame_host_manager.h (right):

https://chromiumcodereview.appspot.com/471603002/diff/1/content/browser/frame...
content/browser/frame_host/render_frame_host_manager.h:397: const
NavigationEntry* current_entry,
On 2014/08/14 06:28:09, Charlie Reis wrote:
> Looks like we can eliminate |current_entry| from this method as well, which
> would make it a more consistent set of parameters (even if we can't yet remove
> it from UpdateStateForNavigate). 

Done.

https://chromiumcodereview.appspot.com/471603002/diff/1/content/browser/frame...
content/browser/frame_host/render_frame_host_manager.h:399: const GURL& new_url,
On 2014/08/14 06:28:09, Charlie Reis wrote:
> It's very important that this is the result of GetEffectiveURL, and not just
the
> entry's URL (for hosted apps).  Let's call it |new_effective_url| and document
> the requirement.

Done.

https://chromiumcodereview.appspot.com/471603002/diff/1/content/browser/frame...
content/browser/frame_host/render_frame_host_manager.h:414: SiteInstance*
navigation_instance,
On 2014/08/14 06:28:09, Charlie Reis wrote:
> We should be clearer about which parameters are for the destination, by
grouping
> them together and giving them a common prefix:
> 
> dest_url, dest_site_instance, dest_transition, dest_is_restore,
> dest_is_view_source_mode, current_instance, force_browsing_instance_swap

Done.

https://chromiumcodereview.appspot.com/471603002/diff/1/content/browser/frame...
content/browser/frame_host/render_frame_host_manager.h:417:
NavigationEntryImpl::RestoreType restore_type,
On 2014/08/14 06:28:09, Charlie Reis wrote:
> We can make this a bool |dest_is_restore| to avoid having to include
> navigation_entry_impl.h in this header file.

Done.

[Charlie Reis, 2014-08-14 14:55:48]

Great.  LGTM.

https://chromiumcodereview.appspot.com/471603002/diff/20001/content/browser/f...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://chromiumcodereview.appspot.com/471603002/diff/20001/content/browser/f...
content/browser/frame_host/render_frame_host_manager.cc:689: // Check with the
content client as well.  Important to pass current_url here,
nit: current_effective_url

[clamy, 2014-08-14 16:26:43]

Thanks!

https://chromiumcodereview.appspot.com/471603002/diff/20001/content/browser/f...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://chromiumcodereview.appspot.com/471603002/diff/20001/content/browser/f...
content/browser/frame_host/render_frame_host_manager.cc:689: // Check with the
content client as well.  Important to pass current_url here,
On 2014/08/14 14:55:48, Charlie Reis wrote:
> nit: current_effective_url

Done.

[clamy, 2014-08-14 16:26:47]

The CQ bit was checked by clamy@chromium.org

[commit-bot: I haz the power, 2014-08-14 16:28:05]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/clamy@chromium.org/471603002/40001

[commit-bot: I haz the power, 2014-08-14 19:55:38]

Committed patchset #3 (40001) as 289681