dullweber@, do you think this can cause troubles for user consent recording?

UserEventServiceImpl checks if history sync is enabled before forwarding an event with NavigationId to the bridge. https://cs.chromium.org/chromium/src/components/sync/user_events/user_event_service_impl.cc?sq=package:chromium&dr=CSs&l=119

This means we might still record UserConsents (intentional) and FieldTrialEvent (That might be an issue?)
(https://cs.chromium.org/chromium/src/components/sync/user_events/user_event_service_impl.cc?sq=package:chromium&dr=CSs&l=25)

We could extend UserEventServiceImpl::ShouldRecordEvent to check explicitly for UserConsent and deny everything else if sync is disabled

I suspect this is WIP, specially for user consents. We precisely want to keep those consents around, right?

For other user events, as per #c2, it's conditioned to history sync.

Friendly ping for vitaliii@: what's the plan forward?

In the past, I've suggested that commit-only types should be rethought such that initial_sync_done is not set to true automatically, and instead MergeSyncData() is exercised just like the for regular datatypes, when sync starts (possibly in ClientTagBasedModelTypeProcessor::ConnectSync()).

Tentatively marking as appropriate for a fixit exercise.

I have it on my list, but with very low priority. I have no idea when I will get to it.
Rethinking commit only types may be too vague for the fixit.

Picking this up since I think it's simple and has important gains, including ongoing discussions around unity.

sync-triage ping: any updates?

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

commit a754378f4822722d17cd879b4b6f12e7b06b8393
Author: Mikel Astiz <mastiz@chromium.org>
Date: Thu Sep 06 12:37:56 2018

Rename tests to follow team convention

We agreed to prefer the ShouldXXX convention for new code. This patch
renames previously existing tests accordingly, prior to adding new ones
in follow-up patches, to avoid violating the local consistency
principle.

Bug:  830535 
Change-Id: Idb9f87f06ff1e4d886f0c4d3d34180e43eca29f2
Reviewed-on: https://chromium-review.googlesource.com/1209483
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#589140}
[modify] https://crrev.com/a754378f4822722d17cd879b4b6f12e7b06b8393/components/sync/model_impl/client_tag_based_model_type_processor_unittest.cc

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

commit 1d64ee16b3b3a6cef2bddc268d015c7adf63dbd2
Author: Mikel Astiz <mastiz@chromium.org>
Date: Mon Sep 24 13:18:51 2018

Persist account ID in sync metadata

This helps datatypes that are interested in the syncing account ID,
without having to wait until sync engine starting (which is usually
deferred for about 10 seconds).

This field is convenient for user consents, although leveraging this
data is not included in this patch.

A dedicated method is introduced in ModelTypeChangeProcessor although it
is somewhat redundant with IsTrackingMetadata(). The rationale is most
datatypes don't care so it seems overkill to update all tests with extra
boilerplate.

Bug:  830535 
Change-Id: I623b87c693894b5c0845fc3d03772b2701292a00
Reviewed-on: https://chromium-review.googlesource.com/1138236
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Reviewed-by: Mohamed Amir Yosef <mamir@chromium.org>
Cr-Commit-Position: refs/heads/master@{#593519}
[modify] https://crrev.com/1d64ee16b3b3a6cef2bddc268d015c7adf63dbd2/components/sync/model/fake_model_type_change_processor.cc
[modify] https://crrev.com/1d64ee16b3b3a6cef2bddc268d015c7adf63dbd2/components/sync/model/fake_model_type_change_processor.h
[modify] https://crrev.com/1d64ee16b3b3a6cef2bddc268d015c7adf63dbd2/components/sync/model/mock_model_type_change_processor.cc
[modify] https://crrev.com/1d64ee16b3b3a6cef2bddc268d015c7adf63dbd2/components/sync/model/mock_model_type_change_processor.h
[modify] https://crrev.com/1d64ee16b3b3a6cef2bddc268d015c7adf63dbd2/components/sync/model/model_type_change_processor.h
[modify] https://crrev.com/1d64ee16b3b3a6cef2bddc268d015c7adf63dbd2/components/sync/model/recording_model_type_change_processor.cc
[modify] https://crrev.com/1d64ee16b3b3a6cef2bddc268d015c7adf63dbd2/components/sync/model/recording_model_type_change_processor.h
[modify] https://crrev.com/1d64ee16b3b3a6cef2bddc268d015c7adf63dbd2/components/sync/model_impl/client_tag_based_model_type_processor.cc
[modify] https://crrev.com/1d64ee16b3b3a6cef2bddc268d015c7adf63dbd2/components/sync/model_impl/client_tag_based_model_type_processor.h
[modify] https://crrev.com/1d64ee16b3b3a6cef2bddc268d015c7adf63dbd2/components/sync/model_impl/client_tag_based_model_type_processor_unittest.cc
[modify] https://crrev.com/1d64ee16b3b3a6cef2bddc268d015c7adf63dbd2/components/sync/protocol/model_type_state.proto

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

commit a10225477359489b4dbe7f245653372776d8d3ff
Author: Mikel Astiz <mastiz@chromium.org>
Date: Mon Nov 12 13:10:52 2018

Defer initial_sync_done for commit-only types

Instead of assuming IsTrackingMetadata() as soon as the model is ready
to sync, we now do it when:
a) The model is ready to sync.
b) Sync is known to be enabled (either because it just got enabled, or
   because we know from persisted data that is was previously enabled).

