Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix: prefs doesn't work in FF's private windows #510

Closed
wants to merge 18 commits into from

Conversation

eight04
Copy link
Collaborator

@eight04 eight04 commented Sep 28, 2018

Fixes #471. @stonecrusher may want to test this PR.

  • Add web-ext build tool, so you can run npm run start-firefox to test the extension.
  • Drop prefs.readOnlyValues. It seems that it is an alias of prefs.get.

@stonecrusher
Copy link
Contributor

Sorry, I forgot to report. Already tested yesterday.

Preferences in private mode are now taken from "normal" mode. If you change something in private mode, it will not affect you prefs in normal mode. However they stay saved (at least for this private session).
But if you change something in normal mode, it will affect prefs in private mode.

No problems detected. LGTM, can be merged!

@eight04
Copy link
Collaborator Author

eight04 commented Oct 1, 2018

If you change something in private mode, it will not affect you prefs in normal mode.

Any example? It sounds like a bug for me.

@stonecrusher
Copy link
Contributor

stonecrusher commented Oct 3, 2018

I tested again, behaviour confirmed.

  • FF 62.0.3 normal mode. Deactivate already installed Stylus, test version temporary loaded from about:debugging.
  • Goto "manage" and change number of "applies to" elements to 1. Close manage page, leave normal window open.
  • Open private window. Under "manage", change number of "applies to" elements to 5.

Result:
No matter if you just close the tab in private mode, close the private mode window or reopen the private window: Normal mode setting is always 1 and private mode setting is always 5.

@eight04
Copy link
Collaborator Author

eight04 commented Oct 3, 2018

This is more complex than I thought:

  • chrome.storage.sync API is unavailable on Firefox when the extension is side-loaded.
  • We store all settings in background page's localStorage.
  • We have to create a pref-server in the background page to handle getting/setting values and sync with chrome.storage.sync.
  • In other tabs, we have to create a pref-client to communicate with the pref-server.

@Mottie do we still want to support Firefox < 53? If not, I suggest changing everything to chrome.storage.sync and add an addon ID to manifest.json.

@stonecrusher
Copy link
Contributor

If you ask me, just adapt the settings from normal mode to private mode and drop any changes made in private mode. That may be easier.

Plus at some point in the future stylus settings should be stored where styles are stored to make complete backup easier and stylus better transferable to other systems. So I wouldn't invest too much effort in this.

And FF<53 can be dropped IMHO. ESR is at version 60 right now.

@Mottie
Copy link
Member

Mottie commented Oct 3, 2018

I think we could drop FF<53; but what happens to all the FF clones that are using older versions? I think if @narcolepticinsomniac agrees, we could drop support.

@eight04
Copy link
Collaborator Author

eight04 commented Oct 3, 2018

Everything accessing storage.sync would throw. Note that we can specify strict_min_version in Firefox. It should stop the auto update in older FF.

@stonecrusher
Copy link
Contributor

@Mottie Don't most clones have their base from FF 54 or 55 when they didn't yet start dropping xul support?

@Mottie
Copy link
Member

Mottie commented Oct 3, 2018

I don't know... I don't use FF clones 😛

@narcolepticinsomniac
Copy link
Member

...we can specify strict_min_version in Firefox. It should stop the auto update in older FF.

Seems reasonable enough to me. Users sticking with ancient, unsupported versions of FF should really be using the legacy version of Stylish anyway. They're a vocal bunch though, so don't be surprised if a few show up with pitchforks. =)

@eight04
Copy link
Collaborator Author

eight04 commented Oct 4, 2018

Then I'll rewrite the prefs and make it depend on storage.sync. I need the UUID that is displayed at https://addons.mozilla.org/en-US/developers/addon/styl-us/edit.
image

@Mottie
Copy link
Member

Mottie commented Oct 4, 2018

@eight04 I messaged it to you in Discord.

Copy link
Collaborator Author

@eight04 eight04 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is ready. @stonecrusher could you test it again?

@@ -34,6 +34,10 @@
window.addEventListener(chrome.runtime.id, orphanCheck, true);
}

// FIXME: does it work with styleViaAPI?
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll delay this to #360 that I'm rewriting the background API.

@stonecrusher
Copy link
Contributor

@eight04 Fast answer: Sure, but need time till tomorrow evening (~30h from now).

@stonecrusher
Copy link
Contributor

Tested different use cases and it looks good to me! Everything working as expected. Nice!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants