[SVG] Transform animation leaves traces of text behind.

[SVG] Transform animation leaves traces of text behind.

AffineTransform.inverse() unexpectedly returned an identity matrix when
the matrix was not invertible. This caused invalid rendering for e.g
scale(0) matrices in svg.

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

[Stephen Chennney, 2014-03-19 12:11:04]

https://codereview.chromium.org/200053005/diff/1/LayoutTests/svg/text/non-inv...
File LayoutTests/svg/text/non-invertible-matrix-text.svg (right):

https://codereview.chromium.org/200053005/diff/1/LayoutTests/svg/text/non-inv...
LayoutTests/svg/text/non-invertible-matrix-text.svg:5:
document.getElementById('first').setAttribute("transform",
"translate(241.0859375, -21)scale(0, 1)translate(-241.0859375, 21)");
Is there anything special about the 241.0859375 value? If not, please just use
200 or something that doesn't imply it's special.

https://codereview.chromium.org/200053005/diff/1/Source/core/rendering/PaintI...
File Source/core/rendering/PaintInfo.h (right):

https://codereview.chromium.org/200053005/diff/1/Source/core/rendering/PaintI...
Source/core/rendering/PaintInfo.h:88: void applyTransform(const AffineTransform&
localToAncestorTransform, bool identityStatusUnknown = true)
What code uses this? I'm worried that this patch is introducing a relatively
expensive operation (isInvertible) that is then immediately repeated inside
AffineTransform::inverse(). If it's just SVG, then fine. But if this is used a
lot then I think we need to add "inverseMapRect" to AffineTransform to avoid the
extra call.

[Erik Dahlström (inactive), 2014-03-19 14:00:09]

https://codereview.chromium.org/200053005/diff/1/LayoutTests/svg/text/non-inv...
File LayoutTests/svg/text/non-invertible-matrix-text.svg (right):

https://codereview.chromium.org/200053005/diff/1/LayoutTests/svg/text/non-inv...
LayoutTests/svg/text/non-invertible-matrix-text.svg:5:
document.getElementById('first').setAttribute("transform",
"translate(241.0859375, -21)scale(0, 1)translate(-241.0859375, 21)");
On 2014/03/19 12:11:05, Stephen Chenney wrote:
> Is there anything special about the 241.0859375 value? If not, please just use
> 200 or something that doesn't imply it's special.

This came from script-generated content, the translate-position is/was the
midpoint on the glyph. It has some relevance, but the decimalpoints might be
possible to remove if you want.

The important thing is to get the glyph to span across two tiles, because that's
when the leftovers become visible. The tiles right now are ~255px wide.

https://codereview.chromium.org/200053005/diff/1/Source/core/rendering/PaintI...
File Source/core/rendering/PaintInfo.h (right):

https://codereview.chromium.org/200053005/diff/1/Source/core/rendering/PaintI...
Source/core/rendering/PaintInfo.h:88: void applyTransform(const AffineTransform&
localToAncestorTransform, bool identityStatusUnknown = true)
On 2014/03/19 12:11:05, Stephen Chenney wrote:
> What code uses this? I'm worried that this patch is introducing a relatively
> expensive operation (isInvertible) that is then immediately repeated inside
> AffineTransform::inverse(). If it's just SVG, then fine. But if this is used a
> lot then I think we need to add "inverseMapRect" to AffineTransform to avoid
the
> extra call.

All of the users of the PaintInfo.applyTransform method I found were in svg,
everyone outside svg uses TransformationMatrix / TransformState AFAICT. 

Note that there was already one place that checked isInvertible (for
non-scaling-stroke), but it wasn't using this particular method just the same
call-pattern (AffineTransform.inverse().mapRect()).

[pdr., 2014-03-19 16:20:39]

On 2014/03/19 14:00:09, Erik Dahlström wrote:
>
https://codereview.chromium.org/200053005/diff/1/LayoutTests/svg/text/non-inv...
> File LayoutTests/svg/text/non-invertible-matrix-text.svg (right):
> 
>
https://codereview.chromium.org/200053005/diff/1/LayoutTests/svg/text/non-inv...
> LayoutTests/svg/text/non-invertible-matrix-text.svg:5:
> document.getElementById('first').setAttribute("transform",
> "translate(241.0859375, -21)scale(0, 1)translate(-241.0859375, 21)");
> On 2014/03/19 12:11:05, Stephen Chenney wrote:
> > Is there anything special about the 241.0859375 value? If not, please just
use
> > 200 or something that doesn't imply it's special.
> 
> This came from script-generated content, the translate-position is/was the
> midpoint on the glyph. It has some relevance, but the decimalpoints might be
> possible to remove if you want.
> 
> The important thing is to get the glyph to span across two tiles, because
that's
> when the leftovers become visible. The tiles right now are ~255px wide.
> 
>
https://codereview.chromium.org/200053005/diff/1/Source/core/rendering/PaintI...
> File Source/core/rendering/PaintInfo.h (right):
> 
>
https://codereview.chromium.org/200053005/diff/1/Source/core/rendering/PaintI...
> Source/core/rendering/PaintInfo.h:88: void applyTransform(const
AffineTransform&
> localToAncestorTransform, bool identityStatusUnknown = true)
> On 2014/03/19 12:11:05, Stephen Chenney wrote:
> > What code uses this? I'm worried that this patch is introducing a relatively
> > expensive operation (isInvertible) that is then immediately repeated inside
> > AffineTransform::inverse(). If it's just SVG, then fine. But if this is used
a
> > lot then I think we need to add "inverseMapRect" to AffineTransform to avoid
> the
> > extra call.
> 
> All of the users of the PaintInfo.applyTransform method I found were in svg,
> everyone outside svg uses TransformationMatrix / TransformState AFAICT. 
> 
> Note that there was already one place that checked isInvertible (for
> non-scaling-stroke), but it wasn't using this particular method just the same
> call-pattern (AffineTransform.inverse().mapRect()).

LGTM

This looks good to me but please wait for Schenney in case he has further
concerns.

[Stephen Chennney, 2014-03-20 14:30:39]

Explanations sound good. Please just add a comment to the test file regarding
the "must span two tiles" issue. It's unfortunate that if tile size changes or
we drop tiles entirely the test will not really be testing, but that's life. I
don't think we can fix it.

LGTM with above nit.

[Erik Dahlström (inactive), 2014-03-20 15:10:39]

The CQ bit was checked by ed@opera.com

[commit-bot: I haz the power, 2014-03-20 15:10:43]

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

[commit-bot: I haz the power, 2014-03-20 15:13:54]

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

[commit-bot: I haz the power, 2014-03-20 15:13:55]

Try jobs failed on following builders:
  tryserver.blink on linux_blink_dbg

[Erik Dahlström (inactive), 2014-03-20 15:45:17]

The CQ bit was checked by ed@opera.com

[commit-bot: I haz the power, 2014-03-20 15:45:23]

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

[commit-bot: I haz the power, 2014-03-20 16:52:44]

Change committed as 169666