Closed Bug 1434374 Opened 2 years ago Closed 2 years ago

Migrate devtools to use ChromeUtils import methods rather than Cu.import and XPCOMUtils

Categories

(DevTools :: General, enhancement)

Product:
DevTools
Component:
General
Type:
enhancement
Priority:
Not set

Tracking

(firefox60 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 60
Tracking Flags:
Tracking Status
firefox60 --- fixed

People

(Reporter: kmag, Assigned: ochameau)

References

Details

Attachments

(4 files, 1 obsolete file)

Bug 1434374 - Always import deprecated-sync-promises as a module.
59 bytes, text/x-review-board-request
jdescottes
: review+
Details
Bug 1434374 - Replace usages of Cu.import in modules (where ChromeUtils isn't defined)
59 bytes, text/x-review-board-request
jdescottes
: review+
Details
Bug 1434374 - Auto-rewrite Cu.import into ChromeUtils.import or require calls.
59 bytes, text/x-review-board-request
jdescottes
: review+
Details
Bug 1434374 - Remove useless Cu imports.
59 bytes, text/x-review-board-request
jdescottes
: review+
Details 
This is more difficult than the migration for most of the rest of the tree, since ChromeUtils is not available to devtools modules by default. We should probably migrate most callers of Cu.import to use require() instead, if we can do it without a performance hit. Then all we need to worry about is the left-over unused Cu declarations that are likely to result.
See Also: → 1434543
(In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're blocked) from comment #0)
> We should probably migrate most callers of Cu.import to use require() instead,

Sounds good to me.
Do not hesitate to delegate to us such work.
(In reply to Alexandre Poirot [:ochameau] from comment #1)
> Sounds good to me.
> Do not hesitate to delegate to us such work.

Fine with me if it's OK with you. I used this script to do the rewrite for the rest of the tree:

https://bitbucket.org/kmaglione/m-c-rewrites/src/8f1732b171db0102c781a6c2668098d2a492aeca/processors/chromeutils-import.jsm?at=default&fileviewer=file-view-default

It can probably be modified pretty easily to the `require()` rewrite. Removing unused `Cu` declarations might be more work.
Assignee: kmaglione+bmo → nobody
Assignee: nobody → poirot.alex
It wasn't trivial as there was other refactoring conflicting with this (Cu removal).
And because of DevTools tests (mochitests, xpcshell). Their scripts aren't commonJS modules and so don't have access to require.
Most of them import Loader.jsm to have require, but many don't and only pull JSMs via Cu.import.

So I decided to convert all the codebase but test to use require() for JSM.
There is a couple of exceptions detailed in the rewrite script I attached to this bug.
For example, we have a couple of JSM still in DevTools and they are using .js extension.
It breaks them if we use require() for them as the loader will consider them as commonjs modules.

If you think that is worth, we can followup to convert all the tests that have require() to use require for JSMs.
But it would be best done in a dedicated followup.
Comment on attachment 8949051 [details]
Bug 1434374 - rewrite scripts

https://reviewboard.mozilla.org/r/218446/#review224486

Do we really intend to land this? If so it needs to be documented, maybe moved inside devtools?
Attachment #8949051 - Flags: review-
Comment on attachment 8949050 [details]
Bug 1434374 - Remove useless Cu imports.

https://reviewboard.mozilla.org/r/218444/#review224484
Attachment #8949050 - Flags: review?(jdescottes) → review+
Comment on attachment 8949049 [details]
Bug 1434374 - Auto-rewrite Cu.import into ChromeUtils.import or require calls.

https://reviewboard.mozilla.org/r/218442/#review224480
Attachment #8949049 - Flags: review?(jdescottes) → review+
Comment on attachment 8949048 [details]
Bug 1434374 - Replace usages of Cu.import in modules (where ChromeUtils isn't defined)

https://reviewboard.mozilla.org/r/218440/#review224478

::: devtools/client/webconsole/webconsole.js:15
(Diff revision 1)
>  
>  const {Utils: WebConsoleUtils, CONSOLE_WORKER_IDS} =
>    require("devtools/client/webconsole/utils");
>  const { getSourceNames } = require("devtools/client/shared/source-utils");
> -const BrowserLoaderModule = {};
> -Cu.import("resource://devtools/client/shared/browser-loader.js", BrowserLoaderModule);
> +const ChromeUtils = require("ChromeUtils");
> +const { BrowserLoader } = ChromeUtils.import("resource://devtools/client/shared/browser-loader.js", {});

Add a comment to highlight that base-loader should not be loaded with require

::: devtools/server/actors/call-watcher.js:11
(Diff revision 1)
>  /* global XPCNativeWrapper */
>  
>  const {Ci, Cu} = require("chrome");
>  const protocol = require("devtools/shared/protocol");
> -const {serializeStack, parseStack} = Cu.import("resource://devtools/shared/base-loader.js", {});
> +const ChromeUtils = require("ChromeUtils");
> +const {serializeStack, parseStack} = ChromeUtils.import("resource://devtools/shared/base-loader.js", {});

ditto
Attachment #8949048 - Flags: review?(jdescottes) → review+
Comment on attachment 8949047 [details]
Bug 1434374 - Always import deprecated-sync-promises as a module.

https://reviewboard.mozilla.org/r/218438/#review224460
Attachment #8949047 - Flags: review?(jdescottes) → review+
Thanks for the cleanup! r+ if there is a green try.
(In reply to Julian Descottes [:jdescottes][:julian] from comment #9)
> Comment on attachment 8949051 [details]
> Bug 1434374 - rewrite scripts
> 
> https://reviewboard.mozilla.org/r/218446/#review224486
> 
> Do we really intend to land this? If so it needs to be documented, maybe
> moved inside devtools?

No, that's the reason why I did not r? it, but may be it wasn't obvious from mozreview ui ;)
I pushed it mostly to help review the "Auto-rewrite Cu.import into ChromeUtils.import or require calls." which is fully automated and very long!
Attachment #8949051 - Attachment is obsolete: true
(In reply to Alexandre Poirot [:ochameau] from comment #15)
> (In reply to Julian Descottes [:jdescottes][:julian] from comment #9)
> > Comment on attachment 8949051 [details]
> > Bug 1434374 - rewrite scripts
> > 
> > https://reviewboard.mozilla.org/r/218446/#review224486
> > 
> > Do we really intend to land this? If so it needs to be documented, maybe
> > moved inside devtools?
> 
> No, that's the reason why I did not r? it, but may be it wasn't obvious from
> mozreview ui ;)
> I pushed it mostly to help review the "Auto-rewrite Cu.import into
> ChromeUtils.import or require calls." which is fully automated and very long!

Ah yes I totally missed that :)
Pushed by apoirot@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/00db0a388e72
Always import deprecated-sync-promises as a module. r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/1f077e990b48
Replace usages of Cu.import in modules (where ChromeUtils isn't defined) r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/bdcaee53dff4
Auto-rewrite Cu.import into ChromeUtils.import or require calls. r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/7756d789a6e7
Remove useless Cu imports. r=jdescottes
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.