Summary: | [Wheel event region] Add support for getting wheel event region from ScrollingTree | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Antti Koivisto <koivisto> | ||||||||
Component: | Compositing | Assignee: | 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
Antti Koivisto
2020-05-12 09:50:59 PDT
Created attachment 399139 [details]
patch
Created attachment 399142 [details]
patch
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 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 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. Created attachment 399245 [details]
patch
> final?
The class is marked final, using override for consistency.
Committed r261602: <https://trac.webkit.org/changeset/261602> All reviewed patches have been landed. Closing bug and clearing flags on attachment 399245 [details]. > 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.
|