New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 839834 link

Starred by 3 users

Issue metadata

Status: Fixed
Owner:
Closed: Sep 5
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , Chrome , Mac
Pri: 2
Type: Task

Blocked on:
issue 856081
issue 854978

Blocking:
issue 840703
issue 856179



Sign in to add a comment

Cleanup ProfileSyncService's exposed state

Project Member Reported by treib@chromium.org, May 4 2018

Issue description

ProfileSyncService currently exposes an unreasonable number of state getters:

General state from SyncService:
- IsFirstSetupComplete
- IsSyncAllowed
- IsSyncActive
- IsLocalSyncEnabled
- CanSyncStart
- IsFirstSetupInProgress
- IsSetupInProgress
- ConfigurationDone
- GetAuthError
- HasUnrecoverableError
- IsEngineInitialized
and added by ProfileSyncService:
- IsSyncRequested
- IsSyncAllowedByFlag
- IsSyncAllowedByPlatform
- IsSyncConfirmationNeeded
- IsManaged
- configure_status
- waiting_for_auth

Additionally there's a bunch of "status" things (mostly?) for display in the UI:
- GetSyncTokenStatus
- QuerySyncStatusSummaryString
- QueryDetailedSyncStatus
- QuerySyncStatusSummary
- GetTypeStatusMap

And some encryption/passphrase-related things:
(SyncService)
- IsPassphraseRequired
- IsPassphraseRequiredForDecryption
- IsUsingSecondaryPassphrase
- IsEncryptEverythingEnabled
- IsCryptographerReady
(ProfileSyncService)
- GetPassphraseType
- IsEncryptEverythingAllowed

And some datatype-related things:
- GetActiveDataTypes
- GetPreferredDataTypes
- GetEncryptedDataTypes
- GetForcedDataTypes
- GetRegisteredDataTypes
- data_type_status_table


Doc with more details and proposal: go/pss-state-cleanup
 
I wonder how we want to approach this beast best. Should we try to better hide those things first, or see if we can better split out responsibilities of this class first and then re-think exposure of internals?

My gut feeling is that if we can move parts into dedicated classes and dependency inject those into the ProfileSyncService, we could reduce the exposure and still support testability.

Comment 2 by treib@chromium.org, May 7 2018

Initial survey:

General state: This is the main place where I'd like to clean things up. I hope that many of these can be merged into an enum. Some can maybe be removed completely. More investigation is required to figure out the actual state machine.

Status things for UI: I think these are more or less okay, if we clearly label them as "for UI". GetSyncTokenStatus has one non-UI caller [1] which should probably be fixed.

Encryption/passphrase things: I don't know much about this yet. Crypto stuff is already partially extracted into SyncServiceCrypto, maybe these getters should all move there.

Datatype-related things: I think these are mostly fine; the only problem is that the semantics of "active data types" aren't really clear.

[1] https://cs.chromium.org/chromium/src/components/ukm/observers/sync_disable_observer.cc?rcl=84578181124fa6dabd79d44a6ca369b7fce68a19&l=56

Comment 3 by treib@chromium.org, May 7 2018

Description: Show this description
Project Member

Comment 4 by bugdroid1@chromium.org, May 7 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d5e946f74972c091816b35c095af947bfe9e950c

commit d5e946f74972c091816b35c095af947bfe9e950c
Author: Marc Treib <treib@chromium.org>
Date: Mon May 07 10:12:07 2018

ProfileSyncService: clean up unused "configure_status"

It's used within OnConfigureDone which receives it as a param, but no
need to store and expose it.

Bug:  839834 
Change-Id: I553e7b8f8b4cbab97c3c00d21e609f0e05d8dff9
Reviewed-on: https://chromium-review.googlesource.com/1044223
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Commit-Queue: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#556400}
[modify] https://crrev.com/d5e946f74972c091816b35c095af947bfe9e950c/components/browser_sync/profile_sync_service.cc
[modify] https://crrev.com/d5e946f74972c091816b35c095af947bfe9e950c/components/browser_sync/profile_sync_service.h

Project Member

Comment 5 by bugdroid1@chromium.org, May 7 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e62da37c09bc3d0c36bf04e9f011fdeeee011149

commit e62da37c09bc3d0c36bf04e9f011fdeeee011149
Author: Marc Treib <treib@chromium.org>
Date: Mon May 07 11:06:27 2018

Cleanup: Remove ProfileSyncService::GetModelSafeRoutingInfo

It's unused.

Bug:  839834 
Change-Id: Ic354b16a5534aab8ce89c036b8a81a2ceeb3ae60
Reviewed-on: https://chromium-review.googlesource.com/1046672
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Commit-Queue: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#556412}
[modify] https://crrev.com/e62da37c09bc3d0c36bf04e9f011fdeeee011149/components/browser_sync/profile_sync_service.cc
[modify] https://crrev.com/e62da37c09bc3d0c36bf04e9f011fdeeee011149/components/browser_sync/profile_sync_service.h

Comment 6 by treib@chromium.org, May 7 2018

Owner: treib@chromium.org
Status: Started (was: Available)

Comment 7 by treib@chromium.org, May 8 2018

Description: Show this description
Project Member

Comment 8 by bugdroid1@chromium.org, May 14 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/03a4c420ebe66e7bcfcb9d77ac22c37068cdb54f

commit 03a4c420ebe66e7bcfcb9d77ac22c37068cdb54f
Author: Marc Treib <treib@chromium.org>
Date: Mon May 14 10:37:46 2018

ProfileSyncService: small cleanups

Making (Un)RegisterAuthNotifications private, and merging
QuerySyncStatusSummary into *String and removing the enum.

Bug:  839834 
Change-Id: Id27dd36468b7893817569982c48653e2a817c5e6
Reviewed-on: https://chromium-review.googlesource.com/1055570
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Commit-Queue: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#558252}
[modify] https://crrev.com/03a4c420ebe66e7bcfcb9d77ac22c37068cdb54f/components/browser_sync/profile_sync_service.cc
[modify] https://crrev.com/03a4c420ebe66e7bcfcb9d77ac22c37068cdb54f/components/browser_sync/profile_sync_service.h

Project Member

Comment 10 by bugdroid1@chromium.org, May 17 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/acf95eaee41fa997ac34cd541cccad474d64efab

commit acf95eaee41fa997ac34cd541cccad474d64efab
Author: Marc Treib <treib@chromium.org>
Date: Thu May 17 08:01:34 2018

Remove ProfileSyncService::GetDataTypeControllerStates

It was only used in some tests, none of which actually cared about the
controller states, only about the set of registered types.

Bug:  839834 
Cq-Include-Trybots: master.tryserver.chromium.mac:ios-simulator-cronet;master.tryserver.chromium.mac:ios-simulator-full-configs
Change-Id: Ia4f69ad3abfeeefc8bad9fc9124dfbd195e6725b
Reviewed-on: https://chromium-review.googlesource.com/1061471
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Commit-Queue: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#559449}
[modify] https://crrev.com/acf95eaee41fa997ac34cd541cccad474d64efab/chrome/browser/sync/profile_sync_service_factory_unittest.cc
[modify] https://crrev.com/acf95eaee41fa997ac34cd541cccad474d64efab/components/browser_sync/profile_sync_service.cc
[modify] https://crrev.com/acf95eaee41fa997ac34cd541cccad474d64efab/components/browser_sync/profile_sync_service.h
[modify] https://crrev.com/acf95eaee41fa997ac34cd541cccad474d64efab/components/sync/driver/data_type_controller.h
[modify] https://crrev.com/acf95eaee41fa997ac34cd541cccad474d64efab/ios/chrome/browser/sync/ios_chrome_profile_sync_service_factory_unittest.cc

Cc: vitaliii@chromium.org markusheintz@chromium.org

Comment 12 by treib@chromium.org, Jun 13 2018

Candidates for merging into a "SyncServiceState" enum, roughly in order of "powerfulness":

- IsSyncAllowed
- IsSyncRequested
- CanSyncStart
- IsEngineInitialized
- (something about DataTypeManager state?)
- IsSyncActive

So the enum could be something like:
NOT_ALLOWED
NOT_REQUESTED
NOT_SIGNED_IN (maybe swap with NOT_REQUESTED?)
NOT_INTIALIZED
CONFIGURING
ACTIVE

Comment 13 by treib@chromium.org, Jun 14 2018

Some more thoughts:
- Does IsLocalSyncEnabled fit in there somehow? I guess that'd have to stay as a separate flag.
- What about auth errors? Since we currently don't persist them, only the ACTIVE and (probably?) CONFIGURING states can have an auth error.
- Should CONFIGURING also cover the "initial Sync cycle not done" (GetLastCycleSnapshot is empty) state?

Comment 14 by treib@chromium.org, Jun 14 2018

And one more thing: "setup in progress/complete" state. There are actually two separate "setup" states: *first* setup and (regular) setup which are quite different:

First setup (IsFirstSetupComplete,IsFirstSetupInProgress):
Before first setup is complete, we *can* create and initialize the SyncEngine, but we can *not* configure data types. So that could be a new state between NOT_INITIALIZED and CONFIGURING, or it could just be merged into CONFIGURING.

