Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2159453002:
[Sync] Don't start up sync when FirstSetupCompleted is false and no setup in progress (Closed)
|
Created:
4 years, 5 months ago by Nicolas Zea Modified:
4 years, 5 months ago Reviewers:
maxbogue CC:
chromium-reviews, sync-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Sync] Don't start up sync when FirstSetupCompleted is false and no setup in progress
Instead of starting up the backend, we now don't even start it up if the first
setup completed flag is false, unless a setup is in progress. On auto start
platforms, first setup completed is manually set to true as part of init.
BUG=624915
Committed: https://crrev.com/d1de1f77e44072018f41b1f54ffca27a7069edb4
Cr-Commit-Position: refs/heads/master@{#405950}
Patch Set 1 #Patch Set 2 : Self review #Patch Set 3 : Fix auto start scenario #Patch Set 4 : Self review #
Total comments: 6
Patch Set 5 : Address comments and fix tests #Patch Set 6 : Self review #
Total comments: 2
Patch Set 7 : Bring back android fix #Patch Set 8 : Address comment #Patch Set 9 : Android fix attempt #
Messages
Total messages: 45 (30 generated)
The CQ bit was checked by zea@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...
zea@chromium.org changed reviewers: + maxbogue@chromium.org
PTAL
The CQ bit was checked by zea@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 checked by zea@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...
PTAL again, android/test issues should be fixed.
Mostly lg, just a few little comments. https://codereview.chromium.org/2159453002/diff/10005/components/browser_sync... File components/browser_sync/browser/profile_sync_service_startup_unittest.cc (left): https://codereview.chromium.org/2159453002/diff/10005/components/browser_sync... components/browser_sync/browser/profile_sync_service_startup_unittest.cc:439: EXPECT_TRUE(sync_service_->IsBackendInitialized()); Leave as EXPECT_FALSE? https://codereview.chromium.org/2159453002/diff/10005/components/sync_driver/... File components/sync_driver/startup_controller.cc (right): https://codereview.chromium.org/2159453002/diff/10005/components/sync_driver/... components/sync_driver/startup_controller.cc:135: else nit: this is somewhat personal style but I very much prefer brackets with a complex if statement (has an else clause, etc). https://codereview.chromium.org/2159453002/diff/10005/components/sync_driver/... File components/sync_driver/startup_controller.h (right): https://codereview.chromium.org/2159453002/diff/10005/components/sync_driver/... components/sync_driver/startup_controller.h:34: bool TryStart(bool request_immediate); I think the code will be much more readable if you add a second method like TryStartImmediately() instead of a boolean parameter. It would just set received_start_request_ and then call TryStart().
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_...)
Patchset #5 (id:70001) has been deleted
The CQ bit was checked by zea@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...
PTAL. Note that I had to make a slight change to the logic, so that IsFirstSetupCompleted is no longer manually set to true until after backend initialization. There were pieces of logic dependent on its value in Android/CrOS tests, and with the TryStartImmediately method I have better control over things. https://codereview.chromium.org/2159453002/diff/10005/components/browser_sync... File components/browser_sync/browser/profile_sync_service_startup_unittest.cc (left): https://codereview.chromium.org/2159453002/diff/10005/components/browser_sync... components/browser_sync/browser/profile_sync_service_startup_unittest.cc:439: EXPECT_TRUE(sync_service_->IsBackendInitialized()); On 2016/07/15 21:44:48, maxbogue wrote: > Leave as EXPECT_FALSE? Done. https://codereview.chromium.org/2159453002/diff/10005/components/sync_driver/... File components/sync_driver/startup_controller.cc (right): https://codereview.chromium.org/2159453002/diff/10005/components/sync_driver/... components/sync_driver/startup_controller.cc:135: else On 2016/07/15 21:44:48, maxbogue wrote: > nit: this is somewhat personal style but I very much prefer brackets with a > complex if statement (has an else clause, etc). Consistency within the file is important in this case. That said, I've replaced this with a ternary instead. https://codereview.chromium.org/2159453002/diff/10005/components/sync_driver/... File components/sync_driver/startup_controller.h (right): https://codereview.chromium.org/2159453002/diff/10005/components/sync_driver/... components/sync_driver/startup_controller.h:34: bool TryStart(bool request_immediate); On 2016/07/15 21:44:48, maxbogue wrote: > I think the code will be much more readable if you add a second method like > TryStartImmediately() instead of a boolean parameter. It would just set > received_start_request_ and then call TryStart(). Good call, done.
The CQ bit was checked by zea@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 checked by zea@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...
lgtm with one nit! https://codereview.chromium.org/2159453002/diff/110001/components/sync_driver... File components/sync_driver/startup_controller_unittest.cc (right): https://codereview.chromium.org/2159453002/diff/110001/components/sync_driver... components/sync_driver/startup_controller_unittest.cc:194: sync_prefs()->SetFirstSetupComplete(); Remove this line. The test needs to pass without it, right?
https://codereview.chromium.org/2159453002/diff/110001/components/sync_driver... File components/sync_driver/startup_controller_unittest.cc (right): https://codereview.chromium.org/2159453002/diff/110001/components/sync_driver... components/sync_driver/startup_controller_unittest.cc:194: sync_prefs()->SetFirstSetupComplete(); On 2016/07/15 23:45:15, maxbogue wrote: > Remove this line. The test needs to pass without it, right? Good catch! done.
The CQ bit was checked by zea@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from maxbogue@chromium.org Link to the patchset: https://codereview.chromium.org/2159453002/#ps140001 (title: "Address comment")
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_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by zea@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_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by zea@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_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by zea@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: This issue passed the CQ dry run.
The CQ bit was checked by zea@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from maxbogue@chromium.org Link to the patchset: https://codereview.chromium.org/2159453002/#ps160001 (title: "Android fix attempt")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== [Sync] Don't start up sync when FirstSetupCompleted is false and no setup in progress Instead of starting up the backend, we now don't even start it up if the first setup completed flag is false, unless a setup is in progress. On auto start platforms, first setup completed is manually set to true as part of init. BUG=624915 ========== to ========== [Sync] Don't start up sync when FirstSetupCompleted is false and no setup in progress Instead of starting up the backend, we now don't even start it up if the first setup completed flag is false, unless a setup is in progress. On auto start platforms, first setup completed is manually set to true as part of init. BUG=624915 Committed: https://crrev.com/d1de1f77e44072018f41b1f54ffca27a7069edb4 Cr-Commit-Position: refs/heads/master@{#405950} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/d1de1f77e44072018f41b1f54ffca27a7069edb4 Cr-Commit-Position: refs/heads/master@{#405950} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
