1562313 - convert thunderbird javascript components over to use static registration (like bug 1524688)

Status: RESOLVED FIXED

(Thunderbird :: General, task)

Description

[Magnus Melin [:mkmelin]]

Bug 1524688 made it possible for javascript components to use static registration.
We should convert them over like mozilla-central did.

mail: https://searchfox.org/comm-central/search?q=XPCOMUtils.generateNSGetFactor&case=true&regexp=false&path=mail%2F

mailnews: https://searchfox.org/comm-central/search?q=XPCOMUtils.generateNSGetFactor&case=true&regexp=false&path=mailnews%2F

calendar: https://searchfox.org/comm-central/search?q=XPCOMUtils.generateNSGetFactor&case=true&regexp=false&path=calendar

chat: https://searchfox.org/comm-central/search?q=XPCOMUtils.generateNSGetFactor&case=true&regexp=false&path=chat

Comment 1

[Khushil Mistry [:khushil324]]

Attachment: Bug-1562313_part-1_convert-mail-to-static-registration.patch

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

Comment 2

[Magnus Melin [:mkmelin]]

Comment on attachment 9103194 [details] [diff] [review]
Bug-1562313_part-1_convert-mail-to-static-registration.patch

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

For the naming, please drop the ns prefixes when moving to real modules. Also uppercase the name of modules. so AboutRedirector.jsm not aboutRedirector.jsm and so on.

::: mail/components/aboutRedirector.jsm
@@ +81,4 @@
>    },
>  };
>  
> +var EXPORTED_SYMBOLS = ["AboutRedirector"];

I think it's a good convention to move this up to first thing after the license boilerplate.
(Here and elsewhere)

::: mail/components/moz.build
@@ +40,5 @@
> +    'aboutRedirector.jsm',
> +    'appIdleManager.js',
> +    'mailContentHandler.jsm',
> +    'mailGlue.jsm',
> +    'nsMailDefaultHandler.jsm',

