Bug 70436 – [MutationObservers] Implement basic subtree observation

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

[Adam Klein]

[MutationObservers] Implement basic subtree observation

[Adam Klein]

Created attachment 111668 [details]
Patch

[Rafael Weinstein]

Comment on attachment 111668 [details]
Patch

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

> Source/WebCore/dom/Node.cpp:2695
> +        const size_t size = observerEntries->size();

maybe extract a static utility function here?

[Adam Klein]

Comment on attachment 111668 [details]
Patch

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

>> Source/WebCore/dom/Node.cpp:2695
>> +        const size_t size = observerEntries->size();
> 
> maybe extract a static utility function here?

That's how I started, but it the inner if-statement differs depending on whether we're looking at the node itself or one of its ancestors.  This means that either we do extra checking inside the loop (bad for performance), or we extract the test out into a functor (bad for readability, imho).  That's why I ended up "unrolling" the code manually.  Let me know if you'd prefer the functor approach (I really don't want to do any more work in the inner loop than I have to).

[Ryosuke Niwa]

Comment on attachment 111668 [details]
Patch

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

>>> Source/WebCore/dom/Node.cpp:2695

>> 
>> maybe extract a static utility function here?
> 
> That's how I started, but it the inner if-statement differs depending on whether we're looking at the node itself or one of its ancestors.  This means that either we do extra checking inside the loop (bad for performance), or we extract the test out into a functor (bad for readability, imho).  That's why I ended up "unrolling" the code manually.  Let me know if you'd prefer the functor approach (I really don't want to do any more work in the inner loop than I have to).

I think we should keep matches to take MutationObserverOptions and do:
if (Vector<MutationObserverEntry>* observerEntries = mutationObserverEntries())
    addMatchedObserverEntriesToSet(observerSet, *observerEntries, type);
for (Node* node = parentNode(); node; node = node->parentNode()) {
    if (Vector<MutationObserverEntry>* observerEntries = node->mutationObserverEntries())
        addMatchedObserverEntriesToSet(observerSet, *observerEntries, type | WebKitMutationObserver::Subtree);
}

[Adam Klein]

Created attachment 111856 [details]
Extracted helper function

[Adam Klein]

Comment on attachment 111668 [details]
Patch

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

>>>> Source/WebCore/dom/Node.cpp:2695
>>>> +        const size_t size = observerEntries->size();
>>> 
>>> maybe extract a static utility function here?
>> 
>> That's how I started, but it the inner if-statement differs depending on whether we're looking at the node itself or one of its ancestors.  This means that either we do extra checking inside the loop (bad for performance), or we extract the test out into a functor (bad for readability, imho).  That's why I ended up "unrolling" the code manually.  Let me know if you'd prefer the functor approach (I really don't want to do any more work in the inner loop than I have to).
> 
> 

After talking to Rafael offline, came up with a fix (just make matches() stricter).  I've now extracted the loop into its own function.

[Adam Klein]

Created attachment 112009 [details]
Merged to r98133

[Ryosuke Niwa]

Comment on attachment 112009 [details]
Merged to r98133

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

r=me provided problems listed below are all addressed.

> Source/WebCore/ChangeLog:9
> +        Note that this patch only implements "basic" subtree
> +        semantics, not the fully robust semantics described in

Nit: semantics should probably on the previous line to avoid awkward line break.

> Source/WebCore/ChangeLog:22
> +        (WebCore::addMatchingObservers): Static helper method for the below.

I'm not sure what you're referring to by "the below". Could you spell out the function name here?

> Source/WebCore/dom/Node.cpp:2703
> +    if (observerEntries) {

We prefer early exiting over nesting ifs especially when the if statement appears at the beginning of a function and the normal flow is to do whatever inside the if.

> LayoutTests/fast/mutation/observe-subtree.html:32
> +        observer = new WebKitMutationObserver(function(m) {
> +            mutations = m;

Nit: please avoid using one-letter variable.

> LayoutTests/fast/mutation/observe-subtree.html:75
> +        observer = new WebKitMutationObserver(function(m) {
> +            mutations = m;
> +        });
> +        observer2 = new WebKitMutationObserver(function(m) {
> +            mutations2 = m;
> +        });

Ditto about one-letter variables.

> LayoutTests/fast/mutation/observe-subtree.html:116
> +        observer = new WebKitMutationObserver(function(m) {
> +            mutations = m;
> +            ++calls;
> +        });

Ditto.

[Adam Klein]

Comment on attachment 112009 [details]
Merged to r98133

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

>> Source/WebCore/ChangeLog:9
>> +        semantics, not the fully robust semantics described in
> 
> Nit: semantics should probably on the previous line to avoid awkward line break.

Done.

>> Source/WebCore/ChangeLog:22
>> +        (WebCore::addMatchingObservers): Static helper method for the below.
> 
> I'm not sure what you're referring to by "the below". Could you spell out the function name here?

Done.

>> Source/WebCore/dom/Node.cpp:2703
>> +    if (observerEntries) {
> 
> We prefer early exiting over nesting ifs especially when the if statement appears at the beginning of a function and the normal flow is to do whatever inside the if.

Done.

>> LayoutTests/fast/mutation/observe-subtree.html:32
>> +            mutations = m;
> 
> Nit: please avoid using one-letter variable.

Done.

>> LayoutTests/fast/mutation/observe-subtree.html:75
>> +        });
> 
> Ditto about one-letter variables.

Done.

>> LayoutTests/fast/mutation/observe-subtree.html:116
>> +        });
> 
> Ditto.

Done.

[Adam Klein]

Created attachment 112033 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 112033 [details]
Patch for landing

Clearing flags on attachment: 112033

Committed r98163: <http://trac.webkit.org/changeset/98163>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.