Fix application of OBB paint-servers when device scale != 1

Fix application of OBB paint-servers when device scale != 1

The additional text transform would be applied after the "regular"
userspace transform, which in the object bounding box case meant that
any possible translation would not be applied correctly (wouldn't be
subject to the additional scaling.) The code was also sensitive to whether
or not any text-decorations needed to be painted with the paint-server,
since that could end up clobbering the userspace transform.

Fix by applying the additional text transform in the correct order, and
always re-compute the "resource space" transform in the relevant
applyResource(...) methods.
Give the additional transform for the non-scaling stroke case the same
treatment, since it could end up clobbering the userspace transform when
the same paint-server was used for both the stroke and the fill of an
object.
The code used for computing the "resource space" transform is split out of
RenderSVGResource{Gradient,Pattern} and moved to the super-class
(RenderSVGResourceContainer.)

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

[Stephen Chennney, 2014-04-09 15:19:20]

I think it's good, but pdr should look too.

[f(malita), 2014-04-09 16:26:15]

https://codereview.chromium.org/230443003/diff/20001/Source/core/rendering/sv...
File Source/core/rendering/svg/RenderSVGResourceGradient.cpp (right):

https://codereview.chromium.org/230443003/diff/20001/Source/core/rendering/sv...
Source/core/rendering/svg/RenderSVGResourceGradient.cpp:110: AffineTransform
computedGradientSpaceTransform = computeResourceSpaceTransform(object,
gradientData->userspaceTransform, svgStyle, resourceMode);
Isn't (resourceMode & ApplyToTextMode) invariant for any given *object (which is
the key for m_gradientMap)? We should be able to include the additional text
transform in the cached based transform, and only adjust the non-scaling stroke
component on applyResource(), no?

[f(malita), 2014-04-09 16:26:59]

On 2014/04/09 16:26:15, Florin Malita wrote:
> transform in the cached based transform

*cached base transform

[fs, 2014-04-09 16:59:13]

https://codereview.chromium.org/230443003/diff/20001/Source/core/rendering/sv...
File Source/core/rendering/svg/RenderSVGResourceGradient.cpp (right):

https://codereview.chromium.org/230443003/diff/20001/Source/core/rendering/sv...
Source/core/rendering/svg/RenderSVGResourceGradient.cpp:110: AffineTransform
computedGradientSpaceTransform = computeResourceSpaceTransform(object,
gradientData->userspaceTransform, svgStyle, resourceMode);
On 2014/04/09 16:26:15, Florin Malita wrote:
> Isn't (resourceMode & ApplyToTextMode) invariant for any given *object (which
is
> the key for m_gradientMap)? We should be able to include the additional text
> transform in the cached based transform, and only adjust the non-scaling
stroke
> component on applyResource(), no?

It's not invariant ATM - decorations are not painted with that flag set. I
suppose it would be possible to just pass/set that flag in that case too, and
hence be able to cache it as part of the "base" transform.
It does seem a bit unnecessary to tie in additional dependencies on the CTM if
it can be avoided - or?

[f(malita), 2014-04-09 17:46:50]

https://codereview.chromium.org/230443003/diff/20001/Source/core/rendering/sv...
File Source/core/rendering/svg/RenderSVGResourceGradient.cpp (right):

https://codereview.chromium.org/230443003/diff/20001/Source/core/rendering/sv...
Source/core/rendering/svg/RenderSVGResourceGradient.cpp:110: AffineTransform
computedGradientSpaceTransform = computeResourceSpaceTransform(object,
gradientData->userspaceTransform, svgStyle, resourceMode);
On 2014/04/09 16:59:13, fs wrote:
> On 2014/04/09 16:26:15, Florin Malita wrote:
> > Isn't (resourceMode & ApplyToTextMode) invariant for any given *object
(which
> is
> > the key for m_gradientMap)? We should be able to include the additional text
> > transform in the cached based transform, and only adjust the non-scaling
> stroke
> > component on applyResource(), no?
> 
> It's not invariant ATM - decorations are not painted with that flag set.

Ugh. I find all that SVGInlineTextBox::m_paintingResourceMode shuffling utterly
confusing.


> I suppose it would be possible to just pass/set that flag in that case too,
and
> hence be able to cache it as part of the "base" transform.

Wouldn't that produce different results though, as we're potentially tweaking
the transform? Maybe that's the correct thing to do for decorations anyway?

[fs, 2014-04-10 08:16:40]

https://codereview.chromium.org/230443003/diff/20001/Source/core/rendering/sv...
File Source/core/rendering/svg/RenderSVGResourceGradient.cpp (right):

https://codereview.chromium.org/230443003/diff/20001/Source/core/rendering/sv...
Source/core/rendering/svg/RenderSVGResourceGradient.cpp:110: AffineTransform
computedGradientSpaceTransform = computeResourceSpaceTransform(object,
gradientData->userspaceTransform, svgStyle, resourceMode);
On 2014/04/09 17:46:51, Florin Malita wrote:
> On 2014/04/09 16:59:13, fs wrote:
> > On 2014/04/09 16:26:15, Florin Malita wrote:
> > > Isn't (resourceMode & ApplyToTextMode) invariant for any given *object
> (which
> > is
> > > the key for m_gradientMap)? We should be able to include the additional
text
> > > transform in the cached based transform, and only adjust the non-scaling
> > stroke
> > > component on applyResource(), no?
> > 
> > It's not invariant ATM - decorations are not painted with that flag set.
> 
> Ugh. I find all that SVGInlineTextBox::m_paintingResourceMode shuffling
utterly
> confusing.

