Implement SVG's transform and effect paint property nodes

Implement SVG's transform and effect paint property nodes

This patch starts generating transform and effect paint property nodes
for SVG. PaintPropertyTreeBuilder has been changed to walk both
LayoutBoxModelObjects and SVG nodes. The effect node generation is
straightforward, but SVG transform nodes need some svg-specific logic:
  1) SVG does not use paint offset because everything is effectively
     absolutely positioned. The SVG root element now always creates a
     paint offset transform so all children do not need to.
  2) SVG bakes the transform origin into all matrices. We may want to
     refactor this later to support compositor animations.
  3) At the SVG->HTML boundary, out-of-flow and fixed positioning
     data is kept up to date by checking for both xforms and isSVGRoot.

BUG=537409
Committed: https://crrev.com/1de8a82b564ec43cdbe64817b95732eb449e8f11
Cr-Commit-Position: refs/heads/master@{#361183}

[pdr., 2015-11-20 04:31:02]

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

[jbroman, 2015-11-20 19:09:21]

lgtm, but please wait for trchen to have a look, too

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

https://codereview.chromium.org/1461223002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:122:
shouldCreatePaintOffsetTranslationNode = toLayoutBoxModelObject(object).layer()
&&
toLayoutBoxModelObject(object).layer()->paintsWithTransform(GlobalPaintNormalPhase);
nit: extract toLayoutBoxModelObject(object).layer() to a local variable

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

https://codereview.chromium.org/1461223002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp:370: "   
transform: translate3D(1px, 2px, 3px);"
super-nit: technically, "translate3d" (with a lowercase d)

[pdr., 2015-11-20 19:33:41]

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

https://codereview.chromium.org/1461223002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:122:
shouldCreatePaintOffsetTranslationNode = toLayoutBoxModelObject(object).layer()
&&
toLayoutBoxModelObject(object).layer()->paintsWithTransform(GlobalPaintNormalPhase);
On 2015/11/20 at 19:09:20, jbroman wrote:
> nit: extract toLayoutBoxModelObject(object).layer() to a local variable

I actually refactored this slightly differently so a local isn't needed:
PaintLayer* layer = toLayoutBoxModelObject(object).layer()
shouldCreatePaintOffsetTranslationNode = layer &&
layer->paintsWithTransform(GlobalPaintNormalPhase);

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

https://codereview.chromium.org/1461223002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp:370: "   
transform: translate3D(1px, 2px, 3px);"
On 2015/11/20 at 19:09:20, jbroman wrote:
> super-nit: technically, "translate3d" (with a lowercase d)

3done.

https://codereview.chromium.org/1461223002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp:419: //
TODO(pdr): Create a test for transform nodes crossing the svg->foreignObject
boundary.
I'd like to do this (crbug.com/87072 will finally be fixed!) as a followup, but
there is a real bug remaining in getting this working.

[jbroman, 2015-11-20 19:53:40]

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

https://codereview.chromium.org/1461223002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:122:
shouldCreatePaintOffsetTranslationNode = toLayoutBoxModelObject(object).layer()
&&
toLayoutBoxModelObject(object).layer()->paintsWithTransform(GlobalPaintNormalPhase);
On 2015/11/20 at 19:33:41, pdr wrote:
> On 2015/11/20 at 19:09:20, jbroman wrote:
> > nit: extract toLayoutBoxModelObject(object).layer() to a local variable
> 
> I actually refactored this slightly differently so a local isn't needed:
> PaintLayer* layer = toLayoutBoxModelObject(object).layer()
> shouldCreatePaintOffsetTranslationNode = layer &&
layer->paintsWithTransform(GlobalPaintNormalPhase);

...which is exactly what I asked you to do?

[pdr., 2015-11-20 21:41:56]

On 2015/11/20 at 19:53:40, jbroman wrote:
>
https://codereview.chromium.org/1461223002/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp
(right):
> 
>
https://codereview.chromium.org/1461223002/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:122:
shouldCreatePaintOffsetTranslationNode = toLayoutBoxModelObject(object).layer()
&&
toLayoutBoxModelObject(object).layer()->paintsWithTransform(GlobalPaintNormalPhase);
> On 2015/11/20 at 19:33:41, pdr wrote:
> > On 2015/11/20 at 19:09:20, jbroman wrote:
> > > nit: extract toLayoutBoxModelObject(object).layer() to a local variable
> > 
> > I actually refactored this slightly differently so a local isn't needed:
> > PaintLayer* layer = toLayoutBoxModelObject(object).layer()
> > shouldCreatePaintOffsetTranslationNode = layer &&
layer->paintsWithTransform(GlobalPaintNormalPhase);
> 
> ...which is exactly what I asked you to do?

Just ignore my comment... nothing to see here.

LGTY overall?

[trchen, 2015-11-20 22:37:30]

Wow this is simpler than I expected. I know little about SVG specs so I'm not
the best person to lg tm... Nothing looks obviously wrong to me though. (thumbs
up to pdr!)

[pdr., 2015-11-20 22:39:08]

On 2015/11/20 at 22:37:30, trchen wrote:
> Wow this is simpler than I expected. I know little about SVG specs so I'm not
the best person to lg tm... Nothing looks obviously wrong to me though. (thumbs
up to pdr!)

The secret is that nobody really understands that insane spec :)

