Closed Bug 164482 (XMLEvents) Opened 22 years ago Closed 20 years ago

Tracking: XML Events

Categories

(Core :: DOM: Events, enhancement)

Product:
Core
Component:
DOM: Events
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: mdubinko, Assigned: smaug)

References

(
URL
)

Details

(Keywords: helpwanted)

Attachments

(1 file, 5 obsolete files)

v5 (v4 + jst 's comments)
37.79 KB, patch
Details | Diff | Splinter Review
"XML Events" is a W3C specification to provide XML languages with the ability to integrate declarative event listeners and event handlers. This is a core technology needed by XHTML 2.0 and XForms. In the future, SVG and SMIL will possibly adopt this technique also.
Blocks: xhtml2
Blocks: xforms
The XBL implementation also uses declarative events, and that code could be reused to implement XML events in about 2-3 hours. :)
Any XBL hackers out there willing to write a prototype?
Keywords: helpwanted, mozilla1.3
xah: I don't see how we can get this in for Mozilla 1.3, considering XML Events is still a W3C Working Draft... I think it's extremely premature to add the mozilla1.3 keyword at this stage.
Right! Thanks.
Keywords: mozilla1.3
:) W3C has advanced XML Events to Candidate Rec. To exit that stage, it needs a working implementation which doesn't rely on XForms. I'm willing to consider helping to develop this (I've been having a lot of fun with XBL lately). I don't think I could quite do it in "2-3 hours"...
I may not be able to do this properly after all. The ev:listener element is totally optional. I could bind to that, but not to all Element nodes, at least not in a way which I'd expect m.o to accept.
ajvincent: In the absense of an ev:listener element, the minimum you need is an ev:event attribute on an element. So a stylesheet to set up the binding properly could look something like: @namespace url("http://www.w3.org/2001/xml-events"); listener, *[event] { -moz-binding: url(xml-events.xml#listener); } This will attach the binding to any ev:listener element or any element with the ev:event attribute, which is exactly what is needed. I've just finished off a Java implementation of XML Events and was thinking of having a shot at this as well, if you want a hand with anything then please let me know as I'd be happy to help - XML Events support in Moz would rock.
Not good enough. (1) You'd need a CSS @namespace rule (it's a Mozilla-specific rule) for the ev:listener element, and that would have to be in a separate CSS stylesheet than the generic approach you're talking. (2) If you use a -moz-binding property on the more generic one, it will override any other -moz-binding which would normally exist on the element. This is really, really bad. I already thought of that.
> (1) You'd need a CSS @namespace rule Like the one above? :) > (2) If you use a -moz-binding property on the more generic one, it will > override any other -moz-binding which would normally exist on the element. Ah yes, you're right. I played around with document.addBinding(), but that mostly seems to make Moz crash. Perhaps getting a custom event fired when XML Event elements/attrs are found (like the way the link toolbar works) might be a solution - although that seems like a pretty big hack.
Actually, I figured out a way to do the observer strictly with XBL. We only need to have <xbl:element/> enabled. (Bug 98712) [ev|event], [ev|xbl_binding] { -moz-binding("events.xml#missingListener"); } <binding id="missingListener"> <content> <ev:listener/> <element/> </content> <implementation> <constructor><![CDATA[ this.setAttributeNS("http://www.w3.org/2001/xml-events", "xbl_binding", "previousAnonymousNode"); var listenerNode = document.getAnonymousNodes(this)[0]; for (var j = 0; j < this.attributes.length; j++) { var attr = this.attributes.item(j); if ((attr.namespaceURI == "http://www.w3.org/2001/xml-events")&&(attr.localName != "xbl_binding")) { this.removeAttributeNode(attr); listenerNode.setAttribute(attr.localName, attr.nodeValue); } } ]]></constructor> </implementation> </binding> What that would do is create for <node ev:event="foo" bar="baz"><child1/></node> is the following tree: <node bar="baz" ev:xbl_binding="previousAnonymousNode"> <ev:listener event="foo"/> <child1/> </node> The ev:xbl_binding attribute makes sure the binding remains in force despite removal of other XML Events attributes. That handles sections 3.1 and 3.2; what are we going to do about section 3.3? 3.3.1, examples 1 and 3, look like something we've tried hard to avoid, namely, IE's approach to events.
Bug 204961, if it's valid, may give a solution for XML Events handlers.
XML Events are now a W3C recommendation: <http://www.w3.org/TR/2003/REC-xml-events-20031014/>, any chance this is going to work soon?
.
Assignee: joki → events
Depends on: 204961
QA Contact: vladimire → ian
This can happen only with some traction on bugs that block this bug. However, I am not sure bug 204961 blocks this bug, given Hixie's final comment there. How about bug 98712, should that be a blocker of this bug? /be
I don't know of any bugs that would block us from implementing XML Events. XML Events should be implemented natively (possibly using code very similar to XBL's, according to Hyatt). XML Events shouldn't be implemented using XBL.
No longer depends on: 204961
Assignee: events → smaug
Reassigned bug to show that we are active on the bug. Already working on a test implementation (native) with Smaug and Sicking.
Pretty much everything should be implemented. But this needs *testing* and also some clarifications from HTML/XML Events WG. Some testcases: http://www.cs.helsinki.fi/u/pettay/moztests/xmlevents/
The patch does not work with XUL or SVG elements.
Sorry the spam. I meant to say that XML Events can't be defined inside XUL/SVG elements. But using for example <xe:listener/> -element it is possible to bind listeners/handlers to XUL elements.
Comment on attachment 146976 [details] [diff] [review] The current (test?) implementation. This is an unsolicited code review from the perspective of someone who knows XML Events from the QA perspective, but doesn't know much about our code base at all. Apologies for the many misunderstandings I'm sure to have about what your code did. Overall comment: You need more comments, preferably pointing to specific sections of the XML Events spec at appropriate points to explain what is going on and why. (I don't mean comment every line, just give the reader an overview of what blocks of code are doing so that you don't have to be an XML Events expert to work it out.) >+ void Unfinish(); Could you put some comments somewhere explaining what "finished" means in this context? It isn't clear in an initial reading. >+ if (nameSpaceID == kNameSpaceID_XMLEvents) //for <xe:listener/> >+ nameSpaceID = kNameSpaceID_None; >+ else >+ nameSpaceID = kNameSpaceID_XMLEvents; It didn't look like you ever actually checked that it was an XML Events "listener" element. The following: <ev:handler event=""> ... </ev:handler> ...shouldn't do _anything_ in this implementation, especially not be treated as an <ev:listener>. >+ if (aContent->HasAttr(nameSpaceID, nsHTMLAtoms::handler)) { >+ aContent->GetAttr(nameSpaceID, nsHTMLAtoms::handler, handlerURI); >+ if (handlerURI.IsEmpty()) >+ return PR_FALSE; Why do you bail if the handler attribute is empty? If the handler attribute is empty, that means that the handler is the entire document (assuming the baseURI at that point is the current document). >+ if (aContent->HasAttr(nameSpaceID, nsHTMLAtoms::target)) { >+ aContent->GetAttr(nameSpaceID, nsHTMLAtoms::target, targetIdref); >+ if (targetIdref.IsEmpty()) >+ return PR_FALSE; Again, why bail if it is empty? As far as I can tell, if it is blank, that means the event target must also have a blank ID (although the spec is not overly clear about this). >+ if (!hasObserver) { ... >+ } >+ else { ... >+ doc->GetElementById(observerID, getter_AddRefs(el)); This won't work because the XML Events node might be added or removed before or after the element with the ID. Basically when "observer" is set you will have to track all changes to IDs in the document in order to make sure you are tracking which element you are supposed to be listening to. (As far as I can tell from the extremely vague and underspecified spec.) >+ if (target) { >+ nsCOMPtr<nsIDOMEventTarget> eventTarget = ... Again, the way to handle "target" is to check, whenever the event listener is fired, whether the target actually has the correct ID, and to only run the event handler _if_ it has that ID. This attribute should not, IMHO, have any effect on the registration part. Note in particular that the _target_ has nothing to do with the _observer_ which is the node you want to be registering the event upon. >+ aManager->RemoveXMLEventsContent(aContent); Could you explain what this does? (And why?) >+ if (mStopPropagation) >+ aEvent->StopPropagation(); >+ if (mCancelDefault) >+ aEvent->PreventDefault(); From the spec, I think this should be done _after_ the event handler code is fired. But the spec is a bit vague. (It does make a difference since you can tell whether the event is cancelled or not from script.) >+ if (!mHandler.IsEmpty() && StringBeginsWith(mHandler, NS_LITERAL_STRING("#"))) >+ doc->GetElementById(Substring(mHandler, 1, mHandler.Length() -1 ), >+ getter_AddRefs(domhandler)); This is wrong. The fragment identifier need not be present, the fragment identifier might not be an ID (it depends on the document type, e.g. it could be XPointer), the above doesn't handle xml:base and other base URI changing features, the above wouldn't handle the case where the given URI is the URI of the current document with a fragment identifier on the end, etc. It also doesn't handle the important case of having the event handler be in a completely different document, e.g. for event handler reuse. >+ if (aNameSpaceID == kNameSpaceID_XMLEvents) { >+ XMLEventsListener::InitXMLEventsListener(aDocument, this, aContent); >+ } Will this unregister event handlers on previous nodes as appropriate? >+ //Is this right, if the ID attribute is not in the empty namespace? Elements can have one ID, it shouldn't matter if that ID is from a namespaced attribute (e.g. <foo xml:id="">) or from a non namespaced one (e.g. <html:foo id="">). It wasn't completely clear from the code but I assume this section is checking to see if the element specified by observer="" is having its ID changed? >+ // Check whether we should be executing the script >+ nsCOMPtr<nsIContent> el(do_QueryInterface(aElement)); >+ if (el && el->HasAttr(kNameSpaceID_None, nsHTMLAtoms::declare)) >+ return FireErrorNotification(NS_ERROR_NOT_AVAILABLE, aElement, aObserver); This seems non-compliant, but I'm not sure. What is the "declare" attribute? >+ const char *gEventArgv[] = {"event"}; Why "event"? >+ * The Initial Developer of the Original Code is >+ * Netscape Communications Corporation. This should be you.
This is still just a test implementation, nothing to review actually. But thank you Hixie for your comments! Currently my idea is to not to support external handler (~The Basic XML Events Profile). Is that reasonable? BTW. one huge bug, load/unload events :(
The load/unload is a bug in Mozilla. Those events use wrong target.
> ------- Additional Comments From ian@hixie.ch 2004-04-25 07:18 PDT ------- > (From update of attachment 146976 [details] [diff] [review]) > This is an unsolicited code review from the perspective of someone who knows > XML Events from the QA perspective, but doesn't know much about our code base > at all. Apologies for the many misunderstandings I'm sure to have about what > your code did. > > Overall comment: You need more comments, preferably pointing to specific > sections of the XML Events spec at appropriate points to explain what is going > on and why. (I don't mean comment every line, just give the reader an overview > of what blocks of code are doing so that you don't have to be an XML Events > expert to work it out.) I know. Comment, comments... > > > >>+ void Unfinish(); > > > Could you put some comments somewhere explaining what "finished" means in this > context? It isn't clear in an initial reading. XML Events Listener is not 'finished' if it cannot be registered to any observer. For example if the observer does not exist yet or anymore. XMLEventsManager has a list of (unfinished) elements which contains XML Events declarations. I'll change the name of the function, maybe SetIncomplete > > > >>+ if (nameSpaceID == kNameSpaceID_XMLEvents) //for <xe:listener/> >>+ nameSpaceID = kNameSpaceID_None; >>+ else >>+ nameSpaceID = kNameSpaceID_XMLEvents; > > > It didn't look like you ever actually checked that it was an XML Events > "listener" element. > > The following: > > <ev:handler event=""> ... </ev:handler> > > ...shouldn't do _anything_ in this implementation, especially not be treated > as > an <ev:listener>. This is a bug or feature. Where does it read that it should not work that way? > > > >>+ if (aContent->HasAttr(nameSpaceID, nsHTMLAtoms::handler)) { >>+ aContent->GetAttr(nameSpaceID, nsHTMLAtoms::handler, handlerURI); >>+ if (handlerURI.IsEmpty()) >>+ return PR_FALSE; > > > Why do you bail if the handler attribute is empty? If the handler attribute is > empty, that means that the handler is the entire document (assuming the > baseURI > at that point is the current document). > Oh, I haven't think about having the whole document as a handler. > > >>+ if (aContent->HasAttr(nameSpaceID, nsHTMLAtoms::target)) { >>+ aContent->GetAttr(nameSpaceID, nsHTMLAtoms::target, targetIdref); >>+ if (targetIdref.IsEmpty()) >>+ return PR_FALSE; > > > Again, why bail if it is empty? As far as I can tell, if it is blank, that > means the event target must also have a blank ID (although the spec is not > overly clear about this). The ID should be "(Letter | '_' | ':') (NameChar)*" > > > >>+ if (!hasObserver) { > > ... > >>+ } >>+ else { > > ... > >>+ doc->GetElementById(observerID, getter_AddRefs(el)); > > > This won't work because the XML Events node might be added or removed before > or > after the element with the ID. Basically when "observer" is set you will have > to track all changes to IDs in the document in order to make sure you are > tracking which element you are supposed to be listening to. (As far as I can > tell from the extremely vague and underspecified spec.) This implementation is actually trying to track all the changes to the IDs. That is the reason why the XMLEventsManager is a nsIDocumentObserver. > > > >>+ if (target) { >>+ nsCOMPtr<nsIDOMEventTarget> eventTarget = ... > > > Again, the way to handle "target" is to check, whenever the event listener is > fired, whether the target actually has the correct ID, and to only run the > event handler _if_ it has that ID. This attribute should not, IMHO, have any > effect on the registration part. > You are missreading my poorly selected variable names. That target is actually observer ;) > Note in particular that the _target_ has nothing to do with the _observer_ > which is the node you want to be registering the event upon. > > > >>+ aManager->RemoveXMLEventsContent(aContent); > > > Could you explain what this does? (And why?) > If the XML Events declaration was registered to some other EventTarget, this function call removes it. I should change the function name > > >>+ if (mStopPropagation) >>+ aEvent->StopPropagation(); >>+ if (mCancelDefault) >>+ aEvent->PreventDefault(); > > > From the spec, I think this should be done _after_ the event handler code is > fired. But the spec is a bit vague. (It does make a difference since you can > tell whether the event is cancelled or not from script.) > Well, this is not specified anywhere, the script handlers are not specified at all. 3.5.Event Handlers " ... It is however recognized that two methods are likely to occur often: scripting (such as XHTML's <script> element) and declarative markup using XML elements (such as WML's <onevent> element). A companion specification will provide markup to support these methods." I haven't see that 'companion specification'. > > >>+ if (!mHandler.IsEmpty() && StringBeginsWith(mHandler, >>NS_LITERAL_STRING("#"))) >>+ doc->GetElementById(Substring(mHandler, 1, mHandler.Length() -1 ), >>+ getter_AddRefs(domhandler)); > > > This is wrong. The fragment identifier need not be present, the fragment > identifier might not be an ID (it depends on the document type, e.g. it could > be XPointer), the above doesn't handle xml:base and other base URI changing > features, the above wouldn't handle the case where the given URI is the URI of > the current document with a fragment identifier on the end, etc. > > It also doesn't handle the important case of having the event handler be in a > completely different document, e.g. for event handler reuse. this is a kind of XML Events basic, it is only possible to use handlers which are defined within the same document. (That XPointer sounds interesting.) I'd like to implement also some kind of inline script handler, maybe: <xe:listener event="click" observer="some_id" handler="javascript:doSomething();"/> > > > >>+ if (aNameSpaceID == kNameSpaceID_XMLEvents) { >>+ XMLEventsListener::InitXMLEventsListener(aDocument, this, aContent); >>+ } > > > Will this unregister event handlers on previous nodes as appropriate? Yes. > > > >>+ //Is this right, if the ID attribute is not in the empty namespace? > > > Elements can have one ID, it shouldn't matter if that ID is from a namespaced > attribute (e.g. <foo xml:id="">) or from a non namespaced one (e.g. <html:foo > id="">). > Actually that was a comment related to the nsIContent's GetIDAttributeName(). It returns only an nsIAtom, not namespace + localname. > It wasn't completely clear from the code but I assume this section is checking > to see if the element specified by observer="" is having its ID changed? Yes. > > > >>+ // Check whether we should be executing the script >>+ nsCOMPtr<nsIContent> el(do_QueryInterface(aElement)); >>+ if (el && el->HasAttr(kNameSpaceID_None, nsHTMLAtoms::declare)) >>+ return FireErrorNotification(NS_ERROR_NOT_AVAILABLE, aElement, >>aObserver); > > > This seems non-compliant, but I'm not sure. What is the "declare" attribute? From XHTML 2.0. It is needed to prevent the execution of the handler scripts during pageload. > > > >>+ const char *gEventArgv[] = {"event"}; > > > Why "event"? Also used in "HTML events", at least in Mozilla. > > > >>+ * The Initial Developer of the Original Code is >>+ * Netscape Communications Corporation. > > > This should be you. I just copy-pasted something ;)
Just wanted you guys to know about bug 238773, which's about refactoring nsDOMEvent and adding DOM 3 events. DOM 3 events use the XML events namespace, so in this regard, our goals are similar. Wonder how many conflict we'll have :)
Here is the latest patch. The load/unload events do not work due the bug 99820. Not so clean testcases can be found in http://www.cs.helsinki.fi/u/pettay/moztests/xmlevents/ It is not defined in any specification how the <script> element should work with XML Events. This patch is using syntax from X+V http://www.voicexml.org/specs/multimodal/x+v/12/index.html. Steven Pemberton: "The 'declare="declare"' trick was more a way of making HTML script elements sort-of work with XML Events, but it isn't pretty. That is why the XML Handlers spec needs to be finished to properly support <script> with XML Events." We could also leave out the <script> handlers, and add only the support for handlers which implement the nsIDOMXMLEventsHandler. This way it could be possible to start to create some kind (probably XBL based) implementation of the XForms Basic. This implementation supports dynamic attribute changes so that it is possible to add, modify and remove XML Events declaration using JavaScript/DOM. This is also a feature which is not specified anywhere.
Attachment #146976 - Attachment is obsolete: true
Attachment #151007 - Flags: review?(jst)
Comment on attachment 151007 [details] [diff] [review] First patch for review and comments. These are some things I noticed. jst may have some additional comments. >--- content/base/src/nsDocument.h 27 May 2004 22:08:36 -0000 3.233 >+++ content/base/src/nsDocument.h 10 Jun 2004 18:33:15 -0000 >@@ -562,16 +564,17 @@ > > nsSupportsHashtable* mBoxObjectTable; > > nsSupportsHashtable mContentWrapperHash; > > nsCOMPtr<nsICSSLoader> mCSSLoader; > nsRefPtr<nsHTMLStyleSheet> mAttrStyleSheet; > nsCOMPtr<nsIHTMLCSSStyleSheet> mStyleAttrStyleSheet; >+ nsCOMPtr<nsXMLEventsManager> mXMLEventsManager; You should use nsRefPtr here since this is a concrete class pointer. >--- content/base/src/nsDocument.cpp 28 May 2004 05:18:46 -0000 3.504 >+++ content/base/src/nsDocument.cpp 10 Jun 2004 18:33:22 -0000 >@@ -666,16 +666,27 @@ > } > > mNodeInfoManager = new nsNodeInfoManager(); > NS_ENSURE_TRUE(mNodeInfoManager, NS_ERROR_OUT_OF_MEMORY); > > return mNodeInfoManager->Init(this); > } > >+//XML Events >+void nsDocument::AddXMLEventsContent(nsIContent * aXMLEventsElement) { >+ if (!mXMLEventsManager) { >+ mXMLEventsManager = new nsXMLEventsManager(); >+ if (mXMLEventsManager) >+ AddObserver(mXMLEventsManager); >+ } >+ if (mXMLEventsManager) >+ mXMLEventsManager->AddXMLEventsContent(this, aXMLEventsElement); >+} >+ I'm generally not in favor of extra nsresult error returns but in this case I'd use one to propagate an allocation failure. For example: nsresult nsDocument::AddXMLEventsContent(nsIContent *aXMLEventsElement) { if (!mXMLEventsManager) { mXMLEventsManager = new nsXMLEventsManager(); NS_ENSURE_TRUE(mXMLEventsManager, NS_ERROR_OUT_OF_MEMORY); AddObserver(mXMLEventsManager); } mXMLEventsManager->AddXMLEventsContent(this, aXMLEventsElement); return NS_OK; } >--- content/base/src/nsScriptLoader.cpp 13 May 2004 18:34:16 -0000 1.61 >+++ content/base/src/nsScriptLoader.cpp 10 Jun 2004 18:33:22 -0000 >@@ -368,16 +368,21 @@ > aObserver); > } > > // Check whether we should be executing scripts at all for this document > if (!mDocument->IsScriptEnabled()) { > return FireErrorNotification(NS_ERROR_NOT_AVAILABLE, aElement, aObserver); > } > >+ // Check whether we should be executing the script >+ nsCOMPtr<nsIContent> el(do_QueryInterface(aElement)); >+ if (el && el->HasAttr(kNameSpaceID_None, nsHTMLAtoms::declare)) >+ return FireErrorNotification(NS_ERROR_NOT_AVAILABLE, aElement, aObserver); >+ Consolidate this nsIContent QI with the one down further in the function so we don't have to do it twice. Also, would it make sense to just not execute an inline <script> if there is an ev:event attribute, rather than worry about declare? >--- content/base/src/nsGenericElement.cpp 22 May 2004 22:14:57 -0000 3.335 >+++ content/base/src/nsGenericElement.cpp 10 Jun 2004 18:33:26 -0000 >@@ -3291,17 +3271,18 @@ > NS_ENSURE_SUCCESS(rv, rv); > } > > if (mDocument) { > nsCOMPtr<nsIXBLBinding> binding; > mDocument->GetBindingManager()->GetBinding(this, getter_AddRefs(binding)); > if (binding) > binding->AttributeChanged(aName, aNamespaceID, PR_FALSE, aNotify); >- >+ if (aNamespaceID == kNameSpaceID_XMLEvents || mNodeInfo->NamespaceID() == kNameSpaceID_XMLEvents) >+ mDocument->AddXMLEventsContent(this); If I'm understanding the patch correctly, this bit is for handling <listener> elements. It's not very obvious though. A couple of questions: - Would it be better to do this in nsXMLElement? - Doesn't doing this here in SetAttr cause AddXMLEventsContent to be called once for each attribute on the listener element? >--- content/html/content/src/nsGenericHTMLElement.cpp 2 Jun 2004 00:24:59 -0000 1.511 >+++ content/html/content/src/nsGenericHTMLElement.cpp 10 Jun 2004 18:33:37 -0000 >@@ -1697,17 +1697,18 @@ > if (!aOldValue.IsEmpty()) { > mutation.mPrevAttrValue = do_GetAtom(aOldValue); > } > mutation.mAttrChange = modType; > nsEventStatus status = nsEventStatus_eIgnore; > HandleDOMEvent(nsnull, &mutation, nsnull, > NS_EVENT_FLAG_INIT, &status); > } >- >+ if (aNamespaceID == kNameSpaceID_XMLEvents) >+ mDocument->AddXMLEventsContent(this); And this bit is for handling ev:* attributes on HTML elements. A couple of questions here too: - I don't see anything that says this is only valid for HTML elements; why not have this in nsGenericElement? - Won't this also fire once for each ev: attribute on the element? Is there something that prevents that from creating multiple listeners? In general, as far as adding to SetAttr, we definitely want to avoid doing more work than we need to during document construction (while still responding to dynamic setting of the attribute). One approach would be to do the work in SetAttr only if aNotify is true, and then also do it in SetParent, which will be called once the attributes are all added during load. >--- content/html/content/src/nsHTMLScriptElement.cpp 18 May 2004 20:58:11 -0000 1.75 >+++ content/html/content/src/nsHTMLScriptElement.cpp 10 Jun 2004 18:33:38 -0000 >@@ -357,17 +362,19 @@ > virtual nsresult SetInnerHTML(const nsAString& aInnerHTML); > > protected: > PRBool IsOnloadEventForWindow(); > > PRUint32 mLineNumber; > PRPackedBool mIsEvaluated; > PRPackedBool mEvaluating; >- >+ PRPackedBool mXMLEventsNeedsCompilation; Would it make sense to just unroot and null out mXMLEventsHandler when we need to recompile, rather than adding an extra variable? Are you deferring freeing it for some reason? >@@ -650,8 +678,98 @@ > > // But we'll only set mIsEvaluated if we did really load or evaluate > // something > if (HasAttr(kNameSpaceID_None, nsHTMLAtoms::src) || > mAttrsAndChildren.ChildCount()) { > mIsEvaluated = PR_TRUE; > } > } >+ >+/* void handleXMLEvents (in nsIDOMEvent event); */ >+NS_IMETHODIMP nsHTMLScriptElement::HandleXMLEvents(nsIDOMEvent *aEvent) >+{ >+ nsresult rv; Style nits: don't declare rv up here, declare it as close to where it's used as you can. Also try to follow the function declaration style in this source file, where the return type (NS_IMETHODIMP here) is on a separate line. Also, some blank lines in this function would make the code more readable (after |if| blocks, etc). >+ if (!aEvent) >+ return NS_OK; >+ /* >+ The 'this' in script is the same as the event.currentTarget. >+ */ Use C++ comments please, particularly for 1 line comments. >+ nsCOMPtr<nsIDOMEventTarget> currentTarget; >+ aEvent->GetCurrentTarget(getter_AddRefs(currentTarget)); >+ nsCOMPtr<nsIContent> targetContent; >+ if (currentTarget) { >+ nsCOMPtr<nsIDOMElement> targetElement = do_QueryInterface(currentTarget); >+ if (targetElement) >+ targetContent = do_QueryInterface(currentTarget); >+ } >+ if (!targetContent) >+ return NS_OK;; >+ nsString script; >+ GetText(script); >+ if (script.IsEmpty()) >+ return NS_OK; >+ PRUint32 lineNumber = 0; >+ GetLineNumber(&lineNumber); >+ nsIScriptContext *scriptContext = nsnull; >+ nsCOMPtr<nsIDocument> doc = GetDocument(); I don't think you need a strong reference here. >+ if (doc) { >+ nsIScriptGlobalObject *sgo = doc->GetScriptGlobalObject(); >+ if (sgo) >+ scriptContext = sgo->GetContext(); >+ } >+ if (!scriptContext) >+ return NS_OK; If this is an unexpected condition you may want to add an NS_ASSERTION/NS_ERROR. jst would know a lot more about whether the rest of this function is correct. >--- dom/public/idl/events/Makefile.in 17 Apr 2004 21:50:09 -0000 1.16 >+++ dom/public/idl/events/Makefile.in 10 Jun 2004 18:33:38 -0000 >@@ -60,11 +60,12 @@ > $(NULL) > > XPIDLSRCS = \ > nsIDOMNSEvent.idl \ > nsIDOMKeyEvent.idl \ > nsIDOMMutationEvent.idl \ > nsIDOMNSUIEvent.idl \ > nsIDOMPopupBlockedEvent.idl \ >+ nsIDOMXMLEventsHandler.idl \ We should probably not call this nsIDOMXMLEventsHandler, since it doesn't come from or extend a DOM interface. I'd suggest moving it to content/base/public and calling it just nsIXMLEventsHandler. If there's a way we could do this without adding an extra interface that would be great. >--- /dev/null 2003-01-30 12:24:37.000000000 +0200 >+++ content/events/src/nsXMLEventsManager.cpp 2004-06-10 21:25:05.000000000 +0300 >+PRBool nsXMLEventsListener::InitXMLEventsListener(nsIDocument * aDocument, >+ nsXMLEventsManager * aManager, >+ nsIContent * aContent) >+{ >+ PRInt32 nameSpaceID; >+ aContent->GetNameSpaceID(&nameSpaceID); >+ //for <xe:listener/>, right now no check for the element name I think we should check for this rather than assuming. It's pretty cheap to do so, just change to a NodeInfo Equals() check and you can check the namespace and tag at the same time. >+ if (nameSpaceID == kNameSpaceID_XMLEvents) >+ nameSpaceID = kNameSpaceID_None; >+ else >+ nameSpaceID = kNameSpaceID_XMLEvents; >+ if (aContent->HasAttr(nameSpaceID, nsHTMLAtoms::_event)) { Rather than wrapping the whole remainder of the function in an |if|, I'd just do an early return here if we don't have the attribute. Also, since we get the attribute immediately after this, just get it here... it won't cost you any more in the case where the attribute isn't there. >+ aContent->GetAttr(nameSpaceID, nsHTMLAtoms::_event, eventType); >+ if (aContent->HasAttr(nameSpaceID, nsHTMLAtoms::handler)) { Same here, just skip the HasAttr() and call GetAttr(). >+ hasHandlerURI = PR_TRUE; >+ aContent->GetAttr(nameSpaceID, nsHTMLAtoms::handler, handlerURIStr); >+ nsCAutoString handlerRef; >+ nsCOMPtr<nsIURI> handlerURI; >+ PRBool equals = PR_FALSE; >+ nsCOMPtr<nsIURI> docURI = aDocument->GetDocumentURI(); Don't need a strong reference here. >+ nsIURI *baseURI = aDocument->GetBaseURI(); >+ nsresult rv = NS_NewURI( getter_AddRefs(handlerURI), handlerURIStr, nsnull, baseURI); >+ if (NS_SUCCEEDED(rv)) { >+ nsCOMPtr<nsIURL> handlerURL(do_QueryInterface(handlerURI)); >+ if (handlerURL) { >+ handlerURL->GetRef(handlerRef); >+ handlerURL->SetRef(EmptyCString()); >+ //We support only XML Events Basic. >+ docURI->Equals(handlerURL, &equals); >+ if(equals) { Space after 'if'. >+ NS_ConvertUTF8toUTF16 handlerRefAString(handlerRef); You don't need to have this as a variable exactly, just put it as an argument: doc->GetElementById(NS_ConvertUTF8toUTF16(handlerRef), getter_AddRefs(domhandler)); The above comment about avoiding HasAttr + GetAttr applies to the whole section below this too. HasAttr() is useful for avoiding a string copy when all you care about is whether there's a value, but if you want the value anyway, just call GetAttr() and check the return code. >+ nsCOMPtr<nsIContent> observer; >+ if (!hasObserver) { >+ if (!hasHandlerURI) //Parent should be the observer >+ observer = aContent->GetParent(); >+ else //We have the handler, so this is the observer >+ observer = aContent; >+ } >+ else if (!observerID.IsEmpty()){ space before brace. >+nsXMLEventsListener::nsXMLEventsListener(nsXMLEventsManager * aManager, >+ nsIContent * aElement, >+ nsIContent * aObserver, >+ nsIContent * aHandler, >+ const nsAString& aEvent, >+ PRBool aPhase, >+ PRBool aStopPropagation, >+ PRBool aCancelDefault, >+ const nsAString& aTarget) >+{ >+ mManager = aManager; >+ mElement = aElement; >+ mObserver = aObserver; >+ mHandler = aHandler; >+ mEvent = aEvent; >+ mPhase = aPhase; >+ mStopPropagation = aStopPropagation; >+ mCancelDefault = aCancelDefault; >+ mTarget = aTarget; I generally prefer member initializers for this, i.e. const nsAString& aTarget) : mManager(aManager), mElement(aElement), ... { >+} >+ >+nsXMLEventsListener::~nsXMLEventsListener() >+{ >+ mElement = nsnull; >+ mObserver = nsnull; nsCOMPtr's do not need to be explicitly nulled out on destruction; they will release their reference when they are destroyed. Or are you avoiding a cycle of some sort here? (If that's the case, please comment on it). >+} >+ >+void nsXMLEventsListener::Unregister() >+{ >+ if (mObserver) { >+ nsCOMPtr<nsIDOMEventTarget> target = do_QueryInterface(mObserver); do_QueryInterface is null-safe; just check the result. >+NS_IMETHODIMP >+nsXMLEventsListener::HandleEvent(nsIDOMEvent* aEvent) >+{ >+ if (!aEvent) >+ return NS_OK; >+ nsCOMPtr<nsIDOMEvent> event(aEvent); >+ if (!mTarget.IsEmpty()) { >+ nsCOMPtr<nsIDOMEventTarget> target; >+ aEvent->GetTarget(getter_AddRefs(target)); >+ if (target) { >+ nsCOMPtr<nsIStyledContent> targetEl(do_QueryInterface(target)); >+ if (targetEl) { >+ nsCOMPtr<nsIAtom> id; >+ targetEl->GetID(getter_AddRefs(id)); >+ if (id) { >+ if (!id->Equals(mTarget)) Might be better to atomize mTarget when we are created? >+ return NS_OK; >+ } >+ else >+ return NS_OK; >+ } >+ else >+ return NS_OK; >+ } >+ else >+ return NS_OK;; >+ } I think you could shorten this a bit (and probably make gcc generate better code) by setting a boolean if the target matched and then checking it outside of all the nested if's. >+NS_IMPL_QUERY_INTERFACE1(nsXMLEventsManager, nsIDocumentObserver) >+NS_IMPL_ADDREF(nsXMLEventsManager) >+NS_IMPL_RELEASE(nsXMLEventsManager) you can just say: NS_IMPL_ISUPPORTS1(nsXMLEventsManager, nsIDocumentObserver) >+void >+nsXMLEventsManager::EndLoad(nsIDocument* aDocument) >+{ >+ nsCOMPtr<nsIContent> cur; This should just be a raw pointer. >+ for (int i = 0; i < mIncomplete.Count(); ++i) { >+ cur = mIncomplete[i]; >+ /*If this succeeds, the object will be removed from the mIncomplete*/ Change to C++-style comment, and this should be ".... be removed from mIncomplete".
Comment on attachment 151007 [details] [diff] [review] First patch for review and comments. This is looking good, but bryners comments need to be answerd. I'll happily review an updated patch once available, but r- for this one.
Attachment #151007 - Flags: review?(jst) → review-
Alias: XMLEvents
(responses to earlier comments in response to my earlier comments, sorry for the dramatic delay) >> The following: >> >> <ev:handler event=""> ... </ev:handler> >> >> ...shouldn't do _anything_ in this implementation, especially not be treated >> as an <ev:listener>. > > This is a bug or feature. Where does it read that it should not work that > way? The spec does not define an <ev:handler> element, so element-partition attributes on that element cannot have any official semantics. >>>+ // Check whether we should be executing the script >>>+ nsCOMPtr<nsIContent> el(do_QueryInterface(aElement)); >>>+ if (el && el->HasAttr(kNameSpaceID_None, nsHTMLAtoms::declare)) >>>+ return FireErrorNotification(NS_ERROR_NOT_AVAILABLE, aElement, >>>aObserver); >> >> This seems non-compliant, but I'm not sure. What is the "declare" attribute? > > From XHTML 2.0. It is needed to prevent the execution of the handler scripts > during pageload. XHTML2 attributes do not apply to XHTML1 content.
>>> This seems non-compliant, but I'm not sure. What is the "declare" attribute? >> >> From XHTML 2.0. It is needed to prevent the execution of the handler scripts >> during pageload. >XHTML2 attributes do not apply to XHTML1 content. You are absolutely right. Having a <script> element be a XML Event handler in the first place is a function of XHTML 2.0 (and others like SVG). XML Events itself doesn't provide for specific handlers, rather it defines how handlers can be set. What handlers can be is up to the implementing technology (XForms, XHTML 2.0, X+V, SVG, etc.) It just happens that most of the examples use <script> elements as handlers. We put in the ability for a <script> element to be a handler because that is going to be the most prevelent way that XML Events will be used. While it is XHTML 2, it can stand on its own without other XHTML 2 pieces and will bring value to the XML Events implementation which can be realized now and doesn't have to wait until X+V, SVG, etc. are implemented in Mozilla. But like Smaug said, we can easily remove the <script> handler altogether. >Also, would it make sense to just not execute an inline <script> if there is an >ev:event attribute, rather than worry about declare? we thought that we could honestly see situations where an author could have a script handler that he'd want to run during page load as well as being an event handler, so bringing along the "declare=declare" attribute was a natural step. This way the author could control whether the script ran during page load or not and we wouldn't be changing how <script> elements are processed in some undocumented manner. While using the declare attribute isn't exactly XHTML 1.x, it is consistent with the specs available from the XHTML WG in general. So I guess what we need answered is -> is it more desireable to strictly adhere to the standards and save the <script> handler until another implementation (like SVG or X+V) needs it? Or should we leave it in so that authors can use XML Events today?
(In reply to comment #29) > We put in the ability for a <script> element to be a handler because that is > going to be the most prevelent way that XML Events will be used. It seems more likely that XML events would be used in the context of XForms, with an <action> block. The main problem with declare=, as I see it, is just that it's incompatible with other UA's that would execute the script as soon as it's loaded. That's all speculation though, since I'm not aware of any other browsers that implement XML Events. I would like to suggest another way we could allow users to hook up handlers in XHTML though, separate from <script>. If the handler points to an element that implements the DOM2 EventListener interface via an XBL binding, we should invoke that listener. I like the idea of javascript: inline script handlers as well. I'd say for now we should get the core support in and then worry about <script>.
Yes I think we shouldn't support <html:script> as far as XML Events goes. There is no reason in the specs to do this, it isn't backwards compatible, and frankly I can't see any real use case; using DOM event listener registrations is easier. We should do what bryner describes, namely we should treat any node that implements EventListener as being a valid XML Events handler. On those elements it should just use that interface to handle the event. On any other element it shouldn't do anything. Then when we do things like XForms actions, we just make that element implement EventListener and it all Just Works. For testing we can either invent a <mozilla:handler> element (or a <xul:handler> element maybe) that implemnts EventListener, or use XBL to make an element that does so.
Attached patch v2 (obsolete) — — Splinter Review
Sorry this delay, my PC was broken (and now I don't have good network connections here in summer cottage :) ). Anyway I removed the <script> thing. Now the only way to use XMLEvents is to use XBL to implement DOM2 EventListener. Bryner, are you working on this too?
Attachment #151007 - Attachment is obsolete: true
Attachment #152431 - Flags: review?(bryner)
Comment on attachment 152431 [details] [diff] [review] v2 >--- ../orig/mozilla/content/html/content/src/nsGenericHTMLElement.cpp 2004-06-25 15:25:57.000000000 +0300 >+++ content/html/content/src/nsGenericHTMLElement.cpp 2004-07-05 22:46:47.000000000 +0300 >@@ -1702,18 +1702,19 @@ nsGenericHTMLElement::SetAttrAndNotify(P > if (!aOldValue.IsEmpty()) { > mutation.mPrevAttrValue = do_GetAtom(aOldValue); > } > mutation.mAttrChange = modType; > nsEventStatus status = nsEventStatus_eIgnore; > HandleDOMEvent(nsnull, &mutation, nsnull, > NS_EVENT_FLAG_INIT, &status); > } >- > if (aNotify) { >+ if (aNamespaceID == kNameSpaceID_XMLEvents) >+ mDocument->AddXMLEventsContent(this); > mDocument->AttributeChanged(this, aNamespaceID, aAttribute, modType); I still think this should be in nsGenericElement. >--- ../orig/mozilla/content/base/src/nsGenericElement.cpp 2004-06-25 15:25:57.000000000 +0300 >+++ content/base/src/nsGenericElement.cpp 2004-07-05 19:43:18.000000000 +0300 >@@ -1747,16 +1747,21 @@ nsGenericElement::SetDocument(nsIDocumen > void > nsGenericElement::SetParent(nsIContent* aParent) > { > nsIContent::SetParent(aParent); > if (aParent) { > nsIContent* bindingPar = aParent->GetBindingParent(); > if (bindingPar) > SetBindingParent(bindingPar); >+ if ((mNodeInfo->NamespaceID() != kNameSpaceID_XMLEvents && >+ this->HasAttr(kNameSpaceID_XMLEvents, nsHTMLAtoms::_event)) || >+ (mNodeInfo->NamespaceID() == kNameSpaceID_XMLEvents && >+ this->HasAttr(kNameSpaceID_None, nsHTMLAtoms::_event))) >+ mDocument->AddXMLEventsContent(this); > } > } 1. This should be in nsXMLElement 2. Null check mDocument; the parent can be set before the element is added to a document. 3. To handle the case where the parent is set before the document, you'll need to hook into SetDocument as well but be careful to only call AddXMLEventsContent once between the two methods on a normal document load. Here's what I'd suggest: void nsXMLElement::SetParent(nsIContent* aParent) { nsIContent *oldParent = GetParent(); nsGenericElement::SetParent(aParent); if (mDocument && oldParent != GetParent() && mNodeInfo->Equals(nsHTMLAtoms::listener, kNameSpaceID_XMLEvents)) mDocument->AddXMLEventsContent(this); } void nsXMLElement::SetDocument(nsIDocument* aDocument, PRBool aDeep, PRBool aCompileEventHandlers { nsIDocument *oldDocument = mDocument; nsGenericElement::SetDocument(aDocument, aDeep, aCompileEventHandlers); if (mDocument && oldDocument != mDocument && GetParent() && mNodeInfo->Equals(nsHTMLAtoms::listener, kNameSpaceID_XMLEvents)) mDocument->AddXMLEventsContent(this); } >--- ../orig/mozilla/content/events/src/Makefile.in 2004-04-18 00:52:14.000000000 +0300 >+++ content/events/src/Makefile.in 2004-07-05 14:11:43.000000000 +0300 >@@ -56,23 +56,28 @@ REQUIRES = xpcom \ > xpconnect \ > docshell \ > pref \ > view \ > necko \ > unicharutil \ > $(NULL) > >+EXPORTS = \ >+ nsXMLEventsManager.h \ >+ $(NULL) >+ Hm, this should probably not be exported (sorry I didn't catch that before). content/events/src should already be in the include path in content/base/src/Makefile.in; add it to content/xml/content/src/Makefile.in if you need to. >--- /dev/null 2003-01-30 12:24:37.000000000 +0200 >+++ content/events/src/nsXMLEventsManager.h 2004-07-05 16:53:29.000000000 +0300 >+ nsString mTarget; I would still suggest converting mTarget to an atom since it's only ever used in an atom comparison. >--- /dev/null 2003-01-30 12:24:37.000000000 +0200 >+++ content/events/src/nsXMLEventsManager.cpp 2004-07-06 19:04:48.000000000 +0300 >+ //We support only XML Events Basic. >+ docURI->Equals(handlerURL, &equals); >+ if (equals) { >+ nsCOMPtr<nsIDOMDocument> doc(do_QueryInterface(aDocument)); >+ if (doc) { >+ nsCOMPtr<nsIDOMElement> domhandler; >+ doc->GetElementById(NS_ConvertUTF8toUTF16(handlerRef), >+ getter_AddRefs(domhandler)); >+ if (domhandler) This 'if' can go away. >+ else if (!observerID.IsEmpty()) { >+ nsCOMPtr<nsIDOMDocument> doc(do_QueryInterface(aDocument)); >+ if (doc) { >+ nsCOMPtr<nsIDOMElement> el; >+ doc->GetElementById(observerID, getter_AddRefs(el)); >+ if (el) same. >+ } >+ } >+ nsCOMPtr<nsIDOMEventTarget> eventObserver; >+ if (observer) and here. >+NS_IMPL_ISUPPORTS1(nsXMLEventsListener, nsIDOMEventListener) >+NS_IMETHODIMP >+nsXMLEventsListener::HandleEvent(nsIDOMEvent* aEvent) >+{ >+ if (!aEvent) >+ return NS_OK; >+ PRBool targetMatched = PR_TRUE; >+ nsCOMPtr<nsIDOMEvent> event(aEvent); >+ if (!mTarget.IsEmpty()) { >+ nsCOMPtr<nsIDOMEventTarget> target; >+ aEvent->GetTarget(getter_AddRefs(target)); >+ if (target) { same. >+ nsCOMPtr<nsIStyledContent> targetEl(do_QueryInterface(target)); >+ if (targetEl) { >+ nsCOMPtr<nsIAtom> id; >+ targetEl->GetID(getter_AddRefs(id)); >+ if (id && !id->Equals(mTarget)) >+ targetMatched = PR_FALSE; >+ } >+ } >+ } >+ if (!targetMatched) >+ return NS_OK; >+ if (mHandler) { same. Looking pretty good, I think one more patch and this should be ready to land.
Attachment #152431 - Flags: review?(bryner) → review-
Attached patch v3 (obsolete) — — Splinter Review
I added NS_NewXMLEventsElement function. It just sets the name of the id attribute for ev:listener elements. nsXMLEventsManager.h is still exported, it is easier than changing all the places where nsDocument.h is used. The check for XML Events attributes must be in both nsGenericElement and in nsGenericHTMLElement (nsGenericHTMLElement has its own implementation for SetAttr). But I moved the check so that the element will be recognized as an XML Event declaration during parsing if it contains ev:event attribute. The dynamic change is handled by the nsIDocumentObserver.
Attachment #152431 - Attachment is obsolete: true
Attachment #152552 - Flags: review?(bryner)
Smaug: why do you have to include nsXMLEventsManager.h before the nsRefPtr member declaration in nsDocument.h? Latest advice says you should not need that. /be
Comment on attachment 152552 [details] [diff] [review] v3 Just one very minor nit: >--- ../orig/mozilla/content/xml/content/src/nsXMLElement.cpp 2004-05-23 01:14:58.000000000 +0300 >+++ content/xml/content/src/nsXMLElement.cpp 2004-07-07 23:33:01.000000000 +0300 >+void >+nsXMLElement::SetDocument(nsIDocument* aDocument, PRBool aDeep, >+ PRBool aCompileEventHandlers) Get rid of the hard tabs here. r=bryner with that fixed - no need to post a new patch. Looks good!
Attachment #152552 - Flags: review?(bryner) → review+
Attachment #152552 - Flags: superreview?(jst)
Comment on attachment 152552 [details] [diff] [review] v3 Sorry, this patch cannot handle the parent.appendChild(parent.removeChild(aChild)) testcase. New patch coming.
Attachment #152552 - Attachment is obsolete: true
Attachment #152552 - Flags: superreview?(jst)
Attached patch v4 (obsolete) — — Splinter Review
This fixes the bug in previous patch. nsXMLEventsElement is now a real class so that the nsXMLElement stays cleaner and faster.
Attachment #152776 - Flags: review?(bryner)
Attachment #152776 - Flags: superreview?(jst)
Attachment #152776 - Flags: review?(bryner)
Attachment #152776 - Flags: review+
Comment on attachment 152776 [details] [diff] [review] v4 - In content/events/src/nsXMLEventsManager.cpp: +class nsXMLEventsElement : public nsXMLElement { +public: + nsXMLEventsElement(nsINodeInfo *aNodeInfo); + virtual ~nsXMLEventsElement(); + NS_FORWARD_NSIDOMNODE_NO_CLONENODE(nsXMLElement::) + virtual nsIAtom *GetIDAttributeName() const; + virtual nsresult SetAttr(PRInt32 aNameSpaceID, nsIAtom* aName, + nsIAtom* aPrefix, const nsAString& aValue, + PRBool aNotify); +}; + +nsXMLEventsElement::nsXMLEventsElement(nsINodeInfo *aNodeInfo) + : nsXMLElement(aNodeInfo) +{ +} ... This really doesn't seem to belong in this file. Create nsXMLEventsElement.cpp and move the nsXMLEventsElement code into that file to keep a cleaner separation between the event implementation and the XML events manager. - In nsXMLEventsElement::GetIDAttributeName(): + if (mNodeInfo->NameAtom() == nsHTMLAtoms::listener) Use mNodeInfo->Equals(nsHTMLAtoms::listener) here (and in all other places where you do this same thing), and in other places too. It's inline, so no overhead in calling Equals() over getting the atom out of the nodeinfo. +nsresult +NS_NewXMLEventsElement(nsIContent** aInstancePtrResult, nsINodeInfo *aNodeInfo) { Method opening brace on its own line, like in the rest of the code you're adding here. - In nsXMLEventsListener::InitXMLEventsListener(): + PRBool hasObserver = PR_FALSE; ... + if (aContent->GetAttr(nameSpaceID, nsHTMLAtoms::observer, observerID) != + NS_CONTENT_ATTR_NOT_THERE) + hasObserver = PR_TRUE; Maybe move the declaration of hasObserver down here and simply initialize it to: + PRBool hasObserver = + aContent->GetAttr(nameSpaceID, nsHTMLAtoms::observer, observerID) != + NS_CONTENT_ATTR_NOT_THERE) + nsAutoString phase; + if (aContent->GetAttr(nameSpaceID, nsHTMLAtoms::phase, phase) != + NS_CONTENT_ATTR_NOT_THERE) { + if (phase.Equals(NS_LITERAL_STRING("capture"))) + capture = PR_TRUE; + } Same idea here, move the declaration of capture down here and intialize it to: + PRBool capture = + aContent->GetAttr(nameSpaceID, nsHTMLAtoms::phase, phase) != + NS_CONTENT_ATTR_NOT_THERE && + phase.Equals(NS_LITERAL_STRING("capture")); ... and same thing for all the other pieces of code that basically do the same as above. - In nsXMLEventsListener::Unregister(): + nsCOMPtr<nsIDOMEventTarget> target = do_QueryInterface(mObserver); + if (target) { + target->RemoveEventListener(mEvent, this, mPhase); + mObserver = nsnull; + } Don't you want to null out mObserver even if it can't be QI'ed to nsIDOMEventTarget here? I.e. move the mObserver = nsnull outside the if check. - In nsXMLEventsListener::HandleEvent(): + if (!aEvent) + return NS_OK; Is this not worthy of an exception? I.e. return NS_ERROR_INVALID_ARG or somesuch. - In nsXMLEventsManager::AttributeChanged(): + //If we are adding the ID attribute, we must check whether we can + //add new listeners + EndLoad(aDocument); This just seems wrong (even though it is correct). How about splitting out the code in EndLoad() into a method with a name that makes more sense, and calling that method from here, and from EndLoad() (and from ContentInserted() etc too)? sr=jst with that.
Attachment #152776 - Flags: superreview?(jst) → superreview+
Could someone check this in?
Attachment #152776 - Attachment is obsolete: true
Checking in...
Status: NEW → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
This patch adds nsIDocument::AddXMLEventsContent without any comment. I'm currently trying to figure out what it does and the lack of comments in this relatively-new and reviewed code is wasting time, let alone going again the style guidelines ("Use JavaDoc-style comments in any new class header files."). Can the author add useful comments to all interfaces this patch touches?
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: