[webkit-reviews] review granted: [Bug 188575] [IntersectionObserver] Do not hold a strong reference to the root element : [Attachment 347106] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has granted Ali Juma
<ajuma at chromium.org>'s request for review:
Bug 188575: [IntersectionObserver] Do not hold a strong reference to the root
element
https://bugs.webkit.org/show_bug.cgi?id=188575

Attachment 347106: Patch

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




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

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

> Source/WebCore/page/IntersectionObserver.cpp:126
> +	   for (size_t i = 0; i < elementObservers.size(); ++i) {
> +	       if (elementObservers.at(i) == this) {
> +		   elementObservers.remove(i);
> +		   break;
> +	       }

This should use removeFirst()

> Source/WebCore/page/IntersectionObserver.h:43
> +    Vector<IntersectionObserver*> observers;

Who owns the IntersectionObservers? Would be nice for the comment to say what
the ownership model is.

> Source/WebCore/page/IntersectionObserver.h:75
> +    Element* m_root;

We should make WeakPtr work for Elements.