The HitTestPoint class introduced in bug #85965, is supposed to replace LayoutPoint as the point tested in nodeAtPoint. This is necessary to fix area-based hittesting on transformed elements, bug #85792.
Created attachment 148293 [details] Patch
The answers to https://bugs.webkit.org/show_bug.cgi?id=85792#c27 will make reviewing this patch trivial. :) You'll likely find eae, leviw and smfr to be good reviewers for these sorts of patches as they're familiar with both the subpixel rendering support and rendering tree transformations/hittesting. I'm also happy to help with these reviews.
Comment on attachment 148293 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=148293&action=review Thank you again for hte long explanations both on the bugs, and over IRC. I have real concern about the long-term readability of this code due to the really port class, function and variable namings. These new objects are more than points, and naming them HitTestPoint is just confusing. HitRegion would be one possible suggested replacement. In eithe rcase, this patch is OK, but we really need to fix this code to read more clearly before we're done here. > Source/WebCore/rendering/InlineFlowBox.cpp:969 > - if (!overflowRect.intersects(result.rectForPoint(pointInContainer))) > + if (!pointInContainer.intersects(overflowRect)) So we're going to remove rectForPoint in this new model? > Source/WebCore/rendering/RenderLayer.cpp:3631 > - if (bgRect.intersects(hitTestArea) && isSelfPaintingLayer()) { > + if (bgRect.intersects(hitTestPoint) && isSelfPaintingLayer()) { Seems like a sad name change. :( > Source/WebCore/rendering/RenderLayer.h:118 > + bool intersects(const HitTestPoint&); Unclear to me if this what it's going to look like in the end. It seems that clipRect and HitTestPoint both represent possibly non-rectilinear shapes. > Source/WebCore/rendering/RenderLineBoxList.cpp:287 > + LayoutPoint point = pointInContainer.point(); This and many similar renames could hav been avoided by changing the argument to something else and using a pointInContainer local which was actually a LayoutPOint.
(In reply to comment #3) > I have real concern about the long-term readability of this code due to the really port class, function and variable namings. These new objects are more than points, and naming them HitTestPoint is just confusing. > > HitRegion would be one possible suggested replacement. > The term point here is a point the same way a touch-point is, since touch points also has an area, but I can see how it might be confusing. Unfortunately Region is also a really overloaded name in WebKit rendering terms, so maybe HitTestArea or TouchPoint would work?
I guess I dont' like TouchPoint either, since it's not a point. :) It would be nice if while you're in here you came up with a cohenent vision for what hit testing should look like. it appears to me that he hit testing code hasn't had someone come and love it in a long time. :) I would be fine with HitTestArea or HitArea. Region is definitely an overloaded term, but I'm not sure that HitRegion makes that overloading any worse. :)
Comment on attachment 148293 [details] Patch Clearing flags on attachment: 148293 Committed r120824: <http://trac.webkit.org/changeset/120824>
All reviewed patches have been landed. Closing bug.