Closed Bug 1521224 Opened 5 years ago Closed 3 years ago

Consider moving the classes in UrlbarUtils.jsm to their own JSMs

Categories

(Firefox :: Address Bar, task, P3)

task

Tracking

()

RESOLVED WONTFIX

People

(Reporter: adw, Unassigned)

References

(Blocks 1 open bug)

Details

When I was getting back into quantumbar development, it was frustrating when I wanted to look at the context class because there isn't a UrlbarQueryContext.jsm, so until I memorized the name of the file it lives in, I had to keep grepping around to find it. It would be nicer for new contributors if it and other major classes lived in their own files. It would be a more logical way to organize important classes too.

Right now the only file that contains multiple important classes is UrlbarUtils.jsm. It contains QueryContext, UrlbarMuxer, UrlbarProvider. We should consider moving those classes to their own files.

I raised this at the meeting this week and Marco pointed out that there's some overhead with JSMs. I ask mconley on IRC about it. It doesn't seem to be a big problem:

mconley: adw: so, the overhead used to be way worse wrt memory

mconley: because each JSM had its own compartment. Now they all share a
compartment, so we lose that overhead which is nice

adw: mconley: ok, thanks. is there any other overhead you know of?

mconley: adw: I think these days, from what I can tell, conversion from JSM to
C++ is occurring mostly for performance reasons, where the JSM code is being
entered from native code over and over again, and crossing the XPConnect
boundary is costing us

mconley: Memory-wise, these days, I think JSMs are less of a pain than they
used to be (assuming they're only loaded when needed)

adw: in my case it’s all js

mconley: adw: I think you should be fine then

mconley: adw: I mean, there are other perf costs for JSMs that we have less
control over - I know the SpiderMonkey team is hoping we can shift to ES6
modules soonish

mconley: but that'll likely happen en masse

adw: ok, thanks!

mconley: well, maybe not en masse

mconley: but, it's not happening yet

Type: enhancement → task

Hi, I think that this bug is no longer valid in the face of the latest changes on the browser. If I'm mistaken, please reopen it.
Regards, Flor.

Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → WORKSFORME

Let's make this wontfix. There's no pressing need to do this and realistically we'll probably never do it.

Resolution: WORKSFORME → WONTFIX
You need to log in before you can comment on or make changes to this bug.