Open Bug 1562313 Opened 5 months ago Updated 4 days ago

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

Categories

(Thunderbird :: General, task)

task
Not set

Tracking

(Not tracked)

ASSIGNED

People

(Reporter: mkmelin, Assigned: khushil324)

References

Details

(Keywords: leave-open)

Attachments

(3 files, 8 obsolete files)

43.75 KB, patch
khushil324
: review+
Details | Diff | Splinter Review
59.19 KB, patch
khushil324
: review?
clokep
Details | Diff | Splinter Review
86.02 KB, patch
mkmelin
: review+
Details | Diff | Splinter Review
Assignee: nobody → khushil324
Status: NEW → ASSIGNED
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.)
Attachment #9103194 - Flags: review?(mkmelin+mozilla) → feedback+
Attachment #9103194 - Attachment is obsolete: true
Attachment #9103235 - Flags: review?(mkmelin+mozilla)

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

See Also: → 1590456
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.
Attachment #9103235 - Flags: review?(mkmelin+mozilla) → review+
Keywords: checkin-needed

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

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

Keywords: checkin-needed
Keywords: checkin-needed
Attachment #9103521 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9103521 [details] [diff] [review]
Bug-1562313_part-2_convert-chat-to-static-registration.patch

Apply after the 1st patch.

Looks OK after rerunning Z4 twice :/

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

Keywords: checkin-needed
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)
Attachment #9104308 - Flags: review?(mkmelin+mozilla)
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
Attachment #9104308 - Flags: review?(mkmelin+mozilla)

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

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

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

Attachment #9104308 - Attachment is obsolete: true
Attachment #9104422 - Flags: review?(mkmelin+mozilla)
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
Attachment #9104422 - Flags: review?(mkmelin+mozilla) → review+

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 on attachment 9104641 [details] [diff] [review]
Bug-1562313_part-2_convert-chat-to-static-registration.patch

Test failure.
Attachment #9104641 - Flags: review+

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 <khushil324@gmail.com>
remote: Bug-1562313 - Convert JavaScript components to use static registration in chat/. r=mkmelin
remote: *************************************************************

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/faa7e3cc7838
Convert JavaScript components to use static registration in chat/. r=mkmelin

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

Product: Thunderbird → Chat Core
Version: Trunk → trunk

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

Product: Chat Core → Thunderbird
Version: trunk → Trunk

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.

Patrick can you review the patch for chat?

Flags: needinfo?(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).

Flags: needinfo?(clokep)

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.

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

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

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

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.

Attachment #9104840 - Flags: review+ → review?(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.
Attachment #9104840 - Flags: review?(clokep) → review-

(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 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
Attachment #9108219 - Flags: review?(mkmelin+mozilla) → review+

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/0f435d155919
Convert JavaScript components to use static registration in mailnews/. r=mkmelin

Pushed by frgrahl@gmx.net:
https://hg.mozilla.org/comm-central/rev/199eeaba5791
Convert JavaScript components to use static registration in mailnews. suite part. r=frg
You need to log in before you can comment on or make changes to this bug.