Re-implement SVG transform paint property nodes [spv2]

Re-implement SVG transform paint property nodes [spv2]

This patch adds a new property node for LayoutSVGRoot which maps from
the local SVG space to the HTML border box[1]. This patch also removes
the local SVG transform and makes SVG objects create transform nodes
in PaintPropertyTreeBuilder::updateTransform[2]. This approach will let
us support the other paint property types in SVG fairly easily in a
followup patch.

[1] Why do we need two transform nodes for the SVG root? LayoutSVGRoot
paints itself (e.g., borders, outlines) in the local HTML border box
transform space. LayoutSVGRoot's SVG children cannot paint in this space
because they need an additional transform (called the local svg to
border box) which offsets children by the border and includes SVG's
"viewbox".

[2] Why was m_svgLocalTransform removed? svgLocalTransform was designed
to support the local transform of both the SVG root and all SVG
descendants. Unfortunately, LayoutSVGRoot required that svgLocalTransform
get created after filters/clips in PaintPropertyTreeBuilder but that
left filters/clips in the wrong transform space. To solve this without
a parallel implementation of filters/clips/etc for SVG, SVG children
now create regular transform property nodes in PaintPropertyTreeBuilder.

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

[pdr., 2016-06-09 06:14:10]

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

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

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

[commit-bot: I haz the power, 2016-06-09 07:54:28]

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

[commit-bot: I haz the power, 2016-06-09 07:54:29]

Dry run: This issue passed the CQ dry run.

[pdr., 2016-06-09 16:55:13]

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

[jbroman, 2016-06-09 22:16:49]

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

https://codereview.chromium.org/2045253005/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:101: //
FIXME(pdr): SVG text nodes inherit from HTML and do change paint offset. Figure
out
TODO, not FIXME

https://codereview.chromium.org/2045253005/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:103: //
DCHECK(context.paintOffset == LayoutPoint());
Huh, removed? It's only a comment right now. Can you clarify?

https://codereview.chromium.org/2045253005/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:105: //
FIXME(pdr): Check for the presence of a transform instead of the value. Checking
for an
TODO, not FIXME

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

https://codereview.chromium.org/2045253005/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/SVGShapePainter.cpp:58: // FIXME(pdr):
Extract this out and use it in all non-root SVG painters.
TODO, not FIXME (also below). Why have you traveled back in time to WebKit
style? ;)

https://www.chromium.org/blink/coding-style

[jbroman, 2016-06-09 22:17:08]

