[SVG2] Make transform, gradientTransform and patternTransform presentation attributes.

[SVG2] Make transform, gradientTransform and patternTransform presentation attributes.

Spec:
http://www.w3.org/TR/css3-transforms/#svg-transform
http://www.w3.org/TR/css3-transforms/#svg-syntax
http://www.w3.org/TR/css3-transforms/#svg-gradient-transform-pattern-transform

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

[pdr., 2014-08-01 22:43:11]

https://codereview.chromium.org/423093014/diff/60001/LayoutTests/svg/custom/t...
File LayoutTests/svg/custom/transform-invalid.svg (left):

https://codereview.chromium.org/423093014/diff/60001/LayoutTests/svg/custom/t...
LayoutTests/svg/custom/transform-invalid.svg:10: <rect width="100" height="100"
transform="scale(20" fill="green"/>
Did you mean to remove this line?

https://codereview.chromium.org/423093014/diff/60001/Source/core/svg/Gradient...
File Source/core/svg/GradientAttributes.h (right):

https://codereview.chromium.org/423093014/diff/60001/Source/core/svg/Gradient...
Source/core/svg/GradientAttributes.h:65: // CSS transforms operate with
pre-scaled lengths. To make this work with SVG
This logic is all duplicated from SVGGraphicsElement::animatedLocalTransform.
Can you extract the svg/css transform logic out so it can be shared with
patterns, gradients, and SVGGraphicsElement too?

https://codereview.chromium.org/423093014/diff/60001/Source/core/svg/Gradient...
Source/core/svg/GradientAttributes.h:89: if (!transform.isIdentity())
Is this check needed?

https://codereview.chromium.org/423093014/diff/60001/Source/core/svg/SVGGraph...
File Source/core/svg/SVGGraphicsElement.cpp (right):

https://codereview.chromium.org/423093014/diff/60001/Source/core/svg/SVGGraph...
Source/core/svg/SVGGraphicsElement.cpp:220:
invalidateSVGPresentationAttributeStyle();
We don't make these calls for many other other presentation attributes in SVG
(e.g., SVGSVGElement's width and height, or SVGImageElement's width and height),
nor for the new pattern and gradient presentation attributes. Why is this
different?

https://codereview.chromium.org/423093014/diff/60001/Source/core/svg/SVGGraph...
File Source/core/svg/SVGGraphicsElement.h (right):

https://codereview.chromium.org/423093014/diff/60001/Source/core/svg/SVGGraph...
Source/core/svg/SVGGraphicsElement.h:85: 
Nit: no need for the extra line.

https://codereview.chromium.org/423093014/diff/60001/Source/core/svg/SVGPatte...
File Source/core/svg/SVGPatternElement.cpp (right):

https://codereview.chromium.org/423093014/diff/60001/Source/core/svg/SVGPatte...
Source/core/svg/SVGPatternElement.cpp:69: // Don't add invalid (empty) values to
the style
I don't understand this comment.

[Erik Dahlström (inactive), 2014-08-04 13:57:57]

https://codereview.chromium.org/423093014/diff/60001/LayoutTests/svg/custom/t...
File LayoutTests/svg/custom/transform-invalid.svg (left):

https://codereview.chromium.org/423093014/diff/60001/LayoutTests/svg/custom/t...
LayoutTests/svg/custom/transform-invalid.svg:10: <rect width="100" height="100"
transform="scale(20" fill="green"/>
On 2014/08/01 22:43:10, pdr wrote:
> Did you mean to remove this line?

CSS.supports("transform", "scale(20") returns true on ToT, which might be a bug
in the css parser (though Firefox Nightly returns the same too), in any case
this is the reason why I removed this line.

If you wish I can add a corresponding line in
LayoutTests/transforms/transform-parsing.html, I don't see why we would want the
parser to differ here. Whatever the CSS parser allows for transform should be
allowed in the presentation attribute.

