Bug 2056071 - remove BridgedEngine, app-services side.#7483
Conversation
bendk
left a comment
bendk
left a comment
There was a problem hiding this comment.
Looks great to me, I love seeing all the deleted lines.
| // This is what UniFFI exposes; it does nothing other than delegate to | ||
| // the shared `BridgedEngineWrapper`. See | ||
| // services/interfaces/mozIBridgedSyncEngine.idl for the Desktop contract. | ||
| // the shared `BridgedEngineWrapper`, which adapts our `SyncEngine`. |
There was a problem hiding this comment.
What's stopping us from removing this macro and the various [Component]BridgedEngine interfaces and replacing them with a single thing (maybe a trait interface that's defined in sync15)? The differences I can see are the GUID handling and the error type, which don't seem that big? I'm not suggesting you do anything different in this PR, I'm just wondering where we are in the larger process.
| /// last-modified timestamps for collections are handled differently; | ||
| /// who does the `get_collection_request()` etc impacts the owner of the | ||
| /// timestamp. | ||
| /// Engines should do both or neither, longer term it should be absorbed. |
There was a problem hiding this comment.
This is connected to last_sync, but it's also for reset_last_sync. What about changing it to double-slashes and adding a newline before the method? Alternatively, it could be reworded to be about last_sync with a note at the end that says something like "this also applies to "reset_last_sync".
This uniffi's (ha) the desktop and mobile sync engine implementations. Desktop still uses the macro generated "bridge", and that's a duck-type JS model - which all of desktop sync already is so, yeah. Breaking change, will need to land phabricator/m-c's side of bug 2056071, which will have its own patch and also vendor this.
3e70c1f to
d4dcbb6
Compare
3 participants
Sorry for the size of this, but combined with the desktop side (which I'll get you all on too, again, sorry), it removes lots of code and combined desktop and mobile syncing significantly. Tagging Bastian too, sharing the love 😆 ❤️
🥂
This uniffi's (ha) the desktop and mobile sync engine implementations. Desktop still uses the macro generated "bridge", and that's a duck-type JS model - which all of desktop sync already is so, yeah.
Breaking change, will need to land phabricator/m-c's side of bug 2056071, which will have its own patch and also vendor this.
Pull Request checklist
[ci full]to the PR title.