Bug 188670

Created attachment 347287 [details]
Patch

Created attachment 347289 [details]
Patch

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.

Created attachment 347389 [details]
Patch

Address review comments

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

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

Created attachment 347451 [details]
Patch for landing

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 on attachment 347451 [details]
Patch for landing

Clearing flags on attachment: 347451

Committed r235014: <https://trac.webkit.org/changeset/235014>

All reviewed patches have been landed.  Closing bug.

<rdar://problem/43469667>