Blocking bug for Bug 1291049. See the current changes in https://reviewboard.mozilla.org/r/85170/diff/4#6 inspector.js and toolsidebar.js are both using browserRequire() in order to load React classes. This browserRequire is not recognized by webpack. In Bug 1291049 I'm currently checking if we are running in content or not in order to use a regular require call, but we need a proper solution to handle this. Some leads to investigate: - make browserRequire the default require used by the inspector client side files, and have a "chromeRequire" available when needed? - find a way to alias browserRequire to require in webpack config?

I think what we should do use reuse the "devtoolsRequire" approach that the debugger took.

Well, that was overly optimistic. The debugger seems to have a compile step before landing in m-c, so it rewrites some require()s to devtoolsRequire(). We can't currently do this for the inspector. (I find this situation mildly distressing, in that it means different components are now forced to find different solutions for the same problem - we have to fix this, though of course we're not going to do so in this bug...)

I came up with a patch to simply use the browser loader everywhere in the inspector. This does the right thing, I believe, because it defers to the devtools loader when needed. I did have to make sure to load react in the inspector, not the toolbox, to avoid loading two copies - that seems a bit ugly. This still leaves a use of Cu.import in inspector.xhtml. We're going to need some special startup magic, I suppose, to avoid this. There's also a use still remaining in inspector.js. This is used in the "query parameters" case. I tend to think this code should be moved elsewhere, but I admit I haven't given it a lot of thought.

Comment on attachment 8807719 [details] Bug 1310416 - use browser loader for inspector; https://reviewboard.mozilla.org/r/90788/#review90786 Sorry for the late review. Looks good but needs rebasing (my fault!). Heads-up, an additional call to browserRequire got added in inspector.js to load layout.js (which in turn no longer contains any browserRequire call).

Comment on attachment 8807719 [details] Bug 1310416 - use browser loader for inspector; https://reviewboard.mozilla.org/r/90788/#review91200 Works for me if try is green. Thanks Tom!

The test failure is because head.js uses the devtools require and not the loader used by the inspector; doing: var {CssRuleView} = require("devtools/client/inspector/rules/rules"); ... let isRuleView = view instanceof CssRuleView;

(In reply to Tom Tromey :tromey from comment #8) > The test failure is because head.js uses the devtools require and not the > loader used by the inspector; doing: > > var {CssRuleView} = require("devtools/client/inspector/rules/rules"); > ... > let isRuleView = view instanceof CssRuleView; Seems that test could check if it's a rule / computed view some other way like existence of a known property or DOM element, yes?

Assuming we are talking about browser_styleinspector_context-menu-copy-urls.js, one option is to do the following: > let isRuleView = view === inspector.ruleview.view;

Pushed by ttromey@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/b3ad07be866f use browser loader for inspector; r=jdescottes

Backed out in https://hg.mozilla.org/mozilla-central/rev/7f0c5e9ac1e6 If you want to add slowness to devtools tests on Linux32 debug, and it certainly seems like everyone does, you have to do one of two things, either add it in small enough amounts that nobody can tell you added it, or choose a time when the current runtime isn't so close to the maximum time allowed. Both dt4 and dt5 on Linux32 debug were within just a few minutes of the 4800 second runtime, so your added slowness pushed them over around half the time.

Backed out on aurora in https://hg.mozilla.org/releases/mozilla-aurora/rev/aeb8bcaafe56ee681cf19a13398cd14d163af6ca

I attempted some workaround by preloading the React dependancy which had a modest improvement on win7 (~1.5% regression instead of ~3%) [0] but still waiting on response from linux debug tests at [1] [0]: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=0502acdda560cd39e25aa9eb7f1233cf121ad2dd&newProject=try&newRevision=46f31ae8cfc6423930c1ee9e803cce3aa4e54c59%20&framework=1&filter=damp&showOnlyImportant=0 [1]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0de489f978b2ee5379cecf678da8cf582137a362

As discussed in standup, this bug will be fixed via a custom webpack loader that will rewrite occurrences of this.browserRequire() as require(). The patch will be tightly linked to the new inspector webpack config, and will therefore be submitted as part of Bug 1291049. Consequently closing this one as duplicate.