MrOtherGuy / fx-autoconfig

Load custom javascript in browser context
Mozilla Public License 2.0
173 stars 11 forks source link

Various questions about 0.8.5 #38

Open aminomancer opened 1 year ago

aminomancer commented 1 year ago

I've just been testing 0.8.5 and I have a few questions and suggestions. Just wanted to let you know nothing seems to be broken for me. I had to change a couple things since the API has changed slightly, but everything is working. The overall organization is really great too.

The new CSS features are pretty cool. I'd like to replace my ad hoc implementation with the new API. But I have some special considerations, particularly because of this. In order to get all this stuff (both stylesheets and scripts) to load in arbitrary browser toolboxes, I had to make config.js load the manifest from the root profile directory (so like chrome_debugger_profile/../chrome/utils/chrome.manifest), then make everything else that uses nsIFile navigate up to the root directory too.

This is a pretty specific use case, so I don't expect fx-autoconfig to explicitly support it. It's hard to support it as an opt-in setting, because the autoconfig loader itself needs to know where to look for the manifest very early in startup. So it's probably always gonna require modifying at least one file. But I suppose those instructions could be simplified a lot if all fx-autoconfig's file operations used the chrome registry. Then, within the browser toolbox context, we'd only need to do 1 thing differently — change the path of the manifest from currentProfile/utils/chrome.manifest to rootProfile/utils/chrome.manifest. And everything else could just follow from that, since the registered URLs would be the same in every context.

Once you've modified config.js so it can find the real manifest, this:

Cc["@mozilla.org/chrome/chrome-registry;1"].getService(Ci.nsIChromeRegistry)
  .convertChromeURL(Services.io.newURI("chrome://userchrome/content/"))
  .QueryInterface(Ci.nsIFileURL).file.parent.path

