Remove direct dependency from SessionModelAssociator on Browser/BrowserList.

This is acheived by having Browser implement SyncedBrowserDelegate

BUG=
TEST=


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

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

8 files changed, 236 insertions, 30 deletions

diff --git a/chrome/browser/sync/glue/session_model_associator.cc b/chrome/browser/sync/glue/session_model_associator.cc
index da4d86f..a923081 100644
--- a/chrome/browser/sync/glue/session_model_associator.cc
+++ b/chrome/browser/sync/glue/session_model_associator.cc
@@ -5,6 +5,7 @@
 #include "chrome/browser/sync/glue/session_model_associator.h"
 
 #include <algorithm>
+#include <set>
 #include <utility>
 
 #include "base/logging.h"
@@ -13,11 +14,9 @@
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/sessions/restore_tab_helper.h"
 #include "chrome/browser/sessions/session_service_factory.h"
+#include "chrome/browser/sync/glue/synced_window_delegate.h"
 #include "chrome/browser/sync/profile_sync_service.h"
 #include "chrome/browser/sync/syncable/syncable.h"
-#include "chrome/browser/tabs/tab_strip_model.h"
-#include "chrome/browser/ui/browser_list.h"
-#include "chrome/browser/ui/browser_window.h"
 #include "chrome/browser/ui/tab_contents/tab_contents_wrapper.h"
 #include "chrome/common/chrome_notification_types.h"
 #include "chrome/common/extensions/extension.h"
