Closed Bug 1287915 Opened 8 years ago Closed 8 years ago

theme.js uses NetUtil for synchronous loading


(DevTools :: Framework, enhancement, P1)



(firefox51 fixed)

Firefox 51
51.1 - Aug 15
Tracking Status
firefox51 --- fixed


(Reporter: tromey, Assigned: tromey)



(Whiteboard: [devtools-html])


(1 file)

While looking into Cu.import uses I found that theme.js is
using NetUtil to load a URL synchronously.
This has to be dealt with somehow.  One idea might be to somehow
"require" it using whatever content loader we come up with.
Whiteboard: [devtools-html] → [devtools-html] [triage]
There's a use of NetUtil in actor-registry.js as well, but perhaps that one can
be deferred to whatever bug we'll have for transport.
Flags: qe-verify-
Priority: -- → P3
Whiteboard: [devtools-html] [triage] → [reserve-html]
I've added a note to the loader requirements (bug 1248830) to mention this.
One way to go would be to replicate the webpack loader syntax:
let css = require("raw!devtools/skin/variables.css");
Assignee: nobody → ttromey
Iteration: --- → 50.4 - Aug 1
Priority: P3 → P1
Whiteboard: [reserve-html] → [devtools-html]
Blocks: 1248830
I wasn't sure if I needed to update the browser loader as well.
Comment on attachment 8775676 [details]
Bug 1287915 - support webpack "raw!" requires in devtools loader;

<p>I think it would be good to have it work in the browser loader as well.  Browser loader is mostly an "overlay" that gives certain files access to the window global, but otherwise behaves a lot like the regular loader in Loader.jsm and even falls back to the regular loader for files outside certain directories.</p>
<p>Browser loader defines its own <code>requireHook</code>.  Not sure what the nicest integration point is...  You could have its hook also check for raw and call the new module mentioned below, or try to chain the hooks, so that it ends up calling Loader.jsm's hook?  I believe without any changes it already would reach Loader.jsm's hook for files outside the directories it cares about, but we probably want it to accept raw syntax for any file.</p>

::: devtools/shared/Loader.jsm:29
(Diff revision 1)
>  /**
> + * A require hook for BuiltinProvider that understands webpack-style
> + * "raw!" requires.  See
> + */
> +function requireHook(id, require) {

It might be nice to move this out to its own module, maybe `loader-plugin-raw.js` or something, since it's pretty self contained.

Loader.jsm seems like it would own the actual requireHook, and it could check for `raw!` and pass to the new module if it is found.

It might make more sense to define the `requireHook` function inside BuiltinProvider.load, to parallel the `paths` setup, or as part of the `setProvider` work.  It's a bit unclear these days since there is only one provider...
(Not sure why MozReview spat out HTML for one of the comments...)
The updated patch moves the new function to a jsm (as discussed on irc) and puts
the requireHook into BuiltinProvider.load.

I needed a small hook on DevToolsLoader so that the browser loader could determine
if the require in question is one that has to be delegated specially.  I considered
having the DevToolsLoader implementation of isSpecialRequireId just call a similar
method on the provider; which is more abstract but also perhaps overkill.  This is
easily changed if you want.

This approach should make it reasonably easy to add new kinds of loader modules if
desired for some reason (though there are no concrete plans for this).

I added a browser loader "raw!" test as well.
Comment on attachment 8775676 [details]
Bug 1287915 - support webpack "raw!" requires in devtools loader;

Review request updated; see interdiff:
Attachment #8775676 - Flags: review- → review?(jryans)
Comment on attachment 8775676 [details]
Bug 1287915 - support webpack "raw!" requires in devtools loader;

Thanks, I think this looks good to proceed (with the tweaks noted below).  We can always edit the integration points here later on if we find a better way since it's a simple API between the modules.

::: devtools/client/shared/test/browser_require_raw.js:15
(Diff revision 2)
> +const { require: browserRequire } = BrowserLoader({
> +  baseURI: "resource://devtools/client/shared/",
> +  window: this
> +});
> +
> +const variableFileContents = browserRequire("raw!devtools/client/themes/variables.css");

Since the intent is for raw! to also work for regular loaders too, let's add another check to verify it works from Loader.jsm also.

::: devtools/shared/Loader.jsm:134
(Diff revision 2)
>    /**
> +   * Return true if |id| requires special handling beyond what the
> +   * addon loader provides.
> +   */
> +  isSpecialRequireId: function (id) {

Maybe `isLoaderPluginId`?
Attachment #8775676 - Flags: review?(jryans) → review+
Comment on attachment 8775676 [details]
Bug 1287915 - support webpack "raw!" requires in devtools loader;

Review request updated; see interdiff:
Iteration: 50.4 - Aug 1 → 51.1 - Aug 15
Pushed by
support webpack "raw!" requires in devtools loader; r=jryans
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.