325726 - some calIObserver impls are missing methods, which causes exceptions

Status: VERIFIED FIXED

(Calendar :: Internal Components, defect)

Description

[Daniel Boelzle [:dbo]]

This avoids XPConnect exceptions when notifying (although those are currently caught and ignored).

Comment 1

[Daniel Boelzle [:dbo]]

Attachment: implemting QueryInterface() + two minor cleanups

Additionally, the patch fixes two leftovers a la "addObserver(observer, 0)", which don't hurt in JS, but are superfluous.

Comment 2

[Daniel Boelzle [:dbo]]

Comment on attachment 210572 [details] [diff] [review]
implemting QueryInterface() + two minor cleanups

does not fix all problems, will create a new one.

Comment 3

[Daniel Boelzle [:dbo]]

Attachment: adding lots of missing QueryInterface() functions, adding missing calIObserver methods

Comment 4

[Michiel van Leeuwen (email: mvl+moz@)]

I always though xpconnect was smart, so that it did some kind of auto QI. Can you show the problem this patch is fixing?

Comment 5

[Daniel Boelzle [:dbo]]

One of the fixes in the patch (I don't remember which one) fixed a js error that has been annoying for me more than once... so I started to investigate, grepped over for all observer occurrences, fixing.  Concerning QI, IMO it doesn't matter whether js can cope with calling; this is more about the future when the client code in no js.  I know, this is currently not the case (for observers), but will may lead to issues in the future.

Comment 6

[Dan Mosedale (:dmosedale, :dmose)]

XPConnect's auto-implementation of QI for certain cases is on behalf of the callee, not the caller.  So if the caller someday moves to C++, there's no issue.  If these JS objects move to C++, the right time to implement QI would be then, as implementing now only bloats the code and provides no advantage.

Comment 7

[Dan Mosedale (:dmosedale, :dmose)]

(This couldn't possibly be overlooked at port-from-JS-to-C++ time, because attempting to port this code without adding the nsISupports method won't compile.)

Comment 8

[Daniel Boelzle [:dbo]]

@mvl/dmose: Yes, you are right, js QI can be omitted if just one interface is implemented and the wrapped js object is passed to native using the correct interface (which is most often enforced by the signature of the called method). [Whether this bloats code is IMO more a matter of taste, for me QI also clearly documents that XPCOM interface(s) are implemented, and which ones, so I commonly just code QI.]
Please keep on reviewing the rest, though.

Comment 9

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 210597 [details] [diff] [review]
adding lots of missing QueryInterface() functions, adding missing calIObserver methods

The QIs are bloat in the sense that they will compile to JS bytecodes, so that even when just that bytecode is loaded via fastload, it'll occupy memory space but never actually be executed.  You have a good point about documentation/clarify, however.  How about just adding a comment that describes what interface is being implemented and points out that XPConnect will take care of the QI in that case?

/cygdrive/d/pim/mozilla/mozilla_ref/calendar/resources/content/unifinder.js mozilla/calendar/resources/content/unifinder.js
>--- /cygdrive/d/pim/mozilla/mozilla_ref/calendar/resources/content/unifinder.js	2006-02-03 14:48:30.197044000 +0100
>+++ mozilla/calendar/resources/content/unifinder.js	2006-02-03 14:49:02.964161600 +0100
>@@ -165,17 +165,17 @@ var unifinderObserver = {
>         if (!this.mInBatch)
>             refreshEventTree();
>     },
>     onDeleteItem: function(aDeletedItem) {
>         if (!this.mInBatch)
>             refreshEventTree();
>     },
>     onAlarm: function(aAlarmItem) {},
>-    onError: function(aMessage) {},
>+    onError: function(errno, aMessage) {},

How about using aErrNo here and in unifinderToDo.js for consistency with the surrounding code?

r=dmose with that tweak, and with the QIs replaced by comments as suggested above.

Comment 10

[Daniel Boelzle [:dbo]]

Attachment: reworked patch

Comment 11

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 215387 [details] [diff] [review]
reworked patch

r=dmose; thanks for the patch!

Comment 12

[Dan Mosedale (:dmosedale, :dmose)]

Fix landed with cross-commit on both trunk and branch.

Comment 13

[Reed Loden [:reed]]

The bugspam monkeys have been set free and are feeding on Calendar :: Internal Components. Be afraid for your sanity!