[Sync] Support commit only types.

[Sync] Support commit only types.

The main differences of commit only types is that they do not have a
progress marker, they use very few metadata fields of SyncEntity, they
do not perform GetUpdates, and they delete themselves after commit
confirmation. Also only supposed on USS/NonBlocking path.

This CL updates internal sync logic to facilitate commit only types.
With these changes user events should be entirely functional on the
client. Can specify the flag --enable-features=SyncUserEvents and
existing translate integration will start emitting.

The difference between using the static CommitOnlyTypes.Has(...), vs a
private boolean in worker vs processor is unfortunate. I think the
CommitOnlyTypes.Has(...) approach is better whenever something has the
model type, however, current unit tests are tightly coupled with
PREFERENCES, which is why I created the |commit_only_| field.

BUG=719041
Review-Url: https://codereview.chromium.org/2916133002
Cr-Commit-Position: refs/heads/master@{#477350}
Committed: https://chromium.googlesource.com/chromium/src/+/c15b61bdf8133f14d7f715ee6e0dcc5ab65db57c

[skym, 2017-06-01 19:21:32]

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

[commit-bot: I haz the power, 2017-06-01 19:22:37]

Dry run: CQ is trying da patch.

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

[skym, 2017-06-01 19:31:52]

Description was changed from

==========
[Sync] Support commit only types.

BUG=719041
==========

to

==========
[Sync] Support commit only types.

The main differences of commit only types is that they do not have a
progress marker, they use very few metadata fields of SyncEntity , they
do not perform GetUpdates, and they delete themselves after commit
confirmation. Also only supposed on USS/NonBlocking path.

This CL updates internal sync logic to facilitate commit only types.
With these changes user events should be entirely functional on the
client. Can specify the flag --enable-features=SyncUserEvents and
existing translate integration will start emitting.

The difference between using the static CommitOnlyTypes.Has(...), vs a
private boolean in worker vs processor is unfortunate. I think the
CommitOnlyTypes.Has(...) approach is better whenever something has the
model type, however, current unit tests are tightly coupled with
PREFERENCES, which is why I created the |commit_only_| field.

BUG=719041
==========

[skym, 2017-06-01 19:31:52]

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

[skym, 2017-06-01 19:32:05]

PTAL

[skym, 2017-06-01 20:27:34]

Description was changed from

==========
[Sync] Support commit only types.

The main differences of commit only types is that they do not have a
progress marker, they use very few metadata fields of SyncEntity , they
do not perform GetUpdates, and they delete themselves after commit
confirmation. Also only supposed on USS/NonBlocking path.

This CL updates internal sync logic to facilitate commit only types.
With these changes user events should be entirely functional on the
client. Can specify the flag --enable-features=SyncUserEvents and
existing translate integration will start emitting.

The difference between using the static CommitOnlyTypes.Has(...), vs a
private boolean in worker vs processor is unfortunate. I think the
CommitOnlyTypes.Has(...) approach is better whenever something has the
model type, however, current unit tests are tightly coupled with
PREFERENCES, which is why I created the |commit_only_| field.

BUG=719041
==========

to

==========
[Sync] Support commit only types.

The main differences of commit only types is that they do not have a
progress marker, they use very few metadata fields of SyncEntity, they
do not perform GetUpdates, and they delete themselves after commit
confirmation. Also only supposed on USS/NonBlocking path.

This CL updates internal sync logic to facilitate commit only types.
With these changes user events should be entirely functional on the
client. Can specify the flag --enable-features=SyncUserEvents and
existing translate integration will start emitting.

The difference between using the static CommitOnlyTypes.Has(...), vs a
private boolean in worker vs processor is unfortunate. I think the
CommitOnlyTypes.Has(...) approach is better whenever something has the
model type, however, current unit tests are tightly coupled with
PREFERENCES, which is why I created the |commit_only_| field.

BUG=719041
==========

[commit-bot: I haz the power, 2017-06-01 20:40:21]

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

[commit-bot: I haz the power, 2017-06-01 20:40:22]

Dry run: This issue passed the CQ dry run.

[Patrick Noland, 2017-06-01 21:34:40]

https://codereview.chromium.org/2916133002/diff/40001/components/sync/engine_...
File components/sync/engine_impl/model_type_worker_unittest.cc (right):

https://codereview.chromium.org/2916133002/diff/40001/components/sync/engine_...
components/sync/engine_impl/model_type_worker_unittest.cc:1201:
specifics.mutable_user_event();
Do you not want to put any data into this?

https://codereview.chromium.org/2916133002/diff/40001/components/sync/engine_...
components/sync/engine_impl/model_type_worker_unittest.cc:1224: EXPECT_EQ(1,
emitter()->GetCommitCounters().num_commits_attempted);
This process(verifying the commit counters and responses) seems kind of
repetitive, is there a good way to streamline it?

https://codereview.chromium.org/2916133002/diff/40001/components/sync/model_i...
File components/sync/model_impl/shared_model_type_processor_unittest.cc (right):

https://codereview.chromium.org/2916133002/diff/40001/components/sync/model_i...
components/sync/model_impl/shared_model_type_processor_unittest.cc:793:
worker()->ExpectPendingCommits(std::vector<std::string>());
weird, does {} not work for this?

https://codereview.chromium.org/2916133002/diff/40001/components/sync/test/en...
File components/sync/test/engine/mock_connection_manager.cc (right):

https://codereview.chromium.org/2916133002/diff/40001/components/sync/test/en...
components/sync/test/engine/mock_connection_manager.cc:551: int data_type_id =
gu.from_progress_marker(i).data_type_id();
any reason not to just inline this into the EXPECT_TRUE call?

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

https://codereview.chromium.org/2916133002/diff/40001/components/sync/user_ev...
components/sync/user_events/user_event_sync_bridge.cc:89:
DCHECK_EQ(EntityChange::ACTION_DELETE, change.type());
Are you sure you just want to DCHECK this condition? Maybe it would be better to
just ignore non-deletes? Imagine you try to delete an entity when the change
type was ADD and fail to do so, are you really in an error state?

[skym, 2017-06-01 22:43:15]

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

[skym, 2017-06-01 22:43:20]

Updates for pnoland.

https://codereview.chromium.org/2916133002/diff/40001/components/sync/engine_...
File components/sync/engine_impl/model_type_worker_unittest.cc (right):

https://codereview.chromium.org/2916133002/diff/40001/components/sync/engine_...
components/sync/engine_impl/model_type_worker_unittest.cc:1201:
specifics.mutable_user_event();
On 2017/06/01 21:34:40, Patrick Noland wrote:
> Do you not want to put any data into this?

Done.

https://codereview.chromium.org/2916133002/diff/40001/components/sync/engine_...
components/sync/engine_impl/model_type_worker_unittest.cc:1224: EXPECT_EQ(1,
emitter()->GetCommitCounters().num_commits_attempted);
On 2017/06/01 21:34:39, Patrick Noland wrote:
> This process(verifying the commit counters and responses) seems kind of
> repetitive, is there a good way to streamline it?

Done.

https://codereview.chromium.org/2916133002/diff/40001/components/sync/model_i...
File components/sync/model_impl/shared_model_type_processor_unittest.cc (right):

https://codereview.chromium.org/2916133002/diff/40001/components/sync/model_i...
components/sync/model_impl/shared_model_type_processor_unittest.cc:793:
worker()->ExpectPendingCommits(std::vector<std::string>());
On 2017/06/01 21:34:40, Patrick Noland wrote:
> weird, does {} not work for this?

../../components/sync/model_impl/shared_model_type_processor_unittest.cc:793:34:
error: chosen constructor is explicit in copy-initialization
  worker()->ExpectPendingCommits({});
                                 ^~
../../build/linux/debian_jessie_amd64-sysroot/usr/lib/gcc/x86_64-linux-gnu/4.8/../../../../include/c++/4.8/debug/vector:79:7:
note: explicit constructor declared here
      vector(const _Allocator& __a = _Allocator())
      ^
../../components/sync/test/engine/mock_model_type_worker.h:54:61: note: passing
argument to parameter 'tag_hashes' here
  void ExpectPendingCommits(const std::vector<std::string>& tag_hashes);
                                                            ^
1 error generated.

Although, std::vector<std::string>{} works, but at that point, it's like, why
bother.

https://codereview.chromium.org/2916133002/diff/40001/components/sync/test/en...
File components/sync/test/engine/mock_connection_manager.cc (right):

https://codereview.chromium.org/2916133002/diff/40001/components/sync/test/en...
components/sync/test/engine/mock_connection_manager.cc:551: int data_type_id =
gu.from_progress_marker(i).data_type_id();
On 2017/06/01 21:34:40, Patrick Noland wrote:
> any reason not to just inline this into the EXPECT_TRUE call?

Cause it gets used down below on line 556!

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

https://codereview.chromium.org/2916133002/diff/40001/components/sync/user_ev...
components/sync/user_events/user_event_sync_bridge.cc:89:
DCHECK_EQ(EntityChange::ACTION_DELETE, change.type());
On 2017/06/01 21:34:40, Patrick Noland wrote:
> Are you sure you just want to DCHECK this condition? Maybe it would be better
to
> just ignore non-deletes? Imagine you try to delete an entity when the change
> type was ADD and fail to do so, are you really in an error state?

The processor should never send us any other action. We don't have any handling
for any other action. You're right that we could ignore it.

Even if we change our minds about USER_EVENTS in the future and start sending
remote changes down to chrome clients, these old clients aren't going to see
that data. We don't currently have progress markers, we don't currently include
USER_EVENTS in GetUpdates calls. I think we're safe to make changes without
locking ourselves in here with the DCHECK.

It's difficult to reason about what the correct handling of an ADD should be,
because it shouldn't happen. Deleting events is low impact anyways.

[commit-bot: I haz the power, 2017-06-01 22:44:41]

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-06-01 22:58:48]

