Implement register_with_sync_manager for places

[bendk]

Make places use register_with_sync_manager like the other components do. This will require a bit of care, since we are going to be doing UniFFI work for both places and sync-manager at the same time. Here's how I think it could work:

• Implement PlacesApi.register_with_sync_manager
• In the sync-manager branch: make setPlaces call register_with_sync_manager
• In the places branch: include registerWithSyncManager in the UniFFI API
• When we merge both, make the android code use registerWithSyncManager and delete the SyncManager.setPlaces method.

┆Issue is synchronized with this Jira Task
┆epic: Uniffi Sync Manager
┆sprintEndDate: 2021-12-10

[mhammond]

This sounds fine, but it might even be possible to go further before the uniffi work is ready - particularly if what you outline end up being a bit of a problem. If we take inspiration from the metadata functions, we could add, say, void places_api_register_with_sync_manager(i64 handle); and the Rust code could obtain an Arc<PlacesApi> from the APIS static.

[bendk]

After trying to implement this I realize there's an underlying issues that needs to be resolved first: places tends to use regular refs, with lifetimes, while the other components use Arc<>. For example BookmarksEngine vs LoginsSyncEngine. This leads to several issues when trying to implement get_registered_sync_engine()

• The sync engine is normally created from a Weak ref (logins uses Weak<LoginsStore>). However, this isn't going to work for BookmarksEngine or HistoryEngine, since you can't tie the lifetime to the Weak instance. It's possible to solve this. I currently have some code that refactors SyncConn then can creates BookmarksEngine / HistoryEngine with the lifetime of the SyncConn.
• get_registered_sync_engine() normally returns an Option<Box<dyn SyncEngine>> with an implied 'static lifetime. Making a return signature that works with the lifetimes of BookmarksEngine / HistoryEngine was beyond my Rust knowledge. My normal go to of adding '_ to make Option<Box<dyn SyncEngine + '_>> didn't work. It's probably possible somehow, but I couldn't figure it out.

So I think we should try to refactor things to use Arc<> more. That would bring the places code closer to logins/autofill and also make it generally easier to use. Here's how I think this could hapen:

• Refactor BookmorksEngine and HistoryEngine to use an Arc<PlacesDb> instead of &PlacesDb
• Refactor PlacesApi.open_sync_connection() to return an Arc<> rather than the SyncConn class.
  • One question here is if we need to continue supporting the safety mechanism that forces you to drop your connection before calling open_sync_connection() again.. I kind of think we can just forget about that, since we know the current code is working okay and once we switch to using the sync manager, then that can manage the concourancy issue. But if needed, I think there's a way to keep supporting it.

What do you all think about the plan? I guess the question is how does migrating to Arc<>s fit with the places UniFFI work. The one thing I want to avoid is refactoring this code, then refactoring it again as part of the UniFFI work.

[bendk]

It thought about this one some more and I think there's a fairly straightforward way to evolve this code. I laid out the overall plan and implemented the first step in #4627