A collection of fixes allowing the chrome.experimental.popup.* set of APIs to function in circumstances where there is no Browser instance present. This is a symptom of a tab-contents view hosted in an ExternalTabContainer.The major change here is the removal of the explicit dependency on a Browser instance across all of the delegates involved when showing a pop-up API. I modified the following delegates:- ExtensionPopupHost::Delegate- TabContentsDelegate- ExtensionFunctionDispatcher::DelegateBecause the pop-up requires a Profile, and a gfx::NativeWindow, I added methods to the above interfaces to provide them.BUG=noneTEST=ExtensionApiTest.FLAKY_Popup

Review URL: http://codereview.chromium.org/434046

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@34219 0039d316-1c4b-4281-b951-d872f2087c98

11 files changed, 121 insertions, 23 deletions

diff --git a/chrome/browser/extensions/extension_dom_ui.cc b/chrome/browser/extensions/extension_dom_ui.cc
index fb95005..e9f1996 100644
--- a/chrome/browser/extensions/extension_dom_ui.cc
+++ b/chrome/browser/extensions/extension_dom_ui.cc
@@ -63,8 +63,31 @@ void ExtensionDOMUI::ProcessDOMUIMessage(const std::string& message,
                                                 has_callback);
 }
 
-Browser* ExtensionDOMUI::GetBrowser() {
-  return static_cast<Browser*>(tab_contents()->delegate());
+Browser* ExtensionDOMUI::GetBrowser() const {
+  TabContentsDelegate* tab_contents_delegate = tab_contents()->delegate();
+  if (tab_contents_delegate)
+    return tab_contents_delegate->GetBrowser();
+  return NULL;
+}
+
+Profile* ExtensionDOMUI::GetProfile() {
+  return DOMUI::GetProfile();
+}
+
+gfx::NativeWindow ExtensionDOMUI::GetFrameNativeWindow() {
+  gfx::NativeWindow native_window =
+      ExtensionFunctionDispatcher::Delegate::GetFrameNativeWindow();
+
+  // If there was no window associated with the function dispatcher delegate,
+  // then this DOMUI may be hosted in an ExternalTabContainer, and a framing
+  // window will be accessible through the tab_contents.
+  if (!native_window) {
+    TabContentsDelegate* tab_contents_delegate = tab_contents()->delegate();
+    if (tab_contents_delegate)
+      native_window = tab_contents_delegate->GetFrameNativeWindow();
+  }
+
+  return native_window;
 }
 
 ////////////////////////////////////////////////////////////////////////////////
diff --git a/chrome/browser/extensions/extension_dom_ui.h b/chrome/browser/extensions/extension_dom_ui.h
index 0a53607..bd7a0ac 100644
--- a/chrome/browser/extensions/extension_dom_ui.h
+++ b/chrome/browser/extensions/extension_dom_ui.h
@@ -40,11 +40,13 @@ class ExtensionDOMUI
                                    bool has_callback);
 
   // ExtensionFunctionDispatcher::Delegate
-  virtual Browser* GetBrowser();
+  virtual Browser* GetBrowser() const;
   virtual ExtensionDOMUI* GetExtensionDOMUI() { return this; }
+  virtual gfx::NativeWindow GetFrameNativeWindow();
 
   // ExtensionPopupHost::Delegate
   virtual RenderViewHost* GetRenderViewHost();
+  virtual Profile* GetProfile();
 
   // BrowserURLHandler
   static bool HandleChromeURLOverride(GURL* url, Profile* profile);
diff --git a/chrome/browser/extensions/extension_function_dispatcher.cc b/chrome/browser/extensions/extension_function_dispatcher.cc
index 071c6f1..c1cb333 100644
--- a/chrome/browser/extensions/extension_function_dispatcher.cc
+++ b/chrome/browser/extensions/extension_function_dispatcher.cc
@@ -7,6 +7,8 @@
 #include "base/process_util.h"
 #include "base/singleton.h"
 #include "base/values.h"
+#include "chrome/browser/browser.h"
+#include "chrome/browser/browser_window.h"
 #include "chrome/browser/extensions/execute_code_in_tab_function.h"
 #include "chrome/browser/extensions/extension_bookmarks_module.h"
 #include "chrome/browser/extensions/extension_bookmarks_module_constants.h"
@@ -181,6 +183,20 @@ ExtensionFunction* FactoryRegistry::NewFunction(const std::string& name) {
 
 };  // namespace
 
