Decouple TabRestoreService from Browser

chrome/browser/sessions/tab_restore_service_delegate.h

+// 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.
+
+#ifndef CHROME_BROWSER_SESSIONS_TAB_RESTORE_SERVICE_DELEGATE_H_
+#define CHROME_BROWSER_SESSIONS_TAB_RESTORE_SERVICE_DELEGATE_H_
+
+#include "chrome/browser/sessions/session_id.h"
+
+class BrowserWindow;
+class Profile;
+class SessionStorageNamespace;
+class TabContents;
+class TabNavigation;
+class TabRestoreService;
+
+// Objects implement this interface to provide necessary functionality for
+// TabRestoreService to operate.
+class TabRestoreServiceDelegate {
+ public:
+  virtual bool IsNormalBrowser() const = 0;

[sky, 2011/03/10 18:43:24]

Remove this and instead push the code into the call to BrowserClosing.

[torne_google.com, 2011/03/11 15:07:12]

Done.

+  virtual Profile* profile() const = 0;

[Ben Goodger (Google), 2011/03/10 18:28:35]

Google style: virtual methods should never use unix_hacker since they are not
cheap (i.e. not inline-able)

[sky, 2011/03/10 18:43:24]

This shouldn't be necessary as TabRestoreService (actually
SessionRestoreService) has a profile.

[torne_google.com, 2011/03/11 15:07:12]

Done.

+  virtual BrowserWindow* window() const = 0;

[sky, 2011/03/10 18:43:24]

If we're trying to minimize the surface area TabRestoreService sees, you should
add a concrete method for what is needed (Show) in this case.

[torne_google.com, 2011/03/11 15:07:12]

Done.

+  virtual const SessionID& session_id() const = 0;
+  virtual int tab_count() const = 0;
+  virtual int selected_index() const = 0;
+  virtual int GetIndexOfController(

[sky, 2011/03/10 18:43:24]

This shouldn't be needed. I believe the index can be passed in where it is
needed.

[torne_google.com, 2011/03/11 15:07:12]

Done.

+      const NavigationController* controller) const = 0;

[Ben Goodger (Google), 2011/03/10 18:28:35]

Code other than tab-restore related code calls this method I think. Because of
this, I don't think Browser should "source" it's having this method from this
interface, because potentially the methods could be overridden in Browser as
protected.

It's probably worth a deeper understanding of what TabRestoreService is actually
doing and changing a lot of these methods to be more TabRestoreService focused.

+  virtual TabContents* GetTabContentsAt(int index) const = 0;
+  virtual TabContents* GetSelectedTabContents() const = 0;

[Ben Goodger (Google), 2011/03/10 18:28:35]

Same comment as above with most of these methods.

[torne_google.com, 2011/03/11 15:07:12]

tab_count, GetTabContentsAt are used to iterate over all the tabs in a closing
browser.

GetSelectedTabContents is used to see if there's a current tab that needs
closing in the case for CloseTab I described below.

Not sure if there's anything to be done about those. I've eliminated
GetIndexOfController as it's more efficient to pass it in in the case where a
whole window is being iterated over and it's easy to pass it in manually for
single tabs.

+  virtual bool IsTabPinned(int index) const = 0;
+  virtual TabContents* AddRestoredTab(
+      const std::vector<TabNavigation>& navigations,
+      int tab_index,
+      int selected_navigation,
+      const std::string& extension_app_id,
+      bool select,
+      bool pin,
+      bool from_last_session,
+      SessionStorageNamespace* storage_namespace) = 0;

[Ben Goodger (Google), 2011/03/10 18:28:35]

This seems like a valid method to define on such an interface.

+  virtual void ReplaceRestoredTab(
+      const std::vector<TabNavigation>& navigations,
+      int selected_navigation,
+      bool from_last_session,
+      const std::string& extension_app_id,
+      SessionStorageNamespace* session_storage_namespace) = 0;

[Ben Goodger (Google), 2011/03/10 18:28:35]

So does this.

+  virtual void CloseTab() = 0;

[Ben Goodger (Google), 2011/03/10 18:28:35]

This... not as much.

[torne_google.com, 2011/03/11 15:07:12]

This is used in the case where the current tab is being replaced with an entire
window's worth of tabs: it adds all the new tabs then closes the original one
(unlike when replacing a single tab, where the tab's NavigationController just
gets its state overwritten). I think we have to keep this.

+
+  static TabRestoreServiceDelegate* Create(Profile* profile);
+  static TabRestoreServiceDelegate* GetBrowserForController(
+      const NavigationController* controller, int* index);
+  static TabRestoreServiceDelegate* FindBrowserWithID(
+      SessionID::id_type desired_id);
+
+ protected:
+  virtual ~TabRestoreServiceDelegate() {}
+};
+
+#endif  // CHROME_BROWSER_SESSIONS_TAB_RESTORE_SERVICE_DELEGATE_H_