Off to the CQ we go

[pdr., 2015-11-20 22:39:10]

The CQ bit was checked by pdr@chromium.org

[pdr., 2015-11-20 22:39:10]

The patchset sent to the CQ was uploaded after l-g-t-m from jbroman@chromium.org
Link to the patchset: https://codereview.chromium.org/1461223002/#ps20001
(title: "Address reviewer comments")

[commit-bot: I haz the power, 2015-11-20 22:40:53]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1461223002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1461223002/20001

[trchen, 2015-11-20 22:52:10]

Oops, are we not too late to add one more thing?

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

https://codereview.chromium.org/1461223002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:232: if
(style.position() != StaticPosition || hasTransform) {
|| object.isSVGForeignObject()

https://codereview.chromium.org/1461223002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:236: if
(hasTransform) {
ditto

[pdr., 2015-11-20 22:55:58]

The CQ bit was unchecked by pdr@chromium.org

[pdr., 2015-11-21 00:09:47]

In the latest patch I went ahead and filled in the todo about adding
foreignObject test.

On 2015/11/20 at 22:52:10, trchen wrote:
> Oops, are we not too late to add one more thing?

Never too late!

> 
>
https://codereview.chromium.org/1461223002/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp
(right):
> 
>
https://codereview.chromium.org/1461223002/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:232: if
(style.position() != StaticPosition || hasTransform) {
> || object.isSVGForeignObject()
> 
>
https://codereview.chromium.org/1461223002/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:236: if
(hasTransform) {
> ditto

I looked into this and wasn't able to construct a testcase that would fail
without this change. Can you think of one? For example, the newly added
FixedTransformAncestorAcrossSVGHTMLBoundary passes without these checks.

[trchen, 2015-11-21 00:31:53]

How about adding <g transform="translate(123)"> in between <svg> and
<foreignObject>?
By the way is SVG transform attribute and CSS transform property treated the
same? What happen if both are specified on the same element?

On 2015/11/21 00:09:47, pdr wrote:
> In the latest patch I went ahead and filled in the todo about adding
> foreignObject test.
> 
> On 2015/11/20 at 22:52:10, trchen wrote:
> > Oops, are we not too late to add one more thing?
> 
> Never too late!
> 
> > 
> >
>
https://codereview.chromium.org/1461223002/diff/20001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp
> (right):
> > 
> >
>
https://codereview.chromium.org/1461223002/diff/20001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:232: if
> (style.position() != StaticPosition || hasTransform) {
> > || object.isSVGForeignObject()
> > 
> >
>
https://codereview.chromium.org/1461223002/diff/20001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:236: if
> (hasTransform) {
> > ditto
> 
> I looked into this and wasn't able to construct a testcase that would fail
> without this change. Can you think of one? For example, the newly added
> FixedTransformAncestorAcrossSVGHTMLBoundary passes without these checks.

[pdr., 2015-11-21 06:15:57]

The latest patch has some non-trivial changes in updateOutOfFlowContext.
@jbroman and @trchen, can you both please take one more look?

On 2015/11/21 at 00:31:53, trchen wrote:
> How about adding <g transform="translate(123)"> in between <svg> and
<foreignObject>?

That did the trick! In fixing this I realized that isSVGForeignObject() isn't
really needed because all SVG content is positioned with transforms. The only
exception is the SVG root element itself, and there we reset the
currentTransform with all accumulated paint offsets. Therefore, I think we just
need to check createdNewTransform and isSVGRoot in updateOutOfFlowContext.

> By the way is SVG transform attribute and CSS transform property treated the
same? What happen if both are specified on the same element?

SVG was born back when XML was hip and dinosaurs roamed the earth, so transforms
were based on attributes (aka <g transform='blah'>). Then CSS came along and
spec'ed transforms based on properties (aka <style>blah { translate:
transform(blah); }</style>). SVG has been gradually moving closer to HTML/CSS
and to solve this discrepancy the SVGWG introduced "presentation attributes"
which just define rules that say, for specific svg attributes, attributes are
properties and properties are attributes.

Presentation attributes are really for bridging old XML SVG content to newer CSS
SVG content, so it's not common to have both. In any case, it looks like CSS
properties win: http://jsbin.com/dojobomogi but I'm not sure where that's
spec'ed.

[pdr., 2015-11-21 06:17:15]

Description was changed from

==========
Implement SVG's transform and effect paint property nodes

This patch starts generating transform and effect paint property nodes
for SVG. PaintPropertyTreeBuilder has been changed to walk both
LayoutBoxModelObjects and SVG nodes. The effect node generation is
straightforward, but SVG transform nodes need some svg-specific logic:
  1) SVG does not use paint offset because everything is effectively
     absolutely positioned. The SVG root element now always creates a
     paint offset transform so all children do not need to.
  2) SVG bakes the transform origin into all matrices. We may want to
     refactor this later to support compositor animations.

BUG=537409
==========

to

==========
Implement SVG's transform and effect paint property nodes

This patch starts generating transform and effect paint property nodes
for SVG. PaintPropertyTreeBuilder has been changed to walk both
LayoutBoxModelObjects and SVG nodes. The effect node generation is
straightforward, but SVG transform nodes need some svg-specific logic:
  1) SVG does not use paint offset because everything is effectively
     absolutely positioned. The SVG root element now always creates a
     paint offset transform so all children do not need to.
  2) SVG bakes the transform origin into all matrices. We may want to
     refactor this later to support compositor animations.
  3) At the SVG->HTML boundary, out-of-flow and fixed positioning
     data is kept up to date by checking for both xforms and isSVGRoot.