Regular setup (GetSetupInProgressHandle,IsSetupInProgress):
This can reasonably coexist with at least NOT_INITIALIZED and ACTIVE, maybe also with CONFIGURING. Technically it can coexist with all other states as well, even though that doesn't make a whole lot of sense. So this will probably need to remain as a separate flag.

Comment 15 by treib@chromium.org, Jun 14 2018

Blocking: 840703

Comment 16 by treib@chromium.org, Jun 14 2018

And yet another one: Unrecoverable errors. When encountering one, we destroy the DataTypeManager and the SyncEngine, hence dropping us back into NOT_INITIALIZED. Maybe that should be another state between NOT_SIGNED_IN and NOT_INITIALIZED?

Comment 17 by treib@chromium.org, Jun 14 2018

...aaand another complication: DataTypeManager's state kinda overlaps with CONFIGURING and ACTIVE.

Probably DTM::CONFIGURING and DRM::RETRYING should map to SS::CONFIGURING, DTM::CONFIGURED to SS::ACTIVE. It looks like DTM::STOPPING can never be observed externally (it gets reset to STOP later in the same method). And finally, DTM::STOPPED maps to either SS::NOT_INITIALIZED or SS::UNRECOVERABLE_ERROR.
Project Member

Comment 18 by bugdroid1@chromium.org, Jun 14 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d3cd58e83b6babbe524d6aee17401ed27640277b

commit d3cd58e83b6babbe524d6aee17401ed27640277b
Author: Marc Treib <treib@chromium.org>
Date: Thu Jun 14 14:01:55 2018

Cleanup: Get rid of ProfileSyncService::waiting_for_auth()

Its semantics were a bit weird: It got set on initial sign-in (i.e. not
during a Chrome restart), and cleared again once we updated the auth
error state: Either to an error, if we failed to get an access token or
got an error from the sync server, or to "no error", if we successfully
connected to the sync server.
It had only a single call site, which only used it to suppress auth
errors while waiting_for_auth was true. However, according to the
above, that's an impossible state: Sync can not be in an auth error
state *before* being signed in, and the waiting_for_auth bit is cleared
when actually entering an auth error state.

Bug:  839834 
Change-Id: Iefbf54a93ede54aabb2a244b3e429b5591bb9934
Reviewed-on: https://chromium-review.googlesource.com/1099162
Reviewed-by: Mihai Sardarescu <msarda@chromium.org>
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Commit-Queue: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#567250}
[modify] https://crrev.com/d3cd58e83b6babbe524d6aee17401ed27640277b/chrome/browser/sync/sync_startup_tracker.cc
[modify] https://crrev.com/d3cd58e83b6babbe524d6aee17401ed27640277b/chrome/browser/ui/webui/signin/dice_turn_sync_on_helper_unittest.cc
[modify] https://crrev.com/d3cd58e83b6babbe524d6aee17401ed27640277b/components/browser_sync/profile_sync_service.cc
[modify] https://crrev.com/d3cd58e83b6babbe524d6aee17401ed27640277b/components/browser_sync/profile_sync_service.h
[modify] https://crrev.com/d3cd58e83b6babbe524d6aee17401ed27640277b/components/browser_sync/profile_sync_service_mock.h
[modify] https://crrev.com/d3cd58e83b6babbe524d6aee17401ed27640277b/components/browser_sync/sync_auth_manager.cc
[modify] https://crrev.com/d3cd58e83b6babbe524d6aee17401ed27640277b/components/browser_sync/sync_auth_manager.h

Project Member

Comment 19 by bugdroid1@chromium.org, Jun 15 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7b4ac3ab60dd13dfad0e623a8c979304f5be644b

commit 7b4ac3ab60dd13dfad0e623a8c979304f5be644b
Author: Marc Treib <treib@chromium.org>
Date: Fri Jun 15 09:01:50 2018

Various small cleanups in ProfileSyncService

- Remove HasSyncingEngine: It just checked "engine_ != nullptr", don't
  need a method for that (and most places already checked the member
  variable directly anyway).
- Make IsSyncAllowedByPlatform private.
- Move is_first_time_sync_configure_ to where it makes more sense.
- Simplify a DCHECK - less negation.
- Remove an unnecessary WeakPtr.
- Merge InitializeEngine into StartUpSlowEngineComponents, its only
  call site.

Bug:  839834 
Change-Id: I2bd5a4829a2d7ade3a9acbdfe5c19f7c4f425968
Reviewed-on: https://chromium-review.googlesource.com/1101207
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Commit-Queue: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#567583}
[modify] https://crrev.com/7b4ac3ab60dd13dfad0e623a8c979304f5be644b/components/browser_sync/profile_sync_service.cc
[modify] https://crrev.com/7b4ac3ab60dd13dfad0e623a8c979304f5be644b/components/browser_sync/profile_sync_service.h

Project Member

Comment 20 by bugdroid1@chromium.org, Jun 15 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/71da4dd6c51f3f94acc984ac5aa4c8519b2df50f

commit 71da4dd6c51f3f94acc984ac5aa4c8519b2df50f
Author: Marc Treib <treib@chromium.org>
Date: Fri Jun 15 09:02:49 2018

Cleanup: Remove ProfileSyncService::data_type_status_table()

It was only used in a single test, and only to verify a precondition,
not an actual test expecation.

Bug:  839834 
Change-Id: I1d27c55b1178f765e89a8c4223533bed8ba24933
Reviewed-on: https://chromium-review.googlesource.com/1101210
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Commit-Queue: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#567584}
[modify] https://crrev.com/71da4dd6c51f3f94acc984ac5aa4c8519b2df50f/chrome/browser/sync/test/integration/enable_disable_test.cc
[modify] https://crrev.com/71da4dd6c51f3f94acc984ac5aa4c8519b2df50f/components/browser_sync/profile_sync_service.cc
[modify] https://crrev.com/71da4dd6c51f3f94acc984ac5aa4c8519b2df50f/components/browser_sync/profile_sync_service.h

Project Member

Comment 21 by bugdroid1@chromium.org, Jun 20 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/dfc227d7c28b3bb182f5ae0ab01d689c95f7cd8e

commit dfc227d7c28b3bb182f5ae0ab01d689c95f7cd8e
Author: Marc Treib <treib@chromium.org>
Date: Wed Jun 20 08:27:18 2018

Various small cleanups in ProfileSyncService, again

- Move "enum SyncInitialState" into the .cc.
- Get rid of many rarely or inconsistently used "using syncer::*"s.
- Update some comments.
- Remove a few unnecessary "virtual"s.
- Merge ChangePreferredDataTypes into OnUserChoseDatatypes - all updates
  must go through that.
  - As a consequence, update SyncAwareCounterTests. In particular, don't
    use syncer::HISTORY_DELETE_DIRECTIVES which is not a user-selectable
    type. Instead use syncer::TYPED_URLS, which corresponds to the
    "History" checkbox, and which implies HISTORY_DELETE_DIRECTIVES.

Bug:  839834 
Change-Id: Ifea0178c1f1e1c1546a1f904c1e52d527df9ed7b
Reviewed-on: https://chromium-review.googlesource.com/1106171
Reviewed-by: Christian Dullweber <dullweber@chromium.org>
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Commit-Queue: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#568764}
[modify] https://crrev.com/dfc227d7c28b3bb182f5ae0ab01d689c95f7cd8e/chrome/browser/browsing_data/counters/sync_aware_counter_browsertest.cc
[modify] https://crrev.com/dfc227d7c28b3bb182f5ae0ab01d689c95f7cd8e/chrome/browser/sync/test/integration/profile_sync_service_harness.cc
[modify] https://crrev.com/dfc227d7c28b3bb182f5ae0ab01d689c95f7cd8e/components/browser_sync/profile_sync_service.cc
[modify] https://crrev.com/dfc227d7c28b3bb182f5ae0ab01d689c95f7cd8e/components/browser_sync/profile_sync_service.h
[modify] https://crrev.com/dfc227d7c28b3bb182f5ae0ab01d689c95f7cd8e/components/browser_sync/profile_sync_service_mock.h
[modify] https://crrev.com/dfc227d7c28b3bb182f5ae0ab01d689c95f7cd8e/components/sync/driver/sync_service.h

Project Member

Comment 22 by bugdroid1@chromium.org, Jun 21 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/5d4cadebbcb4e730c34513f052ccb075d2de1172

commit 5d4cadebbcb4e730c34513f052ccb075d2de1172
Author: Marc Treib <treib@chromium.org>
Date: Thu Jun 21 12:19:17 2018

Sync: Clear Sync data on all unrecoverable errors

Previously, we cleared Sync data on most, but not all types of
unrecoverable errors: Engine initialization failure was treated
specially. Per the TODO removed in this CL, there was no reason
for the special treatment anymore.

Bug:  839834 
Change-Id: I3b473c65b837c810e6fe8456defffe6ebe6e53a2
Reviewed-on: https://chromium-review.googlesource.com/1107991
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Commit-Queue: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#569226}
[modify] https://crrev.com/5d4cadebbcb4e730c34513f052ccb075d2de1172/components/browser_sync/profile_sync_service.cc
[modify] https://crrev.com/5d4cadebbcb4e730c34513f052ccb075d2de1172/components/browser_sync/profile_sync_service.h

