[webkit-reviews] review denied: [Bug 98504] [CSS Shaders] Implement overlay, color-dodge, color-burn, hard-light, soft-light blend modes. : [Attachment 168290] Patch

[bugzilla-daemon at webkit.org]

Dean Jackson <dino at apple.com> has denied Huang Dongsung
<luxtella at company100.net>'s request for review:
Bug 98504: [CSS Shaders] Implement overlay, color-dodge, color-burn,
hard-light, soft-light blend modes.
https://bugs.webkit.org/show_bug.cgi?id=98504

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

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


I think this looks great, but I have one minor concern that I'd like Max's
feedback on: we're now doing more work on the simple compositing operations -
calling per-component functions. I wonder if it would be worth only dropping to
css_BlendComponent if necessary. 

Hmm... I see that Max did in fact make this suggestion "For the simpler blend
modes that don't need to blend by component, we will overwrite this default
value."

In that case I will r-. It should be pretty easy to add this.

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

>      // Cs: is the source color
>      // Cb: is the backdrop color

This comment is now incorrect. These are color components (although see general
comment on the patch)

> LayoutTests/css3/filters/custom/custom-filter-blend-modes-expected.html:54
> +    .hard-light-expected {
> +	   background-color: rgb(0%, 44%, 50%);
> +    }

Is there a reason why you used % here but numbers elsewhere?

> LayoutTests/css3/filters/custom/custom-filter-blend-modes.html:213
> +    .color-burn-expected {
> +	   /*
> +	       ColorBurn:
> +
> +	       Co = if(Cs > 0)
> +			1 - min(1, (1 - Cb) / Cs)
> +		    else
> +			0
> +
> +	       r = 0
> +	       g = 1 - min(1, (1 - 0.3) / 0.6) = 0
> +	       b = 1 - min(1, (1 - 0.5) / 0.5) = 0
> +	   */
> +	   background-color: rgb(0, 0, 1);

I'm not sure why this is rgb(0, 0, 1) when you calculated 0, 0, 0.

Also, this shows that we may need a more complex test for burn, since a
completely black result doesn't really exercise much.