Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11402002:
Add API for hit testing layer_impl touchEventHandlerRegions from the host (Closed)
|
Created:
8 years, 1 month ago by Yusuf Modified:
8 years, 1 month ago CC:
chromium-reviews, cc-bugs_chromium.or, leviw_travelin_and_unemployed, klobag.chromium, shawnsingh Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd API for hit testing layer_impl touchEventHandlerRegions from the host
These APIs will be used for hit testing touchEvents on the compositor. Currently
all touch events coming through input_event_filter return DidNotHandle. Using this check
we will be able to check the touch Ppoint against the touchEventHandlerRegion in all
layer_impls and return DropEvent if there are no hits.
BUG=135818
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=168294
Patch Set 1 #Patch Set 2 : Added missing compositor_bindings call #
Total comments: 4
Patch Set 3 : Renamed parameters and fixed point conversion #
Total comments: 1
Patch Set 4 : Added tests and fixed parameters on static call #
Total comments: 12
Patch Set 5 : Testing more points on the boundaries and also added test for deviceScaleFactor and pageScaleFactor #
Total comments: 19
Patch Set 6 : Fixed pageScale and deviceScale issues, started using ScalePoint instead of scale #Patch Set 7 : Fix typo in layer_tree_host_impl #
Total comments: 1
Patch Set 8 : Renamed call to haveTouchEventHandlersAt #
Messages
Total messages: 33 (0 generated)
This picked up and tried to address danakj@'s comments for the relevant pieces on http://codereview.chromium.org/11365113/. This depends on http://codereview.chromium.org/11377013/ to land first.
This looks pretty good, a few comments about getting coordinate spaces right. http://codereview.chromium.org/11402002/diff/2001/cc/layer_tree_host_common.cc File cc/layer_tree_host_common.cc (right): http://codereview.chromium.org/11402002/diff/2001/cc/layer_tree_host_common.c... cc/layer_tree_host_common.cc:880: static bool pointHitsRegion(const gfx::PointF& screenSpacePoint, const WebTransformationMatrix& localSpaceToScreenSpaceTransform, Region localSpaceRegion) just call the transform "screenSpaceTransform" as that's well defined for cc/ the region, call it layerSpaceRegion (not local, that's not a term we use elsewhere) http://codereview.chromium.org/11402002/diff/2001/cc/layer_tree_host_common.c... cc/layer_tree_host_common.cc:888: gfx::PointF hitTestPointInLocalSpace = MathUtil::projectPoint(localSpaceToScreenSpaceTransform.inverse(), screenSpacePoint, clipped); See LayerImpl::tryScroll() for how to do this, I fixed it the other day. This should be hitTestPointInContentSpace. Then you need to convert it to hitTestPointInLayerSpace. Then you can compare to the region.
https://chromiumcodereview.appspot.com/11402002/diff/2001/cc/layer_tree_host_... File cc/layer_tree_host_common.cc (right): https://chromiumcodereview.appspot.com/11402002/diff/2001/cc/layer_tree_host_... cc/layer_tree_host_common.cc:880: static bool pointHitsRegion(const gfx::PointF& screenSpacePoint, const WebTransformationMatrix& localSpaceToScreenSpaceTransform, Region localSpaceRegion) On 2012/11/08 01:30:38, danakj wrote: > just call the transform "screenSpaceTransform" as that's well defined for cc/ > > the region, call it layerSpaceRegion (not local, that's not a term we use > elsewhere) Done. https://chromiumcodereview.appspot.com/11402002/diff/2001/cc/layer_tree_host_... cc/layer_tree_host_common.cc:888: gfx::PointF hitTestPointInLocalSpace = MathUtil::projectPoint(localSpaceToScreenSpaceTransform.inverse(), screenSpacePoint, clipped); On 2012/11/08 01:30:38, danakj wrote: > See LayerImpl::tryScroll() for how to do this, I fixed it the other day. > > This should be hitTestPointInContentSpace. Then you need to convert it to > hitTestPointInLayerSpace. Then you can compare to the region. Done.
Thanks, this looks great. Can you please add some tests for it? There are some tests in layer_tree_host_impl_unittests for nonFastScrollRegion that may be a good starting point. https://chromiumcodereview.appspot.com/11402002/diff/7001/cc/layer_tree_host_... File cc/layer_tree_host_common.cc (right): https://chromiumcodereview.appspot.com/11402002/diff/7001/cc/layer_tree_host_... cc/layer_tree_host_common.cc:880: static bool pointHitsRegion(const gfx::PointF& screenSpacePoint, const WebTransformationMatrix& screenSpaceTransform, Region layerSpaceRegion, float layerContentScaleX, float layerContentScaleY) You can pass the PointF by value (2 floats), but please pass the Region by reference! (Vector of rects)
Added the tests! I ended up basing them mostly on the tests for findLayerThatIsHitByPoint. On 2012/11/08 02:18:48, danakj wrote: > Thanks, this looks great. Can you please add some tests for it? There are some > tests in layer_tree_host_impl_unittests for nonFastScrollRegion that may be a > good starting point. > > Done https://chromiumcodereview.appspot.com/11402002/diff/7001/cc/layer_tree_host_... > File cc/layer_tree_host_common.cc (right): > > https://chromiumcodereview.appspot.com/11402002/diff/7001/cc/layer_tree_host_... > cc/layer_tree_host_common.cc:880: static bool pointHitsRegion(const gfx::PointF& > screenSpacePoint, const WebTransformationMatrix& screenSpaceTransform, Region > layerSpaceRegion, float layerContentScaleX, float layerContentScaleY) > You can pass the PointF by value (2 floats), but please pass the Region by > reference! (Vector of rects)
Thanks for the tests! They're really nice. I think we could test the boundary cases a little tighter, you check within areas, but not always the closest points to the boundaries. So adding/moving test points would help make us more certain, I feel. Also, please add a test that covers us for device/page scale != 1. LGTM with those changes. http://codereview.chromium.org/11402002/diff/1002/cc/layer_tree_host_common_u... File cc/layer_tree_host_common_unittest.cc (right): http://codereview.chromium.org/11402002/diff/1002/cc/layer_tree_host_common_u... cc/layer_tree_host_common_unittest.cc:3692: LayerTreeHostCommon::calculateDrawTransforms(root.get(), root->bounds(), 1, 1, 0, dummyMaxTextureSize, renderSurfaceLayerList); Can you repeat this test but pass something non-1, like 3 and 5, for device and page scale factors? http://codereview.chromium.org/11402002/diff/1002/cc/layer_tree_host_common_u... cc/layer_tree_host_common_unittest.cc:3707: testPoint = gfx::Point(26, 26); does this also work at 34,34? http://codereview.chromium.org/11402002/diff/1002/cc/layer_tree_host_common_u... cc/layer_tree_host_common_unittest.cc:3711: testPoint = gfx::Point(74, 74); does this also work at 65,65? http://codereview.chromium.org/11402002/diff/1002/cc/layer_tree_host_common_u... cc/layer_tree_host_common_unittest.cc:3716: testPoint = gfx::Point(36, 36); does this also work at 35,35? http://codereview.chromium.org/11402002/diff/1002/cc/layer_tree_host_common_u... cc/layer_tree_host_common_unittest.cc:3773: testPoint = gfx::Point(36, 36); how about 35,35? http://codereview.chromium.org/11402002/diff/1002/cc/layer_tree_host_common_u... cc/layer_tree_host_common_unittest.cc:3782: testPoint = gfx::Point(26, 26); how about 25,25?
Added the deviceScalefactor and pageScalefactor related test. On 2012/11/09 17:02:58, danakj wrote: > Thanks for the tests! They're really nice. I think we could test the boundary > cases a little tighter, you check within areas, but not always the closest > points to the boundaries. So adding/moving test points would help make us more > certain, I feel. > > Also, please add a test that covers us for device/page scale != 1. > > LGTM with those changes. > > http://codereview.chromium.org/11402002/diff/1002/cc/layer_tree_host_common_u... > File cc/layer_tree_host_common_unittest.cc (right): > > http://codereview.chromium.org/11402002/diff/1002/cc/layer_tree_host_common_u... > cc/layer_tree_host_common_unittest.cc:3692: > LayerTreeHostCommon::calculateDrawTransforms(root.get(), root->bounds(), 1, 1, > 0, dummyMaxTextureSize, renderSurfaceLayerList); > Can you repeat this test but pass something non-1, like 3 and 5, for device and > page scale factors? > > http://codereview.chromium.org/11402002/diff/1002/cc/layer_tree_host_common_u... > cc/layer_tree_host_common_unittest.cc:3707: testPoint = gfx::Point(26, 26); > does this also work at 34,34? > > http://codereview.chromium.org/11402002/diff/1002/cc/layer_tree_host_common_u... > cc/layer_tree_host_common_unittest.cc:3711: testPoint = gfx::Point(74, 74); > does this also work at 65,65? > > http://codereview.chromium.org/11402002/diff/1002/cc/layer_tree_host_common_u... > cc/layer_tree_host_common_unittest.cc:3716: testPoint = gfx::Point(36, 36); > does this also work at 35,35? > > http://codereview.chromium.org/11402002/diff/1002/cc/layer_tree_host_common_u... > cc/layer_tree_host_common_unittest.cc:3773: testPoint = gfx::Point(36, 36); > how about 35,35? > > http://codereview.chromium.org/11402002/diff/1002/cc/layer_tree_host_common_u... > cc/layer_tree_host_common_unittest.cc:3782: testPoint = gfx::Point(26, 26); > how about 25,25?
http://codereview.chromium.org/11402002/diff/1002/cc/layer_tree_host_common_u... File cc/layer_tree_host_common_unittest.cc (right): http://codereview.chromium.org/11402002/diff/1002/cc/layer_tree_host_common_u... cc/layer_tree_host_common_unittest.cc:3692: LayerTreeHostCommon::calculateDrawTransforms(root.get(), root->bounds(), 1, 1, 0, dummyMaxTextureSize, renderSurfaceLayerList); On 2012/11/09 17:02:58, danakj wrote: > Can you repeat this test but pass something non-1, like 3 and 5, for device and > page scale factors? Done. http://codereview.chromium.org/11402002/diff/1002/cc/layer_tree_host_common_u... cc/layer_tree_host_common_unittest.cc:3707: testPoint = gfx::Point(26, 26); On 2012/11/09 17:02:58, danakj wrote: > does this also work at 34,34? Done. http://codereview.chromium.org/11402002/diff/1002/cc/layer_tree_host_common_u... cc/layer_tree_host_common_unittest.cc:3711: testPoint = gfx::Point(74, 74); On 2012/11/09 17:02:58, danakj wrote: > does this also work at 65,65? Done. http://codereview.chromium.org/11402002/diff/1002/cc/layer_tree_host_common_u... cc/layer_tree_host_common_unittest.cc:3716: testPoint = gfx::Point(36, 36); On 2012/11/09 17:02:58, danakj wrote: > does this also work at 35,35? Done. http://codereview.chromium.org/11402002/diff/1002/cc/layer_tree_host_common_u... cc/layer_tree_host_common_unittest.cc:3773: testPoint = gfx::Point(36, 36); On 2012/11/09 17:02:58, danakj wrote: > how about 35,35? Done. http://codereview.chromium.org/11402002/diff/1002/cc/layer_tree_host_common_u... cc/layer_tree_host_common_unittest.cc:3782: testPoint = gfx::Point(26, 26); On 2012/11/09 17:02:58, danakj wrote: > how about 25,25? Done.
http://codereview.chromium.org/11402002/diff/6002/cc/layer_tree_host_common_u... File cc/layer_tree_host_common_unittest.cc (right): http://codereview.chromium.org/11402002/diff/6002/cc/layer_tree_host_common_u... cc/layer_tree_host_common_unittest.cc:3738: // by scaling the points accordingly. This sounds like actually the opposite of what we would want/expect. The content bounds of the layers should be scaled. But the hit testing input and output is all in layer space. So the coordinates/bounds you would use and the expectations here should all be the exact same as the other tests. Different behaviour under device/page scale should indicate failure, I believe.
http://codereview.chromium.org/11402002/diff/6002/cc/layer_tree_host_common_u... File cc/layer_tree_host_common_unittest.cc (right): http://codereview.chromium.org/11402002/diff/6002/cc/layer_tree_host_common_u... cc/layer_tree_host_common_unittest.cc:3746: setLayerPropertiesForTesting(root.get(), identityMatrix, identityMatrix, anchor, gfx::PointF(0, 0), gfx::Size(250, 250), false); Don't change the bounds of the root. Layer space should all be the same regardless of device/page scale. http://codereview.chromium.org/11402002/diff/6002/cc/layer_tree_host_common_u... cc/layer_tree_host_common_unittest.cc:3763: float pageScaleFactor = 5.0f; // pageScaleFactor shouldn't factor into this for LayerImpl. I'm not sure what you mean by this comment here. The pageScaleFactor should change the contentsBounds for the layers. But that should not change the layer space, so it shouldn't affect the output of the hit test methods. What did you observe that led to this comment? http://codereview.chromium.org/11402002/diff/6002/cc/layer_tree_host_common_u... cc/layer_tree_host_common_unittest.cc:3764: LayerTreeHostCommon::calculateDrawTransforms(root.get(), root->bounds(), deviceScaleFactor, pageScaleFactor, 0, dummyMaxTextureSize, renderSurfaceLayerList); The 2nd argument is the deviceViewportSize, ie the viewport size in physical pixels. This should be gfx::ToEnclosingRect(gfx::ScaleRect(root->bounds(), deviceScaleFactor)) http://codereview.chromium.org/11402002/diff/6002/cc/layer_tree_host_common_u... cc/layer_tree_host_common_unittest.cc:3767: // The visibleContentRect for testLayer is actually 100x100, even though its layout size is 50x50, positioned at 25x25. I don't see a check related to this comment. http://codereview.chromium.org/11402002/diff/6002/cc/layer_tree_host_common_u... cc/layer_tree_host_common_unittest.cc:3773: EXPECT_RECT_EQ(gfx::Rect(gfx::Point(), gfx::Size(50, 50)), testLayer->visibleContentRect()); The root layer is 250,250 now, so this seems off? You're checking if the testLayer was clipped, right? So: EXPECT_EQ(gfx::Rect(testLayer->contentBounds()), testLayer->VisibleContentRect()); ?
http://codereview.chromium.org/11402002/diff/6002/cc/layer_tree_host_common.cc File cc/layer_tree_host_common.cc (right): http://codereview.chromium.org/11402002/diff/6002/cc/layer_tree_host_common.c... cc/layer_tree_host_common.cc:889: gfx::PointF hitTestPointInLayerSpace = hitTestPointInContentSpace.Scale(1 / layerContentScaleX, 1 / layerContentScaleY); Lastly, you got hit by the geometry conversion stuff a few times through this CL.. This should now be: gfx::PointF hitTestPointInLayerSpace = gfx::ScalePoint(hitTestPointInContentSpace, 1 / layerContentScaleX, 1 / layerContentScaleY); http://codereview.chromium.org/11402002/diff/6002/cc/layer_tree_host_impl.cc File cc/layer_tree_host_impl.cc (right): http://codereview.chromium.org/11402002/diff/6002/cc/layer_tree_host_impl.cc#... cc/layer_tree_host_impl.cc:328: gfx::PointF deviceViewportPoint = viewportPoint.Scale(m_deviceScaleFactor); And this should be: gfx::PointF deviceViewportPoint = gfx::ScalePoint(viewportPoint, m_deviceScaleFactor));
http://codereview.chromium.org/11402002/diff/6002/cc/layer_tree_host_common_u... File cc/layer_tree_host_common_unittest.cc (right): http://codereview.chromium.org/11402002/diff/6002/cc/layer_tree_host_common_u... cc/layer_tree_host_common_unittest.cc:3763: float pageScaleFactor = 5.0f; // pageScaleFactor shouldn't factor into this for LayerImpl. I may be misinterpreting the code I think, but calculateDrawwTransform calls calculateDrawTransformsInternal and in that, the only place we use pageScalefactor is updateLayerContentsScale. And in the implementation corresponding to LayerImpl* it says "There is no contentsScale on impl thread." On 2012/11/13 00:50:43, danakj wrote: > I'm not sure what you mean by this comment here. The pageScaleFactor should > change the contentsBounds for the layers. But that should not change the layer > space, so it shouldn't affect the output of the hit test methods. > > What did you observe that led to this comment?
http://codereview.chromium.org/11402002/diff/6002/cc/layer_tree_host_common_u... File cc/layer_tree_host_common_unittest.cc (right): http://codereview.chromium.org/11402002/diff/6002/cc/layer_tree_host_common_u... cc/layer_tree_host_common_unittest.cc:3763: float pageScaleFactor = 5.0f; // pageScaleFactor shouldn't factor into this for LayerImpl. On 2012/11/13 00:57:36, Yusuf wrote: > I may be misinterpreting the code I think, but calculateDrawwTransform calls > calculateDrawTransformsInternal and in that, the only place we use > pageScalefactor is updateLayerContentsScale. And in the implementation > corresponding to LayerImpl* it says "There is no contentsScale on impl thread." Oh! Okay, right. Since there's no main thread to set the contentsScale it's more or less a no-op for now. In the future, when the compositor is applying the pageScaleFactor, then it will be less of a no-op and more like deviceScaleFactor. So this test should continue to pass when that change happens.
http://codereview.chromium.org/11402002/diff/6002/cc/layer_tree_host_common_u... File cc/layer_tree_host_common_unittest.cc (right): http://codereview.chromium.org/11402002/diff/6002/cc/layer_tree_host_common_u... cc/layer_tree_host_common_unittest.cc:3763: float pageScaleFactor = 5.0f; // pageScaleFactor shouldn't factor into this for LayerImpl. Actually, sorry, I confused myself. The pageScaleFactor is applied, via the setImplTransform() method on the rootScrollLayer. In this case, before calling calcDraw, can you do: WebTransformationMatrix pageScaleTransform; pageScaleTransform.scale(pageScaleFactor); rootLayer->setImplTransform(pageScaleTransform); That will get the contents of both layers scaling by the page scale factor. Again, though, it should not affect anything done in layer space.
http://codereview.chromium.org/11402002/diff/6002/cc/layer_tree_host_common_u... File cc/layer_tree_host_common_unittest.cc (right): http://codereview.chromium.org/11402002/diff/6002/cc/layer_tree_host_common_u... cc/layer_tree_host_common_unittest.cc:3738: // by scaling the points accordingly. Looking at scrollBegin(), we also scale the incoming point by deviceScaleFactor. That was mainly the reason I thought the behavior was different for deviceScaleFactor. Looking at calculateDrawTransformsInternal we factor in the deviceScaleFactor in the screenSpaceTransform, but don't add it to contentsScale, which means the first inversion will divide the input point by deviceScaleFactor. Should I be scaling it inside pointHitsRegion instead of doing it here in the test code and also remove the scale from layer_tree_host_impl? On 2012/11/13 00:42:04, danakj wrote: > This sounds like actually the opposite of what we would want/expect. > > The content bounds of the layers should be scaled. But the hit testing input and > output is all in layer space. So the coordinates/bounds you would use and the > expectations here should all be the exact same as the other tests. > > Different behaviour under device/page scale should indicate failure, I believe. http://codereview.chromium.org/11402002/diff/6002/cc/layer_tree_host_common_u... cc/layer_tree_host_common_unittest.cc:3746: setLayerPropertiesForTesting(root.get(), identityMatrix, identityMatrix, anchor, gfx::PointF(0, 0), gfx::Size(250, 250), false); On 2012/11/13 00:50:43, danakj wrote: > Don't change the bounds of the root. Layer space should all be the same > regardless of device/page scale. Done. http://codereview.chromium.org/11402002/diff/6002/cc/layer_tree_host_common_u... cc/layer_tree_host_common_unittest.cc:3764: LayerTreeHostCommon::calculateDrawTransforms(root.get(), root->bounds(), deviceScaleFactor, pageScaleFactor, 0, dummyMaxTextureSize, renderSurfaceLayerList); Using gfx::ToCeiledSize(root->bounds().Scale(deviceScaleFactor)) as this parameter is gfx::Size On 2012/11/13 00:50:43, danakj wrote: > The 2nd argument is the deviceViewportSize, ie the viewport size in physical > pixels. This should be > > gfx::ToEnclosingRect(gfx::ScaleRect(root->bounds(), deviceScaleFactor)) http://codereview.chromium.org/11402002/diff/6002/cc/layer_tree_host_common_u... cc/layer_tree_host_common_unittest.cc:3767: // The visibleContentRect for testLayer is actually 100x100, even though its layout size is 50x50, positioned at 25x25. On 2012/11/13 00:50:43, danakj wrote: > I don't see a check related to this comment. Sorry meant to swap this check with the one about VisibleContentrect, but left the comment here. It is removed now. http://codereview.chromium.org/11402002/diff/6002/cc/layer_tree_host_common_u... cc/layer_tree_host_common_unittest.cc:3773: EXPECT_RECT_EQ(gfx::Rect(gfx::Point(), gfx::Size(50, 50)), testLayer->visibleContentRect()); On 2012/11/13 00:50:43, danakj wrote: > The root layer is 250,250 now, so this seems off? You're checking if the > testLayer was clipped, right? So: > > EXPECT_EQ(gfx::Rect(testLayer->contentBounds()), > testLayer->VisibleContentRect()); ? Done.
http://codereview.chromium.org/11402002/diff/6002/cc/layer_tree_host_common_u... File cc/layer_tree_host_common_unittest.cc (right): http://codereview.chromium.org/11402002/diff/6002/cc/layer_tree_host_common_u... cc/layer_tree_host_common_unittest.cc:3738: // by scaling the points accordingly. On 2012/11/13 01:17:49, Yusuf wrote: > Looking at scrollBegin(), we also scale the incoming point by deviceScaleFactor. > That was mainly the reason I thought the behavior was different for > deviceScaleFactor. > > Looking at calculateDrawTransformsInternal we factor in the deviceScaleFactor in > the screenSpaceTransform, but don't add it to contentsScale, which means the > first inversion will divide the input point by deviceScaleFactor. Should I be > scaling it inside pointHitsRegion instead of doing it here in the test code and > also remove the scale from layer_tree_host_impl? > > On 2012/11/13 00:42:04, danakj wrote: > > This sounds like actually the opposite of what we would want/expect. > > > > The content bounds of the layers should be scaled. But the hit testing input > and > > output is all in layer space. So the coordinates/bounds you would use and the > > expectations here should all be the exact same as the other tests. > > > > Different behaviour under device/page scale should indicate failure, I > believe. > The incoming point goes through these transformations in total Viewport Space -> Device Viewport Space (This is in scrollBegin()) Device Viewport Space -> Layer Content Space (This is with screenspace transform inverse) Layer Content Space -> Layer Space (This is with scale by contentsScaleX and contentsScaleY). You should not need to do any scaling of the region. Since the deviceScaleFactor does not affect the contentsScale, then the 2nd step above should end up directly in the layer space, and the contentsScaleX/Y scaling should be a no-op (they'll both be one). And then your hit test point will be in layer space, the same as the region.
http://codereview.chromium.org/11402002/diff/6002/cc/layer_tree_host_common_u... File cc/layer_tree_host_common_unittest.cc (right): http://codereview.chromium.org/11402002/diff/6002/cc/layer_tree_host_common_u... cc/layer_tree_host_common_unittest.cc:3738: // by scaling the points accordingly. Then since the viewport space to deviceViewPort space conversion is taking place in layer_tree_host_impl, should I just move this one test to that level as well to avoid scaling these points in the test? Sorry, I think I am still having trouble seeing how to make the test work in layer_tree_host_common without scaling the input points. Should I move the viewport->device viewport conversion to later_tree_host_common as well somehow? On 2012/11/13 01:25:05, danakj wrote: > On 2012/11/13 01:17:49, Yusuf wrote: > > Looking at scrollBegin(), we also scale the incoming point by > deviceScaleFactor. > > That was mainly the reason I thought the behavior was different for > > deviceScaleFactor. > > > > Looking at calculateDrawTransformsInternal we factor in the deviceScaleFactor > in > > the screenSpaceTransform, but don't add it to contentsScale, which means the > > first inversion will divide the input point by deviceScaleFactor. Should I be > > scaling it inside pointHitsRegion instead of doing it here in the test code > and > > also remove the scale from layer_tree_host_impl? > > > > On 2012/11/13 00:42:04, danakj wrote: > > > This sounds like actually the opposite of what we would want/expect. > > > > > > The content bounds of the layers should be scaled. But the hit testing input > > and > > > output is all in layer space. So the coordinates/bounds you would use and > the > > > expectations here should all be the exact same as the other tests. > > > > > > Different behaviour under device/page scale should indicate failure, I > > believe. > > > > The incoming point goes through these transformations in total > > Viewport Space -> Device Viewport Space (This is in scrollBegin()) > Device Viewport Space -> Layer Content Space (This is with screenspace transform > inverse) > Layer Content Space -> Layer Space (This is with scale by contentsScaleX and > contentsScaleY). > > You should not need to do any scaling of the region. Since the deviceScaleFactor > does not affect the contentsScale, then the 2nd step above should end up > directly in the layer space, and the contentsScaleX/Y scaling should be a no-op > (they'll both be one). And then your hit test point will be in layer space, the > same as the region.
http://codereview.chromium.org/11402002/diff/6002/cc/layer_tree_host_common_u... File cc/layer_tree_host_common_unittest.cc (right): http://codereview.chromium.org/11402002/diff/6002/cc/layer_tree_host_common_u... cc/layer_tree_host_common_unittest.cc:3738: // by scaling the points accordingly. On 2012/11/13 01:41:39, Yusuf wrote: > Then since the viewport space to deviceViewPort space conversion is taking place > in layer_tree_host_impl, should I just move this one test to that level as well > to avoid scaling these points in the test? > > Sorry, I think I am still having trouble seeing how to make the test work in > layer_tree_host_common without scaling the input points. Should I move the > viewport->device viewport conversion to later_tree_host_common as well somehow? Ahhh. I see. I forgot this was not going through LayerTreeHostImpl. For scrolling we have hit test tests here, but also some tests in layer_tree_host_impl_unittest to cover the scrollBegin method doing the right thing. It would be nice to have a test like LayerTreeHostImplTest.nonFastScrollableRegionBasic to handle device scale. In fact, I see now we don't even have such a test for scrolling though (only for contents scale). So you're absolutely right, the input touch points in here should all be scaled up by deviceScaleFactor (but layer position/bounds shouldn't still). I'll make a bug to add deviceScale tests for both this and scrolling to LayerTreeHostImplTests.
Sorry for bothering you this much with this, but one last question: After I change the Impltransform, this changes the Screenspacetransform for the child which means I should also be scaling the incoming point by pageScaleFactor for the test in layer_tree_host_common, right? So all points should be scaled by deviceScaleFactor * pageScaleFactor before I use them in findLayerThatIsHitByPointInTouchHandlerRegion? On 2012/11/13 01:12:39, danakj wrote: > http://codereview.chromium.org/11402002/diff/6002/cc/layer_tree_host_common_u... > File cc/layer_tree_host_common_unittest.cc (right): > > http://codereview.chromium.org/11402002/diff/6002/cc/layer_tree_host_common_u... > cc/layer_tree_host_common_unittest.cc:3763: float pageScaleFactor = 5.0f; // > pageScaleFactor shouldn't factor into this for LayerImpl. > Actually, sorry, I confused myself. The pageScaleFactor is applied, via the > setImplTransform() method on the rootScrollLayer. > > In this case, before calling calcDraw, can you do: > > WebTransformationMatrix pageScaleTransform; > pageScaleTransform.scale(pageScaleFactor); > rootLayer->setImplTransform(pageScaleTransform); > > That will get the contents of both layers scaling by the page scale factor. > Again, though, it should not affect anything done in layer space.
On Tue, Nov 13, 2012 at 2:40 PM, <yusufo@chromium.org> wrote: > Sorry for bothering you this much with this, but one last question: No worries, I hope I'm not just confusing you more each time. Thanks for sticking with me. > After I change the Impltransform, this changes the Screenspacetransform for > the > child which means I should also be scaling the incoming point by > pageScaleFactor > for the test in layer_tree_host_common, right? > > So all points should be scaled by deviceScaleFactor * pageScaleFactor before > I > use them in findLayerThatIsHitByPointInTouchHandlerRegion? PageScale will change the screenspace transform, you're right. And the viewport will end up clipping layers. So let's scale the viewport size and the input events by deviceScale*pageScale. That means the rect given to calcDrawTransforms() would be root->bounds() scaled by both as well. I hope that works out perfectly? It should give you a viewport that doesn't clip things. The layers will all appear in it scaled by both. So the hit test points scaled up should give correct answers. > > On 2012/11/13 01:12:39, danakj wrote: > > http://codereview.chromium.org/11402002/diff/6002/cc/layer_tree_host_common_u... >> >> File cc/layer_tree_host_common_unittest.cc (right): > > > > http://codereview.chromium.org/11402002/diff/6002/cc/layer_tree_host_common_u... >> >> cc/layer_tree_host_common_unittest.cc:3763: float pageScaleFactor = 5.0f; >> // >> pageScaleFactor shouldn't factor into this for LayerImpl. >> Actually, sorry, I confused myself. The pageScaleFactor is applied, via >> the >> setImplTransform() method on the rootScrollLayer. > > >> In this case, before calling calcDraw, can you do: > > >> WebTransformationMatrix pageScaleTransform; >> pageScaleTransform.scale(pageScaleFactor); >> rootLayer->setImplTransform(pageScaleTransform); > > >> That will get the contents of both layers scaling by the page scale >> factor. >> Again, though, it should not affect anything done in layer space. > > > > http://codereview.chromium.org/11402002/
Thanks. With the changes above all the tests are working. Sending to try bots to make sure all build errors are gone. On 2012/11/13 19:56:43, danakj wrote: > On Tue, Nov 13, 2012 at 2:40 PM, <mailto:yusufo@chromium.org> wrote: > > Sorry for bothering you this much with this, but one last question: > > No worries, I hope I'm not just confusing you more each time. Thanks > for sticking with me. > > > After I change the Impltransform, this changes the Screenspacetransform for > > the > > child which means I should also be scaling the incoming point by > > pageScaleFactor > > for the test in layer_tree_host_common, right? > > > > So all points should be scaled by deviceScaleFactor * pageScaleFactor before > > I > > use them in findLayerThatIsHitByPointInTouchHandlerRegion? > > PageScale will change the screenspace transform, you're right. And the > viewport will end up clipping layers. So let's scale the viewport size > and the input events by deviceScale*pageScale. > > That means the rect given to calcDrawTransforms() would be > root->bounds() scaled by both as well. > > I hope that works out perfectly? It should give you a viewport that > doesn't clip things. The layers will all appear in it scaled by both. > So the hit test points scaled up should give correct answers. > > > > > On 2012/11/13 01:12:39, danakj wrote: > > > > > http://codereview.chromium.org/11402002/diff/6002/cc/layer_tree_host_common_u... > >> > >> File cc/layer_tree_host_common_unittest.cc (right): > > > > > > > > > http://codereview.chromium.org/11402002/diff/6002/cc/layer_tree_host_common_u... > >> > >> cc/layer_tree_host_common_unittest.cc:3763: float pageScaleFactor = 5.0f; > >> // > >> pageScaleFactor shouldn't factor into this for LayerImpl. > >> Actually, sorry, I confused myself. The pageScaleFactor is applied, via > >> the > >> setImplTransform() method on the rootScrollLayer. > > > > > >> In this case, before calling calcDraw, can you do: > > > > > >> WebTransformationMatrix pageScaleTransform; > >> pageScaleTransform.scale(pageScaleFactor); > >> rootLayer->setImplTransform(pageScaleTransform); > > > > > >> That will get the contents of both layers scaling by the page scale > >> factor. > >> Again, though, it should not affect anything done in layer space. > > > > > > > > http://codereview.chromium.org/11402002/
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yusufo@chromium.org/11402002/16005
Presubmit check for 11402002-16005 failed and returned exit status 1.
Running presubmit commit checks ...
** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
webkit/compositor_bindings
Presubmit checks took 3.4s to calculate.
Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!
https://chromiumcodereview.appspot.com/11402002/diff/16005/webkit/compositor_... File webkit/compositor_bindings/web_to_ccinput_handler_adapter.cc (right): https://chromiumcodereview.appspot.com/11402002/diff/16005/webkit/compositor_... webkit/compositor_bindings/web_to_ccinput_handler_adapter.cc:91: virtual bool checkTouchEventHandlerRegionHit(WebPoint point) what is the intended use of this function?
nduca@ for webkit/compositor_bindings
To be used from WebCompositorInputHandler to decide whether we are an incoming touch event is hitting any touch event handler regions. On 2012/11/13 23:21:40, jamesr wrote: > https://chromiumcodereview.appspot.com/11402002/diff/16005/webkit/compositor_... > File webkit/compositor_bindings/web_to_ccinput_handler_adapter.cc (right): > > https://chromiumcodereview.appspot.com/11402002/diff/16005/webkit/compositor_... > webkit/compositor_bindings/web_to_ccinput_handler_adapter.cc:91: virtual bool > checkTouchEventHandlerRegionHit(WebPoint point) > what is the intended use of this function?
I think the API name could better. Callsites will look like this: bool foo = inputHandler->xxxxx(point) so you'd like to have "xxxxxx" be something that makes it clear what the return value is - with this I don't know if true means that there was an event handler at that point or not. Perhaps something like 'haveTouchEventHandlersAt(point)'?
Renamed it to haveTouchEventHandlersAt On 2012/11/13 23:27:46, jamesr wrote: > I think the API name could better. Callsites will look like this: > > bool foo = inputHandler->xxxxx(point) > > so you'd like to have "xxxxxx" be something that makes it clear what the return > value is - with this I don't know if true means that there was an event handler > at that point or not. Perhaps something like 'haveTouchEventHandlersAt(point)'?
jamesr@ can I get an LGTM for webkit/ for this? On 2012/11/13 23:41:28, Yusuf wrote: > Renamed it to haveTouchEventHandlersAt > > On 2012/11/13 23:27:46, jamesr wrote: > > I think the API name could better. Callsites will look like this: > > > > bool foo = inputHandler->xxxxx(point) > > > > so you'd like to have "xxxxxx" be something that makes it clear what the > return > > value is - with this I don't know if true means that there was an event > handler > > at that point or not. Perhaps something like > 'haveTouchEventHandlersAt(point)'?
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yusufo@chromium.org/11402002/1019
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yusufo@chromium.org/11402002/1019
Change committed as 168294 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
