[Sync] Fake server now updates each model type's progress markers independently.

[Sync] Fake server now updates each model type's progress markers independently.

Previously the fake server used to update all of the progress markers
to the highest single version of an entity being returned in a
GetUpdates response. This caused odd behaviors when clients asked for
different sets of model types. This in turn resulted in tests failing
when they never agreed on some progress marker's version that wasn't
actually being actively updated.

This change separates all model types version/progress markers in the
fake server code. This should help clients that are being enabled
mid-test to get match progress markers when priority and non-priority
types are requested across separate GetUpdates messages.

Also removed some special case logic around filtering deleted items. It
is unclear what purpose that logic was serving.

BUG=672596
Committed: https://crrev.com/ccc880c2592bb8d82d41dc3ca6559db2bb99280f
Cr-Commit-Position: refs/heads/master@{#437690}

[skym, 2016-12-08 23:11:39]

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

[commit-bot: I haz the power, 2016-12-08 23:12:04]

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

[skym, 2016-12-08 23:20:35]

Description was changed from

==========
[Sync] Fake server now updates each model type's progress markers independently.

BUG=672596
==========

to

==========
[Sync] Fake server now updates each model type's progress markers independently.

Previously the fake server used to update all of the progress markers to the
highest single version of an entity being returned in a GetUpdates response.
This caused odd behaviors when clients asked for different sets of model types.
This in turn resulted in tests failing when they never agreed on some progress
marker's version that wasn't actually being actively updated.

This change separates all model types version/progress markers in the fake
server code. This should help clients that are being enabled mid-test to get
match progress markers when priority and non-priority types are requested across
separate GetUpdates messages.

Also removed some special case logic around filtering deleted items. It is
unclear what purpose that logic was serving.

BUG=672596
==========

[skym, 2016-12-08 23:20:35]

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

[commit-bot: I haz the power, 2016-12-08 23:22:38]

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

[commit-bot: I haz the power, 2016-12-08 23:22:39]

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...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[skym, 2016-12-09 00:11:07]

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

[commit-bot: I haz the power, 2016-12-09 00:11:23]

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-12-09 00:22:19]

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

[commit-bot: I haz the power, 2016-12-09 00:22:19]

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

[skym, 2016-12-09 16:50:00]

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

[commit-bot: I haz the power, 2016-12-09 16:50:23]

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

[skym, 2016-12-09 17:24:34]

Bots seem to be passing, assuaging my fears that the deleted entity logic did
anything useful. PTAL.

[commit-bot: I haz the power, 2016-12-09 17:36:53]

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

[commit-bot: I haz the power, 2016-12-09 17:36:54]

Dry run: This issue passed the CQ dry run.

[skym, 2016-12-09 20:48:41]

Description was changed from

==========
[Sync] Fake server now updates each model type's progress markers independently.

Previously the fake server used to update all of the progress markers to the
highest single version of an entity being returned in a GetUpdates response.
This caused odd behaviors when clients asked for different sets of model types.
This in turn resulted in tests failing when they never agreed on some progress
marker's version that wasn't actually being actively updated.

This change separates all model types version/progress markers in the fake
server code. This should help clients that are being enabled mid-test to get
match progress markers when priority and non-priority types are requested across
separate GetUpdates messages.

Also removed some special case logic around filtering deleted items. It is
unclear what purpose that logic was serving.

BUG=672596
==========

to

==========
[Sync] Fake server now updates each model type's progress markers independently.

Previously the fake server used to update all of the progress markers
to the highest single version of an entity being returned in a
GetUpdates response. This caused odd behaviors when clients asked for
different sets of model types. This in turn resulted in tests failing
when they never agreed on some progress marker's version that wasn't
actually being actively updated.

This change separates all model types version/progress markers in the
fake server code. This should help clients that are being enabled
mid-test to get match progress markers when priority and non-priority
types are requested across separate GetUpdates messages.

Also removed some special case logic around filtering deleted items. It
is unclear what purpose that logic was serving.

BUG=672596
==========

[maxbogue, 2016-12-09 21:10:16]

lgtm

https://codereview.chromium.org/2564663003/diff/40001/components/sync/test/fa...
File components/sync/test/fake_server/fake_server.cc (right):

https://codereview.chromium.org/2564663003/diff/40001/components/sync/test/fa...
components/sync/test/fake_server/fake_server.cc:58: // from_progress_marker is
used to determine this, legacy fields are ignored.
the nit of all nits: s/,/;/