Agreed. (Good that you took care of it! *removes from TODO*)
 
> > I suppose it would be possible to just pass/set that flag in that case too,
> and
> > hence be able to cache it as part of the "base" transform.
> 
> Wouldn't that produce different results though, as we're potentially tweaking
> the transform? Maybe that's the correct thing to do for decorations anyway?

Well, if you start weighing in things like <textPath> it becomes less obvious
surely. In that case the additional per-glyph transform shouldn't affect the
gradient (hmm, need to test that...).
What's to be considered "correct" is just a matter of defining the semantics
though. (It's probably also worth noting that using non-solidcolor fills on
decorations is likely broken ATM for scale!=1 [hint: look at
paintDecorationWithStyle].)
Personally I'd lean towards the side that decouples instance-data as much as
possible (i.e. consider the possibility of moving the bbox-transform out as
well.)

[f(malita), 2014-04-10 12:57:47]

lgtm

https://codereview.chromium.org/230443003/diff/20001/Source/core/rendering/sv...
File Source/core/rendering/svg/RenderSVGResourceGradient.cpp (right):

https://codereview.chromium.org/230443003/diff/20001/Source/core/rendering/sv...
Source/core/rendering/svg/RenderSVGResourceGradient.cpp:110: AffineTransform
computedGradientSpaceTransform = computeResourceSpaceTransform(object,
gradientData->userspaceTransform, svgStyle, resourceMode);
On 2014/04/10 08:16:41, fs wrote:
> On 2014/04/09 17:46:51, Florin Malita wrote:
> > Wouldn't that produce different results though, as we're potentially
tweaking
> > the transform? Maybe that's the correct thing to do for decorations anyway?
> 
> Well, if you start weighing in things like <textPath> it becomes less obvious
> surely. In that case the additional per-glyph transform shouldn't affect the
> gradient (hmm, need to test that...).
> What's to be considered "correct" is just a matter of defining the semantics
> though. (It's probably also worth noting that using non-solidcolor fills on
> decorations is likely broken ATM for scale!=1 [hint: look at
> paintDecorationWithStyle].)
> Personally I'd lean towards the side that decouples instance-data as much as
> possible (i.e. consider the possibility of moving the bbox-transform out as
> well.)

SGTM.

[fs, 2014-04-10 14:18:08]

The CQ bit was checked by fs@opera.com

[commit-bot: I haz the power, 2014-04-10 14:18:15]

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

[commit-bot: I haz the power, 2014-04-10 14:18:30]

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

[commit-bot: I haz the power, 2014-04-10 14:18:30]

Failed to apply patch for LayoutTests/TestExpectations:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file LayoutTests/TestExpectations
  Hunk #1 FAILED at 1072.
  1 out of 1 hunk FAILED -- saving rejects to file
LayoutTests/TestExpectations.rej

Patch:       LayoutTests/TestExpectations
Index: LayoutTests/TestExpectations
diff --git a/LayoutTests/TestExpectations b/LayoutTests/TestExpectations
index
1c9a498f06c3aad042f80b28cf8f1c5b725fee68..295db6029983984c6ca5c30094512b9a567dd6a9
100644
--- a/LayoutTests/TestExpectations
+++ b/LayoutTests/TestExpectations
@@ -1072,6 +1072,10 @@ crbug.com/320139
virtual/softwarecompositing/repaint/repaint-into-ancestor-after
 crbug.com/320139 virtual/softwarecompositing/repaint/resize-repaint.html [
NeedsRebaseline ]
 crbug.com/320139 [ Mac ] css3/flexbox/repaint.html [ NeedsRebaseline ]
 
+crbug.com/360571 svg/custom/js-late-gradient-creation.svg [ NeedsRebaseline ]
+crbug.com/360571 svg/transforms/text-with-pattern-inside-transformed-html.xhtml
[ NeedsRebaseline ]
+crbug.com/360571 svg/transforms/text-with-pattern-with-svg-transform.svg [
NeedsRebaseline ]
+
 # Way to Underline position code changed, need to rebase files after
https://codereview.chromium.org/182923003/ .
 crbug.com/347510 [ Linux Mac Mavericks SnowLeopard Lion MountainLion Retina ]
editing/pasteboard/paste-line-endings-002.html [ NeedsRebaseline ]
 crbug.com/347510 [ Linux Mac Mavericks SnowLeopard Lion MountainLion Retina ]
fast/text/international/rtl-white-space-pre-wrap.html [ NeedsRebaseline ]

[fs, 2014-04-10 15:52:07]

The CQ bit was checked by fs@opera.com

[commit-bot: I haz the power, 2014-04-10 15:52:08]

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

[commit-bot: I haz the power, 2014-04-10 16:51:27]

Change committed as 171287