1366081 - Excise `mobileGUIDFetchBatchSize` and other mobile sync-related constants

Status: RESOLVED FIXED

(Firefox :: Sync, enhancement, P1)

Description

[Lina Butler [:lina]]

`MOBILE_BATCH_SIZE`, `DEFAULT_MOBILE_GUID_FETCH_BATCH_SIZE`, and the `client.type` pref seem to be leftovers, maybe from XUL Fennec? Let's remove them.

(`Weave.Service.clientsEngine.isMobile` and mobile bookmarks are still very much in use, though. We want to keep those around).

Comment 1

[Santiago Paez [:tiago]]

Hi Kit!

I just uploaded a WIP patch. I removed the constants `MOBILE_BATCH_SIZE` and `DEFAULT_MOBILE_GUID_FETCH_SIZE`.
As for the `client.type` I wasn't sure if I had to remove them completely or only when it's `mobile`.

I went with removing when it was `mobile` for this version of the patch.

I also need help with the tests, `MOBILE_BATCH_SIZE` appears on these tests:

https://dxr.mozilla.org/mozilla-central/source/services/sync/tests/unit/test_history_engine.js

https://dxr.mozilla.org/mozilla-central/source/services/sync/tests/unit/test_syncengine_sync.js

On this version of the patch, these are the only failing tests under services/sync/

I tried to modify the tests to not use `MOBILE_BATCH_SIZE`, but the removal of `client.type` == `mobile` changes the amount of GET messages and records synced (the things that the tests test). Maybe you can point me in the right direction about this, or to some documentation to better understand what these tests are trying to achieve. 

Thanks!

Comment 2

[Santiago Paez [:tiago]]

Attachment: Bug 1366081 - Excise 'mobileGUIDFetchBatchSize' and other mobile sync-related constants.

Review commit: https://reviewboard.mozilla.org/r/141622/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/141622/

Comment 3

[Lina Butler [:lina]]

Thanks for the patch, Tiago! I think you can remove `test_history_engine.js` `getBatched` takes care of this one level down, and it's covered by other tests. You can also delete the `test_processIncoming_store_toFetch` and `test_processIncoming_mobile_batchSize` tests in `test_syncengine_sync.js`.

Comment 4

[Lina Butler [:lina]]

Comment on attachment 8870179 [details]
Bug 1366081 - Excise 'mobileGUIDFetchBatchSize' and other mobile sync-related constants.

https://reviewboard.mozilla.org/r/141622/#review145722

Thanks! Please fix up the tests, and we should be good to land.

Comment 5

[Santiago Paez [:tiago]]

Comment on attachment 8870179 [details]
Bug 1366081 - Excise 'mobileGUIDFetchBatchSize' and other mobile sync-related constants.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/141622/diff/1-2/

Comment 6

[Lina Butler [:lina]]

Comment on attachment 8870179 [details]
Bug 1366081 - Excise 'mobileGUIDFetchBatchSize' and other mobile sync-related constants.

https://reviewboard.mozilla.org/r/141622/#review147272

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1c1bf54915bb
Excise 'mobileGUIDFetchBatchSize' and other mobile sync-related constants. r=kitcambridge

Comment 8

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/1c1bf54915bb