Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2887513002:
[sync] Scope BrowserListRouterHelper to browsers with a matching profile (Closed)
|
Created:
3 years, 7 months ago by Patrick Noland Modified:
3 years, 7 months ago Reviewers:
skym CC:
chromium-reviews, sync-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[sync] Scope BrowserListRouterHelper to browsers with a matching profile
BrowserListRouterHelper listens to tab insertion/removal events for any and
all profiles, which duplicates events across multiple routers. This is
wrong and bad. This CL induces BrowserListRouterHelper to only add
itself as a listener for a browser with a matching profile.
BUG=721410
R=skym@chromium.org
Review-Url: https://codereview.chromium.org/2887513002
Cr-Commit-Position: refs/heads/master@{#472347}
Committed: https://chromium.googlesource.com/chromium/src/+/952d07a6309c7e2a8f1531d85e3dcf626c3d5dd9
Patch Set 1 #
Total comments: 10
Patch Set 2 : Remove need for friend classing #
Total comments: 3
Patch Set 3 : Remove wrap, use ContainsValue #Patch Set 4 : fix last memory leak #
Messages
Total messages: 34 (23 generated)
The CQ bit was checked by pnoland@chromium.org 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...
Sky, PTAL
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
lgtm https://codereview.chromium.org/2887513002/diff/1/chrome/browser/sync/session... File chrome/browser/sync/sessions/browser_list_router_helper_unittest.cc (right): https://codereview.chromium.org/2887513002/diff/1/chrome/browser/sync/session... chrome/browser/sync/sessions/browser_list_router_helper_unittest.cc:19: std::set<Browser*> attached_browsers(SyncSessionsWebContentsRouter* router) { The naming for this method is odd. GetAttachedBrowsers ? Also, it should take a const& router, right? https://codereview.chromium.org/2887513002/diff/1/chrome/browser/sync/session... chrome/browser/sync/sessions/browser_list_router_helper_unittest.cc:56: // Check that the added browsers are only attached to their respective browser Would it be useful to check that browser counts as expected as well? https://codereview.chromium.org/2887513002/diff/1/chrome/browser/sync/session... chrome/browser/sync/sessions/browser_list_router_helper_unittest.cc:64: // Cleanup needed for manually created browsers so they don't complain about Who is "they"? Why does this test case care? Is is possible to test that BrowserListRouterHelper::OnBrowserRemoved does the right thing? If so, you could just drop this comment, right? https://codereview.chromium.org/2887513002/diff/1/chrome/browser/sync/session... chrome/browser/sync/sessions/browser_list_router_helper_unittest.cc:69: } Would also be cool to test ~BrowserListRouterHelper() but feel free to not do this. https://codereview.chromium.org/2887513002/diff/1/chrome/browser/sync/session... File chrome/browser/sync/sessions/sync_sessions_web_contents_router.h (right): https://codereview.chromium.org/2887513002/diff/1/chrome/browser/sync/session... chrome/browser/sync/sessions/sync_sessions_web_contents_router.h:65: friend class BrowserListRouterHelperTest; I'd prefer to not use friending for tests, when possible. It seems like the first class way to do this would to make a MockLocalSessionEventHandler class, right? Then inject that with SyncSessionsWebContentsRouter::StartRoutingTo. Then any time a tab is modified, you should get events, right?
The CQ bit was checked by pnoland@chromium.org 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...
https://codereview.chromium.org/2887513002/diff/1/chrome/browser/sync/session... File chrome/browser/sync/sessions/browser_list_router_helper_unittest.cc (right): https://codereview.chromium.org/2887513002/diff/1/chrome/browser/sync/session... chrome/browser/sync/sessions/browser_list_router_helper_unittest.cc:19: std::set<Browser*> attached_browsers(SyncSessionsWebContentsRouter* router) { On 2017/05/15 20:58:01, skym wrote: > The naming for this method is odd. GetAttachedBrowsers ? Also, it should take a > const& router, right? n/a since I removed this. https://codereview.chromium.org/2887513002/diff/1/chrome/browser/sync/session... chrome/browser/sync/sessions/browser_list_router_helper_unittest.cc:56: // Check that the added browsers are only attached to their respective browser On 2017/05/15 20:58:01, skym wrote: > Would it be useful to check that browser counts as expected as well? n/a since I removed this https://codereview.chromium.org/2887513002/diff/1/chrome/browser/sync/session... chrome/browser/sync/sessions/browser_list_router_helper_unittest.cc:64: // Cleanup needed for manually created browsers so they don't complain about On 2017/05/15 20:58:01, skym wrote: > Who is "they"? Why does this test case care? > > Is is possible to test that BrowserListRouterHelper::OnBrowserRemoved does the > right thing? If so, you could just drop this comment, right? I will fix this comment, but the destructor for Browser will DCHECK if it has unclosed tabs. The browser created by the base class is automatically cleaned up, but manually created ones aren't. Even if I remove the browsers from the helper, they still need to destruct at some point, so I need to close their tabs. https://codereview.chromium.org/2887513002/diff/1/chrome/browser/sync/session... chrome/browser/sync/sessions/browser_list_router_helper_unittest.cc:69: } On 2017/05/15 20:58:01, skym wrote: > Would also be cool to test ~BrowserListRouterHelper() but feel free to not do > this. Acknowledged. https://codereview.chromium.org/2887513002/diff/1/chrome/browser/sync/session... File chrome/browser/sync/sessions/sync_sessions_web_contents_router.h (right): https://codereview.chromium.org/2887513002/diff/1/chrome/browser/sync/session... chrome/browser/sync/sessions/sync_sessions_web_contents_router.h:65: friend class BrowserListRouterHelperTest; On 2017/05/15 20:58:01, skym wrote: > I'd prefer to not use friending for tests, when possible. It seems like the > first class way to do this would to make a MockLocalSessionEventHandler class, > right? Then inject that with SyncSessionsWebContentsRouter::StartRoutingTo. Then > any time a tab is modified, you should get events, right? Done.
The CQ bit was unchecked by pnoland@chromium.org
The CQ bit was checked by pnoland@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from skym@chromium.org Link to the patchset: https://codereview.chromium.org/2887513002/#ps20001 (title: "Remove need for friend classing")
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 pnoland@chromium.org
The CQ bit was checked by pnoland@chromium.org
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
Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Still lgtm https://codereview.chromium.org/2887513002/diff/20001/chrome/browser/sync/ses... File chrome/browser/sync/sessions/browser_list_router_helper_unittest.cc (right): https://codereview.chromium.org/2887513002/diff/20001/chrome/browser/sync/ses... chrome/browser/sync/sessions/browser_list_router_helper_unittest.cc:16: class MockLocalSessionEventHandler : public LocalSessionEventHandler { Thanks for doing this! https://codereview.chromium.org/2887513002/diff/20001/chrome/browser/sync/ses... chrome/browser/sync/sessions/browser_list_router_helper_unittest.cc:46: profile_2.get(), browser()->type(), false, CreateBrowserWindow())); Is this ASAN failure because your BrowserWindow never gets destructed? Also, what do you think of dropping all the WrapUnique calls in this file? std::unique_ptr<BrowserWindow window_w(CreateBrowserWindow()); std::unique_ptr<Browser> browser_2(CreateBrowser(profile_2.get(), browser()->type(), false, window)2); https://codereview.chromium.org/2887513002/diff/20001/chrome/browser/sync/ses... chrome/browser/sync/sessions/browser_list_router_helper_unittest.cc:64: EXPECT_FALSE(std::find(handler_1_urls->begin(), handler_1_urls->end(), Also, base/stl_util.h has a function for this! What do you think of EXPECT_TRUE(base::ContainsValue(handler_1.seen_urls(), gurl_1)); EXPECT_FALSE(base::ContainsValue(handler_1.seen_urls(), gurl_2));
The CQ bit was checked by pnoland@chromium.org 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-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by pnoland@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from skym@chromium.org Link to the patchset: https://codereview.chromium.org/2887513002/#ps40001 (title: "Remove wrap, use ContainsValue")
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
Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by pnoland@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from skym@chromium.org Link to the patchset: https://codereview.chromium.org/2887513002/#ps60001 (title: "fix last memory leak")
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": 60001, "attempt_start_ts": 1494978395834410,
"parent_rev": "d1ada059c7362462bf72dc44c103db994362d9c3", "commit_rev":
"952d07a6309c7e2a8f1531d85e3dcf626c3d5dd9"}
Message was sent while issue was closed.
Description was changed from ========== [sync] Scope BrowserListRouterHelper to browsers with a matching profile BrowserListRouterHelper listens to tab insertion/removal events for any and all profiles, which duplicates events across multiple routers. This is wrong and bad. This CL induces BrowserListRouterHelper to only add itself as a listener for a browser with a matching profile. BUG=721410 R=skym@chromium.org ========== to ========== [sync] Scope BrowserListRouterHelper to browsers with a matching profile BrowserListRouterHelper listens to tab insertion/removal events for any and all profiles, which duplicates events across multiple routers. This is wrong and bad. This CL induces BrowserListRouterHelper to only add itself as a listener for a browser with a matching profile. BUG=721410 R=skym@chromium.org Review-Url: https://codereview.chromium.org/2887513002 Cr-Commit-Position: refs/heads/master@{#472347} Committed: https://chromium.googlesource.com/chromium/src/+/952d07a6309c7e2a8f1531d85e3d... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/952d07a6309c7e2a8f1531d85e3d... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
