[webkit-reviews] review denied: [Bug 27570] [CSS3 Backgrounds and Borders] Add support for the "space" value for background-repeat : [Attachment 101573] 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 101573: Patch
https://bugs.webkit.org/attachment.cgi?id=101573&action=review

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


> Source/WebCore/platform/graphics/GraphicsContext.h:315
> +					  const IntSize& tilePadding,
CompositeOperator, bool useLowQualityScale);

"useLowQualityScale" is unfortunate. It would be great if you pass 
InterpolationQuality directly.
That kind of change should have separate bug though.

> Source/WebCore/platform/graphics/Image.cpp:108
> +{

Is it possible to extend drawTiled() to support spacing instead of defining
another yet similar method?
How about to add a portable path like this on drawTitled(), for call with
non-zero padding?

> Source/WebCore/platform/graphics/Image.cpp:123
> +		       scaledTileSize.height() / intrinsicTileSize.height());

Is this division safe?

> Source/WebCore/rendering/RenderBoxModelObject.cpp:875
> +bool RenderBoxModelObject::calculateFillTilePadding(EFillRepeat repeatX,
EFillRepeat repeatY, const IntSize& containingBox, BackgroundImageGeometry&
geometry)

It looks this method is part of BackgroundImageGeometry?

> Source/WebCore/rendering/RenderBoxModelObject.cpp:985
> +    switch (backgroundRepeatX) {

Can this switch statement be a method of BackgroundImageGeometry?

> Source/WebCore/rendering/RenderBoxModelObject.cpp:998
> +    switch (backgroundRepeatY) {

Ditto.