[webkit-reviews] review denied: [Bug 64966] [svg] SVGResources applied to Text with Incorrect Transformations in non-CG Implementations : [Attachment 146304] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Fri Jun 8 00:04:11 PDT 2012
Nikolas Zimmermann <zimmermann at kde.org> has denied Dominik Röttsches (drott)
<dominik.rottsches at intel.com>'s request for review:
Bug 64966: [svg] SVGResources applied to Text with Incorrect Transformations in
non-CG Implementations
https://bugs.webkit.org/show_bug.cgi?id=64966
Attachment 146304: Patch
https://bugs.webkit.org/attachment.cgi?id=146304&action=review
------- Additional Comments from Nikolas Zimmermann <zimmermann at kde.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=146304&action=review
Nice patch! A first round of comments, r- mostly because of chromium redness
and some suggestions:
> Source/WebCore/rendering/svg/RenderSVGResourceContainer.cpp:189
> +AffineTransform
RenderSVGResourceContainer::transformOnTextPainting(RenderObject* object, const
AffineTransform& resourceTransform)
We shouldn't return AffineTransforms, but instead pass it by reference as
argument.
I'm aware the existing transformOnNonScalingStroke() code path does it this
way, but it's deprecated and will be changed soon.
> Source/WebCore/rendering/svg/RenderSVGResourceContainer.cpp:201
> + AffineTransform ctm;
> +
SVGRenderingContext::calculateTransformationToOutermostSVGCoordinateSystem(obje
ct, ctm);
> + float scalingFactor = narrowPrecisionToFloat(sqrt((pow(ctm.xScale(), 2)
+ pow(ctm.yScale(), 2)) / 2));
This could needs to be shared between here and RenderSVGInlineText in a single
place.
Maybe SVGRenderingContext is a good place?
> Source/WebCore/rendering/svg/RenderSVGResourceContainer.cpp:206
> + AffineTransform transform;
> + transform.scale(scalingFactor);
> + transform *= resourceTransform;
> + return transform;
You should early exit here for scalingFactor==1, and not build a
transformation.
> Source/WebCore/rendering/svg/RenderSVGResourceGradient.cpp:171
> +#if !USE(CG)
I'd avoid to sprinkle around USE(CG) blocks. I'd propose to add a
shouldTransformResourceOnTextPainting() method, to be used like this:
AffineTransform userspaceTransform = gradientData->userspaceTransform;
if (isPaintingText) {
AffineTranform additionalTextTransform;
if (shouldTransformResourceOnTextPainting(object, additionalTextTransform))
userspaceTransform.multiply(additionalTextTransform); // Eventually the
multiplication must be reversed, didn't check it!
}
gradientData->gradient->setGradientSpaceTransform(userspaceTransform);
shouldTransformResourceOnTextPainting() shouldn't know anything about the
actual gradient/pattern space transform, it should be dumb and only return the
scaling transform for text.
shouldTransformResourceOnTextPainting() can directly return false, for Cg
platforms, and true for anyone else.
Sounds better, eh?
> LayoutTests/svg/transforms/transformed-text-fill-pattern.html:32
> + <text class="ahemblock" y="50" fill="url(#hatch)">AAAAA</text>
Very clever testcases! I hope this actually works in the wild :-) (aka.
cr-linux EWS that is currently red).
There's another way to exercise the scaling-code path, which is probably easier
to test:
foo.svg;
<svg viewBox="0 0 1 1" width="1000" height="1000">
<text y="0.1" font-size="0.1">ABC</text>
</svg>
foo-expected.svg:
<svg width="1000" height="1000">
<text y="100" font-size="100">ABC</text>
</svg>
Ideally this works with any font, as it should render exactly the same, and
thus is ideal for reftests.
More information about the webkit-reviews
mailing list