Summary: | [IntersectionObserver] Do not hold a strong reference to the root element | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Ali Juma <ajuma> | ||||||
Component: | Layout and Rendering | Assignee: | Ali Juma <ajuma> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | bfulgham, cdumez, cmarcelo, commit-queue, dbates, esprehn+autocc, ews-watchlist, kangil.han, simon.fraser, webkit-bug-importer, zalan | ||||||
Priority: | P2 | Keywords: | InRadar | ||||||
Version: | WebKit Nightly Build | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Bug Depends on: | |||||||||
Bug Blocks: | 159475 | ||||||||
Attachments: |
|
Description
Ali Juma
2018-08-14 13:25:21 PDT
Created attachment 347106 [details]
Patch
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. Created attachment 347159 [details]
Patch for landing
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). Comment on attachment 347159 [details] Patch for landing Clearing flags on attachment: 347159 Committed r234884: <https://trac.webkit.org/changeset/234884> All reviewed patches have been landed. Closing bug. |