I think these are all the APIs that should be audited:

https://cs.chromium.org/search/?q=GetPreferredDataTypes+-f:test+-f:fake+-f:mock+case:yes&sq=package:chromium&type=cs

https://cs.chromium.org/search/?q=GetChosenDataTypes+-f:test+-f:fake+-f:mock+case:yes&sq=package:chromium&type=cs

https://cs.chromium.org/search/?q=GetTransportState+-f:test+-f:fake+-f:mock+case:yes&sq=package:chromium&type=cs

https://cs.chromium.org/search/?q=IsEngineInitialized+-f:test+-f:fake+-f:mock+case:yes&sq=package:chromium&type=cs

https://cs.chromium.org/search/?q=GetActiveDataTypes+-f:test+-f:fake+-f:mock+case:yes&sq=package:chromium&type=cs

If those call sites also check CanSyncFeatureStart, IsSyncFeatureEnabled, or IsSyncFeatureActive, then they're okay (or at least not broken by Butter/standalone transport*).


* CanSyncFeatureStart might be problematic: It doesn't include a check for first-time setup, so we don't necessarily have consent yet. But that's already the case before standalone transport.

One more finding: Checks within Sync stuff (DataTypeController, SyncableService etc) are probably fine, since those data types won't be enabled in transport mode. (And if they ever are, we'll have to evaluate these things again anyway.)

I checked GetPreferredDataTypes, and they're all okay. There are 3 instances that are a bit fishy (though currently still okay in practice):

1) IsHistorySyncEnabled in search_tab_helper.cc: Checks *only* GetPreferredDataTypes. It doesn't matter in practice because it's only meant as a "is history sync not disabled" kind of check, and it's only used to disable Most Likely, which has proper checks in other places.

2) CanRecordHistory and IsUserEventsDatatypeEnabled in UserEventServiceImpl: These are broken, but it doesn't matter in practice because the whole USER_EVENTS data type isn't active in standalone transport.

3) GetSyncUsernameIfSyncingPasswords in password_sync_util.cc: Checks only GetPreferredDataTypes, and returns the primary account email if PASSWORDS are there. Not sure what exactly the intended semantics are here, but standalone transport doesn't break this any more than it already is.

GetChosenDataTypes: Not many call sites so far, and only one that is fishy, in UnifiedConsentService::UpdateSettingsForMigration [1].
UnifiedConsentService is changing a lot at the moment, so we should probably first check if that part is here to stay. If so, we should probably add an IsSyncFeatureEnabled check.

[1] https://cs.chromium.org/chromium/src/components/unified_consent/unified_consent_service.cc?rcl=01b3033e8fab4f6fdfcc2ae55ff4ab794a0a9342&l=342

GetTransportState: Only a single caller outside of sync code [1]. It's okay for now since THEMES can't be active in transport mode, but we should add an IsSyncFeatureEnabled check or (probably better) use GetUploadToGoogleState.

[1] https://cs.chromium.org/chromium/src/chrome/browser/chromeos/extensions/wallpaper_private_api.cc?rcl=a2e73cd302ae9b2f28c7ca270ae0b5edda468f33&l=291

a quick note about Comment 5:
It's great to see that these places don't cause a problem today. We should make sure however, to properly document those caveats in that code. Even if these integrations don't change, a common problem of copy&pasting code is that it's usually being picked from the worst locations -- so let's make sure people are aware of the problems.

Agreed, and already on it. I have pending CLs to fix 1) and 2) from #5, and have already updated the comment for 3).

IsEngineInitialized: Mostly used by Sync setup/config UI; those uses are all fine. Some places use it together with IsUsingSecondaryPassphrase (which isn't known before the engine is initialized); that's also fine. Other uses:
- One in UKM's SyncDisableObserver, used to trigger a purge when the engine becomes uninitialized. I'm not convinced that's a good idea (might get some spurious purges), but that's not related to Sync transport.
- Two uses in UnifiedConsentService; should address together with #6.

GetActiveDataTypes: Most callers also check IsSyncFeatureActive, so they're fine (but should probably be moved to GetUploadToGoogleState eventually).
All remaining callers only check data types that aren't supported in transport mode (and not planned to be supported any time soon), so fine too. For some of these we could maybe move to GetUploadToGoogleState or add IsSyncFeatureActive checks, but that's needs evaluation on a case-by-case basis, which we'll do if we ever add support for these in transport mode.

One more API worth checking: GetDisableReasons/HasDisableReason.
https://cs.chromium.org/search/?q=GetDisableReasons+-f:test+-f:fake+-f:mock+case:yes&p=1&sq=package:chromium&type=cs
https://cs.chromium.org/search/?q=HasDisableReason+-f:test+-f:fake+-f:mock+case:yes&sq=package:chromium&type=cs
(Lots of false positives in the results because extensions code has identically-named functions.)

GetDisableReasons/HasDisableReason: These are mostly fine; one GetDisableReasons call from UnifiedConsentService::OnStateChanged should probably be replaced by CanSyncFeatureStart.

Summary / TODOs:
1) UnifiedConsentService: One GetChosenDataTypes and one GetDisableReasons call that need adjusting. (After these changes, the IsEngineInitialized calls will be fine.)
2) WallpaperPrivateAPI: Use GetUploadToGoogleState, just to be precise (no actual issue).
And that's it for now :)

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

