[webkit-reviews] review granted: [Bug 71577] [MutationObservers] Refactor MutationObserverRegistration into it's own class that is referenced by registration points : [Attachment 113690] Patch

[bugzilla-daemon at webkit.org]

Ojan Vafai <ojan at chromium.org> has granted Rafael Weinstein
<rafaelw at chromium.org>'s request for review:
Bug 71577: [MutationObservers] Refactor MutationObserverRegistration into it's
own class that is referenced by registration points
https://bugs.webkit.org/show_bug.cgi?id=71577

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

------- Additional Comments from Ojan Vafai <ojan at chromium.org>
Just a bunch of comment and/or other nits.

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


> Source/WebCore/ChangeLog:11
> +	   MutationObserverRegistration is now owned by the node which is
observed. If transient
> +	   registrations are created, they hold a reference to this object.

This is the appropriate place for all your comments in the code. :) Would be
good if you could make this description more detailed. Specifically, a
condensed version of the description you gave to me in person about how this
works would be perfect.

> Source/WebCore/dom/ChildListMutationScope.cpp:44
> +#include <wtf/text/AtomicString.h>

Is this needed?

> Source/WebCore/dom/MutationObserverRegistration.cpp:64
> +

Nit: don't see the need for this line-break.

> Source/WebCore/dom/MutationObserverRegistration.cpp:77
> +	   m_transientRegistrationNodes = adoptPtr(new NodeHashSet);
> +	   m_registrationNodeKeepAlive = m_registrationNode; // Balanced in
clearTransientRegistrations.

Should this ASSERT(!m_registrationNodeKeepAlive)?

> Source/WebCore/dom/MutationObserverRegistration.cpp:99
> +    m_registrationNode->unregisterMutationObserver(this); // This call will
cause this object to be deleted.

Webkit is generally anti "what" comments. Here and above. What happens when the
code is refactored so this is no longer true? It's very likely whoever does the
refactor won't notice this comment.

I needed your in person descriptions to understand these comments anyways. :)

Might be worth making the comment a bit more explicit, like:
m_registrationNode->unregisterMutationObserver(this);
// The above line causes this object to be deleted, so don't do any more in
this function.

> Source/WebCore/dom/MutationObserverRegistration.h:65
> +    Node* m_registrationNode; // Back-pointer to the Node which owns this
registration.
> +    RefPtr<Node> m_registrationNodeKeepAlive; // This registration takes
reference to its owning node while it has transient registrations.

These comments aren't super useful. Again, without you explaining to me how
this all hooks together, these comments didn't actually help me understand it.

> Source/WebCore/dom/Node.cpp:2736
> +	   collectMatchingObserversForMutation(observers, node, type);

Nit: I don't really see the benefit of moving this into a helper function. I'd
just inline the function code here. Your call though.

> Source/WebCore/dom/Node.cpp:2762
>      size_t index = registry->find(registration);
>      ASSERT(index != notFound);
>      if (index == notFound)

Should this just be an assert as in 2780?

> Source/WebCore/dom/WebKitMutationObserver.h:88
> +    HashSet<MutationObserverRegistration*> m_registrations; // Back-pointers
to registrations which are holding references to this observer.

ditto re: comment.