Make 'transform' a presentation attribute on SVG elements

Make 'transform' a presentation attribute on SVG elements

This makes 'transform', 'gradientTransform' and 'patternTransform'
presentation attributes on SVGGraphicsElements, SVGGradientElements
and SVGPatternElements respectively.

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

Salvaged from https://codereview.chromium.org/423093014, but takes a
different approach to bridge the syntax gap and avoid crbug.com/577219.

The strategy taken here is to use the SVGTransformList to generate a
CSSValue for the presentation attribute style, and hence postponing
both support for the full transform syntax and a way around the bug
mentioned above. Essentially softening the blow. These two "features"
are expected to be implemented eventually, so this is just a "first
step".

BUG=369942
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Committed: https://crrev.com/f72170ee0164e5d4bcf14551603907ffafd969f1
Cr-Commit-Position: refs/heads/master@{#434934}

[fs, 2016-11-04 14:38:34]

The CQ bit was checked by fs@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-11-04 14:38:47]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[fs, 2016-11-04 14:41:41]

Description was changed from

==========
Make 'transform' a presentation attribute on SVG elements

This makes 'transform', 'gradientTransform' and 'patternTransform'
presentation attributes on SVGGraphicsElements, SVGGradientElements
and SVGPatternElements respectively.

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

Salvaged from https://codereview.chromium.org/423093014, but takes a
different approach to bridge the syntax gap and avoid crbug.com/577219.

BUG=369942
==========

to

==========
Make 'transform' a presentation attribute on SVG elements

This makes 'transform', 'gradientTransform' and 'patternTransform'
presentation attributes on SVGGraphicsElements, SVGGradientElements
and SVGPatternElements respectively.

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

Salvaged from https://codereview.chromium.org/423093014, but takes a
different approach to bridge the syntax gap and avoid crbug.com/577219.

The strategy taken here is to use the SVGTransformList to generate a
CSSValue for the presentation attribute style, and hence postponing
both support for the full transform syntax and a way around the bug
mentioned above. Essentially softening the blow. These two "features"
are expected to be implemented eventually, so this is just a "first
step".

BUG=369942
==========

[commit-bot: I haz the power, 2016-11-04 15:02:06]

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

[commit-bot: I haz the power, 2016-11-04 15:02:07]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[fs, 2016-11-04 15:09:03]

The CQ bit was checked by fs@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-11-04 15:09:19]

Dry run: 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-11-04 16:07:15]

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

[commit-bot: I haz the power, 2016-11-04 16:07:16]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[fs, 2016-11-07 13:50:38]

The CQ bit was checked by fs@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-11-07 13:50:50]

Dry run: 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-11-07 15:04:00]

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

[commit-bot: I haz the power, 2016-11-07 15:04:01]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[fs, 2016-11-07 17:00:05]

The CQ bit was checked by fs@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-11-07 17:00:35]

Dry run: 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-11-07 18:25:02]

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

[commit-bot: I haz the power, 2016-11-07 18:25:03]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[fs, 2016-11-16 17:42:36]

The CQ bit was checked by fs@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-11-16 17:43:27]

Dry run: 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-11-16 19:49:21]

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

[commit-bot: I haz the power, 2016-11-16 19:49:22]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[fs, 2016-11-17 16:24:01]

The CQ bit was checked by fs@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-11-17 16:24:32]

Dry run: 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-11-17 18:28:40]

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

[commit-bot: I haz the power, 2016-11-17 18:28:41]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[fs, 2016-11-18 11:42:01]

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

[fs, 2016-11-18 11:42:03]

This is a sort of "step 1". The next step would be to support 3-argument
rotate(...) in the CSS parser per
https://drafts.csswg.org/css-transforms-1/#svg-transform-functions, and then try
to move SVGTransformList over to that.

