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

chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.h

 // Copyright 2012 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_TOOLBAR_RECENT_TABS_SUB_MENU_MODEL_H_
 #define CHROME_BROWSER_UI_TOOLBAR_RECENT_TABS_SUB_MENU_MODEL_H_
 
 #include <set>
 
 #include "base/memory/weak_ptr.h"
 #include "chrome/browser/favicon/favicon_service.h"
 #include "chrome/browser/sessions/tab_restore_service.h"
 #include "chrome/browser/sessions/tab_restore_service_observer.h"
 #include "chrome/browser/sync/glue/synced_session.h"
 #include "chrome/common/cancelable_task_tracker.h"
 #include "ui/base/accelerators/accelerator.h"
 #include "ui/base/models/simple_menu_model.h"
 
 class Browser;
 struct SessionTab;
 
 namespace browser_sync {
-class SessionModelAssociator;
+class OpenTabsUIDelegate;
 }
 
 namespace chrome {
 struct FaviconImageResult;
 }
 
 namespace gfx {
 class Image;
 }
 
 namespace ui {
 class AcceleratorProvider;
 }
 
 // A menu model that builds the contents of "Recent tabs" submenu, which include
 // the recently closed tabs/windows of current device i.e. local entries, and
 // opened tabs of other devices.
 class RecentTabsSubMenuModel : public ui::SimpleMenuModel,
                                public ui::SimpleMenuModel::Delegate,
                                public TabRestoreServiceObserver {
  public:
   // Command Id for recently closed items header or disabled item to which the
   // accelerator string will be appended.
   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:
   virtual bool IsCommandIdChecked(int command_id) const OVERRIDE;
   virtual bool IsCommandIdEnabled(int command_id) const OVERRIDE;
   virtual bool GetAcceleratorForCommandId(
       int command_id,
       ui::Accelerator* accelerator) OVERRIDE;
   virtual void ExecuteCommand(int command_id, int event_flags) OVERRIDE;
   virtual const gfx::Font* GetLabelFontAt(int index) const OVERRIDE;
@@ skipping 45 matching lines @@
   void OnFaviconDataAvailable(int command_id,
                               const chrome::FaviconImageResult& image_result);
 
   // Clear all recently closed tabs and windows.
   void ClearLocalEntries();
 
   // Converts |command_id| of menu item to index in local or other devices'
   // 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;
   virtual void TabRestoreServiceDestroyed(TabRestoreService* service) OVERRIDE;
 
   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_;
 
   // Navigation items for local recently closed tabs.  The |command_id| for
   // these is set to |kFirstLocalTabCommandId| plus the index into the vector.
   // Upon invocation of the menu, the navigation information is retrieved from
   // |local_tab_navigation_items_| and used to navigate to the item specified.
   TabNavigationItems local_tab_navigation_items_;
 
@@ skipping 16 matching lines @@
 
   CancelableTaskTracker local_tab_cancelable_task_tracker_;
   CancelableTaskTracker other_devices_tab_cancelable_task_tracker_;
 
   base::WeakPtrFactory<RecentTabsSubMenuModel> weak_ptr_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(RecentTabsSubMenuModel);
 };
 
 #endif  // CHROME_BROWSER_UI_TOOLBAR_RECENT_TABS_SUB_MENU_MODEL_H_