Please rename it to MessengerContentHandler.jsm mirroring BrowserContentHandler.jsm for Firefox (but we didn't port everything in there yet, naming makes it easier to find what needs doing.)

::: mail/components/shell/moz.build
@@ +37,5 @@
>  
>  CXXFLAGS += CONFIG['TK_CFLAGS']
> +
> +EXTRA_JS_MODULES += [
> +    'nsSetDefaultMail.jsm',

We shouldn't add ns-* named modules.

The functionality should really be merged into MessengerContentHandler.jsm. 
(Do it here or file a followup.)

Comment 3

[Khushil Mistry [:khushil324]]

Attachment: Bug-1562313_part-1_convert-mail-to-static-registration.patch

Comment 4

[Khushil Mistry [:khushil324]]

(In reply to Magnus Melin [:mkmelin] from comment #2)

The functionality should really be merged into MessengerContentHandler.jsm.
(Do it here or file a followup.)

I will do it in the follow-up bug. I guess we can also include MailContentHandler.jsm in MessengerContentHandler.jsm.

Comment 5

[Magnus Melin [:mkmelin]]

Comment on attachment 9103235 [details] [diff] [review]
Bug-1562313_part-1_convert-mail-to-static-registration.patch

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

Looks good I think. 

To give more reference, as discussed outside of bugzilla: cmdarg.js was obsolete, and about:support couldn't be "re-registered" because it is already set up on the docshell nsAboutRedirector.cpp - so we just override the file instead.

Comment 6

[Khushil Mistry [:khushil324]]

I will submit 4 patches here, one for each folder.

Comment 7

[Magnus Melin [:mkmelin]]

Noticed there's a linting error in Activity.jsm and the commit message is not quite right

Comment 8

[Khushil Mistry [:khushil324]]

Attachment: Bug-1562313_part-1_convert-mail-to-static-registration.patch

Comment 9

[Khushil Mistry [:khushil324]]

Attachment: Bug-1562313_part-2_convert-chat-to-static-registration.patch

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

Comment 10

[Khushil Mistry [:khushil324]]

Comment on attachment 9103521 [details] [diff] [review]
Bug-1562313_part-2_convert-chat-to-static-registration.patch

Apply after the 1st patch.

Comment 11

[Jorg K (CEST = GMT+2)]

I don't see a try run for the first part, well, this early one has the Calendar tests pretty busted:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=59694e6e496f274908a94ec58ab5d5c22c5ba2eb

Here's a new one:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=287fd9c8b026825ddf76130f0c5d36544fa9475b

Comment 12

[Jorg K (CEST = GMT+2)]

Looks OK after rerunning Z4 twice :/

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/5624874a8354
Convert JavaScript components to use static registration in mail/. r=mkmelin

Comment 14

[Magnus Melin [:mkmelin]]

Comment on attachment 9103521 [details] [diff] [review]
Bug-1562313_part-2_convert-chat-to-static-registration.patch

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

::: chat/protocols/irc/test/test_ctcpQuote.js
@@ +2,5 @@
>   * http://creativecommons.org/publicdomain/zero/1.0/ */
>  
>  var { Services } = ChromeUtils.import("resource://gre/modules/Services.jsm");
>  var irc = {};
> +Services.scriptloader.loadSubScript("resource://gre/modules/IRC.jsm", irc);

shouldn't this be imported in the normal way?

::: chat/protocols/irc/test/test_ircCommands.js
@@ +6,2 @@
>  var irc = {};
> +Services.scriptloader.loadSubScript("resource://gre/modules/IRC.jsm", irc);

and this?

::: chat/protocols/irc/test/test_ircMessage.js
@@ +2,5 @@
>   * http://creativecommons.org/publicdomain/zero/1.0/ */
>  
>  var { Services } = ChromeUtils.import("resource://gre/modules/Services.jsm");
>  var irc = {};
> +Services.scriptloader.loadSubScript("resource://gre/modules/IRC.jsm", irc);

and this (and all the others similar)

Comment 15

[Khushil Mistry [:khushil324]]

Attachment: Bug-1562313_part-2_convert-chat-to-static-registration.patch

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

Comment 16

[Magnus Melin [:mkmelin]]

Comment on attachment 9104308 [details] [diff] [review]
Bug-1562313_part-2_convert-chat-to-static-registration.patch

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

::: chat/protocols/irc/IRC.jsm
@@ +9,5 @@
> +  "ircMessage",
> +  "clearTimeout",
> +  "setTimeout",
> +  "ircChannel",
> +  "GenericIRCConversation",

please make the irc named ones IRC, like IRCAccount. 
That way constructors look sane and not confused with variable names.

::: chat/protocols/irc/test/test_ircNonStandard.js
@@ +1,5 @@
>  /* Any copyright is dedicated to the Public Domain.
>   * http://creativecommons.org/publicdomain/zero/1.0/ */
>  
>  var { Services } = ChromeUtils.import("resource://gre/modules/Services.jsm");
> +var { ircMessage } = ChromeUtils.import("resource://gre/modules/IRC.jsm");

"resource://modules/IRC.jsm"
gre are for the m-c special modules

Comment 17

[Khushil Mistry [:khushil324]]

(In reply to Magnus Melin [:mkmelin] from comment #16)

"resource://modules/IRC.jsm"
gre are for the m-c special modules

So should we use js modules without gre elsewhere also or here only?

Comment 18

[Magnus Melin [:mkmelin]]

Applies to all. (None of our modules should use gre. It doesn't work either if you do use gre, at least not always.)

Comment 19

[Khushil Mistry [:khushil324]]

Attachment: Bug-1562313_part-2_convert-chat-to-static-registration.patch

We can import it like resource:///modules/filename.jsm. I guess resource:///modules/filename.jsm and resource://gre/modules/filename.jsm do the same thing. We are using both the convention at various places in TB. Should I file a follow-up bug to use the uniform convention in TB? Here, I have used resource:///modules/filename.jsm convention in the static registration part in Mail and Chat directory.
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=16c748f7b25edf5b41a1025db71352dd3bc7c8c0

Comment 20

[Magnus Melin [:mkmelin]]

Comment on attachment 9104422 [details] [diff] [review]
Bug-1562313_part-2_convert-chat-to-static-registration.patch

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

r=mkmelin, with a few smaller changes

::: chat/components/src/IMCommands.jsm
@@ +1,5 @@
>  /* This Source Code Form is subject to the terms of the Mozilla Public
>   * 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/. */
>  
> +var EXPORTED_SYMBOLS = ["CommandsService"];

The file service should probably be named something more appropriate, like IMCommands. But we can change that later

::: chat/components/src/IMConversations.jsm
@@ +1,5 @@
>  /* This Source Code Form is subject to the terms of the Mozilla Public
>   * 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/. */
>  
> +var EXPORTED_SYMBOLS = ["ConversationsService", "imMessage", "UIConversation"];

IMMessage (+ the change to make it so)

::: chat/components/src/IMCore.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/. */
>  
> +var EXPORTED_SYMBOLS = ["CoreService"];
> +

Should be named something more approipriate, like IMCore (we can do that too in a followup patch)

::: chat/components/src/test/test_logger.js
@@ +193,5 @@
>    let dummyRejectedOperation = () => Promise.reject("Rejected!");
>    let dummyResolvedOperation = () => Promise.resolve("Resolved!");
>  
> +  let gFP = gFilePromises;
> +  let qFO = queueFileOperation;

g is used for "global". 

In logger.js please change gFilePromises to fileOperations

... and in this file

Please just replace the 
- gFP occurrances with fileOperations
- qFO with queueFileOperation

Comment 21

[Khushil Mistry [:khushil324]]

Attachment: Bug-1562313_part-2_convert-chat-to-static-registration.patch

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

Comment 22

[Jorg K (CEST = GMT+2)]

Please check your own try runs. The X4 is known, but the X1 comes from here quite clearly:
TEST-UNEXPECTED-FAIL | comm/chat/components/src/test/test_logger.js | xpcshell return code: 0

Comment 23

[Jorg K (CEST = GMT+2)]

Comment on attachment 9104641 [details] [diff] [review]
Bug-1562313_part-2_convert-chat-to-static-registration.patch

Test failure.

Comment 24

[Khushil Mistry [:khushil324]]

Attachment: Bug-1562313_part-2_convert-chat-to-static-registration.patch

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

Comment 25

[Jorg K (CEST = GMT+2)]

Can you please supply a correct commit message. I didn't notice the hyphen, but the system did:
remote: ************************** ERROR ****************************
remote: Rev d9f7fa84e7f2 needs "Bug N" or "No bug" in the commit message.
remote: Khushil Mistry <[e-mail]>
remote: Bug-1562313 - Convert JavaScript components to use static registration in chat/. r=mkmelin
remote: *************************************************************

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/faa7e3cc7838
Convert JavaScript components to use static registration in chat/. r=mkmelin

Comment 27

[Patrick Cloke [:clokep]]

Please file bugs like this in the proper component so that interested parties are CCed automatically.

Comment 28

[Patrick Cloke [:clokep]]

Oops. I see that the same bug was used for multiple components. That's rather unfortunate. Please CC me on massive changes to chat.

Comment 29

[Patrick Cloke [:clokep]]

I backed out the chat changes (https://hg.mozilla.org/comm-central/rev/9ba26fa8d407d7466b79ab338b874e2460aa110e). There's a lot of changes in it that shouldn't be made, I'll do a full review shortly, but some of the things I noticed:

• Private variables are being publicly exported from modules.
• Many changes are made that are unrelated to this bug.
• Many files were renamed and I can't tell if that was done on purpose or not.
• Improper capitalization of variables.
• I suspect that ClassInfo needs to be updated for these changes.

Comment 30

[Khushil Mistry [:khushil324]]

Patrick can you review the patch for chat?

Comment 31

[Patrick Cloke [:clokep]]

(In reply to Khushil Mistry [:khushil324] from comment #30)

Patrick can you review the patch for chat?

Yes. The gist my complaint is that this bug should make the minimum necessary changes. E.g. file and variable renaming needs to happen in a separate bug. This should keep the diff much smaller (and make it much easier to review).

Comment 32

[Magnus Melin [:mkmelin]]

FWIW, I don't think that is for the most part doable. (Well you could do it, but you'd have to create hacks/wrongness from the start.) E.g. IMCore, the module file should preferably be uppercase, and it's now clearly a module, so should be .jsm not .js. Exporting CoreService, which is super generic sounding (when you're actually using it as module, as a js implemented XPCOM component ). So it wants to export something reasonable, like the patch changes it into IMCore.

That's just a small example: in summary, it doesn't make sense to try to split this up. It could actually be much harder to review in steps.

Comment 33

[Patrick Cloke [:clokep]]

(In reply to Magnus Melin [:mkmelin] from comment #32)

FWIW, I don't think that is for the most part doable. (Well you could do it, but you'd have to create hacks/wrongness from the start.) E.g. IMCore, the module file should preferably be uppercase, and it's now clearly a module, so should be .jsm not .js. Exporting CoreService, which is super generic sounding (when you're actually using it as module, as a js implemented XPCOM component ). So it wants to export something reasonable, like the patch changes it into IMCore.

1. I'm not implying we don't rename the files from .js to .jsm. I don't care about the extension.
2. You've chosen what might be the only example of one that makes sense to rename. I'm talking about the gratuitous renames, like all of the IRC and XMPP, code just to change capitalization.

Comment 34

[Magnus Melin [:mkmelin]]

The IRC wasn't a module but some strange middle-thing between a module and not.
Since it became a module, you really shouldn't have any lower case exports (esp. for constructors). That makes it very difficult to see what's a variable or a class.

Re XMPP, I'm confused. That's a tiny change. Do you suggest we rename the file in two commits to do two renames (and nothing else)? The protocol is also all caps...

Comment 35

[Patrick Cloke [:clokep]]

(In reply to Magnus Melin [:mkmelin] from comment #34)

The IRC wasn't a module but some strange middle-thing between a module and not.
Since it became a module, you really shouldn't have any lower case exports (esp. for constructors). That makes it very difficult to see what's a variable or a class.

I don't follow this, sorry. Currently all of that code uses a prefix of irc. If we want to change it we should do it separately. Whether it is exported via a module or not doesn't make it any harder to tell if it is a class or not.

I also don't understand what you mean by "some strange middle-thing between a module and not". It was not a module. Period. There's no question about that.

Comment 36

[Magnus Melin [:mkmelin]]

It wasn't a module, but it was kind of used as one. In any case, it's one after the patch so it should behave like one. Sure, the irc changes could be separate mostly, and if you want to keep the irc prefix, we can just export an IRC (uppercase) instead and that's doable. You'd still have ugly looking things like new IRC.ircAccount(). Even disregarding ugly or not, that's not a typical js way of doing things.

Comment 37

[Patrick Cloke [:clokep]]

Comment on attachment 9104840 [details] [diff] [review]
Bug-1562313_part-2_convert-chat-to-static-registration.patch

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

The mechanical changes look fine, but some overall comments:
* Please do not change the capitalization of the files (e.g. imCommands.js should become imCommands.jsm).
* Please do not change the names of various the services / classes (e.g. CommandsService should stay CommandsService). In the context of how these are used they make sense (see imServices.jsm).

I have a few more specific comments for particular files too.

Is there any clean-up to the `ClassInfo` implementation from imXPCOMUtils.jsm after these changes?

::: chat/components/src/IMConversations.jsm
@@ +27,5 @@
>    cancelled: false,
>    action: false,
>  };
>  
> +function IMMessage(aPrplMessage) {

This class is named imMessage to match the imIMessage interface. Please do not rename it.

::: chat/components/src/IMCore.jsm
@@ -272,5 @@
>      }
>    },
>  };
>  
> -var gCoreService;

Is this unused? Why do we even need to touch this code though? It seems unrelated to this bug.

::: chat/components/src/Logger.jsm
@@ +1,5 @@
>  /* This Source Code Form is subject to the terms of the Mozilla Public
>   * 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/. */
>  
> +var EXPORTED_SYMBOLS = [

Why are all of these being exported? Most of these are private implementation details of the Logger service.

@@ +42,5 @@
>   * Maps file paths to promises returned by ongoing OS.File operations on them.
>   * This is so that a file can be read after a pending write operation completes
>   * and vice versa (opening a file multiple times concurrently may fail on Windows).
>   */
> +var fileOperations = new Map();

Please do not change the name of this.

::: chat/protocols/irc/IRC.jsm
@@ +8,5 @@
> +  "IRCConversation",
> +  "IRCMessage",
> +  "IRCChannel",
> +  "clearTimeout",
> +  "setTimeout",

`clearTimeout` and `setTimeout` do not "belong" to this file (they're imported below) so they shouldn't be incldued in the exported symbols here.

Again, it is weird to me to export anything besides `ircProtocol` since the rest are internal to the protocols functionality.

::: chat/protocols/skype/Skype.jsm
@@ +1,5 @@
>  /* This Source Code Form is subject to the terms of the Mozilla Public
>   * 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/. */
>  
> +var EXPORTED_SYMBOLS = ["SkypeProtocol", "contactUrlToName", "magicSha256"];

Is there a real benefit to exporting these as opposed to using the `Services.scriptloader.loadSubScript` in tests? They're meant to be implementation details, not things that someone else should import and start using.

This questions also applies to e.g. the IRC code.

::: chat/protocols/xmpp/XMPPProtocol.jsm
@@ +1,5 @@
>  /* This Source Code Form is subject to the terms of the Mozilla Public
>   * 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/. */
>  
> +var EXPORTED_SYMBOLS = ["XMPPProtocol"];

It might make more sense to rename the current `xmpp.jsm` to something like `xmpp-base.jsm` and to name the implementation here `xmpp.jsm`. This would better match the other protocols we have.

Comment 38

[Khushil Mistry [:khushil324]]

Attachment: Bug-1562313_part-2_convert-chat-to-static-registration.patch

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

Comment 39

[Khushil Mistry [:khushil324]]

(In reply to Patrick Cloke [:clokep] from comment #37)

Is there any clean-up to the ClassInfo implementation from
imXPCOMUtils.jsm after these changes?

I will work on it in the follow up bug.

Comment 40

[Khushil Mistry [:khushil324]]

Attachment: Bug-1562313_part-3_convert-mailnews-to-static-registration.patch

Comment 41

[Khushil Mistry [:khushil324]]

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=599c051c8f53144a61e270791f3584e13d0b05a8&selectedJob=275758504

Comment 42

[Magnus Melin [:mkmelin]]

Comment on attachment 9107939 [details] [diff] [review]
Bug-1562313_part-3_convert-mailnews-to-static-registration.patch

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

::: mailnews/base/src/MailNewsCommandLineHandler.jsm
@@ +159,5 @@
>      Ci.nsIFactory,
>    ]),
>  };
>  
> +function MailnewsCommandLineHandlerModule() {

can't you have only the MailnewsCommandLineHandler?

::: mailnews/db/gloda/components/moz.build
@@ +3,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/.
>  
> +EXTRA_JS_MODULES += [
> +    'Glautocomp.jsm',

GlodaAutoComplete.jsm perhaps?

::: mailnews/extensions/newsblog/moz.build
@@ +4,5 @@
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  
> +EXTRA_JS_MODULES += [
> +    'content/FeedUtils.jsm',
> +    'content/Newsblog.jsm',

and perhaps NewsBlog.jsm

::: mailnews/jsaccount/moz.build
@@ +11,5 @@
>  
>  EXTRA_JS_MODULES.jsaccount += [
> +  'modules/JaBaseUrl.jsm',
> +  'modules/JSAccountUtils.jsm',
> +  'test/unit/resources/testJaMsgProtocolInfoComponent.jsm',

Capital T

Comment 43

[Khushil Mistry [:khushil324]]

Attachment: Bug-1562313_part-3_convert-mailnews-to-static-registration.patch

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

Comment 44

[Jorg K (CEST = GMT+2)]

The Z1 failure is from bug 1595747.

Comment 45

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/0f435d155919
Convert JavaScript components to use static registration in mailnews/. r=mkmelin

Comment 46

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/199eeaba5791
Convert JavaScript components to use static registration in mailnews. suite part. r=frg

Comment 47

[Patrick Cloke [:clokep]]

Comment on attachment 9107615 [details] [diff] [review]
Bug-1562313_part-2_convert-chat-to-static-registration.patch

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

Looks good! I think I might have misspoken on my last comment, I think that we should rename xmpp.jsm to xmpp-base.jsm and then xmpp.js to xmpp.jsm. This patch renames xmpp.js to xmpp-base.jsm instead, which I think is confusing since xmpp.jsm is the code used by all XMPP based protocols while xmpp.js is the generic XMPP protocol. What do you think?

If we want to rename less files (or don't want to rename xmpp.jsm) I think a name like xmpp-protocol.jsm might make sense as a new name for xmpp.js.

Comment 48

[Khushil Mistry [:khushil324]]

Attachment: Bug-1562313_part-2_convert-chat-to-static-registration.patch

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

Comment 49

[Khushil Mistry [:khushil324]]

(In reply to Patrick Cloke [:clokep] from comment #47)

Looks good! I think I might have misspoken on my last comment, I think that
we should rename xmpp.jsm to xmpp-base.jsm and then xmpp.js to xmpp.jsm.
This patch renames xmpp.js to xmpp-base.jsm instead, which I think is
confusing since xmpp.jsm is the code used by all XMPP based protocols while
xmpp.js is the generic XMPP protocol. What do you think?

There were few changes only so I did it this way.

Comment 50

[Patrick Cloke [:clokep]]

Note that splinter seems to be rendering the diff really weird. Looking at the diff locally I see:

• xmpp.jsm -> xmpp-base.jsm
• xmpp.js -> xmpp.jsm

I did a local build and didn't see any issues.

Thanks for fixing this!

Comment 51

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/8d558f7fb2cc
Convert javascript components from chat to static registration. r=mkmelin DONTBUILD

Comment 52

[Khushil Mistry [:khushil324]]

Attachment: Bug-1562313_part-4_convert-calendar-to-static-registration.patch

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

This patch contains all the components except calStartupService and calBackendLoader and related sub components. CalStartupService will change I guess when we do a deep integration and it also needs certain synchronous calls. For calBackendLoader, I will submit a WIP patch. Some unit tests are failing for that patch as some calBackendLoader components are not loading in time.

Comment 53

[Khushil Mistry [:khushil324]]

Attachment: WIP_Bug-1562313_part-4_convert-calendar-to-static-registration-calBackendLoader.patch

This is a WIP. The calendar is working fine with this but components are not loading at the right time for unit tests. Geoff or Philipp, if you can give some feedback here, it will be really helpful.

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

Comment 54

[Paul Morris [:pmorris]]

Comment on attachment 9117277 [details] [diff] [review]
Bug-1562313_part-4_convert-calendar-to-static-registration.patch

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

I've reviewed the changes and noted a few small things, but overall things look okay.  (Harder to review a large patch with naming changes mixed with other changes.)  But some errors in the console, so there's more to do before landing.  (This was with icaljs, not sure if it's different with libical.)  Here's what I see:

console.log: WebExtensions: Firing profile-after-change listeners for {e2fda1a4-762b-4020-b5ad-a41df1933103}
[calBackendLoader] Using Lightning's icaljs backend
JavaScript error: resource://calendar/calendar-js/calStartupService.js, line 57: TypeError: Cc['@mozilla.org/calendar/timezone-service;1'] is undefined
console.error: WebExtensions: 
  Error firing profile-after-change listener for
  @mozilla.org/calendar/startup-service;1
JavaScript error: resource://gre/modules/XPCOMUtils.jsm, line 215: TypeError: Cc[aContract] is undefined
JavaScript error: resource://gre/modules/XPCOMUtils.jsm, line 215: TypeError: Cc[aContract] is undefined
JavaScript error: resource://calendar/modules/utils/calDateTimeUtils.jsm, line 24: TypeError: cal.getTimezoneService(...) is undefined
JavaScript error: resource://calendar/modules/utils/calDateTimeUtils.jsm, line 38: TypeError: cal.getTimezoneService(...) is undefined
JavaScript error: resource://calendar/modules/utils/calDateTimeUtils.jsm, line 38: TypeError: cal.getTimezoneService(...) is undefined
JavaScript error: resource://calendar/modules/utils/calDateTimeUtils.jsm, line 24: TypeError: cal.getTimezoneService(...) is undefined
JavaScript error: resource://calendar/modules/utils/calDateTimeUtils.jsm, line 24: TypeError: cal.getTimezoneService(...) is undefined
JavaScript error: resource://calendar/modules/utils/calDateTimeUtils.jsm, line 24: TypeError: cal.getTimezoneService(...) is undefined
JavaScript error: resource://calendar/modules/utils/calDateTimeUtils.jsm, line 24: TypeError: cal.getTimezoneService(...) is undefined
JavaScript error: resource://calendar/modules/utils/calDateTimeUtils.jsm, line 24: TypeError: cal.getTimezoneService(...) is undefined
JavaScript error: resource://calendar/modules/utils/calDateTimeUtils.jsm, line 24: TypeError: cal.getTimezoneService(...) is undefined
JavaScript error: resource://calendar/modules/utils/calViewUtils.jsm, line 159: TypeError: Cc['@mozilla.org/calendar/calendar;1?type=composite'] is undefined
JavaScript error: resource://calendar/modules/utils/calViewUtils.jsm, line 159: TypeError: Cc['@mozilla.org/calendar/calendar;1?type=composite'] is undefined
JavaScript error: resource://calendar/modules/utils/calViewUtils.jsm, line 159: TypeError: Cc['@mozilla.org/calendar/calendar;1?type=composite'] is undefined
JavaScript warning: resource:///modules/matrix/bluebird.js, line 5479: unreachable code after return statement
JavaScript warning: resource:///modules/matrix/browser_request/index.js, line 428: unreachable code after return statement
JavaScript error: chrome://calendar/content/today-pane.js, line 95: TypeError: this.paneViews is null

::: calendar/base/src/calItemModule.js
@@ -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/. */
>  
>  /* import-globals-from calItemBase.js */
>  /* import-globals-from calCachedCalendar.js */

Seems like these two lines wouldn't be needed here anymore?

@@ +9,5 @@
>  var { XPCOMUtils } = ChromeUtils.import("resource://gre/modules/XPCOMUtils.jsm");
>  var { Services } = ChromeUtils.import("resource://gre/modules/Services.jsm");
>  
>  this.NSGetFactory = cid => {
> +  let scriptLoadOrder = ["resource://calendar/calendar-js/calStartupService.js"];

Since we now only have one component that's getting loaded here, we can simplify the code in this file further.  E.g. no need to do a for loop or to put the string in an array, just do Services.scriptloader... and pass in the one string.  

We might be able to go even further and drop this file/component entirely and replace the usages of calItemModule.js with calStartupService.js.  But lets do that in a separate follow-up patch (since this patch is already large, better to minimize the changes it makes to make it more easily understandable).

::: calendar/providers/storage/CalStorageCalendar.jsm
@@ +849,5 @@
>            flag = row.getResultByName("offline_journal") || null;
>          });
>        } catch (ex) {
>          aListener.onOperationComplete(
> +          self, // eslint-disable-line

Hm, seems like there should be a way to really fix this eslint issue?  The error is 'self is not defined', so seems like we should pass something that's defined to this function.

Comment 55

[Khushil Mistry [:khushil324]]

(In reply to Paul Morris [:pmorris] (away until Jan 6) from comment #54)

console.log: WebExtensions: Firing profile-after-change listeners for
{e2fda1a4-762b-4020-b5ad-a41df1933103}
[calBackendLoader] Using Lightning's icaljs backend
JavaScript error: resource://calendar/calendar-js/calStartupService.js, line
57: TypeError: Cc['@mozilla.org/calendar/timezone-service;1'] is undefined
console.error: WebExtensions:
Error firing profile-after-change listener for
@mozilla.org/calendar/startup-service;1
JavaScript error: resource://gre/modules/XPCOMUtils.jsm, line 215:
TypeError: Cc[aContract] is undefined
JavaScript error: resource://gre/modules/XPCOMUtils.jsm, line 215:
TypeError: Cc[aContract] is undefined
JavaScript error: resource://calendar/modules/utils/calDateTimeUtils.jsm,
line 24: TypeError: cal.getTimezoneService(...) is undefined
JavaScript error: resource://calendar/modules/utils/calDateTimeUtils.jsm,
line 38: TypeError: cal.getTimezoneService(...) is undefined
JavaScript error: resource://calendar/modules/utils/calDateTimeUtils.jsm,
line 38: TypeError: cal.getTimezoneService(...) is undefined
JavaScript error: resource://calendar/modules/utils/calDateTimeUtils.jsm,
line 24: TypeError: cal.getTimezoneService(...) is undefined
JavaScript error: resource://calendar/modules/utils/calDateTimeUtils.jsm,
line 24: TypeError: cal.getTimezoneService(...) is undefined
JavaScript error: resource://calendar/modules/utils/calDateTimeUtils.jsm,
line 24: TypeError: cal.getTimezoneService(...) is undefined
JavaScript error: resource://calendar/modules/utils/calDateTimeUtils.jsm,
line 24: TypeError: cal.getTimezoneService(...) is undefined
JavaScript error: resource://calendar/modules/utils/calDateTimeUtils.jsm,
line 24: TypeError: cal.getTimezoneService(...) is undefined
JavaScript error: resource://calendar/modules/utils/calDateTimeUtils.jsm,
line 24: TypeError: cal.getTimezoneService(...) is undefined
JavaScript error: resource://calendar/modules/utils/calViewUtils.jsm, line
159: TypeError: Cc['@mozilla.org/calendar/calendar;1?type=composite'] is
undefined
JavaScript error: resource://calendar/modules/utils/calViewUtils.jsm, line
159: TypeError: Cc['@mozilla.org/calendar/calendar;1?type=composite'] is
undefined
JavaScript error: resource://calendar/modules/utils/calViewUtils.jsm, line
159: TypeError: Cc['@mozilla.org/calendar/calendar;1?type=composite'] is
undefined
JavaScript warning: resource:///modules/matrix/bluebird.js, line 5479:
unreachable code after return statement
JavaScript warning: resource:///modules/matrix/browser_request/index.js,
line 428: unreachable code after return statement
JavaScript error: chrome://calendar/content/today-pane.js, line 95:
TypeError: this.paneViews is null

I am not able to see any of these issues. Can you tell me how to reproduce?Magnus can you also check and if you can find any other issues so that I can tackle them in one go?

Comment 56

[Geoff Lankow (:darktrojan)]

I have serious doubts that this is going to work properly while calendar is still an extension. The namespace resource://calendar doesn't exist until the extension is loaded, but it's wanted when component registration happens. (I'm not sure when that is, but I'd bet it's pretty early on.)

Plus AIUI you'll only be able to register libical or ical.js which means the user can't switch between them for testing or debugging purposes.

Comment 57

[Magnus Melin [:mkmelin]]

Comment on attachment 9117277 [details] [diff] [review]
Bug-1562313_part-4_convert-calendar-to-static-registration.patch

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

This part seems to work find for me. Paul, did you have both parts applied? Note the calBackendLoader patch is WIP and not expected to work. I think the idea was to try to get the working parts landed, and then the backend may have to wait before we can resolve i what to do with it.

Comment 58

[Khushil Mistry [:khushil324]]

On Mac, the WIP patch is also working fine, just unit test failures are there. So I am curious about the above errors.
Magnus, did you find any errors with WIP patch?
Any thoughts on Geoff's comment?

Comment 59

[Paul Morris [:pmorris]]

I tried applying both of the part4 patches and got the same errors in the console that I pasted in above (and calendar UI broken). I was seeing those errors with an artifact build, so I did a regular build (again with both part4 patches) and then I didn't get the errors. So the problem is apparently with artifact builds.

Comment 60

[Geoff Lankow (:darktrojan)]

Correct – static registration changes cannot be made on an artifact build.

Comment 61

[Khushil Mistry [:khushil324]]

Attachment: Bug-1562313_part-4_convert-calendar-to-static-registration-1.patch

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

Comment 62

[Khushil Mistry [:khushil324]]

Attachment: WIP_Bug-1562313_part-4_convert-calendar-to-static-registration-calBackendLoader-1.patch

Comment 63

[Paul Morris [:pmorris]]

Comment on attachment 9120130 [details] [diff] [review]
Bug-1562313_part-4_convert-calendar-to-static-registration-1.patch

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

I checked the changes against the previous version of the patch.  Bugzilla's diff wasn't working so I downloaded and diffed the two patches.  The changes look fine, so I think this could land, but...

Given comment 56 and 60, we know that while calendar is still an add-on these changes will at least break artifact builds (which I and others use for local development) if not other things. So I think we should wait until bug 1608610 lands before making these changes.  Then we avoid any potential issues.

Comment 64

[Paul Morris [:pmorris]]

Comment on attachment 9120131 [details] [diff] [review]
WIP_Bug-1562313_part-4_convert-calendar-to-static-registration-calBackendLoader-1.patch

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

I gave this a quick look through and the changes seem fine.  That said, is this the direction we want to go in?  As Geoff mentioned in comment 56: "AIUI you'll only be able to register libical or ical.js which means the user can't switch between them for testing or debugging purposes."  

AIUI the plan is to eventually deprecate libical in favor of icaljs, but since we're not there yet, I'd say it's worth keeping the dynamic loading to allow switching between the two.

Comment 65

[Worcester12345]

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

Comment on attachment 9120130 [details] [diff] [review]
Bug-1562313_part-4_convert-calendar-to-static-registration-1.patch

Review of attachment 9120130 [details] [diff] [review]:

Given comment 56 and 60, we know that while calendar is still an add-on
these changes will at least break artifact builds (which I and others use
for local development) if not other things.

Slightly important in the overall scheme of things, but worth noting.

So I think we should wait until

bug 1608610 lands before making these changes. Then we avoid any potential
issues.

Is there a way to "temporarily" land this bug, so it might help bug 1608610 to see what might break? Then "unland" it again once tested?

Comment 66

[Geoff Lankow (:darktrojan)]

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

Given comment 56 and 60, we know that while calendar is still an add-on
these changes will at least break artifact builds (which I and others use
for local development) if not other things.

That's not quite what I said. You can't make changes to static registration and expect an artifact build to pick them up. Once the changes have landed and built, the artifact build will be fine again.

IOW, static registration components.conf files are part of the build process (just like a .h file is) and a full build needs to happen to pick up any changes.

Comment 67

[Khushil Mistry [:khushil324]]

Attachment: Bug-1562313_part-4_convert-calendar-to-static-registratio-2.patch

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

As discussed with you, Paul, I have made two separate patches: calBackendLoader and one for all other components. This patch is passing all the tests. calBackendLoader patch is causing some test failures.

Comment 68

[Khushil Mistry [:khushil324]]

Attachment: Bug-1562313_part-4_convert-calendar-to-static-registration-calBackendLoader-0.patch

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

Here, libical unit tests are failing.
In the calBackendLoader, we will load components only if Services.prefs.getBoolPref("calendar.icaljs", false) is true which is to indicate if the build if Nightly or not. We are setting "calendar.icaljs" to false for libical unit tests: https://searchfox.org/comm-central/source/calendar/test/unit/head_libical.js#6. So calBackendLoader components should not load. How can we implement that? What kind of flags should we use in moz.build file? I have added if CONFIG['NIGHTLY_BUILD']: in moz.build file. But for manual switching, what can we do?

Comment 69

[Magnus Melin [:mkmelin]]

Comment on attachment 9128437 [details] [diff] [review]
Bug-1562313_part-4_convert-calendar-to-static-registration-calBackendLoader-0.patch

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

I don't think you should do that calendar.icaljs implicit check at build time.

You probably want to have (similar to before) a component that checks which of the components it should load based on the calendar.icaljs pref (and I guess unload too).
If there are major problems with the ical.js vs libical loading we could also leave that part to use dynamic registration for now.

Comment 70

[Paul Morris [:pmorris]]

Comment on attachment 9128436 [details] [diff] [review]
Bug-1562313_part-4_convert-calendar-to-static-registratio-2.patch

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

Nice to have these updated!  Thanks for pausing this until the calendar integration work landed.

Just a couple of minor suggestions, and I'd revise the commit message to "Convert most JavaScript components..." since you have split off the backend loader components to a separate patch.  I have not built and tested locally since the try run is successful.

::: calendar/base/src/CalRelation.jsm
@@ -6,5 @@
>  
> -/**
> - * calRelation prototype definition
> - *
> - * @implements calIRelation

I would keep this @implements comment.  It's helpful to know what interface is being implemented.

::: calendar/lightning/components/LightningTextCalendarConverter.jsm
@@ +7,5 @@
>  var { Services } = ChromeUtils.import("resource://gre/modules/Services.jsm");
>  var { cal } = ChromeUtils.import("resource:///modules/calendar/calUtils.jsm");
>  var { ltn } = ChromeUtils.import("resource:///modules/calendar/ltnInvitationUtils.jsm");
>  
> +function LtnMimeConverter() {

I think we should rename this to CalMimeConverter and the file to match, while we are renaming them.

Comment 71

[Paul Morris [:pmorris]]

Comment on attachment 9128437 [details] [diff] [review]
Bug-1562313_part-4_convert-calendar-to-static-registration-calBackendLoader-0.patch

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

I'll hold off on reviewing, in light of mkmelin's comment (which sounds like the right approach).  Thanks for splitting this off to handle separately.  It will be easier to manage this way.

Comment 72

[Khushil Mistry [:khushil324]]

Attachment: Bug-1562313_part-4_convert-calendar-to-static-registration.patch

Comment 73

[Khushil Mistry [:khushil324]]

Attachment: Bug-1562313_part-4_convert-calendar-to-static-registration-3.patch

Comment 74

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/5958d77c395b
Convert most calendar JavaScript components (not calBackendLoader) to static registration. r=pmorris DONTBUILD

Comment 75

[Khushil Mistry [:khushil324]]

(In reply to Magnus Melin [:mkmelin] from comment #69)

I don't think you should do that calendar.icaljs implicit check at build
time.

You probably want to have (similar to before) a component that checks which
of the components it should load based on the calendar.icaljs pref (and I
guess unload too).

But will it not be the dynamic registration?

Comment 76

[Magnus Melin [:mkmelin]]

It would. While it would be preferable to get it using static registration, it's not too bad if we have to leave one component with dynamic reg for now.

Comment 77

[Khushil Mistry [:khushil324]]

Fine, let's leave them right now.
We have a few left: https://searchfox.org/comm-central/source/ldap/xpcom/src/nsLDAPProtocolHandler.js#59
https://searchfox.org/comm-central/search?q=XPCOMUtils.generateNSGetFactor&case=true&regexp=false&path=mailnews%2F

I guess these are added after the patches are landed. Should I convert them in one patch or you want to leave them right now?

Comment 78

[Magnus Melin [:mkmelin]]

Please take care of them if there's no reason not to.

Comment 79

[Khushil Mistry [:khushil324]]

Attachment: Bug-1562313_part-5_convert-ldap-to-static-registration-0.patch

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

Comment 80

[Khushil Mistry [:khushil324]]

Magnus, I am not able to find your email for raising the flag of review. Please, review the above patch.

Comment 81

[Khushil Mistry [:khushil324]]

Attachment: Bug-1562313_part-5_convert-ldap-to-static-registration-1.patch

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

Comment 82

[Khushil Mistry [:khushil324]]

I got the review from Magnus on Matrix and I have updated the suggested changes.

Comment 83

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/c78b6b855a46
Convert JavaScript components to use static registration in ldap/. r=mkmelin DONTBUILD

Comment 84

[Magnus Melin [:mkmelin]]

To status is that we're left with two components in calendar: https://searchfox.org/comm-central/search?q=XPCOMUtils.generateNSGetFactor&case=true&regexp=false&path=calendar

After bug 978570 (and following removal of libical) those can be removed.

Comment 85

[Kris Maglione [:kmag]]

Note that support for the old system is being removed soon in bug 1770237, so any remaining entries will need to be migrated soon.

Comment 86

[Magnus Melin [:mkmelin]]

Thanks for the heads up!

From a brief look there are a few cases that need fixing:

• mail: https://searchfox.org/comm-central/search?q=contract+%40mozilla.org&path=mail&case=false&regexp=false
• calendar: https://searchfox.org/comm-central/search?q=contract+%40mozilla.org&path=cale&case=false&regexp=false

Comment 87

[Magnus Melin [:mkmelin]]

Actually, more: https://searchfox.org/comm-central/search?q=ComponentUtils.generateNSGetFactory&path=

Comment 88

[Magnus Melin [:mkmelin]]

Attachment: Bug 1562313 - convert StartupRecorder to static component registration. r=freaktechnik

Port of https://phabricator.services.mozilla.com/D148183

Also fix some other small differences we had to the browser equivalent file.

Comment 89

[Magnus Melin [:mkmelin]]

Attachment: WIP: Bug 1562313 - convert testJaFooUrlComponent.js, testJaBaseMsgFolderComponent.js, testJaBaseIncomingServerComponent.js to static registration.

Doesn't work. And not sure any of this makes sense either...

Comment 90

[Magnus Melin [:mkmelin]]

:kmag - do you have advice on what to do for the cases we have where we (per pref) load one or the other implementation? We use this in some cases where one component is being rewritten in JavaScript. Like https://searchfox.org/comm-central/source/mailnews/local/src/Pop3ModuleLoader.jsm
Will this continue to work?

Comment 91

[Kris Maglione [:kmag]]

(In reply to Magnus Melin [:mkmelin] from comment #90)

:kmag - do you have advice on what to do for the cases we have where we (per pref) load one or the other implementation? We use this in some cases where one component is being rewritten in JavaScript. Like https://searchfox.org/comm-central/source/mailnews/local/src/Pop3ModuleLoader.jsm
Will this continue to work?

Yes, that should continue to work.

Comment 92

[Magnus Melin [:mkmelin]]

Attachment: Bug 1562313 - convert calBackendLoader.js to static registration. r=darktrojan

Comment 93

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/142345e84687
convert calBackendLoader.js to static registration. r=darktrojan

Comment 94

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/4e67004743c9
convert StartupRecorder to static component registration. r=freaktechnik
https://hg.mozilla.org/comm-central/rev/0a03775ec04e
adjust package-manifest.in for bug 1770237. rs=bustage-fix
https://hg.mozilla.org/comm-central/rev/a05689892de1
disable now non-working jsaccount xpcshell tests. rs=bustage-fix

Comment 95

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/285b927fef2a
follow-up to 142345e84687 - add (back) category to CalBackendLoader registration. rs=me

Comment 96

[Geoff Lankow (:darktrojan)]

I think that we are done here. The disabled JS account tests are being dealt with in bug 1773772.

Comment 97

[Magnus Melin [:mkmelin]]

Wohoo! \o/