[Sync] Adding storage key concept for ModelTypeServices.

Introducing the concept for a storage key for ModelTypeServices, which is used to communicate with the processor and independent of client tag and client tag hashes.

Ideally the services will be able to use any object, not just a std::string. My intention is that this can be done in a subsequent change, if we decide we still want to do so.

BUG=636140
Committed: https://crrev.com/b9932b5ad0b4632a7d651ae3442241ec759c5293
Cr-Commit-Position: refs/heads/master@{#411438}

[skym, 2016-08-09 23:22:47]

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

[commit-bot: I haz the power, 2016-08-09 23:23:54]

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

[skym, 2016-08-09 23:24:15]

Description was changed from

==========
Introducing the concept for a storage key for ModelTypeServices, which
is used to communicate with the processor and indepedant of client tag
and client tag hashes.

BUG=636140
==========

to

==========
Introducing the concept for a storage key for ModelTypeServices, which is used
to communicate with the processor and independent of client tag and client tag
hashes.

Ideally the services will be able to use any object, not just a std::string. My
intention is that this can be done in a subsequent change, if we decide we still
want to do so.

BUG=636140
==========

[skym, 2016-08-09 23:24:15]

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

[skym, 2016-08-09 23:25:30]

PTAL. I'm not super happy with how some of the processor's unittest came out, I
think having the WriteItem methods return a specifics object would work out
better than it did. Let me know if you have suggestions of how to do things more
elegantly.

[commit-bot: I haz the power, 2016-08-10 00:23:26]

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

[commit-bot: I haz the power, 2016-08-10 00:23:27]

Dry run: This issue passed the CQ dry run.

[skym, 2016-08-10 19:23:23]

https://codereview.chromium.org/2222373003/diff/20001/components/sync_driver/...
File components/sync_driver/device_info_service_unittest.cc (right):

https://codereview.chromium.org/2222373003/diff/20001/components/sync_driver/...
components/sync_driver/device_info_service_unittest.cc:258: // pushes them onto
the given change list. The corresponding client tag the
Some "tag" naming that didn't get updated here.

https://codereview.chromium.org/2222373003/diff/20001/components/sync_driver/...
components/sync_driver/device_info_service_unittest.cc:260: // service to
generate the tag.
And here.

https://codereview.chromium.org/2222373003/diff/20001/components/sync_driver/...
components/sync_driver/device_info_service_unittest.cc:502:
storage_keys.push_back("tag1");
And here.

[maxbogue, 2016-08-10 23:56:03]

Mostly looks good!

https://codereview.chromium.org/2222373003/diff/20001/components/sync/api/mod...
File components/sync/api/model_type_service.h (right):

https://codereview.chromium.org/2222373003/diff/20001/components/sync/api/mod...
components/sync/api/model_type_service.h:85: // that was/would have been
generated in the SyncableService/Directory world.
"world for backward-compatibility with pre-USS clients."

https://codereview.chromium.org/2222373003/diff/20001/components/sync/api/mod...
components/sync/api/model_type_service.h:87: // local data, however we also call
and verify on remote data to make sure the
"however it is also used to verify the hash of remote data."

https://codereview.chromium.org/2222373003/diff/20001/components/sync/api/mod...
components/sync/api/model_type_service.h:93: // called once when first
encountering a remote entity. Local changes should
"Local changes will provide their storage keys directly to Put instead of using
this method."

https://codereview.chromium.org/2222373003/diff/20001/components/sync/core/sh...
File components/sync/core/shared_model_type_processor.h (right):

https://codereview.chromium.org/2222373003/diff/20001/components/sync/core/sh...
components/sync/core/shared_model_type_processor.h:127: std::string
GetClientTagHash(const std::string storage_key,
const std::string&

https://codereview.chromium.org/2222373003/diff/20001/components/sync/core/sh...
File components/sync/core/shared_model_type_processor_unittest.cc (right):

https://codereview.chromium.org/2222373003/diff/20001/components/sync/core/sh...
components/sync/core/shared_model_type_processor_unittest.cc:42: std::string
StorageKeyFromName(const std::string& name) {
Just use the key as the name.

https://codereview.chromium.org/2222373003/diff/20001/components/sync/core/sh...
components/sync/core/shared_model_type_processor_unittest.cc:51: const char
kName1[] = "name1";
I think it will make everything simpler if these are all keys instead of names,
and then we just happen to reuse the key as the name instead of vice-versa. The
name is not important; the key is.

https://codereview.chromium.org/2222373003/diff/20001/components/sync/core/sh...
components/sync/core/shared_model_type_processor_unittest.cc:260:
sync_pb::EntitySpecifics WriteItem(const std::string& name,
No need to return specifics.

https://codereview.chromium.org/2222373003/diff/20001/components/sync/test/en...
File components/sync/test/engine/mock_model_type_worker.h (right):

https://codereview.chromium.org/2222373003/diff/20001/components/sync/test/en...
components/sync/test/engine/mock_model_type_worker.h:30: class
MockModelTypeWorker : public CommitQueue {
As discussed offline, I would like this to use key and value instead of hash and
specifics. I know there are a couple oddities with it but it simplifies the test
case code.

https://codereview.chromium.org/2222373003/diff/20001/components/sync_driver/...
File components/sync_driver/device_info_service_unittest.cc (right):

https://codereview.chromium.org/2222373003/diff/20001/components/sync_driver/...
components/sync_driver/device_info_service_unittest.cc:658: const
DeviceInfoSpecifics unique_local(GenerateTestSpecifics("unique_local"));
Could you create const strings here for the three strings that are re-used in
this test case?

[skym, 2016-08-11 16:27:39]

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

[commit-bot: I haz the power, 2016-08-11 16:28:13]

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

[skym, 2016-08-11 16:28:48]

https://codereview.chromium.org/2222373003/diff/20001/components/sync/api/mod...
File components/sync/api/model_type_service.h (right):

https://codereview.chromium.org/2222373003/diff/20001/components/sync/api/mod...
components/sync/api/model_type_service.h:85: // that was/would have been
generated in the SyncableService/Directory world.
On 2016/08/10 23:56:02, maxbogue wrote:
> "world for backward-compatibility with pre-USS clients."

Done.

https://codereview.chromium.org/2222373003/diff/20001/components/sync/api/mod...
components/sync/api/model_type_service.h:87: // local data, however we also call
and verify on remote data to make sure the
On 2016/08/10 23:56:02, maxbogue wrote:
> "however it is also used to verify the hash of remote data."

Done.

https://codereview.chromium.org/2222373003/diff/20001/components/sync/api/mod...
components/sync/api/model_type_service.h:93: // called once when first
encountering a remote entity. Local changes should
On 2016/08/10 23:56:02, maxbogue wrote:
> "Local changes will provide their storage keys directly to Put instead of
using
> this method."

Done.

https://codereview.chromium.org/2222373003/diff/20001/components/sync/core/sh...
File components/sync/core/shared_model_type_processor.h (right):

https://codereview.chromium.org/2222373003/diff/20001/components/sync/core/sh...
components/sync/core/shared_model_type_processor.h:127: std::string
GetClientTagHash(const std::string storage_key,
On 2016/08/10 23:56:02, maxbogue wrote:
> const std::string&

Done.

https://codereview.chromium.org/2222373003/diff/20001/components/sync/core/sh...
File components/sync/core/shared_model_type_processor_unittest.cc (right):

https://codereview.chromium.org/2222373003/diff/20001/components/sync/core/sh...
components/sync/core/shared_model_type_processor_unittest.cc:42: std::string
StorageKeyFromName(const std::string& name) {
On 2016/08/10 23:56:03, maxbogue wrote:
> Just use the key as the name.

Done.

https://codereview.chromium.org/2222373003/diff/20001/components/sync/core/sh...
components/sync/core/shared_model_type_processor_unittest.cc:51: const char
kName1[] = "name1";
On 2016/08/10 23:56:03, maxbogue wrote:
> I think it will make everything simpler if these are all keys instead of
names,
> and then we just happen to reuse the key as the name instead of vice-versa.
The
> name is not important; the key is.

Done.

https://codereview.chromium.org/2222373003/diff/20001/components/sync/core/sh...
components/sync/core/shared_model_type_processor_unittest.cc:260:
sync_pb::EntitySpecifics WriteItem(const std::string& name,
On 2016/08/10 23:56:03, maxbogue wrote:
> No need to return specifics.

Acknowledged.

https://codereview.chromium.org/2222373003/diff/20001/components/sync/test/en...
File components/sync/test/engine/mock_model_type_worker.h (right):

https://codereview.chromium.org/2222373003/diff/20001/components/sync/test/en...
components/sync/test/engine/mock_model_type_worker.h:30: class
MockModelTypeWorker : public CommitQueue {
On 2016/08/10 23:56:03, maxbogue wrote:
> As discussed offline, I would like this to use key and value instead of hash
and
> specifics. I know there are a couple oddities with it but it simplifies the
test
> case code.

Lets talk more about this.

https://codereview.chromium.org/2222373003/diff/20001/components/sync_driver/...
File components/sync_driver/device_info_service_unittest.cc (right):

https://codereview.chromium.org/2222373003/diff/20001/components/sync_driver/...
components/sync_driver/device_info_service_unittest.cc:258: // pushes them onto
the given change list. The corresponding client tag the
On 2016/08/10 19:23:23, skym wrote:
> Some "tag" naming that didn't get updated here.

Done.

https://codereview.chromium.org/2222373003/diff/20001/components/sync_driver/...
components/sync_driver/device_info_service_unittest.cc:260: // service to
generate the tag.
On 2016/08/10 19:23:23, skym wrote:
> And here.

Done.

https://codereview.chromium.org/2222373003/diff/20001/components/sync_driver/...
components/sync_driver/device_info_service_unittest.cc:502:
storage_keys.push_back("tag1");
On 2016/08/10 19:23:23, skym wrote:
> And here.

Done.

https://codereview.chromium.org/2222373003/diff/20001/components/sync_driver/...
components/sync_driver/device_info_service_unittest.cc:658: const
DeviceInfoSpecifics unique_local(GenerateTestSpecifics("unique_local"));
On 2016/08/10 23:56:03, maxbogue wrote:
> Could you create const strings here for the three strings that are re-used in
> this test case?

Kinda done, removed all the duplicated string usages.

[commit-bot: I haz the power, 2016-08-11 17:50:48]

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

[commit-bot: I haz the power, 2016-08-11 17:50:49]

Dry run: This issue passed the CQ dry run.

[maxbogue, 2016-08-11 18:53:54]

lgtm!

https://codereview.chromium.org/2222373003/diff/40001/components/sync/core/sh...
File components/sync/core/shared_model_type_processor_unittest.cc (right):

https://codereview.chromium.org/2222373003/diff/40001/components/sync/core/sh...
components/sync/core/shared_model_type_processor_unittest.cc:633:
EntitySpecifics specifics1(ResetStateWriteItem(kKey1, kValue1));
Per offline discussion, can we switch to "specificsN = " for all these? :)

https://codereview.chromium.org/2222373003/diff/40001/components/sync/core/sh...
components/sync/core/shared_model_type_processor_unittest.cc:892: const
std::string kKey3Hash = TagHashFromKey(kKey3);
Isn't this just kHash3...?

[skym, 2016-08-11 18:58:56]

https://codereview.chromium.org/2222373003/diff/40001/components/sync/core/sh...
File components/sync/core/shared_model_type_processor_unittest.cc (right):

https://codereview.chromium.org/2222373003/diff/40001/components/sync/core/sh...
components/sync/core/shared_model_type_processor_unittest.cc:633:
EntitySpecifics specifics1(ResetStateWriteItem(kKey1, kValue1));
On 2016/08/11 18:53:54, maxbogue wrote:
> Per offline discussion, can we switch to "specificsN = " for all these? :)

Done.

https://codereview.chromium.org/2222373003/diff/40001/components/sync/core/sh...
components/sync/core/shared_model_type_processor_unittest.cc:892: const
std::string kKey3Hash = TagHashFromKey(kKey3);
On 2016/08/11 18:53:54, maxbogue wrote:
> Isn't this just kHash3...?

Done.

[skym, 2016-08-11 18:59:01]

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

[commit-bot: I haz the power, 2016-08-11 18:59:33]

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, 2016-08-11 21:18:32]

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

[commit-bot: I haz the power, 2016-08-11 21:18:33]

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_...)

[skym1, 2016-08-11 21:19:02]

The CQ bit was checked by skym@google.com

[skym1, 2016-08-11 21:19:03]

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/2222373003/#ps80001
(title: "Removing redundant hash value.")

[commit-bot: I haz the power, 2016-08-11 21:20:37]

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-08-11 23:31:05]

Description was changed from

==========
Introducing the concept for a storage key for ModelTypeServices, which is used
to communicate with the processor and independent of client tag and client tag
hashes.

Ideally the services will be able to use any object, not just a std::string. My
intention is that this can be done in a subsequent change, if we decide we still
want to do so.

BUG=636140
==========

to

==========
Introducing the concept for a storage key for ModelTypeServices, which is used
to communicate with the processor and independent of client tag and client tag
hashes.

Ideally the services will be able to use any object, not just a std::string. My
intention is that this can be done in a subsequent change, if we decide we still
want to do so.

BUG=636140
==========

[commit-bot: I haz the power, 2016-08-11 23:31:06]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-08-11 23:34:14]

Description was changed from

==========
Introducing the concept for a storage key for ModelTypeServices, which is used
to communicate with the processor and independent of client tag and client tag
hashes.

Ideally the services will be able to use any object, not just a std::string. My
intention is that this can be done in a subsequent change, if we decide we still
want to do so.

BUG=636140
==========

to

==========
Introducing the concept for a storage key for ModelTypeServices, which is used
to communicate with the processor and independent of client tag and client tag
hashes.

Ideally the services will be able to use any object, not just a std::string. My
intention is that this can be done in a subsequent change, if we decide we still
want to do so.

BUG=636140

Committed: https://crrev.com/b9932b5ad0b4632a7d651ae3442241ec759c5293
Cr-Commit-Position: refs/heads/master@{#411438}
==========

[commit-bot: I haz the power, 2016-08-11 23:34:15]

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