Bug 188670 - [IntersectionObserver] Fire an initial dummy notification
Summary: [IntersectionObserver] Fire an initial dummy notification
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ali Juma
URL:
Keywords: InRadar
Depends on:
Blocks: 159475 192661
  Show dependency treegraph
 
Reported: 2018-08-16 12:12 PDT by Ali Juma
Modified: 2018-12-13 04:10 PST (History)
12 users (show)

See Also:


Attachments
Patch (54.63 KB, patch)
2018-08-16 12:32 PDT, Ali Juma
no flags Details | Formatted Diff | Diff
Patch (54.69 KB, patch)
2018-08-16 12:35 PDT, Ali Juma
no flags Details | Formatted Diff | Diff
Patch (55.22 KB, patch)
2018-08-17 14:13 PDT, Ali Juma
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews206 for win-future (12.91 MB, application/zip)
2018-08-18 01:59 PDT, Build Bot
no flags Details
Patch for landing (55.50 KB, patch)
2018-08-18 15:13 PDT, Ali Juma
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ali Juma 2018-08-16 12:12:33 PDT
Add logic to track ongoing intersection observations and fire an initial dummy notification for each one.
Comment 1 Ali Juma 2018-08-16 12:32:50 PDT
Created attachment 347287 [details]
Patch
Comment 2 Ali Juma 2018-08-16 12:35:41 PDT
Created attachment 347289 [details]
Patch
Comment 3 Simon Fraser (smfr) 2018-08-16 13:30:56 PDT
Comment on attachment 347289 [details]
Patch

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

