[Webkit-unassigned] [Bug 119324] [CSS Masking] -webkit-mask-repeat: space does not work

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=119324


Dirk Schulze <krit at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #208396|review?                     |review-
               Flag|                            |




--- Comment #12 from Dirk Schulze <krit at webkit.org>  2013-08-09 00:37:32 PST ---
(From update of attachment 208396)
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 ?

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.