BUG=537409
==========

[pdr., 2015-11-23 06:32:52]

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

[commit-bot: I haz the power, 2015-11-23 06:33:15]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1461223002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1461223002/60001

[commit-bot: I haz the power, 2015-11-23 07:45:15]

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

[commit-bot: I haz the power, 2015-11-23 07:45:16]

Dry run: This issue passed the CQ dry run.

[jbroman, 2015-11-23 17:51:01]

still lgtm if trchen is happy

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

https://codereview.chromium.org/1461223002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:233: if
(object.styleRef().position() != StaticPosition || createdNewTransform ||
object.isSVGRoot()) {
Out of curiosity (arguably out of scope for this patch, but I didn't previously
notice it), isn't "object.styleRef().position() != StaticPosition" equivalent to
"object.isPositioned()", which is shorter to write and faster to run IIUC?

[trchen, 2015-11-23 19:28:29]

lgtm too. Thanks!

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

https://codereview.chromium.org/1461223002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:233: if
(object.styleRef().position() != StaticPosition || createdNewTransform ||
object.isSVGRoot()) {
On 2015/11/23 17:51:00, jbroman wrote:
> Out of curiosity (arguably out of scope for this patch, but I didn't
previously
> notice it), isn't "object.styleRef().position() != StaticPosition" equivalent
to
> "object.isPositioned()", which is shorter to write and faster to run IIUC?

I think yes. Looks like it's pretty safe to use.

[pdr., 2015-11-23 20:56:04]

The CQ bit was checked by pdr@chromium.org

[commit-bot: I haz the power, 2015-11-23 20:56:31]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1461223002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1461223002/60001

[commit-bot: I haz the power, 2015-11-23 21:12:16]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-11-23 21:12:53]

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