+// ExtensionFunctionDispatcher::Delegate ---------------------------------------
+
+gfx::NativeWindow ExtensionFunctionDispatcher::Delegate::
+    GetFrameNativeWindow() {
+  Browser* browser = GetBrowser();
+  // If a browser is bound to this dispatcher, then return the widget hosting
+  // the window.  Extensions hosted in ExternalTabContainer objects may not
+  // have a running browser instance.
+  if (browser)
+    return browser->window()->GetNativeHandle();
+
+  return NULL;
+}
+
 // ExtensionFunctionDispatcher -------------------------------------------------
 
 void ExtensionFunctionDispatcher::GetAllFunctionNames(
@@ -318,3 +334,7 @@ void ExtensionFunctionDispatcher::HandleBadMessage(ExtensionFunction* api) {
 Profile* ExtensionFunctionDispatcher::profile() {
   return render_view_host_->process()->profile();
 }
+
+gfx::NativeWindow ExtensionFunctionDispatcher::GetFrameNativeWindow() {
+  return delegate_ ? delegate_->GetFrameNativeWindow() : NULL;
+}
diff --git a/chrome/browser/extensions/extension_function_dispatcher.h b/chrome/browser/extensions/extension_function_dispatcher.h
index df50ceb..b35c069 100644
--- a/chrome/browser/extensions/extension_function_dispatcher.h
+++ b/chrome/browser/extensions/extension_function_dispatcher.h
@@ -9,6 +9,7 @@
 #include <set>
 #include <vector>
 
+#include "app/gfx/native_widget_types.h"
 #include "base/ref_counted.h"
 #include "googleurl/src/gurl.h"
 
@@ -33,7 +34,12 @@ class ExtensionFunctionDispatcher {
  public:
   class Delegate {
    public:
-    virtual Browser* GetBrowser() = 0;
+    virtual Browser* GetBrowser() const = 0;
+
+    // Returns the gfx::NativeWindow that contains the view hosting the
+    // environment in which the function dispatcher resides.
+    virtual gfx::NativeWindow GetFrameNativeWindow();
+
     virtual ExtensionHost* GetExtensionHost() { return NULL; }
     virtual ExtensionDOMUI* GetExtensionDOMUI() { return NULL; }
   };
@@ -94,6 +100,12 @@ class ExtensionFunctionDispatcher {
   // non-tab-hosted extension pages, this will return NULL.
   ExtensionDOMUI* GetExtensionDOMUI();
 
+  // Returns the gfx::NativeWindow that frames the view of the extension
+  // containing the function dispatcher.  This may return NULL.  Refer to the
+  // ExtensionFunctionDispatcher::Delegate::GetFrameNativeWindow()
+  // implementation for an explanation.
+  gfx::NativeWindow GetFrameNativeWindow();
+
   // Gets the extension the function is being invoked by. This should not ever
   // return NULL.
   Extension* GetExtension();
diff --git a/chrome/browser/extensions/extension_host.cc b/chrome/browser/extensions/extension_host.cc
index 0ad3a9e..77bee6c 100644
--- a/chrome/browser/extensions/extension_host.cc
+++ b/chrome/browser/extensions/extension_host.cc
@@ -549,7 +549,7 @@ void ExtensionHost::HandleMouseLeave() {
 #endif
 }
 
-Browser* ExtensionHost::GetBrowser() {
+Browser* ExtensionHost::GetBrowser() const {
   if (view_.get())
     return view_->browser();
 
@@ -604,16 +604,19 @@ void ExtensionHost::RenderViewCreated(RenderViewHost* render_view_host) {
 }
 
 int ExtensionHost::GetBrowserWindowID() const {
+  // Hosts not attached to any browser window have an id of -1.  This includes
+  // those mentioned below, and background pages.
   int window_id = -1;
   if (extension_host_type_ == ViewType::EXTENSION_TOOLSTRIP ||
       extension_host_type_ == ViewType::EXTENSION_MOLE ||
       extension_host_type_ == ViewType::EXTENSION_POPUP) {
-    window_id = ExtensionTabUtil::GetWindowId(
-        const_cast<ExtensionHost* >(this)->GetBrowser());
-  } else if (extension_host_type_ == ViewType::EXTENSION_BACKGROUND_PAGE) {
-    // Background page is not attached to any browser window, so pass -1.
-    window_id = -1;
-  } else {
+    // If the host is bound to a browser, then extract its window id.
+    // Extensions hosted in ExternalTabContainer objects may not have
+    // an associated browser.
+    Browser* browser = GetBrowser();
+    if (browser)
+      window_id = ExtensionTabUtil::GetWindowId(browser);
+  } else if (extension_host_type_ != ViewType::EXTENSION_BACKGROUND_PAGE) {
     NOTREACHED();
   }
   return window_id;
diff --git a/chrome/browser/extensions/extension_host.h b/chrome/browser/extensions/extension_host.h
index 76f20d2..5a27030 100644
--- a/chrome/browser/extensions/extension_host.h
+++ b/chrome/browser/extensions/extension_host.h
@@ -65,7 +65,10 @@ class ExtensionHost : public ExtensionPopupHost::PopupDelegate,
   void* view() const { return NULL; }
 #endif
 
-  // Create an ExtensionView and tie it to this host and |browser|.
+  // Create an ExtensionView and tie it to this host and |browser|.  Note NULL
+  // is a valid argument for |browser|.  Extension views may be bound to
+  // tab-contents hosted in ExternalTabContainer objects, which do not
+  // instantiate Browser objects.
   void CreateView(Browser* browser);
 
   Extension* extension() { return extension_; }
@@ -187,7 +190,7 @@ class ExtensionHost : public ExtensionPopupHost::PopupDelegate,
   // If this ExtensionHost has a view, this returns the Browser that view is a
   // part of.  If this is a global background page, we use the active Browser
   // instead.
-  virtual Browser* GetBrowser();
+  virtual Browser* GetBrowser() const;
   virtual ExtensionHost* GetExtensionHost() { return this; }
 
   // ExtensionPopupHost::Delegate
diff --git a/chrome/browser/extensions/extension_popup_api.cc b/chrome/browser/extensions/extension_popup_api.cc
index 091fa71..f68553c 100644
--- a/chrome/browser/extensions/extension_popup_api.cc
+++ b/chrome/browser/extensions/extension_popup_api.cc
@@ -139,8 +139,12 @@ bool PopupShowFunction::RunImpl() {
   BubbleBorder::ArrowLocation arrow_location =
       (NULL != dispatcher()->GetExtensionHost()) ? BubbleBorder::BOTTOM_LEFT :
                                                    BubbleBorder::TOP_LEFT;
-  popup_ = ExtensionPopup::Show(url, dispatcher()->GetBrowser(), rect,
-                                arrow_location, give_focus);
+  popup_ = ExtensionPopup::Show(url, dispatcher()->GetBrowser(),
+                                dispatcher()->profile(),
+                                dispatcher()->GetFrameNativeWindow(),
+                                rect,
+                                arrow_location,
+                                give_focus);
 
   ExtensionPopupHost* popup_host = dispatcher()->GetPopupHost();
   DCHECK(popup_host);
diff --git a/chrome/browser/extensions/extension_popup_apitest.cc b/chrome/browser/extensions/extension_popup_apitest.cc
index 16c5374..6095d5d 100644
--- a/chrome/browser/extensions/extension_popup_apitest.cc
+++ b/chrome/browser/extensions/extension_popup_apitest.cc
@@ -12,5 +12,6 @@ IN_PROC_BROWSER_TEST_F(ExtensionApiTest, FLAKY_Popup) {
       switches::kEnableExperimentalExtensionApis);
   CommandLine::ForCurrentProcess()->AppendSwitch(
       switches::kEnableExtensionToolstrips);
+
   ASSERT_TRUE(RunExtensionTest("popup_api")) << message_;
 }
diff --git a/chrome/browser/extensions/extension_popup_host.cc b/chrome/browser/extensions/extension_popup_host.cc
index f263494..1a964da 100644
--- a/chrome/browser/extensions/extension_popup_host.cc
+++ b/chrome/browser/extensions/extension_popup_host.cc
@@ -18,6 +18,13 @@
 #include "chrome/common/notification_type.h"
 
 
+ExtensionPopupHost::PopupDelegate::~PopupDelegate() {
+  // If the PopupDelegate is being torn down, then make sure to reset the
+  // cached pointer in the host to prevent access to a stale pointer.
+  if (popup_host_.get())
+    popup_host_->RevokeDelegate();
+}
+
 ExtensionPopupHost* ExtensionPopupHost::PopupDelegate::popup_host() {
   if (!popup_host_.get())
     popup_host_.reset(new ExtensionPopupHost(this));
@@ -25,6 +32,18 @@ ExtensionPopupHost* ExtensionPopupHost::PopupDelegate::popup_host() {
   return popup_host_.get();
 }
 
+Profile* ExtensionPopupHost::PopupDelegate::GetProfile() {
+  // If there is a browser present, return the profile associated with it.
+  // When hosting a view in an ExternalTabContainer, it is possible to have
+  // no Browser instance.
+  Browser* browser = GetBrowser();
+  if (browser) {
+    return browser->profile();
+  }
+
+  return NULL;
+}
+
 ExtensionPopupHost::ExtensionPopupHost(PopupDelegate* delegate)
     :  // NO LINT
 #if defined(TOOLKIT_VIEWS)
@@ -35,8 +54,10 @@ ExtensionPopupHost::ExtensionPopupHost(PopupDelegate* delegate)
 
   // Listen for view close requests, so that we can dismiss a hosted pop-up
   // view, if necessary.
+  Profile* profile = delegate_->GetProfile();
+  DCHECK(profile);
   registrar_.Add(this, NotificationType::EXTENSION_HOST_VIEW_SHOULD_CLOSE,
-                 Source<Profile>(delegate_->GetBrowser()->profile()));
+                 Source<Profile>(profile));
 }
 
 ExtensionPopupHost::~ExtensionPopupHost() {
@@ -88,9 +109,11 @@ void ExtensionPopupHost::DismissPopup() {
     delete child_popup_;
     child_popup_ = NULL;
 
-    PopupEventRouter::OnPopupClosed(
-        delegate_->GetBrowser()->profile(),
-        delegate_->GetRenderViewHost()->routing_id());
+    if (delegate_) {
+      PopupEventRouter::OnPopupClosed(
+          delegate_->GetProfile(),
+          delegate_->GetRenderViewHost()->routing_id());
+    }
   }
 #endif  // defined(TOOLKIT_VIEWS)
 }
diff --git a/chrome/browser/extensions/extension_popup_host.h b/chrome/browser/extensions/extension_popup_host.h
index f8e97a0..102c4ad 100644
--- a/chrome/browser/extensions/extension_popup_host.h
+++ b/chrome/browser/extensions/extension_popup_host.h
@@ -36,12 +36,14 @@ class ExtensionPopupHost :  // NOLINT
   class PopupDelegate {
    public:
     PopupDelegate() {}
-    virtual ~PopupDelegate() {}
-    virtual Browser* GetBrowser() = 0;
+    virtual ~PopupDelegate();
+    virtual Browser* GetBrowser() const = 0;
     virtual RenderViewHost* GetRenderViewHost() = 0;
+    virtual Profile* GetProfile();
 
     // Constructs, or returns the existing ExtensionPopupHost instance.
     ExtensionPopupHost* popup_host();
+
    private:
     scoped_ptr<ExtensionPopupHost> popup_host_;
 
@@ -51,6 +53,8 @@ class ExtensionPopupHost :  // NOLINT
   explicit ExtensionPopupHost(PopupDelegate* delegate);
   virtual ~ExtensionPopupHost();
 
+  void RevokeDelegate() { delegate_ = NULL; }
+
   // Dismiss the hosted pop-up, if one is present.
   void DismissPopup();
 
diff --git a/chrome/browser/extensions/extension_process_manager.cc b/chrome/browser/extensions/extension_process_manager.cc
index 7d6ca0b..78cd214 100644
--- a/chrome/browser/extensions/extension_process_manager.cc
+++ b/chrome/browser/extensions/extension_process_manager.cc
@@ -52,6 +52,7 @@ ExtensionProcessManager::ExtensionProcessManager(Profile* profile)
 }
 
 ExtensionProcessManager::~ExtensionProcessManager() {
+  CloseBackgroundHosts();
   DCHECK(background_hosts_.empty());
 }
 
@@ -60,7 +61,8 @@ ExtensionHost* ExtensionProcessManager::CreateView(Extension* extension,
                                                    Browser* browser,
                                                    ViewType::Type view_type) {
   DCHECK(extension);
-  DCHECK(browser);
+  // A NULL browser may only be given for pop-up views.
+  DCHECK(browser || (!browser && view_type == ViewType::EXTENSION_POPUP));
   ExtensionHost* host =
 #if defined(OS_MACOSX)
       new ExtensionHostMac(extension, GetSiteInstanceForURL(url), url,
@@ -76,7 +78,8 @@ ExtensionHost* ExtensionProcessManager::CreateView(Extension* extension,
 ExtensionHost* ExtensionProcessManager::CreateView(const GURL& url,
                                                    Browser* browser,
                                                    ViewType::Type view_type) {
-  DCHECK(browser);
+  // A NULL browser may only be given for pop-up views.
+  DCHECK(browser || (!browser && view_type == ViewType::EXTENSION_POPUP));
   ExtensionsService* service =
     browsing_instance_->profile()->GetExtensionsService();
   if (service) {