[webkit-reviews] review denied: [Bug 119324] [CSS Masking] -webkit-mask-repeat: space does not work : [Attachment 208098] 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 208098: Patch
https://bugs.webkit.org/attachment.cgi?id=208098&action=review

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


r- because of some change request. I didn't check the change in ModelObject
yet.

> Source/WebCore/ChangeLog:6
> +	   Reviewed by NOBODY (OOPS!).

You need to explain what you do, why you do it and why this is the right thing
to archive the right behavior.

> Source/WebCore/ChangeLog:24
> +	   (WebCore::RenderBoxModelObject::paintFillLayerExtended):
> +	   (WebCore::RenderBoxModelObject::calculateBackgroundImageGeometry):

Smaller inline comments can help to understand what you do.

> Source/WebCore/platform/graphics/cg/ImageCG.cpp:136
> +	   // On Leopard and newer, this code now only runs for partially
decoded images whose buffers do not yet match the overall size of the image.
> +	   static const CGPatternCallbacks patternCallbacks = { 0,
drawPatternCallback, 0 };
> +	   CGAffineTransform matrix =
CGAffineTransformMake(narrowPrecisionToCGFloat(patternTransform.a()), 0, 0,
narrowPrecisionToCGFloat(patternTransform.d()), adjustedX, adjustedY);
> +	   matrix = CGAffineTransformConcat(matrix, CGContextGetCTM(context));
> +	   // The top of a partially-decoded image is drawn at the bottom of
the tile. Map it to the top.
> +	   matrix = CGAffineTransformTranslate(matrix, 0, size().height() - h);

> +	   RetainPtr<CGPatternRef> pattern =
adoptCF(CGPatternCreate(subImage.get(), CGRectMake(0, 0, tileRect.width(),
tileRect.height()),
> +	       matrix, tileRect.width() + spaceSize().width(),
tileRect.height() + spaceSize().height(),
> +	       kCGPatternTilingConstantSpacing, true, &patternCallbacks));
> +	   if (!pattern)
> +	       return;
> +

It would be great if you can use the Pattern class already in WebKit to do
that, so that it works in all ports, not just Mac. You may need to add some
more code to Pattern to make that possible. Can you check this first please?
Resetting review flag for now.

> LayoutTests/css3/background/background-repeat-border-expected.html:16
> +	       var urls = Array(), size = Array(), position = Array(), repeat =
Array(),
> +		   origin = Array(), clip = Array();

Does that all need to be JS? Can't you just write a simple test case to make
sure that it works?

> LayoutTests/css3/background/background-repeat-border-expected.html:18
> +	       function addMasks() {

If you are testing background, then mask might be misleading.