[USS] Store supports hosting multiple datatypes per database

This is 1 of 2 checks in for making ModelTypeStore supports
multiple datatypes.
This one will add ModelType's toor tag as a prefix for keys
in leveldb.
Next check in will make only one ModelTypeStore to be created
if there are multiple datatypes.

BUG=615214
Committed: https://crrev.com/95868e7bb69e4c5cfb90e7c6ea571c351b96f626
Cr-Commit-Position: refs/heads/master@{#398457}

[Gang Wu, 2016-05-26 21:34:08]

Description was changed from

==========
all code first check in


try to_string

BUG=
==========

to

==========
This is 1 of 2 check in for making ModelTypeStore supports
multiple datatypes.
This one will add ModelType's toor tag as a prefix for keys
in leveldb.
Next check in will make only one ModelTypeStore to be created
if there are multiple datatype.

BUG=615214
==========

[Gang Wu, 2016-05-26 21:34:09]

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

[Gang Wu, 2016-05-26 21:34:54]

Description was changed from

==========
This is 1 of 2 check in for making ModelTypeStore supports
multiple datatypes.
This one will add ModelType's toor tag as a prefix for keys
in leveldb.
Next check in will make only one ModelTypeStore to be created
if there are multiple datatype.

BUG=615214
==========

to

==========
This is 1 of 2 check in for making ModelTypeStore supports
multiple datatypes.
This one will add ModelType's toor tag as a prefix for keys
in leveldb.
Next check in will make only one ModelTypeStore to be created
if there are multiple datatypes.

BUG=615214
==========

[Gang Wu, 2016-05-26 21:39:24]

Description was changed from

==========
This is 1 of 2 check in for making ModelTypeStore supports
multiple datatypes.
This one will add ModelType's toor tag as a prefix for keys
in leveldb.
Next check in will make only one ModelTypeStore to be created
if there are multiple datatypes.

BUG=615214
==========

to

==========
This is 1 of 2 checks in for making ModelTypeStore supports
multiple datatypes.
This one will add ModelType's toor tag as a prefix for keys
in leveldb.
Next check in will make only one ModelTypeStore to be created
if there are multiple datatypes.

BUG=615214
==========

[Gang Wu, 2016-05-26 21:53:41]

PTAL

[maxbogue, 2016-05-27 14:23:41]

Did you consider a solution that didn't involve adding the type as a parameter
to all of the methods of ModelTypeStore? For example, what if there was a
generic sync level db object of some sort that held the connection to the
database and looked like the ModelTypeStoreImpl class in this patch, but it
*didn't* implement ModelTypeStore. Instead it would have a method that took a
type and dispensed a wrapper around itself that implemented ModelTypeStore and
provided the type to all the actual methods?

There are other ways to do it too, but it seems less than ideal for the service
to have to pass the type to every call (and also somewhat fragile; the service
code shouldn't be able to write into another type's namespace by accident).

[Gang Wu, 2016-05-27 19:52:24]

updated.
move model type into ctor.

[maxbogue, 2016-05-30 15:34:50]

Mostly lg, just a couple comments.

https://codereview.chromium.org/2018623002/diff/40001/sync/internal_api/model...
File sync/internal_api/model_type_store_impl.cc (right):

https://codereview.chromium.org/2018623002/diff/40001/sync/internal_api/model...
sync/internal_api/model_type_store_impl.cc:39: return
std::string(syncer::kModelTypeInfoMap[type].root_tag) + kDataPrefix;
Computing the prefix could just be done once per object in the constructor
instead of once for every call.

https://codereview.chromium.org/2018623002/diff/40001/sync/internal_api/publi...
File sync/internal_api/public/base/model_type.h (right):

https://codereview.chromium.org/2018623002/diff/40001/sync/internal_api/publi...
sync/internal_api/public/base/model_type.h:165: struct ModelTypeInfo {
Instead of exposing this data structure, I'd prefer you just make an accessor
"ModelTypeToTag" or something like that. Exposing this creates more than one way
to access a lot of the other data fields, which we don't really want.

[Gang Wu, 2016-06-07 20:20:33]

updated.

https://codereview.chromium.org/2018623002/diff/40001/sync/internal_api/model...
File sync/internal_api/model_type_store_impl.cc (right):

https://codereview.chromium.org/2018623002/diff/40001/sync/internal_api/model...
sync/internal_api/model_type_store_impl.cc:39: return
std::string(syncer::kModelTypeInfoMap[type].root_tag) + kDataPrefix;
On 2016/05/30 15:34:50, maxbogue wrote:
> Computing the prefix could just be done once per object in the constructor
> instead of once for every call.

Done.

https://codereview.chromium.org/2018623002/diff/40001/sync/internal_api/publi...
File sync/internal_api/public/base/model_type.h (right):

https://codereview.chromium.org/2018623002/diff/40001/sync/internal_api/publi...
sync/internal_api/public/base/model_type.h:165: struct ModelTypeInfo {
On 2016/05/30 15:34:50, maxbogue wrote:
> Instead of exposing this data structure, I'd prefer you just make an accessor
> "ModelTypeToTag" or something like that. Exposing this creates more than one
way
> to access a lot of the other data fields, which we don't really want.

Done.

[maxbogue, 2016-06-07 23:35:09]

lgtm with a couple nits

https://codereview.chromium.org/2018623002/diff/60001/sync/internal_api/publi...
File sync/internal_api/public/base/model_type.h (right):

https://codereview.chromium.org/2018623002/diff/60001/sync/internal_api/publi...
sync/internal_api/public/base/model_type.h:357: // Returns root_tag for
|model_type| in ModelTypeInfo.
Explain the difference between this and *ToRootTag, since that already exists.

https://codereview.chromium.org/2018623002/diff/60001/sync/internal_api/publi...
File sync/internal_api/public/model_type_store_impl.h (right):

https://codereview.chromium.org/2018623002/diff/60001/sync/internal_api/publi...
sync/internal_api/public/model_type_store_impl.h:111: const std::string
dataPrefix_;
Comment above these?

[Gang Wu, 2016-06-08 00:11:38]

Patchset #5 (id:80001) has been deleted

[pavely, 2016-06-08 01:18:07]

https://codereview.chromium.org/2018623002/diff/60001/components/browser_sync...
File components/browser_sync/browser/profile_sync_service.cc (right):

https://codereview.chromium.org/2018623002/diff/60001/components/browser_sync...
components/browser_sync/browser/profile_sync_service.cc:302:
base::Bind(&ModelTypeStore::CreateStore, syncer::DEVICE_INFO,
Optional comment: If PSS performed conversion from ModelType to RootTag here and
passed tag to store it would make store completely oblivious of ModelType
existence (one less dependency).

https://codereview.chromium.org/2018623002/diff/60001/sync/internal_api/model...
File sync/internal_api/model_type_store_impl.cc (right):

https://codereview.chromium.org/2018623002/diff/60001/sync/internal_api/model...
sync/internal_api/model_type_store_impl.cc:39: return
std::string(syncer::ModelTypeToTag(type)) + kDataPrefix;
syncer::ModelTypeToTag(type) already returns string, no need to contruct new one
out of it.

https://codereview.chromium.org/2018623002/diff/60001/sync/internal_api/publi...
File sync/internal_api/public/base/model_type.h (right):

https://codereview.chromium.org/2018623002/diff/60001/sync/internal_api/publi...
sync/internal_api/public/base/model_type.h:357: // Returns root_tag for
|model_type| in ModelTypeInfo.
On 2016/06/07 23:35:09, maxbogue wrote:
> Explain the difference between this and *ToRootTag, since that already exists.

Could you place this function right next to ModelTypeToRootTag.
You can make it distinct by naming it for how it is used (e.g.
ModelTypeToStorePrefix) rather than what it returns (ModelTypeToRootTag).

https://codereview.chromium.org/2018623002/diff/60001/sync/internal_api/publi...
File sync/internal_api/public/model_type_store_impl.h (right):

https://codereview.chromium.org/2018623002/diff/60001/sync/internal_api/publi...
sync/internal_api/public/model_type_store_impl.h:111: const std::string
dataPrefix_;
On 2016/06/07 23:35:09, maxbogue wrote:
> Comment above these?

Also follow naming conventions: data_prefix_

[Gang Wu, 2016-06-08 01:29:28]

update base on comments

https://codereview.chromium.org/2018623002/diff/60001/sync/internal_api/publi...
File sync/internal_api/public/base/model_type.h (right):

https://codereview.chromium.org/2018623002/diff/60001/sync/internal_api/publi...
sync/internal_api/public/base/model_type.h:357: // Returns root_tag for
|model_type| in ModelTypeInfo.
On 2016/06/07 23:35:09, maxbogue wrote:
> Explain the difference between this and *ToRootTag, since that already exists.

Done.

https://codereview.chromium.org/2018623002/diff/60001/sync/internal_api/publi...
File sync/internal_api/public/model_type_store_impl.h (right):

https://codereview.chromium.org/2018623002/diff/60001/sync/internal_api/publi...
sync/internal_api/public/model_type_store_impl.h:111: const std::string
dataPrefix_;
On 2016/06/07 23:35:09, maxbogue wrote:
> Comment above these?

Done.

[Gang Wu, 2016-06-08 01:29:53]

The CQ bit was checked by gangwu@chromium.org

[Gang Wu, 2016-06-08 01:29:53]

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/2018623002/#ps100001
(title: " ")

[commit-bot: I haz the power, 2016-06-08 01:30:19]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2018623002/100001

[commit-bot: I haz the power, 2016-06-08 01:54:46]

Description was changed from

==========
This is 1 of 2 checks in for making ModelTypeStore supports
multiple datatypes.
This one will add ModelType's toor tag as a prefix for keys
in leveldb.
Next check in will make only one ModelTypeStore to be created
if there are multiple datatypes.

BUG=615214
==========

to

==========
This is 1 of 2 checks in for making ModelTypeStore supports
multiple datatypes.
This one will add ModelType's toor tag as a prefix for keys
in leveldb.
Next check in will make only one ModelTypeStore to be created
if there are multiple datatypes.

BUG=615214
==========

[commit-bot: I haz the power, 2016-06-08 01:54:47]

Committed patchset #5 (id:100001)

[commit-bot: I haz the power, 2016-06-08 01:57:11]

Description was changed from

==========
This is 1 of 2 checks in for making ModelTypeStore supports
multiple datatypes.
This one will add ModelType's toor tag as a prefix for keys
in leveldb.
Next check in will make only one ModelTypeStore to be created
if there are multiple datatypes.

BUG=615214
==========

to

==========
This is 1 of 2 checks in for making ModelTypeStore supports
multiple datatypes.
This one will add ModelType's toor tag as a prefix for keys
in leveldb.
Next check in will make only one ModelTypeStore to be created
if there are multiple datatypes.

BUG=615214

Committed: https://crrev.com/95868e7bb69e4c5cfb90e7c6ea571c351b96f626
Cr-Commit-Position: refs/heads/master@{#398457}
==========

[commit-bot: I haz the power, 2016-06-08 01:57:12]

Patchset 5 (id:??) landed as
https://crrev.com/95868e7bb69e4c5cfb90e7c6ea571c351b96f626
Cr-Commit-Position: refs/heads/master@{#398457}

[Gang Wu, 2016-06-20 21:39:21]

https://codereview.chromium.org/2018623002/diff/60001/sync/internal_api/model...
File sync/internal_api/model_type_store_impl.cc (right):

https://codereview.chromium.org/2018623002/diff/60001/sync/internal_api/model...
sync/internal_api/model_type_store_impl.cc:39: return
std::string(syncer::ModelTypeToTag(type)) + kDataPrefix;
On 2016/06/08 01:18:07, pavely wrote:
> syncer::ModelTypeToTag(type) already returns string, no need to contruct new
one
> out of it.
ModelTypeToTag(type) return const char*

https://codereview.chromium.org/2018623002/diff/60001/sync/internal_api/publi...
File sync/internal_api/public/base/model_type.h (right):

https://codereview.chromium.org/2018623002/diff/60001/sync/internal_api/publi...
sync/internal_api/public/base/model_type.h:357: // Returns root_tag for
|model_type| in ModelTypeInfo.
On 2016/06/08 01:18:07, pavely wrote:
> On 2016/06/07 23:35:09, maxbogue wrote:
> > Explain the difference between this and *ToRootTag, since that already
exists.
> 
> Could you place this function right next to ModelTypeToRootTag.
> You can make it distinct by naming it for how it is used (e.g.
> ModelTypeToStorePrefix) rather than what it returns (ModelTypeToRootTag).

Done.

https://codereview.chromium.org/2018623002/diff/60001/sync/internal_api/publi...
File sync/internal_api/public/model_type_store_impl.h (right):

https://codereview.chromium.org/2018623002/diff/60001/sync/internal_api/publi...
sync/internal_api/public/model_type_store_impl.h:111: const std::string
dataPrefix_;
On 2016/06/08 01:18:07, pavely wrote:
> On 2016/06/07 23:35:09, maxbogue wrote:
> > Comment above these?
> 
> Also follow naming conventions: data_prefix_

Done.