[USS] Add function for update storage key for processor

[USS] Add function for update storage key for processor

Add UpdateStorageKey into processor, so bridge can update storage key in 
processor.



BUG=719623
Review-Url: https://codereview.chromium.org/2864713002
Cr-Commit-Position: refs/heads/master@{#471222}
Committed: https://chromium.googlesource.com/chromium/src/+/6fb560e0be6e124a89f10780668c3d150c0340e4

[Gang Wu, 2017-05-08 17:38:56]

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

[commit-bot: I haz the power, 2017-05-08 17:39:27]

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-05-08 17:49:28]

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

[commit-bot: I haz the power, 2017-05-08 17:49:29]

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

[Gang Wu, 2017-05-08 18:28:56]

Description was changed from

==========
new function

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

to

==========
[USS] Add function for update storage key for processor

Add UpdateStorageKey into processor, so bridge can update storage key in 
processor.



BUG=719623
==========

[Gang Wu, 2017-05-08 18:28:56]

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

[Gang Wu, 2017-05-08 18:29:11]

PTAL

[Gang Wu, 2017-05-08 18:31:06]

Patchset #1 (id:1) has been deleted

[Gang Wu, 2017-05-08 18:31:13]

Patchset #1 (id:20001) has been deleted

[pavely, 2017-05-09 00:49:25]

https://codereview.chromium.org/2864713002/diff/40001/components/sync/model/f...
File components/sync/model/fake_model_type_sync_bridge.cc (left):

https://codereview.chromium.org/2864713002/diff/40001/components/sync/model/f...
components/sync/model/fake_model_type_sync_bridge.cc:265:
EXPECT_TRUE(db_->HasMetadata(kv.first));
Why is this assertion not valid anymore? Is there a valid scenario where it
fails.

https://codereview.chromium.org/2864713002/diff/40001/components/sync/model/m...
File components/sync/model/model_type_change_processor.h (right):

https://codereview.chromium.org/2864713002/diff/40001/components/sync/model/m...
components/sync/model/model_type_change_processor.h:53: // This function should
only be called for the data type which will not create
will not => does not
base on => based on

https://codereview.chromium.org/2864713002/diff/40001/components/sync/model_i...
File components/sync/model_impl/processor_entity_tracker.h (right):

https://codereview.chromium.org/2864713002/diff/40001/components/sync/model_i...
components/sync/model_impl/processor_entity_tracker.h:105: //
ModelTypeChangeProcessor::UpdateStorageKey for the data type which will not
will not => does not
base on => based on

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

https://codereview.chromium.org/2864713002/diff/40001/components/sync/model_i...
components/sync/model_impl/shared_model_type_processor.cc:273: // Update storage
key to tag hash map
nit: End comment sentences with a dot. (the same for comment five lines below)

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

