Now closes existing background contents if app opens a new one.

chrome/browser/tab_contents/render_view_host_delegate_helper.cc

 // Copyright (c) 2011 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/tab_contents/render_view_host_delegate_helper.h"
 
 #include <string>
 
 #include "base/command_line.h"
 #include "base/string_util.h"
@@ skipping 89 matching lines @@
   const Extension* extension =
       extensions_service->GetExtensionByWebExtent(opener_url);
   if (!extension)
     return NULL;
 
   // If the extension manifest specifies a background page, then don't allow one
   // to be created here.
   if (extension->background_url().is_valid())
     return NULL;
 
-  // Only allow a single background contents per app.
+  // No BackgroundContents allowed if BackgroundContentsService doesn't exist.
   BackgroundContentsService* service =
       BackgroundContentsServiceFactory::GetForProfile(profile);
-  if (!service || service->GetAppBackgroundContents(
-          ASCIIToUTF16(extension->id())))
+  if (!service)
     return NULL;
 
   // Ensure that we're trying to open this from the extension's process.
   ExtensionProcessManager* process_manager =
       profile->GetExtensionProcessManager();
   if (!site->GetProcess() || !process_manager ||
       site->GetProcess() != process_manager->GetExtensionProcess(opener_url))
     return NULL;
 
+  // Only allow a single background contents per app. If one already exists,
+  // close it.
+  BackgroundContents* existing =
+      service->GetAppBackgroundContents(ASCIIToUTF16(extension->id()));
+  if (existing) {
+    DLOG(WARNING) << "Closing existing BackgroundContents for " << opener_url;

[brettw, 2011/10/04 22:03:47]

I don't quite get this code. Is this an illegal condition?

I'm also surprised that the right way to close a background contents is to reach
inside its RVH and tell it to close before deleting it. Is that step necessary.
Can it be made less fragile?

[Andrew T Wilson (Slow), 2011/10/05 19:45:58]

The warning is because "reopening an existing BackgroundContents" is a fairly
exceptional condition (which is why it took us several months before we figured
out a repro case).
Good point. I've removed the call to ClosePage() and verified that deleting the
BackgroundContents by itself properly closes the page.

[brettw, 2011/10/05 20:03:34]

Let's think about this in terms of somebody random that sees this message in
their output. What should they do? It's a warning so it must be bad. Should they
fix a bug, report this condition to someone? Unless normal people need to do
something, I'd suggest no warning. If it could be useful to people, it should be
rephrased or made into an INFO.

[Andrew T Wilson (Slow), 2011/10/05 20:49:13]

Done.

+    existing->render_view_host()->ClosePage();
+    delete existing;
+  }
+
   // Passed all the checks, so this should be created as a BackgroundContents.
   return service->CreateBackgroundContents(site, route_id, profile, frame_name,
                                            ASCIIToUTF16(extension->id()));
 }
 
 TabContents* RenderViewHostDelegateViewHelper::CreateNewWindow(
     int route_id,
     Profile* profile,
     SiteInstance* site,
     WebUI::TypeID webui_type,
@@ skipping 400 matching lines @@
   DictionaryValue* inspector_settings = update.Get();
   inspector_settings->SetWithoutPathExpansion(key,
                                               Value::CreateStringValue(value));
 }
 
 void RenderViewHostDelegateHelper::ClearInspectorSettings(
     content::BrowserContext* browser_context) {
   Profile::FromBrowserContext(browser_context)->GetPrefs()->
       ClearPref(prefs::kWebKitInspectorSettings);
 }