[Sync] Implement support for updating storage key for new entities

[Sync] Implement support for updating storage key for new entities

TypedURL's storage key is ROWID which can only be obtained after url record is
inserted into history database. Processor interface should allow bridge to
update storage key of new entity after it was processed by Merge/Apply.

In this change:
- ModelTypeSyncBridge::SupportsGetStorageKey indicates if processor should
call GetStorageKey or rely on bridge to call UpdateStorageKey instead.
- MergeSyncData takes list of changes. The reason is that it needs to support
both types of bridges and EntityChange conveniently includes storage key.
- MergeSyncData currently has implementation that converts change list into
EntityDataMap. In a separate change I'll switch existing bridge
implementations, remove conversion code and method that takes map.
- ProcessorEntityTracker can have empty storage key. It can only be changed to
non-empty one once.
- ModelTypeChangeProcessor::UpdateStorageKey takes EntityData to identify
entity that corresponds to storage key.
- SMTP::OnUpdateReceived handles entities with empty storage keys and ensures
that all storage keys are populated by bridge.
- I refactored SharedModelTypeProcessor::ProcessUpdate to group actios
performed on entities.

R=skym@chromium.org
BUG=719570
Review-Url: https://codereview.chromium.org/2915763005
Cr-Commit-Position: refs/heads/master@{#477113}
Committed: https://chromium.googlesource.com/chromium/src/+/b877904be7dd60869488a48b722fbcf41986ea87

[pavely, 2017-05-31 18:23:51]

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

[commit-bot: I haz the power, 2017-05-31 18:24:43]

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-31 19:16:32]

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

[commit-bot: I haz the power, 2017-05-31 19:16:33]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)

[skym, 2017-05-31 19:53:12]

Looks awesome, love the upgrade/migration story here. Only concern is how this
interacts with encryption.

https://codereview.chromium.org/2915763005/diff/1/components/sync/model/entit...
File components/sync/model/entity_change.h (right):

https://codereview.chromium.org/2915763005/diff/1/components/sync/model/entit...
components/sync/model/entity_change.h:31: // TODO(pavely): data_ptr() is added
temporarily to support converting
Can we get a tracking bug for removing this?

https://codereview.chromium.org/2915763005/diff/1/components/sync/model/model...
File components/sync/model/model_type_sync_bridge.h (right):

https://codereview.chromium.org/2915763005/diff/1/components/sync/model/model...
components/sync/model/model_type_sync_bridge.h:57: //
change_processor()->UpdateStorageKey() iun this case. If a local and sync
iun

https://codereview.chromium.org/2915763005/diff/1/components/sync/model/model...
components/sync/model/model_type_sync_bridge.h:85: // TODO(pavely): This
function should be removed once all bridge
Again, bug number please

https://codereview.chromium.org/2915763005/diff/1/components/sync/model_impl/...
File components/sync/model_impl/shared_model_type_processor.cc (right):

https://codereview.chromium.org/2915763005/diff/1/components/sync/model_impl/...
components/sync/model_impl/shared_model_type_processor.cc:368: // The update
should be ignored.
Why?

https://codereview.chromium.org/2915763005/diff/1/components/sync/model_impl/...
components/sync/model_impl/shared_model_type_processor.cc:371: 
The newlines seem a bit odd here. Maybe group the continues together?

https://codereview.chromium.org/2915763005/diff/1/components/sync/model_impl/...
components/sync/model_impl/shared_model_type_processor.cc:389:
RecommitAllForEncryption(already_updated, metadata_changes.get());
I'm worried this doesn't work correctly anymore, since it's before the Apply
call.

https://codereview.chromium.org/2915763005/diff/1/components/sync/model_impl/...
components/sync/model_impl/shared_model_type_processor.cc:466:
entity->CacheCommitData(update.entity);
Can you add a unit test that exercises this path when SupportsGetStorageKey()
returns false?

https://codereview.chromium.org/2915763005/diff/1/components/sync/model_impl/...
components/sync/model_impl/shared_model_type_processor.cc:595:
entity_data.push_back(EntityChange::CreateAdd(storage_key, update.entity));
I think this would be easier to read if you did the entity_data.push_back first,
and then the if, wdyt?

https://codereview.chromium.org/2915763005/diff/1/components/sync/model_impl/...
components/sync/model_impl/shared_model_type_processor.cc:604:
DCHECK(AllStorageKeysPopulated());
Should this DCHECK be run if we get an error back from merge/apply?

https://codereview.chromium.org/2915763005/diff/1/components/sync/model_impl/...
File components/sync/model_impl/shared_model_type_processor.h (right):

https://codereview.chromium.org/2915763005/diff/1/components/sync/model_impl/...
components/sync/model_impl/shared_model_type_processor.h:211: // The bridge
wants to communicate entirely via storage keys that is free to
that it is free to

