[webkit-reviews] review granted: [Bug 216528] MutationObserverRegistration should be ref counted : [Attachment 408795] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Ryosuke Niwa <rniwa at webkit.org>'s
request for review:
Bug 216528: MutationObserverRegistration should be ref counted
https://bugs.webkit.org/show_bug.cgi?id=216528

Attachment 408795: Patch

https://bugs.webkit.org/attachment.cgi?id=408795&action=review




--- Comment #3 from Darin Adler <darin at apple.com> ---
Comment on attachment 408795
  --> https://bugs.webkit.org/attachment.cgi?id=408795
Patch

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

I’m sort of surprised by how many RELEASE_ASSERT there are in this patch.

> Source/WebCore/dom/MutationObserver.cpp:127
> +	   return std::pair<Ref<Node>, Ref<MutationObserverRegistration>> {
node.releaseNonNull(), makeRef(*registration) };

Using two sets of braces would be nicer than having type write out almost the
entire return type a second time:

    return { { node.releaseNonNull(), makeRef(*registration) } };

> Source/WebCore/dom/MutationObserver.cpp:136
> +    RELEASE_ASSERT(!m_registrations.contains(&registration));
>      m_registrations.add(&registration);

Since we are doing a RELEASE_ASSERT, please avoid double hashing by asserting
based on the return value of add rather than a separate hash table lookup.

> Source/WebCore/dom/MutationObserver.cpp:142
> +    RELEASE_ASSERT(m_registrations.contains(&registration));
>      m_registrations.remove(&registration);

Since we are doing a RELEASE_ASSERT, please avoid double hashing by asserting
based on the return value of remove rather than a separate hash table lookup.

> Source/WebCore/dom/NodeRareData.h:278
> +    static constexpr size_t MutationObserverRegistryInlineSize = 4;

I suggest defining a type with this size in it rather than writing out the type
every time:

    using RegistryVector = Vector<Ref<MutationObserverRegistration>, 4>;
    using RegistrySet= HashSet<Ref<MutationObserverRegistration>>;

The code will be less repetitive and might even be easier to read.