[webkit-reviews] review granted: [Bug 70436] [MutationObservers] Implement basic subtree observation : [Attachment 112009] Merged to r98133

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has granted Adam Klein <adamk at chromium.org>'s
request for review:
Bug 70436: [MutationObservers] Implement basic subtree observation
https://bugs.webkit.org/show_bug.cgi?id=70436

Attachment 112009: Merged to r98133
https://bugs.webkit.org/attachment.cgi?id=112009&action=review

------- Additional Comments from Ryosuke Niwa <rniwa at webkit.org>
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.