lunakurame / firefox-gnome-theme

A theme for Firefox 57+ matching GNOME Adwaita.
The Unlicense
201 stars 15 forks source link

@import local files for convenience #14

Closed folknor closed 6 years ago

folknor commented 6 years ago

Hi!

So I already had both custom userContent.css and userChrome.css, but of course I have to use yours, they are so fantastic.

Which means I merged yours with my local ones, but it's a pain to update.

Can you possibly add something equivalent to

@import "localContent.css" and @import "localChrome.css" at the bottom of your CSS files? Then I could just git pull and put my own selectors in those files, and never have any conflicts.

Firefox will stop parsing at those statements if the files do not exist, so they must be at the bottom.

Thank you for the work you've done, it looks amazing.

folknor commented 6 years ago

I realise that this might have unintended consequences that we are unable to foresee. And also that there might be some better way of doing it that I haven't considered.

folknor commented 6 years ago

I didn't think this through at all - @import statements obviously have to be at the top of the file. I filed this request before I did any investigation into the issue, which was a mistake.

Hopefully we can think about the issue and come up with some solution.

folknor commented 6 years ago

Alright, what I did instead is:

  1. git clone this to some folder
  2. cd ~/.mozilla/firefox/0xdeadbeef/chrome
  3. ln -s /home/user/git/firefox-gnome-theme/ theme

Then I put this in userChrome.css above my content: @import "theme/ui/theme-light.css";

And this in userContent.css above my content: @import "theme/userContent.css";

So I'll close this issue now. Apologies for the spam!

lunakurame commented 6 years ago

Hi! Thanks for your kind words. Actually I wanted to add that feature for quite a while, because I do have my own custom styles too. I haven't updated my own setup for like a week because I'm too lazy to merge them in. Unfortunately @import doesn't work in the userContent.css file due a bug in Electrolysis (Firefox' multi-process rendering engine): https://bugzilla.mozilla.org/show_bug.cgi?id=1416184. Or does it work for you? Did you test your solution before closing this issue? It should work in the userChrome.css file, but not in userContent.css as far as I know, unless you turn off Electrolysis, but that would give you a performance penalty.

I'm thinking about a different way to allow importing custom styles, and it would be nice to also preserve the theme config (currently there are a few things you can enable in userChrome.css and there will be more, but all your config is lost every time you git pull). I'm not sure yet how to do that without overcomplicating things, but I think a Bash script would be good enough. The config would be generated the first time you install the theme (running something like install.sh and answering a few questions) and then when you wanna update, you just run something like

git pull
./update.sh

Any changes in your custom styles for the userContent.css file won't work until you ./update.sh again, because of that Electrolysis bug, but still it's just a single command you need to run after any changes. What do you think? Or maybe you have some suggestions? I'd like to make it work properly once and for good to not scare other users with constantly changing installation instructions. 😛

folknor commented 6 years ago

Ah, yes, I have e10s disabled, and have apparently for a long time. I always run firefox nightly built from source, and have done that for probably over 10 years on linux. Which means that I sometimes have to explicitly disable new features that keep crashing - browser.tabs.remote.autostart.2 set to false is apparently one of them.

But yes, @import does work for me with e10s disabled. That is to say, it works with that pref set to false. Whether or not that enables or disables e10s, I can not say, except that I trust the comment on BMO at face value and can't be bothered to check it further.

My thoughts are that personally I don't care about Windows-users, so a script would be fine - but also that it's working for me now and I've apparently had e10s disabled since its inception - and performance has never been a problem for me, so I am certainly willing to wait until the BMO issue is resolved, and continue with the way I solved it in my previous comment.

But also that it seems more likely that the userContent.css filename is explicitly sandboxed for access from the content processs than it being related to e10s. I can do some code lookup on DXR a bit later and check it out!

folknor commented 6 years ago

Hm, so, there's no doubt that I have e10s enabled now after resetting some preferences (about:support says "Multiprocess Windows 2/2 (Enabled by default)"), and @import from userContent.css is definitely working for me.

https://dxr.mozilla.org/mozilla-central/source/security/sandbox/linux/broker/SandboxBrokerPolicyFactory.cpp#466

This is the first rule of my userContent.css:

@import "theme/userContent.css";

