Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2768633003:
Dynamic updating recent menu for tabs from other devices. (Closed)
DescriptionDynamic 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}
Committed: https://chromium.googlesource.com/chromium/src/+/0f51324df2b3f6a90d7454fe66a17c012bd16277
Patch Set 1 #
Total comments: 20
Patch Set 2 : Fixes after review. #
Total comments: 6
Patch Set 3 : Fixes after remarks and track enabling syncronization. #
Total comments: 8
Patch Set 4 : Fixes after remarks. #Patch Set 5 : Rebasing to master. #Patch Set 6 : Fixes of compilation #Patch Set 7 : Fixes compilation and test on Mac. #Patch Set 8 : Fixes compilation and test on Mac. #
Total comments: 5
Patch Set 9 : Fixes compilation and test on Mac. #Messages
Total messages: 59 (36 generated)
PTAL
Is there an associated BUG= for this? This sounds very similar to some discussions on bug 444816, for example.
On 2017/03/22 08:47:57, Peter Kasting wrote: > Is there an associated BUG= for this? This sounds very similar to some > discussions on bug 444816, for example. In bug 444816 are been discussing almost the same thing not relevant information in a menu. But this issue solves problem with showing opened tabs in the other device. This problem wasn't mentioned in bug 444816.
https://codereview.chromium.org/2768633003/diff/1/chrome/browser/ui/toolbar/a... File chrome/browser/ui/toolbar/app_menu_model.cc (right): https://codereview.chromium.org/2768633003/diff/1/chrome/browser/ui/toolbar/a... chrome/browser/ui/toolbar/app_menu_model.cc:721: new RecentTabsSubMenuModel(provider_, browser_)); Nit: While here, prefer MakeUnique to bare new: recent_tabs_sub_menu_model_ = base::MakeUnique<RecentTabsSubMenuModel>(provider_, browser_); https://codereview.chromium.org/2768633003/diff/1/chrome/browser/ui/toolbar/r... File chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc (right): https://codereview.chromium.org/2768633003/diff/1/chrome/browser/ui/toolbar/r... chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc:205: #if !defined(OS_MACOSX) Nit: Add note about why this isn't on for Mac https://codereview.chromium.org/2768633003/diff/1/chrome/browser/ui/toolbar/r... chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc:209: if (sync_service && sync_service->IsSyncActive()) { Nit: No {} https://codereview.chromium.org/2768633003/diff/1/chrome/browser/ui/toolbar/r... chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc:252: sync_service->RemoveObserver(this); Can this be done with a ScopedObserver class member instead of manual Add/Remove calls? Or is it possible that the sync service will be torn down while the menu is open? (I would think a ScopedObserver would work, and for the TabRestoreService as well.) https://codereview.chromium.org/2768633003/diff/1/chrome/browser/ui/toolbar/r... chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc:732: // Cancel asynchronous FaviconService::GetFaviconImageForPageURL() tasks of Nit: These comments (here and below) add nothing to the code, remove them. https://codereview.chromium.org/2768633003/diff/1/chrome/browser/ui/toolbar/r... File chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.h (right): https://codereview.chromium.org/2768633003/diff/1/chrome/browser/ui/toolbar/r... chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.h:130: // Clear all tabs from other devices. Nit: Clear -> Clears; see http://google.github.io/styleguide/cppguide.html#Function_Comments . (Various other comments above here also get this wrong, feel free to fix or not) https://codereview.chromium.org/2768633003/diff/1/chrome/browser/ui/toolbar/r... chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.h:150: // Overridden from syncer::SyncServiceObserver Nit: Trailing colon https://codereview.chromium.org/2768633003/diff/1/chrome/browser/ui/toolbar/r... chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.h:151: void OnSyncCycleCompleted(syncer::SyncService* sync) override {} Nit: I'd weakly prefer defining this in the .cc than the .h, since it's virtual https://codereview.chromium.org/2768633003/diff/1/chrome/browser/ui/toolbar/r... chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.h:156: sync_sessions::OpenTabsUIDelegate* open_tabs_delegate_ = nullptr; // Weak. Nit: I'm a fan of initializing at the declaration like this, but only if it's done for all member variables that need initialization. So I would say, either do this for all the other members (as applicable), or else don't do it for this one. https://codereview.chromium.org/2768633003/diff/1/chrome/browser/ui/toolbar/r... File chrome/browser/ui/toolbar/recent_tabs_sub_menu_model_unittest.cc (right): https://codereview.chromium.org/2768633003/diff/1/chrome/browser/ui/toolbar/r... chrome/browser/ui/toolbar/recent_tabs_sub_menu_model_unittest.cc:133: local_device_ = base::MakeUnique<syncer::LocalDeviceInfoProviderMock>( This seems very heavyweight. It would be nice to avoid Gmock entirely, perhaps by simply calling the "foreign session updated" method of the menu model directly? See also comments in the sync code. https://codereview.chromium.org/2768633003/diff/1/chrome/browser/ui/toolbar/r... chrome/browser/ui/toolbar/recent_tabs_sub_menu_model_unittest.cc:539: Nit: Prefer to pull this out into its own test, even if it means duplicating setup code (but consider refactoring that setup code to avoid duplication), and give it (and this existing test) descriptive names so it's clear what aspects of the other devices submenu they're testing. https://codereview.chromium.org/2768633003/diff/1/chrome/browser/ui/toolbar/r... chrome/browser/ui/toolbar/recent_tabs_sub_menu_model_unittest.cc:540: // Remove this when dynamic model is enabled in RecentTabsSubMenuModel. Nit: This comment sounds like "remove this block", but I think you mean "remove the #if". Try "Enable this for Mac" instead? https://codereview.chromium.org/2768633003/diff/1/chrome/browser/ui/toolbar/r... chrome/browser/ui/toolbar/recent_tabs_sub_menu_model_unittest.cc:574: num_items = model.GetItemCount(); Do we really need to check everything below in order to verify the change you've made in this CL? It would be good to have a minimal test that just says, hey, we added a tab somewhere, and we picked up on that in the menu, without retesting everything else. https://codereview.chromium.org/2768633003/diff/1/chrome/browser/ui/toolbar/r... chrome/browser/ui/toolbar/recent_tabs_sub_menu_model_unittest.cc:610: EXPECT_TRUE(model.GetLabelFontListAt(4) != nullptr); Nit: Can probably be EXPECT_NE(nullptr, ...); (2 places) https://codereview.chromium.org/2768633003/diff/1/components/browser_sync/pro... File components/browser_sync/profile_sync_service.h (right): https://codereview.chromium.org/2768633003/diff/1/components/browser_sync/pro... components/browser_sync/profile_sync_service.h:616: friend class ::RecentTabsSubMenuModelTest; Is there a way to test this functionality without using private APIs? If not, I'm inclined to suggest refactoring so there is. I'm uncomfortable with a random toolbar test needing to reach into the internals of a sync class. For example, make the recent tabs submenu model have a mockable or injectable data source, so it doesn't need to talk to sync at all in test code. https://codereview.chromium.org/2768633003/diff/1/components/browser_sync/pro... File components/browser_sync/profile_sync_service_mock.cc (right): https://codereview.chromium.org/2768633003/diff/1/components/browser_sync/pro... components/browser_sync/profile_sync_service_mock.cc:23: // If delegate is not mocked then we use usual one Nit: Remove this comment, it adds nothing to the code. https://codereview.chromium.org/2768633003/diff/1/components/browser_sync/pro... components/browser_sync/profile_sync_service_mock.cc:24: sync_sessions::OpenTabsUIDelegate* tabs = GetOpenTabsUIDelegateMock(); Nit: Call this |mock| or |delegate| or |mock_delegate| rather than |tabs|. https://codereview.chromium.org/2768633003/diff/1/components/browser_sync/pro... components/browser_sync/profile_sync_service_mock.cc:27: return tabs; Nit: Simpler: return tabs ? tabs : ProfileSyncService::GetOpenTabsUIDelegate();
On 2017/03/23 04:48:36, Peter Kasting wrote: > https://codereview.chromium.org/2768633003/diff/1/chrome/browser/ui/toolbar/a... > File chrome/browser/ui/toolbar/app_menu_model.cc (right): > > https://codereview.chromium.org/2768633003/diff/1/chrome/browser/ui/toolbar/a... > chrome/browser/ui/toolbar/app_menu_model.cc:721: new > RecentTabsSubMenuModel(provider_, browser_)); > Nit: While here, prefer MakeUnique to bare new: > > recent_tabs_sub_menu_model_ = > base::MakeUnique<RecentTabsSubMenuModel>(provider_, browser_); > > https://codereview.chromium.org/2768633003/diff/1/chrome/browser/ui/toolbar/r... > File chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc (right): > > https://codereview.chromium.org/2768633003/diff/1/chrome/browser/ui/toolbar/r... > chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc:205: #if > !defined(OS_MACOSX) > Nit: Add note about why this isn't on for Mac > > https://codereview.chromium.org/2768633003/diff/1/chrome/browser/ui/toolbar/r... > chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc:209: if (sync_service && > sync_service->IsSyncActive()) { > Nit: No {} > > https://codereview.chromium.org/2768633003/diff/1/chrome/browser/ui/toolbar/r... > chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc:252: > sync_service->RemoveObserver(this); > Can this be done with a ScopedObserver class member instead of manual Add/Remove > calls? Or is it possible that the sync service will be torn down while the menu > is open? (I would think a ScopedObserver would work, and for the > TabRestoreService as well.) > > https://codereview.chromium.org/2768633003/diff/1/chrome/browser/ui/toolbar/r... > chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc:732: // Cancel > asynchronous FaviconService::GetFaviconImageForPageURL() tasks of > Nit: These comments (here and below) add nothing to the code, remove them. > > https://codereview.chromium.org/2768633003/diff/1/chrome/browser/ui/toolbar/r... > File chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.h (right): > > https://codereview.chromium.org/2768633003/diff/1/chrome/browser/ui/toolbar/r... > chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.h:130: // Clear all tabs > from other devices. > Nit: Clear -> Clears; see > http://google.github.io/styleguide/cppguide.html#Function_Comments . (Various > other comments above here also get this wrong, feel free to fix or not) > > https://codereview.chromium.org/2768633003/diff/1/chrome/browser/ui/toolbar/r... > chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.h:150: // Overridden from > syncer::SyncServiceObserver > Nit: Trailing colon > > https://codereview.chromium.org/2768633003/diff/1/chrome/browser/ui/toolbar/r... > chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.h:151: void > OnSyncCycleCompleted(syncer::SyncService* sync) override {} > Nit: I'd weakly prefer defining this in the .cc than the .h, since it's virtual > > https://codereview.chromium.org/2768633003/diff/1/chrome/browser/ui/toolbar/r... > chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.h:156: > sync_sessions::OpenTabsUIDelegate* open_tabs_delegate_ = nullptr; // Weak. > Nit: I'm a fan of initializing at the declaration like this, but only if it's > done for all member variables that need initialization. > > So I would say, either do this for all the other members (as applicable), or > else don't do it for this one. > > https://codereview.chromium.org/2768633003/diff/1/chrome/browser/ui/toolbar/r... > File chrome/browser/ui/toolbar/recent_tabs_sub_menu_model_unittest.cc (right): > > https://codereview.chromium.org/2768633003/diff/1/chrome/browser/ui/toolbar/r... > chrome/browser/ui/toolbar/recent_tabs_sub_menu_model_unittest.cc:133: > local_device_ = base::MakeUnique<syncer::LocalDeviceInfoProviderMock>( > This seems very heavyweight. It would be nice to avoid Gmock entirely, perhaps > by simply calling the "foreign session updated" method of the menu model > directly? See also comments in the sync code. > > https://codereview.chromium.org/2768633003/diff/1/chrome/browser/ui/toolbar/r... > chrome/browser/ui/toolbar/recent_tabs_sub_menu_model_unittest.cc:539: > Nit: Prefer to pull this out into its own test, even if it means duplicating > setup code (but consider refactoring that setup code to avoid duplication), and > give it (and this existing test) descriptive names so it's clear what aspects of > the other devices submenu they're testing. > > https://codereview.chromium.org/2768633003/diff/1/chrome/browser/ui/toolbar/r... > chrome/browser/ui/toolbar/recent_tabs_sub_menu_model_unittest.cc:540: // Remove > this when dynamic model is enabled in RecentTabsSubMenuModel. > Nit: This comment sounds like "remove this block", but I think you mean "remove > the #if". Try "Enable this for Mac" instead? > > https://codereview.chromium.org/2768633003/diff/1/chrome/browser/ui/toolbar/r... > chrome/browser/ui/toolbar/recent_tabs_sub_menu_model_unittest.cc:574: num_items > = model.GetItemCount(); > Do we really need to check everything below in order to verify the change you've > made in this CL? It would be good to have a minimal test that just says, hey, > we added a tab somewhere, and we picked up on that in the menu, without > retesting everything else. > > https://codereview.chromium.org/2768633003/diff/1/chrome/browser/ui/toolbar/r... > chrome/browser/ui/toolbar/recent_tabs_sub_menu_model_unittest.cc:610: > EXPECT_TRUE(model.GetLabelFontListAt(4) != nullptr); > Nit: Can probably be EXPECT_NE(nullptr, ...); (2 places) > > https://codereview.chromium.org/2768633003/diff/1/components/browser_sync/pro... > File components/browser_sync/profile_sync_service.h (right): > > https://codereview.chromium.org/2768633003/diff/1/components/browser_sync/pro... > components/browser_sync/profile_sync_service.h:616: friend class > ::RecentTabsSubMenuModelTest; > Is there a way to test this functionality without using private APIs? If not, > I'm inclined to suggest refactoring so there is. I'm uncomfortable with a > random toolbar test needing to reach into the internals of a sync class. > > For example, make the recent tabs submenu model have a mockable or injectable > data source, so it doesn't need to talk to sync at all in test code. > > https://codereview.chromium.org/2768633003/diff/1/components/browser_sync/pro... > File components/browser_sync/profile_sync_service_mock.cc (right): > > https://codereview.chromium.org/2768633003/diff/1/components/browser_sync/pro... > components/browser_sync/profile_sync_service_mock.cc:23: // If delegate is not > mocked then we use usual one > Nit: Remove this comment, it adds nothing to the code. > > https://codereview.chromium.org/2768633003/diff/1/components/browser_sync/pro... > components/browser_sync/profile_sync_service_mock.cc:24: > sync_sessions::OpenTabsUIDelegate* tabs = GetOpenTabsUIDelegateMock(); > Nit: Call this |mock| or |delegate| or |mock_delegate| rather than |tabs|. > > https://codereview.chromium.org/2768633003/diff/1/components/browser_sync/pro... > components/browser_sync/profile_sync_service_mock.cc:27: return tabs; > Nit: Simpler: > > return tabs ? tabs : ProfileSyncService::GetOpenTabsUIDelegate(); PTAL. I have fixed all remarks.
LGTM https://codereview.chromium.org/2768633003/diff/1/chrome/browser/ui/toolbar/r... File chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.h (right): https://codereview.chromium.org/2768633003/diff/1/chrome/browser/ui/toolbar/r... chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.h:130: // Clear all tabs from other devices. On 2017/03/23 04:48:34, Peter Kasting wrote: > Nit: Clear -> Clears; see > http://google.github.io/styleguide/cppguide.html#Function_Comments . (Various > other comments above here also get this wrong, feel free to fix or not) Looks like you removed this comment instead of updating it. https://codereview.chromium.org/2768633003/diff/1/chrome/browser/ui/toolbar/r... chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.h:150: // Overridden from syncer::SyncServiceObserver On 2017/03/23 04:48:34, Peter Kasting wrote: > Nit: Trailing colon Looks like you removed the namespace instead of adding this. https://codereview.chromium.org/2768633003/diff/20001/chrome/browser/ui/toolb... File chrome/browser/ui/toolbar/recent_tabs_sub_menu_model_unittest.cc (right): https://codereview.chromium.org/2768633003/diff/20001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/recent_tabs_sub_menu_model_unittest.cc:234: std::unique_ptr<FakeSyncServiceObserverList> fake_sync_service_observer_list_; Nit: How come this needs to be stored in a unique_ptr instead of just by value? Does it actually have to be deleted at a certain spot in TearDown()? Is this to ensure that we won't have any observers left on it between tests? https://codereview.chromium.org/2768633003/diff/20001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/recent_tabs_sub_menu_model_unittest.cc:592: int num_items = model.GetItemCount(); Nit: Just inline into the next statement (2 places) https://codereview.chromium.org/2768633003/diff/20001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/recent_tabs_sub_menu_model_unittest.cc:594: model.ActivatedAt(0); Again, is there a way to slim down this test to only check the things we need to verify this fix? I'm wondering how many ActivatedAt(), IsEnabledAt(), GetLabelFontListAt(), and GetURLAndTitleForItemAtIndex() calls we can rip out and still verify this. Basically, I'd expect we could do something like: * Get the item count * Push a sync change * Check that there's one additional item * (Maybe) Check that the second-to-last item has the title of this new tab And just not bother checking anything else, since all the rest should be covered by other tests.
lgtm % one comment https://codereview.chromium.org/2768633003/diff/20001/chrome/browser/ui/toolb... File chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc (right): https://codereview.chromium.org/2768633003/diff/20001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc:214: if (sync_service && sync_service->IsSyncActive()) Is this line executed once per process or every time menu is opened? If former then it could catch sync_service before it is initialized and therefore not subscribe for notifications.
https://codereview.chromium.org/2768633003/diff/20001/chrome/browser/ui/toolb... File chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc (right): https://codereview.chromium.org/2768633003/diff/20001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc:214: if (sync_service && sync_service->IsSyncActive()) On 2017/03/24 01:20:05, pavely wrote: > Is this line executed once per process or every time menu is opened? If former > then it could catch sync_service before it is initialized and therefore not > subscribe for notifications. Looks like it's called each time the menu is opened. However, that makes me wonder: can this conditional be false when the menu is opened, but become true later while the menu is still open, at which point we'd ideally like to be receiving updates? If yes, how do we best fix this? Is it safe to assume that |sync_service| would always be non-null in such a case, but IsSyncActive() might not be, yet? Should we just remove the check for IsSyncActive() here, or is there something more complex to do?
https://codereview.chromium.org/2768633003/diff/20001/chrome/browser/ui/toolb... File chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc (right): https://codereview.chromium.org/2768633003/diff/20001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc:214: if (sync_service && sync_service->IsSyncActive()) On 2017/03/24 01:35:21, Peter Kasting wrote: > On 2017/03/24 01:20:05, pavely wrote: > > Is this line executed once per process or every time menu is opened? If former > > then it could catch sync_service before it is initialized and therefore not > > subscribe for notifications. > > Looks like it's called each time the menu is opened. > > However, that makes me wonder: can this conditional be false when the menu is > opened, but become true later while the menu is still open, at which point we'd > ideally like to be receiving updates? If yes, how do we best fix this? Is it > safe to assume that |sync_service| would always be non-null in such a case, but > IsSyncActive() might not be, yet? Should we just remove the check for > IsSyncActive() here, or is there something more complex to do? Yeah, we should remove call to IsSyncActive(). If sync is enabled then sync_service is not nullptr and you can subscribe for notifications regardless of ISyncActive() state. You should still keep if(sync_service) check though, but if it is nullptr it is guaranteed to stay nullptr.
On 2017/03/24 04:13:22, pavely wrote: > https://codereview.chromium.org/2768633003/diff/20001/chrome/browser/ui/toolb... > File chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc (right): > > https://codereview.chromium.org/2768633003/diff/20001/chrome/browser/ui/toolb... > chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc:214: if (sync_service && > sync_service->IsSyncActive()) > On 2017/03/24 01:35:21, Peter Kasting wrote: > > On 2017/03/24 01:20:05, pavely wrote: > > > Is this line executed once per process or every time menu is opened? If > former > > > then it could catch sync_service before it is initialized and therefore not > > > subscribe for notifications. > > > > Looks like it's called each time the menu is opened. > > > > However, that makes me wonder: can this conditional be false when the menu is > > opened, but become true later while the menu is still open, at which point > we'd > > ideally like to be receiving updates? If yes, how do we best fix this? Is it > > safe to assume that |sync_service| would always be non-null in such a case, > but > > IsSyncActive() might not be, yet? Should we just remove the check for > > IsSyncActive() here, or is there something more complex to do? > > Yeah, we should remove call to IsSyncActive(). If sync is enabled then > sync_service is not nullptr and you can subscribe for notifications regardless > of ISyncActive() state. You should still keep if(sync_service) check though, but > if it is nullptr it is guaranteed to stay nullptr. I fixed all remarks again (I hope). Also I removed IsSyncActive() and added tracking for enabling sync engine to update menu dynamically. I rewrote test and also fixed bug http://crbug.com/603744. PTAL.
LGTM https://codereview.chromium.org/2768633003/diff/40001/chrome/browser/ui/toolb... File chrome/browser/ui/toolbar/recent_tabs_builder_test_helper.cc (right): https://codereview.chromium.org/2768633003/diff/40001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/recent_tabs_builder_test_helper.cc:140: base::TimeDelta::FromMinutes(base::RandGenerator(kMaxMinutesRange)); Do we need randomness at all here? Can we get by with just monotonically increasing timestamps or something? Random numbers in a test always make me nervous. https://codereview.chromium.org/2768633003/diff/40001/chrome/browser/ui/toolb... File chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc (right): https://codereview.chromium.org/2768633003/diff/40001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc:758: OnForeignSessionUpdated(sync); Will we not get a foreign session updated call automatically? (I have no idea, I know nothing about sync, it just sounds plausible that we could.) https://codereview.chromium.org/2768633003/diff/40001/chrome/browser/ui/toolb... File chrome/browser/ui/toolbar/recent_tabs_sub_menu_model_unittest.cc (right): https://codereview.chromium.org/2768633003/diff/40001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/recent_tabs_sub_menu_model_unittest.cc:178: manager_.reset(new sync_sessions::SessionsSyncManager( Nit: Prefer =MakeUnique to reset(new https://codereview.chromium.org/2768633003/diff/40001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/recent_tabs_sub_menu_model_unittest.cc:187: DisableSync(); Is this line necessary? https://codereview.chromium.org/2768633003/diff/40001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/recent_tabs_sub_menu_model_unittest.cc:584: // Create menu with disabled syncronization. Nit: synchronization
lgtm https://codereview.chromium.org/2768633003/diff/40001/chrome/browser/ui/toolb... File chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc (right): https://codereview.chromium.org/2768633003/diff/40001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc:758: OnForeignSessionUpdated(sync); On 2017/03/24 19:54:02, Peter Kasting wrote: > Will we not get a foreign session updated call automatically? (I have no idea, > I know nothing about sync, it just sounds plausible that we could.) We currently won't. That's something we could fix on sync side. I need to check other consumers of this notification to be sure.
I have made some small fixes. https://codereview.chromium.org/2768633003/diff/40001/chrome/browser/ui/toolb... File chrome/browser/ui/toolbar/recent_tabs_builder_test_helper.cc (right): https://codereview.chromium.org/2768633003/diff/40001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/recent_tabs_builder_test_helper.cc:140: base::TimeDelta::FromMinutes(base::RandGenerator(kMaxMinutesRange)); On 2017/03/24 19:54:02, Peter Kasting wrote: > Do we need randomness at all here? Can we get by with just monotonically > increasing timestamps or something? Random numbers in a test always make me > nervous. If I replace random timestamp to monotonically increasing timestamps it will work but I'm not sure that I will not break the idea to add tabs in random order and show them in the right order. https://codereview.chromium.org/2768633003/diff/40001/chrome/browser/ui/toolb... File chrome/browser/ui/toolbar/recent_tabs_sub_menu_model_unittest.cc (right): https://codereview.chromium.org/2768633003/diff/40001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/recent_tabs_sub_menu_model_unittest.cc:187: DisableSync(); On 2017/03/24 19:54:03, Peter Kasting wrote: > Is this line necessary? Okay. I have added DisableSync() or EnabledSync() in each test.
The CQ bit was checked by sath@yandex-team.ru to run a CQ dry run
The CQ bit was unchecked by sath@yandex-team.ru
The CQ bit was checked by sath@yandex-team.ru to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
Still LGTM
The CQ bit was checked by sath@yandex-team.ru to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was unchecked by sath@yandex-team.ru
The CQ bit was checked by sath@yandex-team.ru to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by sath@yandex-team.ru to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by sath@yandex-team.ru to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was unchecked by sath@yandex-team.ru
The CQ bit was unchecked by sath@yandex-team.ru
The CQ bit was checked by sath@yandex-team.ru to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/03/27 19:53:11, Peter Kasting wrote: > Still LGTM I fixed tests on Mac. Here chrome/browser/ui/cocoa/app_menu/app_menu_controller_unittest.mm PTAL.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: CQ has no permission to schedule in bucket master.tryserver.chromium.linux
https://codereview.chromium.org/2768633003/diff/140001/chrome/browser/sync/pr... File chrome/browser/sync/profile_sync_test_util.cc (right): https://codereview.chromium.org/2768633003/diff/140001/chrome/browser/sync/pr... chrome/browser/sync/profile_sync_test_util.cc:78: ProfileSyncService::InitParams init_params = Nit: I might inline this below just to keep it in parallel with the code above more, so it's clear they do almost the same thing https://codereview.chromium.org/2768633003/diff/140001/chrome/browser/sync/pr... File chrome/browser/sync/profile_sync_test_util.h (right): https://codereview.chromium.org/2768633003/diff/140001/chrome/browser/sync/pr... chrome/browser/sync/profile_sync_test_util.h:61: std::unique_ptr<KeyedService> BuildNiceMockProfileSyncService( Nit: Add comment about how this is distinct from the function just above, and when callers would use one versus the other And really, I'm not clear on this either. Why does the Mac test use the nicemock version but the other test doesn't? Why should the niceness of the mock ever matter -- do we error on uninteresting calls in the mac test for some reason? This addition seems suspicious.
https://codereview.chromium.org/2768633003/diff/140001/chrome/browser/sync/pr... File chrome/browser/sync/profile_sync_test_util.cc (right): https://codereview.chromium.org/2768633003/diff/140001/chrome/browser/sync/pr... chrome/browser/sync/profile_sync_test_util.cc:78: ProfileSyncService::InitParams init_params = On 2017/03/28 21:38:39, Peter Kasting wrote: > Nit: I might inline this below just to keep it in parallel with the code above > more, so it's clear they do almost the same thing I can't because of --- error: taking the address of a temporary object of type 'ProfileSyncService::InitParams' [-Waddress-of-temporary] --- https://codereview.chromium.org/2768633003/diff/140001/chrome/browser/sync/pr... File chrome/browser/sync/profile_sync_test_util.h (right): https://codereview.chromium.org/2768633003/diff/140001/chrome/browser/sync/pr... chrome/browser/sync/profile_sync_test_util.h:61: std::unique_ptr<KeyedService> BuildNiceMockProfileSyncService( On 2017/03/28 21:38:39, Peter Kasting wrote: > Nit: Add comment about how this is distinct from the function just above, and > when callers would use one versus the other > > And really, I'm not clear on this either. Why does the Mac test use the > nicemock version but the other test doesn't? Why should the niceness of the > mock ever matter -- do we error on uninteresting calls in the mac test for some > reason? This addition seems suspicious. Before my changes in class RecentTabsSubMenuModel class AppMenuControllerTest used a real instance ProfileSyncService which started inside CocoaProfileTest::SetUp(). Some observers was added inside CocoaProfileTest::SetUp() in the real instance ProfileSyncService. It worked because RecentTabsSubMenuModel used instance of OpenTabsUIDelegate passed through constructor. When I got rid of OpenTabsUIDelegate parameter in constructor RecentTabsSubMenuModel I always need to have a mock of ProfileSyncService with mock GetOpenTabsUIDelegateMock in order to return testing instance of SessionsSyncManager if synchronization is enabled. So I needed to use AddTestingFactories(...) to create mock ProfileSyncService instead of real instance inside CocoaProfileTest::SetUp(). But in this case I got many "unintresting call methods for mock". And I decided to create NiceMock of ProfileSyncService.
https://codereview.chromium.org/2768633003/diff/140001/chrome/browser/sync/pr... File chrome/browser/sync/profile_sync_test_util.h (right): https://codereview.chromium.org/2768633003/diff/140001/chrome/browser/sync/pr... chrome/browser/sync/profile_sync_test_util.h:61: std::unique_ptr<KeyedService> BuildNiceMockProfileSyncService( On 2017/03/29 06:26:15, sath wrote: > On 2017/03/28 21:38:39, Peter Kasting wrote: > > Nit: Add comment about how this is distinct from the function just above, and > > when callers would use one versus the other > > > > And really, I'm not clear on this either. Why does the Mac test use the > > nicemock version but the other test doesn't? Why should the niceness of the > > mock ever matter -- do we error on uninteresting calls in the mac test for > some > > reason? This addition seems suspicious. > > Before my changes in class RecentTabsSubMenuModel class AppMenuControllerTest > used a real instance ProfileSyncService which started inside > CocoaProfileTest::SetUp(). Some observers was added inside > CocoaProfileTest::SetUp() in the real instance ProfileSyncService. It worked > because RecentTabsSubMenuModel used instance of OpenTabsUIDelegate passed > through constructor. > When I got rid of OpenTabsUIDelegate parameter in constructor > RecentTabsSubMenuModel I always need to have a mock of ProfileSyncService with > mock GetOpenTabsUIDelegateMock in order to return testing instance of > SessionsSyncManager if synchronization is enabled. > So I needed to use AddTestingFactories(...) to create mock ProfileSyncService > instead of real instance inside CocoaProfileTest::SetUp(). But in this case I > got many "unintresting call methods for mock". And I decided to create NiceMock > of ProfileSyncService. Did the "uninteresting call"s result in test failures, or just output spew? If the latter, I would probably avoid adding this method. I'd only do it if you need it to fix failures. The other option is to just make the default BuildMockProfileSyncService() return a NiceMock instead of a normal Mock, which seems OK too.
On 2017/03/29 07:20:46, Peter Kasting wrote: > https://codereview.chromium.org/2768633003/diff/140001/chrome/browser/sync/pr... > File chrome/browser/sync/profile_sync_test_util.h (right): > > https://codereview.chromium.org/2768633003/diff/140001/chrome/browser/sync/pr... > chrome/browser/sync/profile_sync_test_util.h:61: std::unique_ptr<KeyedService> > BuildNiceMockProfileSyncService( > On 2017/03/29 06:26:15, sath wrote: > > On 2017/03/28 21:38:39, Peter Kasting wrote: > > > Nit: Add comment about how this is distinct from the function just above, > and > > > when callers would use one versus the other > > > > > > And really, I'm not clear on this either. Why does the Mac test use the > > > nicemock version but the other test doesn't? Why should the niceness of the > > > mock ever matter -- do we error on uninteresting calls in the mac test for > > some > > > reason? This addition seems suspicious. > > > > Before my changes in class RecentTabsSubMenuModel class AppMenuControllerTest > > used a real instance ProfileSyncService which started inside > > CocoaProfileTest::SetUp(). Some observers was added inside > > CocoaProfileTest::SetUp() in the real instance ProfileSyncService. It worked > > because RecentTabsSubMenuModel used instance of OpenTabsUIDelegate passed > > through constructor. > > When I got rid of OpenTabsUIDelegate parameter in constructor > > RecentTabsSubMenuModel I always need to have a mock of ProfileSyncService > with > > mock GetOpenTabsUIDelegateMock in order to return testing instance of > > SessionsSyncManager if synchronization is enabled. > > So I needed to use AddTestingFactories(...) to create mock ProfileSyncService > > instead of real instance inside CocoaProfileTest::SetUp(). But in this case I > > got many "unintresting call methods for mock". And I decided to create > NiceMock > > of ProfileSyncService. > > Did the "uninteresting call"s result in test failures, or just output spew? Just output spew. The tests pass OK. > > If the latter, I would probably avoid adding this method. I'd only do it if you > need it to fix failures. Okay I will remove nice mock variant. > > The other option is to just make the default BuildMockProfileSyncService() > return a NiceMock instead of a normal Mock, which seems OK too.
The CQ bit was checked by sath@yandex-team.ru to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was unchecked by sath@yandex-team.ru
> Did the "uninteresting call"s result in test failures, or just output spew? > > If the latter, I would probably avoid adding this method. I'd only do it if you > need it to fix failures. > > The other option is to just make the default BuildMockProfileSyncService() > return a NiceMock instead of a normal Mock, which seems OK too. I removed NiceMock. All targets are passed. So I hope this is the last one. PTAL.
LGTM
The CQ bit was checked by sath@yandex-team.ru
The patchset sent to the CQ was uploaded after l-g-t-m from pavely@chromium.org Link to the patchset: https://codereview.chromium.org/2768633003/#ps160001 (title: "Fixes compilation and test on Mac.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 160001, "attempt_start_ts": 1490817086750000,
"parent_rev": "402a6fcbf1b694f3655afa23e71b6d1f6d33da55", "commit_rev":
"0f51324df2b3f6a90d7454fe66a17c012bd16277"}
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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} Committed: https://chromium.googlesource.com/chromium/src/+/0f51324df2b3f6a90d7454fe66a1... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001) as https://chromium.googlesource.com/chromium/src/+/0f51324df2b3f6a90d7454fe66a1... |
