[webkit-reviews] review denied: [Bug 208047] [intersection-observer] Accept a Document as an explicit root : [Attachment 392379] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has denied Frédéric Wang (:fredw)
<fred.wang at free.fr>'s request for review:
Bug 208047: [intersection-observer] Accept a Document as an explicit root
https://bugs.webkit.org/show_bug.cgi?id=208047

Attachment 392379: Patch

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




--- Comment #11 from Simon Fraser (smfr) <simon.fraser at apple.com> ---
Comment on attachment 392379
  --> https://bugs.webkit.org/attachment.cgi?id=392379
Patch

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

> Source/WebCore/dom/Document.cpp:638
> +	   // Inform observers that this document is destroyed.

Comment is not necessary.

> Source/WebCore/dom/Document.cpp:640
> +	   for (const auto& observer : m_intersectionObserverData->observers)
> +	       observer->rootDestroyed();

Observers are weak references, so you need to null-check each one.

> Source/WebCore/dom/Document.h:1394
> +    IntersectionObserverData& intersectionObserverDataIfExists() { return
*m_intersectionObserverData; }

This has to return a pointer!

You don't have crashing tests, so this indicates that test coverage is
insufficient.

> Source/WebCore/dom/Document.h:1829
> +    std::unique_ptr<IntersectionObserverData> m_intersectionObserverData;

You could add  a comment saying that this is only non-null when the document is
an explicit root.