> Source/WebCore/dom/Document.cpp:7451
> +        for (auto* target : observer->observationTargets()) {

The auto for target hurts here. I think this is an Element* but I can't easily tell here.

> Source/WebCore/dom/Document.cpp:7490
> +    for (auto* observer : m_activeIntersectionObservers)
> +        observer->notify();

So this is notifying all active observers, but one some of them have a queued entry?

> Source/WebCore/dom/Document.h:1371
> +    void addViewportIntersectionObserver(IntersectionObserver&);
> +    void removeViewportIntersectionObserver(IntersectionObserver&);

These also need to reduce confusion between 'active' and 'viewport' observers.

> Source/WebCore/dom/Document.h:1779
> +    HashSet<IntersectionObserver*> m_activeIntersectionObservers;
> +    HashSet<IntersectionObserver*> m_viewportIntersectionObservers;

These names are confusing. What's the difference between 'active' and 'viewport'? Is the latter a superset? If so, I'd just call it m_IntersectionObservers

The raw pointers are also concerning; this would be a another place where a note about ownership is valid. Maybe we should use WeakPtrs here.

> Source/WebCore/page/IntersectionObserver.cpp:157
> +    // Set previousThresholdIndex to a sentinel value that ensures the first computed threshold
> +    // index will be different, triggering an initial notification.

Normally we use std::optional<> for this kind of thing.

> Source/WebCore/page/IntersectionObserver.h:39
> +class Document;

I stopped reviewing here.
Comment 4 Ali Juma 2018-08-17 14:13:12 PDT
Created attachment 347389 [details]
Patch

Address review comments
Comment 5 Simon Fraser (smfr) 2018-08-17 14:21:34 PDT
Comment on attachment 347389 [details]
Patch

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

> Source/WebCore/dom/Document.cpp:7433
> +    for (auto observer : m_intersectionObservers) {

m_intersectionObservers is a HashSet, so order is not guaranteed. Does the API require that observers fire in registration order? Even if not, I think we should make order predictable, so maybe a ListHashSet, or just a Vector if we think the cost of checking for duplicates is low.  Do we know how many observers common pages have?

> Source/WebCore/dom/Document.h:1777
> +    HashSet<RefPtr<IntersectionObserver>> m_intersectionObservers;
> +    Vector<WeakPtr<IntersectionObserver>> m_intersectionObserversWithPendingNotifications;

Much better.
Comment 6 Ali Juma 2018-08-17 14:23:44 PDT
Comment on attachment 347289 [details]
Patch

Thanks for the review! I've changed the ownership model to (hopefully) simplify the logic. IntersectionObservers with at least one target are now owned by their trackingDocument (which, for observers with a root element, is the root's document, and otherwise is the main frame's document). IntersectionObservers with no targets are only owned by JS wrappers. So Document stores RefPtrs to observers it now owns, and IntersectionObserverData stores WeakPtrs.

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

>> Source/WebCore/dom/Document.cpp:7451
>> +        for (auto* target : observer->observationTargets()) {
> 
> The auto for target hurts here. I think this is an Element* but I can't easily tell here.

Fixed.

>> Source/WebCore/dom/Document.cpp:7490
>> +        observer->notify();
> 
> So this is notifying all active observers, but one some of them have a queued entry?

Changed to only iterating over observers that have pending notifications.

>> Source/WebCore/dom/Document.h:1371
>> +    void removeViewportIntersectionObserver(IntersectionObserver&);
> 
> These also need to reduce confusion between 'active' and 'viewport' observers.

Changed so there's only a single kind of observer.

>> Source/WebCore/dom/Document.h:1779
>> +    HashSet<IntersectionObserver*> m_viewportIntersectionObservers;
> 
> These names are confusing. What's the difference between 'active' and 'viewport'? Is the latter a superset? If so, I'd just call it m_IntersectionObservers
> 
> The raw pointers are also concerning; this would be a another place where a note about ownership is valid. Maybe we should use WeakPtrs here.

Got rid of m_viewportIntersectionObservers, and renamed the other one to just m_intersectionObservers. This now stores RefPtrs.

>> Source/WebCore/page/IntersectionObserver.cpp:157
>> +    // index will be different, triggering an initial notification.
> 
> Normally we use std::optional<> for this kind of thing.

Fixed.
Comment 7 Build Bot 2018-08-18 01:59:22 PDT
Comment on attachment 347389 [details]
Patch

Attachment 347389 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/8899848

New failing tests:
http/tests/security/contentSecurityPolicy/video-with-https-url-allowed-by-csp-media-src-star.html
Comment 8 Build Bot 2018-08-18 01:59:34 PDT
Created attachment 347441 [details]
Archive of layout-test-results from ews206 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews206  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment 9 Ali Juma 2018-08-18 15:13:21 PDT
Created attachment 347451 [details]
Patch for landing
Comment 10 Ali Juma 2018-08-18 15:26:58 PDT
Comment on attachment 347389 [details]
Patch

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

>> Source/WebCore/dom/Document.cpp:7433
>> +    for (auto observer : m_intersectionObservers) {
> 
> m_intersectionObservers is a HashSet, so order is not guaranteed. Does the API require that observers fire in registration order? Even if not, I think we should make order predictable, so maybe a ListHashSet, or just a Vector if we think the cost of checking for duplicates is low.  Do we know how many observers common pages have?

The API doesn't require that observers fire in registration order (but does say that for each observer, targets should be processed in the order they were added).

I'm still waiting to hear back if we have data about how many observers common pages have. For now, I've changed to using just a Vector. If it turns out the having a large number of observers is common, I'll change it to a ListHashSet.

We don't have to worry about duplicates in addIntersectionObserver, because the caller (IntersectionObserver::observe) only makes this call when it knows it's not in the list. I've also added an assertion to addIntersectionObserver to enforce/document this. But removeIntersectionObserver is potentially expensive with a Vector if we have a large number of observers.
Comment 11 WebKit Commit Bot 2018-08-18 16:45:15 PDT
Comment on attachment 347451 [details]
Patch for landing

Clearing flags on attachment: 347451

Committed r235014: <https://trac.webkit.org/changeset/235014>
Comment 12 WebKit Commit Bot 2018-08-18 16:45:17 PDT
All reviewed patches have been landed.  Closing bug.
Comment 13 Radar WebKit Bug Importer 2018-08-18 16:46:20 PDT
<rdar://problem/43469667>