already returns path/to/root_profile/chrome/resources/ in the toolbox (rather than path/to/chrome_debugger_profile/chrome/resources/, which doesn't exist).

And that already seems to be the case with most of the file system functions now, like FS.getStyleDir(). But there are a few things that still get an nsIFile object by navigating from UChrm, which leads to the chrome_debugger_profile in this case. Specifically, FileSystem.BASE_FILEURI, FileSystem.#getEntry, and FileSystem.chromeDir. Idk how easy or desirable it is to refactor these to start from chrome URIs, since they are ultimately trying to get to the chrome dir, which is not itself registered.

But I think you could simply assume the chrome dir is the parent of one of the directories that is registered, probably the utils folder. chrome.manifest already has to stipulate the relationship between itself and the chrome folder, e.g. content userscripts ../JS/ only works if the manifest is in profile/chrome/utils/. So I think it wouldn't be unreasonable to just universally treat the chrome dir as the parent of the utils dir, since if it somehow is not, all of this breaks down anyway unless the user has made major modifications to all the major files.

Having done that, the starting point for the whole fx-autoconfig file system is then a function of the manifest location, which is stipulated by config.js. So you'd only have to modify one thing to make it dynamically locate a single installation of fx-autoconfig. If you do end up implementing INI configuration feature, you could just add a setting for that, instead of me modifying config.js directly.


Something that's impeding my adoption of the new CSS features is that the way they're injected precludes easily saving them from the style editor. When they are loaded via file:// URI, you can live edit them. It has some downsides because there are some :-moz-* prefixed pseudo classes that check the principal and basically require the stylesheet to be registered. I have worked around that by confining those kinds of rules to a separate file that I won't need to edit as often.

A related issue is importing shared stylesheets. If the stylesheet is loaded from a chrome URI, then anything it imports needs to also be imported from a chrome URL. I'm currently doing @import url(uc-globals.css) from my agent sheet, which works since they're both in the same folder. Moving the agent sheet into the CSS folder, I could do @import url(../uc-globals.css), but only if I loaded the agent sheet from its file URI. So as it is, I'd need to register uc-globals.css with a chrome URL, making it so neither can be saved from the devtools.

But it seems like all of this could still work using file URIs. So, there could be a metadata directive controlling whether a file gets injected with a chrome URL or a file URL.

let uri = style.chromeURI;
if (style.useFileURI) {
  uri = FS.convertChromeURIToFileURI(uri);
}
sss.loadAndRegisterSheet(uri, sss.AGENT_SHEET);
ScriptData.markScriptRunning(style);

Another minor weakness of my approach to styling browser toolbox windows arises because fx-autoconfig uses prefs to manage disabling scripts. This might be resolved by your suggestion in #37 though. Basically, the toolbox window doesn't know which scripts are disabled because the toolbox profile may have its own prefs, until it's deleted or something. With INI configuration and the registry-based file navigation I described, the toolbox instance of fx-autoconfig would be able to find the root profile's config.


This could just be an oversight on my part, but I don't understand what's preventing agent sheets from trying (and failing) to load twice. You're adding them to the agentStyleSet, which gets loaded by the stylesheet service. But then you're also adding them to this.styles, like the author sheets. Then on DOMContentLoaded, you check aScript.type === "style" && aScript.styleSheetMode === "author" and return early for author sheets. But agent sheets should just carry on and ultimately reach this:

const injection = aScript.isESM
  ? ScriptData.injectESMIntoGlobal(aScript,win)
  : ScriptData.injectClassicScriptIntoGlobal(aScript,win);

So isn't it going to try executing an agent sheet as JS with the subscript loader? I might be missing something. I didn't notice any errors logged, but idk whether to expect any in this case.


I noticed you're using ChromeUtils.compileScript now for ESM window/page scripts. So I figured this could have some performance benefits. But I'm curious what's the reasoning behind the data URI?

ChromeUtils.compileScript(`data:,"use strict";import("${aScript.chromeURI.spec}").catch(console.error)`)

Is there any reason to think that could change behavior or perhaps how the debugger interacts with scripts?

MrOtherGuy commented 1 year ago

I had to make config.js load the manifest from the root profile directory (so like chrome_debugger_profile/../chrome/utils/chrome.manifest), then make everything else that uses nsIFile navigate up to the root directory too.

Is there a reason you edit config.js here? I kinda do the same thing in the test_profile where the manifest maps chrome paths to point to other locations. Any such re-mappings should happen through the manifest and I don't immediately see why it wouldn't work for the browser_toolbox profile.

So I think it wouldn't be unreasonable to just universally treat the chrome dir as the parent of the utils dir, since if it somehow is not, all of this breaks down anyway unless the user has made major modifications to all the major files.

So, the basis of file system operations would be the parent of whatever directory chrome://userchrome/content/ resolves to and not UChrm? This sounds wrong to me to be honest. In my view, the whole operating context of fx-autoconfig should be the active profile and by extension its chrome directory. See, in test_profile I have this structure:

<profile>
  |__chrome
  | |__css
  | |__utils
  |   |__boot.sys.mjs
  |   |__chrome.manifest
  |__test_profile
    |__chrome
      |__tests
      |__utils
         |__chrome.manifest

The chrome.manifest of the test_profile is what config.js ends up loading. That manifest maps:

So here, the loader and styles exist outside of the current profile. But indeed, like you have said, FS operations still try to access file inside the active profile. This is arguably wrong, FS.getEntry() does this as a hacky way to support relative url (like ../), and simply appends it to UChrm. That is... well it's not great to say the least, but it would be more wrong to always get the parent of utils and appending the path to that. In any event, I should definitely change #getEntry to not look for UChrm specifically, but to one of defined folders instead.

But it seems like all of this could still work using file URIs. So, there could be a metadata directive controlling whether a file gets injected with a chrome URL or a file URL.

Yep, I could totally do that, this is great suggestion, thanks! The main reason for having them be chrome:// urls is exactly what you pointed out - some features are not available for file:// uri style sheets. Calling _ucUtils.updateStyleheet(sheetname,"author") should update the style though - of course, it's still not really live-editable, but it's something.

This could just be an oversight on my part, but I don't understand what's preventing agent sheets from trying (and failing) to load twice. You're adding them to the agentStyleSet, which gets loaded by the stylesheet service. But then you're also adding them to this.styles, like the author sheets.

Okay, the agentStyleSet is not to prevent duplicate entries. It is just to store a subset of styles that are agent styles. And then after all style files are read by the loader send that subset to StyleSheetService to be registered all at once. During onDOMContent they should end up and return here because any Style scripts are also marked with noExec = true during parsing. The reason for styles and scripts arrays is to store objects that the loader is aware of - regardless of if they are enabled or not. Functionally we could just forget agent styles after parsing because we don't do anything about them afterwards (except generate menubar menu items) but its easier to keep reference to them for the whole runtime.

I noticed you're using ChromeUtils.compileScript now for ESM window/page scripts. So I figured this could have some performance benefits. But I'm curious what's the reasoning behind the data URI?

Honestly, please do something about it :) No, really this was the only way I found with which I was able to make the script file itself a module script. I mean here, the data-uri "script" is not loaded as module script, it is just a wrapper that loads the script you care about as a module script. If only Services.scriptloader.loadSubScriptWithOptions() would have an option to inject the subscript as module, but alas it doesn't.

MrOtherGuy commented 1 year ago

I've added support for a new @usefileuri meta-tag in 65fac03 - actually the patch also removes that extra agentStyleSet construction and instead just filters them out during call to addAgentStyles() - this should make the intention more clear, hopefully.

aminomancer commented 1 year ago

I had to make config.js load the manifest from the root profile directory (so like chrome_debugger_profile/../chrome/utils/chrome.manifest), then make everything else that uses nsIFile navigate up to the root directory too.

Is there a reason you edit config.js here? I kinda do the same thing in the test_profile where the manifest maps chrome paths to point to other locations. Any such re-mappings should happen through the manifest and I don't immediately see why it wouldn't work for the browser_toolbox profile.

