576746 - Make calendar XPCOM components use new manifests and data tables

Status: RESOLVED FIXED

(Calendar :: Internal Components, defect)

Description

[Philipp Kewisch [:Fallen][📱📆]]

Attachment: [checked in] Fix C++ Components - v1

This bug is the calendar counterpart of bug 575740.

We need to change our xpcom registration a bit so it works with the new data driven compreg. I'll be uploading multiple patches, so don't be concerned if there's something missing.

One problem I found is that it seems that for extensions, all manifest data should go into chrome.manifest in the extension root dir. It also seems that this data is not auto-generated, as it is for applications. I'll need to talk someone from #developers to either shed light onto that, or fix it on the build side.

Anyway, this patch is the first step, fixing our C++ components.

Comment 1

[Philip Chee]

> One problem I found is that it seems that for extensions, all manifest data
> should go into chrome.manifest in the extension root dir. It also seems that
> this data is not auto-generated, as it is for applications. I'll need to talk
> someone from #developers to either shed light onto that, or fix it on the build
> side.

See Weave Bug 576554 (Need manifests to register XPCOM components on trunk)

Comment 2

[Philipp Kewisch [:Fallen][📱📆]]

weave generates its chrome.manifest by preprocessing it directly, we generate ours using the jar.mn files. Therefore we need a different solution.

I've filed bug 576767 to fix this on the build side. If all fails, I guess we could cheat and add the entries to lightning's jar.mn.

Comment 3

[Martin Schröder [:mschroeder]]

Comment on attachment 455842 [details] [diff] [review]
[checked in] Fix C++ Components - v1

Successfully did a shared build on my Mac, also the code looks good and in line with Firefox and Thunderbird. r=mschroeder, hoping we will make more progress after this first patch.

Comment 4

[Martin Schröder [:mschroeder]]

Comment on attachment 455842 [details] [diff] [review]
[checked in] Fix C++ Components - v1

>diff --git a/calendar/base/build/calBaseModule.cpp b/calendar/base/build/calBaseModule.cpp
>--- a/calendar/base/build/calBaseModule.cpp
>+++ b/calendar/base/build/calBaseModule.cpp
[...]
>+NSMODULE_DEFN(calbase) = &kCalBaseModule;

For a static build it may be mandatory to replace 'calbase' with 'calBaseModule'!

Comment 5

[Ian Neal]