https://codereview.chromium.org/2915763005/diff/1/components/sync/model_impl/...
components/sync/model_impl/shared_model_type_processor.h:215: // Entity is not
included in this map when the bridge doesn't support
This comment seems misleading.  They're only not included in here temporarily,
right?

[pavely, 2017-06-02 18:14:59]

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

[commit-bot: I haz the power, 2017-06-02 18:15:18]

Dry run: CQ is trying da patch.

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

[pavely, 2017-06-02 18:23:46]

https://codereview.chromium.org/2915763005/diff/1/components/sync/model/entit...
File components/sync/model/entity_change.h (right):

https://codereview.chromium.org/2915763005/diff/1/components/sync/model/entit...
components/sync/model/entity_change.h:31: // TODO(pavely): data_ptr() is added
temporarily to support converting
On 2017/05/31 19:53:11, skym wrote:
> Can we get a tracking bug for removing this?

Done.

https://codereview.chromium.org/2915763005/diff/1/components/sync/model/model...
File components/sync/model/model_type_sync_bridge.h (right):

https://codereview.chromium.org/2915763005/diff/1/components/sync/model/model...
components/sync/model/model_type_sync_bridge.h:57: //
change_processor()->UpdateStorageKey() iun this case. If a local and sync
On 2017/05/31 19:53:11, skym wrote:
> iun

Done.

https://codereview.chromium.org/2915763005/diff/1/components/sync/model/model...
components/sync/model/model_type_sync_bridge.h:85: // TODO(pavely): This
function should be removed once all bridge
On 2017/05/31 19:53:11, skym wrote:
> Again, bug number please

Done.

https://codereview.chromium.org/2915763005/diff/1/components/sync/model_impl/...
File components/sync/model_impl/shared_model_type_processor.cc (right):

https://codereview.chromium.org/2915763005/diff/1/components/sync/model_impl/...
components/sync/model_impl/shared_model_type_processor.cc:368: // The update
should be ignored.
On 2017/05/31 19:53:11, skym wrote:
> Why?

Done.

https://codereview.chromium.org/2915763005/diff/1/components/sync/model_impl/...
components/sync/model_impl/shared_model_type_processor.cc:371: 
On 2017/05/31 19:53:11, skym wrote:
> The newlines seem a bit odd here. Maybe group the continues together?

Done.

https://codereview.chromium.org/2915763005/diff/1/components/sync/model_impl/...
components/sync/model_impl/shared_model_type_processor.cc:389:
RecommitAllForEncryption(already_updated, metadata_changes.get());
On 2017/05/31 19:53:11, skym wrote:
> I'm worried this doesn't work correctly anymore, since it's before the Apply
> call.

This actually uncovered an issue. Recommit bumps seq number, and writes updated
metadata to MCL. This doesn't make sense without storage_key.

https://codereview.chromium.org/2915763005/diff/1/components/sync/model_impl/...
components/sync/model_impl/shared_model_type_processor.cc:466:
entity->CacheCommitData(update.entity);
On 2017/05/31 19:53:11, skym wrote:
> Can you add a unit test that exercises this path when SupportsGetStorageKey()
> returns false?

Thanks. Done.

https://codereview.chromium.org/2915763005/diff/1/components/sync/model_impl/...
components/sync/model_impl/shared_model_type_processor.cc:595:
entity_data.push_back(EntityChange::CreateAdd(storage_key, update.entity));
On 2017/05/31 19:53:11, skym wrote:
> I think this would be easier to read if you did the entity_data.push_back
first,
> and then the if, wdyt?

Done.

https://codereview.chromium.org/2915763005/diff/1/components/sync/model_impl/...
components/sync/model_impl/shared_model_type_processor.cc:604:
DCHECK(AllStorageKeysPopulated());
On 2017/05/31 19:53:11, skym wrote:
> Should this DCHECK be run if we get an error back from merge/apply?

Done.

https://codereview.chromium.org/2915763005/diff/1/components/sync/model_impl/...
File components/sync/model_impl/shared_model_type_processor.h (right):

https://codereview.chromium.org/2915763005/diff/1/components/sync/model_impl/...
components/sync/model_impl/shared_model_type_processor.h:211: // The bridge
wants to communicate entirely via storage keys that is free to
On 2017/05/31 19:53:12, skym wrote:
> that it is free to

Done.

https://codereview.chromium.org/2915763005/diff/1/components/sync/model_impl/...
components/sync/model_impl/shared_model_type_processor.h:215: // Entity is not
included in this map when the bridge doesn't support
On 2017/05/31 19:53:12, skym wrote:
> This comment seems misleading.  They're only not included in here temporarily,
> right?

Is this one better?

[commit-bot: I haz the power, 2017-06-02 18:41:26]

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

[commit-bot: I haz the power, 2017-06-02 18:41:27]

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

[skym, 2017-06-02 23:07:57]

lgtm, once unit tests are fixed. Not sure what issue you mentioned during
standup that was outstanding.

