Bug 211785

Summary: [Wheel event region] Add support for getting wheel event region from ScrollingTree
Product: WebKit Reporter: Antti Koivisto <koivisto>
Component: CompositingAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: changseok, cmarcelo, darin, esprehn+autocc, ews-watchlist, fred.wang, glenn, jamesr, kondapallykalyan, luiz, pdr, simon.fraser, tonikitoo, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
patch
none
patch
simon.fraser: review+
patch none

Description Antti Koivisto 2020-05-12 09:50:59 PDT
Add more code.
Comment 1 Antti Koivisto 2020-05-12 10:11:39 PDT
Created attachment 399139 [details]
patch
Comment 2 Antti Koivisto 2020-05-12 10:23:26 PDT
Created attachment 399142 [details]
patch
Comment 3 Simon Fraser (smfr) 2020-05-12 10:42:38 PDT
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?
Comment 4 Darin Adler 2020-05-12 11:04:19 PDT
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?
Comment 5 Simon Fraser (smfr) 2020-05-12 11:21:48 PDT
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.
Comment 6 Antti Koivisto 2020-05-12 23:39:56 PDT
Created attachment 399245 [details]
patch
Comment 7 Antti Koivisto 2020-05-12 23:42:41 PDT
> final?

The class is marked final, using override for consistency.
Comment 8 EWS 2020-05-13 00:10:56 PDT
Committed r261602: <https://trac.webkit.org/changeset/261602>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 399245 [details].
Comment 9 Radar WebKit Bug Importer 2020-05-13 00:11:18 PDT
<rdar://problem/63172645>
Comment 10 Antti Koivisto 2020-05-13 00:12:30 PDT
> 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.