Bug 66079 – [work in progress] Implement MutationListener/MutationTarget, a replacement for DOM Mutation Events

• Status: RESOLVED
• Resolution: WORKSFORME
• Component: New Bugs

[Adam Klein]

[work in progress] Implement MutationListener/MutationTarget, a replacement for DOM Mutation Events

[Adam Klein]

Created attachment 103650 [details]
Patch

[Adam Klein]

As noted in title, this is is a work in progress, and is neither itself a complete implementation nor small enough to be reviewed reasonably.  Instead, it's meant to illustrate a WebKit implementation close to Jonas Sicking's proposal for a replacement to MutationEvents in http://lists.w3.org/Archives/Public/public-webapps/2011JulSep/0369.html.

[Adam Klein]

Also, pulled from the ChangeLog:

        Partially implements an experimental, vendor-prefixed API with the
        hope of eventually replacing MutationEvents. The API is based in
        large part on a proposal by Jonas Sicking in a message to public-webapps:
 
        http://lists.w3.org/Archives/Public/public-webapps/2011JulSep/0369.html

        That thread is very long, but has helpfully been summarized by
        Rafael Weinstein in a recent message:

        http://lists.w3.org/Archives/Public/public-webapps/2011JulSep/0779.html

        Note that this patch is in no way meant to be a complete
        implementation. The biggest missing feature is the actual dispatching
        of MutationRecordLists to MutationListeners: the included layout
        test instead calls a "spinMutationQueue" function to trigger
        notifications. This is not actually meant to be exposed to JS,
        though: my hope is to instead call spinMutationQueue from
        C++, either from somewhere in WebCore or (more likely) in
        the embedder. I have a separate change which implements the
        latter in Chromium.

        The other main thing missing from this patch is that MutationRecords
        are not dispatched in all the proper places, e.g., the handling of
        innerHTML is not as we hope it to be. This is simply a matter of
        wanting to put this patch up to get feedback: it shouldn't be
        much work to add MutationRecord dispatching in all the proper
        places.

[Alexey Proskuryakov]

I suggest sending a note about this work to webkit-dev. The idea of moving away from DOM mutation events is of course a generally accepted one, and there are many people who may want to participate in one way or another.

[Adam Klein]

Created attachment 104260 [details]
Patch

[Dominic Cooney]

Comment on attachment 104260 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=104260&action=review

This looks really cool.

Why do apparently new files have licenses so divergent in terms of copyright holder, date and terms?

> LayoutTests/fast/dom/mutation-listener.html:91
> +  </script>

Is it worth adding negative tests, eg when you’re listening to attribute mutation child mutation doesn’t fire the listener, etc.? And tests that merging work? etc.

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:483
> +        push(@customInternalFields, "mutationListenerCacheIndex");

Why not just use the event listener cache?

> Source/WebCore/dom/MutationRecord.cpp:41
> +// TODO(adamk): Figure out whether we want to share a single empty node list, or

Sharing a single empty node list is a bad idea: Presumably want it to appear as a unique object to JavaScript (ie expandos stuck on in one place not appearing in another) and deciding when to create a wrapper could be tedious.

[Adam Klein]

(In reply to comment #6)
> (From update of attachment 104260 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=104260&action=review
> 
> This looks really cool.
> 
> Why do apparently new files have licenses so divergent in terms of copyright holder, date and terms?

Most should be new and marked Google 2011; the main case where this diverges is under bindings/js, where the classes are modified copies of JSEventListener. Not sure what the usual WebKit style is for this.

> > LayoutTests/fast/dom/mutation-listener.html:91
> > +  </script>
> 
> Is it worth adding negative tests, eg when you’re listening to attribute mutation child mutation doesn’t fire the listener, etc.? And tests that merging work? etc.

More tests are definitely in order, this file for now is a basic proof that _something_ is happening. As noted above, the implementation isn't complete, e.g., setting innerHTML doesn't do what it's supposed to (batching all the removals and insertions together into a single MutationRecord).

> > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:483
> > +        push(@customInternalFields, "mutationListenerCacheIndex");
> 
> Why not just use the event listener cache?

Well, the biggest reason is that it doesn't exist on Node, since there's now an alternative way of handling refcounting for event listeners on nodes.  In general I'm fairly unhappy with how much EventListener logic had to be duplicated in the bindings, so suggestions on how that could be reduced are welcome (I plan to look into some template-based approach when I get a chance).

> > Source/WebCore/dom/MutationRecord.cpp:41
> > +// TODO(adamk): Figure out whether we want to share a single empty node list, or
> 
> Sharing a single empty node list is a bad idea: Presumably want it to appear as a unique object to JavaScript (ie expandos stuck on in one place not appearing in another) and deciding when to create a wrapper could be tedious.

[Adam Klein]

Created attachment 104930 [details]
Replace MutationRecordList with an array of MutationRecords

[Adam Klein]

Created attachment 105032 [details]
Remove MutationTypes, use bitmasks everywhere

[Adam Klein]

Created attachment 105241 [details]
Make Document, DocumentFragment, and Element implement MutationTarget instead of Node

[Adam Klein]

Created attachment 105544 [details]
MutationRecord changes: addedCount, childlistIndex, and inDocument

[Adam Klein]

Created attachment 105558 [details]
MutationRecord: removed prevValue/newValue

[Adam Klein]

Created attachment 105559 [details]
MutationRecord: memory optimization

[Adam Klein]

Created attachment 105685 [details]
Merged to r94094

[Adam Klein]

Created attachment 105727 [details]
Coalesce childlist notifications for innerHTML etc

[Adam Klein]

Created attachment 105845 [details]
Created MutationScope.{h,cpp}, some API updates

[Adam Klein]

Created attachment 106198 [details]
Added tests and fixes for replaceChild, insertBefore, innerHTML

[Adam Klein]

Created attachment 106218 [details]
Test reorganization