Simplify application of perspective origin.

Simplify application of perspective origin.

Previously, we tried to translate the origin to be relative to the layer
centre, which is what Core Animation wanted, and then translate it back.

Not only is this unnecessarily complicated, but it caused an error in
the positioning of the perspective origin when scrollbars exist.
There is still a bug in this case (cf the second example in the layout
test), but this brings us closer to spec compliance and alignment with
Gecko.

This CL also makes perspective always create a child transform layer
in the CompositedDeprecatedPaintLayerMapping, since using the other
graphics layers is not always correct (in particular, they may have
different bounds and cause an incorrect perspective origin).

BUG=471275
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=193291

[jbroman, 2015-03-31 20:50:40]

Patchset #14 (id:260001) has been deleted

[jbroman, 2015-04-01 17:51:03]

jbroman@chromium.org changed reviewers:
+ vollick@chromium.org

[jbroman, 2015-04-01 17:51:04]

Aside from added tests and test expectations, this CL mostly deletes code.

Layout test changes (see trybot runs on second-to-last patchset) consist mostly
of:
- adding this additional layer in cases where perspective is present,
- not forcing scrolling layers to not flatten (previously they did, in cases I
don't believe it's necessary -- but please confirm)

This makes our implementation of perspective origin match more closely with
Gecko and the spec. It also simplifies
DeprecatedPaintLayer::perspectiveTransform, which should make this code more
rational to deal with (in particular, for adding perspective for slimming
paint).

Please review closely; I am not very familiar with this class.

[Ian Vollick, 2015-04-01 18:00:09]

The new perspective origin handling is MUCH easier to follow. Just a few minor
comments.

https://codereview.chromium.org/1030323005/diff/320001/LayoutTests/TestExpect...
File LayoutTests/TestExpectations (right):

https://codereview.chromium.org/1030323005/diff/320001/LayoutTests/TestExpect...
LayoutTests/TestExpectations:1763: 
I realize that this is a pain, but adding the new expectations for one platform
explicitly really helps check whether the new, rebaselined values will be
reasonable.

https://codereview.chromium.org/1030323005/diff/320001/Source/core/layout/com...
File Source/core/layout/compositing/CompositedDeprecatedPaintLayerMapping.cpp
(right):

https://codereview.chromium.org/1030323005/diff/320001/Source/core/layout/com...
Source/core/layout/compositing/CompositedDeprecatedPaintLayerMapping.cpp:1382:
ApplyToChildContainingLayers = (1 << 6), // layers between m_graphicsLayer and
children
bikeshed: ApplyToGraphicsLayerDescendants

https://codereview.chromium.org/1030323005/diff/320001/Source/core/layout/com...
File Source/core/layout/compositing/CompositedDeprecatedPaintLayerMapping.h
(right):

https://codereview.chromium.org/1030323005/diff/320001/Source/core/layout/com...
Source/core/layout/compositing/CompositedDeprecatedPaintLayerMapping.h:311: //  
       + m_layerForScrollCorner [OPTIONAL]
Thank you for updating this comment.

[Ian Vollick, 2015-04-01 18:18:18]

https://codereview.chromium.org/1030323005/diff/320001/Source/core/layout/com...
File Source/core/layout/compositing/CompositedDeprecatedPaintLayerMapping.cpp
(right):

https://codereview.chromium.org/1030323005/diff/320001/Source/core/layout/com...
Source/core/layout/compositing/CompositedDeprecatedPaintLayerMapping.cpp:1382:
ApplyToChildContainingLayers = (1 << 6), // layers between m_graphicsLayer and
children
On 2015/04/01 18:00:09, vollick wrote:
> bikeshed: ApplyToGraphicsLayerDescendants

Chatted with Jeremy about this offline. My bikeshed color isn't accurate
(scrollbars), and I can't come up with a better one. Suggestion withdrawn :)

[chrishtr, 2015-04-01 18:52:20]

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

[chrishtr, 2015-04-01 18:52:21]

I have the same comment as last time a CL went by that messed with the
GraphicsLayer structure: if this doesn't block Slimming Paint, why should we do
it in the deprecated code? Or does it help Slimming Paint?

OTOH, I'm sure we have learned good things about achieving correctness in the
new hotness.

[jbroman, 2015-04-01 19:08:35]