https://codereview.chromium.org/2915763005/diff/1/components/sync/model_impl/...
File components/sync/model_impl/shared_model_type_processor.h (right):

https://codereview.chromium.org/2915763005/diff/1/components/sync/model_impl/...
components/sync/model_impl/shared_model_type_processor.h:215: // Entity is not
included in this map when the bridge doesn't support
On 2017/06/02 18:23:46, pavely wrote:
> On 2017/05/31 19:53:12, skym wrote:
> > This comment seems misleading.  They're only not included in here
temporarily,
> > right?
> 
> Is this one better?

Yes! Thanks.

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

https://codereview.chromium.org/2915763005/diff/20001/components/sync/model_i...
components/sync/model_impl/shared_model_type_processor.cc:391: if (error) {
Good catch!

[pavely, 2017-06-05 18:54:43]

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

[commit-bot: I haz the power, 2017-06-05 18:55:01]

Dry run: CQ is trying da patch.

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

[pavely, 2017-06-05 19:05:28]

The CQ bit was unchecked by pavely@chromium.org

[pavely, 2017-06-05 19:05:29]

The CQ bit was checked by pavely@chromium.org

[pavely, 2017-06-05 19:05:29]

The patchset sent to the CQ was uploaded after l-g-t-m from skym@chromium.org
Link to the patchset: https://codereview.chromium.org/2915763005/#ps40001
(title: "Rebase. Fix recommit for encryption scenario.")

[pavely, 2017-06-05 19:05:34]

The CQ bit was checked by pavely@chromium.org

[commit-bot: I haz the power, 2017-06-05 19:05:58]

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-06-05 22:54:16]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1496689534062100,
"parent_rev": "525a0e6f0d79296a4da4f109f7a704ebbcf7f20a", "commit_rev":
"3188fbca55aa0e4cdce1a88ea270806408d2a3a2"}

[commit-bot: I haz the power, 2017-06-05 22:54:29]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1496689534062100,
"parent_rev": "3b76c8f5278fd13afdfa4eef6cb24f7b7ce1a8f1", "commit_rev":
"b877904be7dd60869488a48b722fbcf41986ea87"}

[commit-bot: I haz the power, 2017-06-05 22:54:43]

Description was changed from

==========
[Sync] Implement support for updating storage key for new entities

TypedURL's storage key is ROWID which can only be obtained after url record is
inserted into history database. Processor interface should allow bridge to
update storage key of new entity after it was processed by Merge/Apply.

In this change:
- ModelTypeSyncBridge::SupportsGetStorageKey indicates if processor should
call GetStorageKey or rely on bridge to call UpdateStorageKey instead.
- MergeSyncData takes list of changes. The reason is that it needs to support
both types of bridges and EntityChange conveniently includes storage key.
- MergeSyncData currently has implementation that converts change list into
EntityDataMap. In a separate change I'll switch existing bridge
implementations, remove conversion code and method that takes map.
- ProcessorEntityTracker can have empty storage key. It can only be changed to
non-empty one once.
- ModelTypeChangeProcessor::UpdateStorageKey takes EntityData to identify
entity that corresponds to storage key.
- SMTP::OnUpdateReceived handles entities with empty storage keys and ensures
that all storage keys are populated by bridge.
- I refactored SharedModelTypeProcessor::ProcessUpdate to group actios
performed on entities.

R=skym@chromium.org
BUG=719570
==========

to

==========
[Sync] Implement support for updating storage key for new entities

TypedURL's storage key is ROWID which can only be obtained after url record is
inserted into history database. Processor interface should allow bridge to
update storage key of new entity after it was processed by Merge/Apply.

In this change:
- ModelTypeSyncBridge::SupportsGetStorageKey indicates if processor should
call GetStorageKey or rely on bridge to call UpdateStorageKey instead.
- MergeSyncData takes list of changes. The reason is that it needs to support
both types of bridges and EntityChange conveniently includes storage key.
- MergeSyncData currently has implementation that converts change list into
EntityDataMap. In a separate change I'll switch existing bridge
implementations, remove conversion code and method that takes map.
- ProcessorEntityTracker can have empty storage key. It can only be changed to
non-empty one once.
- ModelTypeChangeProcessor::UpdateStorageKey takes EntityData to identify
entity that corresponds to storage key.
- SMTP::OnUpdateReceived handles entities with empty storage keys and ensures
that all storage keys are populated by bridge.
- I refactored SharedModelTypeProcessor::ProcessUpdate to group actios
performed on entities.

R=skym@chromium.org
BUG=719570

Review-Url: https://codereview.chromium.org/2915763005
Cr-Commit-Position: refs/heads/master@{#477113}
Committed:
https://chromium.googlesource.com/chromium/src/+/b877904be7dd60869488a48b722f...
==========

[commit-bot: I haz the power, 2017-06-05 22:54:45]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/b877904be7dd60869488a48b722f...