1310416 - Can not use browserRequire in inspector.js when running in content tab

Status: RESOLVED DUPLICATE of bug 1291049

(DevTools :: Inspector, defect)

Description

[Julian Descottes [:jdescottes]]

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?

Comment 1

[Tom Tromey :tromey]

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

Comment 2

[Tom Tromey :tromey]

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...)

Comment 3

[Tom Tromey :tromey]

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 4

[Tom Tromey :tromey]

Attachment: Bug 1310416 - use browser loader for inspector;

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

Comment 5

[Julian Descottes [:jdescottes]]

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 6

[Tom Tromey :tromey]

Comment on attachment 8807719 [details]
Bug 1310416 - use browser loader for inspector;

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

Comment 7

[Julian Descottes [:jdescottes]]

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!

Comment 8

[Tom Tromey :tromey]

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;

Comment 9

[Brian Grinstead [:bgrins]]

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

Comment 10

[Julian Descottes [:jdescottes]]

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;

Comment 11

[Tom Tromey :tromey]

Comment on attachment 8807719 [details]
Bug 1310416 - use browser loader for inspector;

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

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b3ad07be866f
use browser loader for inspector; r=jdescottes

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b3ad07be866f

Comment 14

[Phil Ringnalda (:philor)]

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.

Comment 15

[Phil Ringnalda (:philor)]

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

Comment 16

[Brian Grinstead [:bgrins]]

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

Comment 17

[Julian Descottes [:jdescottes]]

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.