Use transform paint property nodes in SVG [spv2]

Use transform paint property nodes in SVG [spv2]

This patch starts using transform paint property nodes when painting
SVG. Typically PaintLayer is responsible for noticing new property
nodes during painting but non-root SVG objects do not create PaintLayers
and need to check for properties themselves.

Several difficult transform areas remain such as marker transforms,
but this patch gets the bulk of SVG switched over. With this patch, 93
new layout tests pass with spv2 enabled.

This patch builds on https://codereview.chromium.org/2045253005

BUG=600618
Committed: https://crrev.com/e06f2cf02f5ec0ce37164bf3e458593e61fb2e36
Cr-Commit-Position: refs/heads/master@{#401172}

[pdr., 2016-06-09 21:07:16]

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

[commit-bot: I haz the power, 2016-06-09 21:07:39]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2059433002/1

[pdr., 2016-06-09 21:08:11]

Description was changed from

==========
Use transform paint property nodes in SVG [spv2]

This patch starts using transform paint property nodes when painting
SVG. Typically PaintLayer is responsible for noticing new property
nodes during painting but non-root SVG objects do not create PaintLayers
and need to check for properties themselves.

Several difficult transform areas remain such as marker transforms,
but this patch gets the bulk of SVG switched over. With this patch, 75
new layout tests pass with spv2 enabled.

This patch builds on https://codereview.chromium.org/2049263003

BUG=600618
==========

to

==========
Use transform paint property nodes in SVG [spv2]

This patch starts using transform paint property nodes when painting
SVG. Typically PaintLayer is responsible for noticing new property
nodes during painting but non-root SVG objects do not create PaintLayers
and need to check for properties themselves.

Several difficult transform areas remain such as marker transforms,
but this patch gets the bulk of SVG switched over. With this patch, 75
new layout tests pass with spv2 enabled.

This patch builds on https://codereview.chromium.org/2045253005

BUG=600618
==========

[pdr., 2016-06-09 21:08:12]

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

[chrishtr, 2016-06-09 22:21:06]

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

[chrishtr, 2016-06-09 22:21:07]

Will this patch result in transforms for every transform in an SVG?

[pdr., 2016-06-09 22:27:22]

On 2016/06/09 at 22:21:07, chrishtr wrote:
> Will this patch result in transforms for every transform in an SVG?

Yes (sort of). The first patchset in this series
(https://codereview.chromium.org/2045253005/) begins creating transforms for any
SVG object with a transform. I'll add you (and fs) to the review.

[commit-bot: I haz the power, 2016-06-09 23:30:37]

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

[commit-bot: I haz the power, 2016-06-09 23:30:38]

Dry run: This issue passed the CQ dry run.

[pdr., 2016-06-10 00:45:24]

Description was changed from

==========
Use transform paint property nodes in SVG [spv2]

This patch starts using transform paint property nodes when painting
SVG. Typically PaintLayer is responsible for noticing new property
nodes during painting but non-root SVG objects do not create PaintLayers
and need to check for properties themselves.

Several difficult transform areas remain such as marker transforms,
but this patch gets the bulk of SVG switched over. With this patch, 75
new layout tests pass with spv2 enabled.

This patch builds on https://codereview.chromium.org/2045253005

BUG=600618
==========

to

==========
Use transform paint property nodes in SVG [spv2]

This patch starts using transform paint property nodes when painting
SVG. Typically PaintLayer is responsible for noticing new property
nodes during painting but non-root SVG objects do not create PaintLayers
and need to check for properties themselves.

Several difficult transform areas remain such as marker transforms,
but this patch gets the bulk of SVG switched over. With this patch, 93
new layout tests pass with spv2 enabled.

This patch builds on https://codereview.chromium.org/2045253005

BUG=600618
==========

[pdr., 2016-06-10 01:28:35]

pdr@chromium.org changed reviewers:
+ fs@opera.com

[trchen, 2016-06-13 22:45:19]

https://codereview.chromium.org/2059433002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/SVGPaintContext.h (right):

https://codereview.chromium.org/2059433002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/SVGPaintContext.h:62: if
(objectProperties->svgLocalToBorderBoxTransform()) {
Is it possible to add a DCHECK here to verify the transform passed-in being
equivalent to the one we apply by property node? e.g.
DCHECK_EQ(objectProperties->svgLocalToBorderBoxTransform()->transform(),
transform);

https://codereview.chromium.org/2059433002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/SVGPaintContext.h:73: if
(objectProperties->transform()) {
Ditto.

[pdr., 2016-06-13 23:03:19]

https://codereview.chromium.org/2059433002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/SVGPaintContext.h (right):

https://codereview.chromium.org/2059433002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/SVGPaintContext.h:62: if
(objectProperties->svgLocalToBorderBoxTransform()) {
On 2016/06/13 at 22:45:18, trchen wrote:
> Is it possible to add a DCHECK here to verify the transform passed-in being
equivalent to the one we apply by property node? e.g.
> DCHECK_EQ(objectProperties->svgLocalToBorderBoxTransform()->transform(),
transform);

These will actually be different because the svg root needs to support two
independent transform spaces (see SVGRootLocalToBorderBoxTransformNode in
https://codereview.chromium.org/2045253005).

https://codereview.chromium.org/2059433002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/SVGPaintContext.h:73: if
(objectProperties->transform()) {
On 2016/06/13 at 22:45:19, trchen wrote:
> Ditto.

svgLocalToBorderBoxTransform will only be set on the root svg object and this is
dcheck'd on line 71.

[trchen, 2016-06-13 23:26:52]

https://codereview.chromium.org/2059433002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/SVGPaintContext.h (right):

https://codereview.chromium.org/2059433002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/SVGPaintContext.h:62: if
(objectProperties->svgLocalToBorderBoxTransform()) {
On 2016/06/13 23:03:19, pdr. wrote:
> On 2016/06/13 at 22:45:18, trchen wrote:
> > Is it possible to add a DCHECK here to verify the transform passed-in being
> equivalent to the one we apply by property node? e.g.
> > DCHECK_EQ(objectProperties->svgLocalToBorderBoxTransform()->transform(),
> transform);
> 
> These will actually be different because the svg root needs to support two
> independent transform spaces (see SVGRootLocalToBorderBoxTransformNode in
> https://codereview.chromium.org/2045253005).

At this point we are in the border box space of the SVG root. I believe it is
also safe to assume currentTransformNode ==
objectProperties->svgLocalToBorderBoxTransform()->parent() . Whether it's SPv1
or SPv2, the matrix to go from the border box to the SVG viewport are supposed
to be the same. If they don't match, it could be a signal that there is a
mismatch in our implementation.

I believe the problem lies in PaintPropertyTreeBuilder::updateSvgLocalTransform
only baked in the paint offset without doing LayoutUnit snapping. I think it
should match SVGRootPainter::paint, line 24 and 56~62. More layout tests will
pass with that. :)

https://codereview.chromium.org/2059433002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/SVGPaintContext.h:73: if
(objectProperties->transform()) {
On 2016/06/13 23:03:19, pdr. wrote:
> On 2016/06/13 at 22:45:19, trchen wrote:
> > Ditto.
> 
> svgLocalToBorderBoxTransform will only be set on the root svg object and this
is
> dcheck'd on line 71.

I mean something similar like
DCHECK_EQ(objectProperties->transform()->transform(), transform);
Is there a case that they don't match?

https://codereview.chromium.org/2059433002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/SVGRootPainter.cpp (right):

https://codereview.chromium.org/2059433002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/SVGRootPainter.cpp:62:
paintOffsetToBorderBox.multiply(m_layoutSVGRoot.localToBorderBoxTransform());
PaintPropertyTreeBuilder::updateSvgLocalToBorderBoxTransform should match this.

[pdr., 2016-06-21 22:26:21]

https://codereview.chromium.org/2059433002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/SVGPaintContext.h (right):

https://codereview.chromium.org/2059433002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/SVGPaintContext.h:62: if
(objectProperties->svgLocalToBorderBoxTransform()) {
On 2016/06/13 at 23:26:52, trchen wrote:
> On 2016/06/13 23:03:19, pdr. wrote:
> > On 2016/06/13 at 22:45:18, trchen wrote:
> > > Is it possible to add a DCHECK here to verify the transform passed-in
being
> > equivalent to the one we apply by property node? e.g.
> > > DCHECK_EQ(objectProperties->svgLocalToBorderBoxTransform()->transform(),
> > transform);
> > 
> > These will actually be different because the svg root needs to support two
> > independent transform spaces (see SVGRootLocalToBorderBoxTransformNode in
> > https://codereview.chromium.org/2045253005).
> 
> At this point we are in the border box space of the SVG root. I believe it is
also safe to assume currentTransformNode ==
objectProperties->svgLocalToBorderBoxTransform()->parent() . Whether it's SPv1
or SPv2, the matrix to go from the border box to the SVG viewport are supposed
to be the same. If they don't match, it could be a signal that there is a
mismatch in our implementation.
> 
> I believe the problem lies in
PaintPropertyTreeBuilder::updateSvgLocalTransform only baked in the paint offset
without doing LayoutUnit snapping. I think it should match
SVGRootPainter::paint, line 24 and 56~62. More layout tests will pass with that.
:)

Ahh, I misunderstood what you meant. Excellent points.

In the latest patch I've added a FIXME to include pixel snapping in the
svgLocalToBorderBoxTransform. Mind if I fix that in a followup? We discussed
this a bit and it will be a little involved in PaintPropertyTreeBuilder.

https://codereview.chromium.org/2059433002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/SVGPaintContext.h:73: if
(objectProperties->transform()) {
On 2016/06/13 at 23:26:52, trchen wrote:
> On 2016/06/13 23:03:19, pdr. wrote:
> > On 2016/06/13 at 22:45:19, trchen wrote:
> > > Ditto.
> > 
> > svgLocalToBorderBoxTransform will only be set on the root svg object and
this is
> > dcheck'd on line 71.
> 
> I mean something similar like
> DCHECK_EQ(objectProperties->transform()->transform(), transform);
> Is there a case that they don't match?

Done. This actually caught a bug in PaintPropertyTreeBuilder where we need to
use a different (localSVGTransform) for foreignObject instead of the regular
localToSVGParentTransform. FIXME added.

[pdr., 2016-06-21 22:33:57]

@trchen, could you take another look? Sorry it's been a while due to blinkon and
you'll have to re-page this back in.

[trchen, 2016-06-21 22:55:35]

On 2016/06/21 22:33:57, pdr. wrote:
> @trchen, could you take another look? Sorry it's been a while due to blinkon
and
> you'll have to re-page this back in.

Hey welcome back! lgtm.

[pdr., 2016-06-21 23:12:06]

The CQ bit was checked by pdr@chromium.org

[commit-bot: I haz the power, 2016-06-21 23:12:52]

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

[commit-bot: I haz the power, 2016-06-22 02:27:04]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-06-22 02:28:16]

Description was changed from

==========
Use transform paint property nodes in SVG [spv2]

This patch starts using transform paint property nodes when painting
SVG. Typically PaintLayer is responsible for noticing new property
nodes during painting but non-root SVG objects do not create PaintLayers
and need to check for properties themselves.

Several difficult transform areas remain such as marker transforms,
but this patch gets the bulk of SVG switched over. With this patch, 93
new layout tests pass with spv2 enabled.

This patch builds on https://codereview.chromium.org/2045253005

BUG=600618
==========

to

==========
Use transform paint property nodes in SVG [spv2]

This patch starts using transform paint property nodes when painting
SVG. Typically PaintLayer is responsible for noticing new property
nodes during painting but non-root SVG objects do not create PaintLayers
and need to check for properties themselves.

Several difficult transform areas remain such as marker transforms,
but this patch gets the bulk of SVG switched over. With this patch, 93
new layout tests pass with spv2 enabled.

This patch builds on https://codereview.chromium.org/2045253005

BUG=600618

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

[commit-bot: I haz the power, 2016-06-22 02:28:17]

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