Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
328753002:
Clean up PointIsClippedBySurfaceOrClipRect (Closed)
|
Created:
6 years, 6 months ago by Ian Vollick Modified:
6 years, 6 months ago Reviewers:
danakj CC:
chromium-reviews, cc-bugs_chromium.org, enne (OOO) Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionClean up PointIsClippedBySurfaceOrClipRect
This function made unnecessary use of the render
target and didn't walk its clipping ancestor
chain correctly. This CL refactors the function
and fixes that walk (and adds test to show that
the walk is correct).
R=danakj@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=276799
Patch Set 1 #
Total comments: 1
Patch Set 2 : Address reviewer feedback #Patch Set 3 : Address review. Clean up code. Replace nullity check with something moar correcter. #
Total comments: 1
Patch Set 4 : . #
Total comments: 5
Patch Set 5 : . #Messages
Total messages: 14 (0 generated)
https://codereview.chromium.org/328753002/diff/1/cc/trees/layer_tree_impl_uni... File cc/trees/layer_tree_impl_unittest.cc (right): https://codereview.chromium.org/328753002/diff/1/cc/trees/layer_tree_impl_uni... cc/trees/layer_tree_impl_unittest.cc:1176: child1->draw_properties().render_target = NULL; This seems artificial, we don't clear the render target after running UpdateDrawProperties. Is this meant to be a situation where you have a layer that doesn't draw anything, and is therefore skipped my UpdateDrawProperties? How about using SetIsDrawable(false) on some layer in this test to do that?
On 2014/06/10 14:59:26, danakj wrote: > https://codereview.chromium.org/328753002/diff/1/cc/trees/layer_tree_impl_uni... > File cc/trees/layer_tree_impl_unittest.cc (right): > > https://codereview.chromium.org/328753002/diff/1/cc/trees/layer_tree_impl_uni... > cc/trees/layer_tree_impl_unittest.cc:1176: > child1->draw_properties().render_target = NULL; > This seems artificial, we don't clear the render target after running > UpdateDrawProperties. > > Is this meant to be a situation where you have a layer that doesn't draw > anything, and is therefore skipped my UpdateDrawProperties? How about using > SetIsDrawable(false) on some layer in this test to do that? This is the tip of the wrongness iceberg. The nullity check is bogus. It turns out that a NULL pointer isn't the problem, it's a _stale_ pointer. So what needed to be done was to correctly update the current render_target safely as we walk up our clipping ancestors. I've cleaned up this routine as well as I found it a bit hard to follow, especially with the logic changes I added. Hopefully you find it more readable :)
https://codereview.chromium.org/328753002/diff/40001/cc/trees/layer_tree_impl.cc File cc/trees/layer_tree_impl.cc (right): https://codereview.chromium.org/328753002/diff/40001/cc/trees/layer_tree_impl... cc/trees/layer_tree_impl.cc:1132: gfx::Rect(layer->drawable_content_rect()), no gfx::Rect needed is there some guarantee that layer->IsDrawnRenderSurfaceLayerListMember() is true here ie the rect is valid/fresh?
On 2014/06/11 17:43:06, danakj wrote: > https://codereview.chromium.org/328753002/diff/40001/cc/trees/layer_tree_impl.cc > File cc/trees/layer_tree_impl.cc (right): > > https://codereview.chromium.org/328753002/diff/40001/cc/trees/layer_tree_impl... > cc/trees/layer_tree_impl.cc:1132: gfx::Rect(layer->drawable_content_rect()), > no gfx::Rect needed > > is there some guarantee that layer->IsDrawnRenderSurfaceLayerListMember() is > true here ie the rect is valid/fresh? I think there is, but it'd be brittle. I'd need to add a draw-props-are-fresh bit so we could lock in that guarantee. In any case, it looks we don't need the drawable_content_rect here at all. Updated to use the content bounds and the screen space transform of the layer itself. (Also added a unit test related to the freshness of the screen space transform).
https://codereview.chromium.org/328753002/diff/80001/cc/trees/layer_tree_host... File cc/trees/layer_tree_host_common_unittest.cc (right): https://codereview.chromium.org/328753002/diff/80001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_common_unittest.cc:200: EXPECT_FALSE(child->screen_space_transform().IsIdentity()); Can you verify content_bounds() as well since you make use of that? https://codereview.chromium.org/328753002/diff/80001/cc/trees/layer_tree_impl... File cc/trees/layer_tree_impl_unittest.cc (right): https://codereview.chromium.org/328753002/diff/80001/cc/trees/layer_tree_impl... cc/trees/layer_tree_impl_unittest.cc:1189: EXPECT_EQ(4, result_layer->id()); Mmh, you're touch-interacting with grandchild1, but the whole subtree is hidden? That sounds weird.. is that what you really want? I should point out that SetHideLayerAndSubtree() is currently never used by blink only by the browser compositor, so I'm not sure if this is testing what you mean it to.
https://codereview.chromium.org/328753002/diff/80001/cc/trees/layer_tree_host... File cc/trees/layer_tree_host_common_unittest.cc (right): https://codereview.chromium.org/328753002/diff/80001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_common_unittest.cc:200: EXPECT_FALSE(child->screen_space_transform().IsIdentity()); On 2014/06/11 22:15:06, danakj wrote: > Can you verify content_bounds() as well since you make use of that? I've added a check for non-empty content bounds, too. Is that what you meant? https://codereview.chromium.org/328753002/diff/80001/cc/trees/layer_tree_impl... File cc/trees/layer_tree_impl_unittest.cc (right): https://codereview.chromium.org/328753002/diff/80001/cc/trees/layer_tree_impl... cc/trees/layer_tree_impl_unittest.cc:1189: EXPECT_EQ(4, result_layer->id()); On 2014/06/11 22:15:06, danakj wrote: > Mmh, you're touch-interacting with grandchild1, but the whole subtree is hidden? > That sounds weird.. is that what you really want? It is weird, but yeah, it was intentional. We still need to detect bits of the tree with touch handlers even if they're not drawn. > I should point out that SetHideLayerAndSubtree() is currently never used by > blink only by the browser compositor, so I'm not sure if this is testing what > you mean it to. This was one of a few ways I could make SubtreeShouldBeSkipped return true. I've switched it to opacity = 0.
https://codereview.chromium.org/328753002/diff/80001/cc/trees/layer_tree_impl... File cc/trees/layer_tree_impl_unittest.cc (right): https://codereview.chromium.org/328753002/diff/80001/cc/trees/layer_tree_impl... cc/trees/layer_tree_impl_unittest.cc:1189: EXPECT_EQ(4, result_layer->id()); On 2014/06/11 23:50:37, Ian Vollick wrote: > On 2014/06/11 22:15:06, danakj wrote: > > Mmh, you're touch-interacting with grandchild1, but the whole subtree is > hidden? > > That sounds weird.. is that what you really want? > > It is weird, but yeah, it was intentional. We still need to detect bits of the > tree with touch handlers even if they're not drawn. > > > I should point out that SetHideLayerAndSubtree() is currently never used by > > blink only by the browser compositor, so I'm not sure if this is testing what > > you mean it to. > > This was one of a few ways I could make SubtreeShouldBeSkipped return true. I've > switched it to opacity = 0. Well, hm. Isn't the invariant that you have a valid transform if you have a descendant that draws? But if you're touching a leaf node that doesn't draw, then how do you know it or any ancestor have a valid transform?
On 2014/06/12 00:21:23, danakj wrote: > https://codereview.chromium.org/328753002/diff/80001/cc/trees/layer_tree_impl... > File cc/trees/layer_tree_impl_unittest.cc (right): > > https://codereview.chromium.org/328753002/diff/80001/cc/trees/layer_tree_impl... > cc/trees/layer_tree_impl_unittest.cc:1189: EXPECT_EQ(4, result_layer->id()); > On 2014/06/11 23:50:37, Ian Vollick wrote: > > On 2014/06/11 22:15:06, danakj wrote: > > > Mmh, you're touch-interacting with grandchild1, but the whole subtree is > > hidden? > > > That sounds weird.. is that what you really want? > > > > It is weird, but yeah, it was intentional. We still need to detect bits of the > > tree with touch handlers even if they're not drawn. > > > > > I should point out that SetHideLayerAndSubtree() is currently never used by > > > blink only by the browser compositor, so I'm not sure if this is testing > what > > > you mean it to. > > > > This was one of a few ways I could make SubtreeShouldBeSkipped return true. > I've > > switched it to opacity = 0. > > Well, hm. Isn't the invariant that you have a valid transform if you have a > descendant that draws? But if you're touching a leaf node that doesn't draw, > then how do you know it or any ancestor have a valid transform? That's a good point, and I actually think you've gotten at the heart of the problem. The real issue is that we allow layers to participate in hit testing but do not have up-to-date draw properties. We wouldn't be seeing the crash if the layer's draw properties had been computed because all of its ancestors would have valid render targets. I still think that this CL (minus the fishy test) is a good cleanup, but I think that https://codereview.chromium.org/335633003/ is the fix. I'll revamp and rephrase this CL as a code health thing.
On 2014/06/12 14:21:02, Ian Vollick wrote: > On 2014/06/12 00:21:23, danakj wrote: > > > https://codereview.chromium.org/328753002/diff/80001/cc/trees/layer_tree_impl... > > File cc/trees/layer_tree_impl_unittest.cc (right): > > > > > https://codereview.chromium.org/328753002/diff/80001/cc/trees/layer_tree_impl... > > cc/trees/layer_tree_impl_unittest.cc:1189: EXPECT_EQ(4, result_layer->id()); > > On 2014/06/11 23:50:37, Ian Vollick wrote: > > > On 2014/06/11 22:15:06, danakj wrote: > > > > Mmh, you're touch-interacting with grandchild1, but the whole subtree is > > > hidden? > > > > That sounds weird.. is that what you really want? > > > > > > It is weird, but yeah, it was intentional. We still need to detect bits of > the > > > tree with touch handlers even if they're not drawn. > > > > > > > I should point out that SetHideLayerAndSubtree() is currently never used > by > > > > blink only by the browser compositor, so I'm not sure if this is testing > > what > > > > you mean it to. > > > > > > This was one of a few ways I could make SubtreeShouldBeSkipped return true. > > I've > > > switched it to opacity = 0. > > > > Well, hm. Isn't the invariant that you have a valid transform if you have a > > descendant that draws? But if you're touching a leaf node that doesn't draw, > > then how do you know it or any ancestor have a valid transform? > > That's a good point, and I actually think you've gotten at the heart of the > problem. > > The real issue is that we allow layers to participate in hit testing but do not > have up-to-date draw properties. We wouldn't be seeing the crash if the layer's > draw properties had been computed because all of its ancestors would have valid > render targets. I still think that this CL (minus the fishy test) is a good > cleanup, but I think that https://codereview.chromium.org/335633003/ is the fix. > I'll revamp and rephrase this CL as a code health thing. Updated, PTAL!
LGTM
The CQ bit was checked by vollick@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vollick@chromium.org/328753002/140001
Message was sent while issue was closed.
Change committed as 276799 |
