Make background pages work more nicely with the tabs API.

Changed ExtensionHost so that it returns NULL if it doesn't have
a browser, instead of getting the last active one. The problem
is that if ExtensionHost returns the last active browser at the
time it's constructed, it might be garbage by the time it is
used.

Changed tab functions to use more consistent logic to determine
the 'current' browser to operate on.

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

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

6 files changed, 41 insertions, 35 deletions

diff --git a/chrome/browser/browser.h b/chrome/browser/browser.h
index 580cf37..a2702a7 100644
--- a/chrome/browser/browser.h
+++ b/chrome/browser/browser.h
@@ -13,6 +13,7 @@
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/browser_window.h"
 #include "chrome/browser/command_updater.h"
+#include "chrome/browser/extensions/extension_function_dispatcher.h"
 #include "chrome/browser/sessions/session_id.h"
 #include "chrome/browser/shell_dialogs.h"
 #include "chrome/browser/tab_contents/tab_contents.h"
@@ -43,7 +44,8 @@ class Browser : public TabStripModelDelegate,
                 public PageNavigator,
                 public CommandUpdater::CommandUpdaterDelegate,
                 public NotificationObserver,
-                public SelectFileDialog::Listener {
+                public SelectFileDialog::Listener,
+                public ExtensionFunctionDispatcher::Delegate {
  public:
   enum Type {
     TYPE_NORMAL = 0,
@@ -501,6 +503,11 @@ class Browser : public TabStripModelDelegate,
                        const NotificationSource& source,
                        const NotificationDetails& details);
 
+  // Overridden from ExtensionFunctionDispatcher::Delegate
+  virtual Browser* GetBrowser() {
+    return this;
+  }
+
   // Command and state updating ///////////////////////////////////////////////
 
   // Initialize state for all browser commands.
diff --git a/chrome/browser/extensions/extension_function_dispatcher.cc b/chrome/browser/extensions/extension_function_dispatcher.cc
index a8d4ac8..b303778 100644
--- a/chrome/browser/extensions/extension_function_dispatcher.cc
+++ b/chrome/browser/extensions/extension_function_dispatcher.cc
@@ -118,11 +118,12 @@ void ExtensionFunctionDispatcher::GetAllFunctionNames(
 
 ExtensionFunctionDispatcher::ExtensionFunctionDispatcher(
     RenderViewHost* render_view_host,
-    Browser* browser,
+    Delegate* delegate,
     const std::string& extension_id)
   : render_view_host_(render_view_host),
-    browser_(browser),
+    delegate_(delegate),
     extension_id_(extension_id) {
+  DCHECK(delegate);
   RenderProcessHost* process = render_view_host_->process();
   ExtensionMessageService* message_service = 
       ExtensionMessageService::GetInstance(profile()->GetRequestContext());
@@ -131,6 +132,12 @@ ExtensionFunctionDispatcher::ExtensionFunctionDispatcher(
   message_service->RegisterExtension(extension_id, process->pid());
 }
 
+Browser* ExtensionFunctionDispatcher::GetBrowser() {
+  Browser* retval = delegate_->GetBrowser();
+  DCHECK(retval);
+  return retval;
+}
+
 void ExtensionFunctionDispatcher::HandleRequest(const std::string& name,
                                                 const std::string& args,
                                                 int callback_id) {
diff --git a/chrome/browser/extensions/extension_function_dispatcher.h b/chrome/browser/extensions/extension_function_dispatcher.h
index 1ffb3d4..43d2a36 100644
--- a/chrome/browser/extensions/extension_function_dispatcher.h
+++ b/chrome/browser/extensions/extension_function_dispatcher.h
@@ -21,11 +21,16 @@ class RenderViewHostDelegate;
 // appropriate handler. It lives entirely on the UI thread.
 class ExtensionFunctionDispatcher {
  public:
+  class Delegate {
+   public:
+    virtual Browser* GetBrowser() = 0;
+  };
+
   // Gets a list of all known extension function names.
   static void GetAllFunctionNames(std::vector<std::string>* names);
 
   ExtensionFunctionDispatcher(RenderViewHost* render_view_host,
-                              Browser* browser,
+                              Delegate* delegate,
                               const std::string& extension_id);
 
   // Handle a request to execute an extension function.
@@ -35,7 +40,9 @@ class ExtensionFunctionDispatcher {
   // Send a response to a function.
   void SendResponse(ExtensionFunction* api);
 
-  Browser* browser() { return browser_; }
+  // Gets the browser extension functions should operate relative to. For
+  // example, for positioning windows, or alert boxes, or creating tabs.
+  Browser* GetBrowser();
 
   // Handle a malformed message.  Possibly the result of an attack, so kill
   // the renderer.
@@ -50,7 +57,7 @@ class ExtensionFunctionDispatcher {
  private:
   RenderViewHost* render_view_host_;
 
-  Browser* browser_;
+  Delegate* delegate_;
 
   std::string extension_id_;
 };
diff --git a/chrome/browser/extensions/extension_host.cc b/chrome/browser/extensions/extension_host.cc
index 8e3f723..5f91ba1 100755
--- a/chrome/browser/extensions/extension_host.cc
+++ b/chrome/browser/extensions/extension_host.cc
@@ -91,8 +91,7 @@ void ExtensionHost::DidStopLoading(RenderViewHost* render_view_host) {
 ExtensionFunctionDispatcher* ExtensionHost::
     CreateExtensionFunctionDispatcher(RenderViewHost *render_view_host,
                                       const std::string& extension_id) {
-  return new ExtensionFunctionDispatcher(render_view_host, GetBrowser(),
-                                         extension_id);
+  return new ExtensionFunctionDispatcher(render_view_host, this, extension_id);
 }
 
 RenderViewHostDelegate::View* ExtensionHost::GetViewDelegate() const {
@@ -159,7 +158,7 @@ void ExtensionHost::HandleKeyboardEvent(const NativeWebKeyboardEvent& event) {
 Browser* ExtensionHost::GetBrowser() {
   if (view_)
     return view_->browser();
-  Browser* browser = BrowserList::FindBrowserWithProfile(
+  Browser* browser = BrowserList::GetLastActiveWithProfile(
       render_view_host()->process()->profile());
   // TODO(mpcomplete): what this verifies doesn't actually happen yet.
   CHECK(browser) << "ExtensionHost running in Profile with no Browser active."
diff --git a/chrome/browser/extensions/extension_host.h b/chrome/browser/extensions/extension_host.h
index 0e97c91..8c5a8cd 100755
--- a/chrome/browser/extensions/extension_host.h
+++ b/chrome/browser/extensions/extension_host.h
@@ -21,7 +21,8 @@ struct WebPreferences;
 // privileges available to extensions.  It may have a view to be shown in the
 // in the browser UI, or it may be hidden.
 class ExtensionHost : public RenderViewHostDelegate,
-                      public RenderViewHostDelegate::View {
+                      public RenderViewHostDelegate::View,
+                      public ExtensionFunctionDispatcher::Delegate {
  public:
   ExtensionHost(Extension* extension, SiteInstance* site_instance);
   ~ExtensionHost();
@@ -74,10 +75,11 @@ class ExtensionHost : public RenderViewHostDelegate,
   virtual void HandleKeyboardEvent(const NativeWebKeyboardEvent& event);
 
  private:
+  // ExtensionFunctionDispatcher::Delegate
   // 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.
-  Browser* GetBrowser();
+  virtual Browser* GetBrowser();
 
   // The extension that we're hosting in this view.
   Extension* extension_;
diff --git a/chrome/browser/extensions/extension_tabs_module.cc b/chrome/browser/extensions/extension_tabs_module.cc
index 01fff3b..39066c3 100644
--- a/chrome/browser/extensions/extension_tabs_module.cc
+++ b/chrome/browser/extensions/extension_tabs_module.cc
@@ -145,7 +145,8 @@ bool GetWindowFunction::RunImpl() {
 }
 
 bool GetCurrentWindowFunction::RunImpl() {
-  return GetWindowFunctionHelper(dispatcher_->browser(), profile(), &result_);
+  return GetWindowFunctionHelper(dispatcher_->GetBrowser(), profile(),
+                                 &result_);
 }
 
 bool GetFocusedWindowFunction::RunImpl() {
@@ -190,20 +191,15 @@ bool CreateWindowFunction::RunImpl() {
     }
   }
 
-  // Try to get the browser associated with view that this call came from, so
-  // its position can be set relative to its browser window.
-  Browser* browser = dispatcher_->browser();
-  if (browser == NULL)
-    browser = BrowserList::GetLastActiveWithProfile(dispatcher_->profile());
-
   // Try to position the new browser relative its originating browser window.
   gfx::Rect empty_bounds;
   gfx::Rect bounds;
   bool maximized;
   // The call offsets the bounds by kWindowTilePixels (defined in WindowSizer to
   // be 10).
-  WindowSizer::GetBrowserWindowBounds(std::wstring(), empty_bounds, browser,
-      &bounds, &maximized);
+  WindowSizer::GetBrowserWindowBounds(std::wstring(), empty_bounds,
+                                      dispatcher_->GetBrowser(), &bounds,
+                                      &maximized);
 
   // Any part of the bounds can optionally be set by the caller.
   if (args_->IsType(Value::TYPE_DICTIONARY)) {
@@ -280,13 +276,9 @@ bool GetSelectedTabFunction::RunImpl() {
     EXTENSION_FUNCTION_VALIDATE(args_->GetAsInteger(&window_id));
     browser = GetBrowserInProfileWithId(profile(), window_id);
   } else {
-    browser = dispatcher_->browser();
+    browser = dispatcher_->GetBrowser();
   }
 
-  if (!browser)
-    // TODO(rafaelw): return a "no 'current' browser" error.
-    return false;
-
   TabStripModel* tab_strip = browser->tabstrip_model();
   result_.reset(ExtensionTabUtil::CreateTabValue(
       tab_strip->GetSelectedTabContents(),
@@ -303,13 +295,9 @@ bool GetAllTabsInWindowFunction::RunImpl() {
     EXTENSION_FUNCTION_VALIDATE(args_->GetAsInteger(&window_id));
     browser = GetBrowserInProfileWithId(profile(), window_id);
   } else {
-    browser = dispatcher_->browser();
+    browser = dispatcher_->GetBrowser();
   }
 
-  if (!browser)
-    // TODO(rafaelw): return a "no 'current' browser" error.
-    return false;
-
   result_.reset(CreateTabList(browser));
 
   return true;
@@ -326,13 +314,9 @@ bool CreateTabFunction::RunImpl() {
     EXTENSION_FUNCTION_VALIDATE(args->GetInteger(kWindowIdKey, &window_id));
     browser = GetBrowserInProfileWithId(profile(), window_id);
   } else {
-    browser = dispatcher_->browser();
+    browser = dispatcher_->GetBrowser();
   }
 
-  if (!browser)
-    // TODO(rafaelw): return a "no 'current' browser" error.
-    return false;
-
   TabStripModel* tab_strip = browser->tabstrip_model();
 
   // TODO(rafaelw): handle setting remaining tab properties: