[webkit-reviews] review denied: [Bug 90101] [CSS Shaders] Parse mix function : [Attachment 156115] Patch

[bugzilla-daemon at webkit.org]

Dirk Schulze <krit at webkit.org> has denied Max Vujovic <mvujovic at adobe.com>'s
request for review:
Bug 90101: [CSS Shaders] Parse mix function
https://bugs.webkit.org/show_bug.cgi?id=90101

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

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


I am not very happy about the duplication of code that we get. See inline
comments.

> Source/WebCore/css/CSSParser.cpp:7433
> +    case CSSValueClear:
> +    case CSSValueSrc:
> +    case CSSValueDst:
> +    case CSSValueSrcOver:
> +    case CSSValueDstOver:
> +    case CSSValueSrcIn:
> +    case CSSValueDstIn:
> +    case CSSValueSrcOut:
> +    case CSSValueDstOut:
> +    case CSSValueSrcAtop:
> +    case CSSValueDstAtop:
> +    case CSSValueXor:
> +    case CSSValuePlus:
> +	   return true;

The spec changed to take the long version instead of the shorten one.

> Source/WebCore/css/CSSParser.cpp:7503
> +    // blend-mode: normal | multiply | screen | overlay | darken | lighten |
color-dodge |
> +    //	      color-burn | hard-light | soft-light | difference |
exclusion | hue |
> +    //	      saturation | color | luminosity
> +    // alpha-compositing: clear | src | dst | src-over | dst-over | src-in |
dst-in |
> +    //		     src-out | dst-out | src-atop | dst-atop | xor |
plus

comment needs to be updated as well

> Source/WebCore/css/CSSPrimitiveValueMappings.h:3479
> +template<> inline CSSPrimitiveValue::CSSPrimitiveValue(EAlphaCompositingMode
alphaCompositingMode)
> +    : CSSValue(PrimitiveClass)

needs update as well.

> Source/WebCore/css/CSSPrimitiveValueMappings.h:3525
> +template<> inline CSSPrimitiveValue::operator EAlphaCompositingMode() const

This is partly redundant to CSSPrimitiveValue::operator CompositeOperator(). I
would like to avoid that. But I see the difficulty. They are not supported by
Canvas not by background-composite. Can these value still support a subset of
these and it is checked during the parsing of these properties?

> Source/WebCore/css/CSSValueKeywords.in:970
> +src
> +dst
> +src-over
> +dst-over
> +src-in
> +dst-in
> +src-out
> +dst-out
> +src-atop
> +dst-atop
> +// xor
> +plus

Long version of the name please.

> Source/WebCore/platform/graphics/BlendingAndCompositingSettings.h:33
> +#if ENABLE(CSS_SHADERS)

I expect it should work with other features as well. Also we use some of these
values on other places already. Can we make sure that they are used on all
places? I am also not sure if the bundling is that useful. Even if blend and
compositing mode go together, background-compositing just needs
EAlphaCompositing.