Bug 208047 – [intersection-observer] Accept a Document as an explicit root

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Frédéric Wang (:fredw)]

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

[Frédéric Wang (:fredw)]

Created attachment 391401 [details]
Patch (WIP)

[Frédéric Wang (:fredw)]

Created attachment 391458 [details]
Patch

[Frédéric Wang (:fredw)]

Created attachment 391516 [details]
Patch

[Ali Juma]

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.

[Frédéric Wang (:fredw)]

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.

[Frédéric Wang (:fredw)]

Created attachment 391642 [details]
Patch

[Frédéric Wang (:fredw)]

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.

[Simon Fraser (smfr)]

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

[Frédéric Wang (:fredw)]

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

[Frédéric Wang (:fredw)]

Created attachment 392379 [details]
Patch

[Simon Fraser (smfr)]

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.

[Frédéric Wang (:fredw)]

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.

[Frédéric Wang (:fredw)]

Created attachment 392546 [details]
Patch

[Simon Fraser (smfr)]

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.

[Frédéric Wang (:fredw)]

Created attachment 392698 [details]
Patch for landing

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 392698 [details]:

editing/spelling/spellcheck-async-remove-frame.html bug 158401 (authors: [e-mail], [e-mail], and [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 392698 [details]
Patch for landing

Clearing flags on attachment: 392698

Committed r257976: <https://trac.webkit.org/changeset/257976>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/60141273>

[Darin Adler]

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?

[Frédéric Wang (:fredw)]

Created attachment 393752 [details]
patch for ews

[Frédéric Wang (:fredw)]

Reopening to attach new patch

[Frédéric Wang (:fredw)]

Created attachment 393823 [details]
Patch (address post-landing review comment)

[Frédéric Wang (:fredw)]

Created attachment 393824 [details]
Patch (address post-landing review comment)

[Frédéric Wang (:fredw)]

Created attachment 393826 [details]
Patch (address post-landing review comment)

[Rob Buis]

Comment on attachment 393826 [details]
Patch (address post-landing review comment)

LGTM.

[Frédéric Wang (:fredw)]

Created attachment 393873 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 393873 [details]
Patch for landing

Clearing flags on attachment: 393873

Committed r258648: <https://trac.webkit.org/changeset/258648>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

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.

[Simon Fraser (smfr)]

*** Bug 224324 has been marked as a duplicate of this bug. ***