Summary: | Event region computation should respect transforms | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Antti Koivisto <koivisto> | ||||||
Component: | Scrolling | Assignee: | Nobody <webkit-unassigned> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | commit-queue, darin, simon.fraser, webkit-bug-importer | ||||||
Priority: | P2 | Keywords: | InRadar | ||||||
Version: | WebKit Nightly Build | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
See Also: |
https://bugs.webkit.org/show_bug.cgi?id=197818 https://bugs.webkit.org/show_bug.cgi?id=197892 |
||||||||
Attachments: |
|
Description
Antti Koivisto
2019-05-13 05:54:42 PDT
Created attachment 369836 [details]
patch
Comment on attachment 369836 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=369836&action=review > Source/WebCore/ChangeLog:12 > + Add support for transforming regions. Non-rectlinear results use enclosing rects. Is it acceptable to just use enclosing rects? > Source/WebCore/rendering/EventRegion.h:41 > + EventRegionContext(EventRegion&); We want "explicit" here since we wouldn’t want this to be done as a conversion. > Source/WebCore/rendering/EventRegion.h:58 > + EventRegionContext makeContext() { return { *this }; } Not sure why we have this since EventRegionContext also has a public constructor. And it’s only used in one place. I think we could do without it. Not sure, it does seem a little more elegant than invoking a constructor. > Source/WebCore/rendering/RenderLayer.cpp:4603 > + auto oldTransfrom = context.getCTM(); Spelling error here: "transfrom". > Is it acceptable to just use enclosing rects? Event region is used to synchronously determine on UI process side whether swipes should scroll or not. In this use approximation is often unnoticeable and definitely better than getting it completely wrong. If needed this can be improved later, either by approximating with more rects or by using different data structures (bitmap or multiple Regions with attached transforms). > Not sure why we have this since EventRegionContext also has a public > constructor. And it’s only used in one place. I think we could do without > it. Not sure, it does seem a little more elegant than invoking a constructor. I like construction function like this because then the client site does not need to spell out uninteresting types: auto eventRegionContext = eventRegion.makeContext(); Created attachment 369867 [details]
patch
Comment on attachment 369867 [details] patch Clearing flags on attachment: 369867 Committed r245293: <https://trac.webkit.org/changeset/245293> |