Last Comment Bug 201236 - Mutation Events not created or dispatched for XML document that is loaded into memory but not rendered in a window/frame
: Mutation Events not created or dispatched for XML document that is loaded int...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM: Events (show other bugs)
: Trunk
: All All
: P1 major with 2 votes (vote)
: ---
Assigned To: Alex Vincent [:WeirdAl]
: Hixie (not reading bugmail)
:
Mentors:
http://bugzilla.mozilla.org/attachmen...
Depends on: 90983 234455 312522
Blocks: 331668
  Show dependency treegraph
 
Reported: 2003-04-08 15:14 PDT by delza
Modified: 2014-04-26 03:33 PDT (History)
16 users (show)
mscott: blocking1.8b5-
bzbarsky: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Test code for mutation events (3.52 KB, text/html)
2003-04-08 15:25 PDT, delza
no flags Details
XML document needed for test case (50 bytes, text/xml)
2003-04-22 05:45 PDT, Martin Honnen
no flags Details
test case which compares mutation event handling on a document loaded into an iframe with a document loaded into memory (1.92 KB, text/html)
2003-04-22 05:52 PDT, Martin Honnen
no flags Details
patch (work in progress) (11.60 KB, patch)
2005-09-02 10:51 PDT, Alex Vincent [:WeirdAl]
no flags Details | Diff | Splinter Review
patch, v1 (11.86 KB, patch)
2005-09-02 13:13 PDT, Alex Vincent [:WeirdAl]
caillon: review-
Details | Diff | Splinter Review
patch, v1 for 1.8 branch (11.90 KB, patch)
2005-09-02 14:05 PDT, Alex Vincent [:WeirdAl]
no flags Details | Diff | Splinter Review
patch, v2 (21.17 KB, patch)
2005-09-26 19:53 PDT, Alex Vincent [:WeirdAl]
no flags Details | Diff | Splinter Review
patch, v2.1 (22.00 KB, patch)
2005-09-27 08:17 PDT, Alex Vincent [:WeirdAl]
no flags Details | Diff | Splinter Review
patch, v2.2 (22.83 KB, patch)
2005-09-27 08:24 PDT, Alex Vincent [:WeirdAl]
no flags Details | Diff | Splinter Review
patch, v3 (33.63 KB, patch)
2005-10-01 14:08 PDT, Alex Vincent [:WeirdAl]
no flags Details | Diff | Splinter Review
patch for SVG (missed in previous patch) (3.15 KB, patch)
2005-10-02 10:56 PDT, Alex Vincent [:WeirdAl]
no flags Details | Diff | Splinter Review
patch, v3.1 (37.65 KB, patch)
2005-10-06 23:01 PDT, Alex Vincent [:WeirdAl]
peterv: review-
Details | Diff | Splinter Review
patch (not for review) (43.34 KB, patch)
2005-10-14 19:16 PDT, Alex Vincent [:WeirdAl]
no flags Details | Diff | Splinter Review
patch, v4 (41.08 KB, patch)
2006-01-13 23:59 PST, Alex Vincent [:WeirdAl]
no flags Details | Diff | Splinter Review
testcase using createDocument, testing document.appendChild, element.setAttribute (1.29 KB, application/xml)
2006-03-09 22:45 PST, Alex Vincent [:WeirdAl]
no flags Details
patch, v5 (26.48 KB, patch)
2006-03-09 22:48 PST, Alex Vincent [:WeirdAl]
no flags Details | Diff | Splinter Review
patch, v5.1 (23.97 KB, patch)
2006-03-09 23:53 PST, Alex Vincent [:WeirdAl]
no flags Details | Diff | Splinter Review
new test case necessary as bugzilla now uses HTTPS (2.15 KB, text/html)
2006-03-10 09:06 PST, Martin Honnen
no flags Details
patch, v5.2 (23.78 KB, patch)
2006-03-10 14:19 PST, Alex Vincent [:WeirdAl]
bugs: review-
Details | Diff | Splinter Review
patch, v5.3 (23.97 KB, patch)
2006-03-14 15:05 PST, Alex Vincent [:WeirdAl]
bugs: review+
peterv: superreview-
Details | Diff | Splinter Review
patch, v5.4 (21.82 KB, patch)
2006-03-24 19:27 PST, Alex Vincent [:WeirdAl]
peterv: superreview-
Details | Diff | Splinter Review
patch, v5.5 (20.83 KB, patch)
2006-03-30 12:57 PST, Alex Vincent [:WeirdAl]
peterv: superreview+
Details | Diff | Splinter Review
Final patch (23.50 KB, patch)
2006-04-24 11:57 PDT, Alex Vincent [:WeirdAl]
no flags Details | Diff | Splinter Review

Description delza 2003-04-08 15:14:10 PDT
User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.3) Gecko/20030312
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.3) Gecko/20030312

When I create a new XML document in Javascript, which has no associated window
for display, Document.createEvent("MutationEvents") returns null,
Node.dispatchEvent(evt) (with an event created from HTML page document) does not
trigger handlers, and actually mutating the page via DOM methods does not
trigger handlers.

I can understand not firing UI events for "headless" (no UI) XML documents, but
Mutation events should still be fired.  It's a model/view thing.

Reproducible: Always

Steps to Reproduce:
1.  Create a new document: document.implementation.createDocument('','',null)
2.  Subscribe to any mutation event
3.  Call DOM methods to modify document

Actual Results:  
Event handlers are never fired.

Expected Results:  
I should get events in the handlers.  I've tested the exact same code by
subscribing to mutation events on the (visible) HTML DOM and they fire just fine.
Comment 1 delza 2003-04-08 15:25:27 PDT
Created attachment 119875 [details]
Test code for mutation events

Just some sample code to demonstrate what I'm trying to do.
Comment 2 Greg K. 2003-04-09 10:14:25 PDT
delza@alliances.org, what should I see on loading that testcase?
Comment 3 delza 2003-04-09 11:04:15 PDT
The test case creates a new XML document, subscribes to all mutation events on
that document (the handler simply writes events as new itesm of the UL list or
as alerts, depending on configuration), then modifies the document by creating a
new node and inserting it.  Finally, the XML of the resulting document is
printed in the UL to confirm the mutations.  So what you *should* see is events
being printed out.  What you'll actually see is nothing, which makes it kind of
a anticlimatic test case.

If you have suggestions for a better test case, I'll try to do that.
Comment 4 Martin Honnen 2003-04-22 05:45:44 PDT
Created attachment 121269 [details]
XML document needed for test case
Comment 5 Martin Honnen 2003-04-22 05:52:26 PDT
Created attachment 121271 [details]
test case which compares mutation event handling on a document loaded into an iframe with a document loaded into memory

This test case loads an example XML file
  http://bugzilla.mozilla.org/attachment.cgi?id=121269&action=view
