55595 - Implement DOM Level 2 Mutation Events

Status: VERIFIED FIXED

(Core :: DOM: Events, defect, P3)

Description

[David Hyatt]

DOM Level 2 mutation events.  We need 'em.  We don't got 'em.  Patch coming 
soon.  Stay tuned.

Comment 1

[David Hyatt]

Attachment: DOM Patch for mutation events

Comment 2

[David Hyatt]

Attachment: Layout patch for mutation events

Comment 3

[David Hyatt]

Attachment: XUL patch for mutation events

Comment 4

[David Hyatt]

Attachment: XBL patch for mutation events

Comment 5

[David Hyatt]

r=? Anyone? Bueller? Bueller?

Comment 6

[hand]

Have any of these patches been checked in?  I've never applied a patch before.

Comment 7

[Brendan Eich [:brendan]]

Still reviewing, will update later tonight.

/be

Comment 8

[Brendan Eich [:brendan]]

Are there tabs (as required by Unix make) at the beginning of the modified lines
in , e.g.this + line from dom/public/coreEvents/Makefile.in?

+    nsIDOMMutationEvent.h \
	nsIDOMEventListener.h \
	nsIDOMEventCapturer.h \

Similar q for the makefile.win changes, although maybe nmake tolerates leading
spaces.  Unix makes traditionally require a leading tab in some contexts (rule
commands), and it's good style to use them in these multiline macro values too,
even though you usually want to avoid entering tabs into Mozilla files.

From nsJSDOMEventListener.cpp:

-  if (NS_OK != NS_NewScriptKeyEvent(mContext, aEvent, nsnull, (void**)&eventObj))
-    return NS_ERROR_FAILURE;
+  if (NS_OK != NS_NewScriptKeyEvent(mContext, aEvent, nsnull, (void**)&eventObj)) {
+    if (NS_OK != NS_NewScriptMutationEvent(mContext, aEvent, nsnull,
(void**)&eventObj))
+      return NS_ERROR_FAILURE;
+  }

If NS_NewScriptKeyEvent fails, aren't we likely out of memory or badly scrogged
or misconfigured?  Why retry NS_NewScriptMutationEvent in this odd case?

In nsJSKeyEvent.cpp, why did the initialized definition of KeyEventProperties
move up?

More tomorrow.

/be

Comment 9

[Brendan Eich [:brendan]]

nsGenericDOMDataNode.cpp, around line 930:

+  if (mDocument && HasMutationListeners(aOuterContent,
NS_EVENT_BITS_MUTATION_CHARACTERDATAMODIFIED)) {
+    nsCOMPtr<nsIDOMEventTarget> node(do_QueryInterface(aOuterContent));
+    nsMutationEvent mutation;
+    mutation.eventStructType = NS_MUTATION_EVENT;
+    mutation.message = NS_MUTATION_CHARACTERDATAMODIFIED;
+    mutation.mTarget = node;
+
+    // XXX Handle the setting of prevValue!
+    nsAutoString newVal(aBuffer);
+    if (!newVal.IsEmpty())
+      mutation.mNewAttrValue = getter_AddRefs(NS_NewAtom(newVal));

Check for out-of-memory if NS_NewAtom returns null.

+    nsEventStatus status = nsEventStatus_eIgnore;
+    nsCOMPtr<nsIDOMEvent> domEvent;
+    HandleDOMEvent(nsnull, &mutation, getter_AddRefs(domEvent),
NS_EVENT_FLAG_INIT, &status);
+  }

This whole paragraph is repeated in the other overloading of SetText, which to
me cries out for a common subroutine at some point.  Without measuring code size
change, I think the source complexity is enough to warrant a subroutine.  Then
you could play with inlining to minimize code footprint and subroutine call
costs, while still keep source complexity down by commoning the paragraph.

Speaking of code footprint, why copy-paste HasMutationListeners from
nsGenericDOMDataNode.cpp into nsGenericElement.cpp?  I really think a common
utility subroutine or static method is called for here.

