[Sync] Prepend global metadata key with model type prefix

[Sync] ModelTypeStore: Prepend global metadata key with model type prefix

The issue is that two different model types sharing the same backend use the
same key to store global metadata record.
The fix is to add model type prefix to record key.

R=maxbogue@chromium.org
BUG=657082
Committed: https://crrev.com/89cc2c61dc9cd61a732b2f337c53fbf26c41fbe0
Cr-Commit-Position: refs/heads/master@{#429791}

[pavely, 2016-11-03 19:41:33]

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

[commit-bot: I haz the power, 2016-11-03 19:42:07]

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-03 20:46:39]

Description was changed from

==========
[Sync] Prepend global metadata key with model type prefix

The issue is that two different model types sharing the same backend use the
same key to store global metadata record.
The fix is to add model type prefix to record key.

R=maxbogue@chromium.org
BUG=657082
==========

to

==========
[Sync] ModelTypeStore: Prepend global metadata key with model type prefix

The issue is that two different model types sharing the same backend use the
same key to store global metadata record.
The fix is to add model type prefix to record key.

R=maxbogue@chromium.org
BUG=657082
==========

[skym, 2016-11-03 21:03:39]

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

[skym, 2016-11-03 21:03:40]

lgtm

https://codereview.chromium.org/2468183005/diff/20001/components/sync/model_i...
File components/sync/model_impl/model_type_store_impl.cc (right):

https://codereview.chromium.org/2468183005/diff/20001/components/sync/model_i...
components/sync/model_impl/model_type_store_impl.cc:35: // Format prefix key for
data/metadata records with |type|.
This comment is a little weird. It's applying the the following two functions,
but there's a new line between them, and then FormatGlobalMetadataKey doesn't
get anything. I'd prefer a single comment above each of the three.

https://codereview.chromium.org/2468183005/diff/20001/components/sync/model_i...
components/sync/model_impl/model_type_store_impl.cc:105: const ModelType type,
I'm okay with this const, shows intent.

https://codereview.chromium.org/2468183005/diff/20001/components/sync/model_i...
File components/sync/model_impl/model_type_store_impl.h (right):

