Bug 101804 – Extend JavaScript support for blending in canvas

• Status: RESOLVED
• Resolution: FIXED
• Component: Canvas

[Rik Cabanier]

This patch will add JS support to the globalComposite operator

[Rik Cabanier]

Created attachment 173388 [details]
Patch

[Rik Cabanier]

Created attachment 173458 [details]
Fixed testfile

[Dirk Schulze]

Comment on attachment 173458 [details]
Fixed testfile

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.

[Rik Cabanier]

(In reply to comment #3)
> (From update of attachment 173458 [details])
> 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?

Added the fixme. I'm not changing the logic of the webkit specific compositing keyword on images.

> 
> > Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1562
> > +    if (!parseCompositeAndBlendOperator(compositeOperation, op, blendOp) || (blendOp != BlendModeNormal))
> 
> I think you can omit the braces around blendOp != BlendModeNormal

done

> 
> > Source/WebCore/platform/graphics/GraphicsTypes.cpp:46
> >      "darker",
> 
> Add the copy right in this file.

done

> 
> > 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.

done

> 
> > 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.

Added comment

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

done

> 
> > 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.

I think it should always be one method, but it should be extended once we support more than source-over with blending

> 
> > 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.

none of the canvas tests do this. Almost all of them are just 'success'
The ones that don't are testing for a faiure. (I think there's only 1 that has a positive test and is not 'success')

[Rik Cabanier]

Created attachment 177080 [details]
Fixed test file and addressed krit's comments

[Rik Cabanier]

(In reply to comment #3)
> (From update of attachment 173458 [details])
> 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. 

Yes. 
globalCompositeOperator will match the definition of 'mix' and will support both blending and composting for canvas.
see: https://dvcs.w3.org/hg/FXTF/rawfile/tip/compositing/index.html#canvascompositingandblending

[Dirk Schulze]

Comment on attachment 177080 [details]
Fixed test file and addressed krit's comments

LGTM. r=me.

[WebKit Review Bot]

Comment on attachment 177080 [details]
Fixed test file and addressed krit's comments

Clearing flags on attachment: 177080

Committed r136337: <http://trac.webkit.org/changeset/136337>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.