lgtm

https://codereview.chromium.org/2916133002/diff/60001/components/sync/engine_...
File components/sync/engine_impl/syncer.cc (right):

https://codereview.chromium.org/2916133002/diff/60001/components/sync/engine_...
components/sync/engine_impl/syncer.cc:117: ModelTypeSet add_for_commit =
Intersection(*request_types, CommitOnlyTypes());
This name made it harder for me to follow what you're doing here. Maybe
requested_commit_only_types would be easier to follow? And a comment explaining
why you remove these types for download and then add them back for the GU
request?

https://codereview.chromium.org/2916133002/diff/60001/components/sync/model_i...
File components/sync/model_impl/shared_model_type_processor.h (right):

https://codereview.chromium.org/2916133002/diff/60001/components/sync/model_i...
components/sync/model_impl/shared_model_type_processor.h:215: // model types.
Can you (briefly) elaborate on how/why the behavior is different?

[commit-bot: I haz the power, 2017-06-01 23:13:49]

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

[commit-bot: I haz the power, 2017-06-01 23:13:50]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[skym, 2017-06-01 23:15:21]

More updates for pnoland.

https://codereview.chromium.org/2916133002/diff/60001/components/sync/engine_...
File components/sync/engine_impl/syncer.cc (right):

https://codereview.chromium.org/2916133002/diff/60001/components/sync/engine_...
components/sync/engine_impl/syncer.cc:117: ModelTypeSet add_for_commit =
Intersection(*request_types, CommitOnlyTypes());
On 2017/06/01 22:58:48, Patrick Noland wrote:
> This name made it harder for me to follow what you're doing here. Maybe
> requested_commit_only_types would be easier to follow? And a comment
explaining
> why you remove these types for download and then add them back for the GU
> request?

