1529583 - rename Thunderbird related modules that are named .js to .jsm

Status: RESOLVED FIXED

(Thunderbird :: General, enhancement)

Description

[Magnus Melin [:mkmelin]]

We have a lot of modules that are named .js instead. They should be renamed to .jsm instead so it's clear what they are. Will also make it easier to see what needs to be done for bugs like bug 1432901 once that comes around.

Comment 1

[Magnus Melin [:mkmelin]]

Attachment: bug1529583_modules_to_jsm.patch

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=9a12a66187d759dc0d3bb33a0e436c9ed94086f6

Comment 3

[Ian Neal]

I'm sure I saw a .jsmm slip in there :S

Comment 4

[Magnus Melin [:mkmelin]]

Going to pick this up again now. There is now bug 1609269 for mozilla-central.

In the same rename, I'd like to handle some other bad naming, in particular make sure to remove snake namings and properly capitalize the filename like basically all other modules in m-c.

There are some particular cases that needs some though: Philipp, what about ical.js which is kind of the name of the library. Would it be better to name it Icaljs.jsm, Ical.jsm or what would be the preferred naming?

Comment 5

[Magnus Melin [:mkmelin]]

Attachment: bug1529583_js_jsm_rename_2020.folded.patch

I ended up doing this in a few steps. I would have done it in one if I had thought about everything beforehand.

In this successful try the steps are separate, but I've now folded it into one (and fixed a minor naming issue with the RNP jsm). https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=cc740ccf785129b285881289d6eeab514caf15f1

Need to get this work done now so the bug 1308512 blockers can be done for Thunderbird, because otherwise there's going to be back-tracking where one can't see if all the work was done or not.

Comment 6

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 9121726 [details] [diff] [review]
bug1529583_js_jsm_rename_2020.folded.patch

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

calendar looks good. there are a few places where "ical.js" references the library, not the file in calendar.

::: calendar/base/modules/Ical.jsm
@@ +2,5 @@
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  /**
> + * This is Ical.jsm from <https://github.com/mozilla-comm/Ical.jsm>.

This file should likely stay as is.

::: calendar/base/modules/utils/calItipUtils.jsm
@@ +880,2 @@
>              // at all. Single values are handled properly by both backends though.
> +            // Once bug 1206502 lands, Ical.jsm will handle multi-value params, but

Same

::: calendar/base/public/calIICSService.idl
@@ +33,5 @@
>       */
>      readonly attribute calIIcalComponent parent;
>  
>      /**
> +     * Access to the inner Ical.jsm objects. Only use these if you know what you

Same

@@ +158,5 @@
>       */
>      readonly attribute AUTF8String icalString;
>  
>      /**
> +     * Access to the inner Ical.jsm objects. Only use these if you know what you

Same

Comment 7

[Paul Morris [:pmorris]]

Comment on attachment 9121726 [details] [diff] [review]
bug1529583_js_jsm_rename_2020.folded.patch

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

Looks good overall.  Glad we're moving in the direction of standard JS module imports.  I have a few nits, suggestions, comments to address.  I haven't applied and manually tested the patch, since it has a successful try run.

::: mail/extensions/openpgp/jar.mn
@@ +3,5 @@
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  
>  openpgp.jar:
>  % content openpgp %content/openpgp/
> +  content/openpgp/BondOpenPGP.jsm                      (content/BondOpenPGP.jsm)

Indentation nit.

::: mailnews/base/util/JXON.jsm
@@ -193,5 @@
> -        oParentEl.appendChild(oChild);
> -      }
> -    }
> -  }
> -

Huh, not sure what this is for, but I guess it was unused.

::: mailnews/db/gloda/modules/Everybody.jsm
@@ +20,2 @@
>  const { GlodaABAttrs } = ChromeUtils.import(
> +  "resource:///modules/gloda/GlodaMsgIndexer.jsm"

Hm, some of these gloda jsms have "Gloda" at the start of their names and others don't.  Seems like either all of them should have it or none should, for consistency, while we're renaming them.

::: mailnews/db/gloda/modules/Gloda.jsm
@@ -27,3 @@
>  );
>  const { whittlerRegistry, mimeMsgToContentAndMeta } = ChromeUtils.import(
> -  "resource:///modules/gloda/connotent.js"

Oof, "connotent", glad we're fixing that!

::: mailnews/db/gloda/modules/SuffixTree.jsm
@@ -400,5 @@
> -  ];
> -  let b = new MultiSuffixTree(names, names);
> -  b.dump();
> -  dump(b.findMatches("rya") + "\n");
> -}

Huh, seems like this should become an XPCShell test?

::: mailnews/test/fakeserver/Imapd.jsm
@@ -513,5 @@
>  
>  // IMAP Namespaces
>  var IMAP_NAMESPACE_PERSONAL = 0;
> -var IMAP_NAMESPACE_OTHER_USERS = 1;
> -var IMAP_NAMESPACE_SHARED = 2;

Even if these values and the one above are currently unused, I'd rather see them commented out than deleted, otherwise we lose information about how the code is supposed to work, and that there are these other possibilities or cases to be aware of.

Comment 8

[Magnus Melin [:mkmelin]]

(In reply to Paul Morris [:pmorris] from comment #7)

Huh, not sure what this is for, but I guess it was unused.

Yes this is something that wasn't used by Thunderbird. The module has its roots externally.

Hm, some of these gloda jsms have "Gloda" at the start of their names and
others don't. Seems like either all of them should have it or none should,
for consistency, while we're renaming them.

The gloda module situation is quite a mess unfortunately, since modules wasn't much of a thing back in 2010. What I did is, I went through the list and checked if they exported something that would be likely to have naming collisions (globally) or otherwise be very gloda specific. Some of the modules are such that they, in theory, could be used elsewhere too. Some even are, like MimeMessage.jsm (mimemsg.js). Some just need rework/refactoring, like Everybody.jsm (everybody.js) but since this patch is so large I don't want to do things like that here.

Huh, seems like this should become an XPCShell test?

Turns out SuffixTree.jsm is only used by GlodaAutoComplete.jsm (what used to be glautocomp.js), but that appears to be dead code :/
(Will handle that separately.)

Comment 9

[Magnus Melin [:mkmelin]]

Attachment: bug1529583_js_jsm_rename_2020.folded.patch

Sent off to try now. https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=4013e0c4edd81876866d522a5b9b2e6b3cfa489a

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/9dde84974661
rename all Thunderbird related modules to .jsm, capitalize names and fix some namings. r=pmorris,Fallen DONTBUILD