fix: Be smarter when syncing bookmarks. #288
Conversation
Also, sort the dependencies by name while I’m here.
Don’t delete ones we’re just going to create again. Only create ones we need to. All that sorta stuff. Fixes #273. Well, kinda.
| return browser.bookmarks.getChildren(snoozeTabsFolder.id).then((bookmarks) => { | ||
| const tabs = [...Object.values(items)]; | ||
| const toCreate = tabs.filter((tab) => !bookmarks.find((bookmark) => tab.url === bookmark.url)); | ||
| const toRemove = bookmarks.filter((bookmark) => !tabs.find((tab) => bookmark.url === tab.url)); |
There was a problem hiding this comment.
"Kinda" indeed — this still won't work correctly with Sync.
Device A snoozes tabs AAAA, BBBB, and CCCC.
Device B syncs, so its Snoozed Tabs folder now contains AAAA, BBBB, and CCCC. But its snoozed tabs set (items here) is DDDD, EEEE. It'll now efficiently and carefully remove AAAA, BBBB, and CCCC, insert DDDD and EEEE, and sync those up to the server, blowing away Device A's backup next time it syncs bookmarks. The two devices will fight over the folder each time they sync.
There was a problem hiding this comment.
I understood this PR to be just part of the fix. Does this next PR adding a unique string to the folder name improve things?
| })); | ||
| const operations = toCreate.map(item => { | ||
| log(`Creating ${item.url}.`); | ||
| return browser.bookmarks.create({ |
There was a problem hiding this comment.
Unless browser.bookmarks is smart — and looking at what I think is the file that implements it in m-c, it's not — this is a performance nightmare: N SQL reads and writes (however many are needed for a create or a delete) for each change, outside a transaction. That's a lot of filesystem operations and a lot of hits on Places.
Further, a bookmarks sync will be triggered by the first write, not the last. At best, with all of Kit and co's work, each time you flush this to disk with more than one change you'll get two syncs.
browser.bookmarks considered very harmful: it's simply a bad API. There's not much you can do about it, other than to not use it.
There was a problem hiding this comment.
Alternative methods for how to not lose data on uninstall would be appreciated. Until then, I think we're kinda forced into using it. 😞
(I thought about passing a data: url to runtime.setUninstallURL, but that's limited to 255 characters, and only accepts http or https.)
There was a problem hiding this comment.
I'm left wondering what's the practical impact here - and is it bad enough that we should abandon this attempt to retain user data from the Test Pilot experiment? I don't think we have a whole lot of other options that survive WebExtension uninstall. What's the trade off? To be clear, this isn't the implementation we'd use in an eventual product release (if any)
There was a problem hiding this comment.
The practical impacts are:
For users that don't use Sync
One bundle of SQL writes and appropriate fsyncs for each closed and each opened tab. Potential churn in any caches that Places maintains. This is not good for perf, but perhaps users don't snooze tabs often enough for them to notice.
For users that do use Sync
A sync will start as soon as the first change is written, and will continue contending with this code. Syncs will continue to run (probably only one more) for so long as writes in this loop have occurred during a sync. This will be janky as hell, but #291 will avoid the risk of data loss due to competing devices syncing.
If you're flushing tabs frequently, you'll be not only causing potential jank from hitting the filesystem, but also by causing one or two syncs every time you flush.
The sync will also spin the event loop, potentially causing surprising behavior, such as UI elements losing focus. It will definitely test any race conditions in your code!
There was a problem hiding this comment.
If you have filesystem access, I'd consider using that instead: just dump the current set of snoozed URLs into a plain JSON file in the profile directory each time it changes. Heck, dump a fresh file into a fixed folder each time; they're small enough that perhaps a safe history is a good idea. A user who complains about having lost snoozed tabs can be pointed to those dumps.
There was a problem hiding this comment.
If you have filesystem access
Nope, I don't think we have filesystem access from WebExtension APIs.
There was a problem hiding this comment.
Does the TestPilot extension itself offer any hooks for you to maintain this kind of state? We've talked about offering a centralized storage API as part of TestPilot…
There was a problem hiding this comment.
If you're flushing tabs frequently
Yeah, this is kind of what I'm grasping at with practical impacts: The bookmark changes happen when a tab wakes from snooze; a user snoozes a new tab; a user deletes a snoozed tab; a user edits the time on a snoozed tab.
I'd expect there'd usually be many minutes between each of these actions, so I'm wondering how practically bad the impact on sync would be versus not retaining this data in a way the user can easily access at the end of the experiment.
There was a problem hiding this comment.
With #291 the only remaining concerns are risk of provoking bugs (the desktop bookmarks change tracker is an area of perennial disappointment) and perf; the original concern about clients trampling each others' data is fixed. If Kit Cambridge is happy with the risk to the tracker, and you're happy about the potential performance impact, then rock on.
(This doesn't do much for my worries about the viability of WebExtensions, though. 😞 )
There was a problem hiding this comment.
Does the TestPilot extension itself offer any hooks for you to maintain this kind of state? We've talked about offering a centralized storage API as part of TestPilot…
The only thing the Test Pilot extension offers right now is a channel for sending Telemetry metrics from WebExtensions. I don't think there's a storage / data retention API kind of thing on the radar
Don’t delete ones we’re just going to create again.
Only create ones we need to.
All that sorta stuff.
And pin and sort our dependencies while I’m here. 🙂
Fixes #273.
Well, kinda.