https://codereview.chromium.org/2468183005/diff/20001/components/sync/model_i...
components/sync/model_impl/model_type_store_impl.h:37: static void
CreateInMemoryStoreForTest(const ModelType type,
const doesn't mean much here. This is the declaration of a function for a pass
by value struct.

[commit-bot: I haz the power, 2016-11-03 21:10:27]

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

[commit-bot: I haz the power, 2016-11-03 21:10:27]

Dry run: This issue passed the CQ dry run.

[maxbogue, 2016-11-03 21:31:53]

lgtm

https://codereview.chromium.org/2468183005/diff/20001/components/sync/model_i...
File components/sync/model_impl/model_type_store_impl.cc (right):

https://codereview.chromium.org/2468183005/diff/20001/components/sync/model_i...
components/sync/model_impl/model_type_store_impl.cc:35: // Format prefix key for
data/metadata records with |type|.
On 2016/11/03 21:03:40, skym wrote:
> This comment is a little weird. It's applying the the following two functions,
> but there's a new line between them, and then FormatGlobalMetadataKey doesn't
> get anything. I'd prefer a single comment above each of the three.

+1

https://codereview.chromium.org/2468183005/diff/20001/components/sync/model_i...
File components/sync/model_impl/model_type_store_impl.h (right):

https://codereview.chromium.org/2468183005/diff/20001/components/sync/model_i...
components/sync/model_impl/model_type_store_impl.h:37: static void
CreateInMemoryStoreForTest(const ModelType type,
On 2016/11/03 21:03:40, skym wrote:
> const doesn't mean much here. This is the declaration of a function for a pass
> by value struct.

I'd rather have the signatures match but I don't really care.

[pavely, 2016-11-03 23:25:52]

https://codereview.chromium.org/2468183005/diff/20001/components/sync/model_i...
File components/sync/model_impl/model_type_store_impl.cc (right):

https://codereview.chromium.org/2468183005/diff/20001/components/sync/model_i...
components/sync/model_impl/model_type_store_impl.cc:35: // Format prefix key for
data/metadata records with |type|.
On 2016/11/03 21:31:53, maxbogue wrote:
> On 2016/11/03 21:03:40, skym wrote:
> > This comment is a little weird. It's applying the the following two
functions,
> > but there's a new line between them, and then FormatGlobalMetadataKey
doesn't
> > get anything. I'd prefer a single comment above each of the three.
> 
> +1

I moved this comment from header file. I'd rather get rid of it altogether, it
restates what is written in function name and one line body.
Anyway, adjusted comments.

https://codereview.chromium.org/2468183005/diff/20001/components/sync/model_i...
components/sync/model_impl/model_type_store_impl.cc:105: const ModelType type,
On 2016/11/03 21:03:40, skym wrote:
> I'm okay with this const, shows intent.

Removed.

https://codereview.chromium.org/2468183005/diff/20001/components/sync/model_i...
File components/sync/model_impl/model_type_store_impl.h (right):

https://codereview.chromium.org/2468183005/diff/20001/components/sync/model_i...
components/sync/model_impl/model_type_store_impl.h:37: static void
CreateInMemoryStoreForTest(const ModelType type,
On 2016/11/03 21:31:53, maxbogue wrote:
> On 2016/11/03 21:03:40, skym wrote:
> > const doesn't mean much here. This is the declaration of a function for a
pass
> > by value struct.
> 
> I'd rather have the signatures match but I don't really care.

I made it const to match signature of CreateStore, but it is not typical for me
to pass parameter by const value: I use either value, pointer or const
reference.
Beats me how I could have possibly written it this way in CreateStore().

Anyway... I fixed instances of const ModelType I ran across.

[pavely, 2016-11-03 23:26:21]

The CQ bit was checked by pavely@chromium.org

[pavely, 2016-11-03 23:26:22]

The patchset sent to the CQ was uploaded after l-g-t-m from
maxbogue@chromium.org, skym@chromium.org
Link to the patchset: https://codereview.chromium.org/2468183005/#ps40001
(title: "Addressed comments.")

[commit-bot: I haz the power, 2016-11-03 23:27:38]

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-04 03:03:18]

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

[commit-bot: I haz the power, 2016-11-04 03:03:19]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[maxbogue, 2016-11-04 03:52:26]

The CQ bit was checked by maxbogue@chromium.org

[commit-bot: I haz the power, 2016-11-04 03:53:04]

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-04 05:18:37]

Description was changed from

==========
[Sync] ModelTypeStore: Prepend global metadata key with model type prefix

The issue is that two different model types sharing the same backend use the
same key to store global metadata record.
The fix is to add model type prefix to record key.

R=maxbogue@chromium.org
BUG=657082
==========

to

==========
[Sync] ModelTypeStore: Prepend global metadata key with model type prefix

The issue is that two different model types sharing the same backend use the
same key to store global metadata record.
The fix is to add model type prefix to record key.

R=maxbogue@chromium.org
BUG=657082
==========

[commit-bot: I haz the power, 2016-11-04 05:18:37]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-11-04 05:21:25]

Description was changed from

==========
[Sync] ModelTypeStore: Prepend global metadata key with model type prefix

The issue is that two different model types sharing the same backend use the
same key to store global metadata record.
The fix is to add model type prefix to record key.

R=maxbogue@chromium.org
BUG=657082
==========

to

==========
[Sync] ModelTypeStore: Prepend global metadata key with model type prefix

The issue is that two different model types sharing the same backend use the
same key to store global metadata record.
The fix is to add model type prefix to record key.

R=maxbogue@chromium.org
BUG=657082

Committed: https://crrev.com/89cc2c61dc9cd61a732b2f337c53fbf26c41fbe0
Cr-Commit-Position: refs/heads/master@{#429791}
==========

[commit-bot: I haz the power, 2016-11-04 05:21:26]

Patchset 3 (id:??) landed as
https://crrev.com/89cc2c61dc9cd61a732b2f337c53fbf26c41fbe0
Cr-Commit-Position: refs/heads/master@{#429791}