[Webkit-unassigned] [Bug 120476] [CSS Masking] Implement luminance masking

[bugzilla-daemon at webkit.org]

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





--- Comment #6 from Dirk Schulze <krit at webkit.org>  2013-09-19 07:25:16 PST ---
(From update of attachment 211014)
View in context: https://bugs.webkit.org/attachment.cgi?id=211014&action=review

Some nits.

> Source/WebCore/ChangeLog:19
> +        * platform/graphics/GeneratorGeneratedImage.cpp: Converted the ImageBuffer to luminance, if necessary.

s/Converted/Convert/

> Source/WebCore/ChangeLog:28
> +        * svg/graphics/SVGImage.cpp: Converted the ImageBuffer to luminance, if necessary.

Ditto.

> Source/WebCore/ChangeLog:30
> +        * svg/graphics/SVGImageForContainer.cpp: Passed the luminance property to the SVG image.

s/Passed/Pass/

> Source/WebCore/platform/graphics/BitmapImage.cpp:508
> +    if (!ctxt->drawLuminanceMask())
> +        Image::drawPattern(ctxt, tileRect, transform, phase, styleColorSpace, op, destRect, blendMode);
> +    else {

please use early return here"

if (!ctxt->drawLuminanceMask()) {
    Image::drawPattern(ctxt, tileRect, transform, phase, styleColorSpace, op, destRect, blendMode);
    return;
}

> Source/WebCore/platform/graphics/BitmapImage.cpp:510
> +            OwnPtr<ImageBuffer> buffer = ImageBuffer::create(expandedIntSize(tileRect.size()), 1);

Why do you expend every image buffer size by 1? Doesn't that cause problems with the offset?

> Source/WebCore/platform/graphics/BitmapImage.cpp:512
> +            if (!buffer)
> +                return;

Can the tileRect be zero size or negative? If not, you may want an assertion here instead. Even though, better to check for tileRect size isEmpty() at the beginning. and return earlier in this case.

> Source/WebCore/platform/graphics/BitmapImage.cpp:520
> +            draw(buffer->context(), tileRect, tileRect, styleColorSpace, op, blendMode);

Not you do not respect the 1px offset because of your expand earlier.

> Source/WebCore/platform/graphics/BitmapImage.h:137
> +        const FloatPoint& phase, ColorSpace styleColorSpace, CompositeOperator, const FloatRect& destRect, BlendMode = BlendModeNormal);

I think you miss a makro OVERRIDE at the end of the definition.

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