Basic support for backface visibility in spv2 with --layer-lists

Basic support for backface visibility in spv2 with --layer-lists

This patch adds support for backface visibility in spv2 with layer
lists. With this patch, all svp2 backface visibility tests pass (and
two new ones too) when tests are run with the following flags:
--additional-driver-flag=--enable-slimming-paint-v2
--additional-driver-flag=--enable-layer-lists

Test expectations will be updated in a followup when layer list mode
is switched to be the default.

BUG=587236, 630691
Committed: https://crrev.com/a8c172dd5a67c388bb3cd693a8d60e7d3565f2aa
Cr-Commit-Position: refs/heads/master@{#410746}

[pdr., 2016-08-02 05:09:15]

pdr@chromium.org changed reviewers:
+ chrishtr@chromium.org, trchen@chromium.org

[chrishtr, 2016-08-02 20:26:16]

chrishtr@chromium.org changed reviewers:
+ jaydasika@chromium.org

[chrishtr, 2016-08-02 20:26:17]

+jaydasika for more feedback.

Jayadev, two questions for you.

1. Does backface visibility belong on cc::Layers at all, or should it go on
property trees?
2. The code here:

https://cs.chromium.org/chromium/src/cc/trees/property_tree_builder.cc?q=Doub...

appears to depend on the layer hierarchy. Is this code dead, or is there a bug
in cc, or cleanup
still to be done?

[trchen, 2016-08-02 21:19:54]

On 2016/08/02 20:26:17, chrishtr wrote:
> +jaydasika for more feedback.
> 
> Jayadev, two questions for you.
> 
> 1. Does backface visibility belong on cc::Layers at all, or should it go on
> property trees?

Sigh, it really depends on how we redefine the spec.

The current spec says backface visibility applies on individual element, as if
the used value of visibility being adjusted. In this case backface visibility
really should be a Layer/PaintChunk property. However no vendor ever implemented
the spec.

Most vendor implemented to apply backface visibility on stacking context. In
that case, effect nodes become a better place for it.

On Blink side I chose to make it a paint chunk property because it is more
expressive to allow us to implement either spec.

> 2. The code here:
>
https://cs.chromium.org/chromium/src/cc/trees/property_tree_builder.cc?q=Doub...
> 
> appears to depend on the layer hierarchy. Is this code dead, or is there a bug
> in cc, or cleanup
> still to be done?

SPv2/LayerListMode bypass cc prop tree builder. The code you referred to is
tightly coupled with some CSS quirks that are used to be handled in CC in SPv1
but now handled in paint in SPv2. I think we should add some comments in layer.h
to mark which layer properties are considered a part of input interface in
SPv1/SPv2 respectively.

[chrishtr, 2016-08-02 21:27:43]

On 2016/08/02 at 21:19:54, trchen wrote:
> On 2016/08/02 20:26:17, chrishtr wrote:
> > +jaydasika for more feedback.
> > 
> > Jayadev, two questions for you.
> > 
> > 1. Does backface visibility belong on cc::Layers at all, or should it go on
> > property trees?
> 
> Sigh, it really depends on how we redefine the spec.
> 
> The current spec says backface visibility applies on individual element, as if
the used value of visibility being adjusted. In this case backface visibility
really should be a Layer/PaintChunk property. However no vendor ever implemented
the spec.
> 
> Most vendor implemented to apply backface visibility on stacking context. In
that case, effect nodes become a better place for it.
> 
> On Blink side I chose to make it a paint chunk property because it is more
expressive to allow us to implement either spec.
> 
> > 2. The code here:
> >
https://cs.chromium.org/chromium/src/cc/trees/property_tree_builder.cc?q=Doub...
> > 
> > appears to depend on the layer hierarchy. Is this code dead, or is there a
bug
> > in cc, or cleanup
> > still to be done?
> 
> SPv2/LayerListMode bypass cc prop tree builder. The code you referred to is
tightly coupled with some CSS quirks that are used to be handled in CC in SPv1
but now handled in paint in SPv2. I think we should add some comments in layer.h
to mark which layer properties are considered a part of input interface in
SPv1/SPv2 respectively.

Ah right, it's SPv1. Ok nevermind then.

[pdr., 2016-08-09 00:19:31]

How do folks feel about this patch?

[trchen, 2016-08-09 01:01:32]

On 2016/08/09 00:19:31, pdr. wrote:
> How do folks feel about this patch?

lgtm! It is definitely a step forward.

[pdr., 2016-08-09 16:53:22]

On 2016/08/09 at 01:01:32, trchen wrote:
> On 2016/08/09 00:19:31, pdr. wrote:
> > How do folks feel about this patch?
> 
> lgtm! It is definitely a step forward.

Thanks!

Chrishtr, are you okay with this patch too? This fixes a couple tests blocking
https://crbug.com/630691.

[chrishtr, 2016-08-09 16:54:26]

lgtm

[pdr., 2016-08-09 16:54:38]

The CQ bit was checked by pdr@chromium.org

[commit-bot: I haz the power, 2016-08-09 16:55:04]

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-08-09 18:18:01]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-08-09 18:25:45]

Description was changed from

==========
Basic support for backface visibility in spv2 with --layer-lists

This patch adds support for backface visibility in spv2 with layer
lists. With this patch, all svp2 backface visibility tests pass (and
two new ones too) when tests are run with the following flags:
--additional-driver-flag=--enable-slimming-paint-v2
--additional-driver-flag=--enable-layer-lists

Test expectations will be updated in a followup when layer list mode
is switched to be the default.

BUG=587236,630691
==========

to

==========
Basic support for backface visibility in spv2 with --layer-lists

This patch adds support for backface visibility in spv2 with layer
lists. With this patch, all svp2 backface visibility tests pass (and
two new ones too) when tests are run with the following flags:
--additional-driver-flag=--enable-slimming-paint-v2
--additional-driver-flag=--enable-layer-lists

Test expectations will be updated in a followup when layer list mode
is switched to be the default.

BUG=587236,630691

Committed: https://crrev.com/a8c172dd5a67c388bb3cd693a8d60e7d3565f2aa
Cr-Commit-Position: refs/heads/master@{#410746}
==========

[commit-bot: I haz the power, 2016-08-09 18:25:46]

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