[webkit-reviews] review denied: [Bug 70788] [MutationObservers] Implement subtree observation of transiently disconnected nodes : [Attachment 112626] 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 70788: [MutationObservers] Implement subtree observation of transiently
disconnected nodes
https://bugs.webkit.org/show_bug.cgi?id=70788

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

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


I would have r+ed if you were a committer but sadly r- for now.

> Source/WebCore/dom/Node.cpp:553
> +    Vector<MutationObserverRegistration>* registry =
mutationObserverRegistry();
> +    if (registry) {

registry should be declared inside if as in:
if (Vector<MutationObserverRegistration>* registry =
mutationObserverRegistry()) {

> Source/WebCore/dom/Node.cpp:2718
> +	       pair<HashMap<WebKitMutationObserver*,
MutationObserverOptions>::iterator, bool> result =
observers.add(entry.observer.get(), entry.options);
> +	       if (!result.second)
> +		   result.first->second |= entry.options;

This code is unreadable to me. I'd prefer approach like:
if (HashMap<WebKitMutationObserver*, MutationObserverOptions>::iterator it =
observers.find(entry.observer.get()))
    it->options |= entry.options;
else
    observers.add(entry.observer.get(), entry.options);
This code shouldn't be much slower since we're using a hash map but it reads
much better.

> Source/WebCore/dom/Node.cpp:2732
> +    Vector<MutationObserverRegistration>* registry =
ensureRareData()->ensureMutationObserverRegistry();

Can we define a reference here instead?

> Source/WebCore/dom/Node.cpp:2741
> +    (*registry)[index].options = registration.options;

So that we can avoid these awkward de-referencing.

> Source/WebCore/dom/Node.cpp:2761
> +void Node::createTransientMutationObservers()

Hm... this function isn't really creating observers though. How about
registerTransientMutationObservations or startTransientMutationObservations?

> Source/WebCore/dom/Node.cpp:2774
> +	       RefPtr<Node> observedNode = registration.registrationNode;

We can use a raw pointer here, can't we?

> Source/WebCore/dom/WebKitMutationObserver.cpp:59
> +static void unregisterTransientRegistrations(PassOwnPtr<NodeHashSet>
popTransientNodes, WebKitMutationObserver* observer, Node* registrationNode)

inline keyword?

> Source/WebCore/dom/WebKitMutationObserver.cpp:61
> +    OwnPtr<NodeHashSet> transientNodes = popTransientNodes;

We don't need this local variable. This is a two-line function and the
correctness is clear without it.

> Source/WebCore/dom/WebKitMutationObserver.cpp:141
> +void WebKitMutationObserver::clearTransientEntries()
> +{
> +    for (HashMap<RefPtr<Node>, NodeHashSet*>::iterator iter =
m_transientObservedNodes.begin(); iter != m_transientObservedNodes.end();
++iter)
> +	   unregisterTransientRegistrations(adoptPtr(iter->second), this,
iter->first.get());
> +
> +    m_transientObservedNodes.clear();
> +}

There are bunch of different naming conventions used here.
clearTransientEntries, m_transientObservedNodes,
unregisterTransientRegistrations.
We should make them consistent. How about m_transientObservedNodes,
clearAllTransientObservations,
stopTransientObservations/unregisterTransientObservers?
I'd like to avoid using the term "registration" for the third one since that
term is associated with the rare data in Node and the function happens to
destroy NodeHashSet as well.

> LayoutTests/ChangeLog:7
> +

Would you care to describe what kind of test cases you're adding here?

> LayoutTests/fast/mutation/observe-subtree.html:161
> +	   debug('...both changes should be received. Change disconnected
subDiv again.');

Can we s/disconnect/detach/g?

> LayoutTests/fast/mutation/observe-subtree.html:190
> +	   debug('...reattached subtree should now be observable. Try
disconnecting and re-observing.');

Ditto.