[webkit-reviews] review denied: [Bug 27570] [CSS3 Backgrounds and Borders] Add support for the "space" value for background-repeat : [Attachment 101976] Patch

[bugzilla-daemon at webkit.org]

MORITA Hajime <morrita at google.com> has denied Chiculita Alexandru
<achicu at adobe.com>'s request for review:
Bug 27570: [CSS3 Backgrounds and Borders] Add support for the "space" value for
background-repeat
https://bugs.webkit.org/show_bug.cgi?id=27570

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

------- Additional Comments from MORITA Hajime <morrita at google.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=101976&action=review


The code looks almost fine to me.
Could you add some more tests to cover edge cases like: 
- for RoundFill, the case for 
- Different repeat-x repeat-y combinations
- "background-position", which should be ignored (according to the spec.)
- A background area which is smaller than  the background image
- A background area which is as small as that the image cannot repeat twice.
- background-origin: border-box
Some of them looks OK to get them together.

> LayoutTests/fast/css/background-repeat-round.html:10
> +    }

Could you make it clear that the image scaling happens?

> Source/WebCore/platform/graphics/Image.cpp:151
> +

ceilf()?

> Source/WebCore/platform/graphics/Image.cpp:158
> +

Could you use FloatRect constructor to set quad values instead of setting it
individually?

> Source/WebCore/platform/graphics/Image.cpp:174
> +		       visibleSrcRect.setHeight(clippedRect.height() /
scale.height());

Use FloatRect constructor?

> Source/WebCore/rendering/RenderBoxModelObject.cpp:1005
> +	   return;

Is it OK to return here?