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

RESOLVED DUPLICATE of bug 1291049

Status

defect
RESOLVED DUPLICATE of bug 1291049
3 years ago
10 months ago

People

(Reporter: jdescottes, Assigned: tromey)

Tracking

Trunk
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

3 years ago
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?
Flags: qe-verify?
(Assignee)

Comment 1

3 years ago
I think what we should do use reuse the "devtoolsRequire" approach that the debugger took.
Assignee: nobody → ttromey
Status: NEW → ASSIGNED
Iteration: --- → 52.3 - Nov 14
Priority: P2 → P1
(Assignee)

Comment 2

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

Comment 3

3 years ago
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 hidden (mozreview-request)
(Reporter)

Comment 5

3 years ago
mozreview-review
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).
Attachment #8807719 - Flags: review?(jdescottes)
Comment hidden (mozreview-request)
(Reporter)

Comment 7

3 years ago
mozreview-review
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!
Attachment #8807719 - Flags: review?(jdescottes) → review+
Flags: qe-verify? → qe-verify-
(Assignee)

Comment 8

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

Comment 10

3 years ago
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 hidden (mozreview-request)

Comment 12

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

Comment 13

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b3ad07be866f
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
Depends on: 1316910
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: Firefox 52 → ---
Iteration: 52.3 - Nov 14 → 53.1 - Nov 28
(Reporter)

Comment 17

2 years ago
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.
Status: REOPENED → RESOLVED
Last Resolved: 3 years ago2 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1291049
No longer blocks: devtools-html-phase2
Iteration: 53.1 - Nov 28 → ---
Priority: P1 → --
Whiteboard: [devtools-html]

Updated

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