into an iframe inside the test page and into memory by using XMLHttpRequest.
A mutation event listener is added and then a node is inserted. For the XML
document loaded into the iframe the mutation event listener fires while for the
XML document loaded into memory the event listener is not fired.
The debug output shows that for both documents the node is inserted thus the
mutation event listener should fire in both cases.
Comment 6 Martin Honnen 2003-04-22 06:02:24 PDT
As I find the same problem like the reporter I confirm the bug. Also changed
summary to describe the problem more precisely.
I have tested with Mozilla 1.4a (Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.4a)
Gecko/20030401) but the problem also occurs with Netscape 7.02 (Mozilla/5.0
(Windows; U; Win98; en-US; rv:1.0.2) Gecko/20030208 Netscape/7.02) thus this is
not a regression.
Comment 7 delza 2003-05-02 14:43:30 PDT
So there are two basic ways to get a new XML document (that I'm aware of, modulo
some variation) in Mozilla: DOMImplementation.createDocument() and creating a
new iFrame element and loading the document into it.

In the case of createDocument, I do not receive mutateEvents when the document
is changed, or when I create a mutateEvent and call dispatchEvent (note that I
can subscribe to the events and call dispatchEvent with no errors, but it
silently fails).  On the other hand, I can add new properties using
Node.prototype, set the properties, and view the properties when I retrieve the
node from the DOM tree.

In the case of the iFrame document, I can receive mutateEvents whether they
originate from actual changes or dispatchEvent, and I can set properties on an
individual node and retrieve them as long as I keep a pointer to the node, but
when I retrieve the node from the DOM tree the property no longer available. 
Properties added via Node.prototype are not visible on nodes retrieved from the
tree (including if I add the property to the iFrame's
contentWindow.Node.prototype rather than just Node.prototype).

I have some theories, thanks to Martin, but it would be nice to hear from
someone on the Mozilla team about the real reason behind this inconsistent
behaviour and any workaround, or fixability and timeframe.

For what it's worth, here are my theories.  I think that the event dispatching
behaviour is dependent on being tied to a window to manage the events, but that
XML documents created with createDocument are not (and cannot be) associated
with a window.

I think that the modifications to Node.prototype are also local to the window
and are not carried through to the iFrame.contentWindow.  I don't have any
explanation for why adding a property to iFrame.contentWindow.Node.prototype
doesn't work, or why setting a property on an individual node works, but
disappears when the node is retrieved again from the DOM.
Comment 8 Boris Zbarsky [:bz] (still a bit busy) 2004-02-14 14:21:46 PST
I'm seeing several problems here:

1)  The document node does not fire mutation events when a child is appended to
    it.  Doesn't matter whether the document is shown.
2)  Mutation events are not fired for normal DOM modifications for any document
    without a window (since HasMutationListeners returns false).
3)  nsDocument::DispatchEvent will just bail early if it has no presshells.  So
    will nsEventListenerManager::DispatchEvent, if the document has no
    presshells.

We should probably fix all three....
Comment 9 Johnny Stenback (:jst, jst@mozilla.com) 2004-02-16 14:24:22 PST
Yeah, we absolutely should...
Comment 10 Alex Vincent [:WeirdAl] 2005-09-01 20:08:37 PDT
Oh, man.  This bug has just absolutely stopped work on my Verbosio project.  I
am dead without this.  This gives me one hell of an incentive to fix.
Comment 11 Alex Vincent [:WeirdAl] 2005-09-01 20:25:17 PDT
Accepting bug.  Patch coming as soon as I have something.
Comment 12 Alex Vincent [:WeirdAl] 2005-09-02 10:51:24 PDT
Created attachment 194676 [details] [diff] [review]
patch (work in progress)

