1636365 - Refactor trackers for bridged engines

Status: RESOLVED FIXED

(Firefox :: Sync, task)

Description

[Lina Butler [:lina]]

I didn't spend much time on the BridgedTracker stub, but it's not quite enough to build your own tracker. In practice, bridged trackers will only do the "listen for observer notifications and bump the score" part, and not persist a list of changed IDs.

I think a better way to do this is to split out persistence into a PersistentTracker, put a scary "don't use this" comment above it, and make the engines that actually need it (add-ons, clients, forms, history, and passwords) use it. Everyone else can use the base tracker that throws if you try to make it persist anything.

(I thought about doing this while I was adding more docs for BridgedEngine, let's do that, too!).

Comment 1

[Lina Butler [:lina]]

Actually, I'm going to be more opinionated, and call it a LegacyTracker! 😈

Comment 2

[Ryan Kelly [:rfkelly]]

Yes! That's exactly the kind of optimism that's lead to us hauling around a scope named "oldsync" for all these years :thumbsup:

(Seriously though, +1 to the name)

Comment 3

[Lina Butler [:lina]]

Attachment: Bug 1636365 - Split out persistence from `Tracker` into `LegacyTracker`. r?markh!,rfkelly

The tracker base class currently does two things: bump the score in
response to observer notifications, and store a list of changed IDs.
The bookmarks, form autofill, and now bridged trackers need to hack
around this to opt out of persistence, since they handle change
tracking in the storage layer.

This commit keeps the score logic in Tracker, but moves all the
persistence code into an intermediate LegacyTracker class, and
changes all engines that need persistence to inherit from it.

ignoreAll is more interesting. We want new-style stores to emit
observer notifications with change sources, so that the tracker knows
to ignore changes made by Sync. Ignoring all observer notifications
during a sync is a blunter version of this. But, not every new store
supports change sources, so we reimplement ignoreAll manually for
ones that don't.

Comment 4

[Lina Butler [:lina]]

Attachment: Bug 1636365 - Add more docs for `BridgedEngine`, and remove `BridgedTracker`. r?markh!,rfkelly

Now that we have a Tracker base class without persistence, we can
have bridged engines subclass it directly.

Depends on D74374

Comment 5

[Lina Butler [:lina]]

Yes! That's exactly the kind of optimism that's lead to us hauling around a scope named "oldsync" for all these years :thumbsup:

🤣🤣🤣

A girl can dream, right? Right? ☀️

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/40d980163482
Split out persistence from `Tracker` into `LegacyTracker`. r=markh,rfkelly
https://hg.mozilla.org/integration/autoland/rev/86a1d0814d03
Add more docs for `BridgedEngine`, and remove `BridgedTracker`. r=markh,rfkelly

Comment 7

[Lina Butler [:lina]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=62989b76ccb67c2e68345591986ebbf6b3cc2a06

Comment 8

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/40d980163482
https://hg.mozilla.org/mozilla-central/rev/86a1d0814d03