Clean up RenderViewHostManager swapping logic.

content/public/browser/content_browser_client.h

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef CONTENT_PUBLIC_BROWSER_CONTENT_BROWSER_CLIENT_H_
 #define CONTENT_PUBLIC_BROWSER_CONTENT_BROWSER_CLIENT_H_
 #pragma once
 
 #include <string>
 #include <utility>
@@ skipping 127 matching lines @@
   virtual bool ShouldTryToUseExistingProcessHost(
       BrowserContext* browser_context, const GURL& url);
 
   // Called when a site instance is first associated with a process.
   virtual void SiteInstanceGotProcess(SiteInstance* site_instance) {}
 
   // Called from a site instance's destructor.
   virtual void SiteInstanceDeleting(SiteInstance* site_instance) {}
 
   // Returns true if for the navigation from |current_url| to |new_url|,
-  // processes should be swapped (even if we are in a process model that
-  // doesn't usually swap).
-  virtual bool ShouldSwapProcessesForNavigation(const GURL& current_url,
-                                                const GURL& new_url);
+  // a new SiteInstance and BrowsingInstance should be created (even if we are
+  // in a process model that doesn't usually swap).  This forces a process swap
+  // and severs script connections with existing tabs.
+  virtual bool ShouldSwapBrowsingInstancesForNavigation(

[darin (slow to review), 2012/05/31 04:43:08]

Why pluralize BrowsingInstance here?  Also, it looks like the BrowsingInstance
type is a private implementation detail of src/content/.  Should we really use
that name in the public API?

[Charlie Reis, 2012/05/31 17:58:23]

Seemed consistent with ShouldSwapProcessesForX.  I suppose the other way works
just as well.  Fixed.
I think so.  It's a private class out of convenience (because no one happens to
need it), but it's an important concept.

In this case, it has real implications for the process model: whether or not the
new page has any script connections (even postMessage) to existing pages.  Plus,
BrowsingInstances are described on the process model page and the conference
paper about the process model.

That said, in other APIs we've said things like "GetRelatedSiteInstance" to
imply a SiteInstance in the same BrowsingInstance.  If you felt strongly about
this we could try to find a similar name here, but I think it's going to be more
useful to expose the concept.

+      BrowserContext* browser_context,
+      const GURL& current_url,
+      const GURL& new_url);
 
   // Returns true if the given navigation redirect should cause a renderer
   // process swap.
   // This is called on the IO thread.
   virtual bool ShouldSwapProcessesForRedirect(ResourceContext* resource_context,

[darin (slow to review), 2012/05/31 04:43:08]

nit: Should this function be similarly renamed?

[Charlie Reis, 2012/05/31 17:58:23]

I thought about that but decided (naming similarity aside) that this function
was used in a different way.  While the one above is used to determine what kind
of process swap is needed, this one appears to check whether a redirect should
be given to RenderViewHostManager for a navigation decision.  In that sense,
it's more like RenderViewImpl's decidePolicyForNavigation.

I do think this would benefit from a better name (or even being killed in favor
of existing policy functions), but that may be a task for a different CL.

                                               const GURL& current_url,
                                               const GURL& new_url);
 
   // See CharacterEncoding's comment.
   virtual std::string GetCanonicalEncodingNameByAliasName(
       const std::string& alias_name);
 
   // Allows the embedder to pass extra command line flags.
   // switches::kProcessType will already be set at this point.
   virtual void AppendExtraCommandLineSwitches(CommandLine* command_line,
@@ skipping 250 matching lines @@
           const GURL& url);
 #endif
 
  protected:
   virtual ~ContentBrowserClient() {}
 };
 
 }  // namespace content
 
 #endif  // CONTENT_PUBLIC_BROWSER_CONTENT_BROWSER_CLIENT_H_