Note that this is know to regress performance in some cases (see
https://bugs.chromium.org/p/chromium/issues/detail?id=369942#c20 for some data.)
What those tests have in common is heavy use of 'transform' attributes. This is
of course something that needs to be weighed in. (Something like
https://codereview.chromium.org/2476923002 can eliminate the speed-bump, but may
carry some controversy, so best considered separately.)

[pdr., 2016-11-22 04:02:49]

Sorry about the slow review. I think this is reasonable overall.

@Timloh, could you review this as well?

https://codereview.chromium.org/2478233002/diff/120001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/platform/linux/paint/invalidation/svg/text-rescale-expected.txt
(right):

https://codereview.chromium.org/2478233002/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/platform/linux/paint/invalidation/svg/text-rescale-expected.txt:75:
"object": "LayoutSVGForeignObject foreignObject",
Do you know where these new foreign object invalidations are from?

https://codereview.chromium.org/2478233002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/svg/SVGElement.cpp (right):

https://codereview.chromium.org/2478233002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/svg/SVGElement.cpp:364:
ComputedStyle::IncludeMotionPath,
Does this include motion path but exclude animate motion? Is that correct?

https://codereview.chromium.org/2478233002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/svg/SVGElement.h (right):

https://codereview.chromium.org/2478233002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/svg/SVGElement.h:77: bool hasTransform() const;
hasTransform and hasAnimatedLocalTransform are pretty similar. Is there a way to
make their differenced more clear?

[fs, 2016-11-22 13:54:07]

https://codereview.chromium.org/2478233002/diff/120001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/platform/linux/paint/invalidation/svg/text-rescale-expected.txt
(right):

https://codereview.chromium.org/2478233002/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/platform/linux/paint/invalidation/svg/text-rescale-expected.txt:75:
"object": "LayoutSVGForeignObject foreignObject",
On 2016/11/22 at 04:02:48, pdr. wrote:
> Do you know where these new foreign object invalidations are from?

We now fall into the "generic" transform invalidation code (in
LayoutObject::adjustStyleDifference), so we get "subtree" on the <g>'s that have
their transform changed, which propagates to descendants. Previously I believe
we would've been invalidating via the "screen scale changed" code-path (and thus
got the "forced..." reason where applicable.) In the "old" way we presumably
resolved to a "none" reason for the fO (maybe because
paintedOutputOfObjectHasNoEffectRegardlessOfSize returned true.)

https://codereview.chromium.org/2478233002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/svg/SVGElement.cpp (right):

https://codereview.chromium.org/2478233002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/svg/SVGElement.cpp:364:
ComputedStyle::IncludeMotionPath,
On 2016/11/22 at 04:02:48, pdr. wrote:
> Does this include motion path but exclude animate motion? Is that correct?

Yes, correct. (SVGGraphicsElement::calculateAnimatedLocalTransform applies the
latter.)

https://codereview.chromium.org/2478233002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/svg/SVGElement.h (right):

https://codereview.chromium.org/2478233002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/svg/SVGElement.h:77: bool hasTransform() const;
On 2016/11/22 at 04:02:48, pdr. wrote:
> hasTransform and hasAnimatedLocalTransform are pretty similar. Is there a way
to make their differenced more clear?

I guess we could consider renaming the "...AnimatedLocal..." methods to be more
about including the "motion transform" contribution.
Another option would be to just fold the "motion transform" application into
these two, and then have a "IncludeMotionTransform" enumeration argument a'la
the ComputedStyle method? (I think that would trade one vcall for another, so
hopefully not an issue from that PoV.)

[fs, 2016-11-22 14:01:24]

https://codereview.chromium.org/2478233002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/svg/SVGElement.h (right):

https://codereview.chromium.org/2478233002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/svg/SVGElement.h:77: bool hasTransform() const;
On 2016/11/22 at 13:54:07, fs wrote:
> On 2016/11/22 at 04:02:48, pdr. wrote:
> > hasTransform and hasAnimatedLocalTransform are pretty similar. Is there a
way to make their differenced more clear?
> 
> I guess we could consider renaming the "...AnimatedLocal..." methods to be
more about including the "motion transform" contribution.
> Another option would be to just fold the "motion transform" application into
these two, and then have a "IncludeMotionTransform" enumeration argument a'la
the ComputedStyle method? (I think that would trade one vcall for another, so
hopefully not an issue from that PoV.)

(Appears I mistakenly thought one of the "...AnimatedLocal..." methods was
virtual, but that was not the case.)

[fs, 2016-11-22 14:50:00]

Description was changed from

==========
Make 'transform' a presentation attribute on SVG elements

This makes 'transform', 'gradientTransform' and 'patternTransform'
presentation attributes on SVGGraphicsElements, SVGGradientElements
and SVGPatternElements respectively.

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

Salvaged from https://codereview.chromium.org/423093014, but takes a
different approach to bridge the syntax gap and avoid crbug.com/577219.

The strategy taken here is to use the SVGTransformList to generate a
CSSValue for the presentation attribute style, and hence postponing
both support for the full transform syntax and a way around the bug
mentioned above. Essentially softening the blow. These two "features"
are expected to be implemented eventually, so this is just a "first
step".

BUG=369942
==========

to

==========
Make 'transform' a presentation attribute on SVG elements

This makes 'transform', 'gradientTransform' and 'patternTransform'
presentation attributes on SVGGraphicsElements, SVGGradientElements
and SVGPatternElements respectively.

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

Salvaged from https://codereview.chromium.org/423093014, but takes a
different approach to bridge the syntax gap and avoid crbug.com/577219.

