[webkit-reviews] review denied: [Bug 68955] [MutationObservers] Implement WebKitMutationObserver.observe for childList changes : [Attachment 111535] Patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has denied Rafael Weinstein
<rafaelw at chromium.org>'s request for review:
Bug 68955: [MutationObservers] Implement WebKitMutationObserver.observe for
childList changes
https://bugs.webkit.org/show_bug.cgi?id=68955

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

------- Additional Comments from Ryosuke Niwa <rniwa at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=111535&action=review


> Source/WebCore/ChangeLog:8
> +	   Test: fast/mutation/observe-childList.html

Nit: This line should appear after the description.

> Source/WebCore/dom/MutationScope.cpp:46
> +namespace {

We don't normally use anonymous namespace in WebCore.

> Source/WebCore/dom/MutationScope.cpp:48
> +class ChildListAccumulation {

I'd call this class MutatedChildList.

> Source/WebCore/dom/MutationScope.cpp:73
> +class ChildListAccumulator {

I'd call this class ChildListMutationList.

> Source/WebCore/dom/MutationScope.cpp:112
> +	   dispatch();

I don't think scoping level is guaranteed to be 0 at this point. childAdded
could be called inside a mutation, load, unload, beforeunload, etc... event
listeners could have modified DOM. event listener. r- because of this.

> Source/WebCore/dom/MutationScope.cpp:118
> +	   ASSERT(m_nextSibling == child->nextSibling()); // Additions must be
in-order.

Why is this condition guaranteed?

> Source/WebCore/dom/MutationScope.cpp:131
> +    ASSERT(isEmpty() || m_nextSibling.get() == child); // Removals must be
continuous and in-order.

Again, I don't quite understand what guarantees this condition.

> Source/WebCore/dom/MutationScope.cpp:153
> +    RefPtr<MutationRecord> mutation =
MutationRecord::createChildList(m_target, StaticNodeList::adopt(m_addedNodes),
StaticNodeList::adopt(m_removedNodes), m_previousSibling, m_nextSibling);

Maybe split it into two lines?

> Source/WebCore/dom/MutationScope.cpp:155
> +    for (Vector<WebKitMutationObserver*>::iterator iter =
m_observers.begin(); iter != m_observers.end(); ++iter)
> +	   (*iter)->enqueueMutationRecord(mutation);

Please use size_t i instead.

> Source/WebCore/dom/MutationScope.cpp:182
> +// Can this get called

I don't think this comment adds any information. Remove?

> Source/WebCore/dom/MutationScope.cpp:193
> +    OwnPtr<ChildListAccumulator> instance = adoptPtr(new
ChildListAccumulator);
> +    s_instance = instance.leakPtr();

This can be done in one line:
s_instance = adoptPtr(new ChildListAccumulator).leakPtr().

> Source/WebCore/dom/MutationScope.cpp:207
> +    ASSERT(m_accumulations.contains(target));

Why can we assert that m_accumulations always contain the target?

> Source/WebCore/dom/MutationScope.cpp:215
> +    ASSERT(m_accumulations.contains(target));

Ditto.

> Source/WebCore/dom/MutationScope.cpp:251
> +    if (record)
> +	   record->dispatch();

I'm getting confused about this. Why do we need this scoping object all if
we're implemented the end-of-micro-task timing?