sync: Add --enable-sync-sessions-v2 and necessary plumbing to use SessionsSyncManager.

chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.h

Index: chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.h
diff --git a/chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.h b/chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.h
index 3dbbc72bfc8add95f871d2fb707e5b372a1f5e21..0740ce7bd81fd8d11e7ddf6e6a91b696a8109e37 100644
--- a/chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.h
+++ b/chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.h
@@ -20,7 +20,7 @@ class Browser;
 struct SessionTab;
 
 namespace browser_sync {
-class SessionModelAssociator;
+class OpenTabsUIDelegate;
 }
 
 namespace chrome {
@@ -47,11 +47,11 @@ class RecentTabsSubMenuModel : public ui::SimpleMenuModel,
   static const int kRecentlyClosedHeaderCommandId;
   static const int kDisabledRecentlyClosedHeaderCommandId;
 
-  // If |associator| is NULL, default associator for |browser|'s profile will
-  // be used.  Testing may require a specific |associator|.
+  // If |delegate| is NULL, default delegate for |browser|'s profile will

[Peter Kasting, 2013/11/20 21:07:45]

Nit: default -> the default

[tim (not reviewing), 2013/11/21 02:00:06]

Done.

+  // be used.  Testing may require a specific |delegate|.
   RecentTabsSubMenuModel(ui::AcceleratorProvider* accelerator_provider,
                          Browser* browser,
-                         browser_sync::SessionModelAssociator* associator);
+                         browser_sync::OpenTabsUIDelegate* delegate);

[Peter Kasting, 2013/11/20 21:07:45]

Nit: Name this identically to the member (without trailing underscore)

[tim (not reviewing), 2013/11/21 02:00:06]

Done.

   virtual ~RecentTabsSubMenuModel();
 
   // Overridden from ui::SimpleMenuModel::Delegate:
@@ -117,7 +117,7 @@ class RecentTabsSubMenuModel : public ui::SimpleMenuModel,
   // TabNavigationItems, and returns the corresponding local or other devices'
   // TabNavigationItems in |tab_items|.
   int CommandIdToTabVectorIndex(int command_id, TabNavigationItems** tab_items);
-  browser_sync::SessionModelAssociator* GetModelAssociator();
+  browser_sync::OpenTabsUIDelegate* GetOpenTabsUIDelegate();

[Peter Kasting, 2013/11/20 21:07:45]

This function should be separated from the one above, and have a comment,
especially since it's actually crucial currently that no one use |open_tabs_|
directly, and instead must call this function!

This seems very error-prone.  It's easy for a later person to just use
open_tabs_ and then have things break.  Can we instead inline the functionality
of this function into the constructor, so |open_tabs_| is set correctly?  If not
(because we need to wait for an event that happens after construction), can we
replace this accessor function by a callback registration so that once the
correct value is available, we get called back to update open_tabs_?

[tim (not reviewing), 2013/11/21 02:00:06]

Yeah, I'm not sure we can guarantee the delegate can always be pulled up on
construction (I'll reply back if I find otherwise).

As to your suggestion of a callback registration, are you voting for requiring
code accessing open_tabs_delegate_ to NULL-check the value first and deal with
that, rather than force them to go through the lazy-initializer?  Or did you
have something else in mind for avoiding pre-initialized usage.

I'll add a TODO here for now and follow up.

[Peter Kasting, 2013/11/21 02:15:44]

The issue is whether such code can run before we've gotten the callback.  If so,
then there's nothing to be done: this object will be NULL and no attempt to
lazy-init will succeed, so the methods in question have to handle NULL (and have
to do so in today's code).

If, however, we expect some of these methods to only be called once this object
ought to be initialized, they can simply DCHECK that it's non-NULL and not have
to do anything else.

 
   // Overridden from TabRestoreServiceObserver:
   virtual void TabRestoreServiceChanged(TabRestoreService* service) OVERRIDE;
@@ -125,7 +125,7 @@ class RecentTabsSubMenuModel : public ui::SimpleMenuModel,
 
   Browser* browser_;  // Weak.
 
-  browser_sync::SessionModelAssociator* associator_;  // Weak.
+  browser_sync::OpenTabsUIDelegate* open_tabs_;  // Weak.

[Peter Kasting, 2013/11/20 21:07:45]

Nit: |open_tabs_delegate_|

[tim (not reviewing), 2013/11/21 02:00:06]

Done.

 
   // Accelerator for reopening last closed tab.
   ui::Accelerator reopen_closed_tab_accelerator_;