(In reply to comment #4)
> Comment on attachment 455842 [details] [diff] [review]
> Fix C++ Components - v1
> 
> >diff --git a/calendar/base/build/calBaseModule.cpp b/calendar/base/build/calBaseModule.cpp
> >--- a/calendar/base/build/calBaseModule.cpp
> >+++ b/calendar/base/build/calBaseModule.cpp
> [...]
> >+NSMODULE_DEFN(calbase) = &kCalBaseModule;
> 
> For a static build it may be mandatory to replace 'calbase' with
> 'calBaseModule'!
From what has happened on mail/mailnews I think they have done a similar replacement.

Comment 6

[Philipp Kewisch [:Fallen][📱📆]]

Attachment: JS - Fix Protocol Handler - v1

So the JS part is obviously much larger. I needed to clean up a few things so we can use the new XPCOMUtils.generateNSGetFactory() methods. I've specifically tried to make the first few patches compatible to comm-1.9.2, so we can backport them in case the next release needs to stay with comm-1.9.2 (details will follow, there were some Thunderbird decisions made that will make things complicated)


This patch fixes the webcal protocol handler to use its own objects for webcal vs webcals. It should be compatible to comm-1.9.2.

I had a few crashes that went away when deleting compreg.dat/xpti.dat. I've changed the uuid of the components to make sure, but we don't want this to happen for nightly profiles! Please test that specifically.

Comment 7

[Philipp Kewisch [:Fallen][📱📆]]

Attachment: [checked in] JS - Fix Protocol Handler - v2

Ack, took the patch from the wrong tree

Comment 8

[Philipp Kewisch [:Fallen][📱📆]]

Attachment: JS - Remove Component Constructors - v1

This patch gets rid of the component constructors, which are not widely used and just in the way for the new registration.

This patch should also work for comm-1.9.2

Comment 9

[Philipp Kewisch [:Fallen][📱📆]]

Attachment: [checked in] JS - Make classinfo inline - v1

This patch makes the classInfo stuff part of the component object itself. This is needed since XPCOMUtils expects the classID to be part of the object prototype, which is useful for both generateNSGetModule and generateNSGetFactory

With a slight modification in the patch in calCalendarManger.js, this also applies to comm-1.9.2.

Comment 10

[Martin Schröder [:mschroeder]]

Comment on attachment 456831 [details] [diff] [review]
JS - Remove Component Constructors - v1

Constructors are also used and have not been replace at:

http://mxr.mozilla.org/comm-central/source/calendar/base/src/calItipItem.js#208
http://mxr.mozilla.org/comm-central/source/calendar/providers/wcap/calWcapSession.js#875
http://mxr.mozilla.org/comm-central/source/calendar/base/src/calRecurrenceInfo.js#147
http://mxr.mozilla.org/comm-central/source/calendar/base/src/calItemBase.js#857

Comment 11

[Ian Neal]

Are any of these JS patches making use of XPCOMUtils.jsm yet?

Comment 12

[Ian Neal]

(In reply to comment #10)
> Comment on attachment 456831 [details] [diff] [review]
> JS - Remove Component Constructors - v1

When trying to apply this patch I get:
applying xpcom-inline-classinfo.diff
patching file calendar/base/src/calCalendarManager.js
Hunk #1 FAILED at 116
1 out of 1 hunks FAILED -- saving rejects to file calendar/base/src/calCalendarManager.js.rej

Comment 13

[Martin Schröder [:mschroeder]]

(In reply to comment #11)
> Are any of these JS patches making use of XPCOMUtils.jsm yet?

No, because they contain just preliminary changes to prepare the code for converting it to XPCOMUtils usage. Instead of having to review a giant mixed patch, it is imho far easier to review those incremental patches.

Comment 14

[Stefan Sitter [:ssitter]]

No testing and catching trunk regressions possible since 3 weeks, marking as blocker.

After applying all patches including the change from Comment 4 and manually fixing the error from Comment 12 the build runs until the following error:

make[6]: Entering directory `.../obj/tb-dbg/calendar/base/src'
.../src/config/rules.mk:1708: *** .js component without matching .manifest.  Stop.

Comment 15

[Philipp Kewisch [:Fallen][📱📆]]

Sorry, upcoming exams have been keeping me from continuing on this. I've run into a few problems with my further patches, so I couldn't continue uploading. I can upload a wip patch if someone wants to continue?

Comment 16

[Philipp Kewisch [:Fallen][📱📆]]

We need bug 579178 fixed to build lightning without horrible hacks.

Comment 17

[Martin Schröder [:mschroeder]]

Comment on attachment 456831 [details] [diff] [review]
JS - Remove Component Constructors - v1

see comment#4

Comment 18

[Martin Schröder [:mschroeder]]

Comment on attachment 456829 [details] [diff] [review]
[checked in] JS - Fix Protocol Handler - v2

> calProtocolHandler.prototype = {
>+    getInterfaces: function cP_getInterfaces(aCount) {
>+        const interfaces = [Components.interfaces.nsIProtocolHandler,
>+                            Components.interfaces.nsIClassInfo,
>+                            Components.interfaces.nsISupports];
>+
>+        aCount = interfaces.length;
>+        return interfaces;
>+    },

Should be aCount.value = interfaces.length; !

r=mschroeder

Comment 19

[Philipp Kewisch [:Fallen][📱📆]]

Attachment: [checked in] JS - Remove Component Constructors - v2

Fixes most of comment 10. calRecurrenceInfo/calItipItem don't use Component constructors, they just re-instanciate the same object. This is needed for correct cloning.

Comment 20

[Philipp Kewisch [:Fallen][📱📆]]

Attachment: [checked in] JS - More Components (to NSGetModule) - v1

I've decided to go the normal route, as I should have from the beginning: patch for trunk, backport if needed. This patch should apply cleanly to comm-central with the previous patches applied. BTW, This is the patch order:

bug-582695-xpcom-cc-packaging.diff
bug-563170-addon-api.diff
bug-576746-xpcom-cpp.diff
bug-576746-xpcom-remove-constructors.diff
bug-576746-xpcom-fix-protocolhandler.diff
bug-576746-xpcom-inline-classinfo.diff
bug-576746-xpcom-mixed-components.diff
bug-576746-xpcom-change-module.diff

Comment 21

[Martin Schröder [:mschroeder]]

Comment on attachment 460978 [details] [diff] [review]
[checked in] JS - Remove Component Constructors - v2

r=mschroeder

Comment 22

[Philipp Kewisch [:Fallen][📱📆]]

Attachment: [checked in] JS - Change Module Registration - v1

This is the final patch, it changes the component registration from NSGetModule to NSGetFactory, adds the manifests, and so on.

With all the patches from the previous comment applied, calendar should build and work again :-)

Comment 23

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 456829 [details] [diff] [review]
[checked in] JS - Fix Protocol Handler - v2

Pushed to comm-central, changeset c23784eaa1e6

Comment 24

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 460978 [details] [diff] [review]
[checked in] JS - Remove Component Constructors - v2

pushed to comm-central, changeset 7a64ef481f29

Comment 25

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 455842 [details] [diff] [review]
[checked in] Fix C++ Components - v1

pushed to comm-central, changeset 94c963616c8a

Comment 26

[Philipp Kewisch [:Fallen][📱📆]]

This bug doesn't need tb-integration, that flag is rather for bugs that would block lightning being shipped with thunderbird. While this would obviously block, we don't want to put every regression/core fixes on the tb-integration list :-)

Comment 27

[Martin Schröder [:mschroeder]]

Comment on attachment 456836 [details] [diff] [review]
[checked in] JS - Make classinfo inline - v1

The part for calendar/base/src/calCalendarManager.js does not apply cleanly!

>diff --git a/calendar/base/src/calDateTimeFormatter.js b/calendar/base/src/calDateTimeFormatter.js
>--- a/calendar/base/src/calDateTimeFormatter.js
>+++ b/calendar/base/src/calDateTimeFormatter.js
[...]
>+        aCount = interfaces.length;
>+        return interfaces;
>+    },

Use aCount.value

>+        return cal.doQueryInterface(this, calDateTimeFormatter.__proto__, aIID, null, this);

Use calDateTimeFormatter.prototype


>diff --git a/calendar/base/src/calTodo.js b/calendar/base/src/calTodo.js
>--- a/calendar/base/src/calTodo.js
>+++ b/calendar/base/src/calTodo.js
[...]
>-        return doQueryInterface(this, calEvent.prototype, aIID, null, calTodoClassInfo);
>+        return cal.doQueryInterface(this, calEvent.prototype, aIID, null, this);
>     },

calEvent.prototype as argument is plain wrong... but it has been there before.


>diff --git a/calendar/base/src/calTransactionManager.js b/calendar/base/src/calTransactionManager.js
>--- a/calendar/base/src/calTransactionManager.js
>+++ b/calendar/base/src/calTransactionManager.js
[...]
>+        return cal.doQueryInterface(this, calTransactionManager.__proto__, aIID, null, this);

Use calTransactionManager.prototype


>+        return cal.doQueryInterface(this, calTransactionManager.__proto__, aIID, null, this);

Should be calTransaction.prototype !


r=mschroeder with those nits fixed

Comment 28

[Martin Schröder [:mschroeder]]

Comment on attachment 460988 [details] [diff] [review]
[checked in] JS - More Components (to NSGetModule) - v1

r=mschroeder with following considered:

There exist some inconsistencies in 'QueryInterface' occurrences:
* calICSCalendar: Should use 'this' instead of explicit array as last argument
* calGoogleSession:  Should use 'cal.doQueryInterface', the correct prototype (calGoogleSession instead of calGoogleSessionManager) and 'this' as last argument
* calWcapCalendar: Should use 'cal.doQueryInterface' and 'this' as last argument

Can we get rid of this magic script loading in the future (i.e. in a newly filed bug)?

Comment 29

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 460988 [details] [diff] [review]
[checked in] JS - More Components (to NSGetModule) - v1

Issues taken care of, thanks for noticing.
Checked in to comm-central, rev 0776ec4be64a

Comment 30

[Philipp Kewisch [:Fallen][📱📆]]

> Can we get rid of this magic script loading in the future (i.e. in a newly
> filed bug)?

What do you mean with magic script loading? You mean loading extra scripts instead of using a module for each component?

Comment 31

[Martin Schröder [:mschroeder]]

Comment on attachment 460992 [details] [diff] [review]
[checked in] JS - Change Module Registration - v1

r=mschroeder

Comment 32

[Martin Schröder [:mschroeder]]

Comment on attachment 460988 [details] [diff] [review]
[checked in] JS - More Components (to NSGetModule) - v1

Philipp, maybe you should re-add calProviderUtils.jsm to calWcapCalendar and calCompositeCalendar? They have been there before in now removed code, but it is also possible they are not needed anymore.

Comment 33

[Philipp Kewisch [:Fallen][📱📆]]

Attachment: JS - Fix Command Line Service - v1

It really bugged me that there was still an instance of NSGetModule in the tree. This fixes the last file. Note I haven't tested it on a sunbird build, but its at least syntactially correct. I don't expect a functional test, just a brief code review is sufficient.

Comment 34

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 456836 [details] [diff] [review]
[checked in] JS - Make classinfo inline - v1

Checked in to comm-central rev 4f513cc91609

Comment 35

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 460992 [details] [diff] [review]
[checked in] JS - Change Module Registration - v1

Checked in to comm-central rev c2cb7f16ade9

Note I also prefixed getUUID calls with cal. since this caused problems in the memory calendar.

Comment 36

[Martin Schröder [:mschroeder]]

(In reply to comment #27)
> >diff --git a/calendar/base/src/calTransactionManager.js b/calendar/base/src/calTransactionManager.js
> >--- a/calendar/base/src/calTransactionManager.js
> >+++ b/calendar/base/src/calTransactionManager.js
> [...]
> 
> >+        return cal.doQueryInterface(this, calTransactionManager.__proto__, aIID, null, this);
> 
> Should be calTransaction.prototype !

This needs still to be fixed in a small followup patch!

Comment 37

[Martin Schröder [:mschroeder]]

Comment on attachment 463953 [details] [diff] [review]
JS - Fix Command Line Service - v1

>diff --git a/calendar/resources/Makefile.in b/calendar/resources/Makefile.in
>--- a/calendar/resources/Makefile.in
>+++ b/calendar/resources/Makefile.in

[...]

>-CalendarModule.unregisterSelf =
>-function(compMgr, fileSpec, location)
>-{
>-    compMgr = compMgr.QueryInterface(Components.interfaces.nsIComponentRegistrar);
>-
>-    compMgr.unregisterFactoryLocation(CLINE_SERVICE_CID, fileSpec);
>-    compMgr.unregisterFactoryLocation(ICALCNT_HANDLER_CID, fileSpec);
>-
>-    catman = Components.classes["@mozilla.org/categorymanager;1"]
>-                       .getService(nsICategoryManager);
>-    catman.deleteCategoryEntry("command-line-argument-handlers",
>-                               "calendar command line handler", true);
>-    catman.deleteCategoryEntry("command-line-handler",
>-                               "x-default", true);
>-}


>diff --git a/calendar/resources/content/calendarService.manifest b/calendar/resources/content/calendarService.manifest
>new file mode 100644
>--- /dev/null
>+++ b/calendar/resources/content/calendarService.manifest
>@@ -0,0 +1,9 @@
>+component {65ef4b0b-d116-4b93-bf8a-84525991bf27} calendarService.js
>+contract @mozilla.org/commandlinehandler/general-startup;1?type=calendar {65ef4b0b-d116-4b93-bf8a-84525992bf27}
>+category command-line-handler x-default @mozilla.org @mozilla.org/commandlinehandler/general-startup;1?type=calendar

The last line seems to incomplete, and also one category from above seems to be missing. r=mschroeder with this issue fixed.

Comment 38

[Martin Schröder [:mschroeder]]

(In reply to comment #30)
> > Can we get rid of this magic script loading in the future (i.e. in a newly
> > filed bug)?
> 
> What do you mean with magic script loading? You mean loading extra scripts
> instead of using a module for each component?

Correct, especially that it has to be in a particular load order starting with calUtils.js. What are the alternatives?

Comment 39

[Philipp Kewisch [:Fallen][📱📆]]

> >+contract @mozilla.org/commandlinehandler/general-startup;1?type=calendar {65ef4b0b-d116-4b93-bf8a-84525992bf27}
> >+category command-line-handler x-default @mozilla.org @mozilla.org/commandlinehandler/general-startup;1?type=calendar
> 
> The last line seems to incomplete, and also one category from above seems to be
> missing. r=mschroeder with this issue fixed.
Yes, one @mozilla.org too much, thanks! The other category was a left over from way back when there was xpfe. I've removed support for the xpfe command line handler now.


(In reply to comment #38)
> (In reply to comment #30)
> > > Can we get rid of this magic script loading in the future (i.e. in a newly
> > > filed bug)?
> > 
> > What do you mean with magic script loading? You mean loading extra scripts
> > instead of using a module for each component?
> 
> Correct, especially that it has to be in a particular load order starting with
> calUtils.js. What are the alternatives?
If we are luck, this could be "fixed on trunk". One of the problems was that a Cu.import() line with calUtils.jsm didn't work in the module, since the subsitution for calendar was not set in the resource:// protoocl. We do this manually in calItemModule. If we are lucky then due to all the xpcom changes, its now sufficient to have a line to jar.mn/chrome.manifest that registers the calendars subsitution. This line still needs to be first in chrome.manifest, but thats way better than we are doing now. If that turns out to be true, then we can just add a module line in each file.

On the other hand, if we do that, then the way the build system currently works, we need one manifest per component. This will have a negative impact on startup time. What we could do is set NO_JS_MANIFEST=1 and then just add a single manifest file like we are doing now. Not sure if that works though.

Comment 40

[Philipp Kewisch [:Fallen][📱📆]]

Pushed to comm-central <http://hg.mozilla.org/comm-central/rev/69a6185f0d19>

-> FIXED

Comment 41

[Martin Schröder [:mschroeder]]

Attachment: Fix for issue in comment#36

Comment 42

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 490482 [details] [diff] [review]
Fix for issue in comment#36

r=philipp, thanks for taking care!

Comment 43

[Philipp Kewisch [:Fallen][📱📆]]

Pushed to comm-central <http://hg.mozilla.org/comm-central/rev/63f7bf576994>
-> FIXED