Bug 71577 – [MutationObservers] Refactor MutationObserverRegistration into it's own class that is referenced by registration points

• Component: DOM
• Status: RESOLVED FIXED

[Rafael Weinstein]

Right now, copies are made when transient registrations are created. This isn't terrible now, but when the registration needs to hold the attributeFilter hash, we want to avoid making lots of copies of that.

[Rafael Weinstein]

Created attachment 113690 [details]
Patch

[Ojan Vafai]

Comment on attachment 113690 [details]
Patch

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.

[Rafael Weinstein]

Comment on attachment 113690 [details]
Patch

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

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

done. missed from splitting out attributeFilter patch.

>> Source/WebCore/dom/MutationObserverRegistration.cpp:64
>> +
> 
> Nit: don't see the need for this line-break.

done

>> Source/WebCore/dom/MutationObserverRegistration.cpp:77
>> +        m_registrationNodeKeepAlive = m_registrationNode; // Balanced in clearTransientRegistrations.
> 
> Should this ASSERT(!m_registrationNodeKeepAlive)?

done

>> 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.

done

>> Source/WebCore/dom/MutationObserverRegistration.h:65
>> +    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.

removed

>> 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.

done

>> Source/WebCore/dom/Node.cpp:2762
>>      if (index == notFound)
> 
> Should this just be an assert as in 2780?

This needs to exit early if the ASSERT fails. The removal below will ignore the attempt to remove a non-existing element.

[Rafael Weinstein]

Created attachment 113724 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 113724 [details]
Patch for landing

Rejecting attachment 113724 [details] from commit-queue.

[e-mail] does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.

[Early Warning System Bot]

Comment on attachment 113724 [details]
Patch for landing

Attachment 113724 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10336102

[Rafael Weinstein]

Created attachment 113728 [details]
Patch

[WebKit Review Bot]

Comment on attachment 113728 [details]
Patch

Clearing flags on attachment: 113728

Committed r99338: <http://trac.webkit.org/changeset/99338>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.