Bug 211785 – [Wheel event region] Add support for getting wheel event region from ScrollingTree

• Component: Compositing
• Status: RESOLVED FIXED

[Antti Koivisto]

Add more code.

[Antti Koivisto]

Created attachment 399139 [details]
patch

[Antti Koivisto]

Created attachment 399142 [details]
patch

[Simon Fraser (smfr)]

Comment on attachment 399142 [details]
patch

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

> Source/WebCore/page/scrolling/mac/ScrollingTreeMac.mm:106
> +                return eventRegion && eventRegion->contains(IntPoint(localPoint));

Maybe roundedIntPoint()?

> Source/WebCore/page/scrolling/mac/ScrollingTreeMac.mm:216
> +    return eventRegion->eventListenerRegionTypesForPoint(IntPoint(point));

roundedIntPoint?

[Darin Adler]

Comment on attachment 399142 [details]
patch

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

Is IntPoint right here? I’d like to understand better some day when it should be int, float, CGFloat/double, or LayoutUnit.

> Source/WebCore/page/scrolling/mac/ScrollingTreeMac.mm:194
> +    LockHolder lockHolder(m_layerHitTestMutex);

Does this lock need to be held all the way to the end? I don’t fully understand what is being guarded.

> Source/WebCore/page/scrolling/mac/ScrollingTreeMac.mm:208
> +    auto platformCALayer = PlatformCALayer::platformCALayerForLayer((__bridge void*)hitLayer);

Seems like we eventually need to fix this function so we don’t have to cast to void* to use it.

> Source/WebCore/platform/graphics/ca/cocoa/PlatformCALayerCocoa.h:171
> +    const EventRegion* eventRegion() const override { return &m_eventRegion; }

final?

[Simon Fraser (smfr)]

Comment on attachment 399142 [details]
patch

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

>> Source/WebCore/page/scrolling/mac/ScrollingTreeMac.mm:194
>> +    LockHolder lockHolder(m_layerHitTestMutex);
> 
> Does this lock need to be held all the way to the end? I don’t fully understand what is being guarded.

This lock guards CALayers and PlatformCALayers, so the answer is yes.

[Antti Koivisto]

Created attachment 399245 [details]
patch

[Antti Koivisto]

> final?

The class is marked final, using override for consistency.

[EWS]

Committed r261602: <https://trac.webkit.org/changeset/261602>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 399245 [details].

[Radar WebKit Bug Importer]

<rdar://problem/63172645>

[Antti Koivisto]

> Is IntPoint right here? I’d like to understand better some day when it
> should be int, float, CGFloat/double, or LayoutUnit.

The underlying data structure here (Region) is currently integer based, that's why IntPoint. For its current use cases that level of accuracy is sufficient.

I think we should move to 64 bit LayoutUnit at some point. That could allow using a single unit type through the layout code.