[webkit-reviews] review denied: [Bug 119324] [CSS Masking] -webkit-mask-repeat: space does not work : [Attachment 208396] Patch

[bugzilla-daemon at webkit.org]

Dirk Schulze <krit at webkit.org> has denied Andrei Parvu <parvu at adobe.com>'s
request for review:
Bug 119324: [CSS Masking] -webkit-mask-repeat: space does not work
https://bugs.webkit.org/show_bug.cgi?id=119324

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

------- Additional Comments from Dirk Schulze <krit at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=208396&action=review


Great patch! I think we run into a misunderstanding on the use of Pattern. It
would be great if this could be done outside of the platform dependent code, so
that it works on all platforms.

> Source/WebCore/platform/graphics/Pattern.h:63
> +    static PassRefPtr<Pattern> create();

Hm, you create a Pattern without initial data? Don't you alraedy have access to
the Image at that point?

> Source/WebCore/platform/graphics/cg/ImageCG.cpp:126
> +	   matrix = CGAffineTransformTranslate(matrix, 0, size().height() - h);

> +	   RefPtr<Pattern> patternPtr = Pattern::create();
> +	   RetainPtr<CGPatternRef> pattern =
adoptCF(patternPtr->createPlatformPattern(matrix, subImage.get(), tileRect,

I thought more of using Pattern in the platform independent path. Here is does
not make a lot of sense to wrap the Pattern class around the PlatformPattern.
This can not be used by other ports either. I thought more in rendering code,
where we initially call GraphicsContext background drawing routines.

> Source/WebCore/platform/graphics/cg/ImageCG.cpp:143
> +	   CGRect r = CGContextGetClipBoundingBox(context);
> +	   CGContextFillRect(context, r);

Did you initialize the r for testing? You don't need to assign a new variable
here.

> Source/WebCore/platform/graphics/cg/PatternCG.cpp:90
> +CGPatternRef Pattern::createPlatformPattern(const AffineTransform& matrix,
PlatformImagePtr patternImage, FloatRect tileRect, float xStep, float yStep)
const

You may want to pass a FloatSize instead of xStep and yStep.

> Source/WebCore/rendering/RenderBoxModelObject.cpp:1323
> +	   geometry.setSpaceSize(FloatSize(0, 0));

just use FloatSize()

> Source/WebCore/rendering/RenderBoxModelObject.cpp:1367
> +    } else if (backgroundRepeatY == SpaceFill) {
> +	   if (fillTileSize.height() > 0) {

can you combine that a one condition in the else if ?