XKit-Rewritten icon indicating copy to clipboard operation
XKit-Rewritten copied to clipboard

Refactor: Use browser.storage.local.onChanged.addListener

Open marcustyphoon opened this issue 9 months ago • 3 comments

Description

As of Firefox 101, as per https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/storage/StorageArea/onChanged, we should be able to use browser.storage.local.onChanged.addListener rather than using browser.storage.onChanged.addListener and checking if the second callback parameter is 'local'.

In addition to being a nice refactor, this appears—weirdly enough—to be a bugfix on Safari 18.1.1 on iOS in my testing, because Safari is giving undefined as the second callback parameter when I use browser.storage.onChanged.addListener, which breaks all of our storage listeners.

Testing steps

  • [x] Confirm that changing extension preferences works in desktop Chromium 105.
  • [x] Confirm that changing extension preferences works in desktop Firefox 121.
  • [x] Confirm that changing extension preferences works in desktop Safari (tested in 17.6).
  • [x] Confirm that changing extension preferences works in Firefox for Android.
  • ~~Confirm that changing extension preferences works in Kiwi Browser for Android.~~ Kiwi Browser is being retired!

marcustyphoon avatar Mar 24 '25 20:03 marcustyphoon

Hm.

  • This note seems to appear (with slightly different, but not clearly different, copy... should probably put an mdn issue about that) in the equivalent storage.onChanged docs. If there are any problematic behaviors resulting from this, we should already be encountering them in Firefox.
  • Some quick testing (browser.storage.local.set({ enabledScripts, specialAccess, hello: 'world' }); + export const onStorageChanged = changes => console.log(changes);) indicates that this is indeed the case. Chrome logs only the enabledScripts value when toggling a script; Firefox logs all of them. edit: By "all of them" I mean "all three of the keys set by the set call."
  • We only seem to set multiple values in one storage.local.set call in one place and seem to set primitive values in very few places; the number of updates we could save by equality checking appears to be zero and if we needed to do so we would need a deep equality check.

So I think—if I haven't missed anything, which is definitely possible—it should be fine as-is!

marcustyphoon avatar Mar 30 '25 19:03 marcustyphoon

The difference between MDN's descriptions seems to be implying that while storage.onChanged's first argument includes all the keys present in the triggering StorageArea.set() call, storage.StorageArea.onChanged's argument includes all keys present in the entire storage area, not just those from the triggering StorageArea.set() call.

https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/storage/onChanged Note: In Firefox, the information returned includes all keys within the storage area storageArea.set ran against whether they changed or not.

https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/storage/StorageArea/onChanged Note: In Firefox, the information returned includes all keys within the storage area.

Is the MDN note for storage.StorageArea.onChanged just unclear to the point of being incorrect? It does look copy-pasted.

AprilSylph avatar Mar 30 '25 19:03 AprilSylph

Yeah, I think something bad must have happened during review of this PR: https://github.com/mdn/content/pull/31227. Looks like it was probably intended to be consistent and didn't wind up being so?

marcustyphoon avatar Mar 30 '25 19:03 marcustyphoon