[webkit-reviews] review granted: [Bug 106226] [CSS Shaders] Implement color and luminosity non-separable blend modes : [Attachment 190084] Patch

[bugzilla-daemon at webkit.org]

Dean Jackson <dino at apple.com> has granted Michelangelo De Simone
<michelangelo at webkit.org>'s request for review:
Bug 106226: [CSS Shaders] Implement color and luminosity non-separable blend
modes
https://bugs.webkit.org/show_bug.cgi?id=106226

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

------- Additional Comments from Dean Jackson <dino at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=190084&action=review


Looks good. Upload a patch with the small changes and I'll cq+.

> Source/WebCore/ChangeLog:10
> +	       - Lum(C): returns the luminosity for the color C
> +	       - ClipColor(C): clips color C
> +	       - SetLum(C, l): sets the luminosity l on the color C

Might as well include the css_ prefix here to make it clear.

> Source/WebCore/platform/graphics/filters/CustomFilterValidatedProgram.cpp:426

> +    if (needsLuminosityHelperFunctions)
> +	   blendFunctionString.append(SHADER(
> +	       mediump float css_Lum(mediump vec3 C)

Since the single statement here spans multiple lines, you should use braces.
"One-line control clauses should not use braces unless comments are included or
a single statement spans multiple lines." -
http://www.webkit.org/coding/coding-style.html

>
LayoutTests/css3/filters/custom/custom-filter-nonseparable-blend-mode-luminosit
y-expected.html:7
> +	   /* There are exactly the color values we expect. Some platforms may
have slight different
> +	      color result. */

You forgot the typo that Max pointed out earlier -> "may have a slightly
different"