(will look again tomorrow, but I'm heading home soon)

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

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

[pdr., 2016-06-09 23:36:37]

Description was changed from

==========
Re-implement SVG transform paint property nodes [spv2]

This patch adds a new property node for LayoutSVGRoot which maps from
the local SVG space to the HTML border box[1]. This patch also removes
the local SVG transform and switches SVG objects to create transform
in PaintPropertyTreeBuilder::updateTransform[2]. This approach will let
us support the other paint property types in SVG fairly easily in a
followup patch.

[1] Why do we need two transform nodes for the SVG root? LayoutSVGRoot
paints itself (e.g., borders, outlines) in the local HTML border box
transform space. LayoutSVGRoot's SVG children cannot paint in this space
because they need an additional transform (called the local svg to
border box) which offsets children by the border and includes SVG's
"viewbox".

[2] Why was m_svgLocalTransform removed? svgLocalTransform was designed
to support the local transform of both the SVG root and all SVG
descendants. Unfortunately, LayoutSVGRoot required that svgLocalTransform
get created after filters/clips in PaintPropertyTreeBuilder but that
left filters/clips in the wrong transform space. To solve this without
a parallel implementation of filters/clips/etc for SVG, SVG children
now create regular transform property nodes in PaintPropertyTreeBuilder.

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

to

==========
Re-implement SVG transform paint property nodes [spv2]

This patch adds a new property node for LayoutSVGRoot which maps from
the local SVG space to the HTML border box[1]. This patch also removes
the local SVG transform and makes SVG objects create transform nodes
in PaintPropertyTreeBuilder::updateTransform[2]. This approach will let
us support the other paint property types in SVG fairly easily in a
followup patch.

[1] Why do we need two transform nodes for the SVG root? LayoutSVGRoot
paints itself (e.g., borders, outlines) in the local HTML border box
transform space. LayoutSVGRoot's SVG children cannot paint in this space
because they need an additional transform (called the local svg to
border box) which offsets children by the border and includes SVG's
"viewbox".

[2] Why was m_svgLocalTransform removed? svgLocalTransform was designed
to support the local transform of both the SVG root and all SVG
descendants. Unfortunately, LayoutSVGRoot required that svgLocalTransform
get created after filters/clips in PaintPropertyTreeBuilder but that
left filters/clips in the wrong transform space. To solve this without
a parallel implementation of filters/clips/etc for SVG, SVG children
now create regular transform property nodes in PaintPropertyTreeBuilder.

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

[pdr., 2016-06-09 23:39:04]

PTAL

+chrishtr and fs as reviewers too.

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

https://codereview.chromium.org/2045253005/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:101: //
FIXME(pdr): SVG text nodes inherit from HTML and do change paint offset. Figure
out
On 2016/06/09 at 22:16:49, jbroman wrote:
> TODO, not FIXME

Newest patch answers the "fixme" and removes this comment.

https://codereview.chromium.org/2045253005/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:103: //
DCHECK(context.paintOffset == LayoutPoint());
On 2016/06/09 at 22:16:49, jbroman wrote:
> Huh, removed? It's only a comment right now. Can you clarify?

Clarified with removal.

https://codereview.chromium.org/2045253005/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:105: //
FIXME(pdr): Check for the presence of a transform instead of the value. Checking
for an
On 2016/06/09 at 22:16:49, jbroman wrote:
> TODO, not FIXME

Done.

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

https://codereview.chromium.org/2045253005/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/SVGShapePainter.cpp:58: // FIXME(pdr):
Extract this out and use it in all non-root SVG painters.
On 2016/06/09 at 22:16:49, jbroman wrote:
> TODO, not FIXME (also below). Why have you traveled back in time to WebKit
style? ;)
> 
> https://www.chromium.org/blink/coding-style

WTF::ShameCube(pdr)

We should add a presubmit script to catch this.