This patch fixes the issue with regard to document nodes appending child nodes,
but with a window still existing for the document.  I'm still working on
this...
Comment 13 Alex Vincent [:WeirdAl] 2005-09-02 11:08:06 PDT
:( I need an event stage manager.  It does lots of good checks (including
checking security state, whether the event's been initialized, etc.).
Comment 14 Boris Zbarsky [:bz] (still a bit busy) 2005-09-02 11:27:57 PDT
> :( I need an event stage manager. 

Why?  Whatever functionality is in the ESM that's not presentation-dependent
should probably move out of the ESM...
Comment 15 Alex Vincent [:WeirdAl] 2005-09-02 11:35:57 PDT
(In reply to comment #14)
> > :( I need an event stage manager. 
> 
> Why?  Whatever functionality is in the ESM that's not presentation-dependent
> should probably move out of the ESM...

I made a wrong guess; nsGenericElement doesn't use it for mutation events:
http://lxr.mozilla.org/seamonkey/source/content/base/src/nsGenericElement.cpp#2717
Comment 16 Alex Vincent [:WeirdAl] 2005-09-02 13:13:39 PDT
Created attachment 194692 [details] [diff] [review]
patch, v1

* Adds a check function for mutation listeners to nsDocument
* Adds dispatching of mutation events to nsDocument, targeted at inserted or
removed nodes
* Changes nsGenericElement's HasMutationListeners to allow for a document
missing a global script object and window
* Adds bubbling of DOM events in general to nsGenericDOMDataNode.
Comment 17 Alex Vincent [:WeirdAl] 2005-09-02 13:15:52 PDT
Requesting blocking1.8b5 to try to get this in for the 1.8 milestone.
Comment 18 Alex Vincent [:WeirdAl] 2005-09-02 14:05:05 PDT
Created attachment 194700 [details] [diff] [review]
patch, v1 for 1.8 branch
Comment 19 Scott MacGregor 2005-09-13 09:41:48 PDT
It's really too late in the release cycle for this change. Furthermore I don't
see any justification or risk analysis as to why we should consider this change
for this release. 

Not a regression over 1.0. Minusing this for now.
Comment 20 Alex Vincent [:WeirdAl] 2005-09-17 19:13:28 PDT
cc'ing caillon; should I start work on a new patch based on your comments over
IRC, or wait for the full review?
Comment 21 Alex Vincent [:WeirdAl] 2005-09-17 19:15:44 PDT
Comment on attachment 194700 [details] [diff] [review]
patch, v1 for 1.8 branch

Dropping support for 1.8, per comment 19's first sentence.
Comment 22 Alex Vincent [:WeirdAl] 2005-09-20 19:57:15 PDT
Comment on attachment 194692 [details] [diff] [review]
patch, v1

back to peterv for the review request.
Comment 23 Christopher Aillon (sabbatical, not receiving bugmail) 2005-09-26 08:02:26 PDT
Comment on attachment 194692 [details] [diff] [review]
patch, v1

Your static method should be placed on nsContentUtils, and all callers (from
nsDocument and nsGenericElement) should be updated.


>-  if (NS_EVENT_FLAG_BUBBLE & aFlags && parent) {
>-    ret = parent->HandleDOMEvent(aPresContext, aEvent, aDOMEvent,
>-                                 aFlags & NS_EVENT_BUBBLE_MASK, aEventStatus);
>+  if (NS_EVENT_FLAG_BUBBLE & aFlags) {
>+    //Initiate bubbling phase.  Special case first call to document
>+    if (parent) {
>+      ret = parent->HandleDOMEvent(aPresContext, aEvent, aDOMEvent,
>+                                   aFlags & NS_EVENT_BUBBLE_MASK, aEventStatus);
>+    } else {
>+      if (document) {
>+        document->HandleDOMEvent(aPresContext, aEvent, aDOMEvent,
>+                                 aFlags & NS_EVENT_BUBBLE_MASK,
>+                                 aEventStatus);

Don't you want to assign the return of HandleDOMEvent() to |ret| here?
Comment 24 Alex Vincent [:WeirdAl] 2005-09-26 17:37:59 PDT
> Don't you want to assign the return of HandleDOMEvent() to |ret| here?

Interesting question, in that nsGenericElement and nsGenericDOMDataNode do
different things in the capturing and bubbling phases.  

nsGenericElement capturing:  ret assigned for document, not parent
nsGenericElement bubbling:   ret assigned for document and parent
nsGenericDOMDataNode capturing: ret not assigned for document or parent
nsGenericDOMDataNode bubbling: ret not assigned for parent.  My patch changed
that (probably incorrectly).

For nsDocument, there was never a ret value checked.  In none of the cases where
ret was assigned did anyone call NS_FAILED(ret) or NS_ENSURE_SUCCESS, so I don't
know what the intent is.

This is pretty inconsistent; I'd like to have someone clarify for me what the
code should be doing in all these cases.
Comment 25 Alex Vincent [:WeirdAl] 2005-09-26 17:47:24 PDT
sicking says always assign, always check with NS_ENSURE_SUCCESS.
Comment 26 Alex Vincent [:WeirdAl] 2005-09-26 19:53:53 PDT
Created attachment 197502 [details] [diff] [review]
patch, v2

fixing caillon's review nits in comment 23, adding NS_ENSURE_SUCCESS per
comments 24, 25
Comment 27 Jonas Sicking (:sicking) No longer reading bugmail consistently 2005-09-27 07:03:37 PDT
Don't put the NS_ENSURE_SUCCESS after the |if|. Always use the following pattern:

...
    rv = foo->bar()
    NS_ENSURE_SUCCESS(rv, rv);
...

return NS_OK;


Or possibly end with a |return baz->bin();|

Never end with |return rv;|
Comment 28 Alex Vincent [:WeirdAl] 2005-09-27 08:17:05 PDT
Created attachment 197557 [details] [diff] [review]
patch, v2.1

Fixing sicking's nit, though I do have a question.  This change means
NS_ENSURE_SUCCESS comes before aEvent->flags is reset to its original value for
the local handling stage of each event.  Could that be troublesome?
Comment 29 Alex Vincent [:WeirdAl] 2005-09-27 08:19:34 PDT
Comment on attachment 197557 [details] [diff] [review]
patch, v2.1

Whoops.  Missed something.
Comment 30 Alex Vincent [:WeirdAl] 2005-09-27 08:24:02 PDT
Created attachment 197559 [details] [diff] [review]
patch, v2.2
Comment 31 Jonas Sicking (:sicking) No longer reading bugmail consistently 2005-09-28 11:03:30 PDT
That sounds like it could be a problem, though I'd have to look more in detail
at the code.

In this instance:

+    ret = mListenerManager->HandleEvent(aPresContext, aEvent, aDOMEvent, this,
+                                        aFlags, aEventStatus);
+    NS_ENSURE_SUCCESS(ret, ret);
     aEvent->flags &= ~aFlags;

you could just move the ENSURE one row down.
Comment 32 Alex Vincent [:WeirdAl] 2005-09-28 12:14:11 PDT
I'll not submit a new patch yet, pending the actual review.
Comment 33 Alex Vincent [:WeirdAl] 2005-09-30 20:32:49 PDT
Comment on attachment 197559 [details] [diff] [review]
patch, v2.2

bz's checkin for bug 278472 causes this patch to bitrot.  I'll re-examine and
submit a new patch.
Comment 34 Alex Vincent [:WeirdAl] 2005-09-30 23:42:43 PDT
I'll be using attachment 198110 [details] as a testcase for this bug.
Comment 35 Alex Vincent [:WeirdAl] 2005-10-01 14:08:23 PDT
Created attachment 198166 [details] [diff] [review]
patch, v3

This patch moves HasMutationListeners into nsContentUtils, and rewrites it to
work with documents.
Comment 36 Alex Vincent [:WeirdAl] 2005-10-02 10:56:11 PDT
Created attachment 198236 [details] [diff] [review]
patch for SVG (missed in previous patch)

Please consider this an addition to attachment 198166 [details] [diff] [review].
Comment 37 Alex Vincent [:WeirdAl] 2005-10-06 22:27:25 PDT
Comment on attachment 198236 [details] [diff] [review]
patch for SVG (missed in previous patch)

curses, bitrotted again.
Comment 38 Alex Vincent [:WeirdAl] 2005-10-06 23:01:19 PDT
Created attachment 198779 [details] [diff] [review]
patch, v3.1

fixing conflict, combining with SVG patch.
Comment 39 Peter Van der Beken [:peterv] 2005-10-10 08:39:39 PDT
Comment on attachment 198779 [details] [diff] [review]
patch, v3.1

>Index: mozilla/content/base/public/nsContentUtils.h
>===================================================================

>+   * Quick helper to determine whether there are any mutation listeners
>+   * of a given type that apply to the node or window passed in.

Given that this takes windows, I think it shouldn't be name *Node*Has...

>+   * @param aType the type of listener (NS_EVENT_BITS_MUTATION_*)

There's only an aNode argument.

>+   * @return whether there are mutation listeners of the specified type for

The type isn't specified.

>+  static PRBool NodeHasMutationListeners(nsISupports* aNode);

This function doesn't seem to be used outside nsContentUtils, it should be file
static in nsContentUtils.cpp, don't put it in the header.

>+   * Quick helper to determine whether there are any mutation listeners
>+   * of a given type that apply to this content (are at or above it).
>+   * @param aContent the content of the node to search for listeners

s/the content of//

>Index: mozilla/content/base/src/nsContentUtils.cpp
>===================================================================

>+/* static */
>+PRBool
>+nsContentUtils::NodeHasMutationListeners(nsISupports* aNode)
>+{
>+  nsCOMPtr<nsIDOMEventReceiver> rec(do_QueryInterface(aNode));
>+  if (rec) {
>+    nsCOMPtr<nsIEventListenerManager> manager;
>+    rec->GetListenerManager(getter_AddRefs(manager));
>+    if (manager) {
>+      PRBool hasMutationListeners = PR_FALSE;
>+      manager->HasMutationListeners(&hasMutationListeners);
>+      if (hasMutationListeners)
>+        return PR_TRUE;

Just do |return hasMutationListeners;|

>+/* static */
>+PRBool
>+nsContentUtils::HasMutationListeners(nsIContent* aContent,
>+                                     nsIDocument* aDocument,
>+                                     PRUint32 aType)
>+{
>+  if (aContent) {
>+    if (aContent->GetCurrentDoc() == nsnull)
>+      return PR_FALSE;
>+    NS_PRECONDITION(aContent->GetCurrentDoc() == aDocument, 
>+                    "Document must be owner document of content!");
>+  } else 
>+  if (!aDocument)
>+    return PR_FALSE;

  NS_PRECONDITION(aContent || aDocument, "Must have document if no content!");
  NS_PRECONDITION(!aContent || !aContent->IsInDoc() ||
		  aContent->GetCurrentDoc() == aDocument,
		  "Incorrect aDocument");

  if ((aContent && !aContent->IsInDoc()) || !aDocument)
    return PR_FALSE;

>+    if (window && (!window->HasMutationListeners(aType)))

Drop the braces around !window->HasMutationListeners(aType)

>+  for (nsIContent* curr = aContent; curr; curr = curr->GetParent())
>+    if (NodeHasMutationListeners(curr))
>+      return PR_TRUE;

  nsIContent* curr;
  for (curr = aContent; curr; curr = curr->GetParent()) {
    if (NodeHasMutationListeners(curr))
      return PR_TRUE;
  }

>+  if (NodeHasMutationListeners(aDocument))
>+    return PR_TRUE;
>+
>+  return (window && NodeHasMutationListeners(window));

You could keep the old code:

  return NodeHasMutationListeners(aDocument) ||
	 NodeHasMutationListeners(window);

>Index: mozilla/content/base/src/nsDocument.cpp
>===================================================================

>@@ -3579,16 +3582,28 @@ nsDocument::RemoveChild(nsIDOMNode* aOld
>     return NS_ERROR_DOM_HIERARCHY_REQUEST_ERR;
>   }
> 
>   PRInt32 indx = mChildren.IndexOfChild(content);
>   if (indx == -1) {
>     return NS_ERROR_DOM_NOT_FOUND_ERR;
>   }
> 
>+  if (nsContentUtils::HasMutationListeners(nsnull,
>+        this,
>+        NS_EVENT_BITS_MUTATION_NODEREMOVED)) {
>+    nsCOMPtr<nsIDOMEventTarget> aMutationOldTarget(do_QueryInterface(aOldChild));
>+    nsMutationEvent mutationOld(PR_TRUE, NS_MUTATION_NODEREMOVED, aMutationOldTarget);
>+    nsIDOMNode* relNode = this;
>+    mutationOld.mRelatedNode = relNode;
>+
>+    nsEventStatus status = nsEventStatus_eIgnore;
>+    content->HandleDOMEvent(nsnull, &mutationOld, nsnull, NS_EVENT_FLAG_INIT, &status);
>+  }
>+
>   ContentRemoved(nsnull, content, indx);
> 
>   mChildren.RemoveChildAt(indx);
>   if (content == mRootContent) {
>     DestroyLinkMap();
>     mRootContent = nsnull;
>   }
> 

Instead of adding that code, please add nsGenericElement::doRemoveChild,
something like:

/* static */
nsresult
nsGenericElement::doRemoveChild(nsIDOMNode *aOldChild, nsIContent* aParent,
				nsIDocument* aDocument,
				nsAttrAndChildArray& aChildArray,
				nsIDOMNode** aReturn)
{
  NS_PRECONDITION(aParent || aDocument, "Must have document if no parent!");
  NS_PRECONDITION(!aParent || aParent->GetCurrentDoc() == aDocument,
		  "Incorrect aDocument");

  *aReturn = nsnull;

  if (!aOldChild) {
    return NS_ERROR_NULL_POINTER;
  }

  nsCOMPtr<nsIContent> content(do_QueryInterface(aOldChild));
  if (!content) {
    /*
     * If we're asked to remove something that doesn't support nsIContent
     * it can not be one of our children, i.e. we return NOT_FOUND_ERR.
     */
    return NS_ERROR_DOM_NOT_FOUND_ERR;
  }

  nsContentOrDocument container(aParent, aDocument);
  PRInt32 pos = container.IndexOf(content);

  if (pos < 0) {
    /*
     * aOldChild isn't one of our children.
     */
    return NS_ERROR_DOM_NOT_FOUND_ERR;
  }

  nsresult rv = container.RemoveChildAt(pos, PR_TRUE, aChildArray);

  *aReturn = aOldChild;
  NS_ADDREF(aOldChild);

  return rv;
}

Call nsGenericElement::doRemoveChild from nsDocument::RemoveChild and
nsGenericElement::RemoveChild and I think that gives the same result with less
code.

>Index: mozilla/content/base/src/nsGenericDOMDataNode.cpp
>===================================================================

>@@ -793,54 +793,67 @@ nsGenericDOMDataNode::HandleDOMEvent(nsP

>   //Bubbling stage
>-  if (NS_EVENT_FLAG_BUBBLE & aFlags && parent) {
>-    ret = parent->HandleDOMEvent(aPresContext, aEvent, aDOMEvent,
>-                                 aFlags & NS_EVENT_BUBBLE_MASK, aEventStatus);
>+  if (NS_EVENT_FLAG_BUBBLE & aFlags) {
>+    //Initiate bubbling phase.  Special case first call to document

What does |Special case first call to document| mean?

>+    if (parent) {
>+      ret = parent->HandleDOMEvent(aPresContext, aEvent, aDOMEvent,
>+                                   aFlags & NS_EVENT_BUBBLE_MASK, aEventStatus);
>+      NS_ENSURE_SUCCESS(ret, ret);
>+    } else {
>+      if (document) {

    }
    else if (document) {

>Index: mozilla/content/html/content/src/nsGenericHTMLElement.cpp
>===================================================================

>@@ -1674,17 +1674,19 @@ nsGenericHTMLElement::SetAttr(PRInt32 aN

>   if (IsInDoc()) {
>-    hasListeners = nsGenericElement::HasMutationListeners(this,
>+    nsIDocument* document = GetCurrentDoc();
>+    hasListeners = nsContentUtils::HasMutationListeners(this,
>+      document,

Make that

  nsIDocument* document = GetCurrentDoc();
  if (document) {
    hasListeners = nsContentUtils::HasMutationListeners(this,
      document,

>@@ -1984,17 +1986,19 @@ nsresult

>   if (IsInDoc()) {
>-    hasListeners = nsGenericElement::HasMutationListeners(this,
>+    nsIDocument* document = GetCurrentDoc();
>+    hasListeners = nsContentUtils::HasMutationListeners(this,
>+      document,

Same here.

>Index: mozilla/content/svg/content/src/nsSVGElement.cpp
>===================================================================

>   if (IsInDoc()) {
>-    hasListeners = nsGenericElement::HasMutationListeners(this,
>+    nsIDocument* document = GetCurrentDoc();
>+    hasListeners = nsContentUtils::HasMutationListeners(this,
>+      document,

Same here.

>@@ -286,17 +288,19 @@ nsSVGElement::WalkContentStyleRules(nsRu

>   if (IsInDoc()) {
>-    hasListeners = nsGenericElement::HasMutationListeners(this,
>+    nsIDocument* document = GetCurrentDoc();
>+    hasListeners = nsContentUtils::HasMutationListeners(this,
>+      document,

Same here.

>@@ -573,17 +577,19 @@ nsSVGElement::DidModifySVGObservable(nsI

>   if (IsInDoc()) {
>     modification = !!mAttrsAndChildren.GetAttr(attrName->LocalName(),
>                                                attrName->NamespaceID());
>-    hasListeners = nsGenericElement::HasMutationListeners(this,
>+    nsIDocument* document = GetCurrentDoc();
>+    hasListeners = nsContentUtils::HasMutationListeners(this,
>+      document,

Same here.

>Index: mozilla/content/xul/content/src/nsXULElement.cpp
>===================================================================

>@@ -1050,17 +1050,19 @@ nsXULElement::RemoveChildAt(PRUint32 aIn

>+    if (nsContentUtils::HasMutationListeners(this,
>+                                             doc,
>+                                             NS_EVENT_BITS_MUTATION_NODEREMOVED)) {

Long line.

>@@ -1211,17 +1213,19 @@ nsXULElement::SetAttr(PRInt32 aNamespace

>     if (IsInDoc()) {
>         PRBool isAccessKey = aName == nsXULAtoms::accesskey &&
>                              aNamespaceID == kNameSpaceID_None;
>-        hasListeners = nsGenericElement::HasMutationListeners(this,
>+        nsIDocument* document = GetCurrentDoc();
>+        hasListeners = nsContentUtils::HasMutationListeners(this,
>+            document,

Again, replace |if (IsInDoc()) {| with |if (document) {|

>@@ -1510,17 +1514,19 @@ nsXULElement::UnsetAttr(PRInt32 aNameSpa

>     PRBool hasMutationListeners =
>-        HasMutationListeners(this, NS_EVENT_BITS_MUTATION_ATTRMODIFIED);
>+        nsContentUtils::HasMutationListeners(this,
>+                                             doc,
>+                                             NS_EVENT_BITS_MUTATION_ATTRMODIFIED);

Long line.

>@@ -2320,17 +2326,19 @@ nsXULElement::GetInlineStyleRule()

>     if (IsInDoc()) {
>-        hasListeners = nsGenericElement::HasMutationListeners(this,
>+        nsIDocument* document = GetCurrentDoc();
>+        hasListeners = nsContentUtils::HasMutationListeners(this,
>+            document,

Again, replace |if (IsInDoc()) {| with |if (document) {|
Comment 40 Alex Vincent [:WeirdAl] 2005-10-10 09:17:27 PDT
Thanks, peterv.  I'll get a new patch ready tonight or tomorrow for re-review.
Comment 41 Alex Vincent [:WeirdAl] 2005-10-10 09:38:07 PDT
(In reply to comment #39)
> >@@ -3579,16 +3582,28 @@ nsDocument::RemoveChild(nsIDOMNode* aOld
> Instead of adding that code, please add nsGenericElement::doRemoveChild,
...
> Call nsGenericElement::doRemoveChild from nsDocument::RemoveChild and
> nsGenericElement::RemoveChild and I think that gives the same result with less
> code.

Actually, I did think of that, but bz didn't do that in the patch for bug
278472.  Given that he did do that for ReplaceChild and InsertBefore, I figured
there must have been a good reason he didn't do so.  bz?

That said, I can still do it.
Comment 42 Boris Zbarsky [:bz] (still a bit busy) 2005-10-10 10:48:37 PDT
> I figured there must have been a good reason he didn't do so.  bz?

Yeah, the reason was that I wanted as small a patch as I could make, since I
needed to land it on branch too.  Nothing was a priori broken about RemoveChild
on documents, so I left it alone.
Comment 43 Alex Vincent [:WeirdAl] 2005-10-14 19:16:49 PDT
Created attachment 199621 [details] [diff] [review]
patch (not for review)

I'm filing this patch as a response to peterv's r- commentary in comment 39. 
However, this patch does not cover the doRemoveChild changes peterv requested,
which is why I am not requesting review at this time.

I filed bug 312522 for the doRemoveChild changes, which I no longer feel
comfortable doing on my own.
Comment 44 Alex Vincent [:WeirdAl] 2006-01-13 23:59:13 PST
Created attachment 208460 [details] [diff] [review]
patch, v4

In writing this latest version of the patch, I noticed that the rv checking is a bit interesting at the end points.

At the side of nsEventListenerManager->HandleEvent(), it appears to always return NS_OK.  So any failures that happen are absorbed.

For capturing/bubbling at nsGlobalWindow, we just aren't checking any return values on mChromeEventHandler.  Similarly (though I suppose it doesn't matter for this patch), local event handling on nsGlobalWindow doesn't check rv values.  So again, we're returning NS_OK.

This patch passes the returned values through NS_ENSURE_SUCCESS() calls, which happily let things be.  After we get through with HandleDOMEvent() at each level, the level returns NS_OK.

On the receiving end (the functions which call HandleDOMEvent in the first place), a lot of HTML elements do use NS_ENSURE_SUCCESS calls to double-check that things were done correctly.
http://lxr.mozilla.org/seamonkey/search?string=%3A%3AHandleDOMEvent

Add all of the above items up, and I'm a bit concerned.  The extra NS_ENSURE_SUCCESS calls are good, but we could still have failures we're supposed to acknowledge, and we don't.

What do you guys think?  Should these concerns affect the patch?  At the very least, I think we might need some HTML testcases.
Comment 45 Alex Vincent [:WeirdAl] 2006-01-14 00:17:37 PST
(In reply to comment #44)
> Created an attachment (id=208460) [edit]
> patch, v4

Note to self:  anywhere we have IsInDoc(), we also have nsIDocument *document = GetCurrentDoc().  So a few if (document) or if (IsInDoc()) calls are probably redundant.
Comment 46 Alex Vincent [:WeirdAl] 2006-01-18 20:32:29 PST
Comment on attachment 208460 [details] [diff] [review]
patch, v4

bitrotted for the third time, thanks to bug 323311
Comment 47 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2006-01-30 10:47:40 PST
Comment on attachment 208460 [details] [diff] [review]
patch, v4

Just some comments.

>+PRBool
>+ObjectHasMutationListeners(nsISupports* aNode)
>+{
>+  nsCOMPtr<nsIDOMEventReceiver> rec(do_QueryInterface(aNode));
>+  if (rec) {
>+    nsCOMPtr<nsIEventListenerManager> manager;
>+    rec->GetListenerManager(getter_AddRefs(manager));
>+    if (manager) {
>+      PRBool hasMutationListeners = PR_FALSE;
>+      manager->HasMutationListeners(&hasMutationListeners);
>+      return hasMutationListeners;
>+    }
>+  }

I know this code was there already before, but EventListenerManager really shouldn't be
created just to check whether there are mutation listeners. And if aNode
is an nsGenericElement, things are even worse. nsDOMEventRTTearoff is created first to
get the nsIDOMEventReceiver and then nsIEventListenerManager is created to
get the result PR_FALSE from the HadMutationListeners() - that is the normal case I think.
If/when bug 234455 gets fixed, one can ask to get the event listener manager only
if one already exists, and that can be got from nsINode, no need for nsIDOMEventReceiver.


>+PRBool
>+nsContentUtils::HasMutationListeners(nsIContent* aContent,
>+                                     nsIDocument* aDocument,
>+                                     PRUint32 aType)
>+{
>+  // If we don't have a document, we can't dispatch any mutation events anyway.

Why?

>+  if (!aDocument)
>+    return PR_FALSE;
Comment 48 Alex Vincent [:WeirdAl] 2006-01-30 11:03:35 PST
(In reply to comment #47)

> >+  // If we don't have a document, we can't dispatch any mutation events anyway.
> 
> Why?

I remember there being a specific reason for this that I discussed outside this bug (irc with one of the content folks?).  Something along the lines of mutation events inside document fragment nodes (which are the only other possibility I'm aware of).

That said, you might have this fixed with bug 234455.
Comment 49 Alex Vincent [:WeirdAl] 2006-03-09 19:27:20 PST
Martin, could you update the testcase in comment 5 to use https instead of http?  I think this is causing this error:

Error: uncaught exception: Permission denied to call method XMLHttpRequest.open
Comment 50 Alex Vincent [:WeirdAl] 2006-03-09 22:45:13 PST
Created attachment 214644 [details]
testcase using createDocument, testing document.appendChild, element.setAttribute
Comment 51 Alex Vincent [:WeirdAl] 2006-03-09 22:48:38 PST
Created attachment 214645 [details] [diff] [review]
patch, v5

(In reply to comment #47)
> if one already exists, and that can be got from nsINode, no need for
> nsIDOMEventReceiver.

smaug: Correct me if I'm wrong, but windows don't implement nsINode.  If that's the case, what should I do?

Also, for the record, since event dispatch is now highly centralized, I'm going to stop caring (at least for this bug) as to whether the handle event routines check rv.  :)
Comment 52 Alex Vincent [:WeirdAl] 2006-03-09 23:17:24 PST
Comment on attachment 119875 [details]
Test code for mutation events

I'm looking at the testcase for comment 1, and I don't think it works properly.  It checks for the node name of evt.relatedNode, which according to DOM 2 Events "holds the parent node".  Since the inserted node has no parent, evt.relatedNode is null, and errors get thrown in my browser suite with the testcase.
Comment 53 Alex Vincent [:WeirdAl] 2006-03-09 23:53:59 PST
Created attachment 214652 [details] [diff] [review]
patch, v5.1

*sigh* The preceding patch crashed on Martin Honnen's testcase.  Crash fixed.
Comment 54 Martin Honnen 2006-03-10 09:06:24 PST
Created attachment 214685 [details]
new test case necessary as bugzilla now uses HTTPS

Alex, I have updated the test case to access all files by HTTPS.
Comment 55 Alex Vincent [:WeirdAl] 2006-03-10 10:00:02 PST
My Firefox 1.5 build shows the iframe receiving the mutation event, but not the XMLHttpRequest version.

SeaMonkey debug trunk build with patch v5.1 shows the iframe not receiving the mutation event, and the XMLHttpRequest version receiving it.

SeaMonkey debug trunk build without patch v5.1 shows the iframe receiving the mutation event, but not the XMLHttpRequest version.

So somewhere I caused a regression.  I'll need to look into what I did that caused that.
Comment 56 Alex Vincent [:WeirdAl] 2006-03-10 14:19:31 PST
Created attachment 214727 [details] [diff] [review]
patch, v5.2

Stupid copy & paste typo. :) It would have been caught in reviews, anyway.
Comment 57 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2006-03-14 14:16:32 PST
Comment on attachment 214727 [details] [diff] [review]
patch, v5.2


>+PRBool
>+ObjectHasMutationListeners(nsISupports* aNode)
>+{
>+  nsCOMPtr<nsIDOMEventReceiver> rec(do_QueryInterface(aNode));

Could you QI here to nsINode and use its GetEventListenerManager.
nsIDOMEventReceiver is often a tearoff, and although it is cached,
I think it should be better to avoid using it in this case.


>+
>+/* static */
>+PRBool
>+nsContentUtils::HasMutationListeners(nsIContent* aContent,
>+                                     nsIDocument* aDocument,
>+                                     PRUint32 aType)
....
>+  return ObjectHasMutationListeners(aDocument) ||
>+         ObjectHasMutationListeners(window);
>+}

If you change ObjectHasMutationListeners to handle only nsINodes
(perhaps its name should be then NodeHasMutationListeners), you could have
here a special case for the window, something like
if (!NodeHasMutationListeners(aDocument)) {
  nsCOMPtr<nsIDOMEventReceiver> receiver = do_QueryInterface(window);
  ....
}


>+    PRBool hasListeners = PR_FALSE;
>+    if (doc) {
>+      hasListeners = nsContentUtils::HasMutationListeners(this,
>+        doc,
>+        NS_EVENT_BITS_MUTATION_NODEREMOVED);
>+    }
>+
>+    if (hasListeners) {

Why do you need hasListeners variable?
if (doc && nsContentUtils::HasMutationListeners...)
should be enough
Comment 58 Alex Vincent [:WeirdAl] 2006-03-14 15:05:47 PST
Created attachment 215071 [details] [diff] [review]
patch, v5.3

Answering smaug's r- comments for previous patch.
Comment 59 Jonas Sicking (:sicking) No longer reading bugmail consistently 2006-03-14 15:08:30 PST
Note that the HasMutationListeners is a very critical performance path. Right now I think we spend about 4% of Tp time in that function so you must not do anything to slow it down.

Of course, speedups are always welcome :)
Comment 60 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2006-03-14 23:59:22 PST
(In reply to comment #59)
> Note that the HasMutationListeners is a very critical performance path. Right
> now I think we spend about 4% of Tp time in that function so you must not do
> anything to slow it down.
> 
> Of course, speedups are always welcome :)
>

Exactly :) That is why I proposed using nsINode, not nsIDOMEventReceiver.
Is that 4% really true? We must do something to that.

Comment 61 Jonas Sicking (:sicking) No longer reading bugmail consistently 2006-03-15 00:01:47 PST
I don't remember where I got that number, but yeah, iirc it's somewhere around there.

The solution is actually pretty simple, we should only check for mutation events when aNotify is true.
Comment 62 Alex Vincent [:WeirdAl] 2006-03-15 06:26:53 PST
Comment on attachment 215071 [details] [diff] [review]
patch, v5.3

That's for some other bug, where I might want to be cc'd, but not actually do the work... :)
Comment 63 Alex Vincent [:WeirdAl] 2006-03-15 06:28:00 PST
Comment on attachment 215071 [details] [diff] [review]
patch, v5.3

Oops, I promised sicking a chance to look the patch over.
Comment 64 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2006-03-15 06:55:58 PST
(In reply to comment #63)
> (From update of attachment 215071 [details] [diff] [review] [edit])
> Oops, I promised sicking a chance to look the patch over.
> 

that's a good idea. I'm not in anyway 'official' reviewer for anything else than xforms ;)
Comment 65 timeless 2006-03-15 22:15:00 PST
Comment on attachment 215071 [details] [diff] [review]
patch, v5.3

>+   * Quick helper to determine whether there are any mutation listeners
>+   * of a given type that apply to this content (are at or above it).

I know you're just moving the comment, but the parenthetical bothers me :).

>+   * @param aContent  The node to search for listeners
>+   * @param aDocument The owner document of the node, or the node to search
>+   * @param aType     The type of listener (NS_EVENT_BITS_MUTATION_*)

>+PRBool
>+NodeHasMutationListeners(nsINode* aNode)
>+{
>+  if (!aNode)
>+    return PR_FALSE;
>+  nsCOMPtr<nsIEventListenerManager> manager;
>+  aNode->GetEventListenerManager(PR_FALSE, getter_AddRefs(manager));

i wonder if you should return early here instead of overindenting. but this is copied code.

>+  if (manager) {
>+    PRBool hasMutationListeners = PR_FALSE;
>+    manager->HasMutationListeners(&hasMutationListeners);
>+    return hasMutationListeners;
>+  }
>+  return PR_FALSE;
>+}

>+PRBool
>+nsContentUtils::HasMutationListeners(nsIContent* aContent,
>+                                     nsIDocument* aDocument,
>+                                     PRUint32 aType)
>+{
>+  NS_PRECONDITION(!aContent || !aContent->IsInDoc() ||
>+     aContent->GetCurrentDoc() == aDocument,
>+     "Incorrect aDocument");
>+
>+  nsCOMPtr<nsPIDOMWindow> window;
>+
>+  // global will be null for documents that don't have windows.

this wasn't a comptr before so it probably shouldn't be one now, as it would incurr an addref and release as well as given your placement the general construction of the comptr. the code should probably be:

if (aDocument) {
  nsIScriptGlobalObject* global = ...
  if (global) ...

>+  nsCOMPtr<nsIScriptGlobalObject> global;
>+  if (aDocument)
>+    global = aDocument->GetScriptGlobalObject();
>+  if (global) {
>+    window = do_QueryInterface(global);
>+    if (window && !window->HasMutationListeners(aType))
>+      return PR_FALSE;
>+  }
>+
>+  // If we have a window, we know a mutation listener is registered, but it
>+  // might not be in our chain.  If we don't have a window, we might have a
>+  // mutation listener.  Check quickly to see.

The original code declared curr in the for loop init, since you're not using curr outside, i don't think you should change that

>+  nsIContent* curr;
>+  for (curr = aContent; curr; curr = curr->GetParent())
>+    if (NodeHasMutationListeners(curr))
>+      return PR_TRUE;
Comment 66 Jonas Sicking (:sicking) No longer reading bugmail consistently 2006-03-17 19:48:26 PST
Comment on attachment 215071 [details] [diff] [review]
patch, v5.3

>Index: content/base/src/nsContentUtils.cpp
>+nsContentUtils::HasMutationListeners(nsIContent* aContent,
>+                                     nsIDocument* aDocument,
>+                                     PRUint32 aType)
>+{
>+  NS_PRECONDITION(!aContent || !aContent->IsInDoc() ||
>+     aContent->GetCurrentDoc() == aDocument,
>+     "Incorrect aDocument");
>+
>+  nsCOMPtr<nsPIDOMWindow> window;
>+
>+  // global will be null for documents that don't have windows.
>+  nsCOMPtr<nsIScriptGlobalObject> global;
>+  if (aDocument)
>+    global = aDocument->GetScriptGlobalObject();
>+  if (global) {
>+    window = do_QueryInterface(global);
>+    if (window && !window->HasMutationListeners(aType))
>+      return PR_FALSE;
>+  }
>+
>+  // If we have a window, we know a mutation listener is registered, but it
>+  // might not be in our chain.  If we don't have a window, we might have a
>+  // mutation listener.  Check quickly to see.
>+  nsIContent* curr;
>+  for (curr = aContent; curr; curr = curr->GetParent())
>+    if (NodeHasMutationListeners(curr))
>+      return PR_TRUE;
>+
>+  if (NodeHasMutationListeners(aDocument))
>+    return PR_TRUE;
>+
>+  // Last chance:  check the window.
>+  PRBool hasMutationListeners = PR_FALSE;
>+  nsCOMPtr<nsIDOMEventReceiver> rec(do_QueryInterface(window));
>+  if (rec) {
>+    nsCOMPtr<nsIEventListenerManager> manager;
>+    rec->GetListenerManager(PR_FALSE, getter_AddRefs(manager));
>+    if (manager) {
>+      manager->HasMutationListeners(&hasMutationListeners);
>+    }
>+  }
>+  return hasMutationListeners;
>+}

You can move this window check to inside the initial if statements. I think that'd be cleaner.
Comment 67 Jonas Sicking (:sicking) No longer reading bugmail consistently 2006-03-17 19:52:33 PST
Other then that a quick glance looks ok.
Comment 68 Alex Vincent [:WeirdAl] 2006-03-17 19:58:41 PST
Comment on attachment 215071 [details] [diff] [review]
patch, v5.3

Now I'm requesting sr from peterv.

peterv:  if you want a new patch that incorporates the aforementioned changes, just say so.
Comment 69 Jonas Sicking (:sicking) No longer reading bugmail consistently 2006-03-18 23:54:30 PST
Comment on attachment 215071 [details] [diff] [review]
patch, v5.3

Another comment I forgot to make:

>+nsContentUtils::HasMutationListeners(nsIContent* aContent,
>+                                     nsIDocument* aDocument,
>+                                     PRUint32 aType)
...
>+  nsIContent* curr;
>+  for (curr = aContent; curr; curr = curr->GetParent())
>+    if (NodeHasMutationListeners(curr))
>+      return PR_TRUE;

Please follow convention and use {} around |if| and |for| statements. That goes for everywhere in this function. It makes future patches easier to read.
Comment 70 Peter Van der Beken [:peterv] 2006-03-23 07:44:43 PST
Comment on attachment 215071 [details] [diff] [review]
patch, v5.3

>Index: content/base/public/nsContentUtils.h
>===================================================================

>+   * Quick helper to determine whether there are any mutation listeners
>+   * of a given type that apply to this content (are at or above it).
>+   *
>+   * @param aContent  The node to search for listeners
>+   * @param aDocument The owner document of the node, or the node to search

You need to specify this more. Must aDocument be non-null? What about aContent? This talks about the owner document, but in the code you use GetCurrentDoc, which is it?

>+   * @param aType     The type of listener (NS_EVENT_BITS_MUTATION_*)
>+   *
>+   * @return boolean true if there are mutation listeners of the specified type

w/boolean //

>Index: content/base/src/nsContentUtils.cpp
>===================================================================

>+/**
>+ * Quick helper to determine whether there are any mutation listeners
>+ * of a given type that apply to the node or window passed in.

This talks about window but it takes nsINode?

>+ * @param aNode nsINode to check for listeners.

s/nsINode/node/

>+ * @return PRBool true if there are mutation listeners or not.

s/PRBool true //

>+NodeHasMutationListeners(nsINode* aNode)
>+{
>+  if (!aNode)
>+    return PR_FALSE;

It seems like it would be cheaper to put this null check around the one caller where it can be null (NodeHasMutationListeners(aDocument) below).

>+nsContentUtils::HasMutationListeners(nsIContent* aContent,
>+                                     nsIDocument* aDocument,
>+                                     PRUint32 aType)
>+{
>+  NS_PRECONDITION(!aContent || !aContent->IsInDoc() ||
>+     aContent->GetCurrentDoc() == aDocument,
>+     "Incorrect aDocument");

Line this up on the opening parens.

>+
>+  nsCOMPtr<nsPIDOMWindow> window;
>+
>+  // global will be null for documents that don't have windows.
>+  nsCOMPtr<nsIScriptGlobalObject> global;
>+  if (aDocument)
>+    global = aDocument->GetScriptGlobalObject();
>+  if (global) {
>+    window = do_QueryInterface(global);
>+    if (window && !window->HasMutationListeners(aType))
>+      return PR_FALSE;
>+  }

Why not just

  nsCOMPtr<nsPIDOMWindow> window;
  if (aDocument) {
    window = do_QueryInterface(aDocument->GetScriptGlobalObject());
    if (window && !window->HasMutationListeners(aType))
      return PR_FALSE;
  }

>Index: content/xul/content/src/nsXULElement.cpp
>===================================================================

>@@ -1439,31 +1445,31 @@ nsXULElement::UnsetAttr(PRInt32 aNameSpa

>-    if (doc) {
>-        if (hasMutationListeners) {
>-            nsMutationEvent mutation(PR_TRUE, NS_MUTATION_ATTRMODIFIED);
>+    if (hasMutationListeners) {
>+        nsMutationEvent mutation(PR_TRUE, NS_MUTATION_ATTRMODIFIED);
> 
>-            mutation.mRelatedNode = attrNode;
>-            mutation.mAttrName = aName;
>+        mutation.mRelatedNode = attrNode;
>+        mutation.mAttrName = aName;
> 
>-            if (!oldValue.IsEmpty())
>-              mutation.mPrevAttrValue = do_GetAtom(oldValue);
>-            mutation.mAttrChange = nsIDOMMutationEvent::REMOVAL;
>+        if (!oldValue.IsEmpty())
>+            mutation.mPrevAttrValue = do_GetAtom(oldValue);
>+        mutation.mAttrChange = nsIDOMMutationEvent::REMOVAL;
> 
>-            nsEventDispatcher::Dispatch(NS_STATIC_CAST(nsIContent*, this),
>-                                        nsnull, &mutation);
>-        }
>+        nsEventDispatcher::Dispatch(NS_STATIC_CAST(nsIContent*, this),
>+                                    nsnull, &mutation);
>+    }
> 
>+    if (doc) {

This change doesn't really add anything? It just messes up cvs blame.
Comment 71 Alex Vincent [:WeirdAl] 2006-03-24 19:27:38 PST
Created attachment 216183 [details] [diff] [review]
patch, v5.4

fixing nits from peterv's sr- and other comments since previous patch was posted.
Comment 72 Peter Van der Beken [:peterv] 2006-03-29 21:45:58 PST
Comment on attachment 216183 [details] [diff] [review]
patch, v5.4

>Index: content/base/src/nsContentUtils.cpp
>===================================================================

>+nsContentUtils::HasMutationListeners(nsIContent* aContent,
>+                                     nsIDocument* aDocument,
>+                                     PRUint32 aType)
>+{
>+  NS_PRECONDITION(aDocument,
>+                  "nsContentUtils::HasMutationListeners requires a document!");

I don't understand this, you require aDocument to be non-null, but then you made the function handle a null aDocument below? This really needs to be cleared up.

>+  NS_PRECONDITION(!aContent || aContent->GetCurrentDoc() == aDocument,
>+                  "Incorrect aDocument");
>+  if (aDocument)
>+  {

The style in these files is to put the brace on the same line, so do that everywhere.

>+    // global object will be null for documents that don't have windows.
>+    nsCOMPtr<nsPIDOMWindow> window;
>+    window = do_QueryInterface(aDocument->GetScriptGlobalObject());
>+    if (window && !window->HasMutationListeners(aType))
>+      return PR_FALSE;

You didn't add the braces around this if.

>+      PRBool hasListeners = PR_FALSE;
>+      if (manager)
>+      {
>+        manager->HasMutationListeners(&hasListeners);
>+      }
>+      if (hasListeners)
>+      {
>+        return PR_TRUE;
>+      }

      if (manager) {
        PRBool hasListeners = PR_FALSE;
        manager->HasMutationListeners(&hasListeners);
        if (hasListeners) {
          return PR_TRUE;
        }
      }

>+  if (aDocument && NodeHasMutationListeners(aDocument))
>+  {
>+    return PR_TRUE;
>+  }
>+
>+  return PR_FALSE;

  return aDocument && NodeHasMutationListeners(aDocument);
Comment 73 Alex Vincent [:WeirdAl] 2006-03-30 06:49:40 PST
Marking dependent on bug 90983 so I can track that bug before posting another patch.
Comment 74 Jonas Sicking (:sicking) No longer reading bugmail consistently 2006-03-30 09:12:28 PST
No need to wait for that bug before checking this in. This patch will still make us fire mutation events for the vast majority of nodes in the document. Take one step at a time, it doesn't need to be perfect right away.
Comment 75 Alex Vincent [:WeirdAl] 2006-03-30 12:57:02 PST
Created attachment 216776 [details] [diff] [review]
patch, v5.5

I'm going to keep trying until I get this right.  :)

In current trunk builds, we would hit my assertion for NS_PRECONDITION(aDocument) while constructing the browser window, so I just took it out.  I'm now returning PR_FALSE in nsContentUtils::HasMutationListeners if (!aDocument), because this patch isn't about supporting mutation events in content that doesn't have a document.

If someone wants that, they can file their own bug for it.  :)
Comment 76 Peter Van der Beken [:peterv] 2006-04-24 02:25:07 PDT
Comment on attachment 216776 [details] [diff] [review]
patch, v5.5

>Index: content/base/src/nsContentUtils.cpp
>===================================================================

>+nsContentUtils::HasMutationListeners(nsIContent* aContent,
>+                                     nsIDocument* aDocument,
>+                                     PRUint32 aType)
>+{
>+  NS_PRECONDITION(!aContent || aContent->GetCurrentDoc() == aDocument,
>+                  "Incorrect aDocument");
>+  if (!aDocument) {
>+    // We do not support event listeners on content not attached to documents.
>+    return PR_FALSE;
>+  }

Since you check here for aDocument, you should remove all the checks for null doc at the callsites for this function and remove the mention in the documentation that aDocument must be non-null.

>+  if (rec) {
>+    nsCOMPtr<nsIEventListenerManager> manager;
>+    rec->GetListenerManager(PR_FALSE, getter_AddRefs(manager));
>+    PRBool hasListeners = PR_FALSE;

Drop this var. didn't you get a warning about the next declaration shadowing this one?

>+    if (manager) {
>+      PRBool hasListeners = PR_FALSE;


>+  if (NodeHasMutationListeners(aDocument)) {
>+    return PR_TRUE;
>+  }
>+
>+  return PR_FALSE;

Just replace this with:

  return NodeHasMutationListeners(aDocument);
Comment 77 Alex Vincent [:WeirdAl] 2006-04-24 11:57:09 PDT
Created attachment 219630 [details] [diff] [review]
Final patch
Comment 78 Alex Vincent [:WeirdAl] 2006-04-24 13:06:13 PDT
Fix checked in by bz.  Thanks, everyone!

Note You need to log in before you can comment on or make changes to this bug.