https://codereview.chromium.org/423093014/diff/60001/Source/core/svg/Gradient...
File Source/core/svg/GradientAttributes.h (right):

https://codereview.chromium.org/423093014/diff/60001/Source/core/svg/Gradient...
Source/core/svg/GradientAttributes.h:65: // CSS transforms operate with
pre-scaled lengths. To make this work with SVG
On 2014/08/01 22:43:10, pdr wrote:
> This logic is all duplicated from SVGGraphicsElement::animatedLocalTransform.
> Can you extract the svg/css transform logic out so it can be shared with
> patterns, gradients, and SVGGraphicsElement too?

Yes, who likes duplicated code, right? :) I'll see what I can do.

https://codereview.chromium.org/423093014/diff/60001/Source/core/svg/Gradient...
Source/core/svg/GradientAttributes.h:89: if (!transform.isIdentity())
On 2014/08/01 22:43:10, pdr wrote:
> Is this check needed?

Technically no, since it won't make any visible difference. Was added to mostly
mimic the old code (which checked for the existence of the attribute). The
setGradientTransform method toggles a flag that makes the processing consider
the transform later on.

I guess this could just as well be if(style->hasTransform() ||
element->gradientTransform()->isSpecified()) instead.

https://codereview.chromium.org/423093014/diff/60001/Source/core/svg/SVGGraph...
File Source/core/svg/SVGGraphicsElement.cpp (right):

https://codereview.chromium.org/423093014/diff/60001/Source/core/svg/SVGGraph...
Source/core/svg/SVGGraphicsElement.cpp:220:
invalidateSVGPresentationAttributeStyle();
On 2014/08/01 22:43:10, pdr wrote:
> We don't make these calls for many other other presentation attributes in SVG
> (e.g., SVGSVGElement's width and height, or SVGImageElement's width and
height),
> nor for the new pattern and gradient presentation attributes. Why is this
> different?

We do call invalidateSVGPresentationAttributeStyle(); and
setNeedsStyleRecalc(LocalStyleChange); for the width/height attributes that have
been made presentation attributes (see SVGSVGElement and
SVGForeignObjectElement).

The gradient and pattern ones were missing, will add.

The reasoning behind this is in
http://code.google.com/p/chromium/issues/detail?id=364807.

https://codereview.chromium.org/423093014/diff/60001/Source/core/svg/SVGPatte...
File Source/core/svg/SVGPatternElement.cpp (right):

https://codereview.chromium.org/423093014/diff/60001/Source/core/svg/SVGPatte...
Source/core/svg/SVGPatternElement.cpp:69: // Don't add invalid (empty) values to
the style
On 2014/08/01 22:43:10, pdr wrote:
> I don't understand this comment.

It was a misunderstanding of the spec (this avoided some testsuite fails, but
was wrong). Will remove.

[pdr., 2014-08-04 16:11:17]

LGTM

https://codereview.chromium.org/423093014/diff/100001/Source/core/svg/SVGPatt...
File Source/core/svg/SVGPatternElement.cpp (right):

https://codereview.chromium.org/423093014/diff/100001/Source/core/svg/SVGPatt...
Source/core/svg/SVGPatternElement.cpp:132:
setNeedsStyleRecalc(LocalStyleChange);
Could a pattern's children depend on the transform of their parent? I'm
wondering if this should be SubtreeStyleChange instead of LocalStyleChange. The
font we use for text does depend on the full transform but this invalidation may
be handled elsewhere.

[Erik Dahlström (inactive), 2014-08-05 10:12:15]

On 2014/08/04 16:11:17, pdr wrote:
> LGTM
> 
>
https://codereview.chromium.org/423093014/diff/100001/Source/core/svg/SVGPatt...
> File Source/core/svg/SVGPatternElement.cpp (right):
> 
>
https://codereview.chromium.org/423093014/diff/100001/Source/core/svg/SVGPatt...
> Source/core/svg/SVGPatternElement.cpp:132:
> setNeedsStyleRecalc(LocalStyleChange);
> Could a pattern's children depend on the transform of their parent? 

