Bug 208047

Summary: [intersection-observer] Accept a Document as an explicit root
Product: WebKit Reporter: Frédéric Wang (:fredw) <fred.wang>
Component: WebCore Misc.Assignee: Frédéric Wang (:fredw) <fred.wang>
Status: RESOLVED FIXED    
Severity: Normal CC: ajuma, annulen, cathiechen, cdumez, cmarcelo, commit-queue, darin, dbates, esprehn+autocc, ews-watchlist, gyuyoung.kim, kangil.han, keith_miller, kondapallykalyan, mark.lam, msaboff, rbuis, ryuan.choi, sbarati, sergio, simon.fraser, tzagallo, webkit-bug-importer
Priority: P2 Keywords: InRadar, WebExposed
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugzilla.mozilla.org/show_bug.cgi?id=1617154
Bug Depends on: 208053    
Bug Blocks: 159475    
Attachments:
Description Flags
IDL changes
none
Patch (WIP)
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
simon.fraser: review+
Patch for landing
none
patch for ews
none
Patch (address post-landing review comment)
none
Patch (address post-landing review comment)
none
Patch (address post-landing review comment)
none
Patch for landing none

Comment 1 Frédéric Wang (:fredw) 2020-02-21 08:22:37 PST
Created attachment 391401 [details]
Patch (WIP)
Comment 2 Frédéric Wang (:fredw) 2020-02-22 03:48:31 PST
Created attachment 391458 [details]
Patch
Comment 3 Frédéric Wang (:fredw) 2020-02-24 00:08:21 PST
Created attachment 391516 [details]
Patch
Comment 4 Ali Juma 2020-02-24 07:06:02 PST
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 5 Frédéric Wang (:fredw) 2020-02-24 23:41:17 PST
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.
Comment 6 Frédéric Wang (:fredw) 2020-02-25 02:15:18 PST
Created attachment 391642 [details]
Patch
Comment 7 Frédéric Wang (:fredw) 2020-02-25 02:19:10 PST
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 8 Simon Fraser (smfr) 2020-03-03 13:14:04 PST
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 9 Frédéric Wang (:fredw) 2020-03-03 19:46:30 PST
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).
Comment 10 Frédéric Wang (:fredw) 2020-03-03 23:22:19 PST
Created attachment 392379 [details]
Patch
Comment 11 Simon Fraser (smfr) 2020-03-04 11:06:45 PST
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 12 Frédéric Wang (:fredw) 2020-03-04 12:43:07 PST
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.
Comment 13 Frédéric Wang (:fredw) 2020-03-05 01:09:42 PST
Created attachment 392546 [details]
Patch
Comment 14 Simon Fraser (smfr) 2020-03-05 15:47:44 PST
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.
Comment 15 Frédéric Wang (:fredw) 2020-03-06 02:54:54 PST
Created attachment 392698 [details]
Patch for landing
Comment 16 WebKit Commit Bot 2020-03-06 03:53:14 PST
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 17 WebKit Commit Bot 2020-03-06 03:53:53 PST
Comment on attachment 392698 [details]
Patch for landing

Clearing flags on attachment: 392698

Committed r257976: <https://trac.webkit.org/changeset/257976>
Comment 18 WebKit Commit Bot 2020-03-06 03:53:56 PST
All reviewed patches have been landed.  Closing bug.
Comment 19 Radar WebKit Bug Importer 2020-03-06 03:54:22 PST
<rdar://problem/60141273>
Comment 20 Darin Adler 2020-03-06 10:03:22 PST
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?
Comment 21 Frédéric Wang (:fredw) 2020-03-17 07:31:13 PDT
Created attachment 393752 [details]
patch for ews
Comment 22 Frédéric Wang (:fredw) 2020-03-18 00:07:47 PDT
Reopening to attach new patch
Comment 23 Frédéric Wang (:fredw) 2020-03-18 00:09:40 PDT
Created attachment 393823 [details]
Patch (address post-landing review comment)
Comment 24 Frédéric Wang (:fredw) 2020-03-18 00:17:07 PDT
Created attachment 393824 [details]
Patch (address post-landing review comment)
Comment 25 Frédéric Wang (:fredw) 2020-03-18 00:35:55 PDT
Created attachment 393826 [details]
Patch (address post-landing review comment)
Comment 26 Rob Buis 2020-03-18 09:49:29 PDT
Comment on attachment 393826 [details]
Patch (address post-landing review comment)

LGTM.
Comment 27 Frédéric Wang (:fredw) 2020-03-18 10:01:44 PDT
Created attachment 393873 [details]
Patch for landing
Comment 28 WebKit Commit Bot 2020-03-18 10:55:17 PDT
Comment on attachment 393873 [details]
Patch for landing

Clearing flags on attachment: 393873

Committed r258648: <https://trac.webkit.org/changeset/258648>
Comment 29 WebKit Commit Bot 2020-03-18 10:55:19 PDT
All reviewed patches have been landed.  Closing bug.
Comment 30 Darin Adler 2020-03-18 12:09:09 PDT
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.