Bug 188575 – [IntersectionObserver] Do not hold a strong reference to the root element

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Ali Juma]

An IntersectionObserver should not keep its root element alive when there aren't any other references to the root.

[Ali Juma]

Created attachment 347106 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 347106 [details]
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.

[Ali Juma]

Created attachment 347159 [details]
Patch for landing

[Ali Juma]

Comment on attachment 347106 [details]
Patch

Thanks for the review!

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

>> Source/WebCore/page/IntersectionObserver.cpp:126
>> +            }
> 
> This should use removeFirst()

Done.

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

Updated the comment to mention that observers are owned by JavaScript wrappers and by IntersectionObserverRegistrations for currently-observed targets (to be implemented in a future patch).

[WebKit Commit Bot]

Comment on attachment 347159 [details]
Patch for landing

Clearing flags on attachment: 347159

Committed r234884: <https://trac.webkit.org/changeset/234884>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/43332185>