play-to-xbmc-chrome icon indicating copy to clipboard operation
play-to-xbmc-chrome copied to clipboard

Migration from localStorage to browser.storage.sync API

Open maciex opened this issue 7 years ago • 9 comments

This is first pull request for migration of localStorage API usage to browser.storage.sync API.

This change required quite a lot of rework in the code handling the settings.

maciex avatar Nov 13 '17 21:11 maciex

@khloke what do you think about this pull request?

maciex avatar Nov 29 '17 23:11 maciex

@maciex Sorry I haven't had a chance to look at it, will try to do so in the next couple of days.

khloke avatar Nov 30 '17 23:11 khloke

I released the fix from this pull request in a beta version (1.9.1beta1) for Firefox on December 1st. About 3-8 users daily since then and no complains.

maciex avatar Dec 29 '17 21:12 maciex

@maciex Sorry I accidentally pushed a merge into your branch. Revert as you see fit.

khloke avatar Jan 01 '18 08:01 khloke

@maciex There are quite a few things I'm going to have to change to make this work with Chrome. Things I found so far:

  • Chrome doesn't use Promise, instead it requires a callback function as a second parameter to set and get
  • Chrome doesn't support browser.storage, it needs chrome.storage

Are you keen on making the code as cross-browser compatible as possible to allow features being merged from one another? We may have to rethink the settings code.

khloke avatar Jan 02 '18 08:01 khloke

I would like to keep the code cross-browser compatible, this would allow us easier merges between the forks.

That's unexpected... I mean the differences... the documentation mentioned that Firefox and Chrome support that feature...

maciex avatar Jan 02 '18 17:01 maciex

@khloke I found a solution:

Firefox supports both chrome and browser namespaces

As a porting aid, the Firefox implementation of WebExtensions supports chrome, using callbacks, as well as browser, using promises. This means that many Chrome extensions will just work in Firefox without any changes. However, this is not part of the WebExtensions standard, and might not be supported by all compliant browsers.

If you do write your extension to use browser and promises, then we've also developed a polyfill that will enable it to run in Chrome: https://github.com/mozilla/webextension-polyfill.

https://developer.mozilla.org/en-US/Add-ons/WebExtensions/Chrome_incompatibilities

So to have a common approach in both browsers we should use that polyfill library. Adding it's browser-polyfill.js file to the archive and these lines below to manifest should be enough:


{
  // ...

  "background": {
    "scripts": [
      "browser-polyfill.js",
      "background.js"
    ]
  },

  "content_scripts": [{
    // ...
    "js": [
      "browser-polyfill.js",
      "content.js"
    ]
  }]
}

maciex avatar Jan 02 '18 17:01 maciex

I'm getting reports from Firefox users that the settings are not working (not saved after restart, etc.) That's why I would like to push this issue further and release a new stable version with the new storage approach.

@khloke Should I add the "browser-polyfill.js" to the code? Is it an acceptable solution?

maciex avatar Jan 10 '18 13:01 maciex

@khloke I added the "browser-polyfill.js" to this branch. After this common API can be used to get she browser.storage.sync settings.

I tested on Firefox and Google Chrome and all is working fine.

maciex avatar Feb 08 '18 10:02 maciex