Created attachment 391392 [details] IDL changes Spec allows a Node: https://w3c.github.io/IntersectionObserver/#dom-intersectionobserver-root Discussion: https://github.com/w3c/IntersectionObserver/issues/372 Chromium change: https://chromium-review.googlesource.com/c/chromium/src/+/2003750
Created attachment 391401 [details] Patch (WIP)
Created attachment 391458 [details] Patch
Created attachment 391516 [details] Patch
Comment on attachment 391516 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=391516&action=review Thanks for working on this! The patch generally looks great. A few comments: > Source/JavaScriptCore/ChangeLog:7 > + IntersectionObserverDocumentAsExplicitRootEnabled that enables a recent Do we need a flag for this? It seems like a small change that has already shipped in another browser, so adding a flag probably adds more complexity than it's worth. > Source/WebCore/dom/Document.cpp:7719 > + Document's destructor needs to iterate over m_documentExplicitIntersectionObserverData and call observer->rootDestroyed() on each one, just like Element's destructor does (see Element::disconnectFromIntersectionObservers()). > Source/WebCore/page/IntersectionObserver.cpp:145 > +Document* IntersectionObserver::trackingDocument() const I believe the existing implementation (in the ..h) file will already do what's needed, since m_root->document() will return itself when m_root is a Document. > Source/WebCore/page/IntersectionObserver.idl:51 > + // FIXME(fwang): Ideally, this should be written '(Element or Document)? = null' but that's not properly supported by WebKit's IDL generator. Please file a bug for this if you haven't already.
Comment on attachment 391516 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=391516&action=review >> Source/JavaScriptCore/ChangeLog:7 >> + IntersectionObserverDocumentAsExplicitRootEnabled that enables a recent > > Do we need a flag for this? It seems like a small change that has already shipped in another browser, so adding a flag probably adds more complexity than it's worth. Yes, I'm not really sure about this. Let's see what other reviewers say. >> Source/WebCore/dom/Document.cpp:7719 >> + > > Document's destructor needs to iterate over m_documentExplicitIntersectionObserverData and call observer->rootDestroyed() on each one, just like Element's destructor does (see Element::disconnectFromIntersectionObservers()). Good catch! I guess this will depend on where the this is stored at the end. > Source/WebCore/dom/Document.h:1829 > + // FIXME(fwang): Make this a rare data? I guess that's more a question for reviewers than a follow-up task. Basically I see three options: (1) Keep it as it (increase the size) (2) Use rare data in Node (maybe move the Element's one too? that might affect the size of other Nodes) (3) New rare data in Document (that will still increase the size though) >> Source/WebCore/page/IntersectionObserver.cpp:145 >> +Document* IntersectionObserver::trackingDocument() const > > I believe the existing implementation (in the ..h) file will already do what's needed, since m_root->document() will return itself when m_root is a Document. I think you are correct. I was not sure whether I would have to do some special handling for the Document case, that's why I move it to cpp, but that does not seem necessary. >> Source/WebCore/page/IntersectionObserver.idl:51 >> + // FIXME(fwang): Ideally, this should be written '(Element or Document)? = null' but that's not properly supported by WebKit's IDL generator. > > Please file a bug for this if you haven't already. Yes, I'll take care of that.
Created attachment 391642 [details] Patch
Comment on attachment 391516 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=391516&action=review >>> Source/WebCore/dom/Document.cpp:7719 >>> + >> >> Document's destructor needs to iterate over m_documentExplicitIntersectionObserverData and call observer->rootDestroyed() on each one, just like Element's destructor does (see Element::disconnectFromIntersectionObservers()). > > Good catch! I guess this will depend on where the this is stored at the end. Done. >> Source/WebCore/dom/Document.h:1829 >> + // FIXME(fwang): Make this a rare data? > > I guess that's more a question for reviewers than a follow-up task. Basically I see three options: > (1) Keep it as it (increase the size) > (2) Use rare data in Node (maybe move the Element's one too? that might affect the size of other Nodes) > (3) New rare data in Document (that will still increase the size though) For now the patch still uses (1). I noticed that IntersectionObserverRegistration is useless for Document, though. >>> Source/WebCore/page/IntersectionObserver.cpp:145 >>> +Document* IntersectionObserver::trackingDocument() const >> >> I believe the existing implementation (in the ..h) file will already do what's needed, since m_root->document() will return itself when m_root is a Document. > > I think you are correct. I was not sure whether I would have to do some special handling for the Document case, that's why I move it to cpp, but that does not seem necessary. Done. >>> Source/WebCore/page/IntersectionObserver.idl:51 >>> + // FIXME(fwang): Ideally, this should be written '(Element or Document)? = null' but that's not properly supported by WebKit's IDL generator. >> >> Please file a bug for this if you haven't already. > > Yes, I'll take care of that. OK, I was finally able to do the proper C++ changes so that we can use the same IDL as the spec.
Comment on attachment 391642 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=391642&action=review Let's do a patch without the flag. > Source/JavaScriptCore/ChangeLog:6 > + This patch introduces a new runtime flag I don't think we need a runtime flag. > Source/JavaScriptCore/ChangeLog:13 > + Reviewed by NOBODY (OOPS!). This line goes above the description (prepare-Changelogs does this for you). > Source/WebCore/dom/Document.cpp:7716 > +IntersectionObserverData& Document::ensureDocumentExplicitIntersectionObserverData() Just intersectionObserverData > Source/WebCore/dom/Document.cpp:7723 > +IntersectionObserverData* Document::documentExplicitIntersectionObserverData() intersectionObserverDataIfExists() > Source/WebCore/dom/Document.cpp:7725 > + return m_documentExplicitIntersectionObserverData ? m_documentExplicitIntersectionObserverData.get() : nullptr; Just return m_documentExplicitIntersectionObserverData; > Source/WebCore/dom/Document.h:1830 > + // FIXME(fwang): Make this a rare data? > + std::unique_ptr<IntersectionObserverData> m_documentExplicitIntersectionObserverData; Adding one pointer to Document is fine. The "documentExplicit" name is confusing. Maybe just m_intersectionObserverData? Or m_documentRootIntersectionObserverData
Comment on attachment 391642 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=391642&action=review >> Source/WebCore/dom/Document.h:1830 >> + std::unique_ptr<IntersectionObserverData> m_documentExplicitIntersectionObserverData; > > Adding one pointer to Document is fine. > > The "documentExplicit" name is confusing. Maybe just m_intersectionObserverData? Or m_documentRootIntersectionObserverData I think the idea is that if you don't specify the { root: ... } option, the root is implicitly the topmost document and in that case the observer data is not needed on that document (as that's the case now).
Created attachment 392379 [details] Patch
Comment on attachment 392379 [details] 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.
Comment on attachment 392379 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=392379&action=review >> Source/WebCore/dom/Document.cpp:640 >> + observer->rootDestroyed(); > > Observers are weak references, so you need to null-check each one. OK, note that this cleanup code as well as the version of intersectionObserverDataIfExists() from the previous patch were copied from what we currently do for Element. So I guess I should fix these as well? >> 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. OK, I'll change the signature. I feel silly to ask but how do you see that a crash can happen? As I see this function is only called in the destructor of IntersectionObserver when document is an explicit root, and in that case the constructor would have called ensureIntersectionObserverData() for the same document. Currently, the only WPT test with an explicit document root is with only one explicit document root and only one observer. That can definitely be improved to have multiple observer and multiple explicit document roots and different lifetime of objetcs, but I still don't see which configuration would cause a crash in the current patch. >> 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. Right, will do. That was the purpose of the former "document explicit" name.
Created attachment 392546 [details] Patch
Comment on attachment 392546 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=392546&action=review > Source/WebCore/page/IntersectionObserver.cpp:126 > + auto& observerData = downcast<Document>(m_root)->ensureIntersectionObserverData(); Nicer as: downcast<Document>(*m_root).ensureIntersectionObserverData(); since the is<> null-checks. > Source/WebCore/page/IntersectionObserver.cpp:130 > + auto& observerData = downcast<Element>(m_root)->ensureIntersectionObserverData(); Ditto. > Source/WebCore/page/IntersectionObserver.cpp:142 > + downcast<Document>(m_root)->intersectionObserverDataIfExists()->observers.removeFirst(this); Ditto. > Source/WebCore/page/IntersectionObserver.cpp:145 > + downcast<Element>(m_root)->intersectionObserverDataIfExists()->observers.removeFirst(this); Ditto.
Created attachment 392698 [details] Patch for landing
The commit-queue encountered the following flaky tests while processing attachment 392698 [details]: editing/spelling/spellcheck-async-remove-frame.html bug 158401 (authors: morrita@google.com, rniwa@webkit.org, and tony@chromium.org) The commit-queue is continuing to process your patch.
Comment on attachment 392698 [details] Patch for landing Clearing flags on attachment: 392698 Committed r257976: <https://trac.webkit.org/changeset/257976>
All reviewed patches have been landed. Closing bug.
<rdar://problem/60141273>
Comment on attachment 392698 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=392698&action=review > Source/WebCore/page/IntersectionObserver.h:77 > + Node* root() const { return m_root; } How about ContainerNode instead of Node?
Created attachment 393752 [details] patch for ews
Reopening to attach new patch
Created attachment 393823 [details] Patch (address post-landing review comment)
Created attachment 393824 [details] Patch (address post-landing review comment)
Created attachment 393826 [details] Patch (address post-landing review comment)
Comment on attachment 393826 [details] Patch (address post-landing review comment) LGTM.
Created attachment 393873 [details] Patch for landing
Comment on attachment 393873 [details] Patch for landing Clearing flags on attachment: 393873 Committed r258648: <https://trac.webkit.org/changeset/258648>
Comment on attachment 393873 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=393873&action=review > Source/WebCore/page/IntersectionObserver.h:44 > +class ContainerNode; Should move to the top to re-sort at some point.
*** Bug 224324 has been marked as a duplicate of this bug. ***