[webkit-reviews] review denied: [Bug 79068] SVG should support transform-origin and relative values : [Attachment 130180] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Mar 5 12:48:12 PST 2012


Simon Fraser (smfr) <simon.fraser at apple.com> has denied Hans Muller
<giles_joplin at yahoo.com>'s request for review:
Bug 79068: SVG should support transform-origin and relative values
https://bugs.webkit.org/show_bug.cgi?id=79068

Attachment 130180: Patch
https://bugs.webkit.org/attachment.cgi?id=130180&action=review

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


r- for the function naming, but the patch seems OK.

> Source/WebCore/platform/Length.h:168
> -    float calcFloatValue(int maxValue) const
> +    float calcFloatValue(float maxValue) const

It's possible that this could change rounding behavior in other parts of the
code. Have you run all the layout tests?

> Source/WebCore/rendering/style/RenderStyle.cpp:774
> +inline bool optimizeForTransformOrigin(const TransformOperations&
transformOperations, RenderStyle::ApplyTransformOrigin applyOrigin)

optimizeForTransformOrigin() doesn't tell me what this function actually does.
It should have a name like requireTransformOrigin().

> Source/WebCore/rendering/style/RenderStyle.cpp:828
> +    
> +

Remove one of these blank lines.

> LayoutTests/svg/transforms/transform-origin-css-property.xhtml:32
> +  You should see 9 green 20x20 rectangles below, each rectangle should be
rotated by 45 degrees.	Each green rectangle
> +  is actually one of a set of about 6, each one with a different but
equivalent value for the CSS transform-origin
> +  property.	Behind each stack of blue rectangles is a single 20x20 red div
that has been specified in in the same way.
> +  The red divs should be completely obscured.
> +  </p>

Please avoid text in tests where pixel testing is important. Differences
between platforms will almost always make it impossible to share pixel results.

This text can go into an HTML comment.

Which blue rectangles?

Can you make the rects bigger to make failures easier to detect?

This could also be a ref test, perhaps.


More information about the webkit-reviews mailing list