@@ -123,26 +122,28 @@ void SessionModelAssociator::ReassociateWindows(bool reload_tabs) {
   sync_pb::SessionHeader* header_s = specifics.mutable_header();
   SyncedSession* current_session =
       synced_session_tracker_.GetSession(local_tag);
-  current_session->windows.reserve(BrowserList::size());
 
   size_t window_num = 0;
-  for (BrowserList::const_iterator i = BrowserList::begin();
-       i != BrowserList::end(); ++i) {
-    // Make sure the browser has tabs and a window. Browsers destructor
-    // removes itself from the BrowserList. When a browser is closed the
+  std::set<SyncedWindowDelegate*> windows =
+      SyncedWindowDelegate::GetSyncedWindowDelegates();
+  current_session->windows.reserve(windows.size());
+  for (std::set<SyncedWindowDelegate*>::const_iterator i =
+       windows.begin(); i != windows.end(); ++i) {
+    // Make sure the window has tabs and a viewable window. The viewable window
+    // check is necessary because, for example, when a browser is closed the
     // destructor is not necessarily run immediately. This means its possible
     // for us to get a handle to a browser that is about to be removed. If
     // the tab count is 0 or the window is NULL, the browser is about to be
     // deleted, so we ignore it.
-    if (ShouldSyncWindow(*i) && (*i)->tab_count() &&
-        (*i)->window()) {
+    if (ShouldSyncWindow(*i) && (*i)->GetTabCount() &&
+        (*i)->HasWindow()) {
       sync_pb::SessionWindow window_s;
-      SessionID::id_type window_id = (*i)->session_id().id();
+      SessionID::id_type window_id = (*i)->GetSessionId().id();
       VLOG(1) << "Reassociating window " << window_id << " with " <<
-          (*i)->tab_count() << " tabs.";
+          (*i)->GetTabCount() << " tabs.";
       window_s.set_window_id(window_id);
-      window_s.set_selected_tab_index((*i)->active_index());
-      if ((*i)->is_type_tabbed()) {
+      window_s.set_selected_tab_index((*i)->GetActiveIndex());
+      if ((*i)->IsTypeTabbed()) {
         window_s.set_browser_type(
             sync_pb::SessionWindow_BrowserType_TYPE_TABBED);
       } else {
@@ -152,7 +153,7 @@ void SessionModelAssociator::ReassociateWindows(bool reload_tabs) {
 
       // Store the order of tabs.
       bool found_tabs = false;
-      for (int j = 0; j < (*i)->tab_count(); ++j) {
+      for (int j = 0; j < (*i)->GetTabCount(); ++j) {
         TabContentsWrapper* tab = (*i)->GetTabContentsWrapperAt(j);
         DCHECK(tab);
         if (IsValidTab(*tab)) {
@@ -194,10 +195,11 @@ void SessionModelAssociator::ReassociateWindows(bool reload_tabs) {
 }
 
 // Static.
-bool SessionModelAssociator::ShouldSyncWindow(const Browser* browser) {
-  if (browser->is_app())
+bool SessionModelAssociator::ShouldSyncWindow(
+    const SyncedWindowDelegate* window) {
+  if (window->IsApp())
     return false;
-  return browser->is_type_tabbed() || browser->is_type_popup();
+  return window->IsTypeTabbed() || window->IsTypePopup();
 }
 
 void SessionModelAssociator::ReassociateTabs(
@@ -245,9 +247,10 @@ void SessionModelAssociator::Associate(const TabContentsWrapper* tab,
                                        int64 sync_id) {
   DCHECK(CalledOnValidThread());
   SessionID::id_type session_id = tab->restore_tab_helper()->session_id().id();
-  Browser* browser = BrowserList::FindBrowserWithID(
-      tab->restore_tab_helper()->window_id().id());
-  if (!browser) {  // Can happen for weird things like developer console.
+  const SyncedWindowDelegate* window =
+      SyncedWindowDelegate::FindSyncedWindowDelegateWithId(
+          tab->restore_tab_helper()->window_id().id());
+  if (!window) {  // Can happen for weird things like developer console.
     tab_pool_.FreeTabNode(sync_id);
     return;
   }
@@ -256,11 +259,11 @@ void SessionModelAssociator::Associate(const TabContentsWrapper* tab,
   tab_map_[session_id] = t;
 
   sync_api::WriteTransaction trans(FROM_HERE, sync_service_->GetUserShare());
-  WriteTabContentsToSyncModel(*browser, *tab, sync_id, &trans);
+  WriteTabContentsToSyncModel(*window, *tab, sync_id, &trans);
 }
 
 bool SessionModelAssociator::WriteTabContentsToSyncModel(
-    const Browser& browser,
+    const SyncedWindowDelegate& window,
     const TabContentsWrapper& tab,
     int64 sync_id,
     sync_api::WriteTransaction* trans) {
@@ -284,10 +287,9 @@ bool SessionModelAssociator::WriteTabContentsToSyncModel(
   const int max_index = std::min(current_index + max_sync_navigation_count,
                                  tab.controller().entry_count());
   const int pending_index = tab.controller().pending_entry_index();
-  int index_in_window = browser.tabstrip_model()->GetIndexOfTabContents(&tab);
-  DCHECK(index_in_window != TabStripModel::kNoTab);
-  tab_s->set_pinned(browser.tabstrip_model()->IsTabPinned(index_in_window));
-  if (tab.extension_tab_helper()->extension_app()) {
+  tab_s->set_pinned(window.IsTabContentsWrapperPinned(&tab));
+  if (tab.extension_tab_helper() &&
+      tab.extension_tab_helper()->extension_app()) {
     tab_s->set_extension_app_id(
         tab.extension_tab_helper()->extension_app()->id());
   }
diff --git a/chrome/browser/sync/glue/session_model_associator.h b/chrome/browser/sync/glue/session_model_associator.h
index 5423f27..72fa1e4 100644
--- a/chrome/browser/sync/glue/session_model_associator.h
+++ b/chrome/browser/sync/glue/session_model_associator.h
@@ -23,10 +23,10 @@
 #include "chrome/browser/sessions/session_types.h"
 #include "chrome/browser/sync/engine/syncapi.h"
 #include "chrome/browser/sync/glue/synced_session_tracker.h"
+#include "chrome/browser/sync/glue/synced_window_delegate.h"
 #include "chrome/browser/sync/glue/model_associator.h"
 #include "chrome/browser/sync/protocol/session_specifics.pb.h"
 #include "chrome/browser/sync/syncable/model_type.h"
-#include "chrome/browser/ui/browser_window.h"
 #include "chrome/browser/ui/tab_contents/tab_contents_wrapper.h"
 
 class Profile;
@@ -309,7 +309,7 @@ class SessionModelAssociator
   void DeleteForeignSessions();
 
   // Determine if a window is of a type we're interested in syncing.
-  static bool ShouldSyncWindow(const Browser* browser);
+  static bool ShouldSyncWindow(const SyncedWindowDelegate* window);
 
   // Build a sync tag from tab_node_id.
   static inline std::string TabIdToTag(
@@ -333,7 +333,7 @@ class SessionModelAssociator
 
   // Fills a tab sync node with data from a TabContents object.
   // (from a local navigation event)
-  bool WriteTabContentsToSyncModel(const Browser& browser,
+  bool WriteTabContentsToSyncModel(const SyncedWindowDelegate& window,
                                    const TabContentsWrapper& tab,
                                    const int64 sync_id,
                                    sync_api::WriteTransaction* trans);
diff --git a/chrome/browser/sync/glue/synced_window_delegate.h b/chrome/browser/sync/glue/synced_window_delegate.h
new file mode 100644
index 0000000..6ef948e
--- /dev/null
+++ b/chrome/browser/sync/glue/synced_window_delegate.h
@@ -0,0 +1,68 @@
+// 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_SYNC_GLUE_SYNCED_WINDOW_DELEGATE_H_
+#define CHROME_BROWSER_SYNC_GLUE_SYNCED_WINDOW_DELEGATE_H_
+#pragma once
+
+#include <set>
+
+#include "chrome/browser/sessions/session_id.h"
+
+class TabContentsWrapper;
+
+namespace browser_sync {
+
+// A SyncedWindowDelegate is used to insulate the sync code from depending
+// directly on Browser and BrowserList.
+class SyncedWindowDelegate {
+ public:
+  // Methods originating from WindowList
+
+  // This method is to be used instead of using the BrowserList iterator.
+  static const std::set<SyncedWindowDelegate*> GetSyncedWindowDelegates();
+
+  // This method is to be used instead of using BrowserList::FindBrowserWithId()
+  static const SyncedWindowDelegate* FindSyncedWindowDelegateWithId(
+      SessionID::id_type id);
+
+  // Methods originating from Browser.
+
+  // Returns true iff the provided tab is currently "pinned" in the tab strip.
+  virtual bool IsTabContentsWrapperPinned(
+      const TabContentsWrapper* tab) const = 0;
+
+  // see Browser::GetTabContentsWrapperAt
+  virtual TabContentsWrapper* GetTabContentsWrapperAt(int index) const = 0;
+
+  // Returns true iff this browser has a visible window representation
+  // associated with it. Sometimes, if a window is being created/removed the
+  // model object may exist without its UI counterpart.
+  virtual bool HasWindow() const = 0;
+
+  // see Browser::session_id
+  virtual const SessionID& GetSessionId() const = 0;
+
+  // see Browser::tab_count
+  virtual int GetTabCount() const = 0;
+
+  // see Browser::active_index
+  virtual int GetActiveIndex() const = 0;
+
+  // see Browser::is_app
+  virtual bool IsApp() const = 0;
+
+  // see Browser::is_type_tabbed
+  virtual bool IsTypeTabbed() const = 0;
+
+  // see Browser::is_type_popup
+  virtual bool IsTypePopup() const = 0;
+
+ protected:
+  virtual ~SyncedWindowDelegate() {}
+};
+
+}  // namespace browser_sync
+
+#endif  // CHROME_BROWSER_SYNC_GLUE_SYNCED_WINDOW_DELEGATE_H_
diff --git a/chrome/browser/ui/browser.cc b/chrome/browser/ui/browser.cc
index d92651e..dfb70e7 100644
--- a/chrome/browser/ui/browser.cc
+++ b/chrome/browser/ui/browser.cc
@@ -80,6 +80,7 @@
 #include "chrome/browser/ui/browser_dialogs.h"
 #include "chrome/browser/ui/browser_list.h"
 #include "chrome/browser/ui/browser_navigator.h"
+#include "chrome/browser/ui/browser_synced_window_delegate.h"
 #include "chrome/browser/ui/browser_tab_restore_service_delegate.h"
 #include "chrome/browser/ui/browser_window.h"
 #include "chrome/browser/ui/find_bar/find_bar.h"
@@ -251,6 +252,9 @@ Browser::Browser(Type type, Profile* profile)
       ALLOW_THIS_IN_INITIALIZER_LIST(
           tab_restore_service_delegate_(
               new BrowserTabRestoreServiceDelegate(this))),
+      ALLOW_THIS_IN_INITIALIZER_LIST(
+          synced_window_delegate_(
+              new BrowserSyncedWindowDelegate(this))),
       bookmark_bar_state_(BookmarkBar::HIDDEN) {
   registrar_.Add(this, content::NOTIFICATION_SSL_VISIBLE_STATE_CHANGED,
                  NotificationService::AllSources());
diff --git a/chrome/browser/ui/browser.h b/chrome/browser/ui/browser.h
index 657f562..b30d8e4 100644
--- a/chrome/browser/ui/browser.h
+++ b/chrome/browser/ui/browser.h
@@ -45,6 +45,7 @@
 #include "content/common/page_zoom.h"
 #include "ui/gfx/rect.h"
 
+class BrowserSyncedWindowDelegate;
 class BrowserTabRestoreServiceDelegate;
 class BrowserWindow;
 class Extension;
@@ -213,6 +214,9 @@ class Browser : public TabHandlerDelegate,
   BrowserTabRestoreServiceDelegate* tab_restore_service_delegate() {
     return tab_restore_service_delegate_.get();
   }
+  BrowserSyncedWindowDelegate* synced_window_delegate() {
+    return synced_window_delegate_.get();
+  }
 
   // Get the FindBarController for this browser, creating it if it does not
   // yet exist.
@@ -362,6 +366,7 @@ class Browser : public TabHandlerDelegate,
   int tab_count() const;
   int active_index() const;
   int GetIndexOfController(const NavigationController* controller) const;
+
   // TODO(dpapad): Rename to GetActiveTabContentsWrapper().
   TabContentsWrapper* GetSelectedTabContentsWrapper() const;
   TabContentsWrapper* GetTabContentsWrapperAt(int index) const;
@@ -1261,6 +1266,9 @@ class Browser : public TabHandlerDelegate,
   // Helper which implements the TabRestoreServiceDelegate interface.
   scoped_ptr<BrowserTabRestoreServiceDelegate> tab_restore_service_delegate_;
 
+  // Helper which implements the SyncedWindowDelegate interface.
+  scoped_ptr<BrowserSyncedWindowDelegate> synced_window_delegate_;
+
   scoped_ptr<InstantController> instant_;
   scoped_ptr<InstantUnloadHandler> instant_unload_handler_;
 
diff --git a/chrome/browser/ui/browser_synced_window_delegate.cc b/chrome/browser/ui/browser_synced_window_delegate.cc
new file mode 100644
index 0000000..d9a0947
--- /dev/null
+++ b/chrome/browser/ui/browser_synced_window_delegate.cc
@@ -0,0 +1,80 @@
+// 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/ui/browser_synced_window_delegate.h"
+
+#include <set>
+
+#include "chrome/browser/sessions/session_id.h"
+#include "chrome/browser/tabs/tab_strip_model.h"
+#include "chrome/browser/ui/browser.h"
+#include "chrome/browser/ui/browser_list.h"
+#include "chrome/browser/ui/tab_contents/tab_contents_wrapper.h"
+
+// static SyncedWindowDelegate implementations
+
+// static
+const std::set<browser_sync::SyncedWindowDelegate*>
+    browser_sync::SyncedWindowDelegate::GetSyncedWindowDelegates() {
+  std::set<browser_sync::SyncedWindowDelegate*> synced_window_delegates;
+  for (BrowserList::const_iterator i = BrowserList::begin();
+       i != BrowserList::end(); ++i) {
+    synced_window_delegates.insert((*i)->synced_window_delegate());
+  }
+  return synced_window_delegates;
+}
+
+// static
+const browser_sync::SyncedWindowDelegate*
+    browser_sync::SyncedWindowDelegate::FindSyncedWindowDelegateWithId(
+        SessionID::id_type id) {
+  return BrowserList::FindBrowserWithID(id)->synced_window_delegate();
+}
+
+// BrowserSyncedWindowDelegate implementations
+
+BrowserSyncedWindowDelegate::BrowserSyncedWindowDelegate(Browser* browser)
+    : browser_(browser) {}
+
+BrowserSyncedWindowDelegate::~BrowserSyncedWindowDelegate() {}
+
+bool BrowserSyncedWindowDelegate::IsTabContentsWrapperPinned(
+    const TabContentsWrapper* tab) const {
+  int index = browser_->GetIndexOfController(&tab->controller());
+  DCHECK(index != TabStripModel::kNoTab);
+  return browser_->tabstrip_model()->IsTabPinned(index);
+}
+
+TabContentsWrapper* BrowserSyncedWindowDelegate::GetTabContentsWrapperAt(
+    int index) const {
+  return browser_->GetTabContentsWrapperAt(index);
+}
+
+bool BrowserSyncedWindowDelegate::HasWindow() const {
+  return browser_->window() != NULL;
+}
+
+const SessionID& BrowserSyncedWindowDelegate::GetSessionId() const {
+  return browser_->session_id();
+}
+
+int BrowserSyncedWindowDelegate::GetTabCount() const {
+  return browser_->tab_count();
+}
+
+int BrowserSyncedWindowDelegate::GetActiveIndex() const {
+  return browser_->active_index();
+}
+
+bool BrowserSyncedWindowDelegate::IsApp() const {
+  return browser_->is_app();
+}
+
+bool BrowserSyncedWindowDelegate::IsTypeTabbed() const {
+  return browser_->is_type_tabbed();
+}
+
+bool BrowserSyncedWindowDelegate::IsTypePopup() const {
+  return browser_->is_type_popup();
+}
diff --git a/chrome/browser/ui/browser_synced_window_delegate.h b/chrome/browser/ui/browser_synced_window_delegate.h
new file mode 100644
index 0000000..b23e211
--- /dev/null
+++ b/chrome/browser/ui/browser_synced_window_delegate.h
@@ -0,0 +1,41 @@
+// 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_UI_BROWSER_SYNCED_WINDOW_DELEGATE_H_
+#define CHROME_BROWSER_UI_BROWSER_SYNCED_WINDOW_DELEGATE_H_
+#pragma once
+
+#include "base/compiler_specific.h"
+#include "chrome/browser/sessions/session_id.h"
+#include "chrome/browser/sync/glue/synced_window_delegate.h"
+
+class Browser;
+class TabContentsWrapper;
+
+// A BrowserSyncedWindowDelegate is the Browser-based implementation of
+// SyncedWindowDelegate.
+class BrowserSyncedWindowDelegate : public browser_sync::SyncedWindowDelegate {
+ public:
+  explicit BrowserSyncedWindowDelegate(Browser* browser);
+  virtual ~BrowserSyncedWindowDelegate();
+
+  // SyncedWindowDelegate:
+  virtual bool IsTabContentsWrapperPinned(
+      const TabContentsWrapper* tab) const OVERRIDE;
+  virtual TabContentsWrapper* GetTabContentsWrapperAt(int index) const OVERRIDE;
+  virtual bool HasWindow() const OVERRIDE;
+  virtual const SessionID& GetSessionId() const OVERRIDE;
+  virtual int GetTabCount() const OVERRIDE;
+  virtual int GetActiveIndex() const OVERRIDE;
+  virtual bool IsApp() const OVERRIDE;
+  virtual bool IsTypeTabbed() const OVERRIDE;
+  virtual bool IsTypePopup() const OVERRIDE;
+
+ private:
+  Browser* browser_;
+
+  DISALLOW_COPY_AND_ASSIGN(BrowserSyncedWindowDelegate);
+};
+
+#endif  // CHROME_BROWSER_UI_BROWSER_SYNCED_WINDOW_DELEGATE_H_
diff --git a/chrome/chrome_browser.gypi b/chrome/chrome_browser.gypi
index 2c2449e..8f42043 100644
--- a/chrome/chrome_browser.gypi
+++ b/chrome/chrome_browser.gypi
@@ -2052,6 +2052,7 @@
         'browser/sync/glue/session_model_associator.h',
         'browser/sync/glue/syncable_service_adapter.cc',
         'browser/sync/glue/syncable_service_adapter.h',
+        'browser/sync/glue/synced_window_delegate.h',
         'browser/sync/glue/sync_backend_host.cc',
         'browser/sync/glue/sync_backend_host.h',
         'browser/sync/glue/synced_session_tracker.cc',
@@ -2228,6 +2229,8 @@
         'browser/ui/browser_list_win.cc',
         'browser/ui/browser_navigator.cc',
         'browser/ui/browser_navigator.h',
+        'browser/ui/browser_synced_window_delegate.h',
+        'browser/ui/browser_synced_window_delegate.cc',
         'browser/ui/browser_tab_restore_service_delegate.cc',
         'browser/ui/browser_tab_restore_service_delegate.h',
         'browser/ui/browser_window.h',