The browser toolbox profile doesn't have a manifest at all. These profiles are created on the spot when opening the toolbox, and I delete them pretty frequently. I feel like the devtools, especially the toolbox, are pretty fragile. Bugs are pretty frequent and sometimes a toolbox just fails to load. Especially in some of my testing profiles that I don't use frequently, where maybe 4 or 5 major releases pass without my opening the devtools in that profile. It'll just show a blank page, but if I close it and delete the toolbox profile and reopen it, it's able to construct a new one just fine. So deleting these periodically has just become part of my routine, I guess.

Plus, a toolbox profile can have an arbitrary number of additional toolbox profiles nested within each other, if you open a toolbox on another toolbox. So that would require you to recreate files in every profile, every time a new one is created. Editing config.js is something I did once several years ago and I haven't needed to touch it again (until this recent change to .sys.mjs). That time saving is the rationale behind all these changes.

You're right that adding a custom manifest for the toolbox profile is less effort than just duplicating the whole chrome directory in the toolbox profile. But profile/chrome_debugger_profile needs its own paths, and profile/chrome_debugger_profile/chrome_debugger_profile needs its own, and so on. I guess there could be ways of automating it, but it's so much easier to just tell config.js to look for the manifest in the real profile. So far, I've never had any cause to put custom files in the chrome_debugger_profile. Like, when I want to customize the toolbox specifically, I wouldn't give it its own chrome folder, I would just add a script to my real profile and restrict it to the toolbox URL.

I get what you're saying, fx-autoconfig is basically installed to a profile, and all its filesystem operations should be relative to that profile. I guess the way I'm thinking about this, fx-autoconfig would be installed to a "real" profile, and then automagically apply to every "child" toolbox profile. Of course, if someone actually installed fx-autoconfig in the chrome_debugger_profile/chrome folder, then it could load that first, and only try to load the "real" profile's files as a fallback, when a manifest is not found within chrome_debugger_profile.

This could just be an oversight on my part, but I don't understand what's preventing agent sheets from trying (and failing) to load twice. You're adding them to the agentStyleSet, which gets loaded by the stylesheet service. But then you're also adding them to this.styles, like the author sheets.

any Style scripts are also marked with noExec = true during parsing.

Ah man, I knew I must have been missing something obvious like that. My bad.

The reason for styles and scripts arrays is to store objects that the loader is aware of - regardless of if they are enabled or not. Functionally we could just forget agent styles after parsing because we don't do anything about them afterwards (except generate menubar menu items) but its easier to keep reference to them for the whole runtime.

I see what you mean. That makes sense now!

I noticed you're using ChromeUtils.compileScript now for ESM window/page scripts. So I figured this could have some performance benefits. But I'm curious what's the reasoning behind the data URI?

Honestly, please do something about it :) No, really this was the only way I found with which I was able to make the script file itself a module script. I mean here, the data-uri "script" is not loaded as module script, it is just a wrapper that loads the script you care about as a module script. If only Services.scriptloader.loadSubScriptWithOptions() would have an option to inject the subscript as module, but alas it doesn't.

That's fascinating. If I understand correctly, the goal here is to load an ESM page script in the chrome window in the same way we can load .mjs files in content. The motivation being to give the script access to static import declarations? Actually, I would have expected <script type="module" src="${aScript.chromeURI.spec}"> to work out of the box. But I guess the chrome window is pretty weird. I'll try messing around with it sometime.


I've added support for a new @usefileuri meta-tag in 65fac03 - actually the patch also removes that extra agentStyleSet construction and instead just filters them out during call to addAgentStyles() - this should make the intention more clear, hopefully.

Awesome! Yeah, removing the set helps clarify the part I got confused about.

aminomancer commented 1 year ago

By the way, one other suggestion that just occurred to me — I mentioned earlier how I have 2 author sheets. Till now, the main one was loaded by file URI, and it imported the second one by chrome URI. So the cascading order was explicit that way.

With the new @usefileuri tag, I can remove the CSS import and just put both author sheets in the CSS folder, with the main one using the new tag, and the second one not using it. But the cascading order then becomes dependent on file names (I think). So I went to check if I can use @loadOrder, but it looks like that only works for scripts. Not a big deal for my use case since I can just keep using CSS import for now, but it could be a nice feature for 0.9.

MrOtherGuy commented 1 year ago

Plus, a toolbox profile can have an arbitrary number of additional toolbox profiles nested within each other, if you open a toolbox on another toolbox. So that would require you to recreate files in every profile, every time a new one is created. Editing config.js is something I did once several years ago and I haven't needed to touch it again (until this recent change to .sys.mjs). That time saving is the rationale behind all these changes.

Fair enough. I feel like an argument could be made that chrome_debugger_profile should behave like you describe - so that it inherits the autoconfig from the parent, but I think there is a bit of a risk there with somebody then accidentally affecting the toolbox profile causing who-knows-what. Besides, if editing config.js is all you need for your purpose then I suppose that's fine. Another possibility that comes to mind, I suppose, is to have a script generate a modified copy of the chrome.manifest when toolbox profile is created.

That's fascinating. If I understand correctly, the goal here is to load an ESM page script in the chrome window in the same way we can load .mjs files in content. The motivation being to give the script access to static import declarations? Actually, I would have expected Githubissues.

  • Githubissues is a development platform for aggregating issues.