cc: Set clip rect to empty rect when layer's is_clipped is false

cc: Set clip rect to empty rect when layer's is_clipped is false

And also verify is_clipped calculated dynamically matches the current
is_clipped value, also verify clip rects even when is_clipped is false

BUG=594675
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

Committed: https://crrev.com/1fc2fd1adf7c5af04f7d8e2fe752b0a31e0dde7c
Cr-Commit-Position: refs/heads/master@{#429723}

[jaydasika, 2016-11-01 22:17:07]

Description was changed from

==========
cc: Set clip rect to empty rect when layer's is_clipped is false

And also verify is_clipped calculated dynamically matches the current
is_clipped value

BUG=594675
==========

to

==========
cc: Set clip rect to empty rect when layer's is_clipped is false

And also verify is_clipped calculated dynamically matches the current
is_clipped value

BUG=594675
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[jaydasika, 2016-11-01 22:21:38]

jaydasika@chromium.org changed reviewers:
+ ajuma@chromium.org, weiliangc@chromium.org

[jaydasika, 2016-11-01 22:21:38]

PTAL

[jaydasika, 2016-11-01 22:21:49]

The CQ bit was checked by jaydasika@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-01 22:22:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jaydasika, 2016-11-01 22:22:56]

Description was changed from

==========
cc: Set clip rect to empty rect when layer's is_clipped is false

And also verify is_clipped calculated dynamically matches the current
is_clipped value

BUG=594675
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
cc: Set clip rect to empty rect when layer's is_clipped is false

And also verify is_clipped calculated dynamically matches the current
is_clipped value, also verify clip rects even when is_clipped is false

BUG=594675
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[commit-bot: I haz the power, 2016-11-01 23:39:15]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-01 23:39:16]

Dry run: This issue passed the CQ dry run.

[weiliangc, 2016-11-02 21:08:07]

Sorry I'm not sure I fully understand why this change is setting clip rect to
empty when layer is not clipped?

https://codereview.chromium.org/2464153004/diff/20001/cc/trees/draw_property_...
File cc/trees/draw_property_utils.cc (left):

https://codereview.chromium.org/2464153004/diff/20001/cc/trees/draw_property_...
cc/trees/draw_property_utils.cc:286: !clip_node->layers_are_clipped &&
!clip_node->target_is_clipped;
Could you explain why target_is_clipped is ok to be removed here?

[jaydasika, 2016-11-02 21:21:47]

On 2016/11/02 21:08:07, weiliangc wrote:
> Sorry I'm not sure I fully understand why this change is setting clip rect to
> empty when layer is not clipped?
> 
>
https://codereview.chromium.org/2464153004/diff/20001/cc/trees/draw_property_...
> File cc/trees/draw_property_utils.cc (left):
> 
>
https://codereview.chromium.org/2464153004/diff/20001/cc/trees/draw_property_...
> cc/trees/draw_property_utils.cc:286: !clip_node->layers_are_clipped &&
> !clip_node->target_is_clipped;
> Could you explain why target_is_clipped is ok to be removed here?

When layer is not clipped, clip rect is never used, so it doesn't matter what
its set to. While calculating clip rects dynamically, we are setting it to be an
empty rect always. I have matched current clip rect computation to do the same
so that verification can be turned on unconditionally (we currently have
verification turned on only when is_clipped is true)

[jaydasika, 2016-11-02 21:21:56]

https://codereview.chromium.org/2464153004/diff/20001/cc/trees/draw_property_...
File cc/trees/draw_property_utils.cc (left):

https://codereview.chromium.org/2464153004/diff/20001/cc/trees/draw_property_...
cc/trees/draw_property_utils.cc:286: !clip_node->layers_are_clipped &&
!clip_node->target_is_clipped;
On 2016/11/02 21:08:07, weiliangc wrote:
> Could you explain why target_is_clipped is ok to be removed here?

if !non_root_surfaces_enabled = true, we wont reach here(line 288 in new code)

When non_root_surface_enabled,
if clip_node->layers_are_clipped = false, we wont reach here (line 283 in new
code).

So, here clip_node->layers_are_clipped = true always => fully_visible = false
always (irrespective of what target_is_clipped is)

[weiliangc, 2016-11-02 21:34:28]

Thanks for the explanation.

https://codereview.chromium.org/2464153004/diff/20001/cc/trees/draw_property_...
File cc/trees/draw_property_utils.cc (left):

https://codereview.chromium.org/2464153004/diff/20001/cc/trees/draw_property_...
cc/trees/draw_property_utils.cc:286: !clip_node->layers_are_clipped &&
!clip_node->target_is_clipped;
On 2016/11/02 at 21:21:56, jaydasika wrote:
> On 2016/11/02 21:08:07, weiliangc wrote:
> > Could you explain why target_is_clipped is ok to be removed here?
> 
> if !non_root_surfaces_enabled = true, we wont reach here(line 288 in new code)
> 
> When non_root_surface_enabled,
> if clip_node->layers_are_clipped = false, we wont reach here (line 283 in new
code).
> 
> So, here clip_node->layers_are_clipped = true always => fully_visible = false
always (irrespective of what target_is_clipped is)

For !non_root_surface_enabled, old cold will set the root layer to
clip_in_target_space, and new code would set it to empty. This is expected?

