Two things:

- remove views dependencies from browser by moving profile related dialog actions into BrowserWindow.
- simplify the include dependencies in TabStripModel (making it easier to bring up on mac) by implementing more of its high level functionality in the delegate.

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

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

3 files changed, 26 insertions, 73 deletions

diff --git a/chrome/browser/tabs/tab_strip_model.cc b/chrome/browser/tabs/tab_strip_model.cc
index 2599382..938bd74 100644
--- a/chrome/browser/tabs/tab_strip_model.cc
+++ b/chrome/browser/tabs/tab_strip_model.cc
@@ -2,25 +2,15 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include "chrome/browser/tabs/tab_strip_model.h"
+
 #include <algorithm>
 
-#include "base/gfx/point.h"
-#include "base/logging.h"
-#include "chrome/browser/browser.h"
-#include "chrome/browser/browser_about_handler.h"
-#include "chrome/browser/browser_process.h"
 #include "chrome/browser/metrics/user_metrics.h"
 #include "chrome/browser/profile.h"
-#include "chrome/browser/renderer_host/render_view_host.h"
-#include "chrome/browser/sessions/tab_restore_service.h"
 #include "chrome/browser/tab_contents/navigation_controller.h"
-#include "chrome/browser/tab_contents/navigation_entry.h"
-#include "chrome/browser/tab_contents/tab_contents_factory.h"
-#include "chrome/browser/tabs/tab_strip_model.h"
+#include "chrome/browser/tab_contents/tab_contents.h"
 #include "chrome/browser/tabs/tab_strip_model_order_controller.h"
-#include "chrome/common/notification_service.h"
-#include "chrome/common/pref_names.h"
-#include "chrome/common/pref_service.h"
 #include "chrome/common/stl_util-inl.h"
 
 ///////////////////////////////////////////////////////////////////////////////
@@ -235,23 +225,6 @@ bool TabStripModel::TabsAreLoading() const {
   return false;
 }
 