This makes commit-only types more similar to regular types, and allows
bridges to leverage the state. For example, while sync is enabled (i.e.
IsTrackingMetadata()), there should always be means to know which
account ID is syncing, which is now exposed in ModelTypeChangeProcessor.

From the bridge's perspective, the only transition from not tracking
metadata, and metadata being tracked (sync enabled), is MergeSyncData().

ModelReadyToSync() now presents the transition from unknown (i.e. unsure
whether sync is enabled or not until sync metadata is read from disk)
to true/false depending on the persisted value (true if sync was running
for the datatype in previous executions of the browser). Just like for
regular (non-commit-only) datatypes.

Bug:  830535 
Change-Id: I573e27c2dcde1bd5835c30319065c178eb7b4679
Reviewed-on: https://chromium-review.googlesource.com/c/1136307
Reviewed-by: Olivier Robin <olivierrobin@chromium.org>
Reviewed-by: Markus Heintz <markusheintz@chromium.org>
Reviewed-by: vitaliii <vitaliii@chromium.org>
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#607207}
[modify] https://crrev.com/a10225477359489b4dbe7f245653372776d8d3ff/chrome/browser/sync/test/integration/single_client_user_events_sync_test.cc
[modify] https://crrev.com/a10225477359489b4dbe7f245653372776d8d3ff/components/consent_auditor/consent_sync_bridge_impl.cc
[modify] https://crrev.com/a10225477359489b4dbe7f245653372776d8d3ff/components/consent_auditor/consent_sync_bridge_impl.h
[modify] https://crrev.com/a10225477359489b4dbe7f245653372776d8d3ff/components/consent_auditor/consent_sync_bridge_impl_unittest.cc
[modify] https://crrev.com/a10225477359489b4dbe7f245653372776d8d3ff/components/reading_list/core/reading_list_store_unittest.cc
[modify] https://crrev.com/a10225477359489b4dbe7f245653372776d8d3ff/components/sync/device_info/device_info_sync_bridge_unittest.cc
[modify] https://crrev.com/a10225477359489b4dbe7f245653372776d8d3ff/components/sync/model/model_type_store_test_util.cc
[modify] https://crrev.com/a10225477359489b4dbe7f245653372776d8d3ff/components/sync/model/model_type_store_test_util.h
[modify] https://crrev.com/a10225477359489b4dbe7f245653372776d8d3ff/components/sync/model_impl/client_tag_based_model_type_processor.cc
[modify] https://crrev.com/a10225477359489b4dbe7f245653372776d8d3ff/components/sync/model_impl/client_tag_based_model_type_processor_unittest.cc
[modify] https://crrev.com/a10225477359489b4dbe7f245653372776d8d3ff/components/sync/user_events/user_event_service_impl_unittest.cc
[modify] https://crrev.com/a10225477359489b4dbe7f245653372776d8d3ff/components/sync/user_events/user_event_sync_bridge.cc
[modify] https://crrev.com/a10225477359489b4dbe7f245653372776d8d3ff/components/sync/user_events/user_event_sync_bridge.h
[modify] https://crrev.com/a10225477359489b4dbe7f245653372776d8d3ff/components/sync/user_events/user_event_sync_bridge_unittest.cc