[Sync] Adding a passthrough MetadataChangeList implementation.

[Sync] Adding a passthrough MetadataChangeList implementation.

BUG=661260
Committed: https://crrev.com/890cc54a9df16b8cc36e1d29df9707ec7bed3f24
Cr-Commit-Position: refs/heads/master@{#429473}

[skym, 2016-11-01 21:29:36]

skym@chromium.org changed reviewers:
+ gangwu@chromium.org, maxbogue@chromium.org, pavely@chromium.org,
pnoland@chromium.org

[skym, 2016-11-01 21:29:36]

PTAL.

Two things I dislike about this
1. ModelTypeChangeProcessor::Put and Delete both take pointers to MCLs, which
means we cannot inline a temporary.
2. WriteBatchPassthroughMetadataChangeList is comically long. ChangeList isn't
really an accurate description of this approach either.

[skym, 2016-11-01 21:29:39]

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

[commit-bot: I haz the power, 2016-11-01 21:30:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[pavely, 2016-11-01 21:46:54]

On 2016/11/01 21:29:36, skym wrote:
> PTAL.
> 
> Two things I dislike about this
> 1. ModelTypeChangeProcessor::Put and Delete both take pointers to MCLs, which
> means we cannot inline a temporary.
> 2. WriteBatchPassthroughMetadataChangeList is comically long. ChangeList isn't
> really an accurate description of this approach either.

1. MCL shouldn't be viewed as temporary. You create one, make all changes in its
context and in the end these changes magically appear in the store. It should
equally apply to Simple and Passthrough MCL.
I think it's Ok to force consumers create separate stack variable for MCL.

2. After you introduce this implementation it should become *the* implementation
of MCL for ModelTypeStore based services. SimpleMCL shouldn't be used at all.
Thus name could be as bold as you want.
MetadataChangeListForModelTypeStore? It's a whole four chars shorter than yours.

Speaking of which, why does updated device_info use mixture of MCLs?

[skym, 2016-11-01 22:00:19]

On 2016/11/01 21:46:54, pavely wrote:
> On 2016/11/01 21:29:36, skym wrote:
> > PTAL.
> > 
> > Two things I dislike about this
> > 1. ModelTypeChangeProcessor::Put and Delete both take pointers to MCLs,
which
> > means we cannot inline a temporary.
> > 2. WriteBatchPassthroughMetadataChangeList is comically long. ChangeList
isn't
> > really an accurate description of this approach either.
> 
> 1. MCL shouldn't be viewed as temporary. You create one, make all changes in
its
> context and in the end these changes magically appear in the store. It should
> equally apply to Simple and Passthrough MCL.
> I think it's Ok to force consumers create separate stack variable for MCL.
> 
> 2. After you introduce this implementation it should become *the*
implementation
> of MCL for ModelTypeStore based services. SimpleMCL shouldn't be used at all.
> Thus name could be as bold as you want.
> MetadataChangeListForModelTypeStore? It's a whole four chars shorter than
yours.
> 
> Speaking of which, why does updated device_info use mixture of MCLs?

1. I think MCL being a temporary is great. All it's supposed to do is allow the
processor to make metadata changes. It's the sync bridge's job to make sure they
land atomically. If it's a temporary or not, that's all an implementation detail
to the processor. Yeah, making separate stack variables it's okay, but kind of
unfortunate. I don't see a nice way around it given C++'s syntax here.

2. I disagree here. I don't think you want it to be *the* implementation,
because you don't always have a WriteBatch in scope.

> Speaking of which, why does updated device_info use mixture of MCLs?

Because a WriteBatch is not always in scope to make this easy. I wanted to keep
the bridge as stateless as possible. Having a mixture is a little bit ugly, I
agree. But it should work just fine.

[commit-bot: I haz the power, 2016-11-01 23:02:50]

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

[commit-bot: I haz the power, 2016-11-01 23:02:51]

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

[skym, 2016-11-02 21:58:00]

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

[skym, 2016-11-02 21:58:08]

Updates for Max.

[commit-bot: I haz the power, 2016-11-02 21:58:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[skym, 2016-11-02 22:04:35]

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

[skym, 2016-11-02 22:04:45]

Renamed for real.

[commit-bot: I haz the power, 2016-11-02 22:04:45]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[maxbogue, 2016-11-02 22:53:30]

lgtm w/ couple nits

https://codereview.chromium.org/2470953002/diff/60001/components/sync/model_i...
File components/sync/model_impl/passthrough_metadata_change_list_unittest.cc
(right):

https://codereview.chromium.org/2470953002/diff/60001/components/sync/model_i...
components/sync/model_impl/passthrough_metadata_change_list_unittest.cc:149:
EXPECT_NE(update_metadata_map().find(kKey1)->second,
would probably be easier to parse this and the next one if you did like...

auto key1_iter = update_metadata_map().find(kKey1);

on a preceding line. Also if you made metadata1 and metadata2 separate, you
could EXPECT_EQ(metadata1.SerializeAsString(), ...)

https://codereview.chromium.org/2470953002/diff/60001/components/sync/model_i...
components/sync/model_impl/passthrough_metadata_change_list_unittest.cc:151:
EXPECT_EQ(std::next(update_metadata_map().find(kKey1))->second,
expected thing first! here and above and below.

[skym, 2016-11-02 23:10:56]

https://codereview.chromium.org/2470953002/diff/60001/components/sync/model_i...
File components/sync/model_impl/passthrough_metadata_change_list_unittest.cc
(right):

https://codereview.chromium.org/2470953002/diff/60001/components/sync/model_i...
components/sync/model_impl/passthrough_metadata_change_list_unittest.cc:149:
EXPECT_NE(update_metadata_map().find(kKey1)->second,
On 2016/11/02 22:53:30, maxbogue wrote:
> would probably be easier to parse this and the next one if you did like...
> 
> auto key1_iter = update_metadata_map().find(kKey1);
> 
> on a preceding line. Also if you made metadata1 and metadata2 separate, you
> could EXPECT_EQ(metadata1.SerializeAsString(), ...)

Done and Done.

https://codereview.chromium.org/2470953002/diff/60001/components/sync/model_i...
components/sync/model_impl/passthrough_metadata_change_list_unittest.cc:151:
EXPECT_EQ(std::next(update_metadata_map().find(kKey1))->second,
On 2016/11/02 22:53:30, maxbogue wrote:
> expected thing first! here and above and below.

Done.

[skym, 2016-11-02 23:13:33]

The CQ bit was checked by skym@chromium.org

[skym, 2016-11-02 23:13:34]

The patchset sent to the CQ was uploaded after l-g-t-m from
maxbogue@chromium.org
Link to the patchset: https://codereview.chromium.org/2470953002/#ps100001
(title: "Last update for Max.")

[commit-bot: I haz the power, 2016-11-02 23:14:12]

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, 2016-11-03 00:23:39]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-11-03 00:25:27]

Description was changed from

==========
[Sync] Adding a passthrough MetadataChangeList implementation.

BUG=661260
==========

to

==========
[Sync] Adding a passthrough MetadataChangeList implementation.

BUG=661260

Committed: https://crrev.com/890cc54a9df16b8cc36e1d29df9707ec7bed3f24
Cr-Commit-Position: refs/heads/master@{#429473}
==========

[commit-bot: I haz the power, 2016-11-03 00:25:28]

Patchset 6 (id:??) landed as
https://crrev.com/890cc54a9df16b8cc36e1d29df9707ec7bed3f24
Cr-Commit-Position: refs/heads/master@{#429473}