Dynamic updating recent menu for tabs from other devices.

If user opens recent/history menu and at the same moment syncronization
is updating session from other devices then user will can see
non-relevant a list of opened tabs from the other devices.

It's easy to reproduce.
1. Open a browser with enabled syncronization.
2. Open chrome://sync-internals page.
3. Open recent/history menu and wait for updating session on
   chrome://sync-internals page.
4. Open new tab on other synced device.
5. When chrome://sync-internals shows that session was updated the
   opened menu will show previous state of list list of opened tabs
   from the other devices.

R=pkasting@chromium.org, pavely@chromium.org

Review-Url: https://codereview.chromium.org/2768633003
Cr-Commit-Position: refs/heads/master@{#460503}

chrome/browser/ui/browser_tabrestore_browsertest.cc

@@ -82,7 +82,7 @@ IN_PROC_BROWSER_TEST_F(BrowserTabRestoreTest, RecentTabsMenuTabDisposition) {
   // Restore tabs using the browser's recent tabs menu.
   content::DOMMessageQueue queue;
   Browser* browser = active_browser_list->get(0);
-  RecentTabsSubMenuModel menu(NULL, browser, NULL);
+  RecentTabsSubMenuModel menu(nullptr, browser);
   menu.ExecuteCommand(
       RecentTabsSubMenuModel::GetFirstRecentTabsCommandId(), 0);
   AwaitTabsReady(&queue, 2);

chrome/browser/ui/cocoa/app_menu/app_menu_controller_unittest.mm

@@ -2,17 +2,19 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#import "chrome/browser/ui/cocoa/app_menu/app_menu_controller.h"
 #include "base/command_line.h"
 #include "base/mac/scoped_nsobject.h"
 #include "base/macros.h"
+#include "base/memory/ptr_util.h"
 #include "base/run_loop.h"
 #include "base/strings/sys_string_conversions.h"
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/app/chrome_command_ids.h"
 #include "chrome/browser/sync/profile_sync_service_factory.h"
+#include "chrome/browser/sync/profile_sync_test_util.h"
 #include "chrome/browser/ui/browser_list.h"
 #include "chrome/browser/ui/browser_list_observer.h"
-#import "chrome/browser/ui/cocoa/app_menu/app_menu_controller.h"
 #include "chrome/browser/ui/cocoa/test/cocoa_profile_test.h"
 #include "chrome/browser/ui/cocoa/test/run_loop_testing.h"
 #import "chrome/browser/ui/cocoa/toolbar/toolbar_controller.h"
@@ -26,6 +28,7 @@
 #include "chrome/grit/theme_resources.h"
 #include "chrome/test/base/testing_profile.h"
 #include "components/browser_sync/profile_sync_service.h"
+#include "components/browser_sync/profile_sync_service_mock.h"
 #include "components/sync/base/sync_prefs.h"
 #include "components/sync/device_info/local_device_info_provider_mock.h"
 #include "components/sync/driver/sync_client.h"
@@ -40,6 +43,10 @@
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/base/resource/resource_bundle.h"
 
+using testing::_;
+using testing::Invoke;
+using testing::Return;
+
 namespace {
 
 class MockAppMenuModel : public AppMenuModel {
@@ -59,30 +66,37 @@ class DummyRouter : public sync_sessions::LocalSessionEventRouter {
 
 class AppMenuControllerTest : public CocoaProfileTest {
  public:
-  AppMenuControllerTest()
-      : local_device_(new syncer::LocalDeviceInfoProviderMock(
-            "AppMenuControllerTest",
-            "Test Machine",
-            "Chromium 10k",
-            "Chrome 10k",
-            sync_pb::SyncEnums_DeviceType_TYPE_LINUX,
-            "device_id")) {}
+  AppMenuControllerTest() {
+    TestingProfile::TestingFactories factories;
+    factories.push_back(std::make_pair(ProfileSyncServiceFactory::GetInstance(),
+                                       BuildMockProfileSyncService));
+    AddTestingFactories(factories);
+  }
 
   void SetUp() override {
     CocoaProfileTest::SetUp();
     ASSERT_TRUE(browser());
 
+    local_device_ = base::MakeUnique<syncer::LocalDeviceInfoProviderMock>(
+        "AppMenuControllerTest", "Test Machine", "Chromium 10k", "Chrome 10k",
+        sync_pb::SyncEnums_DeviceType_TYPE_LINUX, "device_id");
+
     controller_.reset([[AppMenuController alloc] initWithBrowser:browser()]);
-    fake_model_.reset(new MockAppMenuModel);
 
-    sync_prefs_.reset(new syncer::SyncPrefs(profile()->GetPrefs()));
-    dummy_router_.reset(new DummyRouter());
-    manager_.reset(new sync_sessions::SessionsSyncManager(
+    fake_model_ = base::MakeUnique<MockAppMenuModel>();
+
+    sync_prefs_ = base::MakeUnique<syncer::SyncPrefs>(profile()->GetPrefs());
+
+    mock_sync_service_ = static_cast<browser_sync::ProfileSyncServiceMock*>(
+        ProfileSyncServiceFactory::GetInstance()->GetForProfile(profile()));
+
+    manager_ = base::MakeUnique<sync_sessions::SessionsSyncManager>(
         ProfileSyncServiceFactory::GetForProfile(profile())
             ->GetSyncClient()
             ->GetSyncSessionsClient(),
-        sync_prefs_.get(), local_device_.get(), dummy_router_.get(),
-        base::Closure(), base::Closure()));
+        sync_prefs_.get(), local_device_.get(), &dummy_router_, base::Closure(),
+        base::Closure());
+
     manager_->MergeDataAndStartSyncing(
         syncer::SESSIONS, syncer::SyncDataList(),
         std::unique_ptr<syncer::SyncChangeProcessor>(
@@ -95,17 +109,28 @@ class AppMenuControllerTest : public CocoaProfileTest {
     helper->ExportToSessionsSyncManager(manager_.get());
   }
 
-  sync_sessions::OpenTabsUIDelegate* GetOpenTabsDelegate() {
-    return manager_.get();
-  }
-
   void TearDown() override {
     fake_model_.reset();
     controller_.reset();
     manager_.reset();
+    sync_prefs_.reset();
+    local_device_.reset();
     CocoaProfileTest::TearDown();
   }
 
+  void EnableSync() {
+    EXPECT_CALL(*mock_sync_service_, IsSyncActive())
+        .WillRepeatedly(Return(true));
+    EXPECT_CALL(*mock_sync_service_,
+                IsDataTypeControllerRunning(syncer::SESSIONS))
+        .WillRepeatedly(Return(true));
+    EXPECT_CALL(*mock_sync_service_,
+                IsDataTypeControllerRunning(syncer::PROXY_TABS))
+        .WillRepeatedly(Return(true));
+    EXPECT_CALL(*mock_sync_service_, GetOpenTabsUIDelegateMock())
+        .WillRepeatedly(Return(manager_.get()));
+  }
+
   AppMenuController* controller() {
     return controller_.get();
   }
@@ -115,10 +140,11 @@ class AppMenuControllerTest : public CocoaProfileTest {
   std::unique_ptr<MockAppMenuModel> fake_model_;
 
  private:
+  std::unique_ptr<syncer::LocalDeviceInfoProviderMock> local_device_;
+  DummyRouter dummy_router_;
   std::unique_ptr<syncer::SyncPrefs> sync_prefs_;
-  std::unique_ptr<DummyRouter> dummy_router_;
+  browser_sync::ProfileSyncServiceMock* mock_sync_service_ = nullptr;
   std::unique_ptr<sync_sessions::SessionsSyncManager> manager_;
-  std::unique_ptr<syncer::LocalDeviceInfoProviderMock> local_device_;
 };
 
 TEST_F(AppMenuControllerTest, Initialized) {
@@ -139,14 +165,15 @@ TEST_F(AppMenuControllerTest, DispatchSimple) {
 }
 
 TEST_F(AppMenuControllerTest, RecentTabsFavIcon) {
+  EnableSync();
+
   RecentTabsBuilderTestHelper recent_tabs_builder;
   recent_tabs_builder.AddSession();
   recent_tabs_builder.AddWindow(0);
   recent_tabs_builder.AddTab(0, 0);
   RegisterRecentTabs(&recent_tabs_builder);
 
-  RecentTabsSubMenuModel recent_tabs_sub_menu_model(
-      NULL, browser(), GetOpenTabsDelegate());
+  RecentTabsSubMenuModel recent_tabs_sub_menu_model(nullptr, browser());
   fake_model_->AddSubMenuWithStringId(
       IDC_RECENT_TABS_MENU, IDS_RECENT_TABS_MENU,
       &recent_tabs_sub_menu_model);
@@ -174,6 +201,8 @@ TEST_F(AppMenuControllerTest, RecentTabsFavIcon) {
 }
 
 TEST_F(AppMenuControllerTest, RecentTabsElideTitle) {
+  EnableSync();
+
   // Add 1 session with 1 window and 2 tabs.
   RecentTabsBuilderTestHelper recent_tabs_builder;
   recent_tabs_builder.AddSession();
@@ -186,8 +215,7 @@ TEST_F(AppMenuControllerTest, RecentTabsElideTitle) {
       base::Time::Now() - base::TimeDelta::FromMinutes(10), tab2_long_title);
   RegisterRecentTabs(&recent_tabs_builder);
 
-  RecentTabsSubMenuModel recent_tabs_sub_menu_model(
-      NULL, browser(), GetOpenTabsDelegate());
+  RecentTabsSubMenuModel recent_tabs_sub_menu_model(nullptr, browser());
   fake_model_->AddSubMenuWithStringId(
       IDC_RECENT_TABS_MENU, IDS_RECENT_TABS_MENU,
       &recent_tabs_sub_menu_model);

chrome/browser/ui/toolbar/app_menu_model.cc

@@ -717,9 +717,8 @@ void AppMenuModel::Build() {
   AddSeparator(ui::NORMAL_SEPARATOR);
 
   if (!browser_->profile()->IsOffTheRecord()) {
-    recent_tabs_sub_menu_model_.reset(new RecentTabsSubMenuModel(provider_,
-                                                                 browser_,
-                                                                 NULL));
+    recent_tabs_sub_menu_model_ =
+        base::MakeUnique<RecentTabsSubMenuModel>(provider_, browser_);
     AddSubMenuWithStringId(IDC_RECENT_TABS_MENU, IDS_HISTORY_MENU,
                            recent_tabs_sub_menu_model_.get());
   }

chrome/browser/ui/toolbar/recent_tabs_builder_test_helper.cc

@@ -23,6 +23,7 @@ const char kBaseSessionTag[] = "session_tag";
 const char kBaseSessionName[] = "session_name";
 const char kBaseTabUrl[] = "http://foo/?";
 const char kTabTitleFormat[] = "session=%d;window=%d;tab=%d";
+const uint64_t kMaxMinutesRange = 1000;
 
 struct TitleTimestampPair {
   base::string16 title;
@@ -135,7 +136,8 @@ SessionID::id_type RecentTabsBuilderTestHelper::GetWindowID(int session_index,
 
 void RecentTabsBuilderTestHelper::AddTab(int session_index, int window_index) {
   base::Time timestamp =
-      start_time_ + base::TimeDelta::FromMinutes(base::RandUint64());
+      start_time_ +
+      base::TimeDelta::FromMinutes(base::RandGenerator(kMaxMinutesRange));
   AddTabWithInfo(session_index, window_index, timestamp, base::string16());
 }
 

chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc

@@ -179,15 +179,18 @@ const int RecentTabsSubMenuModel::kDisabledRecentlyClosedHeaderCommandId = 1121;
 
 RecentTabsSubMenuModel::RecentTabsSubMenuModel(
     ui::AcceleratorProvider* accelerator_provider,
-    Browser* browser,
-    sync_sessions::OpenTabsUIDelegate* open_tabs_delegate)
+    Browser* browser)
     : ui::SimpleMenuModel(this),
       browser_(browser),
-      open_tabs_delegate_(open_tabs_delegate),
+      open_tabs_delegate_(nullptr),
       last_local_model_index_(kHistorySeparatorIndex),
       default_favicon_(
           ui::ResourceBundle::GetSharedInstance().GetNativeImageNamed(
               IDR_DEFAULT_FAVICON)),
+#if !defined(OS_MACOSX)
+      tab_restore_service_observer_(this),
+      sync_observer_(this),
+#endif  // !defined(OS_MACOSX)
       weak_ptr_factory_(this) {
   // Invoke asynchronous call to load tabs from local last session, which does
   // nothing if the tabs have already been loaded or they shouldn't be loaded.
@@ -197,13 +200,21 @@ RecentTabsSubMenuModel::RecentTabsSubMenuModel(
   if (service) {
     service->LoadTabsFromLastSession();
 
-  // TODO(sail): enable this when mac implements the dynamic menu, together with
-  // MenuModelDelegate::MenuStructureChanged().
+// Mac doesn't support the dynamic menu.
 #if !defined(OS_MACOSX)
-    service->AddObserver(this);
+    tab_restore_service_observer_.Add(service);
 #endif
   }
 
+// Mac doesn't support the dynamic menu.
+#if !defined(OS_MACOSX)
+  browser_sync::ProfileSyncService* sync_service =
+      ProfileSyncServiceFactory::GetInstance()->GetForProfile(
+          browser_->profile());
+  if (sync_service)
+    sync_observer_.Add(sync_service);
+#endif  // !defined(OS_MACOSX)
+
   Build();
 
   // Retrieve accelerator key for IDC_RESTORE_TAB now, because on ASH, it's not
@@ -231,12 +242,7 @@ RecentTabsSubMenuModel::RecentTabsSubMenuModel(
   }
 }
 
-RecentTabsSubMenuModel::~RecentTabsSubMenuModel() {
-  sessions::TabRestoreService* service =
-      TabRestoreServiceFactory::GetForProfile(browser_->profile());
-  if (service)
-    service->RemoveObserver(this);
-}
+RecentTabsSubMenuModel::~RecentTabsSubMenuModel() {}
 
 bool RecentTabsSubMenuModel::IsCommandIdChecked(int command_id) const {
   return false;
@@ -474,8 +480,7 @@ void RecentTabsSubMenuModel::BuildLocalEntries() {
 
 void RecentTabsSubMenuModel::BuildTabsFromOtherDevices() {
   // All other devices' items (device headers or tabs) use AddItem*() to append
-  // a menu item, because they are always only built once (i.e. invoked from
-  // Constructor()) and don't change after that.
+  // a menu item, because they take always place in the end of menu.
 
   sync_sessions::OpenTabsUIDelegate* open_tabs = GetOpenTabsUIDelegate();
   std::vector<const sync_sessions::SyncedSession*> sessions;
@@ -708,6 +713,17 @@ void RecentTabsSubMenuModel::ClearLocalEntries() {
   local_window_items_.clear();
 }
 
+void RecentTabsSubMenuModel::ClearTabsFromOtherDevices() {
+  DCHECK_GE(last_local_model_index_, 0);
+  int count = GetItemCount();
+  for (int index = count - 1; index > last_local_model_index_; --index)
+    RemoveItemAt(index);
+
+  other_devices_tab_cancelable_task_tracker_.TryCancelAll();
+
+  other_devices_tab_navigation_items_.clear();
+}
+
 sync_sessions::OpenTabsUIDelegate*
 RecentTabsSubMenuModel::GetOpenTabsUIDelegate() {
   if (!open_tabs_delegate_) {
@@ -736,3 +752,19 @@ void RecentTabsSubMenuModel::TabRestoreServiceDestroyed(
     sessions::TabRestoreService* service) {
   TabRestoreServiceChanged(service);
 }
+
+void RecentTabsSubMenuModel::OnSyncConfigurationCompleted(
+    syncer::SyncService* sync) {
+  OnForeignSessionUpdated(sync);
+}
+
+void RecentTabsSubMenuModel::OnForeignSessionUpdated(
+    syncer::SyncService* sync) {
+  ClearTabsFromOtherDevices();
+
+  BuildTabsFromOtherDevices();
+
+  ui::MenuModelDelegate* menu_model_delegate = GetMenuModelDelegate();
+  if (menu_model_delegate)
+    menu_model_delegate->OnMenuStructureChanged();
+}

chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.h

@@ -9,11 +9,13 @@
 
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
+#include "base/scoped_observer.h"
 #include "base/task/cancelable_task_tracker.h"
 #include "base/timer/elapsed_timer.h"
 #include "components/favicon/core/favicon_service.h"
 #include "components/sessions/core/tab_restore_service.h"
 #include "components/sessions/core/tab_restore_service_observer.h"
+#include "components/sync/driver/sync_service_observer.h"
 #include "components/sync_sessions/synced_session.h"
 #include "ui/base/accelerators/accelerator.h"
 #include "ui/base/models/simple_menu_model.h"
@@ -36,6 +38,11 @@ namespace sync_sessions {
 class OpenTabsUIDelegate;
 }
 
+namespace syncer {
+class SyncService;
+class SyncServiceBase;
+}  // namespace syncer
+
 namespace ui {
 class AcceleratorProvider;
 }
@@ -45,7 +52,8 @@ class AcceleratorProvider;
 // opened tabs of other devices.
 class RecentTabsSubMenuModel : public ui::SimpleMenuModel,
                                public ui::SimpleMenuModel::Delegate,
-                               public sessions::TabRestoreServiceObserver {
+                               public sessions::TabRestoreServiceObserver,
+                               public syncer::SyncServiceObserver {
  public:
   // Command Id for recently closed items header or disabled item to which the
   // accelerator string will be appended.
@@ -57,10 +65,9 @@ class RecentTabsSubMenuModel : public ui::SimpleMenuModel,
   static int GetFirstRecentTabsCommandId();
 
   // If |open_tabs_delegate| is NULL, the default delegate for |browser|'s
-  // profile will be used. Testing may require a specific |open_tabs_delegate|.
+  // profile will be used.
   RecentTabsSubMenuModel(ui::AcceleratorProvider* accelerator_provider,
-                         Browser* browser,
-                         sync_sessions::OpenTabsUIDelegate* open_tabs_delegate);
+                         Browser* browser);
   ~RecentTabsSubMenuModel() override;
 
   // Overridden from ui::SimpleMenuModel::Delegate:
@@ -122,6 +129,9 @@ class RecentTabsSubMenuModel : public ui::SimpleMenuModel,
   // Clear all recently closed tabs and windows.
   void ClearLocalEntries();
 
+  // Clears all tabs from other devices.
+  void ClearTabsFromOtherDevices();
+
   // 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|.
@@ -139,6 +149,10 @@ class RecentTabsSubMenuModel : public ui::SimpleMenuModel,
   void TabRestoreServiceDestroyed(
       sessions::TabRestoreService* service) override;
 
+  // Overridden from syncer::SyncServiceObserver:
+  void OnSyncConfigurationCompleted(syncer::SyncService* sync) override;
+  void OnForeignSessionUpdated(syncer::SyncService* sync) override;
+
   Browser* browser_;  // Weak.
 
   sync_sessions::OpenTabsUIDelegate* open_tabs_delegate_;  // Weak.
@@ -178,6 +192,15 @@ class RecentTabsSubMenuModel : public ui::SimpleMenuModel,
   // Time the menu is open for until a recent tab is selected.
   base::ElapsedTimer menu_opened_timer_;
 
+// Mac doesn't support the dynamic menu.
+#if !defined(OS_MACOSX)
+  ScopedObserver<sessions::TabRestoreService, RecentTabsSubMenuModel>
+      tab_restore_service_observer_;
+
+  ScopedObserver<syncer::SyncServiceBase, RecentTabsSubMenuModel>
+      sync_observer_;
+#endif
+
   base::WeakPtrFactory<RecentTabsSubMenuModel> weak_ptr_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(RecentTabsSubMenuModel);

components/browser_sync/profile_sync_service_mock.cc

@@ -18,4 +18,12 @@ ProfileSyncServiceMock::ProfileSyncServiceMock(InitParams* init_params)
 
 ProfileSyncServiceMock::~ProfileSyncServiceMock() {}
 
+sync_sessions::OpenTabsUIDelegate*
+ProfileSyncServiceMock::GetOpenTabsUIDelegate() {
+  sync_sessions::OpenTabsUIDelegate* mock_delegate =
+      GetOpenTabsUIDelegateMock();
+  return mock_delegate ? mock_delegate
+                       : ProfileSyncService::GetOpenTabsUIDelegate();
+}
+
 }  // namespace browser_sync

components/browser_sync/profile_sync_service_mock.h

@@ -80,6 +80,9 @@ class ProfileSyncServiceMock : public ProfileSyncService {
   MOCK_METHOD1(OnActionableError, void(const syncer::SyncProtocolError&));
   MOCK_CONST_METHOD1(IsDataTypeControllerRunning, bool(syncer::ModelType));
 
+  MOCK_METHOD0(GetOpenTabsUIDelegateMock, sync_sessions::OpenTabsUIDelegate*());
+  sync_sessions::OpenTabsUIDelegate* GetOpenTabsUIDelegate() override;
+
   // DataTypeManagerObserver mocks.
   MOCK_METHOD1(OnConfigureDone,
                void(const syncer::DataTypeManager::ConfigureResult&));