@namespace html "http://www.w3.org/1999/xhtml";
@namespace xul "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul";

@-moz-document domain(reddit.com) {
    section.infobar:nth-child(1) {
        display: none !important;
    }
}

And I can comment out/in the top line to enable/disable your userContent changes.

lunakurame commented 6 years ago

Well this is weird. Something just randomly started working for me. I've got two keys in my about:config:

By default the first one is set to false and the second one to true. If I enable both, nothing changes. If I disable the second one, about:support shows "Multiprocess Windows" is disabled. When I enable the first one, but disable the second one, "Multiprocess Windows" is enabled, but "Web Content Processes" is set to just 1 instead of usual 4. Also doesn't matter which settings I pick, @import always works for XUL pages (about:addons, about:preferences), but if the second option is enabled (which is the default), then @import doesn't work for HTML pages (like about:newtab). HTML pages only work properly when the second option is disabled, which forces "Web Content Processes" to be 1. So as I understand it, XUL pages work just fine, but if you want HTML pages to work properly, you can still keep multiprocess windows, but you can't keep multiprocess web content. And Firefox uses both XUL and HTML for its internal pages.

folknor commented 6 years ago
browser.tabs.remote.autostart: true
browser.tabs.remote.autostart.2: false
Multiprocess Windows    1/1 (Enabled by default)
Web Content Processes   4/4

browser.tabs.remote.autostart: true
browser.tabs.remote.autostart.2: true
Multiprocess Windows    1/1 (Enabled by default)
Web Content Processes   4/4

browser.tabs.remote.autostart: false
browser.tabs.remote.autostart.2: true
Multiprocess Windows    0/1 (Disabled)
Web Content Processes   no-show

browser.tabs.remote.autostart: false
browser.tabs.remote.autostart.2: false
Multiprocess Windows    0/1 (Disabled)
Web Content Processes   no-show

As far as I can see, browser.tabs.remote.autostart.2 has no impact at all, which is backed up by the lack of any result from DXR on it: https://dxr.mozilla.org/mozilla-central/search?q=browser.tabs.remote.autostart&redirect=false

folknor commented 6 years ago

https://dxr.mozilla.org/mozilla-central/source/toolkit/modules/Troubleshoot.jsm#212 explains what "Web Content Processes" means:

    // Services.ppmm.childCount is a count of how many processes currently
    // exist that might respond to messages sent through the ppmm, including
    // the parent process. So we subtract the parent process with the "- 1",
    // and that’s how many content processes we’re waiting for.
    data.currentContentProcesses = Services.ppmm.childCount - 1;
    data.maxContentProcesses = Services.appinfo.maxWebProcessCount;

ppmm implements, according to https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/Services.jsm "nsIMessageBroadcaster" and "nsIProcessScriptLoader"

https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsIMessageBroadcaster#childCount

https://developer.mozilla.org/en-US/Firefox/Multiprocess_Firefox/Message_Manager/Message_manager_overview#Global_parent_process_message_manager

lunakurame commented 6 years ago

I see you are talking about Firefox Nightly again. I just checked and it does work as you describe it. However, Firefox 57, which is the current stable release, still has this problem: https://dxr.mozilla.org/mozilla-release/search?q=browser.tabs.remote.autostart. Firefox 58.0b5 appears to behave like FF Nightly, so at least I'm glad it's gonna be fixed as soon as the new release rolls out.

Anyway, I think we could temporary sacrifice that option to make @import work properly in the userContent.css file, since the problem will disappear on January 23 (FF58 release date). It was nice to talk to someone who knows Firefox' code better than I do. Thanks, I'll commit a patch soon.

lunakurame commented 6 years ago

Okay after trying to implement this, I noticed @import doesn't work for HTML content in Firefox 57 if any of those are set to true:

I don't know why it worked before with browser.tabs.remote.autostart enabled, maybe they implemented a RNG... Anyway, I give up with that bug, either you disable e10s or you can't use userContent.css for HTML content. This applies only for Firefox 57 tho.

I just added @imports for customChrome.css and customContent.css, you can use them for your own styles. Also since all options are now disabled by default, you can store your config in these too and it's gonna survive when you git pull (copy and paste the relevant @imports... unless you prefer setting them up manually every time).

And again, thanks for help.