Clip a layer when its render target's render surface is clipped.

Clip a layer when its render target's render surface is clipped.

An empty clip rect of a render surface does not necessarily mean
the surface is unclippped.

BUG=435797
Committed: https://crrev.com/887bb5444a8474c45fda32bbd750f1a72a21ec49
Cr-Commit-Position: refs/heads/master@{#306481}

[hush (inactive), 2014-11-22 03:58:21]

hush@chromium.org changed reviewers:
+ boliu@chromium.org, danakj@chromium.org

[hush (inactive), 2014-11-22 04:03:02]

Hi!
PTAL!

A bunch of cc_unittests failed by the way. Not sure if it is the tests that are
making any wrong assumptions or not. But we will get to that later.

Dana:
is this the right thing to do?

[hush (inactive), 2014-11-24 20:25:27]

PS2: set the viewport size to a non-empty rect before the failed tests.

[hush (inactive), 2014-11-24 20:28:29]

Patchset #2 (id:20001) has been deleted

[hush (inactive), 2014-11-26 18:56:57]

hush@chromium.org changed reviewers:
+ enne@chromium.org

[hush (inactive), 2014-11-26 18:57:15]

enne@: could you take a look?

[hush (inactive), 2014-11-26 19:02:35]

hush@chromium.org changed reviewers:
- danakj@chromium.org

[danakj, 2014-11-26 19:05:09]

danakj@chromium.org changed reviewers:
+ danakj@chromium.org

[danakj, 2014-11-26 19:05:10]

https://codereview.chromium.org/751453005/diff/40001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_common.cc (right):

https://codereview.chromium.org/751453005/diff/40001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_common.cc:395: if
(layer->render_target()->render_surface()->is_clipped()) {
Sorry for the OOO delays. This does look better. Can you add a
LayerTreeHostCommon unit test that would fail before this change and pass after
it?

[hush (inactive), 2014-11-26 19:06:59]

https://codereview.chromium.org/751453005/diff/40001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_common.cc (right):

https://codereview.chromium.org/751453005/diff/40001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_common.cc:395: if
(layer->render_target()->render_surface()->is_clipped()) {
On 2014/11/26 19:05:09, danakj wrote:
> Sorry for the OOO delays. This does look better. Can you add a
> LayerTreeHostCommon unit test that would fail before this change and pass
after
> it?

Sure. I will do it in the next patch

[hush (inactive), 2014-11-26 22:30:09]

PTAL PS3.
I've made sure the test fails with the old if-statement.

[danakj, 2014-12-02 17:27:16]

Thanks for the test! LGTM w/ some nits

https://codereview.chromium.org/751453005/diff/60001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_common_unittest.cc (right):

https://codereview.chromium.org/751453005/diff/60001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_common_unittest.cc:2854: root.get(), gfx::Size(0, 0),
&render_surface_layer_list);
nit: gfx::Size()

https://codereview.chromium.org/751453005/diff/60001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_common_unittest.cc:2860: const gfx::Rect empty(0, 0, 0,
0);
nit: no const, or kEmpty. I prefer no const in this case.
nit nit: just use default constructor "gfx::Rect empty;" (or you can compare
directly against gfx::Rect() in the EXPECT calls)

https://codereview.chromium.org/751453005/diff/60001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_common_unittest.cc:2861: EXPECT_RECT_EQ(empty,
root->render_surface()->clip_rect());
just use EXPECT_EQ please, it knows about rects now

https://codereview.chromium.org/751453005/diff/60001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_common_unittest.cc:2867: EXPECT_RECT_EQ(empty,
child1->visible_content_rect());
expect_eq etc

[hush (inactive), 2014-12-02 18:12:36]

https://codereview.chromium.org/751453005/diff/60001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_common_unittest.cc (right):

https://codereview.chromium.org/751453005/diff/60001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_common_unittest.cc:2854: root.get(), gfx::Size(0, 0),
&render_surface_layer_list);
On 2014/12/02 17:27:16, danakj wrote:
> nit: gfx::Size()

Done.

https://codereview.chromium.org/751453005/diff/60001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_common_unittest.cc:2861: EXPECT_RECT_EQ(empty,
root->render_surface()->clip_rect());
On 2014/12/02 17:27:16, danakj wrote:
> just use EXPECT_EQ please, it knows about rects now

Done.

And there is a bunch of other places that use EXPECT_RECT_EQ in this file.
I will do the same in a follow up CL.

https://codereview.chromium.org/751453005/diff/60001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_common_unittest.cc:2867: EXPECT_RECT_EQ(empty,
child1->visible_content_rect());
On 2014/12/02 17:27:16, danakj wrote:
> expect_eq etc

Done.

[danakj, 2014-12-02 19:48:44]

Thanks that'd be great.

[hush (inactive), 2014-12-02 21:47:42]

The CQ bit was checked by hush@chromium.org

[commit-bot: I haz the power, 2014-12-02 21:48:42]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/751453005/80001

[commit-bot: I haz the power, 2014-12-02 22:49:12]

Committed patchset #4 (id:80001)

[commit-bot: I haz the power, 2014-12-02 22:49:53]

Patchset 4 (id:??) landed as
https://crrev.com/887bb5444a8474c45fda32bbd750f1a72a21ec49
Cr-Commit-Position: refs/heads/master@{#306481}