[Sync] USS: Filter out redundant changes in SMTP.

[Sync] USS: Filter out redundant changes in SMTP.

BUG=561829
Committed: https://crrev.com/621c7e4472039cc5e3c55acd738ca7bb32908e8a
Cr-Commit-Position: refs/heads/master@{#383800}

[maxbogue, 2016-03-28 18:54:16]

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

[maxbogue, 2016-03-28 18:54:17]

PTAL!

[Gang Wu, 2016-03-28 22:09:40]

lgtm

[skym, 2016-03-28 22:35:38]

lgtm

https://codereview.chromium.org/1835953002/diff/40001/sync/internal_api/proce...
File sync/internal_api/processor_entity_tracker.cc (right):

https://codereview.chromium.org/1835953002/diff/40001/sync/internal_api/proce...
sync/internal_api/processor_entity_tracker.cc:23:
specifics.AppendToString(&hash_input);
Why not use SerializeAsString() inline?

https://codereview.chromium.org/1835953002/diff/40001/sync/internal_api/proce...
sync/internal_api/processor_entity_tracker.cc:87: return
!metadata_.has_specifics_hash();
What's the reason for this special logic? We don't match specifics hashes for
deletes anyways right?

https://codereview.chromium.org/1835953002/diff/40001/sync/internal_api/share...
File sync/internal_api/shared_model_type_processor.cc (right):

https://codereview.chromium.org/1835953002/diff/40001/sync/internal_api/share...
sync/internal_api/shared_model_type_processor.cc:246: if
(entity->MatchesSpecificsHash(data->specifics)) {
Should we even bother making this check if entity == nullptr?

[maxbogue, 2016-03-29 00:32:09]

https://codereview.chromium.org/1835953002/diff/40001/sync/internal_api/proce...
File sync/internal_api/processor_entity_tracker.cc (right):

https://codereview.chromium.org/1835953002/diff/40001/sync/internal_api/proce...
sync/internal_api/processor_entity_tracker.cc:23:
specifics.AppendToString(&hash_input);
On 2016/03/28 22:35:38, skym wrote:
> Why not use SerializeAsString() inline?

Because I was just copying from UpdateSpecificsHash() and didn't know that was a
thing :P Done.

https://codereview.chromium.org/1835953002/diff/40001/sync/internal_api/proce...
sync/internal_api/processor_entity_tracker.cc:87: return
!metadata_.has_specifics_hash();
On 2016/03/28 22:35:38, skym wrote:
> What's the reason for this special logic? We don't match specifics hashes for
> deletes anyways right?

Hmm... doesn't seem very important. It should be fine to just set specifics hash
as the empty string. Removed it here and below.

https://codereview.chromium.org/1835953002/diff/40001/sync/internal_api/share...
File sync/internal_api/shared_model_type_processor.cc (right):

https://codereview.chromium.org/1835953002/diff/40001/sync/internal_api/share...
sync/internal_api/shared_model_type_processor.cc:246: if
(entity->MatchesSpecificsHash(data->specifics)) {
On 2016/03/28 22:35:38, skym wrote:
> Should we even bother making this check if entity == nullptr?

Guess not. Done.

[maxbogue, 2016-03-29 01:02:05]

https://codereview.chromium.org/1835953002/diff/40001/sync/internal_api/proce...
File sync/internal_api/processor_entity_tracker.cc (right):

https://codereview.chromium.org/1835953002/diff/40001/sync/internal_api/proce...
sync/internal_api/processor_entity_tracker.cc:87: return
!metadata_.has_specifics_hash();
On 2016/03/29 00:32:09, maxbogue wrote:
> On 2016/03/28 22:35:38, skym wrote:
> > What's the reason for this special logic? We don't match specifics hashes
for
> > deletes anyways right?
> 
> Hmm... doesn't seem very important. It should be fine to just set specifics
hash
> as the empty string. Removed it here and below.

Update: not below, apparently the logic doesn't return empty string for the
default proto. I've added a DCHECK here that ByteSize > 0 and restored the logic
in UpdateSpecificsHash.

[pavely, 2016-03-29 18:57:06]

lgtm

[maxbogue, 2016-03-29 20:03:08]

The CQ bit was checked by maxbogue@chromium.org

[maxbogue, 2016-03-29 20:03:09]

The patchset sent to the CQ was uploaded after l-g-t-m from gangwu@chromium.org,
skym@chromium.org
Link to the patchset: https://codereview.chromium.org/1835953002/#ps80001
(title: "Undo UpdateSpecificsHash change and add DCHECK.")

[commit-bot: I haz the power, 2016-03-29 20:03:47]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1835953002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1835953002/80001

[commit-bot: I haz the power, 2016-03-29 20:10:08]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-03-29 20:12:03]

Description was changed from

==========
[Sync] USS: Filter out redundant changes in SMTP.

BUG=561829
==========

to

==========
[Sync] USS: Filter out redundant changes in SMTP.

BUG=561829

Committed: https://crrev.com/621c7e4472039cc5e3c55acd738ca7bb32908e8a
Cr-Commit-Position: refs/heads/master@{#383800}
==========

[commit-bot: I haz the power, 2016-03-29 20:12:04]

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