Open maciex opened 7 years ago
@khloke what do you think about this pull request?
@maciex Sorry I haven't had a chance to look at it, will try to do so in the next couple of days.
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 Sorry I accidentally pushed a merge into your branch. Revert as you see fit.
@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:
Promise
, instead it requires a callback function as a second parameter to set
and get
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.
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...
@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"
]
}]
}
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?
@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.
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.