Done.

https://codereview.chromium.org/2916133002/diff/60001/components/sync/model_i...
File components/sync/model_impl/shared_model_type_processor.h (right):

https://codereview.chromium.org/2916133002/diff/60001/components/sync/model_i...
components/sync/model_impl/shared_model_type_processor.h:215: // model types.
On 2017/06/01 22:58:48, Patrick Noland wrote:
> Can you (briefly) elaborate on how/why the behavior is different?

Done.

[skym, 2017-06-01 23:15:25]

The CQ bit was checked by skym@chromium.org

[skym, 2017-06-01 23:15:26]

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/2916133002/#ps80001
(title: "More updates for pnoland.")

[commit-bot: I haz the power, 2017-06-01 23:16:09]

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-06-01 23:41:52]

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

[commit-bot: I haz the power, 2017-06-01 23:41:53]

Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[skym, 2017-06-02 16:12:04]

The CQ bit was checked by skym@chromium.org

[skym, 2017-06-02 16:12:04]

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/2916133002/#ps100001
(title: "Undo unittest refactor for GetUpdatesCounters that checks commit
data.")

[commit-bot: I haz the power, 2017-06-02 16:12:16]

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-06-02 18:13:23]

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

[commit-bot: I haz the power, 2017-06-02 18:13:26]

Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[skym, 2017-06-02 18:18:14]

