[webkit-reviews] review granted: [Bug 134869] Document::unregisterNodeListForInvalidation() incorrectly cooperates with Document::invalidateNodeListAndCollectionCaches() : [Attachment 234829] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Zan Dobersek
<zandobersek at gmail.com>'s request for review:
Bug 134869: Document::unregisterNodeListForInvalidation() incorrectly
cooperates with Document::invalidateNodeListAndCollectionCaches()
https://bugs.webkit.org/show_bug.cgi?id=134869

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

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=234829&action=review


> Source/WebCore/dom/Document.cpp:3498
> +    ASSERT(m_inInvalidateNodeListAndCollectionCaches &&
!m_listsInvalidatedAtDocument.size()
> +	   || m_listsInvalidatedAtDocument.contains(&list));
> +    // There's no harm in removing an item that perhaps is not in the
HashSet.
> +    m_listsInvalidatedAtDocument.remove(&list);

This looks OK, but I think it’s confusing. I would write this instead:

    ASSERT(m_inInvalidateNodeListAndCollectionCaches ?
m_listsInvalidatedAtDocument.isEmpty() :
m_listsInvalidatedAtDocument.contains(&list));
    m_listsInvalidatedAtDocument.remove(&list);

For some people the ?: might make it harder to read the assertion, but to me it
makes it easier.

It also seems that unregisterCollection has the same bug. The two functions
should be basically identical, but there are many arbitrary differences between
which assertions they have and how the functions are structured.

Please fix the bug in unregisterCollection too.