Clean up ChromeContentBrowserClient::ShouldSwapProcessesForNavigation.

chrome/browser/chrome_content_browser_client.cc

 // 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.
 
 #include "chrome/browser/chrome_content_browser_client.h"
 
 #include <set>
 #include <utility>
 #include <vector>
 
@@ skipping 1304 matching lines @@
   BrowserThread::PostTask(
       BrowserThread::IO,
       FROM_HERE,
       base::Bind(&InfoMap::UnregisterExtensionProcess,
                  extensions::ExtensionSystem::Get(profile)->info_map(),
                  extension->id(),
                  site_instance->GetProcess()->GetID(),
                  site_instance->GetId()));
 }
 
 bool ChromeContentBrowserClient::ShouldSwapProcessesForNavigation(

[Matt Perry, 2013/12/02 21:45:20]

So this method really decides whether to swap BrowsingInstances? Would it
instead be simpler to have it swap SiteInstances, so postMessage would work
without the extra logic?

[Charlie Reis, 2013/12/02 21:59:03]

I think we do want this method to swap BrowsingInstances for extensions, just
not for hosted apps.

Also, I've actually already renamed it to
ShouldSwapBrowsingInstancesForNavigation.  :)  I just rebased so you can see the
change.  In the past, this method was more about forcing a process swap even
when in a mode that cross-site navigations don't cause process swaps (e.g.,
process-per-tab).  That happened to translate as a BrowsingInstance swap, and
now that's explicit.

(Note that the existing navigation logic in RenderViewHostManager would already
cause a SiteInstance swap for extensions since they look like different sites,
but the intent was to put them in their own BrowsingInstances.)

     SiteInstance* site_instance,
     const GURL& current_url,
     const GURL& new_url) {
-  if (current_url.is_empty()) {
-    // Always choose a new process when navigating to extension URLs. The
-    // process grouping logic will combine all of a given extension's pages
-    // into the same process.
-    if (new_url.SchemeIs(extensions::kExtensionScheme))
-      return true;
-
-    return false;
-  }
-
-  // Also, we must switch if one is an extension and the other is not the exact
-  // same extension.
-  if (current_url.SchemeIs(extensions::kExtensionScheme) ||
-      new_url.SchemeIs(extensions::kExtensionScheme)) {
-    if (current_url.GetOrigin() != new_url.GetOrigin())
-      return true;
-  }
-
-  // The checks below only matter if we can retrieve which extensions are
-  // installed.
+  // If we don't have an ExtensionService, then rely on the SiteInstance logic
+  // in RenderViewHostManager to decide when to swap.
   Profile* profile =
       Profile::FromBrowserContext(site_instance->GetBrowserContext());
   ExtensionService* service =
       extensions::ExtensionSystem::Get(profile)->extension_service();
   if (!service)
     return false;
 
-  // We must swap if the URL is for an extension and we are not using an
-  // extension process.
+  // We must use a new BrowsingInstance (forcing a process swap and disabling
+  // scripting by existing tabs) if one of the URLs is an extension and the
+  // other is not the exact same extension.
+  //
+  // We ignore hosted apps here so that other tabs in their BrowsingInstance can
+  // use postMessage with them.  (The exception is the Chrome Web Store, which
+  // is a hosted app that requires its own BrowsingInstance.)  Navigations
+  // to/from a hosted app will still trigger a SiteInstance swap in
+  // RenderViewHostManager.
+  const Extension* current_extension =
+      service->extensions()->GetExtensionOrAppByURL(current_url);
+  if (current_extension &&
+      current_extension->is_hosted_app() &&
+      current_extension->id() != extension_misc::kWebStoreAppId)
+    current_extension = NULL;
+
   const Extension* new_extension =
       service->extensions()->GetExtensionOrAppByURL(new_url);
-  // Ignore all hosted apps except the Chrome Web Store, since they do not
-  // require their own BrowsingInstance (e.g., postMessage is ok).
   if (new_extension &&
       new_extension->is_hosted_app() &&
       new_extension->id() != extension_misc::kWebStoreAppId)
     new_extension = NULL;
+
+  // First do a process check.  We should force a BrowsingInstance swap if the
+  // current process doesn't know about new_extension, even if current_extension
+  // is somehow the same as new_extension.
   if (new_extension &&
       site_instance->HasProcess() &&
       !service->process_map()->Contains(new_extension->id(),
                                         site_instance->GetProcess()->GetID()))
     return true;
 
-  return false;
+  // Otherwise, swap BrowsingInstances if current_extension and new_extension
+  // differ.
+  return current_extension != new_extension;
 }
 
 bool ChromeContentBrowserClient::ShouldSwapProcessesForRedirect(
     content::ResourceContext* resource_context, const GURL& current_url,
     const GURL& new_url) {
   ProfileIOData* io_data = ProfileIOData::FromResourceContext(resource_context);
   return extensions::CrossesExtensionProcessBoundary(
       io_data->GetExtensionInfoMap()->extensions(),
       current_url, new_url, false);
 }
@@ skipping 1247 matching lines @@
   return IsExtensionOrSharedModuleWhitelisted(url, extension_set,
                                               allowed_file_handle_origins_) ||
          IsHostAllowedByCommandLine(url, extension_set,
                                     switches::kAllowNaClFileHandleAPI);
 #else
   return false;
 #endif
 }
 
 }  // namespace chrome