164482 - (XMLEvents) Tracking: XML Events

Status: RESOLVED FIXED

(Core :: DOM: Events, enhancement)

Description

[Micah Dubinko]

"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.

Comment 1

[David Hyatt]

The XBL implementation also uses declarative events, and that code could be reused to 
implement XML events in about 2-3 hours. :)

Comment 2

[Marcus S]

Any XBL hackers out there willing to write a prototype?

Comment 3

[Alex Vincent [:WeirdAl]]

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.

Comment 4

[Andrew Hagen]

Right! Thanks.

Comment 5

[Alex Vincent [:WeirdAl]]

:) 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"...

Comment 6

[Alex Vincent [:WeirdAl]]

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.

Comment 7

[Mike Gratton]

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.

Comment 8

[Alex Vincent [:WeirdAl]]

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.

Comment 9

[Mike Gratton]

> (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.

Comment 10

[Alex Vincent [:WeirdAl]]

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.

Comment 11

[Alex Vincent [:WeirdAl]]

Bug 204961, if it's valid, may give a solution for XML Events handlers.

Comment 12

[Anne (:annevk)]

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?

Comment 13

[Hixie (not reading bugmail)]

.

Comment 14

[Brendan Eich [:brendan]]

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

Comment 15

[Hixie (not reading bugmail)]

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.

Comment 16

[aaronr]

Reassigned bug to show that we are active on the bug.  Already working on a test
implementation (native) with Smaug and Sicking.

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

Attachment: The current (test?) implementation.

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/

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

The patch does not work with XUL or SVG elements.

Comment 19

[Olli Pettay [:smaug][[e-mail]]]

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 20

[Hixie (not reading bugmail)]

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.

Comment 21

[Olli Pettay [:smaug][[e-mail]]]

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 :(

Comment 22

[Olli Pettay [:smaug][[e-mail]]]

The load/unload is a bug in Mozilla.
Those events use wrong target.

Comment 23

[Olli Pettay [:smaug][[e-mail]]]

> ------- Additional Comments From [e-mail]  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 ;)

Comment 24

[Ilya Konstantinov]

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 :)  

Comment 25

[Olli Pettay [:smaug][[e-mail]]]

Attachment: First patch for review and comments.

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.

Comment 26

[Brian Ryner (not reading)]

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 27

[Johnny Stenback (:jst)]

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.

Comment 28

[Hixie (not reading bugmail)]

(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.

Comment 29

[aaronr]

>>> 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?

Comment 30

[Brian Ryner (not reading)]

(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>.

Comment 31

[Hixie (not reading bugmail)]

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.

Comment 32

[Olli Pettay [:smaug][[e-mail]]]

Attachment: v2

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?

Comment 33

[Brian Ryner (not reading)]

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.

Comment 34

[Olli Pettay [:smaug][[e-mail]]]

Attachment: v3

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.

Comment 35

[Brendan Eich [:brendan]]

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 36

[Brian Ryner (not reading)]

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!

Comment 37

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 152552 [details] [diff] [review]
v3

Sorry, this patch cannot handle the
parent.appendChild(parent.removeChild(aChild))
testcase. New patch coming.

Comment 38

[Olli Pettay [:smaug][[e-mail]]]

Attachment: v4

This fixes the bug in previous patch. 
nsXMLEventsElement is now a real class so that the nsXMLElement
stays cleaner and faster.

Comment 39

[Johnny Stenback (:jst)]

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.

Comment 40

[Olli Pettay [:smaug][[e-mail]]]

Attachment: v5 (v4 + jst 's comments)

Could someone check this in?

Comment 41

[Johnny Stenback (:jst)]

Checking in...

Comment 42

[Ilya Konstantinov]

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?