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

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

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


Great progress. Some more snippets.

> Source/WebCore/ChangeLog:6
> +	   Added the -webkit-mask-type property, which can have a value of
auto, alpha or

mask-source-type.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1436
> +    default:
> +	   return cssValuePool().createValue(CSSValueAuto);
> +    }

Usually we have ASSERT_NOT_REACHED in the default. To make sure we do not
support more than expected.

> Source/WebCore/css/CSSToStyleMap.cpp:323
> +    default:
> +	   type = EMaskSourceType::MaskAuto;
> +    }

Ditto. And still set type of course (to initial value: auto).

> Source/WebCore/rendering/style/FillLayer.h:191
> +    static EMaskSourceType initialMaskSourceType(EFillLayerType) { return
MaskAuto; }

The spec says that mask-image has alpha as default. So even if we need to
support auto for SVG Masks, we already know that it will be alpha for
FillLayers. No need to store auto or return auto. Will affect computed value as
well.

> LayoutTests/fast/masking/parsing-mask-expected.txt:118
> +PASS innerStyle("-webkit-mask-source-type", "") is null

Test for existing but not supported keywords would be great as well.

> LayoutTests/fast/masking/parsing-mask.html:177
> +negativeTest("-webkit-mask-source-type", "");

Ditto.