https://codereview.chromium.org/2564663003/diff/40001/components/sync/test/fa...
components/sync/test/fake_server/fake_server.cc:69: for (auto& kv :
response_version_map_) {
const auto& perhaps?

https://codereview.chromium.org/2564663003/diff/40001/components/sync/test/fa...
components/sync/test/fake_server/fake_server.cc:93: static
UpdateSieve::ModelTypeToVersionMap MessageToVersionMap(
you shouldn't need the UpdateSieve:: bit right?

https://codereview.chromium.org/2564663003/diff/40001/components/sync/test/fa...
components/sync/test/fake_server/fake_server.cc:113:
request_version_map[model_type] = version;
Consider adding a DCHECK above this that model_type doesn't already exist in the
map.

[skym, 2016-12-09 21:51:51]

https://codereview.chromium.org/2564663003/diff/40001/components/sync/test/fa...
File components/sync/test/fake_server/fake_server.cc (right):

https://codereview.chromium.org/2564663003/diff/40001/components/sync/test/fa...
components/sync/test/fake_server/fake_server.cc:58: // from_progress_marker is
used to determine this, legacy fields are ignored.
On 2016/12/09 21:10:15, maxbogue wrote:
> the nit of all nits: s/,/;/

Done.

https://codereview.chromium.org/2564663003/diff/40001/components/sync/test/fa...
components/sync/test/fake_server/fake_server.cc:69: for (auto& kv :
response_version_map_) {
On 2016/12/09 21:10:15, maxbogue wrote:
> const auto& perhaps?

Done.

https://codereview.chromium.org/2564663003/diff/40001/components/sync/test/fa...
components/sync/test/fake_server/fake_server.cc:93: static
UpdateSieve::ModelTypeToVersionMap MessageToVersionMap(
On 2016/12/09 21:10:15, maxbogue wrote:
> you shouldn't need the UpdateSieve:: bit right?

Does not compile w/o it. I think it has to do with the function being static.

https://codereview.chromium.org/2564663003/diff/40001/components/sync/test/fa...
components/sync/test/fake_server/fake_server.cc:113:
request_version_map[model_type] = version;
On 2016/12/09 21:10:15, maxbogue wrote:
> Consider adding a DCHECK above this that model_type doesn't already exist in
the
> map.

Done.

[skym, 2016-12-09 21:51:55]

The CQ bit was checked by skym@chromium.org

[skym, 2016-12-09 21:51:56]

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/2564663003/#ps60001
(title: "Updates for max.")

[commit-bot: I haz the power, 2016-12-09 21:52:30]

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-12-09 23:28:53]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1481320315800650,
"parent_rev": "e57b53d5a6fa5be98a51ec2751e218beb73266ff", "commit_rev":
"5c15f7bc3282f4f77dfc3fc82cfdd1e30cf141d1"}

[commit-bot: I haz the power, 2016-12-09 23:29:22]

Description was changed from

==========
[Sync] Fake server now updates each model type's progress markers independently.

Previously the fake server used to update all of the progress markers
to the highest single version of an entity being returned in a
GetUpdates response. This caused odd behaviors when clients asked for
different sets of model types. This in turn resulted in tests failing
when they never agreed on some progress marker's version that wasn't
actually being actively updated.

This change separates all model types version/progress markers in the
fake server code. This should help clients that are being enabled
mid-test to get match progress markers when priority and non-priority
types are requested across separate GetUpdates messages.

Also removed some special case logic around filtering deleted items. It
is unclear what purpose that logic was serving.

BUG=672596
==========

to

==========
[Sync] Fake server now updates each model type's progress markers independently.

Previously the fake server used to update all of the progress markers
to the highest single version of an entity being returned in a
GetUpdates response. This caused odd behaviors when clients asked for
different sets of model types. This in turn resulted in tests failing
when they never agreed on some progress marker's version that wasn't
actually being actively updated.

This change separates all model types version/progress markers in the
fake server code. This should help clients that are being enabled
mid-test to get match progress markers when priority and non-priority
types are requested across separate GetUpdates messages.

Also removed some special case logic around filtering deleted items. It
is unclear what purpose that logic was serving.

BUG=672596

Review-Url: https://codereview.chromium.org/2564663003
==========

[commit-bot: I haz the power, 2016-12-09 23:29:23]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-12-12 14:58:19]

Description was changed from

==========
[Sync] Fake server now updates each model type's progress markers independently.

Previously the fake server used to update all of the progress markers
to the highest single version of an entity being returned in a
GetUpdates response. This caused odd behaviors when clients asked for
different sets of model types. This in turn resulted in tests failing
when they never agreed on some progress marker's version that wasn't
actually being actively updated.

This change separates all model types version/progress markers in the
fake server code. This should help clients that are being enabled
mid-test to get match progress markers when priority and non-priority
types are requested across separate GetUpdates messages.

Also removed some special case logic around filtering deleted items. It
is unclear what purpose that logic was serving.

BUG=672596

Review-Url: https://codereview.chromium.org/2564663003
==========

to

==========
[Sync] Fake server now updates each model type's progress markers independently.

Previously the fake server used to update all of the progress markers
to the highest single version of an entity being returned in a
GetUpdates response. This caused odd behaviors when clients asked for
different sets of model types. This in turn resulted in tests failing
when they never agreed on some progress marker's version that wasn't
actually being actively updated.

This change separates all model types version/progress markers in the
fake server code. This should help clients that are being enabled
mid-test to get match progress markers when priority and non-priority
types are requested across separate GetUpdates messages.

Also removed some special case logic around filtering deleted items. It
is unclear what purpose that logic was serving.

BUG=672596

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

[commit-bot: I haz the power, 2016-12-12 14:58:19]

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