HitTestPoint represents a rectangle used for hit-testing, this rectangle is currently stored as a point plus padding values. The padding value are very rarely used though, while the rectangle they span is constantly used, which requires it to be recalculated all the time.
Created attachment 148300 [details] Patch
Comment on attachment 148300 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=148300&action=review Patch looks good. I have one question: > Source/WebCore/page/EventHandler.cpp:2488 > + IntRect touchRect(touchCenter - touchRadius, touchRadius + touchRadius); I think the formula is a bit different: 187 // Formula: 188 // x = p.x() - rightPadding 189 // y = p.y() - topPadding 190 // width = leftPadding + rightPadding + 1 191 // height = topPadding + bottomPadding + 1 192 inline IntRect HitTestPoint::rectForPoint(const LayoutPoint& point) const 193 { 194 return rectForPoint(point, m_topPadding, m_rightPadding, m_bottomPadding, m_leftPadding); 195 } does it return different rects now in some cases?
(In reply to comment #2) > (From update of attachment 148300 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=148300&action=review > > Patch looks good. I have one question: > > > Source/WebCore/page/EventHandler.cpp:2488 > > + IntRect touchRect(touchCenter - touchRadius, touchRadius + touchRadius); > > I think the formula is a bit different: > > 187 // Formula: > 188 // x = p.x() - rightPadding > 189 // y = p.y() - topPadding > 190 // width = leftPadding + rightPadding + 1 > 191 // height = topPadding + bottomPadding + 1 > 192 inline IntRect HitTestPoint::rectForPoint(const LayoutPoint& point) const > 193 { > 194 return rectForPoint(point, m_topPadding, m_rightPadding, m_bottomPadding, m_leftPadding); > 195 } > > does it return different rects now in some cases? In this particular case the radius has originally been calculated as rect.width()/2 and rect.height()/2, so the most correct thing is to calculate back to a rectangle by just adding left and right padding, and top and bottom padding together. The formula has not been changed anywhere else. Unrelated to this bug though, I do hope at some point to move away from the formula of adding 1 to width and height, since I don't see the center-point as a whole pixel, but as an infinitely small point. Once you add transformations it is just odd if the point itself changes size. But I have deliberately left this unchanged for now.
Created attachment 148766 [details] Patch Re-attaching the original patch to rerun EWS tests after prerequisite patch has landed.
Comment on attachment 148766 [details] Patch Attachment 148766 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13029106 New failing tests: transforms/3d/hit-testing/perspective-clipped.html fast/forms/search-transformed.html fast/dom/elementFromPoint-relative-to-viewport.html WebFrameTest.DivAutoZoomParamsTest
Created attachment 148785 [details] Archive of layout-test-results from ec2-cr-linux-02 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-02 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Created attachment 148995 [details] Patch Move rect when moving point
Comment on attachment 148995 [details] Patch Attachment 148995 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13067013 New failing tests: transforms/3d/hit-testing/perspective-clipped.html fast/dom/elementFromPoint-relative-to-viewport.html
Created attachment 149001 [details] Archive of layout-test-results from ec2-cr-linux-03 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-03 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Created attachment 149033 [details] Patch Avoid changing corner-case behavior for point hit-testing, and eliminate code complication.
(In reply to comment #8) > (From update of attachment 148995 [details]) > Attachment 148995 [details] did not pass chromium-ews (chromium-xvfb): > Output: http://queues.webkit.org/results/13067013 > > New failing tests: > transforms/3d/hit-testing/perspective-clipped.html > fast/dom/elementFromPoint-relative-to-viewport.html I should note these two tests depends on the 1x1 expansion of a hit-test point when subpixel layout is enabled.
Comment on attachment 149033 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=149033&action=review These changes all look OK, but things seem a bit mixed up. And there are coding style mistakes and typos that should be fixed before landing. > Source/WebCore/rendering/HitTestResult.cpp:69 > // If all padding values passed in are zero then it is not a rect based hit test. > m_isRectBased = topPadding || rightPadding || bottomPadding || leftPadding; > + m_boundingRect = rectForPoint(centerPoint, topPadding, rightPadding, bottomPadding, leftPadding); Why are these not done as initializers rather than assignment? The code generated will be slightly inferior this way and it seems unnecessary. > Source/WebCore/rendering/HitTestResult.cpp:105 > +template<typename SomeRect> > +bool hitTestPointIntersects(const HitTestPoint& hitTestPoint, const SomeRect &rect) > +{ > + // FIXME: When the hit-test is not rect-based we should use rect.contains(m_point). > + // That does change some corner case tests though. > + > + // First check if rect even intersects our bounding rect. > + if (!rect.intersects(hitTestPoint.boundingBox())) > + return false; > + > + // FIXME: Implement quad-based interesection test to handle transformed hittest points. > + return true; > + > +} The name SomeRect here is not conventional terminology. Maybe RectType would be a better name. The & is positioned wrong and should be const SomeRect& rect. The word intersection is spelled wrong as “interesection”. Since hittest is not a word it should be “hit test”, and it should not be “hit-test” above, and it should be “rect based”, not “rect-based”. > Source/WebCore/rendering/HitTestResult.cpp:107 > +bool HitTestPoint::intersects(const LayoutRect &rect) const Same formatting mistake. > Source/WebCore/rendering/HitTestResult.cpp:109 > + return hitTestPointIntersects<LayoutRect>(*this, rect); There is no reason to specify the type here explicitly as <LayoutRect>. The compiler selects the appropriate type for the template automatically. > Source/WebCore/rendering/HitTestResult.cpp:112 > +bool HitTestPoint::intersects(const FloatRect &rect) const Ditto. > Source/WebCore/rendering/HitTestResult.cpp:114 > + return hitTestPointIntersects<FloatRect>(*this, rect); Ditto. > Source/WebCore/rendering/HitTestResult.h:63 > - void setPoint(const LayoutPoint& p) { m_point = p; } > + void setPoint(const LayoutPoint& p) { m_boundingRect.move(roundedIntPoint(p) - roundedIntPoint(m_point)); m_point = p; } Is it correct to round twice here rather than doing the math and then rounding? Is this covered by a test case? Perhaps this function is too long to have it inside the class definition. We could put it somewhere below. > Source/WebCore/rendering/HitTestResult.h:81 > LayoutPoint m_point; > > - int m_topPadding; > - int m_rightPadding; > - int m_bottomPadding; > - int m_leftPadding; > + IntRect m_boundingRect; I think it’s confusing to have a LayoutPoint named m_point and a IntRect named m_boundingRect. Because of the strangely different naming and types it’s unclear how these two relate to each other. Further, it’s unclear how m_isRectBased works. Is one of these ignored completely in one of the two cases?
(In reply to comment #12) > > Source/WebCore/rendering/HitTestResult.h:63 > > - void setPoint(const LayoutPoint& p) { m_point = p; } > > + void setPoint(const LayoutPoint& p) { m_boundingRect.move(roundedIntPoint(p) - roundedIntPoint(m_point)); m_point = p; } > > Is it correct to round twice here rather than doing the math and then rounding? Is this covered by a test case? Perhaps this function is too long to have it inside the class definition. We could put it somewhere below. > Rounding twice is necessary to ensure the rect will be in the same place it would be if it had been declared in this spot with the original padding. Rounding the distance could generate different results when sub-pixel layout is enabled. Ideally the boundingBox will move to using layoutRect later, but that changes a few test cases, so I will do that in a separate bug and patch. > > Source/WebCore/rendering/HitTestResult.h:81 > > LayoutPoint m_point; > > > > - int m_topPadding; > > - int m_rightPadding; > > - int m_bottomPadding; > > - int m_leftPadding; > > + IntRect m_boundingRect; > > I think it’s confusing to have a LayoutPoint named m_point and a IntRect named m_boundingRect. Because of the strangely different naming and types it’s unclear how these two relate to each other. Further, it’s unclear how m_isRectBased works. Is one of these ignored completely in one of the two cases? The boundingRect is named this way because this is a patch preparing for the patch in bug #85792, where the area is stored in a FloatQuad and the bounding rect is the cached result of the FloatQuads bounding box. m_isRectBased is currently needed instead of just using m_boundingRect.isEmpty() because the rect is expanded by 1x1 below and left of the m_point, so even point based hit testing has a non empty area..
Created attachment 149170 [details] Patch
Comment on attachment 149170 [details] Patch Attachment 149170 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13085294
Comment on attachment 149170 [details] Patch Attachment 149170 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/13033689
Comment on attachment 149170 [details] Patch Attachment 149170 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/13084258
Comment on attachment 149170 [details] Patch Attachment 149170 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/13032664
Comment on attachment 149170 [details] Patch Attachment 149170 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/13085295
Comment on attachment 149170 [details] Patch Attachment 149170 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/13030617
Comment on attachment 149170 [details] Patch Attachment 149170 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13037598
Created attachment 149173 [details] Patch
Comment on attachment 149173 [details] Patch Clearing flags on attachment: 149173 Committed r121096: <http://trac.webkit.org/changeset/121096>
All reviewed patches have been landed. Closing bug.