[Webkit-unassigned] [Bug 119614] [CSS Masking]Add -webkit-mask-type property, with alpha and luminance values

[bugzilla-daemon at webkit.org]

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


Dirk Schulze <krit at webkit.org> changed:

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




--- Comment #4 from Dirk Schulze <krit at webkit.org>  2013-08-12 05:41:04 PST ---
(From update of attachment 208398)
View in context: https://bugs.webkit.org/attachment.cgi?id=208398&action=review

Great patch! Still some snippets.

> Source/WebCore/ChangeLog:4
> +        [CSS Masking]Add -webkit-mask-type property, with alpha and luminance values
> +        Added the -webkit-mask-type property, which can have a value of alpha or

First title, then in a new line the link to the bug report, a newline and then the description.

> Source/WebCore/ChangeLog:13
> +        * css/CSSComputedStyleDeclaration.cpp: Added case for CSSPropertyWebkitMaskType

Sentences please. You miss the period.

> Source/WebCore/ChangeLog:15
> +        * css/CSSParser.cpp: Parsed the values for CSSPropertyWebkitMaskType

Ditto, in the other sentences as well. Stop here.

> Source/WebCore/ChangeLog:39
> +        * rendering/style/RenderStyleConstants.h: Added EMaskType
> +        * rendering/style/SVGRenderStyleDefs.h: Removed EMaskType

s/Added EMaskType/Moved EMaskType from SVGRenderStyleDefs.h./

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1789
> +        case CSSPropertyWebkitMaskType: {
> +            const FillLayer* layers = style->maskLayers();
> +
> +            if (!layers)
> +                return cssValuePool().createIdentifierValue(CSSValueNone);
> +
> +            if (!layers->next()) {
> +                if (layers->maskType() == MT_ALPHA)
> +                    return cssValuePool().createIdentifierValue(CSSValueAlpha);
> +                return cssValuePool().createIdentifierValue(CSSValueLuminance);
> +            }
> +
> +            RefPtr<CSSValueList> list = CSSValueList::createCommaSeparated();
> +            for (const FillLayer* currLayer = layers; currLayer; currLayer = currLayer->next()) {
> +                if (currLayer->maskType() == MT_ALPHA)
> +                    list->append(cssValuePool().createIdentifierValue(CSSValueAlpha));
> +                else
> +                    list->append(cssValuePool().createIdentifierValue(CSSValueLuminance));
> +            }
> +            return list.release();
> +        }

This needs testing. If you want to, you can add it to a new patch, since we do not compute the style for -webkit-mask yet. Or you need to add the code for testing as we do for the background property.

> Source/WebCore/css/CSSParser.cpp:4438
> +                case CSSPropertyWebkitMaskType: {
> +                    if (val->id == CSSValueAlpha || val->id == CSSValueLuminance) {
> +                        currValue = cssValuePool().createIdentifierValue(val->id);
> +                        m_valueList->next();
> +                    } else
> +                        currValue = 0;
> +                    break;
> +                }

Hm, so the -webkit-mask property does not support parsing of luminance and alpha yet. Can you open a separate bug report and state that in the ChangeLog explicitly please?

> Source/WebCore/css/CSSToStyleMap.cpp:313
> +    CSSPrimitiveValue* primitiveValue = static_cast<CSSPrimitiveValue*>(value);
> +    layer->setMaskType(*primitiveValue);

I think you want to set the enumeration and not pass the CSSPrimtiveValue.

> Source/WebCore/rendering/style/FillLayer.cpp:57
> +    , m_maskType(MT_ALPHA)

For consistency add a initialFillMaskType function that returns the enumeration.

> Source/WebCore/rendering/style/FillLayer.h:191
> +    static EMaskType initialMaskType(EFillLayerType) { return MT_ALPHA; }

So you already have the initial function here. Why didn't you use it?

> LayoutTests/ChangeLog:10
> +        * fast/masking/parsing-mask-type-expected.txt: Added.
> +        * fast/masking/parsing-mask-type.html: Added.

You can reuse the existing parsing-mask.html test. Just add your -webkit-mask-type tests there.

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