theme.js uses NetUtil for synchronous loading

RESOLVED FIXED in Firefox 51

Status

enhancement
P1
normal
RESOLVED FIXED
3 years ago
10 months ago

People

(Reporter: tromey, Assigned: tromey)

Tracking

unspecified
Firefox 51
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox51 fixed)

Details

(Whiteboard: [devtools-html])

Attachments

(1 attachment)

(Assignee)

Description

3 years ago
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]
(Assignee)

Comment 1

3 years ago
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]
(Assignee)

Comment 2

3 years ago
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
(Assignee)

Updated

3 years ago
Assignee: nobody → ttromey
Status: NEW → ASSIGNED
Iteration: --- → 50.4 - Aug 1
Priority: P3 → P1
Whiteboard: [reserve-html] → [devtools-html]
(Assignee)

Updated

3 years ago
Blocks: 1248830
(Assignee)

Comment 4

3 years ago
I wasn't sure if I needed to update the browser loader as well.
Attachment #8775676 - Flags: review?(jryans) → review-
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...
(Not sure why MozReview spat out HTML for one of the comments...)
(Assignee)

Comment 7

3 years ago
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.
(Assignee)

Comment 8

3 years ago
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/
Attachment #8775676 - Flags: review- → review?(jryans)
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`?
Attachment #8775676 - Flags: review?(jryans) → review+
(Assignee)

Comment 10

3 years ago
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/
Iteration: 50.4 - Aug 1 → 51.1 - Aug 15

Comment 11

3 years ago
Pushed by ttromey@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0850c3a2f986
support webpack "raw!" requires in devtools loader; r=jryans

Comment 12

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/0850c3a2f986
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51

Updated

10 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.