-bool TabStripModel::TabHasUnloadListener(int index) {
-  // TODO(beng): this should call through to the delegate, so we can mock it
-  //             in testing and then provide better test coverage for features
-  //             like "close other tabs".
-  WebContents* web_contents = GetContentsAt(index)->AsWebContents();
-  if (web_contents) {
-    // If the WebContents is not connected yet, then there's no unload
-    // handler we can fire even if the WebContents has an unload listener.
-    // One case where we hit this is in a tab that has an infinite loop 
-    // before load.
-    return web_contents->notify_disconnection() && 
-        !web_contents->showing_interstitial_page() &&
-        web_contents->render_view_host()->HasUnloadListener();
-  }
-  return false;
-}
-
 NavigationController* TabStripModel::GetOpenerOfTabContentsAt(int index) {
   DCHECK(ContainsIndex(index));
   return contents_data_.at(index)->opener;
@@ -528,17 +501,8 @@ bool TabStripModel::InternalCloseTabContentsAt(int index,
                                                bool create_historical_tab) {
   TabContents* detached_contents = GetContentsAt(index);
 
-  if (TabHasUnloadListener(index)) {
-    // If the page has unload listeners, then we tell the renderer to fire
-    // them. Once they have fired, we'll get a message back saying whether
-    // to proceed closing the page or not, which sends us back to this method
-    // with the HasUnloadListener bit cleared.
-    WebContents* web_contents = detached_contents->AsWebContents();
-    // If we hit this code path, the tab had better be a WebContents tab.
-    DCHECK(web_contents);
-    web_contents->render_view_host()->FirePageBeforeUnload();
+  if (delegate_->RunUnloadListenerBeforeClosing(detached_contents))
     return false;
-  }
 
   // TODO: Now that we know the tab has no unload/beforeunload listeners,
   // we should be able to do a fast shutdown of the RenderViewProcess.
@@ -547,14 +511,12 @@ bool TabStripModel::InternalCloseTabContentsAt(int index,
   FOR_EACH_OBSERVER(TabStripModelObserver, observers_,
       TabClosingAt(detached_contents, index));
 
-  const bool add_to_restore_service =
-      (detached_contents && create_historical_tab &&
-       ShouldAddToTabRestoreService(detached_contents));
   if (detached_contents) {
-    if (add_to_restore_service) {
-      profile()->GetTabRestoreService()->
-          CreateHistoricalTab(detached_contents->controller());
-    }
+    // Ask the delegate to save an entry for this tab in the historical tab
+    // database if applicable.
+    if (create_historical_tab)
+      delegate_->CreateHistoricalTab(detached_contents);
+
     detached_contents->CloseContents();
     // Closing the TabContents will later call back to us via
     // NotificationObserver and detach it.
@@ -589,19 +551,6 @@ void TabStripModel::SetOpenerForContents(TabContents* contents,
   contents_data_.at(index)->opener = opener->controller();
 }
 
-bool TabStripModel::ShouldAddToTabRestoreService(TabContents* contents) {
-  if (!profile() || profile()->IsOffTheRecord() ||
-      !profile()->GetTabRestoreService()) {
-    return false;
-  }
-
-  Browser* browser =
-      Browser::GetBrowserForController(contents->controller(), NULL);
-  if (!browser)
-    return false; // Browser is null during unit tests.
-  return browser->type() == Browser::TYPE_NORMAL;
-}
-
 // static
 bool TabStripModel::OpenerMatches(TabContentsData* data,
                                   NavigationController* opener,
diff --git a/chrome/browser/tabs/tab_strip_model.h b/chrome/browser/tabs/tab_strip_model.h
index 0e5ceea..a9a6578 100644
--- a/chrome/browser/tabs/tab_strip_model.h
+++ b/chrome/browser/tabs/tab_strip_model.h
@@ -7,21 +7,19 @@
 
 #include <vector>
 
-#include "base/basictypes.h"
 #include "base/observer_list.h"
-#include "chrome/browser/history/history.h"
-#include "chrome/browser/tab_contents/site_instance.h"
 #include "chrome/common/notification_service.h"
 #include "chrome/common/page_transition_types.h"
-#include "chrome/common/pref_member.h"
 
 namespace gfx {
 class Point;
+class Rect;
 }
 class DockInfo;
 class GURL;
 class NavigationController;
 class Profile;
+class SiteInstance;
 class TabContents;
 class TabStripModelOrderController;
 class TabStripModel;
@@ -132,6 +130,17 @@ class TabStripModelDelegate {
   // Called when a drag session has completed and the frame that initiated the
   // the session should be closed.
   virtual void CloseFrameAfterDragSession() = 0;
+
+  // Creates an entry in the historical tab database for the specified
+  // TabContents.
+  virtual void CreateHistoricalTab(TabContents* contents) = 0;
+
+  // Runs any unload listeners associated with the specified TabContents before
+  // it is closed. If there are unload listeners that need to be run, this
+  // function returns true and the TabStripModel will wait before closing the
+  // TabContents. If it returns false, there are no unload listeners and the
+  // TabStripModel can close the TabContents immediately.
+  virtual bool RunUnloadListenerBeforeClosing(TabContents* contents) = 0;
 };
 
 ////////////////////////////////////////////////////////////////////////////////
@@ -286,10 +295,6 @@ class TabStripModel : public NotificationObserver {
   // Returns true if there are any TabContents that are currently loading.
   bool TabsAreLoading() const;
 
-  // Whether the tab has a beforeunload/unload listener that needs firing before
-  // being closed.
-  bool TabHasUnloadListener(int index);
-
   // Returns the controller controller that opened the TabContents at |index|.
   NavigationController* GetOpenerOfTabContentsAt(int index);
 
@@ -423,11 +428,6 @@ class TabStripModel : public NotificationObserver {
   // be |opener|'s NavigationController.
   void SetOpenerForContents(TabContents* contents, TabContents* opener);
 
-  // Returns true if closing the tab should add it to TabRestoreService. This
-  // returns true only if the profile has a TabRestoreService and the browser
-  // type is TABBED_BROWSER.
-  bool ShouldAddToTabRestoreService(TabContents* contents);
-
   // Returns true if the tab represented by the specified data has an opener
   // that matches the specified one. If |use_group| is true, then this will
   // fall back to check the group relationship as well.
diff --git a/chrome/browser/tabs/tab_strip_model_unittest.cc b/chrome/browser/tabs/tab_strip_model_unittest.cc
index 21ea248..84b3ceb 100644
--- a/chrome/browser/tabs/tab_strip_model_unittest.cc
+++ b/chrome/browser/tabs/tab_strip_model_unittest.cc
@@ -1012,6 +1012,10 @@ class TabStripDummyDelegate : public TabStripModelDelegate {
   virtual bool CanDuplicateContentsAt(int index) { return false; }
   virtual void DuplicateContentsAt(int index) {}
   virtual void CloseFrameAfterDragSession() {}
+  virtual void CreateHistoricalTab(TabContents* contents) {}
+  virtual bool RunUnloadListenerBeforeClosing(TabContents* contents) {
+    return false;
+  }
 
  private:
   // A dummy TabContents we give to callers that expect us to actually build a