Excise `mobileGUIDFetchBatchSize` and other mobile sync-related constants

RESOLVED FIXED in Firefox 55

Status

()

enhancement
P1
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: lina, Assigned: tiago, Mentored)

Tracking

({good-first-bug})

unspecified
Firefox 55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(1 attachment)

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

Comment 1

2 years ago
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!
Flags: needinfo?(kit)
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`.
Flags: needinfo?(kit)
Reporter

Updated

2 years ago
Assignee: nobody → tiago.paez11
Status: NEW → ASSIGNED
Priority: -- → P1
Reporter

Comment 4

2 years ago
mozreview-review
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.
Attachment #8870179 - Flags: review?(kit)
Comment hidden (mozreview-request)
Reporter

Comment 6

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

https://reviewboard.mozilla.org/r/141622/#review147272
Attachment #8870179 - Flags: review?(kit) → review+

Comment 7

2 years ago
Pushed by kcambridge@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1c1bf54915bb
Excise 'mobileGUIDFetchBatchSize' and other mobile sync-related constants. r=kitcambridge
https://hg.mozilla.org/mozilla-central/rev/1c1bf54915bb
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
You need to log in before you can comment on or make changes to this bug.