Closed Bug 1485451 Opened Last year Closed Last year

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

Categories

(Thunderbird :: General, enhancement)

enhancement
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 63.0

People

(Reporter: jorgk-bmo, Assigned: jorgk-bmo)

Details

Attachments

(1 file, 3 obsolete files)

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.
Attached patch 1485451-cu-import.patch (obsolete) — Splinter Review
One call had no second argument, I wonder whether 'null' could just be left off altogether.
Attachment #9003234 - Flags: review?(geoff)
Attachment #9003234 - Attachment is obsolete: true
Attachment #9003234 - Flags: review?(geoff)
Attachment #9003236 - Flags: review?(geoff)
(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).
OK, I didn't remove it, in fact, I added one in Windows8WindowFrameColor.jsm, or should I take that one out again?
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.
Attachment #9003236 - Flags: review?(geoff) → review-
(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.
Attached patch 1485451-cu-import.patch (v2) (obsolete) — Splinter Review
Attachment #9003236 - Attachment is obsolete: true
Attachment #9003316 - Flags: review?(geoff)
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?)
Attachment #9003316 - Flags: review?(geoff) → review+
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.
Attachment #9003316 - Attachment is obsolete: true
Attachment #9003375 - Flags: review?(geoff)
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 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.
Attachment #9003375 - Flags: review?(geoff) → review+
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/5b963c1c18ee
Remove throw-away object in ChromeUtils.import() call. r=darktrojan
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 63.0
You need to log in before you can comment on or make changes to this bug.