The CQ bit was checked by skym@chromium.org

[commit-bot: I haz the power, 2017-06-02 18:19:05]

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-06-02 20:33:30]

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

[commit-bot: I haz the power, 2017-06-02 20:33:31]

Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[skym, 2017-06-02 22:54:06]

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

[commit-bot: I haz the power, 2017-06-02 22:54:40]

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-06-03 01:04:00]

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

[commit-bot: I haz the power, 2017-06-03 01:04:01]

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

[skym, 2017-06-06 17:23:34]

The CQ bit was checked by skym@chromium.org

[skym, 2017-06-06 17:23:35]

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/2916133002/#ps140001
(title: "Rebase again.")

[commit-bot: I haz the power, 2017-06-06 17:24:14]

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-06-06 18:38:55]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1496769814498150,
"parent_rev": "7a3b1574bb3a12078de8008771103163e0317652", "commit_rev":
"c15b61bdf8133f14d7f715ee6e0dcc5ab65db57c"}

[commit-bot: I haz the power, 2017-06-06 18:39:07]

Description was changed from

==========
[Sync] Support commit only types.

The main differences of commit only types is that they do not have a
progress marker, they use very few metadata fields of SyncEntity, they
do not perform GetUpdates, and they delete themselves after commit
confirmation. Also only supposed on USS/NonBlocking path.

This CL updates internal sync logic to facilitate commit only types.
With these changes user events should be entirely functional on the
client. Can specify the flag --enable-features=SyncUserEvents and
existing translate integration will start emitting.

The difference between using the static CommitOnlyTypes.Has(...), vs a
private boolean in worker vs processor is unfortunate. I think the
CommitOnlyTypes.Has(...) approach is better whenever something has the
model type, however, current unit tests are tightly coupled with
PREFERENCES, which is why I created the |commit_only_| field.

BUG=719041
==========

to

==========
[Sync] Support commit only types.

The main differences of commit only types is that they do not have a
progress marker, they use very few metadata fields of SyncEntity, they
do not perform GetUpdates, and they delete themselves after commit
confirmation. Also only supposed on USS/NonBlocking path.

This CL updates internal sync logic to facilitate commit only types.
With these changes user events should be entirely functional on the
client. Can specify the flag --enable-features=SyncUserEvents and
existing translate integration will start emitting.

The difference between using the static CommitOnlyTypes.Has(...), vs a
private boolean in worker vs processor is unfortunate. I think the
CommitOnlyTypes.Has(...) approach is better whenever something has the
model type, however, current unit tests are tightly coupled with
PREFERENCES, which is why I created the |commit_only_| field.

BUG=719041

Review-Url: https://codereview.chromium.org/2916133002
Cr-Commit-Position: refs/heads/master@{#477350}
Committed:
https://chromium.googlesource.com/chromium/src/+/c15b61bdf8133f14d7f715ee6e0d...
==========

[commit-bot: I haz the power, 2017-06-06 18:39:10]

Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/c15b61bdf8133f14d7f715ee6e0d...