[webkit-reviews] review denied: [Bug 101804] Extend JavaScript support for blending in canvas : [Attachment 173458] Fixed testfile

[bugzilla-daemon at webkit.org]

Dirk Schulze <krit at webkit.org> has denied Rik Cabanier <cabanier at adobe.com>'s
request for review:
Bug 101804: Extend JavaScript support for blending in canvas
https://bugs.webkit.org/show_bug.cgi?id=101804

Attachment 173458: Fixed testfile
https://bugs.webkit.org/attachment.cgi?id=173458&action=review

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


A general question. Did we agree on the mailing list to just support one entry
on "globalCompositeOperator" for now? I think so, but am not sure. The patch
looks great in general. Just some snippets.

> Source/WebCore/html/HTMLImageElement.cpp:180
> +	   if (!parseCompositeAndBlendOperator(attribute.value(),
m_compositeOperator, blendOp))

why isn't blenOp a member variable as well? Do we may add this here in the
future? If so, can you add a FIXME here?

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1562
> +    if (!parseCompositeAndBlendOperator(compositeOperation, op, blendOp) ||
(blendOp != BlendModeNormal))

I think you can omit the braces around blendOp != BlendModeNormal

> Source/WebCore/platform/graphics/GraphicsTypes.cpp:46
>      "darker",

Add the copy right in this file.

> Source/WebCore/platform/graphics/GraphicsTypes.cpp:73
>      for (int i = 0; i < numCompositeOperatorNames; i++)
>	   if (s == compositeOperatorNames[i]) {

braces around multi line loops and conditions.

> Source/WebCore/platform/graphics/GraphicsTypes.cpp:77
> +	       blendOp = BlendModeNormal;
> +	       return true;
> +	   }

If the plans was to support both types together in the future, you should maybe
add a comment that explains it. Otherwise there might be questions why not put
it in the same array.

> Source/WebCore/platform/graphics/GraphicsTypes.cpp:79
> +    for (int i = 0; i < numBlendOperatorNames; i++)
> +	   if (s == blendOperatorNames[i]) {

braces.

> Source/WebCore/platform/graphics/GraphicsTypes.h:85
> +    String compositeOperatorName(CompositeOperator, BlendMode);

Does it make sense to add two methods, one for compositor and one for the blend
mode? I guess you want to optimize it for now, where we have all modes in one
list.

> LayoutTests/canvas/philip/tests/2d.composite.globalComposite.html:18
> +_addTest(function(canvas, ctx) {
> +var compModes = [ "clear", "copy", "source-over", "destination-over",
"source-in", "destination-in", "source-out", "destination-out", "source-atop",
"destination-atop", "xor", "lighter", "multiply", "screen", "overlay",
"darken", "lighten", "color-dodge", "color-burn", "hard-light", "soft-light",
"difference", "exclusion", "hue", "saturation", "color", "luminosity"];
> +for (var i = 0; i < compModes.length; i++) {
> +    ctx.globalCompositeOperation = compModes[i];
> +    _assertEqual(ctx.globalCompositeOperation, compModes[i],
"ctx.globalAlpha", "compModes[i]");
> +}

I like tests that state all tested modes in the expectation file. This makes it
easier to debug failures and to see changes on blend modes and compositing
modes (like name changes) faster. Can you add these kind of tests? Please use
the scripts in fast/resources for that.