Closed Bug 1414438 Opened 2 years ago Closed 2 years ago

Use `getBatched` instead of `get` to backfill records

Categories

(Firefox :: Sync, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
Firefox 58
Tracking Status
firefox58 --- fixed

People

(Reporter: Lina, Assigned: tfeserver, Mentored)

References

Details

(Keywords: good-first-bug)

Attachments

(1 file)

`_processIncoming` currently uses `get` to fetch payloads for backfilled records. I think we can replace this with `getBatched`, as we already do for new records.
Hi,

Maybe i could fix this bug?
This is not my first bug, but i am quite new with the firefox source code.
So the replace needs to replace get by getBatched on https://dxr.mozilla.org/mozilla-central/source/services/sync/modules/engines.js#1311, right?

How can this be tester? There is a scary comment on the beginning of the function saying it cannot be tested?
Flags: needinfo?(kit)
(In reply to tfe from comment #1)
> Maybe i could fix this bug?

Of course, please feel free! The scary function got a refactor in bug 1368209, and has a test now. Let's change https://searchfox.org/mozilla-central/rev/5a60492a53667fc61a62af1847d005a210b7a4f6/services/sync/modules/engines.js#1207 to use `getBatched`.

For testing, I think setting `engine.downloadBatchSize` in https://searchfox.org/mozilla-central/rev/7e090b227f7a0ec44d4ded604823d48823158c51/services/sync/tests/unit/test_history_engine.js#96, and verifying the test still passes, should be sufficient.

Thanks!
Flags: needinfo?(kit)
Comment on attachment 8925847 [details]
Bug 1414438 - Use `getBatched` instead of `get` to backfill records

https://reviewboard.mozilla.org/r/197050/#review202644

Thanks!

::: services/sync/modules/engines.js:1202
(Diff revision 1)
>      let backfilledItems = this.itemSource();
>  
>      backfilledItems.sort = "newest";
>      backfilledItems.full = true;
>  
>      // `get` includes the list of IDs as a query parameter, so we need to fetch

Please change `get` to `getBatched` in the comment, too.
Attachment #8925847 - Flags: review+
Assignee: nobody → tfeserver
Status: NEW → ASSIGNED
Priority: -- → P1
Pushed by kcambridge@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e794bc60c010
Use `getBatched` instead of `get` to backfill records r=kitcambridge
https://hg.mozilla.org/mozilla-central/rev/e794bc60c010
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
You need to log in before you can comment on or make changes to this bug.