[webkit-reviews] review denied: [Bug 119614] [CSS Masking]Add -webkit-mask-type property, with alpha and luminance values : [Attachment 208398] 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 119614: [CSS Masking]Add -webkit-mask-type property, with alpha and
luminance values
https://bugs.webkit.org/show_bug.cgi?id=119614

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

------- Additional Comments from Dirk Schulze <krit at webkit.org>
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.