On 2015/04/01 18:52:21, chrishtr wrote:
> I have the same comment as last time a CL went by that messed with the
> GraphicsLayer structure: if this doesn't block Slimming Paint, why should we
do
> it in the deprecated code? Or does it help Slimming Paint?
> 
> OTOH, I'm sure we have learned good things about achieving correctness in the
> new hotness.

My original goal was to make the DeprecatedPaintLayer change so that we don't
propagate Core Animation's notion of transform origin ((0,0) being the centre)
to new code.

Just making that change causes a change in how we incorrectly behave, though. So
the options are then:
- have two methods: one that preserves the current behaviour, and one which does
the sane thing, or
- make the change, and move toward correct behaviour

The latter didn't seem too risky to me, and as a bonus, gives us layout tests
that can help ensure that we do the right thing in a SP-phase-2 world.

If it's really better, I can instead rename
DeprecatedPaintLayer::perspectiveTransform to ::deprecatedPerspectiveTransform
or something (keeping the cruft that this CL deletes there), and create another
method to use going forward.

[chrishtr, 2015-04-01 19:13:42]

This comes up in the implementation of property trees in Blink? If you go with
the change to make two perspective transform methods (or add a
shouldSetAutomaticOrigin option), is there anything else that blocks slimming
paint?

My concern is just creating extra work for you and bugs that have to be triaged
and fixed in the old system.

[jbroman, 2015-04-01 19:16:26]

Patchset #18 (id:350001) has been deleted

[jbroman, 2015-04-01 19:16:40]

Patchset #18 (id:350001) has been deleted

[Ian Vollick, 2015-04-01 19:47:26]

On 2015/04/01 18:52:21, chrishtr wrote:
> I have the same comment as last time a CL went by that messed with the
> GraphicsLayer structure: if this doesn't block Slimming Paint, why should we
do
> it in the deprecated code? Or does it help Slimming Paint?

It's a fair concern, but I'll repeat my reasoning from last time.

I think this is worth doing because
1. it helps us get the new code correct, so we're not replicating bugs in SP,
2. it fixes real bugs and improves compatibility and code health,
3. it's done, and
4. it low risk since the patch can be reverted if it causes issues (since this
is sunset code, there's not much chance of the new change being built upon and
becoming hard to revert).

In the case of the other patch, the code wasn't correct and it wouldn't make
sense to prioritize it over other SP work, so it was put on hold. But that
doesn't appear to be the case here.

[chrishtr, 2015-04-01 21:10:18]

On 2015/04/01 at 19:47:26, vollick wrote:
> On 2015/04/01 18:52:21, chrishtr wrote:
> > I have the same comment as last time a CL went by that messed with the
> > GraphicsLayer structure: if this doesn't block Slimming Paint, why should we
do
> > it in the deprecated code? Or does it help Slimming Paint?
> 
> It's a fair concern, but I'll repeat my reasoning from last time.
> 
> I think this is worth doing because
> 1. it helps us get the new code correct, so we're not replicating bugs in SP,
> 2. it fixes real bugs and improves compatibility and code health,
> 3. it's done, and
> 4. it low risk since the patch can be reverted if it causes issues (since this
is sunset code, there's not much chance of the new change being built upon and
becoming hard to revert).
> 
> In the case of the other patch, the code wasn't correct and it wouldn't make
sense to prioritize it over other SP work, so it was put on hold. But that
doesn't appear to be the case here.

Ok. This is where we ended up with the other patch as well, before we ended up
finding more reasons not to do it (which in fairness were unrelated to the
flavor of my arguments here IIRC). :)

If you and Jeremy feel strongly that finishing this off has the right risk
tradeoffs given what I said, then go ahead. However, note that e.g. trchen wants
to implement more changes on top of this for his bugfixes, which are related,
which makes me worried about slippery slopes of short-term work that gets
deleted a few months from now..

[chrishtr, 2015-04-01 21:11:59]

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

[chrishtr, 2015-04-01 21:12:01]

Tien-Ren, please chime in on this thread with what you would want to build on
top of this to solve the problems you are investigating. I believe you also have
suggestions about this CL related to that?

[trchen, 2015-04-01 22:37:46]

What I'm trying to achieve is orthogonal to this CL, but I think the CL may be
simpler if we do both goals in one.
Anyway I would vote yes for this CL because it improves correctness. Improving
correctness makes it easier to verify future works.

My goal is as following:
IMO we should eliminate child transform layer at all.