https://codereview.chromium.org/2864713002/diff/40001/components/sync/model_i...
components/sync/model_impl/shared_model_type_processor_unittest.cc:117:
base::Optional<ModelError> MergeSyncData(
The code where this function is copied from had simpler structure:
- loop through local storage to pass local only entities to processor
- loop through remote data to update local storage.

In this implementation "// Store any new remote entities." section is more
complicated.

What if you restructure it into three sections:
- loop through remote data to update storage key for entities with random one
- loop through local storage to pass local only entities to processor
- loop through remote data to update local storage.
(Note, last two bullets are exactly the same as before)

This way implementation is cleaner and allows you to refactor function to avoid
code duplication. It also allows you not to expose error_next_ and
ApplyMetadataChangeList from base class.

https://codereview.chromium.org/2864713002/diff/40001/components/sync/model_i...
components/sync/model_impl/shared_model_type_processor_unittest.cc:148: 
nit: Indentation is off in the next few lines.

https://codereview.chromium.org/2864713002/diff/40001/components/sync/model_i...
components/sync/model_impl/shared_model_type_processor_unittest.cc:161:
base::Optional<ModelError> ApplySyncChanges(
Same as above, is there a way to refactor this function so that you don't have
to copy implementation of FakeModelTypeSyncBridge::ApplySyncChanges and
introduce code duplication?

https://codereview.chromium.org/2864713002/diff/40001/components/sync/model_i...
components/sync/model_impl/shared_model_type_processor_unittest.cc:207: if
(db().HasData(entity_data.specifics.preference().name())) {
Alternatively GetStorageKey could generate either real or random storage key
based on entity data. You could for example partition preference names into
groups by first letter of preference name:
- [a-k]: return preference name
- [l-z]: generate random key

[Gang Wu, 2017-05-09 18:59:17]

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

[commit-bot: I haz the power, 2017-05-09 19:00:22]

Dry run: CQ is trying da patch.

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

[Gang Wu, 2017-05-09 19:02:50]

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

[Gang Wu, 2017-05-09 19:03:03]

Patchset #2 (id:60001) has been deleted

[commit-bot: I haz the power, 2017-05-09 19:04:32]

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-05-09 20:00:10]

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

[commit-bot: I haz the power, 2017-05-09 20:00:11]

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

[Gang Wu, 2017-05-09 22:33:15]

updated, most of tests are passed, seems other jobs failed because try-bots are
broken. will keep an eye on that.

https://codereview.chromium.org/2864713002/diff/40001/components/sync/model/f...
File components/sync/model/fake_model_type_sync_bridge.cc (left):

https://codereview.chromium.org/2864713002/diff/40001/components/sync/model/f...
components/sync/model/fake_model_type_sync_bridge.cc:265:
EXPECT_TRUE(db_->HasMetadata(kv.first));
On 2017/05/09 00:49:24, pavely wrote:
> Why is this assertion not valid anymore? Is there a valid scenario where it
> fails.

yes, since now the processor will add an invalid storage key, and then delete it
and add a valid storage key, so db_ does not have invalid storage key yet when
delete the invalid storage key, so this check will fail in this scenario.

https://codereview.chromium.org/2864713002/diff/40001/components/sync/model/m...
File components/sync/model/model_type_change_processor.h (right):

https://codereview.chromium.org/2864713002/diff/40001/components/sync/model/m...
components/sync/model/model_type_change_processor.h:53: // This function should
only be called for the data type which will not create
On 2017/05/09 00:49:24, pavely wrote:
> will not => does not
> base on => based on

Done.

https://codereview.chromium.org/2864713002/diff/40001/components/sync/model_i...
File components/sync/model_impl/processor_entity_tracker.h (right):

https://codereview.chromium.org/2864713002/diff/40001/components/sync/model_i...
components/sync/model_impl/processor_entity_tracker.h:105: //
ModelTypeChangeProcessor::UpdateStorageKey for the data type which will not
On 2017/05/09 00:49:24, pavely wrote:
> will not => does not
> base on => based on

Done.

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

https://codereview.chromium.org/2864713002/diff/40001/components/sync/model_i...
components/sync/model_impl/shared_model_type_processor.cc:273: // Update storage
key to tag hash map
On 2017/05/09 00:49:24, pavely wrote:
> nit: End comment sentences with a dot. (the same for comment five lines below)

Done.

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

https://codereview.chromium.org/2864713002/diff/40001/components/sync/model_i...
components/sync/model_impl/shared_model_type_processor_unittest.cc:117:
base::Optional<ModelError> MergeSyncData(
On 2017/05/09 00:49:25, pavely wrote:
> The code where this function is copied from had simpler structure:
> - loop through local storage to pass local only entities to processor
> - loop through remote data to update local storage.
> 
> In this implementation "// Store any new remote entities." section is more
> complicated.
> 
> What if you restructure it into three sections:
> - loop through remote data to update storage key for entities with random one
> - loop through local storage to pass local only entities to processor
> - loop through remote data to update local storage.
> (Note, last two bullets are exactly the same as before)
> 
> This way implementation is cleaner and allows you to refactor function to
avoid
> code duplication. It also allows you not to expose error_next_ and
> ApplyMetadataChangeList from base class.

Done.

https://codereview.chromium.org/2864713002/diff/40001/components/sync/model_i...
components/sync/model_impl/shared_model_type_processor_unittest.cc:148: 
On 2017/05/09 00:49:25, pavely wrote:
> nit: Indentation is off in the next few lines.

Done.

https://codereview.chromium.org/2864713002/diff/40001/components/sync/model_i...
components/sync/model_impl/shared_model_type_processor_unittest.cc:161:
base::Optional<ModelError> ApplySyncChanges(
On 2017/05/09 00:49:24, pavely wrote:
> Same as above, is there a way to refactor this function so that you don't have
> to copy implementation of FakeModelTypeSyncBridge::ApplySyncChanges and
> introduce code duplication?

Done.

https://codereview.chromium.org/2864713002/diff/40001/components/sync/model_i...
components/sync/model_impl/shared_model_type_processor_unittest.cc:207: if
(db().HasData(entity_data.specifics.preference().name())) {
On 2017/05/09 00:49:25, pavely wrote:
> Alternatively GetStorageKey could generate either real or random storage key
> based on entity data. You could for example partition preference names into
> groups by first letter of preference name:
> - [a-k]: return preference name
> - [l-z]: generate random key

Done.

[Gang Wu, 2017-05-11 18:48:26]

ping

[pavely, 2017-05-11 20:58:42]

lgtm % couple of nits

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

https://codereview.chromium.org/2864713002/diff/80001/components/sync/model_i...
components/sync/model_impl/shared_model_type_processor_unittest.cc:125:
std::map<std::string, std::string> updated_key;
updated_key => updated_keys

https://codereview.chromium.org/2864713002/diff/80001/components/sync/model_i...
components/sync/model_impl/shared_model_type_processor_unittest.cc:140:
entity_data_map[kv.second] = entity_data_map[kv.first];
nit: I know it is not possible with current code, but just in case, DCHECK that
kv.first != kv.second.

https://codereview.chromium.org/2864713002/diff/80001/components/sync/model_i...
components/sync/model_impl/shared_model_type_processor_unittest.cc:159:
EXPECT_TRUE(iter->type() == EntityChange::ACTION_ADD);
nit: Could you move EXPECT_TRUE right above
change_processor()->UpdateStorageKey(). Usually checks for invariants are
performed at the beginning of the block before any actions.

[Gang Wu, 2017-05-11 21:56:15]

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

[Gang Wu, 2017-05-11 21:56:23]

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

https://codereview.chromium.org/2864713002/diff/80001/components/sync/model_i...
components/sync/model_impl/shared_model_type_processor_unittest.cc:125:
std::map<std::string, std::string> updated_key;
On 2017/05/11 20:58:42, pavely wrote:
> updated_key => updated_keys

Done.

https://codereview.chromium.org/2864713002/diff/80001/components/sync/model_i...
components/sync/model_impl/shared_model_type_processor_unittest.cc:140:
entity_data_map[kv.second] = entity_data_map[kv.first];
On 2017/05/11 20:58:42, pavely wrote:
> nit: I know it is not possible with current code, but just in case, DCHECK
that
> kv.first != kv.second.

Done.

https://codereview.chromium.org/2864713002/diff/80001/components/sync/model_i...
components/sync/model_impl/shared_model_type_processor_unittest.cc:159:
EXPECT_TRUE(iter->type() == EntityChange::ACTION_ADD);
On 2017/05/11 20:58:42, pavely wrote:
> nit: Could you move EXPECT_TRUE right above
> change_processor()->UpdateStorageKey(). Usually checks for invariants are
> performed at the beginning of the block before any actions.

Done.

[commit-bot: I haz the power, 2017-05-11 21:56:59]

Dry run: CQ is trying da patch.

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

[Gang Wu, 2017-05-11 22:09:54]

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

[Gang Wu, 2017-05-11 22:10:03]

Patchset #3 (id:100001) has been deleted

[commit-bot: I haz the power, 2017-05-11 22:10:54]

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-05-11 23:50:02]

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

[commit-bot: I haz the power, 2017-05-11 23:50:02]

Dry run: This issue passed the CQ dry run.

[Gang Wu, 2017-05-12 04:29:17]

The CQ bit was checked by gangwu@chromium.org

[Gang Wu, 2017-05-12 04:29:17]

The patchset sent to the CQ was uploaded after l-g-t-m from pavely@chromium.org
Link to the patchset: https://codereview.chromium.org/2864713002/#ps120001
(title: "nit")

[commit-bot: I haz the power, 2017-05-12 04:29:32]

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-05-12 04:34:26]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1494563357130190,
"parent_rev": "383d1bb3f30b49fcdf3918cefc588637aa678ec0", "commit_rev":
"6fb560e0be6e124a89f10780668c3d150c0340e4"}

[commit-bot: I haz the power, 2017-05-12 04:34:37]

Description was changed from

==========
[USS] Add function for update storage key for processor

Add UpdateStorageKey into processor, so bridge can update storage key in 
processor.



BUG=719623
==========

to

==========
[USS] Add function for update storage key for processor

Add UpdateStorageKey into processor, so bridge can update storage key in 
processor.



BUG=719623

Review-Url: https://codereview.chromium.org/2864713002
Cr-Commit-Position: refs/heads/master@{#471222}
Committed:
https://chromium.googlesource.com/chromium/src/+/6fb560e0be6e124a89f10780668c...
==========

[commit-bot: I haz the power, 2017-05-12 04:34:38]

Committed patchset #3 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/6fb560e0be6e124a89f10780668c...