Open Bug 1410921 Opened 2 years ago Updated 7 months ago

All BrowserLoader instances in panels should be given a commonLibRequire argument

Categories

(DevTools :: General, defect, P3)

defect

Tracking

(Not tracked)

People

(Reporter: ochameau, Unassigned)

References

(Blocks 2 open bugs)

Details

(Keywords: regression)

Attachments

(3 files)

This was regressed by bug 1350215 (netmonitor) and bug 1402262 (console).

See bug 1333131 comment 3 which highlights the cost of dupliated React instances.

Toolbox already need to load React and various other significant 3rd party libraries.

In order to reuse these instances, BrowserLoader supports "commonLibRequire" argument which allow to share some modules.
As of today, it is only used by old Console frontend:
http://searchfox.org/mozilla-central/rev/8a6a6bef7c54425970aa4fb039cc6463a19c0b7f/devtools/client/webconsole/webconsole.js#232
But it should be used in every BrowserLoader instance. Except the toolbox one and code that lives outside of a toolbox like about:debugging, RDM or panels loaded in a tab.

This is missing in many places:
http://searchfox.org/mozilla-central/search?q=BrowserLoader(&case=false&regexp=false&path=
* animation inspector
* debugger
* dom
* memory
* netmonitor
* performance
* new webconsole frontend
Comment on attachment 8921106 [details]
Bug 1410921 - Fix netmonitor and webconsole URLs in react-dom monkey patch. ?

The URLs were outdated in react-dom patch. This reintroduces issues with DOM events. For example the filter bar doesn't toggle correctly.
Blocks: console-perf
We may want to add a test to ensure this behavior is preserved, so we can avoid regressing it (again) in the future.
Ah. This becomes interesting.
DAMP reports significant regression.
I'm wondering if it is:
* cross compartment wrappers between toolbox and panel documents?
  We end up using toolbox's React which comes from another document and so I imagine there may be wrappers here.
* cost of the monkey patch of ReactDOM?
  This patch involves Proxy, if this is in a React hot codepath, it must be slow:
  http://searchfox.org/mozilla-central/source/devtools/client/shared/vendor/REACT_UPGRADING.md#patching-buildreact-dom.js
Priority: -- → P2
It may be interesting to revisit this as the monkey patch has been removed.

Also, Julian recently highlighted that the inspector is doing something special, that can be replaced with a dedicated BrowserLoader using `commonLibRequire`.
Instead of that, it requires all react and component modules via toolbox's BrowserLoader instance, via toolbox.browserRequire/inspectorPanel.browserRequire:
  https://searchfox.org/mozilla-central/search?q=browserRequire(&case=false&regexp=false&path=%2Finspector%2F

(The toolbox has its own BrowserLoader instanciated here:
 https://searchfox.org/mozilla-central/rev/4114ad2cfcbc511705c7865a4a34741812f9a2a9/devtools/client/framework/toolbox.js#431)
As of today, `commonLibRequire` is no longer used anywhere.

It has been removed from console without clear reason in bug 1402262.

The inspector still uses toolbox loader to share react instances as described in comment 8.
Product: Firefox → DevTools
Moving this inactive P2 to the backlog (P3)
Priority: P2 → P3
You need to log in before you can comment on or make changes to this bug.