The strategy taken here is to use the SVGTransformList to generate a
CSSValue for the presentation attribute style, and hence postponing
both support for the full transform syntax and a way around the bug
mentioned above. Essentially softening the blow. These two "features"
are expected to be implemented eventually, so this is just a "first
step".

BUG=369942
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[fs, 2016-11-22 14:50:05]

The CQ bit was checked by fs@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-11-22 14:50:24]

Dry run: 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-11-22 14:53:52]

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

[commit-bot: I haz the power, 2016-11-22 14:53:53]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[fs, 2016-11-22 14:54:35]

https://codereview.chromium.org/2478233002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/svg/SVGElement.h (right):

https://codereview.chromium.org/2478233002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/svg/SVGElement.h:77: bool hasTransform() const;
On 2016/11/22 at 14:01:24, fs wrote:
> On 2016/11/22 at 13:54:07, fs wrote:
> > On 2016/11/22 at 04:02:48, pdr. wrote:
> > > hasTransform and hasAnimatedLocalTransform are pretty similar. Is there a
way to make their differenced more clear?
> > 
> > I guess we could consider renaming the "...AnimatedLocal..." methods to be
more about including the "motion transform" contribution.
> > Another option would be to just fold the "motion transform" application into
these two, and then have a "IncludeMotionTransform" enumeration argument a'la
the ComputedStyle method? (I think that would trade one vcall for another, so
hopefully not an issue from that PoV.)
> 
> (Appears I mistakenly thought one of the "...AnimatedLocal..." methods was
virtual, but that was not the case.)

PS8 contains something along the lines of the latter suggestion.

[pdr., 2016-11-22 21:19:28]

LGTM

@timloh, I think the SVG side of this looks good. I also think the CSS side
looks good, but I'd like a second pair of eyes from someone who knows more about
CSS.

[Timothy Loh, 2016-11-23 00:17:25]

On 2016/11/22 21:19:28, pdr. wrote:
> LGTM
> 
> @timloh, I think the SVG side of this looks good. I also think the CSS side
> looks good, but I'd like a second pair of eyes from someone who knows more
about
> CSS.

lgtm

[fs, 2016-11-28 15:28:52]

The CQ bit was checked by fs@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-11-28 15:29:12]

Dry run: 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-11-28 16:57:35]

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

[commit-bot: I haz the power, 2016-11-28 16:57:36]

Dry run: This issue passed the CQ dry run.

[fs, 2016-11-29 09:14:52]

The CQ bit was checked by fs@opera.com

[fs, 2016-11-29 09:14:52]

The patchset sent to the CQ was uploaded after l-g-t-m from pdr@chromium.org,
timloh@chromium.org
Link to the patchset: https://codereview.chromium.org/2478233002/#ps160001
(title: "Rebase")

[commit-bot: I haz the power, 2016-11-29 09:15:18]

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-11-29 09:21:00]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1480410892140010,
"parent_rev": "a3b28337d18aab2732ed64e2a930fd207bab27ba", "commit_rev":
"f9ebe50238b75c6b6e6ec711054b9d3ad17a1f9c"}

[commit-bot: I haz the power, 2016-11-29 09:21:29]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-11-29 09:34:38]

Description was changed from

==========
Make 'transform' a presentation attribute on SVG elements

This makes 'transform', 'gradientTransform' and 'patternTransform'
presentation attributes on SVGGraphicsElements, SVGGradientElements
and SVGPatternElements respectively.

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

Salvaged from https://codereview.chromium.org/423093014, but takes a
different approach to bridge the syntax gap and avoid crbug.com/577219.

The strategy taken here is to use the SVGTransformList to generate a
CSSValue for the presentation attribute style, and hence postponing
both support for the full transform syntax and a way around the bug
mentioned above. Essentially softening the blow. These two "features"
are expected to be implemented eventually, so this is just a "first
step".

BUG=369942
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Make 'transform' a presentation attribute on SVG elements

This makes 'transform', 'gradientTransform' and 'patternTransform'
presentation attributes on SVGGraphicsElements, SVGGradientElements
and SVGPatternElements respectively.

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

Salvaged from https://codereview.chromium.org/423093014, but takes a
different approach to bridge the syntax gap and avoid crbug.com/577219.

The strategy taken here is to use the SVGTransformList to generate a
CSSValue for the presentation attribute style, and hence postponing
both support for the full transform syntax and a way around the bug
mentioned above. Essentially softening the blow. These two "features"
are expected to be implemented eventually, so this is just a "first
step".

BUG=369942
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

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

[commit-bot: I haz the power, 2016-11-29 09:34:40]

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