Make calendar XPCOM components use new manifests and data tables

RESOLVED FIXED in 1.0b4

Status

defect
--
blocker
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: Fallen, Assigned: Fallen)

Tracking

Trunk
1.0b4
Dependency tree / graph

Details

Attachments

(8 attachments, 2 obsolete attachments)

11.63 KB, patch
martinschroeder
: review+
Details | Diff | Splinter Review
5.49 KB, patch
martinschroeder
: review+
Details | Diff | Splinter Review
75.46 KB, patch
martinschroeder
: review+
Details | Diff | Splinter Review
10.01 KB, patch
martinschroeder
: review+
Details | Diff | Splinter Review
129.62 KB, patch
martinschroeder
: review+
Details | Diff | Splinter Review
55.44 KB, patch
martinschroeder
: review+
Details | Diff | Splinter Review
19.50 KB, patch
martinschroeder
: review+
Details | Diff | Splinter Review
2.34 KB, patch
Fallen
: review+
Details | Diff | Splinter Review
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.
Attachment #455842 - Flags: review?(dbo.moz)

Comment 1

9 years ago
> 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)
(Assignee)

Comment 2

9 years ago
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.
Depends on: 576767
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.
Attachment #455842 - Flags: review?(dbo.moz) → review+
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

9 years ago
(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.
(Assignee)

Comment 6

9 years ago
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.
Attachment #456828 - Flags: review?(Mozilla)
(Assignee)

Comment 7

9 years ago
Ack, took the patch from the wrong tree
Attachment #456828 - Attachment is obsolete: true
Attachment #456829 - Flags: review?(Mozilla)
Attachment #456828 - Flags: review?(Mozilla)
(Assignee)

Comment 8

9 years ago
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
Attachment #456831 - Flags: review?(Mozilla)
(Assignee)

Comment 9

9 years ago
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.
Attachment #456836 - Flags: review?(mschroeder)
(Assignee)

Updated

9 years ago
Attachment #456836 - Attachment description: JS - Make classinfo inline → JS - Make classinfo inline - v1

Comment 11

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

Comment 12

9 years ago
(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
(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.
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.
Severity: normal → blocker
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?
We need bug 579178 fixed to build lightning without horrible hacks.
Depends on: 579178
Comment on attachment 456831 [details] [diff] [review]
JS - Remove Component Constructors - v1

see comment#4
Attachment #456831 - Flags: review?(Mozilla) → review-
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
Attachment #456829 - Flags: review?(Mozilla) → review+
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.
Attachment #460978 - Flags: review?(mschroeder)
(Assignee)

Updated

9 years ago
Attachment #456831 - Attachment is obsolete: true
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
Attachment #460988 - Flags: review?(mschroeder)
Comment on attachment 460978 [details] [diff] [review]
[checked in] JS - Remove Component Constructors - v2

r=mschroeder
Attachment #460978 - Flags: review?(mschroeder) → review+
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 :-)
Attachment #460992 - Flags: review?(mschroeder)
Comment on attachment 456829 [details] [diff] [review]
[checked in] JS - Fix Protocol Handler - v2

Pushed to comm-central, changeset c23784eaa1e6
Attachment #456829 - Attachment description: JS - Fix Protocol Handler - v2 → [checked in] JS - Fix Protocol Handler - v2
Comment on attachment 460978 [details] [diff] [review]
[checked in] JS - Remove Component Constructors - v2

pushed to comm-central, changeset 7a64ef481f29
Attachment #460978 - Attachment description: JS - Remove Component Constructors - v2 → [checked in] JS - Remove Component Constructors - v2
Comment on attachment 455842 [details] [diff] [review]
[checked in] Fix C++ Components - v1

pushed to comm-central, changeset 94c963616c8a
Attachment #455842 - Attachment description: Fix C++ Components - v1 → [checked in] Fix C++ Components - v1
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 :-)
Flags: tb-integration?
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
Attachment #456836 - Flags: review?(mschroeder) → review+
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)?
Attachment #460988 - Flags: review?(mschroeder) → review+
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
Attachment #460988 - Attachment description: JS - More Components (to NSGetModule) - v1 → [checked in] JS - More Components (to NSGetModule) - v1
> 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 on attachment 460992 [details] [diff] [review]
[checked in] JS - Change Module Registration - v1

r=mschroeder
Attachment #460992 - Flags: review?(mschroeder) → review+
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.
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.
Attachment #463953 - Flags: review?
(Assignee)

Updated

9 years ago
Attachment #463953 - Flags: review? → review?(mschroeder)
Comment on attachment 456836 [details] [diff] [review]
[checked in] JS - Make classinfo inline - v1

Checked in to comm-central rev 4f513cc91609
Attachment #456836 - Attachment description: JS - Make classinfo inline - v1 → [checked in] JS - Make classinfo inline - v1
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.
Attachment #460992 - Attachment description: JS - Change Module Registration - v1 → [checked in] JS - Change Module Registration - v1
(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 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.
Attachment #463953 - Flags: review?(mschroeder) → review+
(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?
> >+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.
Pushed to comm-central <http://hg.mozilla.org/comm-central/rev/69a6185f0d19>

-> FIXED
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.0b3
Comment on attachment 490482 [details] [diff] [review]
Fix for issue in comment#36

r=philipp, thanks for taking care!
Attachment #490482 - Flags: review?(philipp) → review+
(Assignee)

Updated

9 years ago
Target Milestone: 1.0b3 → Trunk
You need to log in before you can comment on or make changes to this bug.