Use same layout logic in animations for deciding whether an element can transform

Update logic in animations for deciding whether an element can transform

This change makes CompositorAnimations.cpp more closely match the logic
used in paint code for deciding whether an element's used transform
is not none.

BUG=633021
Committed: https://crrev.com/dc4a00b29ecc52a9eb29ceb433101c4075a5d84f
Cr-Commit-Position: refs/heads/master@{#410987}

[alancutter (OOO until 2018), 2016-08-08 07:14:04]

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

[jbroman, 2016-08-08 22:50:58]

Seems reasonable to me, but you should probably get a core/layout/ owner's
opinion as well.

(Also cc-ing trchen, who has thought about what affects whether an object can or
cannot be transformed.)

[loyso (OOO), 2016-08-09 00:16:03]

JFYI, it shouldn't regress the bug 592803 (see the fix from dstockwell@ here:
https://codereview.chromium.org/1775743002 )

[alancutter (OOO until 2018), 2016-08-09 01:00:02]

alancutter@chromium.org changed reviewers:
+ eae@chromium.org

[alancutter (OOO until 2018), 2016-08-09 01:00:02]

+eae for core/layout review.

[alancutter (OOO until 2018), 2016-08-09 01:01:22]

Note that this patch doesn't affect whether an element can be transformed, only
whether we start compositor driven transform animations for them.

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

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

[trchen, 2016-08-09 01:22:02]

https://codereview.chromium.org/2225743002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutObject.h (right):

https://codereview.chromium.org/2225743002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutObject.h:715: bool canTransform()
const { return isBox(); }
SVG? For example: http://codepen.io/chriscoyier/pen/dvjhn/
I think the condition in CompositorAnimations.cpp was written in the way to not
to exclude SVG.

According to the spec:
https://drafts.csswg.org/css-transforms/#transformable-element
For CSS box model elements it is essentially isBox(). I think the standard just
spec'd after WebKit.
I'm not familiar with SVGs though. I guess all LayoutSVGModelObject supports
transform?

https://codereview.chromium.org/2225743002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp (right):

https://codereview.chromium.org/2225743002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:278: if
(!object.isBox() || !style.hasPerspective()) {
object.canTransform() here too.

[alancutter (OOO until 2018), 2016-08-09 02:07:21]

On 2016/08/09 at 01:22:02, trchen wrote:
>
https://codereview.chromium.org/2225743002/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/layout/LayoutObject.h (right):
> 
>
https://codereview.chromium.org/2225743002/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/layout/LayoutObject.h:715: bool canTransform()
const { return isBox(); }
> SVG? For example: http://codepen.io/chriscoyier/pen/dvjhn/
> I think the condition in CompositorAnimations.cpp was written in the way to
not to exclude SVG.
> 
> According to the spec:
https://drafts.csswg.org/css-transforms/#transformable-element
> For CSS box model elements it is essentially isBox(). I think the standard
just spec'd after WebKit.
> I'm not familiar with SVGs though. I guess all LayoutSVGModelObject supports
transform?

We don't run SVG element transform animations on the compositor since SVGs are
composited on the main thread. CompositorAnimations::attachCompositedLayers()
checks !isBoxModelObject().

Renamed canTransform() to canTransformOnCompositor() for clarity, thanks for
bringing up the SVG angle.

https://codereview.chromium.org/2225743002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp (right):

https://codereview.chromium.org/2225743002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:278: if
(!object.isBox() || !style.hasPerspective()) {
On 2016/08/09 at 01:22:02, trchen wrote:
> object.canTransform() here too.

Done.

[eae, 2016-08-09 13:50:13]

LGTM for core/layout

[trchen, 2016-08-09 21:57:13]

Other than that, lgtm.

https://codereview.chromium.org/2225743002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp (right):

https://codereview.chromium.org/2225743002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:136: if
(object.canTransformOnCompositor() && (style.hasTransform() ||
style.preserves3D())) {
Hmm... Since you changed the semantics of the function to
canTransformOnCompositor(). I think we should leave it as-is (object.isBox())
for here and below.

FYI this class PaintPropertyTreeBuilder is for generating property trees for
SPv2, and the compositing architecture changed a lot in SPv2. Basically we
delayed compositing decision to a very late stage in the pipeline. The transform
nodes will be always generated, but whether layers need to be created are solely
on cc's discretion.

[alancutter (OOO until 2018), 2016-08-10 00:12:11]

On 2016/08/09 at 21:57:13, trchen wrote:
> Other than that, lgtm.
> 
>
https://codereview.chromium.org/2225743002/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp
(right):
> 
>
https://codereview.chromium.org/2225743002/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:136: if
(object.canTransformOnCompositor() && (style.hasTransform() ||
style.preserves3D())) {
> Hmm... Since you changed the semantics of the function to
canTransformOnCompositor(). I think we should leave it as-is (object.isBox())
for here and below.
> 
> FYI this class PaintPropertyTreeBuilder is for generating property trees for
SPv2, and the compositing architecture changed a lot in SPv2. Basically we
delayed compositing decision to a very late stage in the pipeline. The transform
nodes will be always generated, but whether layers need to be created are solely
on cc's discretion.

I thought the isBox() check determined whether specified transforms applied to
an element since it guards the call to style.applyTransform(). I'm not sure I
understand what "transform nodes will be always generated" means.

My intention is for animations to mirror that decision making so it'd be ideal
if the function name accurately reflected the decision and could be used in
place of isBox().

[trchen, 2016-08-10 00:56:59]

On 2016/08/10 00:12:11, alancutter wrote:
> On 2016/08/09 at 21:57:13, trchen wrote:
> > Other than that, lgtm.
> > 
> >
>
https://codereview.chromium.org/2225743002/diff/20001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp
> (right):
> > 
> >
>
https://codereview.chromium.org/2225743002/diff/20001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:136: if
> (object.canTransformOnCompositor() && (style.hasTransform() ||
> style.preserves3D())) {
> > Hmm... Since you changed the semantics of the function to
> canTransformOnCompositor(). I think we should leave it as-is (object.isBox())
> for here and below.
> > 
> > FYI this class PaintPropertyTreeBuilder is for generating property trees for
> SPv2, and the compositing architecture changed a lot in SPv2. Basically we
> delayed compositing decision to a very late stage in the pipeline. The
transform
> nodes will be always generated, but whether layers need to be created are
solely
> on cc's discretion.
> 
> I thought the isBox() check determined whether specified transforms applied to
> an element since it guards the call to style.applyTransform(). I'm not sure I
> understand what "transform nodes will be always generated" means.

That's correct, but that is not equivalent to "canTransformOnCompositor". Let's
say we added a hypothetical CSS value that affects the used value of transform
and it needs to be calculated in the main thread, so canTransformOnCompositor
will be false. However we still need to create transform node for it in SPv2.

In other words, (isBox() || isSVGModelObject()) determines whether transform
property is applicable to a certain element. If no, the used value of transform
will be forced to 'none'. While "canTransformOnCompositor" doesn't sound like it
affects the transform value but just some internal compositing decision.

> My intention is for animations to mirror that decision making so it'd be ideal
> if the function name accurately reflected the decision and could be used in
> place of isBox().

I think "canTransformOnCompositor" is not really a layout property but internals
of our compositing system. I suggest to have a function like
"transformApplicable" that returns (isBox() || isSVGModelObject()). Your change
in CompositorAnimations shouldn't be affected because it didn't rule out SVG
before your CL either.

[alancutter (OOO until 2018), 2016-08-10 01:42:18]

On 2016/08/10 at 00:56:59, trchen wrote:
> > My intention is for animations to mirror that decision making so it'd be
ideal
> > if the function name accurately reflected the decision and could be used in
> > place of isBox().
> 
> I think "canTransformOnCompositor" is not really a layout property but
internals of our compositing system. I suggest to have a function like
"transformApplicable" that returns (isBox() || isSVGModelObject()). Your change
in CompositorAnimations shouldn't be affected because it didn't rule out SVG
before your CL either.

Thanks for clarifying, your suggestion sgtm.
Removed changes to PaintPropertyTreeBuilder.cpp and renamed to
isTransformApplicable().

[alancutter (OOO until 2018), 2016-08-10 01:45:09]

Description was changed from

==========
Use same layout logic in animations for deciding whether an element can
transform

Previously animations had its own hand rolled rules for deciding whether
an element could animate which didn't line up with layout code.
This patch uses layout logic over (incorrect) animation logic for
consistency and correctness.

BUG=633021
==========

to

==========
Update logic in animations for deciding whether an element can transform

This change makes CompositorAnimations.cpp more closely match the logic
used in paint code for deciding whether an element's used transform
is not none.

BUG=633021
==========

[alancutter (OOO until 2018), 2016-08-10 01:45:50]

The CQ bit was checked by alancutter@chromium.org

[alancutter (OOO until 2018), 2016-08-10 01:45:51]

The patchset sent to the CQ was uploaded after l-g-t-m from trchen@chromium.org,
eae@chromium.org
Link to the patchset: https://codereview.chromium.org/2225743002/#ps40001
(title: "isTransformApplicable")

[commit-bot: I haz the power, 2016-08-10 01:46:47]

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-10 03:17:33]

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

[commit-bot: I haz the power, 2016-08-10 03:17:34]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[alancutter (OOO until 2018), 2016-08-10 04:43:41]

The CQ bit was checked by alancutter@chromium.org

[commit-bot: I haz the power, 2016-08-10 04:44:06]

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-10 06:55:10]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-08-10 06:57:06]

Description was changed from

==========
Update logic in animations for deciding whether an element can transform

This change makes CompositorAnimations.cpp more closely match the logic
used in paint code for deciding whether an element's used transform
is not none.

BUG=633021
==========

to

==========
Update logic in animations for deciding whether an element can transform

This change makes CompositorAnimations.cpp more closely match the logic
used in paint code for deciding whether an element's used transform
is not none.

BUG=633021

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

[commit-bot: I haz the power, 2016-08-10 06:57:07]

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