For non_root_surface_enabled, and layers_are_clipped false, new code will set
clip rect to empty, and old code would set to empty or calculate the clip rect
depends on whether target_is_clipped? For target_is_clipped true case, does it
always ends being empty clip?

[jaydasika, 2016-11-02 21:46:41]

https://codereview.chromium.org/2464153004/diff/20001/cc/trees/draw_property_...
File cc/trees/draw_property_utils.cc (left):

https://codereview.chromium.org/2464153004/diff/20001/cc/trees/draw_property_...
cc/trees/draw_property_utils.cc:286: !clip_node->layers_are_clipped &&
!clip_node->target_is_clipped;
On 2016/11/02 21:34:28, weiliangc wrote:
> On 2016/11/02 at 21:21:56, jaydasika wrote:
> > On 2016/11/02 21:08:07, weiliangc wrote:
> > > Could you explain why target_is_clipped is ok to be removed here?
> > 
> > if !non_root_surfaces_enabled = true, we wont reach here(line 288 in new
code)
> > 
> > When non_root_surface_enabled,
> > if clip_node->layers_are_clipped = false, we wont reach here (line 283 in
new
> code).
> > 
> > So, here clip_node->layers_are_clipped = true always => fully_visible =
false
> always (irrespective of what target_is_clipped is)
> 
> For !non_root_surface_enabled, old cold will set the root layer to
> clip_in_target_space, and new code would set it to empty. This is expected?
> 
> For non_root_surface_enabled, and layers_are_clipped false, new code will set
> clip rect to empty, and old code would set to empty or calculate the clip rect
> depends on whether target_is_clipped? For target_is_clipped true case, does it
> always ends being empty clip?
> 

When layer is not clipped, clip rect is never used, so it doesn't matter what
its set to

[weiliangc, 2016-11-03 19:21:54]

LGTM

https://codereview.chromium.org/2464153004/diff/20001/cc/trees/draw_property_...
File cc/trees/draw_property_utils.cc (left):

https://codereview.chromium.org/2464153004/diff/20001/cc/trees/draw_property_...
cc/trees/draw_property_utils.cc:286: !clip_node->layers_are_clipped &&
!clip_node->target_is_clipped;
On 2016/11/02 at 21:46:40, jaydasika wrote:
> On 2016/11/02 21:34:28, weiliangc wrote:
> > On 2016/11/02 at 21:21:56, jaydasika wrote:
> > > On 2016/11/02 21:08:07, weiliangc wrote:
> > > > Could you explain why target_is_clipped is ok to be removed here?
> > > 
> > > if !non_root_surfaces_enabled = true, we wont reach here(line 288 in new
code)
> > > 
> > > When non_root_surface_enabled,
> > > if clip_node->layers_are_clipped = false, we wont reach here (line 283 in
new
> > code).
> > > 
> > > So, here clip_node->layers_are_clipped = true always => fully_visible =
false
> > always (irrespective of what target_is_clipped is)
> > 
> > For !non_root_surface_enabled, old cold will set the root layer to
> > clip_in_target_space, and new code would set it to empty. This is expected?
> > 
> > For non_root_surface_enabled, and layers_are_clipped false, new code will
set
> > clip rect to empty, and old code would set to empty or calculate the clip
rect
> > depends on whether target_is_clipped? For target_is_clipped true case, does
it
> > always ends being empty clip?
> > 
> 
> When layer is not clipped, clip rect is never used, so it doesn't matter what
its set to

Ah I see because !non_root_surface_enabled's root layer is not clipped, and
non_root_surface_enabled we use layers_are_clipped as signal to whether layer is
clipped.

Thanks for patient replies. :)

[jaydasika, 2016-11-03 19:28:08]

The CQ bit was checked by jaydasika@chromium.org

[commit-bot: I haz the power, 2016-11-03 19:28:49]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-03 23:22:59]

Description was changed from

==========
cc: Set clip rect to empty rect when layer's is_clipped is false

And also verify is_clipped calculated dynamically matches the current
is_clipped value, also verify clip rects even when is_clipped is false

BUG=594675
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
cc: Set clip rect to empty rect when layer's is_clipped is false

And also verify is_clipped calculated dynamically matches the current
is_clipped value, also verify clip rects even when is_clipped is false

BUG=594675
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[commit-bot: I haz the power, 2016-11-03 23:22:59]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-11-03 23:27:43]

Description was changed from

==========
cc: Set clip rect to empty rect when layer's is_clipped is false

And also verify is_clipped calculated dynamically matches the current
is_clipped value, also verify clip rects even when is_clipped is false

BUG=594675
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
cc: Set clip rect to empty rect when layer's is_clipped is false

And also verify is_clipped calculated dynamically matches the current
is_clipped value, also verify clip rects even when is_clipped is false

BUG=594675
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

Committed: https://crrev.com/1fc2fd1adf7c5af04f7d8e2fe752b0a31e0dde7c
Cr-Commit-Position: refs/heads/master@{#429723}
==========

[commit-bot: I haz the power, 2016-11-03 23:27:44]

Patchset 2 (id:??) landed as
https://crrev.com/1fc2fd1adf7c5af04f7d8e2fe752b0a31e0dde7c
Cr-Commit-Position: refs/heads/master@{#429723}