Project Member

Comment 23 by bugdroid1@chromium.org, Jun 21 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/cbfb84c5ad60e3df695c625c90a4b50a5c4ca051

commit cbfb84c5ad60e3df695c625c90a4b50a5c4ca051
Author: Marc Treib <treib@chromium.org>
Date: Thu Jun 21 15:45:06 2018

Introduce syncer::StartupController::State enum

This adds a State enum to StartupController and exposes it via GetState().
It's not strictly required right now, but
a) it'll help with sanitizing ProfileSyncService's state,
b) it makes the tests a bit nicer, now they can use the enum instead of
   relying on debug strings, and
c) it lets us move GetEngineInitializationStateString from
   StartupController to ProfileSyncService, where it's slightly less
   out of place (bonus: extra DCHECKs for consistent state).

Other semi-related test cleanups: don't call SetFirstSetupComplete from
FakeStartBackend (no idea why that call was there), and don't check for
the kSyncDisableDeferredStartup cmdline flag (if you run the tests with
a custom flag, it's your own fault if they fail).

Bug:  839834 
Change-Id: I16f83be8ee828024329cbce899be4723241323db
Reviewed-on: https://chromium-review.googlesource.com/1109939
Commit-Queue: Marc Treib <treib@chromium.org>
Reviewed-by: Jan Krcal <jkrcal@chromium.org>
Cr-Commit-Position: refs/heads/master@{#569273}
[modify] https://crrev.com/cbfb84c5ad60e3df695c625c90a4b50a5c4ca051/components/browser_sync/profile_sync_service.cc
[modify] https://crrev.com/cbfb84c5ad60e3df695c625c90a4b50a5c4ca051/components/sync/driver/startup_controller.cc
[modify] https://crrev.com/cbfb84c5ad60e3df695c625c90a4b50a5c4ca051/components/sync/driver/startup_controller.h
[modify] https://crrev.com/cbfb84c5ad60e3df695c625c90a4b50a5c4ca051/components/sync/driver/startup_controller_unittest.cc

Comment 24 by treib@chromium.org, Jun 21 2018

Blockedon: 854978

Comment 25 by treib@chromium.org, Jun 25 2018

Blockedon: 856081
Project Member

Comment 26 by bugdroid1@chromium.org, Jun 25 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e11e9c7fcd8d486838ef276b6614f98b47537af4

commit e11e9c7fcd8d486838ef276b6614f98b47537af4
Author: Marc Treib <treib@chromium.org>
Date: Mon Jun 25 13:38:33 2018

about:sync-internals: Add Request Start and Request Stop buttons

Currently, the "Sync is possible but disabled by the user" state is very
hard to reach: On non-Android, the only way is to trigger a "Reset Sync"
from the dashboard (and on non-ChromeOS, that also signs you out, so
there's really no way at all to reach that state).
This CL adds buttons to request start/stop to about:sync-internals, to
make it easy to reach this state for testing.

Bug:  839834 
Change-Id: Ie0757f3c9a7658ac281adbcdf227fc4e6807e6ac
Reviewed-on: https://chromium-review.googlesource.com/1113445
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Commit-Queue: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#570025}
[modify] https://crrev.com/e11e9c7fcd8d486838ef276b6614f98b47537af4/chrome/browser/ui/webui/sync_internals_message_handler.cc
[modify] https://crrev.com/e11e9c7fcd8d486838ef276b6614f98b47537af4/chrome/browser/ui/webui/sync_internals_message_handler.h
[modify] https://crrev.com/e11e9c7fcd8d486838ef276b6614f98b47537af4/components/sync/driver/about_sync_util.cc
[modify] https://crrev.com/e11e9c7fcd8d486838ef276b6614f98b47537af4/components/sync/driver/about_sync_util.h
[modify] https://crrev.com/e11e9c7fcd8d486838ef276b6614f98b47537af4/components/sync/driver/resources/about.html
[modify] https://crrev.com/e11e9c7fcd8d486838ef276b6614f98b47537af4/components/sync/driver/resources/about.js
[modify] https://crrev.com/e11e9c7fcd8d486838ef276b6614f98b47537af4/components/sync/driver/resources/chrome_sync.js

Comment 27 by treib@chromium.org, Jun 25 2018

Description: Show this description
Project Member

Comment 28 by bugdroid1@chromium.org, Jun 26 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/5bcc5f9fbad2782622e41c2321118f6355ede7a3

commit 5bcc5f9fbad2782622e41c2321118f6355ede7a3
Author: Marc Treib <treib@chromium.org>
Date: Tue Jun 26 11:50:15 2018

Introduce a SyncService::DisableReason enum/bitmask

This combines (and will eventually replace) several state getters:
- IsSyncAllowedByFlag
- IsSyncAllowedByPlatform (private on ProfileSyncService)
- IsManaged (i.e. enterprise policy)
- IsSyncAllowed (combination of the 3 above)
- IsSignedIn (private on ProfileSyncService) / GetAuthenticatedAccountInfo().empty()
- IsSyncRequested
- HasUnrecoverableError

Bug:  839834 
Change-Id: I415cb3e64d639553f6815c1736f4a4bd87e056a8
Reviewed-on: https://chromium-review.googlesource.com/1113746
Commit-Queue: Marc Treib <treib@chromium.org>
Reviewed-by: Jan Krcal <jkrcal@chromium.org>
Cr-Commit-Position: refs/heads/master@{#570376}
[modify] https://crrev.com/5bcc5f9fbad2782622e41c2321118f6355ede7a3/components/browser_sync/profile_sync_service.cc
[modify] https://crrev.com/5bcc5f9fbad2782622e41c2321118f6355ede7a3/components/browser_sync/profile_sync_service.h
[modify] https://crrev.com/5bcc5f9fbad2782622e41c2321118f6355ede7a3/components/browser_sync/profile_sync_service_startup_unittest.cc
[modify] https://crrev.com/5bcc5f9fbad2782622e41c2321118f6355ede7a3/components/browser_sync/profile_sync_service_unittest.cc
[modify] https://crrev.com/5bcc5f9fbad2782622e41c2321118f6355ede7a3/components/sync/driver/fake_sync_service.cc
[modify] https://crrev.com/5bcc5f9fbad2782622e41c2321118f6355ede7a3/components/sync/driver/fake_sync_service.h
[modify] https://crrev.com/5bcc5f9fbad2782622e41c2321118f6355ede7a3/components/sync/driver/sync_service.h

Project Member

Comment 29 by bugdroid1@chromium.org, Jun 27 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1934e528a3915248923ff711fea37d53b28733b6

commit 1934e528a3915248923ff711fea37d53b28733b6
Author: Marc Treib <treib@chromium.org>
Date: Wed Jun 27 14:58:17 2018

ProfileSyncService: don't use deprecated state getters

IsSyncAllowed, IsSyncRequested and HasUnrecoverableError have all been
replaced by GetDisableReasons/HasDisableReason, so use those instead.

Bug:  839834 
Change-Id: Id4939bd4de6a43ae4bfd5c64b6aa79be51a82ebf
Reviewed-on: https://chromium-review.googlesource.com/1116704
Commit-Queue: Marc Treib <treib@chromium.org>
Reviewed-by: Jan Krcal <jkrcal@chromium.org>
Cr-Commit-Position: refs/heads/master@{#570772}
[modify] https://crrev.com/1934e528a3915248923ff711fea37d53b28733b6/components/browser_sync/profile_sync_service.cc
[modify] https://crrev.com/1934e528a3915248923ff711fea37d53b28733b6/components/sync/driver/sync_service.h

Project Member

Comment 30 by bugdroid1@chromium.org, Jun 28 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c3b3eeff9e8f8b189471ed3dc8ab7d1701987a78

commit c3b3eeff9e8f8b189471ed3dc8ab7d1701987a78
Author: Marc Treib <treib@chromium.org>
Date: Thu Jun 28 09:46:39 2018

Remove ProfileSyncService::IsManaged

https://crrev.com/c/1113746 introduced a GetDisableReasons which
replaces it. This CL updates all callers to use the new
GetDisableReasons instead.

Bug:  839834 
Change-Id: If832ffb12f24308e08890a7c073d5827c0ef44a7
Reviewed-on: https://chromium-review.googlesource.com/1114959
Commit-Queue: Marc Treib <treib@chromium.org>
Reviewed-by: Steven Bennetts <stevenjb@chromium.org>
Reviewed-by: Jacob Dufault <jdufault@chromium.org>
Reviewed-by: Jan Krcal <jkrcal@chromium.org>
Cr-Commit-Position: refs/heads/master@{#571059}
[modify] https://crrev.com/c3b3eeff9e8f8b189471ed3dc8ab7d1701987a78/chrome/browser/chromeos/login/screens/sync_consent_screen.cc
[modify] https://crrev.com/c3b3eeff9e8f8b189471ed3dc8ab7d1701987a78/chrome/browser/sync/sync_ui_util.cc
[modify] https://crrev.com/c3b3eeff9e8f8b189471ed3dc8ab7d1701987a78/chrome/browser/ui/webui/settings/people_handler.cc
[modify] https://crrev.com/c3b3eeff9e8f8b189471ed3dc8ab7d1701987a78/components/browser_sync/profile_sync_service.cc
[modify] https://crrev.com/c3b3eeff9e8f8b189471ed3dc8ab7d1701987a78/components/browser_sync/profile_sync_service.h
[modify] https://crrev.com/c3b3eeff9e8f8b189471ed3dc8ab7d1701987a78/components/browser_sync/profile_sync_service_startup_unittest.cc
[modify] https://crrev.com/c3b3eeff9e8f8b189471ed3dc8ab7d1701987a78/components/browser_sync/profile_sync_service_unittest.cc

Project Member

Comment 31 by bugdroid1@chromium.org, Jun 28 2018

Project Member

Comment 32 by bugdroid1@chromium.org, Jun 28 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/86462a946d6f9d093579ec2a5f3cd46f432aee8e

commit 86462a946d6f9d093579ec2a5f3cd46f432aee8e
Author: Marc Treib <treib@chromium.org>
Date: Thu Jun 28 14:24:30 2018

Introduce a SyncService::State enum to replace many individual state bits

The new State encompasses (and should eventually largely replace) the
following state getters:
- CanSyncStart
- IsEngineInitialized
- GetAuthError
- IsFirstSetupComplete
- HasUnrecoverableError
- IsSyncActive
- ConfigurationDone

Other SyncService state that it *not* packed into the enum:
- Setup in progress
- Encryption
- Anything that's per-ModelType
- Local Sync

Bug:  839834 
Change-Id: Iec1c3d744ffcb33fceccfe0bb0d028cc17203655
Reviewed-on: https://chromium-review.googlesource.com/1102331
Commit-Queue: Marc Treib <treib@chromium.org>
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#571110}
[modify] https://crrev.com/86462a946d6f9d093579ec2a5f3cd46f432aee8e/components/browser_sync/profile_sync_service.cc
[modify] https://crrev.com/86462a946d6f9d093579ec2a5f3cd46f432aee8e/components/browser_sync/profile_sync_service.h
[modify] https://crrev.com/86462a946d6f9d093579ec2a5f3cd46f432aee8e/components/browser_sync/profile_sync_service_startup_unittest.cc
[modify] https://crrev.com/86462a946d6f9d093579ec2a5f3cd46f432aee8e/components/browser_sync/profile_sync_service_unittest.cc
[modify] https://crrev.com/86462a946d6f9d093579ec2a5f3cd46f432aee8e/components/sync/driver/fake_sync_service.cc
[modify] https://crrev.com/86462a946d6f9d093579ec2a5f3cd46f432aee8e/components/sync/driver/fake_sync_service.h
[modify] https://crrev.com/86462a946d6f9d093579ec2a5f3cd46f432aee8e/components/sync/driver/sync_service.h

Project Member

Comment 33 by bugdroid1@chromium.org, Jul 3

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/37bb67e8e98c03fa774dea4d8c8fd9548cd1b554

commit 37bb67e8e98c03fa774dea4d8c8fd9548cd1b554
Author: Marc Treib <treib@chromium.org>
Date: Tue Jul 03 11:18:38 2018

Add tests for the full ProfileSyncService startup sequence

This adds tests FullStartupSequenceFirstTime and
FullStartupSequenceNthTime, based on the new SyncService::State enum.
To be able to properly test the full flow, this also introduces a
MockSyncEngine (previous tests use a Fake, which bypasses some steps).

Bug:  839834 
Change-Id: I145becf840f476d73815526b8abd43a9bfa159b8
Reviewed-on: https://chromium-review.googlesource.com/1118383
Commit-Queue: Marc Treib <treib@chromium.org>
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#572170}
[modify] https://crrev.com/37bb67e8e98c03fa774dea4d8c8fd9548cd1b554/components/browser_sync/profile_sync_service_startup_unittest.cc
[modify] https://crrev.com/37bb67e8e98c03fa774dea4d8c8fd9548cd1b554/components/sync/BUILD.gn
[add] https://crrev.com/37bb67e8e98c03fa774dea4d8c8fd9548cd1b554/components/sync/engine/mock_sync_engine.cc
[add] https://crrev.com/37bb67e8e98c03fa774dea4d8c8fd9548cd1b554/components/sync/engine/mock_sync_engine.h

Project Member

Comment 34 by bugdroid1@chromium.org, Jul 3

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/5e4aae833c2dc4e398896ced1cefd65eac7bd4b4

commit 5e4aae833c2dc4e398896ced1cefd65eac7bd4b4
Author: Marc Treib <treib@chromium.org>
Date: Tue Jul 03 11:43:37 2018

Show SyncService::State and DisableReasons in about:sync-internals

This replaces SyncService::QuerySyncStatusSummaryString (which didn't
really make a whole lot of sense in the first place) and
SyncService::GetEngineInitializationStateString (which is now simply
a subset of the new State).

Bug:  839834 
Change-Id: I9e3d9785a9bbafe5b8a7397b4a7c6c97acf9adf9
Reviewed-on: https://chromium-review.googlesource.com/1118223
Commit-Queue: Marc Treib <treib@chromium.org>
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#572172}
[modify] https://crrev.com/5e4aae833c2dc4e398896ced1cefd65eac7bd4b4/chrome/browser/ui/webui/sync_internals_browsertest.js
[modify] https://crrev.com/5e4aae833c2dc4e398896ced1cefd65eac7bd4b4/components/browser_sync/profile_sync_service.cc
[modify] https://crrev.com/5e4aae833c2dc4e398896ced1cefd65eac7bd4b4/components/browser_sync/profile_sync_service.h
[modify] https://crrev.com/5e4aae833c2dc4e398896ced1cefd65eac7bd4b4/components/sync/driver/about_sync_util.cc
[modify] https://crrev.com/5e4aae833c2dc4e398896ced1cefd65eac7bd4b4/components/sync/driver/about_sync_util_unittest.cc
[modify] https://crrev.com/5e4aae833c2dc4e398896ced1cefd65eac7bd4b4/components/sync/driver/fake_sync_service.cc
[modify] https://crrev.com/5e4aae833c2dc4e398896ced1cefd65eac7bd4b4/components/sync/driver/fake_sync_service.h
[modify] https://crrev.com/5e4aae833c2dc4e398896ced1cefd65eac7bd4b4/components/sync/driver/sync_service.h

Project Member

Comment 35 by bugdroid1@chromium.org, Jul 5

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7f62c56e241189ff75333cc2a3e682e40942303f

commit 7f62c56e241189ff75333cc2a3e682e40942303f
Author: Marc Treib <treib@chromium.org>
Date: Thu Jul 05 12:17:12 2018

SyncService::State: prioritize auth errors over any non-disabled state

Before this CL, State::AUTH_ERROR implied that the SyncEngine was at
least initialized. As it turns out, that doesn't quite make sense,
since some kinds of auth errors can occur independently of Sync being
active.
This CL increases the priority of auth errors when determining the
state: Now an auth error will override any other non-disabled state.
That means while an auth error exists, clients can not determine the
"initialization level" of Sync, which might or might not turn out to be
a problem.

This CL also adds a few TODOs to clean up the underlying handling of
auth errors.

Bug:  839834 
Change-Id: Ie4be659ecca913cb8301dc81d1094bf9fdc58bfe
Reviewed-on: https://chromium-review.googlesource.com/1126117
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Commit-Queue: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#572765}
[modify] https://crrev.com/7f62c56e241189ff75333cc2a3e682e40942303f/components/browser_sync/profile_sync_service.cc
[modify] https://crrev.com/7f62c56e241189ff75333cc2a3e682e40942303f/components/browser_sync/sync_auth_manager.cc
[modify] https://crrev.com/7f62c56e241189ff75333cc2a3e682e40942303f/components/browser_sync/sync_auth_manager.h
[modify] https://crrev.com/7f62c56e241189ff75333cc2a3e682e40942303f/components/sync/driver/fake_sync_service.cc
[modify] https://crrev.com/7f62c56e241189ff75333cc2a3e682e40942303f/components/sync/driver/sync_service.h

Project Member

Comment 36 by bugdroid1@chromium.org, Jul 7

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/db1e1a0fd12a32148d7a6bdfe144e555963e5c54

commit db1e1a0fd12a32148d7a6bdfe144e555963e5c54
Author: Marc Treib <treib@chromium.org>
Date: Sat Jul 07 15:30:48 2018

Move deprecated getters from ProfileSyncService to SyncService

This CL moves the implementations of CanSyncStart, IsSyncAllowed, and
HasUnrecoverableError from ProfileSyncService to the base class
SyncService.
These deprecated methods are implemented in terms of their
replacements. Moving them into the base class makes sure that they
can't be overridden by test/fake implementations, and thus ensures that
all test implementations of SyncService expose consistent state.

Bug:  839834 
Cq-Include-Trybots: luci.chromium.try:ios-simulator-full-configs;master.tryserver.chromium.mac:ios-simulator-cronet
Change-Id: I33667691c36f46fd3b0cc182893e8c65d2df408e
Reviewed-on: https://chromium-review.googlesource.com/1120247
Reviewed-by: Mihai Sardarescu <msarda@chromium.org>
Reviewed-by: Peter Lee <pkl@chromium.org>
Reviewed-by: Istiaque Ahmed <lazyboy@chromium.org>
Reviewed-by: Hector Carmona <hcarmona@chromium.org>
Reviewed-by: Vaclav Brozek <vabr@chromium.org>
Reviewed-by: Jan Krcal <jkrcal@chromium.org>
Commit-Queue: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#573165}
[modify] https://crrev.com/db1e1a0fd12a32148d7a6bdfe144e555963e5c54/chrome/browser/extensions/external_pref_loader_unittest.cc
[modify] https://crrev.com/db1e1a0fd12a32148d7a6bdfe144e555963e5c54/chrome/browser/sync/sync_startup_tracker_unittest.cc
[modify] https://crrev.com/db1e1a0fd12a32148d7a6bdfe144e555963e5c54/chrome/browser/sync/sync_ui_util_unittest.cc
[modify] https://crrev.com/db1e1a0fd12a32148d7a6bdfe144e555963e5c54/chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_util_unittest.cc
[modify] https://crrev.com/db1e1a0fd12a32148d7a6bdfe144e555963e5c54/chrome/browser/ui/passwords/manage_passwords_bubble_model_unittest.cc
[modify] https://crrev.com/db1e1a0fd12a32148d7a6bdfe144e555963e5c54/chrome/browser/ui/webui/settings/people_handler_unittest.cc
[modify] https://crrev.com/db1e1a0fd12a32148d7a6bdfe144e555963e5c54/chrome/browser/ui/webui/signin/dice_turn_sync_on_helper_unittest.cc
[modify] https://crrev.com/db1e1a0fd12a32148d7a6bdfe144e555963e5c54/components/autofill/core/browser/autofill_experiments_unittest.cc
[modify] https://crrev.com/db1e1a0fd12a32148d7a6bdfe144e555963e5c54/components/autofill/core/browser/test_sync_service.cc
[modify] https://crrev.com/db1e1a0fd12a32148d7a6bdfe144e555963e5c54/components/autofill/core/browser/test_sync_service.h
[modify] https://crrev.com/db1e1a0fd12a32148d7a6bdfe144e555963e5c54/components/browser_sync/profile_sync_service.cc
[modify] https://crrev.com/db1e1a0fd12a32148d7a6bdfe144e555963e5c54/components/browser_sync/profile_sync_service.h
[modify] https://crrev.com/db1e1a0fd12a32148d7a6bdfe144e555963e5c54/components/browser_sync/profile_sync_service_mock.h
[modify] https://crrev.com/db1e1a0fd12a32148d7a6bdfe144e555963e5c54/components/ntp_snippets/remote/test_utils.cc
[modify] https://crrev.com/db1e1a0fd12a32148d7a6bdfe144e555963e5c54/components/ntp_snippets/remote/test_utils.h
[modify] https://crrev.com/db1e1a0fd12a32148d7a6bdfe144e555963e5c54/components/password_manager/core/browser/password_bubble_experiment_unittest.cc
[modify] https://crrev.com/db1e1a0fd12a32148d7a6bdfe144e555963e5c54/components/suggestions/suggestions_service_impl_unittest.cc
[modify] https://crrev.com/db1e1a0fd12a32148d7a6bdfe144e555963e5c54/components/sync/driver/about_sync_util_unittest.cc
[modify] https://crrev.com/db1e1a0fd12a32148d7a6bdfe144e555963e5c54/components/sync/driver/fake_sync_service.cc
[modify] https://crrev.com/db1e1a0fd12a32148d7a6bdfe144e555963e5c54/components/sync/driver/fake_sync_service.h
[modify] https://crrev.com/db1e1a0fd12a32148d7a6bdfe144e555963e5c54/components/sync/driver/sync_service.cc
[modify] https://crrev.com/db1e1a0fd12a32148d7a6bdfe144e555963e5c54/components/sync/driver/sync_service.h
[modify] https://crrev.com/db1e1a0fd12a32148d7a6bdfe144e555963e5c54/components/sync/driver/sync_service_utils_unittest.cc
[modify] https://crrev.com/db1e1a0fd12a32148d7a6bdfe144e555963e5c54/components/unified_consent/unified_consent_service_unittest.cc
[modify] https://crrev.com/db1e1a0fd12a32148d7a6bdfe144e555963e5c54/components/unified_consent/url_keyed_data_collection_consent_helper_unittest.cc
[modify] https://crrev.com/db1e1a0fd12a32148d7a6bdfe144e555963e5c54/ios/chrome/browser/signin/authentication_service_unittest.mm

Project Member

Comment 37 by bugdroid1@chromium.org, Jul 9

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/24db2e5db7a326b0abb03ffb260e63cabba2bed2

commit 24db2e5db7a326b0abb03ffb260e63cabba2bed2
Author: Marc Treib <treib@chromium.org>
Date: Mon Jul 09 16:16:16 2018

Sync: Implement GetUploadToGoogleState in terms of GetState

The new GetState replaces calls to CanSyncStart, IsSyncActive, and
ConfigurationDone. (Unfortunately GetAuthError is still required to
distinguish transient from persistent errors.)

One change in behavior is that transient auth errors now map to
INITIALIZING instead of ACTIVE. That's because with GetState, we can't
tell if sync was actually active before the auth error happened. But
this change seems like an improvement anyway.

Bug:  839834 , 856081
Change-Id: I2ccee268ffe226817289950f4db5c7f98ea69487
Reviewed-on: https://chromium-review.googlesource.com/1124567
Reviewed-by: Thomas Tangl <tangltom@chromium.org>
Reviewed-by: Sebastien Seguin-Gagnon <sebsg@chromium.org>
Reviewed-by: Istiaque Ahmed <lazyboy@chromium.org>
Reviewed-by: Jan Krcal <jkrcal@chromium.org>
Commit-Queue: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#573325}
[modify] https://crrev.com/24db2e5db7a326b0abb03ffb260e63cabba2bed2/chrome/browser/extensions/api/sessions/sessions_apitest.cc
[modify] https://crrev.com/24db2e5db7a326b0abb03ffb260e63cabba2bed2/components/autofill/core/browser/test_sync_service.cc
[modify] https://crrev.com/24db2e5db7a326b0abb03ffb260e63cabba2bed2/components/autofill/core/browser/test_sync_service.h
[modify] https://crrev.com/24db2e5db7a326b0abb03ffb260e63cabba2bed2/components/suggestions/suggestions_service_impl_unittest.cc
[modify] https://crrev.com/24db2e5db7a326b0abb03ffb260e63cabba2bed2/components/sync/driver/sync_service_utils.cc
[modify] https://crrev.com/24db2e5db7a326b0abb03ffb260e63cabba2bed2/components/sync/driver/sync_service_utils.h
[modify] https://crrev.com/24db2e5db7a326b0abb03ffb260e63cabba2bed2/components/sync/driver/sync_service_utils_unittest.cc
[modify] https://crrev.com/24db2e5db7a326b0abb03ffb260e63cabba2bed2/components/unified_consent/url_keyed_data_collection_consent_helper_unittest.cc

Project Member

Comment 38 by bugdroid1@chromium.org, Jul 10

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7b6190a2ffe3d9376f98f3f5a0f88673c2232c4a

commit 7b6190a2ffe3d9376f98f3f5a0f88673c2232c4a
Author: Marc Treib <treib@chromium.org>
Date: Tue Jul 10 11:34:41 2018

ProfileSyncService: generally prefer StopImpl over RequestStop

RequestStop calls StopImpl, but also sets IsSyncRequested to false. In
many of the places where RequestStop was called from within
ProfileSyncService, that doesn't make sense. E.g. when the user signs
out, Sync can't start up again anyway, no reason to also clear the
IsSyncRequested flag (it'll get set again during the next Sync setup
anyway).
This makes GetDisableReasons a bit more sane/predictable, since
DISABLE_REASON_USER_CHOICE doesn't get set arbitrarily.

Bug:  839834 
Change-Id: I750e697612875b4b6c747922d8c4d5ac5c7a6968
Reviewed-on: https://chromium-review.googlesource.com/1127041
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Commit-Queue: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#573684}
[modify] https://crrev.com/7b6190a2ffe3d9376f98f3f5a0f88673c2232c4a/components/browser_sync/profile_sync_service.cc
[modify] https://crrev.com/7b6190a2ffe3d9376f98f3f5a0f88673c2232c4a/components/browser_sync/profile_sync_service_unittest.cc

Project Member

Comment 39 by bugdroid1@chromium.org, Jul 10

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/160a13834b8c18ee69ed0555e1214b0278d6e99c

commit 160a13834b8c18ee69ed0555e1214b0278d6e99c
Author: Marc Treib <treib@chromium.org>
Date: Tue Jul 10 14:55:42 2018

Cleanup: Get rid of SyncService::ConfigurationDone()

This method has been replaced by GetState(), and it didn't have many
callers to begin with.
One wrinkle is that FakeSyncService now doesn't know about this part of
GetState() anymore, so tests that depend on it have to implement it
themselves. This is only a temporary state of affairs though, until
everything has been fully migrated over to GetState().

TBRing trivial changes (methods moved to base class FakeSyncService)
in components/autofill/core/browser/test_sync_service.h/cc and
components/unified_consent/url_keyed_data_collection_consent_helper_unittest.cc.

TBR=sebsg@chromium.org,tangltom@chromium.org

Bug:  839834 
Change-Id: I3c8dd38537c373f78d89964880033ad04184e371
Reviewed-on: https://chromium-review.googlesource.com/1124698
Reviewed-by: Istiaque Ahmed <lazyboy@chromium.org>
Reviewed-by: Jan Krcal <jkrcal@chromium.org>
Commit-Queue: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#573724}
[modify] https://crrev.com/160a13834b8c18ee69ed0555e1214b0278d6e99c/chrome/browser/chromeos/extensions/wallpaper_private_api.cc
[modify] https://crrev.com/160a13834b8c18ee69ed0555e1214b0278d6e99c/chrome/browser/sync/test/integration/profile_sync_service_harness.cc
[modify] https://crrev.com/160a13834b8c18ee69ed0555e1214b0278d6e99c/chrome/browser/sync/test/integration/quiesce_status_change_checker.cc
[modify] https://crrev.com/160a13834b8c18ee69ed0555e1214b0278d6e99c/components/autofill/core/browser/test_sync_service.cc
[modify] https://crrev.com/160a13834b8c18ee69ed0555e1214b0278d6e99c/components/autofill/core/browser/test_sync_service.h
[modify] https://crrev.com/160a13834b8c18ee69ed0555e1214b0278d6e99c/components/browser_sync/profile_sync_service.cc
[modify] https://crrev.com/160a13834b8c18ee69ed0555e1214b0278d6e99c/components/browser_sync/profile_sync_service.h
[modify] https://crrev.com/160a13834b8c18ee69ed0555e1214b0278d6e99c/components/ntp_snippets/remote/test_utils.cc
[modify] https://crrev.com/160a13834b8c18ee69ed0555e1214b0278d6e99c/components/ntp_snippets/remote/test_utils.h
[modify] https://crrev.com/160a13834b8c18ee69ed0555e1214b0278d6e99c/components/sync/driver/fake_sync_service.cc
[modify] https://crrev.com/160a13834b8c18ee69ed0555e1214b0278d6e99c/components/sync/driver/fake_sync_service.h
[modify] https://crrev.com/160a13834b8c18ee69ed0555e1214b0278d6e99c/components/sync/driver/sync_service.h
[modify] https://crrev.com/160a13834b8c18ee69ed0555e1214b0278d6e99c/components/sync/driver/sync_service_utils_unittest.cc
[modify] https://crrev.com/160a13834b8c18ee69ed0555e1214b0278d6e99c/components/unified_consent/url_keyed_data_collection_consent_helper_unittest.cc

Project Member

Comment 40 by bugdroid1@chromium.org, Jul 16

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/6a2be53da97fad3f1f84e0f6fcd63b00d18831ea

commit 6a2be53da97fad3f1f84e0f6fcd63b00d18831ea
Author: Marc Treib <treib@chromium.org>
Date: Mon Jul 16 18:30:15 2018

SyncService::CanSyncStart: return false after an unrecoverable error

This should make no difference whatsoever in practice (in case of an
unrecoverable error, we won't try to start up again anyway), it just
makes things a bit simpler to reason about.

Bug:  839834 
Change-Id: Ie3ac83b172f9fab100b1a176421025aa1f3699dc
Reviewed-on: https://chromium-review.googlesource.com/1138252
Commit-Queue: Marc Treib <treib@chromium.org>
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#575355}
[modify] https://crrev.com/6a2be53da97fad3f1f84e0f6fcd63b00d18831ea/components/sync/driver/sync_service.cc
[modify] https://crrev.com/6a2be53da97fad3f1f84e0f6fcd63b00d18831ea/components/sync/driver/sync_service.h

Project Member

Comment 41 by bugdroid1@chromium.org, Jul 18

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8c17f12a9b58f74c8d4213942d5b232248052aca

commit 8c17f12a9b58f74c8d4213942d5b232248052aca
Author: Marc Treib <treib@chromium.org>
Date: Wed Jul 18 11:41:23 2018

Remove SyncService::State::AUTH_ERROR

It was kind of an artificial state: All the other states are mutually
exclusive and form a clear hierarchy, but auth errors can coexist with
most of the other states. In that case, we'd "override" the overall
state to be AUTH_ERROR, which made it impossible to check whether, e.g.,
the SyncEngine was initialized.
This CL removes the AUTH_ERROR state - interested clients will have to
call GetAuthError() again, which they probably had to do anyway to check
for transient vs. persistent errors etc.

Bug:  839834 
Change-Id: I829a1495671a34eb783d56dc73519025f9bfed7b
Reviewed-on: https://chromium-review.googlesource.com/1141578
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Commit-Queue: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#576007}
[modify] https://crrev.com/8c17f12a9b58f74c8d4213942d5b232248052aca/components/browser_sync/profile_sync_service.cc
[modify] https://crrev.com/8c17f12a9b58f74c8d4213942d5b232248052aca/components/browser_sync/profile_sync_service_unittest.cc
[modify] https://crrev.com/8c17f12a9b58f74c8d4213942d5b232248052aca/components/sync/driver/about_sync_util.cc
[modify] https://crrev.com/8c17f12a9b58f74c8d4213942d5b232248052aca/components/sync/driver/fake_sync_service.cc
[modify] https://crrev.com/8c17f12a9b58f74c8d4213942d5b232248052aca/components/sync/driver/sync_service.h
[modify] https://crrev.com/8c17f12a9b58f74c8d4213942d5b232248052aca/components/sync/driver/sync_service_utils.cc

Project Member

Comment 42 by bugdroid1@chromium.org, Jul 18

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7be271d784a8bb29e1936073fdc1458064784803

commit 7be271d784a8bb29e1936073fdc1458064784803
Author: Marc Treib <treib@chromium.org>
Date: Wed Jul 18 16:36:25 2018

Replace SyncService::State::WAITING_FOR_CONSENT by PENDING_DESIRED_CONFIGURATION

This makes more sense to describe the actual state of the SyncService (rather
than the reason for it). Presence or absence of consent isn't part of the
SyncService state.
It'll also make it easier to decouple Sync-the-machinery from
Sync-the-feature, since the concept of consent doesn't apply for the
machinery.

In practice, WAITING_FOR_CONSENT and PENDING_DESIRED_CONFIGURATION are almost
equivalent, but they are defined in different terms.

Bug:  839834 , 856179
Change-Id: I5fc20a1359bd302cae233a5bce9b25bda2681e2b
Reviewed-on: https://chromium-review.googlesource.com/1141804
Commit-Queue: Marc Treib <treib@chromium.org>
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#576091}
[modify] https://crrev.com/7be271d784a8bb29e1936073fdc1458064784803/components/browser_sync/profile_sync_service.cc
[modify] https://crrev.com/7be271d784a8bb29e1936073fdc1458064784803/components/browser_sync/profile_sync_service_startup_unittest.cc
[modify] https://crrev.com/7be271d784a8bb29e1936073fdc1458064784803/components/browser_sync/profile_sync_service_unittest.cc
[modify] https://crrev.com/7be271d784a8bb29e1936073fdc1458064784803/components/sync/driver/about_sync_util.cc
[modify] https://crrev.com/7be271d784a8bb29e1936073fdc1458064784803/components/sync/driver/fake_sync_service.cc
[modify] https://crrev.com/7be271d784a8bb29e1936073fdc1458064784803/components/sync/driver/sync_service.h
[modify] https://crrev.com/7be271d784a8bb29e1936073fdc1458064784803/components/sync/driver/sync_service_utils.cc

Project Member

Comment 43 by bugdroid1@chromium.org, Jul 19

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/947024456c41583f641a7923c11e0d389b74c8f4

commit 947024456c41583f641a7923c11e0d389b74c8f4
Author: Marc Treib <treib@chromium.org>
Date: Thu Jul 19 09:04:43 2018

Move IsSyncActive implementation into SyncService

IsSyncActive can now be implemented in terms of GetState (as an
intermediate step before fully removing it). Moving the implementation
into the base class makes sure tests can't expose inconsistent state.

This does require a good number of test changes, since IsSyncActive
can't be overridden anymore. The following ones are entirely trivial/
mechanical and will be TBRed:
chrome/browser/extensions/external_pref_loader_unittest.cc
chrome/browser/ui/cocoa/app_menu/app_menu_controller_unittest.mm
components/autofill/core/browser/test_sync_service.h/cc
components/unified_consent/unified_consent_service_unittest.cc
components/unified_consent/url_keyed_data_collection_consent_helper_unittest.cc

TBR=jochen

Bug:  839834 
Cq-Include-Trybots: luci.chromium.try:ios-simulator-full-configs;master.tryserver.chromium.mac:ios-simulator-cronet
Change-Id: I27c6568330e0377d903eb0548221fc264a0d5dd6
Reviewed-on: https://chromium-review.googlesource.com/1138613
Commit-Queue: Marc Treib <treib@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Reviewed-by: Jochen Eisinger <jochen@chromium.org>
Reviewed-by: edchin <edchin@chromium.org>
Reviewed-by: Michael Wasserman <msw@chromium.org>
Reviewed-by: Martin Šrámek <msramek@chromium.org>
Reviewed-by: Vadym Doroshenko <dvadym@chromium.org>
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#576425}
[modify] https://crrev.com/947024456c41583f641a7923c11e0d389b74c8f4/chrome/browser/extensions/external_pref_loader_unittest.cc
[modify] https://crrev.com/947024456c41583f641a7923c11e0d389b74c8f4/chrome/browser/password_manager/chrome_password_manager_client_unittest.cc
[modify] https://crrev.com/947024456c41583f641a7923c11e0d389b74c8f4/chrome/browser/sync/sync_ui_util_unittest.cc
[modify] https://crrev.com/947024456c41583f641a7923c11e0d389b74c8f4/chrome/browser/ui/cocoa/app_menu/app_menu_controller_unittest.mm
[modify] https://crrev.com/947024456c41583f641a7923c11e0d389b74c8f4/chrome/browser/ui/passwords/manage_passwords_bubble_model_unittest.cc
[modify] https://crrev.com/947024456c41583f641a7923c11e0d389b74c8f4/chrome/browser/ui/sync/one_click_signin_sync_observer_unittest.cc
[modify] https://crrev.com/947024456c41583f641a7923c11e0d389b74c8f4/chrome/browser/ui/toolbar/recent_tabs_sub_menu_model_unittest.cc
[modify] https://crrev.com/947024456c41583f641a7923c11e0d389b74c8f4/chrome/browser/ui/webui/browsing_history_handler_unittest.cc
[modify] https://crrev.com/947024456c41583f641a7923c11e0d389b74c8f4/components/autofill/core/browser/test_sync_service.cc
[modify] https://crrev.com/947024456c41583f641a7923c11e0d389b74c8f4/components/autofill/core/browser/test_sync_service.h
[modify] https://crrev.com/947024456c41583f641a7923c11e0d389b74c8f4/components/browser_sync/profile_sync_service.cc
[modify] https://crrev.com/947024456c41583f641a7923c11e0d389b74c8f4/components/browser_sync/profile_sync_service.h
[modify] https://crrev.com/947024456c41583f641a7923c11e0d389b74c8f4/components/browser_sync/profile_sync_service_mock.h
[modify] https://crrev.com/947024456c41583f641a7923c11e0d389b74c8f4/components/browsing_data/core/history_notice_utils_unittest.cc
[modify] https://crrev.com/947024456c41583f641a7923c11e0d389b74c8f4/components/ntp_snippets/remote/test_utils.cc
[modify] https://crrev.com/947024456c41583f641a7923c11e0d389b74c8f4/components/ntp_snippets/remote/test_utils.h
[modify] https://crrev.com/947024456c41583f641a7923c11e0d389b74c8f4/components/password_manager/core/browser/password_bubble_experiment_unittest.cc
[modify] https://crrev.com/947024456c41583f641a7923c11e0d389b74c8f4/components/suggestions/suggestions_service_impl_unittest.cc
[modify] https://crrev.com/947024456c41583f641a7923c11e0d389b74c8f4/components/sync/driver/fake_sync_service.cc
[modify] https://crrev.com/947024456c41583f641a7923c11e0d389b74c8f4/components/sync/driver/fake_sync_service.h
[modify] https://crrev.com/947024456c41583f641a7923c11e0d389b74c8f4/components/sync/driver/sync_service.cc
[modify] https://crrev.com/947024456c41583f641a7923c11e0d389b74c8f4/components/sync/driver/sync_service.h
[modify] https://crrev.com/947024456c41583f641a7923c11e0d389b74c8f4/components/sync/driver/sync_service_utils_unittest.cc
[modify] https://crrev.com/947024456c41583f641a7923c11e0d389b74c8f4/components/unified_consent/unified_consent_service_unittest.cc
[modify] https://crrev.com/947024456c41583f641a7923c11e0d389b74c8f4/components/unified_consent/url_keyed_data_collection_consent_helper_unittest.cc
[modify] https://crrev.com/947024456c41583f641a7923c11e0d389b74c8f4/ios/chrome/browser/ui/settings/clear_browsing_data_collection_view_controller_unittest.mm
[modify] https://crrev.com/947024456c41583f641a7923c11e0d389b74c8f4/ios/chrome/browser/ui/settings/clear_browsing_data_manager_unittest.mm

Blocking: 856179
Project Member

Comment 45 by bugdroid1@chromium.org, Jul 20

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/eb482432028036f6fc7f035d5889cf6bccf20180

commit eb482432028036f6fc7f035d5889cf6bccf20180
Author: Marc Treib <treib@chromium.org>
Date: Fri Jul 20 08:29:41 2018

Remove IsSyncActive() checks from ProfileSyncService's unit tests

As of https://crrev.com/c/1138613, IsSyncActive() is implemented in
terms of GetState(), in the base class SyncService. All the tests that
checked IsSyncActive() also check GetState(), so the IsSyncActive()
checks are now entirely redundant.

Bug:  839834 
Change-Id: I1f3898a84643f6f8df7eef91dd038b6b6be30822
Reviewed-on: https://chromium-review.googlesource.com/1143397
Reviewed-by: Mohamed Amir Yosef <mamir@chromium.org>
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Commit-Queue: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#576811}
[modify] https://crrev.com/eb482432028036f6fc7f035d5889cf6bccf20180/components/browser_sync/profile_sync_service_startup_unittest.cc
[modify] https://crrev.com/eb482432028036f6fc7f035d5889cf6bccf20180/components/browser_sync/profile_sync_service_unittest.cc

Project Member

Comment 46 by bugdroid1@chromium.org, Jul 20

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2eac56fd3acecd9e53d77c333218f5c59b210790

commit 2eac56fd3acecd9e53d77c333218f5c59b210790
Author: Marc Treib <treib@chromium.org>
Date: Fri Jul 20 13:29:33 2018

Move IsFirstSetupInProgress impl from ProfileSyncService to SyncService

The implementation is based on other publicly-exposed state. Moving it
to the base class makes sure tests can't expose inconsistent state.
In particular, a bunch of tests were updated to override
IsSetupInProgress instead of IsFirstSetupInProgress.

Bug:  839834 
Change-Id: Ie5329970df919ee2b26f793f17da0b68a5bfb998
Reviewed-on: https://chromium-review.googlesource.com/1145190
Reviewed-by: Mohamed Amir Yosef <mamir@chromium.org>
Reviewed-by: Leonard Grey <lgrey@chromium.org>
Commit-Queue: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#576841}
[modify] https://crrev.com/2eac56fd3acecd9e53d77c333218f5c59b210790/chrome/browser/sync/sync_ui_util_unittest.cc
[modify] https://crrev.com/2eac56fd3acecd9e53d77c333218f5c59b210790/chrome/browser/ui/cocoa/profiles/avatar_button_controller_unittest.mm
[modify] https://crrev.com/2eac56fd3acecd9e53d77c333218f5c59b210790/chrome/browser/ui/sync/one_click_signin_sync_observer_unittest.cc
[modify] https://crrev.com/2eac56fd3acecd9e53d77c333218f5c59b210790/components/browser_sync/profile_sync_service.cc
[modify] https://crrev.com/2eac56fd3acecd9e53d77c333218f5c59b210790/components/browser_sync/profile_sync_service.h
[modify] https://crrev.com/2eac56fd3acecd9e53d77c333218f5c59b210790/components/browser_sync/profile_sync_service_mock.h
[modify] https://crrev.com/2eac56fd3acecd9e53d77c333218f5c59b210790/components/sync/driver/fake_sync_service.cc
[modify] https://crrev.com/2eac56fd3acecd9e53d77c333218f5c59b210790/components/sync/driver/fake_sync_service.h
[modify] https://crrev.com/2eac56fd3acecd9e53d77c333218f5c59b210790/components/sync/driver/sync_service.cc
[modify] https://crrev.com/2eac56fd3acecd9e53d77c333218f5c59b210790/components/sync/driver/sync_service.h

Project Member

Comment 47 by bugdroid1@chromium.org, Jul 23

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/90ce10aa5aec77d696f4c3641b043a15690d1423

commit 90ce10aa5aec77d696f4c3641b043a15690d1423
Author: Marc Treib <treib@chromium.org>
Date: Mon Jul 23 07:30:03 2018

Move IsEngineInitialized implementation into SyncService

IsEngineInitialized can now be implemented in terms of GetState (as an
intermediate step before fully removing it). Moving the implementation
into the base class makes sure tests can't expose inconsistent state.

TBR=marq for ios/
who did say LGTM, but forgot to press "+1" :)

Bug:  839834 
Cq-Include-Trybots: luci.chromium.try:ios-simulator-full-configs;master.tryserver.chromium.mac:ios-simulator-cronet
Change-Id: I67b5174d4a0bdb0b286526a6deecfed58d01267f
Reviewed-on: https://chromium-review.googlesource.com/1141844
Commit-Queue: Marc Treib <treib@chromium.org>
Reviewed-by: Sebastien Seguin-Gagnon <sebsg@chromium.org>
Reviewed-by: Thomas Tangl <tangltom@chromium.org>
Reviewed-by: Ilya Sherman <isherman@chromium.org>
Reviewed-by: Scott Chen <scottchen@chromium.org>
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#577119}
[modify] https://crrev.com/90ce10aa5aec77d696f4c3641b043a15690d1423/chrome/browser/sync/sync_startup_tracker_unittest.cc
[modify] https://crrev.com/90ce10aa5aec77d696f4c3641b043a15690d1423/chrome/browser/ui/webui/settings/people_handler_unittest.cc
[modify] https://crrev.com/90ce10aa5aec77d696f4c3641b043a15690d1423/chrome/browser/ui/webui/signin/dice_turn_sync_on_helper_unittest.cc
[modify] https://crrev.com/90ce10aa5aec77d696f4c3641b043a15690d1423/components/autofill/core/browser/test_sync_service.cc
[modify] https://crrev.com/90ce10aa5aec77d696f4c3641b043a15690d1423/components/autofill/core/browser/test_sync_service.h
[modify] https://crrev.com/90ce10aa5aec77d696f4c3641b043a15690d1423/components/browser_sync/profile_sync_service.cc
[modify] https://crrev.com/90ce10aa5aec77d696f4c3641b043a15690d1423/components/browser_sync/profile_sync_service.h
[modify] https://crrev.com/90ce10aa5aec77d696f4c3641b043a15690d1423/components/browser_sync/profile_sync_service_mock.h
[modify] https://crrev.com/90ce10aa5aec77d696f4c3641b043a15690d1423/components/suggestions/suggestions_service_impl_unittest.cc
[modify] https://crrev.com/90ce10aa5aec77d696f4c3641b043a15690d1423/components/sync/driver/fake_sync_service.cc
[modify] https://crrev.com/90ce10aa5aec77d696f4c3641b043a15690d1423/components/sync/driver/fake_sync_service.h
[modify] https://crrev.com/90ce10aa5aec77d696f4c3641b043a15690d1423/components/sync/driver/sync_service.cc
[modify] https://crrev.com/90ce10aa5aec77d696f4c3641b043a15690d1423/components/sync/driver/sync_service.h
[modify] https://crrev.com/90ce10aa5aec77d696f4c3641b043a15690d1423/components/sync/driver/sync_service_utils_unittest.cc
[modify] https://crrev.com/90ce10aa5aec77d696f4c3641b043a15690d1423/components/sync/user_events/user_event_service_impl_unittest.cc
[modify] https://crrev.com/90ce10aa5aec77d696f4c3641b043a15690d1423/components/ukm/observers/sync_disable_observer_unittest.cc
[modify] https://crrev.com/90ce10aa5aec77d696f4c3641b043a15690d1423/components/unified_consent/unified_consent_service_unittest.cc
[modify] https://crrev.com/90ce10aa5aec77d696f4c3641b043a15690d1423/components/unified_consent/url_keyed_data_collection_consent_helper_unittest.cc
[modify] https://crrev.com/90ce10aa5aec77d696f4c3641b043a15690d1423/ios/chrome/browser/ui/ntp/recent_tabs/recent_tabs_table_coordinator_unittest.mm
[modify] https://crrev.com/90ce10aa5aec77d696f4c3641b043a15690d1423/ios/chrome/browser/ui/settings/passphrase_collection_view_controller_test.mm
[modify] https://crrev.com/90ce10aa5aec77d696f4c3641b043a15690d1423/ios/chrome/browser/ui/settings/sync_encryption_collection_view_controller_unittest.mm
[modify] https://crrev.com/90ce10aa5aec77d696f4c3641b043a15690d1423/ios/chrome/browser/ui/settings/sync_settings_collection_view_controller_unittest.mm

Project Member

Comment 48 by bugdroid1@chromium.org, Jul 24

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/5c590c23a0925612eb6b99cbb4cefe2720fa3476

commit 5c590c23a0925612eb6b99cbb4cefe2720fa3476
Author: Marc Treib <treib@chromium.org>
Date: Tue Jul 24 15:14:23 2018

Remove SyncService::IsSyncAllowed

This deprecated method was already implemented in terms of the new
GetDisableReasons. This CL moves the remaining callers over to use
GetDisableReasons directly, and removes IsSyncAllowed.

Bug:  839834 
Change-Id: I41d1a59aff323f0752431a281a432118d437df44
Reviewed-on: https://chromium-review.googlesource.com/1138250
Commit-Queue: Marc Treib <treib@chromium.org>
Reviewed-by: Vasilii Sukhanov <vasilii@chromium.org>
Reviewed-by: Justin Cohen <justincohen@chromium.org>
Reviewed-by: Mihai Sardarescu <msarda@chromium.org>
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#577546}
[modify] https://crrev.com/5c590c23a0925612eb6b99cbb4cefe2720fa3476/chrome/browser/profiles/profile.cc
[modify] https://crrev.com/5c590c23a0925612eb6b99cbb4cefe2720fa3476/chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_util.cc
[modify] https://crrev.com/5c590c23a0925612eb6b99cbb4cefe2720fa3476/components/password_manager/core/browser/password_bubble_experiment.cc
[modify] https://crrev.com/5c590c23a0925612eb6b99cbb4cefe2720fa3476/components/sync/driver/sync_service.cc
[modify] https://crrev.com/5c590c23a0925612eb6b99cbb4cefe2720fa3476/components/sync/driver/sync_service.h
[modify] https://crrev.com/5c590c23a0925612eb6b99cbb4cefe2720fa3476/components/unified_consent/unified_consent_service.cc

Project Member

Comment 49 by bugdroid1@chromium.org, Jul 26

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/24920caebaf0cf2ed92539a9cd3552861dcdd41e

commit 24920caebaf0cf2ed92539a9cd3552861dcdd41e
Author: Marc Treib <treib@chromium.org>
Date: Thu Jul 26 12:56:01 2018

ProfileSyncService: Remove handling of IsSyncAllowedByFlag

If IsSyncAllowedByFlag is false, then the ProfileSyncService isn't even
instantiated (its factory checks for that). So no need to handle it in
the service.
This CL replaces corresponding "if"s with DCHECKs. It also updates the
comment for DISABLE_REASON_PLATFORM_OVERRIDE, which now doesn't include
the commandline flag anymore.

Bug:  839834 
Change-Id: Ia8466bb1eddddc4b55ab8477e9d8a4a6a0017bf5
Reviewed-on: https://chromium-review.googlesource.com/1151200
Reviewed-by: Jan Krcal <jkrcal@chromium.org>
Commit-Queue: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#578274}
[modify] https://crrev.com/24920caebaf0cf2ed92539a9cd3552861dcdd41e/components/browser_sync/profile_sync_service.cc
[modify] https://crrev.com/24920caebaf0cf2ed92539a9cd3552861dcdd41e/components/sync/driver/sync_service.h

Project Member

Comment 50 by bugdroid1@chromium.org, Aug 7

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8e46ab3705be5a80c52fe568735f8488184a4b78

commit 8e46ab3705be5a80c52fe568735f8488184a4b78
Author: Marc Treib <treib@chromium.org>
Date: Tue Aug 07 11:44:49 2018

ProfileSyncService: shut down immediately on unrecoverable errors

Previously, we'd post a task to perform the shutdown. This meant that
there was a brief period where the unrecoverable error was already set,
but Sync was still active, which made things hard to reason about.
This CL removes the task-posting and instead just directly calls
OnUnrecoverableErrorImpl (which calls ShutdownImpl), which doesn't seem
to have any ill effects.

Bug:  839834 
Change-Id: I998488332c531e817ffcd19e44df0e1273d091a5
Reviewed-on: https://chromium-review.googlesource.com/1163707
Commit-Queue: Marc Treib <treib@chromium.org>
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#581196}
[modify] https://crrev.com/8e46ab3705be5a80c52fe568735f8488184a4b78/components/browser_sync/profile_sync_service.cc
[modify] https://crrev.com/8e46ab3705be5a80c52fe568735f8488184a4b78/components/browser_sync/profile_sync_service.h

Project Member

Comment 51 by bugdroid1@chromium.org, Aug 8

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/5c488794d4d63b05b74d29cb5ca76528fa05e517

commit 5c488794d4d63b05b74d29cb5ca76528fa05e517
Author: Marc Treib <treib@chromium.org>
Date: Wed Aug 08 13:28:44 2018

SyncAuthManager: Clear auth error and access token on sign-out

After sign-out, it's not meaningful to keep the last auth error or any
access token (or requests for one) around.
This was causing a UI glitch, where after signing out and signing in to
a different account, we'd temporarily show an auth error the belonged to
the previous account.

Bug:  839834 , 854579
Change-Id: I1dac8d5cef020555119e49dbfdf46fc26e5a8c85
Reviewed-on: https://chromium-review.googlesource.com/1164953
Commit-Queue: Marc Treib <treib@chromium.org>
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#581545}
[modify] https://crrev.com/5c488794d4d63b05b74d29cb5ca76528fa05e517/components/browser_sync/sync_auth_manager.cc
[modify] https://crrev.com/5c488794d4d63b05b74d29cb5ca76528fa05e517/components/browser_sync/sync_auth_manager_unittest.cc

Status: Fixed (was: Started)
Let's consider this done. I don't have concrete plans for any additional changes apart from some "polish". Also there are enough CLs on this bug :)

Sign in to add a comment