[webkit-reviews] review granted: [Bug 71499] Only walk up the tree in search of MutationObservers if one has been added : [Attachment 113546] Patch

[bugzilla-daemon at webkit.org]

Ojan Vafai <ojan at chromium.org> has granted Adam Klein <adamk at chromium.org>'s
request for review:
Bug 71499: Only walk up the tree in search of MutationObservers if one has been
added
https://bugs.webkit.org/show_bug.cgi?id=71499

Attachment 113546: Patch
https://bugs.webkit.org/attachment.cgi?id=113546&action=review

------- Additional Comments from Ojan Vafai <ojan at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=113546&action=review


> Source/WebCore/ChangeLog:15
> +	   This could be improved upon to keep a count of each type, as
removing
> +	   an observer currently has no effect on m_mutationObserverTypes.
> +	   But that would require more space in Document.

So essentially, we do perf optimizations until you add any observer of a type.
After that, if you remove the observer, we don't get back the perf
optimization. As discussed in person, the reason we're not doing a count is
more about code complexity than memory savings since you don't actually have
that many documents in a page.

> Source/WebCore/dom/Node.cpp:2550
> +

Mind adding a FIXME to make the old mutation events update their relevant
document bits here as well?

> Source/WebCore/dom/Node.cpp:2744
> +    document()->addMutationObserverTypes(options |
WebKitMutationObserver::AllMutationTypes);

As discussed in person, we should only do this if it's a subtree observer.

> LayoutTests/fast/mutation/cross-document.html:6
> +<meta charset="utf-8">
> +<script src="../js/resources/js-test-pre.js"></script>
> +<title></title>

Please remove unnecessary bits. title and meta don't need to be here, right?

> LayoutTests/fast/mutation/cross-document.html:10
> +<p id=description></p>
> +<div id="console"></div>

description and console are no long required. js-test-pre will generate these
elements for you.

> LayoutTests/fast/mutation/cross-document.html:47
> +    start();

Stylistic nit: I wouldn't have a start method at all. Just inline the contents
of the start method here.