1287915 - theme.js uses NetUtil for synchronous loading

Status: RESOLVED FIXED

(DevTools :: Framework, enhancement, P1)

Description

[Tom Tromey :tromey]

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.

Comment 1

[Tom Tromey :tromey]

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.

Comment 2

[Tom Tromey :tromey]

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");
See https://github.com/webpack/raw-loader

Comment 3

[Tom Tromey :tromey]

Attachment: Bug 1287915 - support webpack "raw!" requires in devtools loader;

Review commit: https://reviewboard.mozilla.org/r/67816/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/67816/

Comment 4

[Tom Tromey :tromey]

I wasn't sure if I needed to update the browser loader as well.

Comment 5

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8775676 [details]
Bug 1287915 - support webpack "raw!" requires in devtools loader;

https://reviewboard.mozilla.org/r/67816/#review65218

<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 https://github.com/webpack/raw-loader.
> + */
> +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...

Comment 6

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(Not sure why MozReview spat out HTML for one of the comments...)

Comment 7

[Tom Tromey :tromey]

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 8

[Tom Tromey :tromey]

Comment on attachment 8775676 [details]
Bug 1287915 - support webpack "raw!" requires in devtools loader;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67816/diff/1-2/

Comment 9

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8775676 [details]
Bug 1287915 - support webpack "raw!" requires in devtools loader;

https://reviewboard.mozilla.org/r/67816/#review65694

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`?

Comment 10

[Tom Tromey :tromey]

Comment on attachment 8775676 [details]
Bug 1287915 - support webpack "raw!" requires in devtools loader;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67816/diff/2-3/

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0850c3a2f986
support webpack "raw!" requires in devtools loader; r=jryans

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/0850c3a2f986