At other places in Blink (e.g. mapLocalToContainer) we simply use
getTransformFromContainer, which bakes the perspective transform from the
container to the transform of current element. This conforms to the spec. i.e.

<div style="perspective:100px">
  <div style="transform:blah"></div>
</div>

Is fully equivalent to

<div>
  <div style="transform:perspective(100px)blah"></div>
</div>

Eliminating child transform layer will allow us to have this invariant in cc:
IsRootLayerOfNewRenderingContext(layer) == !layer->parent() ||
layer->parent()->should_flatten_transform()

It will also fix some simple cases of backface visibility bugs I'm working on,
without introducing code complexity (actually it reduces it).

[Ian Vollick, 2015-04-01 22:45:19]

On 2015/04/01 22:37:46, trchen wrote:
> What I'm trying to achieve is orthogonal to this CL, but I think the CL may be
> simpler if we do both goals in one.
> Anyway I would vote yes for this CL because it improves correctness. Improving
> correctness makes it easier to verify future works.
> 
> My goal is as following:
> IMO we should eliminate child transform layer at all.

It would be nice to get rid of the child transform layer, but to be clear, are
you suggesting that be attempted in this CL? That feels like quite a bit for one
change.

[trchen, 2015-04-01 22:52:17]

On 2015/04/01 22:45:19, vollick wrote:
> On 2015/04/01 22:37:46, trchen wrote:
> > What I'm trying to achieve is orthogonal to this CL, but I think the CL may
be
> > simpler if we do both goals in one.
> > Anyway I would vote yes for this CL because it improves correctness.
Improving
> > correctness makes it easier to verify future works.
> > 
> > My goal is as following:
> > IMO we should eliminate child transform layer at all.
> 
> It would be nice to get rid of the child transform layer, but to be clear, are
> you suggesting that be attempted in this CL? That feels like quite a bit for
one
> change.

I just realized that doing so would require baking transform origin into
transform too. Doing it in one CL will be too risky. Anyway I think we should
land this CL.

[jbroman, 2015-04-02 15:34:16]

https://codereview.chromium.org/1030323005/diff/320001/LayoutTests/TestExpect...
File LayoutTests/TestExpectations (right):

https://codereview.chromium.org/1030323005/diff/320001/LayoutTests/TestExpect...
LayoutTests/TestExpectations:1763: 
On 2015/04/01 18:00:09, vollick wrote:
> I realize that this is a pain, but adding the new expectations for one
platform
> explicitly really helps check whether the new, rebaselined values will be
> reasonable.

Done.

https://codereview.chromium.org/1030323005/diff/320001/Source/core/layout/com...
File Source/core/layout/compositing/CompositedDeprecatedPaintLayerMapping.cpp
(right):

https://codereview.chromium.org/1030323005/diff/320001/Source/core/layout/com...
Source/core/layout/compositing/CompositedDeprecatedPaintLayerMapping.cpp:1382:
ApplyToChildContainingLayers = (1 << 6), // layers between m_graphicsLayer and
children
On 2015/04/01 18:18:18, vollick wrote:
> On 2015/04/01 18:00:09, vollick wrote:
> > bikeshed: ApplyToGraphicsLayerDescendants
> 
> Chatted with Jeremy about this offline. My bikeshed color isn't accurate
> (scrollbars), and I can't come up with a better one. Suggestion withdrawn :)

Acknowledged.

https://codereview.chromium.org/1030323005/diff/320001/Source/core/layout/com...
File Source/core/layout/compositing/CompositedDeprecatedPaintLayerMapping.h
(right):

https://codereview.chromium.org/1030323005/diff/320001/Source/core/layout/com...
Source/core/layout/compositing/CompositedDeprecatedPaintLayerMapping.h:311: //  
       + m_layerForScrollCorner [OPTIONAL]
On 2015/04/01 18:00:09, vollick wrote:
> Thank you for updating this comment.

No problem. :)

[Ian Vollick, 2015-04-07 03:53:12]

