[webkit-reviews] review denied: [Bug 82800] Support pattern transformations in canvas : [Attachment 150051] patch v.4

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has denied Huang Dongsung
<luxtella at company100.net>'s request for review:
Bug 82800: Support pattern transformations in canvas
https://bugs.webkit.org/show_bug.cgi?id=82800

Attachment 150051: patch v.4
https://bugs.webkit.org/attachment.cgi?id=150051&action=review

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=150051&action=review


> LayoutTests/fast/canvas/script-tests/pattern-with-settransform.js:53
> +var svgElement = document.createElementNS("http://www.w3.org/2000/svg",
"svg");
> +var matrix = svgElement.createSVGMatrix();

Does new SVGMatrix() not work?

> LayoutTests/fast/canvas/script-tests/pattern-with-settransform.js:55
> +matrix = matrix.translate(20, 20);
> +pattern.setTransform(matrix);

I think you need some more complex tests here. e.g. drawing a transformed
pattern when the context is already transformed. Testing transforms like
rotates and skews. Testing that clipping of such transforms works.

> Source/WebCore/html/canvas/CanvasPattern.cpp:70
> +    m_pattern->setPatternSpaceTransform(static_cast<const
AffineTransform&>(transform));

Why is the cast necessary? SVGMatrix _is_ a AffineTransform.

> Source/WebCore/html/canvas/CanvasPattern.h:52
> +#if ENABLE(SVG)
> +	   void setTransform(const SVGMatrix&);
> +#endif

It's unfortunate that this feature is tied to SVG, but I guess that's how the
spec is.

>> Source/WebCore/svg/SVGMatrix.h:28
>> +#if USE(V8)
>> +// Only used from the bindings.
>> +#include "SVGPropertyTearOff.h"
> 
> It is solution that I found for building with V8.
> 
> Is it Ok? or I need advice, again.

Seems like this should be fixed on the V8 side, not here.