[webkit-reviews] review denied: [Bug 71309] Allow SVG elements to be transformed using webkit-transform : [Attachment 114992] Added more tests and allow SVGTextElements to be CSS transformed

[bugzilla-daemon at webkit.org]

Nikolas Zimmermann <zimmermann at kde.org> has denied Raul Hudea
<rhudea at adobe.com>'s request for review:
Bug 71309: Allow SVG elements to be transformed using webkit-transform
https://bugs.webkit.org/show_bug.cgi?id=71309

Attachment 114992: Added more tests and allow SVGTextElements to be CSS
transformed
https://bugs.webkit.org/attachment.cgi?id=114992&action=review

------- Additional Comments from Nikolas Zimmermann <zimmermann at kde.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=114992&action=review


Great job! I'd still have some open issues that currently lead to r-, sorry for
being strict!

> LayoutTests/svg/clip-path/clip-path-css-transform-1.svg:5
> +    <!-- second rect causes masking -->

It's a circle :-)
Excellent that you've added tests for both clipping code paths, once with
buffers and once without.

> LayoutTests/svg/custom/clip-path-with-css-transform.svg:6
> +	   <path style="-webkit-transform: scale(.5)" d="M 0 0 l 200 0 l 0 200
l -200 0 Z"/>

It would be great if you could add the same test again, just like with
transformed <clipPaths> but using two another primitive, like
<rect>/<circle>/etc.. to trigger the image buffer code path.

> LayoutTests/svg/custom/pointer-events-image-css-transform.svg:1
> +<?xml version="1.0"?>

While I don't like that this is copy&paste, it's okay as-is for now, if you
leave a comment that this is 1:1 a copy of an existing test, but using
webkit-transform as only difference, which is what I assume you did :-)

> LayoutTests/svg/custom/pointer-events-text-css-transform.svg:1
> +<?xml version="1.0"?>

Ditto.

> LayoutTests/svg/dom/css-transforms.xhtml:41
> +

Also dump the SVGTransformList here of the rect, and make sure it doesn't
interfere with the -webkit-transform at the moment.
(There are existing tests, I think, in svg/dynamic-updates, that dump
SVGTransformLists, you can check for examples here).
All methods from SVGLocatable interface should be tested and documented here
for both the circle and the rect: getScreenCTM/getTransformToElement/getCTM.

Sorry for demanding so much, but I think it's important to document all corner
cases right from the beginning, so that we can easily see the influence of
future changes in this area.

> Source/WebCore/ChangeLog:13
> +	   Tests: svg/clip-path/clip-path-css-transform-1.svg

The test coverage got much better, thanks for tackling this. 
Another area which should be tested is: dynamic updates. I'd suggest to use
svg/dynamic-updates/SVGTextElement-dom-transform-attr.html as template for
svg/transforms/svg-css-transforms-updates.xhtml. You don't need to use the
script-tests framework as it's done in the svg/dynamic-update tests, instead
you can concatenate the script-tests/SVGTextElement-dom-transform-attr.js and
the html file into one. You also don't need to use any scripting to setup the
document, I just wanted to suggest to create a test in the spirit of that
svg/dynamic-updates test.

What needs testing:
a) setting an inline style attribute of eg. a rect, to a specific
-webkit-transform. Verify repainting worked fine and CSSOM/SVG DOM reflect
those changes.
b) setting the style using CSS OM - test the same.
c) setting the -webkit-transform on a <div>, which contains an inline <svg> in
a HTML5 compound document. Eg. set a scale to 2, and check that the SVG zoomed
correctly.
d) whatever you can also imagine.

This should give basic coverage, if we react to dynamic changes properly.
Sorry again, for demanding all these changes, but I'd like to be certain this
is going to work as-is and not break anything existing.

>> Source/WebCore/svg/SVGStyledTransformableElement.cpp:79
>>	if (m_supplementalTransform)
> 
> One line control clauses should not use braces.  [whitespace/braces] [4]

Please fix the style issue :-)

> Source/WebCore/svg/SVGTextElement.cpp:125
> +	   transform().concatenate(matrix);

Ditto.