According to Rune Lillesveen (rune@opera.com) "LocalStyleChange means
recalculate the computed styles for that element, and recalculate descendants as
far down as you need to cater for implicitly and explicitely inherited
properties that changed".

> I'm wondering if this should be SubtreeStyleChange instead of
LocalStyleChange. The
> font we use for text does depend on the full transform but this invalidation
may
> be handled elsewhere.

To verify, Patchset #7 used SubtreeStyleChange for the transform attribute
(which should have a much higher likelihood of triggering bugs than
patternTransform) and there was no real difference in LayoutTest results
compared to patchset #6.

I think LocalStyleChange is the right choice.

[Erik Dahlström (inactive), 2014-08-05 10:12:49]

The CQ bit was checked by ed@opera.com

[commit-bot: I haz the power, 2014-08-05 10:13:56]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ed@opera.com/423093014/140001

[commit-bot: I haz the power, 2014-08-05 15:16:39]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/1...)
  mac_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/18437)
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/20906)

[commit-bot: I haz the power, 2014-08-05 16:09:44]

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

[commit-bot: I haz the power, 2014-08-05 16:09:45]

Try jobs failed on following builders:
  mac_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/18466)

[Erik Dahlström (inactive), 2014-08-06 14:54:14]

The CQ bit was checked by ed@opera.com

[commit-bot: I haz the power, 2014-08-06 14:55:08]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ed@opera.com/423093014/160001

[commit-bot: I haz the power, 2014-08-06 18:09:13]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  mac_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/18676)
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/21394)

[commit-bot: I haz the power, 2014-08-06 18:54:32]

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

[commit-bot: I haz the power, 2014-08-06 18:54:33]

Try jobs failed on following builders:
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/21405)

[pdr., 2014-08-06 18:58:27]

The CQ bit was checked by pdr@chromium.org

[commit-bot: I haz the power, 2014-08-06 18:59:13]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ed@opera.com/423093014/160001

[pdr., 2014-08-06 19:05:12]

On 2014/08/06 18:54:33, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   win_blink_rel on tryserver.blink
>
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/21405)

It looks like the patch doesn't apply any longer :/

[commit-bot: I haz the power, 2014-08-06 20:09:34]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  mac_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/18692)
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/21412)

[commit-bot: I haz the power, 2014-08-06 20:55:40]

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

[commit-bot: I haz the power, 2014-08-06 20:55:41]

Try jobs failed on following builders:
  mac_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/18700)

[Erik Dahlström (inactive), 2014-08-07 08:45:31]

The CQ bit was checked by ed@opera.com

[commit-bot: I haz the power, 2014-08-07 08:46:30]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ed@opera.com/423093014/180001

[commit-bot: I haz the power, 2014-08-07 10:08:39]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/2...)
  win_blink_compile_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_compile_dbg/bu...)
  mac_gpu_retina_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu_retina_tr...)

[commit-bot: I haz the power, 2014-08-07 10:18:39]

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

[commit-bot: I haz the power, 2014-08-07 10:18:41]

Try jobs failed on following builders:
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/2...)

[Erik Dahlström (inactive), 2014-08-07 11:42:58]

The CQ bit was checked by ed@opera.com

[commit-bot: I haz the power, 2014-08-07 11:43:42]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ed@opera.com/423093014/180001

[commit-bot: I haz the power, 2014-08-07 12:41:20]

Change committed as 179702

[pdr., 2014-08-09 05:16:18]

On 2014/08/07 12:41:20, I haz the power (commit-bot) wrote:
> Change committed as 179702

This caused a 50% perf regression, see: http://crbug.com/402133

[Erik Dahlström (inactive), 2014-08-11 07:55:53]

A revert of this CL has been created in
https://codereview.chromium.org/456283002/ by ed@opera.com.

The reason for reverting is: Caused 10-50% performance regressions on svg, see
http://crbug.com/402133..