Chromium Code Reviews
Help | Chromium Project | Sign in
(3)

Issue 2824073002: [sync] Prevent sessions flare on startup (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 week, 4 days ago by Patrick Noland
Modified:
1 week, 2 days ago
Reviewers:
skym
CC:
chromium-reviews, sync-reviews_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

[sync] Prevent sessions flare on startup SyncSessionsWebContents router triggered its flare for any notification it received, no matter what. This caused it to trigger a flare more or less immediately upon startup. This change caused the flare to trigger only if the notification is meaningful; i.e. it's for a valid tab's load completed event. This is a change from the past behavior, which wouldn't flare for tabs that shouldn't sync, like the new tab page, but would flare for any event type. R=skym@chromium.org BUG=712372 Review-Url: https://codereview.chromium.org/2824073002 Cr-Commit-Position: refs/heads/master@{#466119} Committed: https://chromium.googlesource.com/chromium/src/+/e8227a0e1c5fc7f230fe575086e1816a4e9fa6ae

Patch Set 1 #

Total comments: 6

Patch Set 2 : Add unit tests, fix flare conditions #

Total comments: 2

Patch Set 3 : Add syncable_service comment, add test case #

Patch Set 4 : Disable test on android #

Unified diffs Side-by-side diffs Delta from patch set Stats (+121 lines, -19 lines) Patch
M chrome/browser/sync/sessions/browser_list_router_helper.cc View 1 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/sync/sessions/sync_sessions_router_tab_helper.h View 1 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/sync/sessions/sync_sessions_router_tab_helper.cc View 1 2 chunks +3 lines, -3 lines 0 comments Download
M chrome/browser/sync/sessions/sync_sessions_web_contents_router.h View 1 1 chunk +2 lines, -1 line 0 comments Download
M chrome/browser/sync/sessions/sync_sessions_web_contents_router.cc View 1 2 chunks +10 lines, -7 lines 0 comments Download
A chrome/browser/sync/sessions/sync_sessions_web_contents_router_unittest.cc View 1 2 3 1 chunk +92 lines, -0 lines 0 comments Download
M chrome/test/BUILD.gn View 1 1 chunk +1 line, -0 lines 0 comments Download
M components/sync/model/syncable_service.h View 1 2 1 chunk +11 lines, -6 lines 0 comments Download
Commit queue not available (can’t edit this change).

Messages

Total messages: 35 (28 generated)
Patrick Noland
Sky,PTAL
1 week, 4 days ago (2017-04-17 23:53:48 UTC) #3
skym
Would like to see unit tests if we're going ahead with this change. Though, I ...
1 week, 4 days ago (2017-04-18 15:50:51 UTC) #6
Patrick Noland
Sky, PTAL https://codereview.chromium.org/2824073002/diff/1/chrome/browser/sync/sessions/sync_sessions_web_contents_router.cc File chrome/browser/sync/sessions/sync_sessions_web_contents_router.cc (right): https://codereview.chromium.org/2824073002/diff/1/chrome/browser/sync/sessions/sync_sessions_web_contents_router.cc#newcode63 chrome/browser/sync/sessions/sync_sessions_web_contents_router.cc:63: return; On 2017/04/18 15:50:51, skym wrote: > ...
1 week, 4 days ago (2017-04-18 22:37:42 UTC) #14
skym
lgtm Can you update the CL description? https://codereview.chromium.org/2824073002/diff/40001/chrome/browser/sync/sessions/sync_sessions_web_contents_router_unittest.cc File chrome/browser/sync/sessions/sync_sessions_web_contents_router_unittest.cc (right): https://codereview.chromium.org/2824073002/diff/40001/chrome/browser/sync/sessions/sync_sessions_web_contents_router_unittest.cc#newcode72 chrome/browser/sync/sessions/sync_sessions_web_contents_router_unittest.cc:72: router()->InjectStartSyncFlare( What ...
1 week, 4 days ago (2017-04-18 22:39:53 UTC) #15
Patrick Noland
https://codereview.chromium.org/2824073002/diff/40001/chrome/browser/sync/sessions/sync_sessions_web_contents_router_unittest.cc File chrome/browser/sync/sessions/sync_sessions_web_contents_router_unittest.cc (right): https://codereview.chromium.org/2824073002/diff/40001/chrome/browser/sync/sessions/sync_sessions_web_contents_router_unittest.cc#newcode72 chrome/browser/sync/sessions/sync_sessions_web_contents_router_unittest.cc:72: router()->InjectStartSyncFlare( On 2017/04/18 22:39:52, skym wrote: > What about ...
1 week, 4 days ago (2017-04-18 22:53:19 UTC) #21
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2824073002/120001
1 week, 2 days ago (2017-04-20 20:40:38 UTC) #32
commit-bot: I haz the power
1 week, 2 days ago (2017-04-20 20:48:01 UTC) #35
Message was sent while issue was closed.
Committed patchset #4 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/e8227a0e1c5fc7f230fe575086e1...
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld cc6ac46