Open Bug 1562313 Opened 1 year ago Updated 4 months ago

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

Categories

(Thunderbird :: General, task)

task
Not set
normal

Tracking

(Not tracked)

ASSIGNED

People

(Reporter: mkmelin, Assigned: khushil324)

References

(Depends on 1 open bug)

Details

(Keywords: leave-open, Whiteboard: [two cases remaining in calendar])

Attachments

(6 files, 16 obsolete files)

43.75 KB, patch
khushil324
: review+
Details | Diff | Splinter Review
86.02 KB, patch
mkmelin
: review+
Details | Diff | Splinter Review
166.82 KB, patch
khushil324
: review+
Details | Diff | Splinter Review
35.18 KB, patch
Details | Diff | Splinter Review
125.77 KB, patch
khushil324
: review+
Details | Diff | Splinter Review
4.72 KB, patch
khushil324
: 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 :/

Keywords: leave-open

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.

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

The Z1 failure is from bug 1595747.

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
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.
Attachment #9113443 - Flags: review+

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

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!

Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/8d558f7fb2cc
Convert javascript components from chat to static registration. r=mkmelin DONTBUILD

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.

Attachment #9117277 - Flags: review?(paul)
Attachment #9117277 - Flags: feedback?(mkmelin+mozilla)

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

Attachment #9117278 - Flags: feedback?(philipp)
Attachment #9117278 - Flags: feedback?(geoff)
Attachment #9117278 - Attachment is patch: true
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.
Attachment #9117277 - Flags: review?(paul)

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

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 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.
Attachment #9117277 - Flags: feedback?(mkmelin+mozilla) → feedback+

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?

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.

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

Attachment #9117278 - Attachment is obsolete: true
Attachment #9117278 - Flags: feedback?(philipp)
Attachment #9117278 - Flags: feedback?(geoff)
Attachment #9120131 - Flags: feedback?(paul)
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.
Attachment #9120130 - Flags: review?(paul) → review+
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.
Attachment #9120131 - Flags: feedback?(paul) → feedback+

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

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

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.

Attachment #9120130 - Attachment is obsolete: true
Attachment #9128436 - Flags: review?(paul)

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?

Attachment #9120131 - Attachment is obsolete: true
Attachment #9128437 - Flags: feedback?(paul)
Attachment #9128437 - Flags: feedback?(mkmelin+mozilla)
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.
Attachment #9128437 - Flags: feedback?(mkmelin+mozilla)
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.
Attachment #9128436 - Flags: review?(paul) → review+
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.
Attachment #9128437 - Flags: feedback?(paul)
Attachment #9128436 - Attachment is obsolete: true
Attachment #9128726 - Attachment is obsolete: true
Attachment #9128727 - Flags: review+

Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/5958d77c395b
Convert most calendar JavaScript components (not calBackendLoader) to static registration. r=pmorris DONTBUILD

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

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.

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?

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

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

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

Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/c78b6b855a46
Convert JavaScript components to use static registration in ldap/. r=mkmelin DONTBUILD

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 978570 (and following removal of libical) those can be removed.

Depends on: icaljs
Whiteboard: [two cases remaining in calendar]
You need to log in before you can comment on or make changes to this bug.