In any case, I've removed this code as it really belongs in the followup patch
(https://codereview.chromium.org/2059433002/) and wasn't needed here.

[fs, 2016-06-10 08:21:16]

Looks reasonable to me

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

https://codereview.chromium.org/2045253005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/ObjectPaintProperties.h:41: //     |  
+---[ svgLocalToBorderBoxTransform ] Additional transform for children of the
root SVG.
(Maybe mention that this would only ever apply for (outermost) <svg> roots. "for
children of the root" might make people think this node could usefully apply in
other places as well.)

[pdr., 2016-06-10 17:50:43]

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

https://codereview.chromium.org/2045253005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/ObjectPaintProperties.h:41: //     |  
+---[ svgLocalToBorderBoxTransform ] Additional transform for children of the
root SVG.
On 2016/06/10 at 08:21:16, fs wrote:
> (Maybe mention that this would only ever apply for (outermost) <svg> roots.
"for children of the root" might make people think this node could usefully
apply in other places as well.)

SGTM, done.

[pdr., 2016-06-10 18:11:05]

Tien-Ren is out until Monday and I'd like to wait for his input, so this will be
in review for a few more days.

[jbroman, 2016-06-10 18:33:53]

lgtm

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

https://codereview.chromium.org/2045253005/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:366: if
(boxModelObject.isBox() && (boxModelObject.isSVGRoot() ||
!boxModelObject.isSVG())) {
premature optimization: consider putting the !isSVG check first, because it's
more likely to be true and we can short-circuit the isSVGRoot call in that case.

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

https://codereview.chromium.org/2045253005/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/SVGRootPainter.cpp:52:
DCHECK(!objectProperties || !objectProperties->transform() ||
m_layoutSVGRoot.hasLayer());
To be clear, "layer" here is blink::PaintLayer, right?

[pdr., 2016-06-10 19:45:46]

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

https://codereview.chromium.org/2045253005/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:366: if
(boxModelObject.isBox() && (boxModelObject.isSVGRoot() ||
!boxModelObject.isSVG())) {
On 2016/06/10 at 18:33:52, jbroman wrote:
> premature optimization: consider putting the !isSVG check first, because it's
more likely to be true and we can short-circuit the isSVGRoot call in that case.

Optimized!

Nevermind the TODO one line down about a O(n^2) hiding in this conditional
though :P

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

https://codereview.chromium.org/2045253005/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/SVGRootPainter.cpp:52:
DCHECK(!objectProperties || !objectProperties->transform() ||
m_layoutSVGRoot.hasLayer());
On 2016/06/10 at 18:33:52, jbroman wrote:
> To be clear, "layer" here is blink::PaintLayer, right?

Yeah. I updated the comment to specify "paint layer" so this is more clear.

[trchen, 2016-06-13 22:44:27]

lgtm too

[pdr., 2016-06-13 22:57:30]

The CQ bit was checked by pdr@chromium.org

[pdr., 2016-06-13 22:57:31]

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/2045253005/#ps80001
(title: "Address reviewer comments")

[commit-bot: I haz the power, 2016-06-13 22:59:12]

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

[commit-bot: I haz the power, 2016-06-14 01:35:56]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-06-14 01:35:59]

CQ bit was unchecked

[commit-bot: I haz the power, 2016-06-14 01:37:41]

Description was changed from

==========
Re-implement SVG transform paint property nodes [spv2]

This patch adds a new property node for LayoutSVGRoot which maps from
the local SVG space to the HTML border box[1]. This patch also removes
the local SVG transform and makes SVG objects create transform nodes
in PaintPropertyTreeBuilder::updateTransform[2]. This approach will let
us support the other paint property types in SVG fairly easily in a
followup patch.

[1] Why do we need two transform nodes for the SVG root? LayoutSVGRoot
paints itself (e.g., borders, outlines) in the local HTML border box
transform space. LayoutSVGRoot's SVG children cannot paint in this space
because they need an additional transform (called the local svg to
border box) which offsets children by the border and includes SVG's
"viewbox".

[2] Why was m_svgLocalTransform removed? svgLocalTransform was designed
to support the local transform of both the SVG root and all SVG
descendants. Unfortunately, LayoutSVGRoot required that svgLocalTransform
get created after filters/clips in PaintPropertyTreeBuilder but that
left filters/clips in the wrong transform space. To solve this without
a parallel implementation of filters/clips/etc for SVG, SVG children
now create regular transform property nodes in PaintPropertyTreeBuilder.

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

to

==========
Re-implement SVG transform paint property nodes [spv2]

This patch adds a new property node for LayoutSVGRoot which maps from
the local SVG space to the HTML border box[1]. This patch also removes
the local SVG transform and makes SVG objects create transform nodes
in PaintPropertyTreeBuilder::updateTransform[2]. This approach will let
us support the other paint property types in SVG fairly easily in a
followup patch.

[1] Why do we need two transform nodes for the SVG root? LayoutSVGRoot
paints itself (e.g., borders, outlines) in the local HTML border box
transform space. LayoutSVGRoot's SVG children cannot paint in this space
because they need an additional transform (called the local svg to
border box) which offsets children by the border and includes SVG's
"viewbox".

[2] Why was m_svgLocalTransform removed? svgLocalTransform was designed
to support the local transform of both the SVG root and all SVG
descendants. Unfortunately, LayoutSVGRoot required that svgLocalTransform
get created after filters/clips in PaintPropertyTreeBuilder but that
left filters/clips in the wrong transform space. To solve this without
a parallel implementation of filters/clips/etc for SVG, SVG children
now create regular transform property nodes in PaintPropertyTreeBuilder.

BUG=600618

Committed: https://crrev.com/18f6c6a247ea3301c38e4d79a5785c7119f7cbca
Cr-Commit-Position: refs/heads/master@{#399635}
==========

[commit-bot: I haz the power, 2016-06-14 01:37:42]

Patchset 5 (id:??) landed as
https://crrev.com/18f6c6a247ea3301c38e4d79a5785c7119f7cbca
Cr-Commit-Position: refs/heads/master@{#399635}