1485451 - Remove throw-away object from calls like |let {Foo} = ChromeUtils.import("foo.jsm", {})|

Status: RESOLVED FIXED

(Thunderbird :: General, enhancement)

Description

[Jorg K (CEST = GMT+2)]

As per bug 1480393 comment #14 this
  let {Foo} = ChromeUtils.import("foo.jsm", {});
should be
  let {Foo} = ChromeUtils.import("foo.jsm", null);

We've only recently started using this construct, so there is only little clean-up to do. Patch coming.

Comment 1

[Jorg K (CEST = GMT+2)]

Attachment: 1485451-cu-import.patch

One call had no second argument, I wonder whether 'null' could just be left off altogether.

Comment 2

[Jorg K (CEST = GMT+2)]

Attachment: 1485451-cu-import.patch - one more

Comment 3

[Jorg K (CEST = GMT+2)]

The second argument is optional, no?
https://searchfox.org/mozilla-central/rev/3fa761ade83ed0b8ab463acb057c2cf0b104689e/dom/chrome-webidl/ChromeUtils.webidl#296

Comment 4

[Geoff Lankow (:darktrojan)]

(In reply to Jorg K (GMT+2) from comment #3)
> The second argument is optional, no?

No. Well it is, but the behaviour is different. If the second argument is missing, the exported symbols are added to the global scope, which we do not want (especially when we've been explicit about using this form in the past).

Comment 5

[Jorg K (CEST = GMT+2)]

OK, I didn't remove it, in fact, I added one in Windows8WindowFrameColor.jsm, or should I take that one out again?

Comment 6

[Geoff Lankow (:darktrojan)]

Comment on attachment 9003236 [details] [diff] [review]
1485451-cu-import.patch - one more

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

Please use this form:

> let { Foo } = ChromeUtils.import("resource://gre/modules/Foo.jsm", null);

rather than:

> let Foo = ChromeUtils.import("resource://gre/modules/Foo.jsm", null).Foo;

It's much tidier. Also consider switching to const instead of var or let. We shouldn't be redefining these things if we've imported them.

Comment 7

[Geoff Lankow (:darktrojan)]

(In reply to Jorg K (GMT+2) from comment #5)
> OK, I didn't remove it, in fact, I added one in
> Windows8WindowFrameColor.jsm, or should I take that one out again?

No keep it, just check what's exported from that JSM and see if it's used elsewhere. I suspect it's the only thing exported and they've done it like that to rename it.

Comment 8

[Jorg K (CEST = GMT+2)]

Attachment: 1485451-cu-import.patch (v2)

Comment 9

[Geoff Lankow (:darktrojan)]

Comment on attachment 9003316 [details] [diff] [review]
1485451-cu-import.patch (v2)

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

Okay, with that one change.

::: mail/base/modules/Windows8WindowFrameColor.jsm
@@ +7,5 @@
>  this.EXPORTED_SYMBOLS = ["Windows8WindowFrameColor"];
>  
>  ChromeUtils.import("resource://gre/modules/Services.jsm");
>  ChromeUtils.import("resource://gre/modules/XPCOMUtils.jsm");
> +const { Registry } = ChromeUtils.import("resource://gre/modules/WindowsRegistry.jsm", null);

The exported object is WindowsRegistry, not Registry. You want const { WindowsRegistry: Registry } = …

(Or just rename the uses of Registry in that file. Really, who wrote it like that?)

Comment 10

[Jorg K (CEST = GMT+2)]

Attachment: 1485451-cu-import.patch (v3)

Here's another round. You really made me wonder now whether it should/can be const, so I'll comment on it in the next comment.

Comment 11

[Jorg K (CEST = GMT+2)]

Comment on attachment 9003375 [details] [diff] [review]
1485451-cu-import.patch (v3)

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

::: chat/content/convbrowser.xml
@@ +120,5 @@
>            if (!this._finder) {
>              if (!this.docShell)
>                return null;
>  
> +            const { Finder } = ChromeUtils.import("resource://gre/modules/Finder.jsm", null);

Looks like we're not changing this object, so const is fine.

::: mail/base/content/mailWindowOverlay.js
@@ +15,5 @@
>  ChromeUtils.defineModuleGetter(this, "BrowserToolboxProcess", "resource://devtools/client/framework/ToolboxProcess.jsm");
>  ChromeUtils.defineModuleGetter(this, "ScratchpadManager","resource://devtools/client/scratchpad/scratchpad-manager.jsm");
>  Object.defineProperty(this, "HUDService", {
>    get: function HUDService_getter() {
> +    let { devtools } = ChromeUtils.import("resource://devtools/shared/Loader.jsm", null);

Should we switch to const here?

::: mail/base/modules/Windows8WindowFrameColor.jsm
@@ +7,5 @@
>  this.EXPORTED_SYMBOLS = ["Windows8WindowFrameColor"];
>  
>  ChromeUtils.import("resource://gre/modules/Services.jsm");
>  ChromeUtils.import("resource://gre/modules/XPCOMUtils.jsm");
> +const { WindowsRegistry } = ChromeUtils.import("resource://gre/modules/WindowsRegistry.jsm", null);

Looks like we don't touch the object, so const is fine here.

::: mail/components/devtools/devtools-loader.js
@@ +33,5 @@
>      }
>    },
>  
>    initialize() {
> +    var { devtools, require, DevToolsLoader } = ChromeUtils.import("resource://devtools/shared/Loader.jsm", null);

Switch to const?

Comment 12

[Geoff Lankow (:darktrojan)]

Comment on attachment 9003375 [details] [diff] [review]
1485451-cu-import.patch (v3)

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

Looks good. I wouldn't waste any more time on var/let/const choices, it doesn't make much difference in most situations.

::: mail/base/content/mailWindowOverlay.js
@@ +15,5 @@
>  ChromeUtils.defineModuleGetter(this, "BrowserToolboxProcess", "resource://devtools/client/framework/ToolboxProcess.jsm");
>  ChromeUtils.defineModuleGetter(this, "ScratchpadManager","resource://devtools/client/scratchpad/scratchpad-manager.jsm");
>  Object.defineProperty(this, "HUDService", {
>    get: function HUDService_getter() {
> +    let { devtools } = ChromeUtils.import("resource://devtools/shared/Loader.jsm", null);

I have been using const in this situation, but it doesn't really matter.

::: mail/components/devtools/devtools-loader.js
@@ +33,5 @@
>      }
>    },
>  
>    initialize() {
> +    var { devtools, require, DevToolsLoader } = ChromeUtils.import("resource://devtools/shared/Loader.jsm", null);

Whoever wrote this was using var below, so I'd just stick with that for the sake of prettiness.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/5b963c1c18ee
Remove throw-away object in ChromeUtils.import() call. r=darktrojan