Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1252313004:
Add ClipNode when Render Surface Inherits Clip (Closed)
DescriptionAdd ClipNode when Render Surface Inherits Clip.
Propertytree transforms and the CDP version are diffrent in the case where a layer :
has_render_surface=true, ancestor_clips_subtee=true and
data_for_children->ancestor_clips_subtee=false.
We need a clip node in this case. This patch adds a clip node for this
case and computes the clip required in this case.
BUG=510185
TEST=visible rects from CDP and property trees match in
http://www.clicktorelease.com/code/css3dclouds with this patch.
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
Committed: https://crrev.com/67d7989e6dad1905bddf00e539ee78e4abcbd606
Cr-Commit-Position: refs/heads/master@{#342209}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Fixed layout test crashes and addressed a few comments on patch 1 #Patch Set 3 : Added a unit test #
Total comments: 14
Patch Set 4 : Map/Project based on condition #Patch Set 5 : Unit test modified #Patch Set 6 : Unit test easier to understand #
Total comments: 2
Patch Set 7 : Add clip node when render surface applies clip and owning layer has non flat transform #
Total comments: 3
Patch Set 8 : to run layout tests #
Total comments: 6
Patch Set 9 : Unit test modified and sub layer scale bug removed #Patch Set 10 : Run Unit test only with property trees #Patch Set 11 : Rebase #
Total comments: 14
Patch Set 12 : Layout tests pass , unit test added , comments on patch 11 #
Total comments: 4
Patch Set 13 : Comments on patch 12 #
Messages
Total messages: 32 (2 generated)
jaydasika@chromium.org changed reviewers: + ajuma@chromium.org
Please also change the title to say what the patch does. https://codereview.chromium.org/1252313004/diff/1/cc/trees/draw_property_util... File cc/trees/draw_property_utils.cc (right): https://codereview.chromium.org/1252313004/diff/1/cc/trees/draw_property_util... cc/trees/draw_property_utils.cc:422: target_id, &parent_to_target); I'm having trouble following this. parent_to_target used to be the transformation from the parent transform node to the target node (so, in the example given in the comment above, it would be the transform from A to T). But here it's the transform from the transform node's render surface to the target (and, depending on the transform tree, the transform node's render surface might be the same as target, or might be the target's target). I'd have expected this change to also require a change below (where we use parent_to_target to transform the parent_clip_node's combined_clip from parent space to target space). https://codereview.chromium.org/1252313004/diff/1/cc/trees/draw_property_util... cc/trees/draw_property_utils.cc:455: if (parent_transform_node->id > target_id && We weren't previously handling the case where parent_transform_node->id < target_id, so that was itself probably causing bugs. Please add (at least) two test cases to layer_tree_host_common_unittest.cc showing what this change fixes: one where we have parent_transform_node->id < target_id, and another where we have render_surface_applies_clip.
https://codereview.chromium.org/1252313004/diff/1/cc/trees/draw_property_util... File cc/trees/draw_property_utils.cc (right): https://codereview.chromium.org/1252313004/diff/1/cc/trees/draw_property_util... cc/trees/draw_property_utils.cc:422: target_id, &parent_to_target); On 2015/07/29 16:44:29, ajuma wrote: > I'm having trouble following this. parent_to_target used to be the > transformation from the parent transform node to the target node (so, in the > example given in the comment above, it would be the transform from A to T). But > here it's the transform from the transform node's render surface to the target > (and, depending on the transform tree, the transform node's render surface might > be the same as target, or might be the target's target). I'd have expected this > change to also require a change below (where we use parent_to_target to > transform the parent_clip_node's combined_clip from parent space to target > space). Done. https://codereview.chromium.org/1252313004/diff/1/cc/trees/draw_property_util... cc/trees/draw_property_utils.cc:455: if (parent_transform_node->id > target_id && On 2015/07/29 16:44:29, ajuma wrote: > We weren't previously handling the case where parent_transform_node->id < > target_id, so that was itself probably causing bugs. > > Please add (at least) two test cases to layer_tree_host_common_unittest.cc > showing what this change fixes: one where we have parent_transform_node->id < > target_id, and another where we have render_surface_applies_clip. Add a unit test where render surface applies clip. I am not sure if the other case ever occurs.
https://codereview.chromium.org/1252313004/diff/40001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_common_unittest.cc (right): https://codereview.chromium.org/1252313004/diff/40001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_common_unittest.cc:8243: TEST_F(LayerTreeHostCommonTest, RenderSurfaceClipsSubtree) { This unit test will crash without this patch.
https://codereview.chromium.org/1252313004/diff/40001/cc/trees/draw_property_... File cc/trees/draw_property_utils.cc (right): https://codereview.chromium.org/1252313004/diff/40001/cc/trees/draw_property_... cc/trees/draw_property_utils.cc:423: // When render surface applies clip, we need the clip from the target's Is it that we need a clip "from" that space, or that we actually need to perform the clip "in" that space? https://codereview.chromium.org/1252313004/diff/40001/cc/trees/draw_property_... cc/trees/draw_property_utils.cc:469: parent_clip_node->data.combined_clip); Will it always be true that in this case, the parent node will be an ancestor of the transform target node (since we're using Project rather than Map)? I think that makes sense, but please add a DCHECK. https://codereview.chromium.org/1252313004/diff/40001/cc/trees/draw_property_... cc/trees/draw_property_utils.cc:471: transform_target_to_target, combined_clip_in_transform_target_space); The combined effect of this line and the line above is to project parent_clip_node->data.combined_clip from parent space to target space. What goes wrong (geometrically) if we just do this projection in a single step? https://codereview.chromium.org/1252313004/diff/40001/cc/trees/draw_property_... cc/trees/draw_property_utils.cc:482: // stored in the clip node. Just to see if I understand: the "layer" here is the owner of the render surface, and instead of clipping to its own bounds, it ignores its own bounds and applies the clip inherited from its parent? If that's the case, would it be equivalent to simply not create a clip node for this layer, and allow the clip tree to propagate the clip? https://codereview.chromium.org/1252313004/diff/40001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_common_unittest.cc (right): https://codereview.chromium.org/1252313004/diff/40001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_common_unittest.cc:8255: +0.04, +0.15, -1.19, +0.12, +17.4304, 0, 0, 0, 1); I assume these numbers come from an actual page where you observed this, but is it possible to have 'nicer' transforms and still have a test that fails at ToT but passes with your patch? That would make this test a lot easier to understand. https://codereview.chromium.org/1252313004/diff/40001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_common_unittest.cc:8281: EXPECT_TRUE(node->data.render_surface_applies_clip); Please also add an expectation for the test_layer's visible rect (since that's what's ultimately being fixed by this patch). https://codereview.chromium.org/1252313004/diff/40001/cc/trees/property_tree_... File cc/trees/property_tree_builder.cc (right): https://codereview.chromium.org/1252313004/diff/40001/cc/trees/property_tree_... cc/trees/property_tree_builder.cc:143: layer->has_render_surface() && ancestor_clips_subtree; What happens if we use this condition as a reason to *not* create a ClipNode?
https://codereview.chromium.org/1252313004/diff/40001/cc/trees/draw_property_... File cc/trees/draw_property_utils.cc (right): https://codereview.chromium.org/1252313004/diff/40001/cc/trees/draw_property_... cc/trees/draw_property_utils.cc:423: // When render surface applies clip, we need the clip from the target's On 2015/07/30 20:56:50, ajuma wrote: > Is it that we need a clip "from" that space, or that we actually need to perform > the clip "in" that space? We need the clip in "that" space projected into current target space. https://codereview.chromium.org/1252313004/diff/40001/cc/trees/draw_property_... cc/trees/draw_property_utils.cc:469: parent_clip_node->data.combined_clip); On 2015/07/30 20:56:50, ajuma wrote: > Will it always be true that in this case, the parent node will be an ancestor of > the transform target node (since we're using Project rather than Map)? I think > that makes sense, but please add a DCHECK. It is not always true that parent node will be ancestor of the transform target node as transform target is target of the render surface and not the surface itself. https://codereview.chromium.org/1252313004/diff/40001/cc/trees/draw_property_... cc/trees/draw_property_utils.cc:471: transform_target_to_target, combined_clip_in_transform_target_space); On 2015/07/30 20:56:50, ajuma wrote: > The combined effect of this line and the line above is to project > parent_clip_node->data.combined_clip from parent space to target space. What > goes wrong (geometrically) if we just do this projection in a single step? Since we might map/project(next patch), I don't think they can be combined? https://codereview.chromium.org/1252313004/diff/40001/cc/trees/draw_property_... cc/trees/draw_property_utils.cc:482: // stored in the clip node. On 2015/07/30 20:56:50, ajuma wrote: > Just to see if I understand: the "layer" here is the owner of the render > surface, and instead of clipping to its own bounds, it ignores its own bounds > and applies the clip inherited from its parent? > If that's the case, would it be equivalent to simply not create a clip node for > this layer, and allow the clip tree to propagate the clip? Yes, it ignores its own bounds. It uses clip inherited from ancestor "intersected" with projection of clip rect of its render surface's target into the render surface's space. We need the clip node for the second rect. https://codereview.chromium.org/1252313004/diff/40001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_common_unittest.cc (right): https://codereview.chromium.org/1252313004/diff/40001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_common_unittest.cc:8255: +0.04, +0.15, -1.19, +0.12, +17.4304, 0, 0, 0, 1); On 2015/07/30 20:56:50, ajuma wrote: > I assume these numbers come from an actual page where you observed this, but is > it possible to have 'nicer' transforms and still have a test that fails at ToT > but passes with your patch? That would make this test a lot easier to > understand. I am still working on it. transform2 has to be such that it is not axis aligned with parent. https://codereview.chromium.org/1252313004/diff/40001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_common_unittest.cc:8281: EXPECT_TRUE(node->data.render_surface_applies_clip); On 2015/07/30 20:56:50, ajuma wrote: > Please also add an expectation for the test_layer's visible rect (since that's > what's ultimately being fixed by this patch). Done.
Added an 'easy to understand' unit test that fails without the patch and passes with it.
https://codereview.chromium.org/1252313004/diff/100001/cc/trees/draw_property... File cc/trees/draw_property_utils.cc (right): https://codereview.chromium.org/1252313004/diff/100001/cc/trees/draw_property... cc/trees/draw_property_utils.cc:477: inherited_clip_in_target_space = MathUtil::ProjectClippedRect( I am not sure if this is ever reached as it did not exist before this patch though I think it should as it is possible that target is an ancestor of parent. Should I leave it or delete it?
https://codereview.chromium.org/1252313004/diff/100001/cc/trees/draw_property... File cc/trees/draw_property_utils.cc (right): https://codereview.chromium.org/1252313004/diff/100001/cc/trees/draw_property... cc/trees/draw_property_utils.cc:477: inherited_clip_in_target_space = MathUtil::ProjectClippedRect( On 2015/07/31 17:34:14, jaydasika wrote: > I am not sure if this is ever reached as it did not exist before this patch > though I think it should as it is possible that target is an ancestor of parent. > Should I leave it or delete it? I think you meant "parent is an ancestor of target" (since the current logic assumes the target is an ancestor of parent, since it Maps rather than Projects). Wouldn't this happen for most clip nodes owned by a render surface? Since in that case, the target would be the render surface's transform node, and the parent clip node's transform node would be some ancestor. Perhaps trying adding a DCHECK that this doesn't happen, and see if it any layout tests hit it. We can also try to work through an example together next week.
Added a patch that forces clip node creation when render surface applies clip and owning layer has non flat transform.
https://codereview.chromium.org/1252313004/diff/120001/cc/trees/property_tree... File cc/trees/property_tree_builder.cc (right): https://codereview.chromium.org/1252313004/diff/120001/cc/trees/property_tree... cc/trees/property_tree_builder.cc:82: if (!layer->transform().IsFlat()) What if the transform is animated, and becomes non-flat on the compositor thread (after tree creation)? How about just unconditionally creating a node if we reach this point (and then we can get rid of the axis-alignment logic below)?
https://codereview.chromium.org/1252313004/diff/120001/cc/trees/property_tree... File cc/trees/property_tree_builder.cc (right): https://codereview.chromium.org/1252313004/diff/120001/cc/trees/property_tree... cc/trees/property_tree_builder.cc:82: if (!layer->transform().IsFlat()) On 2015/08/04 15:37:19, ajuma wrote: > What if the transform is animated, and becomes non-flat on the compositor thread > (after tree creation)? How about just unconditionally creating a node if we > reach this point (and then we can get rid of the axis-alignment logic below)? That breaks a lot of unit tests.
https://codereview.chromium.org/1252313004/diff/120001/cc/trees/property_tree... File cc/trees/property_tree_builder.cc (right): https://codereview.chromium.org/1252313004/diff/120001/cc/trees/property_tree... cc/trees/property_tree_builder.cc:82: if (!layer->transform().IsFlat()) On 2015/08/04 15:38:53, jaydasika wrote: > On 2015/08/04 15:37:19, ajuma wrote: > > What if the transform is animated, and becomes non-flat on the compositor > thread > > (after tree creation)? How about just unconditionally creating a node if we > > reach this point (and then we can get rid of the axis-alignment logic below)? > > That breaks a lot of unit tests. It'd be good to understand why (since if there's something incorrect about that, there might also be something incorrect about the non-flat case too, but we maybe we just don't have that many unit tests with non-flat transforms).
Update: 1) Added patch to create clip node whenever we have the "new case" except for root (irrespective of axis alignment) 2) For the case of singular transforms, set combined clip = clip. But for the "new case", we don't want to use the clip of layer creating the node. So, for now, I have copied values from parent clip node to create illusion that this clip node is never created.(I am still thinking of a cleaner way to do it). 3) The layer that creates the clip node in the new case should itself use parent clip node as its clip tree index. (I am not sure why, but it looks like CDP does the same thing and it breaks a few unittests if I don't do the same). 4) In the old code, we always Map(don't Project). There are many cases in unittests where we should be projecting as parent transform node is ancestor of current target. Unittests don't fail currently because in all these unittests map=project **The patch I have just uploaded is just to run layout tests, it still needs some clean up
On 2015/08/04 23:01:15, jaydasika wrote: > 3) The layer that creates the clip node in the new case should itself use parent > clip node as its clip tree index. (I am not sure why, but it looks like CDP does > the same thing and it breaks a few unittests if I don't do the same). Please look into why this is needed. Having a clip node whose owner has a different clip tree index seems confusing. https://codereview.chromium.org/1252313004/diff/140001/cc/trees/draw_property... File cc/trees/draw_property_utils.cc (right): https://codereview.chromium.org/1252313004/diff/140001/cc/trees/draw_property... cc/trees/draw_property_utils.cc:460: clip_node->data.transform_id = parent_clip_node->data.transform_id; I don't think we should be changing the transform id here -- that should only change during tree building. Otherwise, suppose the value of the transform that's currently uninvertible changes on the next frame, such that it becomes invertible. If we've changed the transform_id here, we're going to do the wrong thing, I think. https://codereview.chromium.org/1252313004/diff/140001/cc/trees/property_tree... File cc/trees/property_tree_builder.cc (right): https://codereview.chromium.org/1252313004/diff/140001/cc/trees/property_tree... cc/trees/property_tree_builder.cc:85: bool axis_aligned_with_respect_to_parent = Will this ever be true now? If we reach this point, we're at the root layer, which can't be non-axis aligned.
> Please look into why this is needed. Having a clip node whose owner has a > different clip tree index seems confusing. One difference here is layer->is_clipped() is false(for the owning layer). So drawable content rect in CDP is rect in target space and not clip rect in target space. But, I am still not convinced we should be using parent clip tree index. I put it in the patch because it works, but I am not sure why.
https://codereview.chromium.org/1252313004/diff/140001/cc/trees/draw_property... File cc/trees/draw_property_utils.cc (right): https://codereview.chromium.org/1252313004/diff/140001/cc/trees/draw_property... cc/trees/draw_property_utils.cc:460: clip_node->data.transform_id = parent_clip_node->data.transform_id; On 2015/08/05 13:51:09, ajuma wrote: > I don't think we should be changing the transform id here -- that should only > change during tree building. Otherwise, suppose the value of the transform > that's currently uninvertible changes on the next frame, such that it becomes > invertible. If we've changed the transform_id here, we're going to do the wrong > thing, I think. Ah, I see the problem. But I am still not sure what the alternative is? https://codereview.chromium.org/1252313004/diff/140001/cc/trees/property_tree... File cc/trees/property_tree_builder.cc (right): https://codereview.chromium.org/1252313004/diff/140001/cc/trees/property_tree... cc/trees/property_tree_builder.cc:85: bool axis_aligned_with_respect_to_parent = On 2015/08/05 13:51:09, ajuma wrote: > Will this ever be true now? If we reach this point, we're at the root layer, > which can't be non-axis aligned. Done.
On 2015/08/05 15:07:09, jaydasika wrote: > > Please look into why this is needed. Having a clip node whose owner has a > > different clip tree index seems confusing. > > One difference here is layer->is_clipped() is false(for the owning layer). > So drawable content rect in CDP is rect in target space and not clip rect in > target space. > But, I am still not convinced we should be using parent clip tree index. I put > it in the patch because it works, but I am not sure why. Keep in mind that even though it works on the tests we have, it won't necessarily work in general. So it's worth understanding why this (or any other non-obvious change) makes tests pass, at least to a point of being able to write a meaningful comment so that someone reading this later understands why it needs to behave the way it does. Can the need to treat the node owner specially be addressed by adding logic to CalculateVisibleRects (just like, when computing draw transforms using property trees, there's special case logic for layers that own render surfaces in DrawTransformFromPropretyTreesInternal)?
https://codereview.chromium.org/1252313004/diff/140001/cc/trees/draw_property... File cc/trees/draw_property_utils.cc (right): https://codereview.chromium.org/1252313004/diff/140001/cc/trees/draw_property... cc/trees/draw_property_utils.cc:460: clip_node->data.transform_id = parent_clip_node->data.transform_id; On 2015/08/05 15:07:20, jaydasika wrote: > On 2015/08/05 13:51:09, ajuma wrote: > > I don't think we should be changing the transform id here -- that should only > > change during tree building. Otherwise, suppose the value of the transform > > that's currently uninvertible changes on the next frame, such that it becomes > > invertible. If we've changed the transform_id here, we're going to do the > wrong > > thing, I think. > > Ah, I see the problem. But I am still not sure what the alternative is? If we reach this point, we have an uninvertible transform, so we just need to have some reasonable fallback. For example, there are places where CDP pretends that an uninvertible transform's inverse is the identity. I think the equivalent here might be to set the combined clip as you do on the next line, but leave the transform id as-is.
> If we reach this point, we have an uninvertible transform, so we just need to > have some reasonable fallback. For example, there are places where CDP pretends > that an uninvertible transform's inverse is the identity. I think the equivalent > here might be to set the combined clip as you do on the next line, but leave the > transform id as-is. But the combined clip is in parent space.
On 2015/08/05 15:27:12, jaydasika wrote: > > If we reach this point, we have an uninvertible transform, so we just need to > > have some reasonable fallback. For example, there are places where CDP > pretends > > that an uninvertible transform's inverse is the identity. I think the > equivalent > > here might be to set the combined clip as you do on the next line, but leave > the > > transform id as-is. > But the combined clip is in parent space. Right, and if the transform from local space to parent space is uninvertible, there's no "correct" way to express the combined clip in local space. Or is the problem that the |success| bool isn't sufficiently fine-grained, so in some cases we can actually compute a combined clip in local space? In that case, we should have separate |success| bools.
https://codereview.chromium.org/1252313004/diff/140001/cc/trees/draw_property... File cc/trees/draw_property_utils.cc (right): https://codereview.chromium.org/1252313004/diff/140001/cc/trees/draw_property... cc/trees/draw_property_utils.cc:460: clip_node->data.transform_id = parent_clip_node->data.transform_id; On 2015/08/05 15:21:58, ajuma wrote: > On 2015/08/05 15:07:20, jaydasika wrote: > > On 2015/08/05 13:51:09, ajuma wrote: > > > I don't think we should be changing the transform id here -- that should > only > > > change during tree building. Otherwise, suppose the value of the transform > > > that's currently uninvertible changes on the next frame, such that it > becomes > > > invertible. If we've changed the transform_id here, we're going to do the > > wrong > > > thing, I think. > > > > Ah, I see the problem. But I am still not sure what the alternative is? > > If we reach this point, we have an uninvertible transform, so we just need to > have some reasonable fallback. For example, there are places where CDP pretends > that an uninvertible transform's inverse is the identity. I think the equivalent > here might be to set the combined clip as you do on the next line, but leave the > transform id as-is. Done.
Looks like there's still a layout test failure to sort out. Aside from that, the overall logic looks good. https://codereview.chromium.org/1252313004/diff/200001/cc/trees/draw_property... File cc/trees/draw_property_utils.cc (right): https://codereview.chromium.org/1252313004/diff/200001/cc/trees/draw_property... cc/trees/draw_property_utils.cc:432: success &= transform_tree.ComputeTransformWithSourceSublayerScale( ComputeTransformWithSourceSublayerScale can divide by zero if the source has sublayer scale zero. I think we've avoided problems with this so far just because the other caller happens to get skipped when we have zero sublayer scale. Please add a check for division by zero there (and make it return false if it's about to divide by zero). https://codereview.chromium.org/1252313004/diff/200001/cc/trees/draw_property... cc/trees/draw_property_utils.cc:488: // When render surface applies clip, the layer that created the clip node s/applies clip/inherits its parent's target space clip/ https://codereview.chromium.org/1252313004/diff/200001/cc/trees/draw_property... cc/trees/draw_property_utils.cc:489: // doesn't apply any clip. So, we should clip using the clip value Do you mean "we shouldn't clip" instead? (Since in that case, we're ignoring data.clip.) https://codereview.chromium.org/1252313004/diff/200001/cc/trees/layer_tree_ho... File cc/trees/layer_tree_host_common_unittest.cc (right): https://codereview.chromium.org/1252313004/diff/200001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_common_unittest.cc:6957: // |surface| and layers have empty visible rect. "|surface| and layers that draw into it as having empty visible rects." https://codereview.chromium.org/1252313004/diff/200001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_common_unittest.cc:7834: test_layer->visible_layer_rect().ToString()); Please compare to an actual value rather than to CDP's output, so that this test continues to be meaningful once CDP is deleted. https://codereview.chromium.org/1252313004/diff/200001/cc/trees/property_tree.h File cc/trees/property_tree.h (right): https://codereview.chromium.org/1252313004/diff/200001/cc/trees/property_tree... cc/trees/property_tree.h:142: bool inherit_parent_target_space_clip; Please add a comment explaining what this means (since it seems more non-obvious than the other members). https://codereview.chromium.org/1252313004/diff/200001/cc/trees/property_tree... File cc/trees/property_tree_builder.cc (right): https://codereview.chromium.org/1252313004/diff/200001/cc/trees/property_tree... cc/trees/property_tree_builder.cc:82: if (layer->parent()) return !!layer-parent();
Patch 12 passes layout test failing with patch 11. Added a unit test that crashes without this patch (because we always Map). https://codereview.chromium.org/1252313004/diff/200001/cc/trees/draw_property... File cc/trees/draw_property_utils.cc (right): https://codereview.chromium.org/1252313004/diff/200001/cc/trees/draw_property... cc/trees/draw_property_utils.cc:432: success &= transform_tree.ComputeTransformWithSourceSublayerScale( On 2015/08/06 14:25:45, ajuma wrote: > ComputeTransformWithSourceSublayerScale can divide by zero if the source has > sublayer scale zero. I think we've avoided problems with this so far just > because the other caller happens to get skipped when we have zero sublayer > scale. Please add a check for division by zero there (and make it return false > if it's about to divide by zero). Done. https://codereview.chromium.org/1252313004/diff/200001/cc/trees/draw_property... cc/trees/draw_property_utils.cc:488: // When render surface applies clip, the layer that created the clip node On 2015/08/06 14:25:45, ajuma wrote: > s/applies clip/inherits its parent's target space clip/ Done. https://codereview.chromium.org/1252313004/diff/200001/cc/trees/draw_property... cc/trees/draw_property_utils.cc:489: // doesn't apply any clip. So, we should clip using the clip value On 2015/08/06 14:25:45, ajuma wrote: > Do you mean "we shouldn't clip" instead? (Since in that case, we're ignoring > data.clip.) Done. https://codereview.chromium.org/1252313004/diff/200001/cc/trees/layer_tree_ho... File cc/trees/layer_tree_host_common_unittest.cc (right): https://codereview.chromium.org/1252313004/diff/200001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_common_unittest.cc:6957: // |surface| and layers have empty visible rect. On 2015/08/06 14:25:46, ajuma wrote: > "|surface| and layers that draw into it as having empty visible rects." Done. https://codereview.chromium.org/1252313004/diff/200001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_common_unittest.cc:7834: test_layer->visible_layer_rect().ToString()); On 2015/08/06 14:25:45, ajuma wrote: > Please compare to an actual value rather than to CDP's output, so that this test > continues to be meaningful once CDP is deleted. Done. https://codereview.chromium.org/1252313004/diff/200001/cc/trees/property_tree.h File cc/trees/property_tree.h (right): https://codereview.chromium.org/1252313004/diff/200001/cc/trees/property_tree... cc/trees/property_tree.h:142: bool inherit_parent_target_space_clip; On 2015/08/06 14:25:46, ajuma wrote: > Please add a comment explaining what this means (since it seems more non-obvious > than the other members). Done. https://codereview.chromium.org/1252313004/diff/200001/cc/trees/property_tree... File cc/trees/property_tree_builder.cc (right): https://codereview.chromium.org/1252313004/diff/200001/cc/trees/property_tree... cc/trees/property_tree_builder.cc:82: if (layer->parent()) On 2015/08/06 14:25:46, ajuma wrote: > return !!layer-parent(); Done.
Thanks! Just two more comments about the tests. https://codereview.chromium.org/1252313004/diff/220001/cc/trees/layer_tree_ho... File cc/trees/layer_tree_host_common_unittest.cc (right): https://codereview.chromium.org/1252313004/diff/220001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_common_unittest.cc:7830: ClipNode* clip_node = clip_tree.Node(3); Rather than hardcoding a node id here (which seems fragile, since a future change that makes us create more/less nodes might break this without being "wrong"), I'd rather that we just use the clip_tree_index from |render_surface|. https://codereview.chromium.org/1252313004/diff/220001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_common_unittest.cc:7867: ClipNode* clip_node = clip_tree.Node(3); Same comment as above: use the clip_tree_index from target_layer instead of hardcoding a value.
https://codereview.chromium.org/1252313004/diff/220001/cc/trees/layer_tree_ho... File cc/trees/layer_tree_host_common_unittest.cc (right): https://codereview.chromium.org/1252313004/diff/220001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_common_unittest.cc:7830: ClipNode* clip_node = clip_tree.Node(3); On 2015/08/06 18:06:31, ajuma wrote: > Rather than hardcoding a node id here (which seems fragile, since a future > change that makes us create more/less nodes might break this without being > "wrong"), I'd rather that we just use the clip_tree_index from |render_surface|. Done. https://codereview.chromium.org/1252313004/diff/220001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_common_unittest.cc:7867: ClipNode* clip_node = clip_tree.Node(3); On 2015/08/06 18:06:31, ajuma wrote: > Same comment as above: use the clip_tree_index from target_layer instead of > hardcoding a value. Done.
lgtm
The CQ bit was checked by jaydasika@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1252313004/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1252313004/240001
Message was sent while issue was closed.
Committed patchset #13 (id:240001)
Message was sent while issue was closed.
Patchset 13 (id:??) landed as https://crrev.com/67d7989e6dad1905bddf00e539ee78e4abcbd606 Cr-Commit-Position: refs/heads/master@{#342209} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
