[Sync] Create UserEventSyncBridge.

[Sync] Create UserEventSyncBridge.

This is the first step towards implementing UserEvents and integrating
with Sync. This CL adds a ModelTypeSyncBridge implementation, but it is
never instantiated or called, yet.

While adding unit tests for UserEventSyncBridge, I added
ModelTypeSyncBridge::ChangeProcessorFactory which was usable in both
auto complete and device info sync bridge unit tests.

BUG=701032
Review-Url: https://codereview.chromium.org/2856933005
Cr-Commit-Position: refs/heads/master@{#469829}
Committed: https://chromium.googlesource.com/chromium/src/+/647aa09a706ee13efc535c94698e09559ce6d595

[skym, 2017-05-03 00:13:37]

The CQ bit was checked by skym@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-03 00:14:05]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[skym, 2017-05-03 00:17:28]

Description was changed from

==========
[Sync] Create UserEventSyncBridge.

BUG=701032
==========

to

==========
[Sync] Create UserEventSyncBridge.

This is the first step towards implementing UserEvents and integrating
with Sync. This CL adds a ModelTypeSyncBridge implementation, but it is
never instantiated or called, yet.

While adding unit tests for UserEventSyncBridge, I added
ModelTypeSyncBridge::ChangeProcessorFactory which was usable in both
auto complete and device info sync bridge unit tests.

BUG=701032
==========

[skym, 2017-05-03 00:17:28]

skym@chromium.org changed reviewers:
+ mathp@chromium.org, pnoland@chromium.org

[commit-bot: I haz the power, 2017-05-03 00:30:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-03 00:30:25]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)

[skym, 2017-05-03 00:38:45]

The CQ bit was checked by skym@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-03 00:39:37]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-03 01:00:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-03 01:00:18]

Dry run: Try jobs failed on following builders:
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[skym, 2017-05-03 15:22:52]

The CQ bit was checked by skym@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-03 15:23:28]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[skym, 2017-05-03 15:41:25]

The CQ bit was checked by skym@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-03 15:41:58]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[skym, 2017-05-03 15:58:56]

Patchset #5 (id:80001) has been deleted

[skym, 2017-05-03 15:59:32]

PTAL

