As part of the patch for bug 234455, we lost the checking for NOTIFY_HANDLE_DEFAULT. This is not good, because the flag in nsIXTFElement is essentially dead. I'm trying to implement a XTF-based language now, and I worry that throwing NOT_IMPLEMENTED will break things. There's two options: (1) Restore the check for NOTIFY_HANDLE_EVENT, or (2) Remove the NOTIFY_HANDLE_EVENT flag from nsIXTFElement. Smaug, which do you think we should do?

(1)

I'll write a xpcshell test afterwards.

Comment on attachment 262328 [details] [diff] [review] patch, v1 No need to add () around eventStatus check.

sayrer: This patch also adds a do_load_module() capability to head.js, for convenience. Now we'll be able to build JS-based XPCOM components for testing only, and load them via xpcshell, just as this testcase does. It's also worth noting this patch contributes the first sample for a JS-based XTF component in mozilla.org's cvs.

Comment on attachment 262334 [details] [diff] [review] xpcshell testcase, v1 >+FooElement.prototype = >+{ >+ // nsIXTFElement >+ onCreated: function onCreated(aWrapper) { >+ aWrapper.notificationMask = 0; >+ this._baseElementNode = aWrapper.elementNode; Doesn't this cause a leak? At least it does in C++. FooElement has the pointer to DOMElement and DOMElement has pointer to FooElement. See comment http://lxr.mozilla.org/seamonkey/source/content/xtf/public/nsIXTFElement.idl#56

That should be easily fixable by setting this._baseElementNode to null in onDestroyed. Honestly, I don't know if it leaks.

(In reply to comment #6) > That should be easily fixable by setting this._baseElementNode to null in > onDestroyed. No, that doesn't work. Because of the cycle OnDestroyed doesn't get called ever.

(In reply to comment #7) > No, that doesn't work. Because of the cycle OnDestroyed doesn't get called > ever. > Again, that it at least in C++.

Smaug: I really don't see any way around it. This is the sort of thing the XPCOM cycle collector should take care of, I'd think. I agree that for the purposes of *this* testcase, it's not needed. But for other testcases, where the user is implementing a custom DOM for an element (say, pulling the operator property of MathMLApplyElement - http://www.w3.org/TR/MathML2/appendixd.html#dom.ApplyElement), it does become necessary to store the original element.

You can store the wrapper, or in C++ you can store raw pointer to the domelement, but make sure not to use those after ondestroyed.

Comment on attachment 262334 [details] [diff] [review] xpcshell testcase, v1 transferring review request to smaug; sayrer says he isn't appropriate for this.

Comment on attachment 262334 [details] [diff] [review] xpcshell testcase, v1 >+FooElement.prototype = >+{ >+ // nsIXTFElement >+ onCreated: function onCreated(aWrapper) { >+ aWrapper.notificationMask = 0; >+ this._baseElementNode = aWrapper.elementNode; Please don't store .elementNode. >+ /* XXX ajvincent Stupid API doesn't support namespaced attributes!!! >+ <markup:foo bar:baz="wop" foo:baz="bleh"/> will cause problems. >+ Alternative: use nsIXTFElement.handleDefault() >+ */ Er, what. XTF supports only attributes in null namespace. So foo:baz/bar:baz won't cause any problems. And anyway, there is a bug open to support namespaced attributes. So please remove the comment. And what has handleDefault to do with attributes? (maybe your idea is to use mutation events or something) >+ // test for Bug 378247 >+ handleDefault: function handleDefault(aEvent) { >+ this.inner.wrappedJSObject.testpassed = false; >+ }, I'd like the test to test both cases, when handleDefault should be called and when it shouldn't. >+ // Discover what interfaces we need >+ var i = Components.interfaces; >+ var found = false; >+ for (var prop in i) { >+ if (aIID.equals(i[prop])) { >+ found = true; >+ dump("QUERYINTERFACE: FooElement " + prop + "\n"); >+ break; >+ } >+ } >+ if (!found) { >+ dump("QUERYINTERFACE: FooElement Unknown\n"); >+ } >+ return null; What is this? Or why it is here? >+function FooElementFactory() {} >+FooElementFactory.prototype = >+{ >+ // nsIXTFElementFactory >+ createElement: function createElement(aLocalName) { >+ if (aLocalName in FooInner) { Because this tries to be an example of XTF element too, I think it might be better to keep it simple. So (aLocalName in FooInner) is sort of strange. I'd guess the normal way is "if (aLocalName == 'foo')" >+ return (new FooElement(aLocalName)).QueryInterface(nsIXTFElement); >+ } >+ NOT_IMPLEMENTED(); >+ return null; >+ }, Just return null >+ >+ // nsIClassInfo Do you need classinfo in this example/test? >+ // Discover what interfaces we need >+ var i = Components.interfaces; >+ var found = false; >+ for (var prop in i) { >+ if (aIID.equals(i[prop])) { >+ found = true; >+ dump("QUERYINTERFACE: FooElementFactory " + prop + "\n"); >+ break; >+ } >+ } >+ if (!found) { >+ dump("QUERYINTERFACE: FooElementFactory Unknown\n"); >+ } Again, why this is here. Please keep the example/test simple.

transferring r+ from smaug, cruft cleaned up. Something funny, though: Normally unit tests go into (foo)/test/unit, not (foo)/tests/unit. When I looked at content/xtf earlier, there was a content/xtf/tests directory. Now I can't find it. Would you please check this patch in, smaug, if it still meets with your approval?

Oops, did I mark it r+. Wasn't going to. I'll re-review.

Comment on attachment 262556 [details] [diff] [review] xpcshell testcase So you're still not testing the both cases, when the NOTIFY_HANDLE_DEFAULT is set and when it is not set.

This testcase also covers support for handling default events.

Comment on attachment 264182 [details] [diff] [review] xpcshell testcase I think this is enough. Though I'm not too familiar with xpcshell tests