Similar comments to those above on SetText apply to SetAttribute.  The code in
each case has little specializations (as the SetText did, depending on PRUnichar
vs. char strings and doing different nsAutoString init accordingly -- but that
could be done in the caller of the common subroutine).  I hope I'm not asking
for too much here, but again I worry that code footprint and source complexity
both argue for a common subroutine here (and inline if code footprint savings
aren't enough to justify the out-of-line call overhead).

The inserted and removed cases also look like specialized inlinings of a common
subroutine.

Looks like nsGenericHTMLElement.cpp has very similar code that could be rolled
up into a couple of common methods (HasMutationListeners for sure, and perhaps
one or two HandleMutationEvent variations).

Speaking of bloat, why can't nsIDOMMutationListener use
NS_DECLARE_STATIC_IID_ACCESSOR so the compiler can handle the static IID and
relieve you from having to do

 +NS_DEFINE_IID(kIDOMMutationListenerIID, NS_IDOMMUTATIONLISTENER_IID);

Or is this by-hand method actually better for data footprint?  Wait, I see later
NS_GET_IID(nsIDOMMutationListener) usage, so there must be a static IID accessor
in the interface -- so why do we need this old-style kFoo one?

NS_STATIC_CAST here, please:

+  nsMutationEvent* mutation = (nsMutationEvent*)aEvent;
+  SetTarget(mutation->mTarget);

(this is from nsDOMMutationEvent's constructor)

From InitMutationEvent, a nit:

+  mEvent->flags |= aCanBubbleArg ? NS_EVENT_FLAG_NONE : NS_EVENT_FLAG_CANT_BUBBLE;
+  mEvent->flags |= aCancelableArg ? NS_EVENT_FLAG_NONE : NS_EVENT_FLAG_CANT_CANCEL;

relies on the compiler to optimize away the |= 0 branch of the ?: expression.
It's cute cuz fits on two lines, but

+  if (!aCanBubbleArg) mEvent->flags |= NS_EVENT_FLAG_CANT_BUBBLE;
+  if (!aCancelableArg) mEvent->flags |= NS_EVENT_FLAG_CANT_CANCEL;

fits also.

More unchecked NS_NewAtom calls below in the same method.

Ok, on to XUL and XBL.

/be

Comment 10

[David Hyatt]

My attitude is that coalescing is too much of a pain at this point, and I may as 
well wait for the real coalescing of the content models that jst is going to be 
working on, i.e., many of the methods I've patched are slated to go away anyway 
in favor of new coalesced methods, which would make my attempts to coalesce only 
my piece useless (or at the very least work that would be thrown away when 
better coalescing happened).

Comment 11

[Brendan Eich [:brendan]]

Ok, the only other comment I had was about the unchecked NS_NewAtom calls, esp.
those done at event handling time (which seem likelier to fail due to memory
shortfall).  Is it worth returning NS_ERROR_OUT_OF_MEMORY for those rather than
storing null or crashing on a null nsIAtom*?

Can you give before and after code size numbers on Windows easily?  On Linux you
could use size on the particular libraries.  Of course, on Linux gcc generates
twice as much code, byte-for-byte, from the same source, as MSVC does.  I'll sr=
here: sr=[e-mail], but please do quantify the code bloat costs.  If
they're tiny, I'll shut up (for now -- too much of this kind of copy-paste will
cost -- it wasn't the last piece of pumpkin pie I ate that made me fat ;-).

/be

Comment 12

[David Hyatt]

Sure, I can patch the NS_NewAtom calls.

Comment 13

[David Hyatt]

Partial support is in.  Bugs should be filed to implement the remaining 3 
events that I didn't get to.

Comment 14

[Vladimir Ermakov]

QA Contact Update

Comment 15

[Vladimir Ermakov]

Verifying on build 2001-08-02-03-trunk
bugs 74218, 74219, and 74220 are filed to cover the nonimplemented mutation events.