pnoland@ - c/sync/*
mathp@ - c/autofill/*

[commit-bot: I haz the power, 2017-05-03 17:18:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-03 17:18:46]

Dry run: This issue passed the CQ dry run.

[Patrick Noland, 2017-05-04 19:43:19]

https://codereview.chromium.org/2856933005/diff/60001/components/sync/model/r...
File components/sync/model/recording_model_type_change_processor.cc (right):

https://codereview.chromium.org/2856933005/diff/60001/components/sync/model/r...
components/sync/model/recording_model_type_change_processor.cc:17:
std::unique_ptr<ModelTypeChangeProcessor> SideAssignNewProcessor(
Given that this
1) creates a new processor
2) returns the new processor 
3) side assigns it to the provided pointer-to-pointer, 
Can you name it something that reflects all that?

https://codereview.chromium.org/2856933005/diff/60001/components/sync/model/r...
File components/sync/model/recording_model_type_change_processor.h (right):

https://codereview.chromium.org/2856933005/diff/60001/components/sync/model/r...
components/sync/model/recording_model_type_change_processor.h:48: static
ModelTypeSyncBridge::ChangeProcessorFactory FactoryForBridgeTest(
Can you explain why it's necessary for the callback to both return and
side-assign the processor?

https://codereview.chromium.org/2856933005/diff/60001/components/sync/user_ev...
File components/sync/user_events/user_event_sync_bridge.cc (right):

https://codereview.chromium.org/2856933005/diff/60001/components/sync/user_ev...
components/sync/user_events/user_event_sync_bridge.cc:38:
base::WriteBigEndian(&key[0], specifics.event_time_usec());
For my own curiosity, why does this (and other uses of WriteBigEndian) use
&key[0] and not c_str()?

https://codereview.chromium.org/2856933005/diff/60001/components/sync/user_ev...
components/sync/user_events/user_event_sync_bridge.cc:183: UserEventSpecifics
specifics;
Again for my own curiosity, any reason this is declared outside the loop body?

https://codereview.chromium.org/2856933005/diff/60001/components/sync/user_ev...
components/sync/user_events/user_event_sync_bridge.cc:189:
change_processor()->ReportError(FROM_HERE,
Do we want to keep the loop going if serialization fails?

https://codereview.chromium.org/2856933005/diff/60001/components/sync/user_ev...
File components/sync/user_events/user_event_sync_bridge_unittest.cc (right):

https://codereview.chromium.org/2856933005/diff/60001/components/sync/user_ev...
components/sync/user_events/user_event_sync_bridge_unittest.cc:36: while
(batch->HasNext()) {
Given that DataBatch is just a wrapper over a vector in practice, it looks a bit
weird that you have to count its size manually. Is there a reason we want to
hide the size of the batch?

https://codereview.chromium.org/2856933005/diff/60001/components/sync/user_ev...
components/sync/user_events/user_event_sync_bridge_unittest.cc:96:
TEST_F(UserEventSyncBridgeTest, Metadata) {
[nit] Maybe MetadataIsInitialized() ?

As an aside, does "Initial sync done" make any sense for UserEvents? If it's
commit only, it hardly matters, right?

[skym, 2017-05-04 20:33:44]

The CQ bit was checked by skym@chromium.org to run a CQ dry run

[skym, 2017-05-04 20:34:00]

Updates for Patrick.

https://codereview.chromium.org/2856933005/diff/60001/components/sync/model/r...
File components/sync/model/recording_model_type_change_processor.cc (right):

https://codereview.chromium.org/2856933005/diff/60001/components/sync/model/r...
components/sync/model/recording_model_type_change_processor.cc:17:
std::unique_ptr<ModelTypeChangeProcessor> SideAssignNewProcessor(
On 2017/05/04 19:43:18, Patrick Noland wrote:
> Given that this
> 1) creates a new processor
> 2) returns the new processor 
> 3) side assigns it to the provided pointer-to-pointer, 
> Can you name it something that reflects all that?

As discussed offline, changing to CreateAndAssignProcessor.

https://codereview.chromium.org/2856933005/diff/60001/components/sync/model/r...
File components/sync/model/recording_model_type_change_processor.h (right):

https://codereview.chromium.org/2856933005/diff/60001/components/sync/model/r...
components/sync/model/recording_model_type_change_processor.h:48: static
ModelTypeSyncBridge::ChangeProcessorFactory FactoryForBridgeTest(
On 2017/05/04 19:43:18, Patrick Noland wrote:
> Can you explain why it's necessary for the callback to both return and
> side-assign the processor?

Done.

https://codereview.chromium.org/2856933005/diff/60001/components/sync/user_ev...
File components/sync/user_events/user_event_sync_bridge.cc (right):

https://codereview.chromium.org/2856933005/diff/60001/components/sync/user_ev...
components/sync/user_events/user_event_sync_bridge.cc:38:
base::WriteBigEndian(&key[0], specifics.event_time_usec());
On 2017/05/04 19:43:18, Patrick Noland wrote:
> For my own curiosity, why does this (and other uses of WriteBigEndian) use
> &key[0] and not c_str()? 

It would seem c_str() is too const.

../../components/sync/user_events/user_event_sync_bridge.cc:39:3: error: no
matching function for call to 'WriteBigEndian'
  base::WriteBigEndian(key.c_str(), specifics.event_time_usec());
  ^~~~~~~~~~~~~~~~~~~~
../../base/big_endian.h:34:13: note: candidate function not viable: 1st argument
('const char *') would lose const qualifier
inline void WriteBigEndian(char buf[], T val) {

https://codereview.chromium.org/2856933005/diff/60001/components/sync/user_ev...
components/sync/user_events/user_event_sync_bridge.cc:183: UserEventSpecifics
specifics;
I'm mostly copying this pattern from other places that call
MessageLite::ParseFromString. We avoid re-creating the protobuffer object (on
the stack) on each iteration.

https://codereview.chromium.org/2856933005/diff/60001/components/sync/user_ev...
components/sync/user_events/user_event_sync_bridge.cc:189:
change_processor()->ReportError(FROM_HERE,
On 2017/05/04 19:43:18, Patrick Noland wrote:
> Do we want to keep the loop going if serialization fails?

Probably not! Added an early return.

https://codereview.chromium.org/2856933005/diff/60001/components/sync/user_ev...
File components/sync/user_events/user_event_sync_bridge_unittest.cc (right):

https://codereview.chromium.org/2856933005/diff/60001/components/sync/user_ev...
components/sync/user_events/user_event_sync_bridge_unittest.cc:36: while
(batch->HasNext()) {
On 2017/05/04 19:43:18, Patrick Noland wrote:
> Given that DataBatch is just a wrapper over a vector in practice, it looks a
bit
> weird that you have to count its size manually. Is there a reason we want to
> hide the size of the batch?

I think the logical conclusion of going down your path of reasoning is to
replace DataBatch/MutableDataBatch with a vector. I don't think it makes sense
to add a size method to DataBatch, it add flexibility and simplicity to
potential implementers, which is the point of the interface. Regardless this is
out of scope of this CL, and I think having slightly awkward code like this in
tests is just fine. I'm significantly less happy about things like
VerifyDataBatch (harder to move to a shared location) showing up in 3+ unittest
files than writing VerifyDataBatchCount once (it can be moved to a shared
location easily).

https://codereview.chromium.org/2856933005/diff/60001/components/sync/user_ev...
components/sync/user_events/user_event_sync_bridge_unittest.cc:96:
TEST_F(UserEventSyncBridgeTest, Metadata) {
On 2017/05/04 19:43:18, Patrick Noland wrote:
> [nit] Maybe MetadataIsInitialized() ?
> 
> As an aside, does "Initial sync done" make any sense for UserEvents? If it's
> commit only, it hardly matters, right? 

Done.

Initial sync done is checked by lots of internal sync logic things. For now, it
is required. Lets talk about this more when we get to my CL that messes with
sync internals to make commit types work.

[commit-bot: I haz the power, 2017-05-04 20:34:23]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Patrick Noland, 2017-05-04 20:38:06]

lgtm

[commit-bot: I haz the power, 2017-05-04 21:52:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-04 21:52:22]

Dry run: This issue passed the CQ dry run.

[skym, 2017-05-05 16:15:49]

Friendly ping! Just a single unit test file for you to look at mathp@ :)

[skym, 2017-05-05 21:51:11]

skym@chromium.org changed reviewers:
- mathp@chromium.org

[skym, 2017-05-05 21:51:13]

Removing autocomplete_sync_bridge_unittest.cc from cl.

[skym, 2017-05-05 21:51:19]

The CQ bit was checked by skym@chromium.org

[skym, 2017-05-05 21:51:19]

The patchset sent to the CQ was uploaded after l-g-t-m from pnoland@chromium.org
Link to the patchset: https://codereview.chromium.org/2856933005/#ps120001
(title: "Removing autocomplete_sync_bridge_unittest.cc from cl.")

[commit-bot: I haz the power, 2017-05-05 21:52:33]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-06 00:33:38]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1494021079206680,
"parent_rev": "97ebdc00887c1581c7adea19c579d194c80f2b04", "commit_rev":
"647aa09a706ee13efc535c94698e09559ce6d595"}

[commit-bot: I haz the power, 2017-05-06 00:34:12]

Description was changed from

==========
[Sync] Create UserEventSyncBridge.

This is the first step towards implementing UserEvents and integrating
with Sync. This CL adds a ModelTypeSyncBridge implementation, but it is
never instantiated or called, yet.

While adding unit tests for UserEventSyncBridge, I added
ModelTypeSyncBridge::ChangeProcessorFactory which was usable in both
auto complete and device info sync bridge unit tests.

BUG=701032
==========

to

==========
[Sync] Create UserEventSyncBridge.

This is the first step towards implementing UserEvents and integrating
with Sync. This CL adds a ModelTypeSyncBridge implementation, but it is
never instantiated or called, yet.

While adding unit tests for UserEventSyncBridge, I added
ModelTypeSyncBridge::ChangeProcessorFactory which was usable in both
auto complete and device info sync bridge unit tests.

BUG=701032

Review-Url: https://codereview.chromium.org/2856933005
Cr-Commit-Position: refs/heads/master@{#469829}
Committed:
https://chromium.googlesource.com/chromium/src/+/647aa09a706ee13efc535c94698e...
==========

[commit-bot: I haz the power, 2017-05-06 00:34:15]

Committed patchset #6 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/647aa09a706ee13efc535c94698e...