Open astral-sa opened 6 years ago
WIP List of renames:
quickReplyYoutube
will need a rename to something like quickReplyPasteAddsTags
or something.enableMouseMenu
to reference the alt toggle option it controlsIf a user has cookies disabled globally (but enabling them with per-site exception rules), I've found that the extension does not function at all - cookie settings control whether a site, and apparently even an extension, can use localStorage. Another reason to migrate it!
One other thing to plan out is how the default settings will be set. If we unwind the loading logic, we'll need to avoid the situation where the prefs are unset at the time of content script load - or, worse, on Firefox where content scripts are loaded into all existing tabs, we'll need to avoid having all of the content scripts trying to set default settings at the same time. On Legacy SALR, this was handled by the frame scripts (roughly equivalent to a content script) being loaded programmatically into pages by the main script (roughly equivalent to the background page) only after the setting situation was figured out. If all else fails, one possible solution to this is switching content script loading to be programmatically done instead of by the manifest. An advantage there would be to avoid loading content scripts into existing tabs in Firefox. It would also be possible to more easily split up the main content script into a script just for inside threads, a different script just for forum/user control panel view, etc., and only load the relevant script for that page. I'm not 100% on the internals, though, to know if checking the tab URL + programmatically loading the content scripts would be any slower, but I suppose it should still be faster than a round-trip of messages if that's the only other alternative.
Ideally, the content script can check if default settings exist/are up to date, and if so proceed immediately to load itself (nice speedup). Otherwise, it can send a message up to the background script to set the default settings, then continue loading after a slight delay when the background script responds (this case would be roughly equivalent in speed to what we have currently)
Motivations:
First, since content scripts can access chrome.storage directly, we can simplify the initial loading of the add-on - we'd no longer have to do an explicit round-trip message from content -> background -> content just to get our settings. Setting saves also wouldn't require posting a message up to the background script.
Next, we can store many settings directly without having to serialize them ourselves. Objects may still need to be serialized.
Further, we can have onChanged listeners for chrome.storage - I remember seeing at least one comment in the code where the author wished they had a change event to simplify something they were trying to do.
Lastly, localStorage is in danger of being cleared by clearing browser history/private browsing/browser space issues. A couple of Firefox users who clear their browsing history on exit have already noted that their SALR settings are being erased. storage.local has a limit of 5MB but, if we need to, this limit can be removed with the
unlimitedStorage
permission, so we wouldn't be in danger of either hitting the limit or having data erased.Bonus: "It's asynchronous with bulk read and write operations, and therefore faster than the blocking and serial localStorage API."
Prep work:
Since we'd have to migrate pretty much all of the preferences anyway, now's a good time to discuss any changes in how we name some preferences. For example, I'd like to suggest we rename the preferences so an individual service isn't named in the preference (SOAP/SAARS comes to mind). That way, if such a service goes down, we don't have to change a bunch of references and add a bunch of separate preference-renaming migration logic each time. Instead, we could just alter what the preference does+its text. So for example the aforementioned one would instead reference avatar history lookups.
Another thing to check and perhaps modify in the transition is if some preference names match their current behavior - for example,
enableMouseMenu
originally seems to have controlled whether the context menu was displayed during gesture navigation, but now controls whether the alt toggle key is used to show the context menu or the gestures - I wouldn't really guess that from the preference name.Final thoughts:
I'd be happy to work on this eventually after finishing the avatar/user BGs feature, and perhaps after doing some more work to help reduce the number of times we iterate through every post. This issue probably requires a good bit of discussion and looking into some of the settings, so it's more of a medium-to-long-term goal.
References:
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/storage https://developer.chrome.com/extensions/storage