On 2015/04/01 21:10:18, chrishtr wrote:
> On 2015/04/01 at 19:47:26, vollick wrote:
> > On 2015/04/01 18:52:21, chrishtr wrote:
> > > I have the same comment as last time a CL went by that messed with the
> > > GraphicsLayer structure: if this doesn't block Slimming Paint, why should
we
> do
> > > it in the deprecated code? Or does it help Slimming Paint?
> > 
> > It's a fair concern, but I'll repeat my reasoning from last time.
> > 
> > I think this is worth doing because
> > 1. it helps us get the new code correct, so we're not replicating bugs in
SP,
> > 2. it fixes real bugs and improves compatibility and code health,
> > 3. it's done, and
> > 4. it low risk since the patch can be reverted if it causes issues (since
this
> is sunset code, there's not much chance of the new change being built upon and
> becoming hard to revert).
> > 
> > In the case of the other patch, the code wasn't correct and it wouldn't make
> sense to prioritize it over other SP work, so it was put on hold. But that
> doesn't appear to be the case here.
> 
> Ok. This is where we ended up with the other patch as well, before we ended up
> finding more reasons not to do it (which in fairness were unrelated to the
> flavor of my arguments here IIRC). :)
>
> If you and Jeremy feel strongly that finishing this off has the right risk
> tradeoffs given what I said, then go ahead. However, note that e.g. trchen
wants
> to implement more changes on top of this for his bugfixes, which are related,
> which makes me worried about slippery slopes of short-term work that gets
> deleted a few months from now..

Thanks -- I would really like to land this (seems low risk and a big
quality/correctness win), but I agree that we need to be careful about our
investment here.

[Ian Vollick, 2015-04-07 03:55:15]

On 2015/04/02 15:34:16, jbroman wrote:
>
https://codereview.chromium.org/1030323005/diff/320001/LayoutTests/TestExpect...
> File LayoutTests/TestExpectations (right):
> 
>
https://codereview.chromium.org/1030323005/diff/320001/LayoutTests/TestExpect...
> LayoutTests/TestExpectations:1763: 
> On 2015/04/01 18:00:09, vollick wrote:
> > I realize that this is a pain, but adding the new expectations for one
> platform
> > explicitly really helps check whether the new, rebaselined values will be
> > reasonable.
> 
> Done.

Thank you for doing all that legwork. This lgtm.
> 
>
https://codereview.chromium.org/1030323005/diff/320001/Source/core/layout/com...
> File Source/core/layout/compositing/CompositedDeprecatedPaintLayerMapping.cpp
> (right):
> 
>
https://codereview.chromium.org/1030323005/diff/320001/Source/core/layout/com...
> Source/core/layout/compositing/CompositedDeprecatedPaintLayerMapping.cpp:1382:
> ApplyToChildContainingLayers = (1 << 6), // layers between m_graphicsLayer and
> children
> On 2015/04/01 18:18:18, vollick wrote:
> > On 2015/04/01 18:00:09, vollick wrote:
> > > bikeshed: ApplyToGraphicsLayerDescendants
> > 
> > Chatted with Jeremy about this offline. My bikeshed color isn't accurate
> > (scrollbars), and I can't come up with a better one. Suggestion withdrawn :)
> 
> Acknowledged.
> 
>
https://codereview.chromium.org/1030323005/diff/320001/Source/core/layout/com...
> File Source/core/layout/compositing/CompositedDeprecatedPaintLayerMapping.h
> (right):
> 
>
https://codereview.chromium.org/1030323005/diff/320001/Source/core/layout/com...
> Source/core/layout/compositing/CompositedDeprecatedPaintLayerMapping.h:311: //
 
>        + m_layerForScrollCorner [OPTIONAL]
> On 2015/04/01 18:00:09, vollick wrote:
> > Thank you for updating this comment.
> 
> No problem. :)

[jbroman, 2015-04-07 14:05:03]

The CQ bit was checked by jbroman@chromium.org

[commit-bot: I haz the power, 2015-04-07 14:05:15]

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

[jbroman, 2015-04-07 17:14:55]

The CQ bit was unchecked by jbroman@chromium.org

[jbroman, 2015-04-07 17:27:20]

The CQ bit was checked by jbroman@chromium.org

[jbroman, 2015-04-07 17:27:21]

The patchset sent to the CQ was uploaded after l-g-t-m from vollick@chromium.org
Link to the patchset: https://codereview.chromium.org/1030323005/#ps450001
(title: "add three more test expectation changes (added since earlier)")

[commit-bot: I haz the power, 2015-04-07 17:27:58]

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

[commit-bot: I haz the power, 2015-04-07 18:52:59]

Committed patchset #22 (id:450001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=193291