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

Status: RESOLVED FIXED

(DevTools :: General, enhancement)

Description

[Kris Maglione [:kmag]]

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.

Comment 1

[Alexandre Poirot [:ochameau]]

(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.

Comment 2

[Kris Maglione [:kmag]]

(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.

Comment 3

[Alexandre Poirot [:ochameau]]

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 4

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1434374 - Always import deprecated-sync-promises as a module.

Review commit: https://reviewboard.mozilla.org/r/218438/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/218438/

Comment 5

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1434374 - Replace usages of Cu.import in modules (where ChromeUtils isn't defined)

Review commit: https://reviewboard.mozilla.org/r/218440/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/218440/

Comment 6

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1434374 - Auto-rewrite Cu.import into ChromeUtils.import or require calls.

Review commit: https://reviewboard.mozilla.org/r/218442/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/218442/

Comment 7

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1434374 - Remove useless Cu imports.

Review commit: https://reviewboard.mozilla.org/r/218444/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/218444/

Comment 8

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1434374 - rewrite scripts

Review commit: https://reviewboard.mozilla.org/r/218446/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/218446/

Comment 9

[Julian Descottes [:jdescottes]]

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?

Comment 10

[Julian Descottes [:jdescottes]]

Comment on attachment 8949050 [details]
Bug 1434374 - Remove useless Cu imports.

https://reviewboard.mozilla.org/r/218444/#review224484

Comment 11

[Julian Descottes [:jdescottes]]

Comment on attachment 8949049 [details]
Bug 1434374 - Auto-rewrite Cu.import into ChromeUtils.import or require calls.

https://reviewboard.mozilla.org/r/218442/#review224480

Comment 12

[Julian Descottes [:jdescottes]]

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

Comment 13

[Julian Descottes [:jdescottes]]

Comment on attachment 8949047 [details]
Bug 1434374 - Always import deprecated-sync-promises as a module.

https://reviewboard.mozilla.org/r/218438/#review224460

Comment 14

[Julian Descottes [:jdescottes]]

Thanks for the cleanup! r+ if there is a green try.

Comment 15

[Alexandre Poirot [:ochameau]]

(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!

Comment 16

[Alexandre Poirot [:ochameau]]

Comment on attachment 8949047 [details]
Bug 1434374 - Always import deprecated-sync-promises as a module.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/218438/diff/1-2/

Comment 17

[Alexandre Poirot [:ochameau]]

Comment on attachment 8949048 [details]
Bug 1434374 - Replace usages of Cu.import in modules (where ChromeUtils isn't defined)

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/218440/diff/1-2/

Comment 18

[Alexandre Poirot [:ochameau]]

Comment on attachment 8949049 [details]
Bug 1434374 - Auto-rewrite Cu.import into ChromeUtils.import or require calls.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/218442/diff/1-2/

Comment 19

[Alexandre Poirot [:ochameau]]

Comment on attachment 8949050 [details]
Bug 1434374 - Remove useless Cu imports.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/218444/diff/1-2/

Comment 20

[Julian Descottes [:jdescottes]]

(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 :)

Comment 21

[Alexandre Poirot [:ochameau]]

Looks green enough:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=09ad26e49c4101ee3296bfb2a4a0e891d52ff9f3

Comment 22

[Alexandre Poirot [:ochameau]]

Comment on attachment 8949048 [details]
Bug 1434374 - Replace usages of Cu.import in modules (where ChromeUtils isn't defined)

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/218440/diff/2-3/

Comment 23

[Alexandre Poirot [:ochameau]]

Comment on attachment 8949049 [details]
Bug 1434374 - Auto-rewrite Cu.import into ChromeUtils.import or require calls.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/218442/diff/2-3/

Comment 24

[Alexandre Poirot [:ochameau]]

Comment on attachment 8949050 [details]
Bug 1434374 - Remove useless Cu imports.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/218444/diff/2-3/

Comment 25

[Pulsebot]

Pushed by [e-mail]:
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

Comment 26

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/00db0a388e72
https://hg.mozilla.org/mozilla-central/rev/1f077e990b48
https://hg.mozilla.org/mozilla-central/rev/bdcaee53dff4
https://hg.mozilla.org/mozilla-central/rev/7756d789a6e7