Remove usage of jsm files so we can reload

NEW
Assigned to

Status

defect
4 years ago
5 months ago

People

(Reporter: jlong, Assigned: ochameau, NeedInfo)

Tracking

(Depends on 1 bug, Blocks 2 bugs, {addon-compat, leave-open})

40 Branch
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 1 obsolete attachment)

This might be a lot of work but we need a bug for it. In order to reload our tools, we need to avoid using `Cu.import` anywhere, which involves not using .jsm files. I don't see any major barriers for doing this, just a lot of grunt work to clean up our files.
Assignee: nobody → jlong
A couple of random thoughts:
How far do we go with this e.g. does this mean moving Services.jsm to a js file and then using require?

It would also be great to always use lazy getters but then we couldn't deconstruct values:
e.g. let {x, y, z} = Cu.Import("path/blah.jsm");
(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #1)
> A couple of random thoughts:
> How far do we go with this e.g. does this mean moving Services.jsm to a js
> file and then using require?

I doubt it, for special cases like that every module just needs to make sure that if they mutate a service in any way, they "unmutate" it when being reloaded. For example adding an event handler on the Observer service. I already ran into this when playing with loaders: promise and event-emitter don't unregister themselves from the observer service, but it shouldn't be hard to do that.

These services are global C++ singleton services (as far as I know) so we'll always have to deal with them like that.

> It would also be great to always use lazy getters but then we couldn't
> deconstruct values:
> e.g. let {x, y, z} = Cu.Import("path/blah.jsm");

Yeah, I'm not convinced that we really need to lazy load that much. We shouldn't ever need to lazy load shared files, as they are almost always needed immediately anyway. The only times we should lazy load in my opinion is when a tool tried to access a different tool (like the console loads a location in the debugger), the console shouldn't load the whole debugger.
Thinking about it more, I guess there are several places where it's important to lazy-load, but Jordan is working through the loader APIs to make that easy, so we'll figure that out.
Note that JSMs can be unloaded too, but we would still prefer a more common API (especially outside Mozilla) like require():

https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Language_Bindings/Components.utils.unload
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8893f500dc03
I kept that out of the refactoring patch, as we may want to keep it for addons??
But I ran on try with this patch to see if that pass without it!
Comment on attachment 8639766 [details] [diff] [review]
Use DevToolsUtils module instead of JSM

This patch is really naive.
Replace Cu.import(DevToolsUtils.jsm) by require(DevToolsUtils.js)
with some related cleanup here and there.

Note that there is no need to lazy load some modules if:
- they are already loaded (Loader.jsm)
- they are going to be used by the related module no matter what (DevtoolsUtils in some cases)
Attachment #8639766 - Flags: review?(jwalker)
Comment on attachment 8639767 [details] [diff] [review]
Remove DevToolsUtils.jsm

This one is more at risk as it's not clear if any addon is using this module or not.
Attachment #8639767 - Flags: review?(jwalker)
(In reply to Alexandre Poirot [:ochameau] from comment #8)
> Comment on attachment 8639767 [details] [diff] [review]
> Remove DevToolsUtils.jsm
> 
> This one is more at risk as it's not clear if any addon is using this module
> or not.

If I look at this, there is only two addons using it on AMO:
https://mxr.mozilla.org/addons/search?string=DevToolsUtils.jsm&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=addons
  ICS Inspector
  Remote Developer Tools Server
Both written by
  Philipp Kewisch
Attachment #8639767 - Flags: review?(jwalker) → review+
Comment on attachment 8639766 [details] [diff] [review]
Use DevToolsUtils module instead of JSM

Review of attachment 8639766 [details] [diff] [review]:
-----------------------------------------------------------------

My opinion is that we should avoid devtools.require and define a require const instead to make it easier to move to an html5 module system in the future.
I wouldn't r- this for that though.
Attachment #8639766 - Flags: review?(jwalker) → review+
Philipp, this is just to let you know that your two addons may break with upcoming Fx release.
You would just need to replace:
  Cu.import("resource://gre/modules/devtools/DevToolsUtils.jsm");
By:
  let {devtools} = Cu.import("resource://gre/modules/devtools/Loader.jsm", {});
  let DevToolsUtils = devtools.require("devtools/toolkit/DevToolsUtils");
Flags: needinfo?(philipp)
Keywords: leave-open
(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #1)
> A couple of random thoughts:
> How far do we go with this e.g. does this mean moving Services.jsm to a js
> file and then using require?

My initiative here is to convert most *devtools* JSM,
that to ensure that `tools srcdir`/`tools reload` gcli commands, correctly reload devtools from sources. We plan to eventually have devtools sources in a distinct repo.
So that we only care about devtools modules, the others are going to be pulled out of Firefox resources.
(In reply to Joe Walker [:jwalker] (overloaded - needinfo me or ping on irc) from comment #10)
> Comment on attachment 8639766 [details] [diff] [review]
> Use DevToolsUtils module instead of JSM
> 
> Review of attachment 8639766 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> My opinion is that we should avoid devtools.require and define a require
> const instead to make it easier to move to an html5 module system in the
> future.

Sounds like a separate issue. I do have idea that would ease that and could be another refactoring.
See bug 1172010 comment 2. In most cases I just reuse devtools.require, like other usage in the same file. I filled bug 1188401.
Thanks for working on this! re-assigning to you for now. I was going to audit our usage of it soon but it's great your actually landing changes.
Assignee: jlong → poirot.alex
Thanks for the note. I'll make sure to change this. From what I have been reading I believe the new way to import is backwards compatible, in case not please let me know.
Flags: needinfo?(philipp)
This time, with a green try.
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=e8c874cde522

I removed the JSM in this patch as it sounds even less used than DevToolsUtils.
But actually, two addons are using it:
https://mxr.mozilla.org/addons/search?string=LayoutHelpers.jsm
  DevTools Tweaks by Luke Bryan
    (Pinged here about this - https://github.com/programmin1/DevTools-Tweaks/issues/6)
  YouTube Animated Thumbnails by victor
    (Will ni? him)
Attachment #8639895 - Attachment is obsolete: true
Attachment #8640382 - Flags: review?(jwalker)
Victor, please ack the upcoming breakage for your "YouTube Animated Thumbnails" addon.
Flags: needinfo?(vporof)
I'm going to handle SourceMap.jsm in existing bug 935366.
Depends on: 935366
Attachment #8640382 - Flags: review?(jwalker) → review+
(In reply to Alexandre Poirot [:ochameau] from comment #20)
> Victor, please ack the upcoming breakage for your "YouTube Animated
> Thumbnails" addon.

Ugh, ok.
Flags: needinfo?(vporof)
Depends on: 1190452
Depends on: 1192863
Depends on: 1386299
Depends on: 1242987
Depends on: 1399449
Duplicate of this bug: 996596
Product: Firefox → DevTools

The leave-open keyword is there and there is no activity for 6 months.
:ochameau, maybe it's time to close this bug?

Flags: needinfo?(poirot.alex)
You need to log in before you can comment on or make changes to this bug.