commit eb8f8994c41f2cbe2e7a4c8c5492a845756d0084
Author: Marc Treib <treib@chromium.org>
Date: Wed Nov 21 13:10:20 2018

UserEventServiceImpl: Check IsSyncFeatureEnabled

If IsSyncFeatureEnabled is false, then GetPreferredDataTypes doesn't
really mean anything.
In practice, this wasn't an actual issue: If Sync isn't enabled, then
the whole UserEventService can't upload anything anyway. But still best
to be precise here.

Bug:  907027 , 856081
Change-Id: I2e3ee7200118175ed9397b5a1b7d9fedd0eb7692
Reviewed-on: https://chromium-review.googlesource.com/c/1344104
Reviewed-by: vitaliii <vitaliii@chromium.org>
Commit-Queue: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#610022}
[modify] https://crrev.com/eb8f8994c41f2cbe2e7a4c8c5492a845756d0084/components/sync/user_events/user_event_service_impl.cc

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

commit 93083e2e13605d03fac9d8ca1d7fcc09ba2b3659
Author: Marc Treib <treib@chromium.org>
Date: Wed Nov 21 13:56:58 2018

UnifiedConsentService: fix Sync feature state checks

This includes two fixes:
1) In OnStateChanged, check CanSyncFeatureStart instead of checking for
   absence of disable reasons. CanSyncFeatureStart also implies no
   disable reasons, but additionally checks that the syncing account is
   primary, meaning that Sync the *feature* can start, not just the
   transport layer.
2) In UpdateSettingsForMigration, when determining the UKM state, add a
   check for IsSyncFeatureEnabled. The reason is that
   GetChosenDataTypes returns "all types" by default, even if Sync is
   disabled, or only the transport layer is running. So the old code
   could result in some false positives.
   This change is analogous to https://crrev.com/c/1343369.

Bug:  907027 
Change-Id: Ie5eaf261b17da072dcd2cb8a596a7342e2fc8192
Reviewed-on: https://chromium-review.googlesource.com/c/1346131
Reviewed-by: Thomas Tangl <tangltom@chromium.org>
Commit-Queue: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#610038}
[modify] https://crrev.com/93083e2e13605d03fac9d8ca1d7fcc09ba2b3659/components/unified_consent/unified_consent_service.cc

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

commit 725a59b7a069055de7b382b0c8bda26fc2628d80
Author: Marc Treib <treib@chromium.org>
Date: Wed Nov 21 15:03:04 2018

SearchTabHelper: Add IsSyncFeatureEnabled check to IsHistorySyncEnabled

Previously it just checked GetPreferredDataTypes. This was probably
okay in practice (the check was anyway more of a "has the user not
disabled history sync" rather than "is history sync enabled"), but
still better to be correct.

While we're here, also move to SyncUserSettings, which is the new way
to query any user-configurable bits of Sync.

This also includes a bunch of test cleanup: Removing a Mock class that
isn't used, and using NiceMock to avoid unnecessary chattiness.

Bug:  907027 , 884159
Change-Id: I2a3d0f8ef13a74b51f4b4a2edc19976a61c6c430
Reviewed-on: https://chromium-review.googlesource.com/c/1343087
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Commit-Queue: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#610055}
[modify] https://crrev.com/725a59b7a069055de7b382b0c8bda26fc2628d80/chrome/browser/ui/search/search_tab_helper.cc
[modify] https://crrev.com/725a59b7a069055de7b382b0c8bda26fc2628d80/chrome/browser/ui/search/search_tab_helper_unittest.cc

This is done modulo the pending CL https://crrev.com/c/1346391 (which won't land this week because a reviewer is OOO).

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

commit d4a0ec5f48ba957aa67d12fff58ed61aa2e95235
Author: Marc Treib <treib@chromium.org>
Date: Tue Nov 27 13:08:02 2018

WallpaperPrivate API: Fix CheckProfileSyncServiceStatus logic

The previous logic failed to handle various edge cases, such as when
Sync as a whole is disabled or fails to start up.
The new logic checks whether the user has chosen to sync themes,
independent of the current runtime state of Sync.

Bug:  907027 
Change-Id: If9974c0cfd99ad66c28726e319b6f3c565073d5d
Reviewed-on: https://chromium-review.googlesource.com/c/1346391
Commit-Queue: Marc Treib <treib@chromium.org>
Reviewed-by: Xiaoqian Dai <xdai@chromium.org>
Cr-Commit-Position: refs/heads/master@{#611083}
[modify] https://crrev.com/d4a0ec5f48ba957aa67d12fff58ed61aa2e95235/chrome/browser/chromeos/extensions/wallpaper_private_api.cc