Bug 100070 – Add canvas blending modes using Core Graphics

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

[Rik Cabanier]

This bug will add the canvas blending modes using CG

[Rik Cabanier]

Created attachment 177626 [details]
First try. Not ready for review

[Early Warning System Bot]

Comment on attachment 177626 [details]
First try. Not ready for review

Attachment 177626 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/15126816

[Early Warning System Bot]

Comment on attachment 177626 [details]
First try. Not ready for review

Attachment 177626 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/15126824

[WebKit Review Bot]

Comment on attachment 177626 [details]
First try. Not ready for review

Attachment 177626 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15132679

[Rik Cabanier]

Created attachment 177660 [details]
not for review

[Early Warning System Bot]

Comment on attachment 177660 [details]
not for review

Attachment 177660 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/15147313

[Early Warning System Bot]

Comment on attachment 177660 [details]
not for review

Attachment 177660 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/15133696

[Rik Cabanier]

Created attachment 177662 [details]
not for review

[WebKit Review Bot]

Attachment 177662 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/canv..." exit_code: 1
Source/WebCore/platform/graphics/qt/ImageBufferQt.cpp:129:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 1 in 182 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Early Warning System Bot]

Comment on attachment 177662 [details]
not for review

Attachment 177662 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/15138464

[Early Warning System Bot]

Comment on attachment 177662 [details]
not for review

Attachment 177662 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/15151329

[Rik Cabanier]

Created attachment 177665 [details]
not for review

[Rik Cabanier]

Created attachment 177668 [details]
not for review

[Peter Beverloo (cr-android ews)]

Comment on attachment 177668 [details]
not for review

Attachment 177668 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/15151364

[Build Bot]

Comment on attachment 177668 [details]
not for review

Attachment 177668 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/15154267

[WebKit Review Bot]

Comment on attachment 177668 [details]
not for review

Attachment 177668 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15154274

[EFL EWS Bot]

Comment on attachment 177668 [details]
not for review

Attachment 177668 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/15154307

[Rik Cabanier]

Created attachment 178334 [details]
Patch

[Rik Cabanier]

Created attachment 178335 [details]
Added QT testexpectations file

[Dirk Schulze]

Comment on attachment 178335 [details]
Added QT testexpectations file

View in context: https://bugs.webkit.org/attachment.cgi?id=178335&action=review

> Source/WebCore/ChangeLog:14
> +               canvas/philip/tests/2d.composite.canvas.darken.html

Please don't add new tests to the test suite from Philip, as long as they are not official part of the suite.

> Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp:1755
> +        } else

Alignment problem + missing brace.

> LayoutTests/canvas/philip/tests/2d.composite.canvas.color-burn.html:25
> +_assertPixelApprox(canvas, 50,25, 110,255,37,223, "50,25", "110,255,37,223", 5);

We actually have our own test suite for this. Can you take a look in this instead? Makes it easier to maintain. Or you start writing W3C Testharness.js tests.

> LayoutTests/canvas/philip/tests/2d.composite.canvas.color-expected.txt:1
> +Passed

This is not enough on information. It should have a description what the test is testing. See other tests outside the test suite from Philip.

[Rik Cabanier]

(In reply to comment #20)
> (From update of attachment 178335 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=178335&action=review
> 
> > Source/WebCore/ChangeLog:14
> > +               canvas/philip/tests/2d.composite.canvas.darken.html
> 
> Please don't add new tests to the test suite from Philip, as long as they are not official part of the suite.

How do I add tests to his test suite? Can I contact him?

> 
> > Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp:1755
> > +        } else
> 
> Alignment problem + missing brace.
> 
> > LayoutTests/canvas/philip/tests/2d.composite.canvas.color-burn.html:25
> > +_assertPixelApprox(canvas, 50,25, 110,255,37,223, "50,25", "110,255,37,223", 5);
> 
> We actually have our own test suite for this. Can you take a look in this instead? Makes it easier to maintain. Or you start writing W3C Testharness.js tests.
> 
> > LayoutTests/canvas/philip/tests/2d.composite.canvas.color-expected.txt:1
> > +Passed
> 
> This is not enough on information. It should have a description what the test is testing. See other tests outside the test suite from Philip.

[Dirk Schulze]

(In reply to comment #21)
> (In reply to comment #20)
> > (From update of attachment 178335 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=178335&action=review
> > 
> > > Source/WebCore/ChangeLog:14
> > > +               canvas/philip/tests/2d.composite.canvas.darken.html
> > 
> > Please don't add new tests to the test suite from Philip, as long as they are not official part of the suite.
> 
> How do I add tests to his test suite? Can I contact him?
> 
> > 
> > > Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp:1755
> > > +        } else
> > 
> > Alignment problem + missing brace.
> > 
> > > LayoutTests/canvas/philip/tests/2d.composite.canvas.color-burn.html:25
> > > +_assertPixelApprox(canvas, 50,25, 110,255,37,223, "50,25", "110,255,37,223", 5);
> > 
> > We actually have our own test suite for this. Can you take a look in this instead? Makes it easier to maintain. Or you start writing W3C Testharness.js tests.
> > 
> > > LayoutTests/canvas/philip/tests/2d.composite.canvas.color-expected.txt:1
> > > +Passed
> > 
> > This is not enough on information. It should have a description what the test is testing. See other tests outside the test suite from Philip.

See: http://philip.html5.org/tests/canvas/suite/tests/

But the test suite is very outdated and needs a detailed review. IIRC a lot of tests in our repo are either modified or are expected to fail because of wrong tests. Maybe you can ask him to assist on updating the test suite directly.

[Rik Cabanier]

Created attachment 178693 [details]
Updated bug per Dirk's comments

[Rik Cabanier]

(In reply to comment #22)
> (In reply to comment #21)
> > (In reply to comment #20)
> > > (From update of attachment 178335 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=178335&action=review
> > > 
> > > > Source/WebCore/ChangeLog:14
> > > > +               canvas/philip/tests/2d.composite.canvas.darken.html
> > > 
> > > Please don't add new tests to the test suite from Philip, as long as they are not official part of the suite.
> > 
> > How do I add tests to his test suite? Can I contact him?
> > 
> > > 
> > > > Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp:1755
> > > > +        } else
> > > 
> > > Alignment problem + missing brace.
> > > 
> > > > LayoutTests/canvas/philip/tests/2d.composite.canvas.color-burn.html:25
> > > > +_assertPixelApprox(canvas, 50,25, 110,255,37,223, "50,25", "110,255,37,223", 5);
> > > 
> > > We actually have our own test suite for this. Can you take a look in this instead? Makes it easier to maintain. Or you start writing W3C Testharness.js tests.
> > > 
> > > > LayoutTests/canvas/philip/tests/2d.composite.canvas.color-expected.txt:1
> > > > +Passed
> > > 
> > > This is not enough on information. It should have a description what the test is testing. See other tests outside the test suite from Philip.
> 
> See: http://philip.html5.org/tests/canvas/suite/tests/
> 
> But the test suite is very outdated and needs a detailed review. IIRC a lot of tests in our repo are either modified or are expected to fail because of wrong tests. Maybe you can ask him to assist on updating the test suite directly.

I sent him an email a couple of days ago but didn't receive a reply yet.
Until then, I followed your advice and created new test files that use the existing framework. The tests also test all the blend modes (so no test per blend mode).

[Dirk Schulze]

Comment on attachment 178693 [details]
Updated bug per Dirk's comments

View in context: https://bugs.webkit.org/attachment.cgi?id=178693&action=review

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:628
> +    c->setCompositeOperation(op, blendOp);

this is inconsistent, later you use blendMode. At some point we may want to rename 'op' to something meaningful.

> Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp:1755
> +        } else {

wrong indentation.

> LayoutTests/canvas/canvas.blendmode.canvas.html:14
> +    }
> +	
> +	function _getPixel(canvas, x,y)
> +	{
> +	   var ctx = canvas.getContext('2d');

Please avoid using tabs!

> LayoutTests/canvas/canvas.blendmode.canvas.html:20
> +	function _assertPixelApprox(canvas, x,y, color, tolerance, bmode)

looks like you copy / pasted the assertion code from philips test suite. Actually not the proper way. We have tests that do the right thing already. I'll need to take a look at the scripts tomorrow.

[Rik Cabanier]

Created attachment 179508 [details]
fixed per Dirk's comments

[WebKit Review Bot]

Comment on attachment 179508 [details]
fixed per Dirk's comments

Attachment 179508 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15316818

[Peter Beverloo (cr-android ews)]

Comment on attachment 179508 [details]
fixed per Dirk's comments

Attachment 179508 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/15321750

[Build Bot]

Comment on attachment 179508 [details]
fixed per Dirk's comments

Attachment 179508 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15323768

[Build Bot]

Comment on attachment 179508 [details]
fixed per Dirk's comments

Attachment 179508 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/15318756

[Early Warning System Bot]

Comment on attachment 179508 [details]
fixed per Dirk's comments

Attachment 179508 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/15310818

[Rik Cabanier]

Created attachment 179523 [details]
Patch

[Rik Cabanier]

(In reply to comment #25)
> (From update of attachment 178693 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=178693&action=review
> 
> > Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:628
> > +    c->setCompositeOperation(op, blendOp);
> 
> this is inconsistent, later you use blendMode. At some point we may want to rename 'op' to something meaningful.
> 
> > Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp:1755
> > +        } else {
> 
> wrong indentation.
> 
> > LayoutTests/canvas/canvas.blendmode.canvas.html:14
> > +    }
> > +	
> > +	function _getPixel(canvas, x,y)
> > +	{
> > +	   var ctx = canvas.getContext('2d');
> 
> Please avoid using tabs!
> 
> > LayoutTests/canvas/canvas.blendmode.canvas.html:20
> > +	function _assertPixelApprox(canvas, x,y, color, tolerance, bmode)
> 
> looks like you copy / pasted the assertion code from philips test suite. Actually not the proper way. We have tests that do the right thing already. I'll need to take a look at the scripts tomorrow.

Thanks for pointing me to the other canvas test.
I've added 3 tests that test all the blend modes.

[WebKit Review Bot]

Comment on attachment 179523 [details]
Patch

Attachment 179523 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15316849

New failing tests:
WebCompositorInputHandlerImplTest.lastInputEventForVSync
WebCompositorInputHandlerImplTest.gestureFlingIgnoredTouchpad
WebCompositorInputHandlerImplTest.gestureFlingTransferResetsTouchpad
platform/chromium/virtual/gpu/fast/canvas/canvas-blend-canvas.html
platform/chromium/virtual/gpu/fast/canvas/canvas-blend.html
platform/chromium/virtual/gpu/fast/canvas/canvas-blend-image.html
WebCompositorInputHandlerImplTest.gestureFlingAnimatesTouchpad
WebCompositorInputHandlerImplTest.gestureFlingIgnoredTouchscreen

[Rik Cabanier]

Created attachment 179535 [details]
fixed chromium testexpectations

[Rik Cabanier]

Created attachment 179872 [details]
Patch

[Rik Cabanier]

Created attachment 179874 [details]
Patch

[Rik Cabanier]

(In reply to comment #37)
> Created an attachment (id=179874) [details]
> Patch

This is the code to generate the dataset:
function createArray() {
		debug('[');
        for (var i = 0; i < blendModes.length; i++) {
                debug('  [\'' + blendModes[i][0] + '\',');
				var s = '';
                for (var j = 0; j < testScenario.length; j++) {
                        ctx.clearRect(0,0,200,200);
                        ctx.save();

                        // Draw backdrop.
                        ctx.fillStyle = 'rgba(0, 0, 255, ' + testScenario[j][1] + ')';
                        ctx.fillRect(0,0,200,200);

                        // Apply blend mode.
                        ctx.globalCompositeOperation = blendModes[i][0];
                        ctx.globalAlpha = testScenario[j][2];
                        ctx.fillStyle = "red";
						ctx.fillRect(0,0,100,100);
						ctx.fillStyle = "yellow";
						ctx.fillRect(100,0,100,100);
						ctx.fillStyle = "green";
						ctx.fillRect(100,100,100,100);
						ctx.fillStyle = "blue";
						ctx.fillRect(0,100,100,100);
						ctx.restore();
						s += '         [';
						for (var k = 0; k < testPoints.length; k++) {
                			var resultColor = ctx.getImageData(testPoints[k].x, testPoints[k].y , 1, 1).data;
							s += '[' + resultColor[0] + ', ' + resultColor[1] + ', ' + resultColor[2] + ', ' + resultColor[3] + '],'
						}
						s += '],\n';
				}
				debug(s);
				debug('  ],');
		}
		debug(']');
}

[Dirk Schulze]

Comment on attachment 179872 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=179872&action=review

> Source/WebCore/ChangeLog:21
> +        * html/canvas/CanvasRenderingContext2D.cpp:
> +        (WebCore::CanvasRenderingContext2D::setGlobalCompositeOperation):
> +        (WebCore::CanvasRenderingContext2D::drawImage):
> +        (WebCore::CanvasRenderingContext2D::drawImageFromRect):
> +        * html/canvas/CanvasRenderingContext2D.h:
> +        (CanvasRenderingContext2D):
> +        * platform/graphics/cg/GraphicsContextCG.cpp:

line by line comments for such a fundamental feature: "Added BlendMode argument to drawImage to pass blend mode.", "Added new switch for blend modes and appendant CG call." and so on.

> LayoutTests/fast/canvas/canvas-blend-image-expected.txt:4
> +On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
> +
> +
> +FAIL myGetImageData(0, 'color-dodge')[0] should be within 5 of 0. Was 255.

The description is mission. Can you elaborate why it is not here? Same for the other descriptions.

> LayoutTests/fast/canvas/canvas-blend-solid-expected.txt:76
> +FAIL myGetImageData(2, 'saturation')[1] should be within 5 of 46. Was 85.
> +FAIL myGetImageData(2, 'saturation')[2] should be within 5 of 130. Was 84.
> +FAIL myGetImageData(0, 'color')[0] should be within 5 of 93. Was 255.
> +FAIL myGetImageData(1, 'color')[0] should be within 5 of 31. Was 255.

Why are there so many luminocity and saturation tests but not so much on others? A lot of blend modes seem to be missing.

> LayoutTests/fast/canvas/canvas-blend.html:5
> +    <meta name="DC.creator" content="Rik Cabanier, [e-mail]">
> +    <meta name="DC.publisher" content="Adobe Systems, www.adobe.com">

This is unusual and not done in WebKit if the test is written for WebKit.

> LayoutTests/fast/canvas/canvas-blend.html:19
> +      if (window.testRunner)
> +	    testRunner.dumpAsText(true);
> +
> +      var compositeTypes = [
> +        'source-over','multiply', 'screen', 'overlay', 'darken',
> +        'lighten', 'color-dodge', 'color-burn', 'hard-light', 'soft-light',
> +        'difference', 'exclusion', 'hue', 'saturation', 'color', 'luminosity'
> +      ];
> +      function draw(){
> +        for (i=0;i<compositeTypes.length;i++){
> +          var label = document.createTextNode(compositeTypes[i]);
> +          document.getElementById('lab'+i).appendChild(label);
> +          var ctx = document.getElementById('tut'+i).getContext('2d');

This test is not testing anything meaningful. With dumpAsText it is actually not testing anything at all. Can you remove it?

> LayoutTests/fast/canvas/script-tests/canvas-blend-image.js:1
> +description("Series of tests to ensure correct results on applying different blend modes.");

Still wonder why this is not showing up in the expectation files.

> LayoutTests/fast/canvas/script-tests/canvas-blend-image.js:144
> +var testScenario = [
> +        ['solid on solid', 1, 1],
> +        ['solid on alpha', 1, 0.5],
> +        ['alpha on solid', 0.5, 1],
> +        ['alpha on alpha', 0.5, 0.5]

Can you replace all tabs with spaces please?

> LayoutTests/fast/canvas/script-tests/canvas-blend-image.js:149
> +function myGetImageData(i, blend)

Please rename this function into e.g. pixelDataAtPoint

> LayoutTests/fast/canvas/script-tests/canvas-blend-image.js:157
> +                var resultColor = "myGetImageData(" + i + ", '" + blendMode + "')";

blendMode is actually not needed if the descriptors work. Don't duplicate output but investigate in the description() function please.

> LayoutTests/fast/canvas/script-tests/canvas-blend-solid.js:185
> +function createArray() {

Can you remove this from the patch? You can add it to the bug report as an extra file. Once created, it should always work. No treats needed.

> LayoutTests/fast/canvas/script-tests/canvas-blend-solid.js:225
> +//createArray();

No commented out code in patches.

> LayoutTests/platform/chromium/TestExpectations:133
> +webkit.org/b/100071 fast/canvas/canvas-blend-image.html [ Skip ]
> +webkit.org/b/100071 fast/canvas/canvas-blend-solid.html [ Skip ]
> +webkit.org/b/100071 fast/canvas/canvas-blend.html [ Skip ]

You are submitting with failing test results. Do you actually need to skip tests? If so, why?

> LayoutTests/platform/qt/TestExpectations:1191
> +# https://bugs.webkit.org/show_bug.cgi?id=100072

Why just chromium and qt? What about the other bots? mac, gtk, efl and so on.

[Dirk Schulze]

Comment on attachment 179872 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=179872&action=review

> Source/WebCore/ChangeLog:21
> +        * html/canvas/CanvasRenderingContext2D.cpp:
> +        (WebCore::CanvasRenderingContext2D::setGlobalCompositeOperation):
> +        (WebCore::CanvasRenderingContext2D::drawImage):
> +        (WebCore::CanvasRenderingContext2D::drawImageFromRect):
> +        * html/canvas/CanvasRenderingContext2D.h:
> +        (CanvasRenderingContext2D):
> +        * platform/graphics/cg/GraphicsContextCG.cpp:

line by line comments for such a fundamental feature: "Added BlendMode argument to drawImage to pass blend mode.", "Added new switch for blend modes and appendant CG call." and so on.

> LayoutTests/fast/canvas/canvas-blend-image-expected.txt:4
> +On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
> +
> +
> +FAIL myGetImageData(0, 'color-dodge')[0] should be within 5 of 0. Was 255.

The description is mission. Can you elaborate why it is not here? Same for the other descriptions.

> LayoutTests/fast/canvas/canvas-blend-solid-expected.txt:76
> +FAIL myGetImageData(2, 'saturation')[1] should be within 5 of 46. Was 85.
> +FAIL myGetImageData(2, 'saturation')[2] should be within 5 of 130. Was 84.
> +FAIL myGetImageData(0, 'color')[0] should be within 5 of 93. Was 255.
> +FAIL myGetImageData(1, 'color')[0] should be within 5 of 31. Was 255.

Why are there so many luminocity and saturation tests but not so much on others? A lot of blend modes seem to be missing.

> LayoutTests/fast/canvas/canvas-blend.html:5
> +    <meta name="DC.creator" content="Rik Cabanier, [e-mail]">
> +    <meta name="DC.publisher" content="Adobe Systems, www.adobe.com">

This is unusual and not done in WebKit if the test is written for WebKit.

> LayoutTests/fast/canvas/canvas-blend.html:19
> +      if (window.testRunner)
> +	    testRunner.dumpAsText(true);
> +
> +      var compositeTypes = [
> +        'source-over','multiply', 'screen', 'overlay', 'darken',
> +        'lighten', 'color-dodge', 'color-burn', 'hard-light', 'soft-light',
> +        'difference', 'exclusion', 'hue', 'saturation', 'color', 'luminosity'
> +      ];
> +      function draw(){
> +        for (i=0;i<compositeTypes.length;i++){
> +          var label = document.createTextNode(compositeTypes[i]);
> +          document.getElementById('lab'+i).appendChild(label);
> +          var ctx = document.getElementById('tut'+i).getContext('2d');

This test is not testing anything meaningful. With dumpAsText it is actually not testing anything at all. Can you remove it?

> LayoutTests/fast/canvas/script-tests/canvas-blend-image.js:1
> +description("Series of tests to ensure correct results on applying different blend modes.");

Still wonder why this is not showing up in the expectation files.

> LayoutTests/fast/canvas/script-tests/canvas-blend-image.js:144
> +var testScenario = [
> +        ['solid on solid', 1, 1],
> +        ['solid on alpha', 1, 0.5],
> +        ['alpha on solid', 0.5, 1],
> +        ['alpha on alpha', 0.5, 0.5]

Can you replace all tabs with spaces please?

> LayoutTests/fast/canvas/script-tests/canvas-blend-image.js:149
> +function myGetImageData(i, blend)

Please rename this function into e.g. pixelDataAtPoint

> LayoutTests/fast/canvas/script-tests/canvas-blend-image.js:157
> +                var resultColor = "myGetImageData(" + i + ", '" + blendMode + "')";

blendMode is actually not needed if the descriptors work. Don't duplicate output but investigate in the description() function please.

> LayoutTests/fast/canvas/script-tests/canvas-blend-solid.js:185
> +function createArray() {

Can you remove this from the patch? You can add it to the bug report as an extra file. Once created, it should always work. No treats needed.

> LayoutTests/fast/canvas/script-tests/canvas-blend-solid.js:225
> +//createArray();

No commented out code in patches.

> LayoutTests/platform/chromium/TestExpectations:133
> +webkit.org/b/100071 fast/canvas/canvas-blend-image.html [ Skip ]
> +webkit.org/b/100071 fast/canvas/canvas-blend-solid.html [ Skip ]
> +webkit.org/b/100071 fast/canvas/canvas-blend.html [ Skip ]

You are submitting with failing test results. Do you actually need to skip tests? If so, why?

> LayoutTests/platform/qt/TestExpectations:1191
> +# https://bugs.webkit.org/show_bug.cgi?id=100072

Why just chromium and qt? What about the other bots? mac, gtk, efl and so on.

[Dirk Schulze]

Comment on attachment 179874 [details]
Patch

See previous comments. r-

[Rik Cabanier]

(In reply to comment #40)
> (From update of attachment 179872 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=179872&action=review
> 
> > Source/WebCore/ChangeLog:21
> > +        * html/canvas/CanvasRenderingContext2D.cpp:
> > +        (WebCore::CanvasRenderingContext2D::setGlobalCompositeOperation):
> > +        (WebCore::CanvasRenderingContext2D::drawImage):
> > +        (WebCore::CanvasRenderingContext2D::drawImageFromRect):
> > +        * html/canvas/CanvasRenderingContext2D.h:
> > +        (CanvasRenderingContext2D):
> > +        * platform/graphics/cg/GraphicsContextCG.cpp:
> 
> line by line comments for such a fundamental feature: "Added BlendMode argument to drawImage to pass blend mode.", "Added new switch for blend modes and appendant CG call." and so on.

Why did this suddenly become an issue? Will fix

> 
> > LayoutTests/fast/canvas/canvas-blend-image-expected.txt:4
> > +On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
> > +
> > +
> > +FAIL myGetImageData(0, 'color-dodge')[0] should be within 5 of 0. Was 255.
> 
> The description is mission. Can you elaborate why it is not here? Same for the other descriptions.

This is how shouldBeCloseTo works. Should I not use that function, but write my own instead?

> 
> > LayoutTests/fast/canvas/canvas-blend-solid-expected.txt:76
> > +FAIL myGetImageData(2, 'saturation')[1] should be within 5 of 46. Was 85.
> > +FAIL myGetImageData(2, 'saturation')[2] should be within 5 of 130. Was 84.
> > +FAIL myGetImageData(0, 'color')[0] should be within 5 of 93. Was 255.
> > +FAIL myGetImageData(1, 'color')[0] should be within 5 of 31. Was 255.
> 
> Why are there so many luminocity and saturation tests but not so much on others? A lot of blend modes seem to be missing.

I don't understand. Every blend mode is tested.
Did you review the latest patch?

> 
> > LayoutTests/fast/canvas/canvas-blend.html:5
> > +    <meta name="DC.creator" content="Rik Cabanier, [e-mail]">
> > +    <meta name="DC.publisher" content="Adobe Systems, www.adobe.com">
> 
> This is unusual and not done in WebKit if the test is written for WebKit.
> 
> > LayoutTests/fast/canvas/canvas-blend.html:19
> > +      if (window.testRunner)
> > +	    testRunner.dumpAsText(true);
> > +
> > +      var compositeTypes = [
> > +        'source-over','multiply', 'screen', 'overlay', 'darken',
> > +        'lighten', 'color-dodge', 'color-burn', 'hard-light', 'soft-light',
> > +        'difference', 'exclusion', 'hue', 'saturation', 'color', 'luminosity'
> > +      ];
> > +      function draw(){
> > +        for (i=0;i<compositeTypes.length;i++){
> > +          var label = document.createTextNode(compositeTypes[i]);
> > +          document.getElementById('lab'+i).appendChild(label);
> > +          var ctx = document.getElementById('tut'+i).getContext('2d');
> 
> This test is not testing anything meaningful. With dumpAsText it is actually not testing anything at all. Can you remove it?

OK. Why are the compositing modes tested that way?

> 
> > LayoutTests/fast/canvas/script-tests/canvas-blend-image.js:1
> > +description("Series of tests to ensure correct results on applying different blend modes.");
> 
> Still wonder why this is not showing up in the expectation files.

description() is continually updated.
If you want to see it in the output, a different function is needed.

> 
> > LayoutTests/fast/canvas/script-tests/canvas-blend-image.js:144
> > +var testScenario = [
> > +        ['solid on solid', 1, 1],
> > +        ['solid on alpha', 1, 0.5],
> > +        ['alpha on solid', 0.5, 1],
> > +        ['alpha on alpha', 0.5, 0.5]
> 
> Can you replace all tabs with spaces please?

OK

> 
> > LayoutTests/fast/canvas/script-tests/canvas-blend-image.js:149
> > +function myGetImageData(i, blend)
> 
> Please rename this function into e.g. pixelDataAtPoint

OK

> 
> > LayoutTests/fast/canvas/script-tests/canvas-blend-image.js:157
> > +                var resultColor = "myGetImageData(" + i + ", '" + blendMode + "')";
> 
> blendMode is actually not needed if the descriptors work. Don't duplicate output but investigate in the description() function please.
> 
> > LayoutTests/fast/canvas/script-tests/canvas-blend-solid.js:185
> > +function createArray() {
> 
> Can you remove this from the patch? You can add it to the bug report as an extra file. Once created, it should always work. No treats needed.
> 
> > LayoutTests/fast/canvas/script-tests/canvas-blend-solid.js:225
> > +//createArray();
> 
> No commented out code in patches.

Patch that you r-'ed did not have this

> 
> > LayoutTests/platform/chromium/TestExpectations:133
> > +webkit.org/b/100071 fast/canvas/canvas-blend-image.html [ Skip ]
> > +webkit.org/b/100071 fast/canvas/canvas-blend-solid.html [ Skip ]
> > +webkit.org/b/100071 fast/canvas/canvas-blend.html [ Skip ]
> 
> You are submitting with failing test results. Do you actually need to skip tests? If so, why?

As far as I can tell, this is how it is done with other missing functionality.
What else should happen?

> 
> > LayoutTests/platform/qt/TestExpectations:1191
> > +# https://bugs.webkit.org/show_bug.cgi?id=100072
> 
> Why just chromium and qt? What about the other bots? mac, gtk, efl and so on.

[Dirk Schulze]

Comment on attachment 179872 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=179872&action=review

>>>> Source/WebCore/ChangeLog:21
>>>> +        * platform/graphics/cg/GraphicsContextCG.cpp:
>>> 
>>> line by line comments for such a fundamental feature: "Added BlendMode argument to drawImage to pass blend mode.", "Added new switch for blend modes and appendant CG call." and so on.
>> 
>> line by line comments for such a fundamental feature: "Added BlendMode argument to drawImage to pass blend mode.", "Added new switch for blend modes and appendant CG call." and so on.
> 
> Why did this suddenly become an issue? Will fix

With every review I find more issues that I didn't see in previous review :)

>>>> LayoutTests/fast/canvas/canvas-blend-image-expected.txt:4
>>>> +FAIL myGetImageData(0, 'color-dodge')[0] should be within 5 of 0. Was 255.
>>> 
>>> The description is mission. Can you elaborate why it is not here? Same for the other descriptions.
>> 
>> The description is mission. Can you elaborate why it is not here? Same for the other descriptions.
> 
> This is how shouldBeCloseTo works. Should I not use that function, but write my own instead?

Use debug() instead of description(). My fault. Then it will work. Then you can skip the extra mentioning of the unnecessary blend mode naming.

>>>> LayoutTests/fast/canvas/canvas-blend-solid-expected.txt:76
>>>> +FAIL myGetImageData(1, 'color')[0] should be within 5 of 31. Was 255.
>>> 
>>> Why are there so many luminocity and saturation tests but not so much on others? A lot of blend modes seem to be missing.
>> 
>> Why are there so many luminocity and saturation tests but not so much on others? A lot of blend modes seem to be missing.
> 
> I don't understand. Every blend mode is tested.
> Did you review the latest patch?

no, i didn't :/

>>>> LayoutTests/fast/canvas/canvas-blend.html:5
>>>> +    <meta name="DC.publisher" content="Adobe Systems, www.adobe.com">
>>> 
>>> This is unusual and not done in WebKit if the test is written for WebKit.
>> 
>> This is unusual and not done in WebKit if the test is written for WebKit.
> 
> OK. Why are the compositing modes tested that way?

I don't know. This test does not test anything.

>>>> LayoutTests/fast/canvas/script-tests/canvas-blend-image.js:1
>>>> +description("Series of tests to ensure correct results on applying different blend modes.");
>>> 
>>> Still wonder why this is not showing up in the expectation files.
>> 
>> Still wonder why this is not showing up in the expectation files.
> 
> description() is continually updated.
> If you want to see it in the output, a different function is needed.

Yes, my fault. It is debug()

>>>> LayoutTests/fast/canvas/script-tests/canvas-blend-image.js:157
>>>> +                var resultColor = "myGetImageData(" + i + ", '" + blendMode + "')";
>>> 
>>> blendMode is actually not needed if the descriptors work. Don't duplicate output but investigate in the description() function please.
>> 
>> blendMode is actually not needed if the descriptors work. Don't duplicate output but investigate in the description() function please.
> 
> Patch that you r-'ed did not have this

great!

>>>> LayoutTests/platform/chromium/TestExpectations:133
>>>> +webkit.org/b/100071 fast/canvas/canvas-blend.html [ Skip ]
>>> 
>>> You are submitting with failing test results. Do you actually need to skip tests? If so, why?
>> 
>> You are submitting with failing test results. Do you actually need to skip tests? If so, why?
> 
> As far as I can tell, this is how it is done with other missing functionality.
> What else should happen?

Shouldn't the test pass with these results on all platforms?

[Rik Cabanier]

Created attachment 179986 [details]
Patch

[Rik Cabanier]

(In reply to comment #43)
> (From update of attachment 179872 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=179872&action=review
> 
> >>>> Source/WebCore/ChangeLog:21
> >>>> +        * platform/graphics/cg/GraphicsContextCG.cpp:
> >>> 
> >>> line by line comments for such a fundamental feature: "Added BlendMode argument to drawImage to pass blend mode.", "Added new switch for blend modes and appendant CG call." and so on.
> >> 
> >> line by line comments for such a fundamental feature: "Added BlendMode argument to drawImage to pass blend mode.", "Added new switch for blend modes and appendant CG call." and so on.
> > 
> > Why did this suddenly become an issue? Will fix
> 
> With every review I find more issues that I didn't see in previous review :)

OK! I better make this a good one!

> 
> >>>> LayoutTests/fast/canvas/canvas-blend-image-expected.txt:4
> >>>> +FAIL myGetImageData(0, 'color-dodge')[0] should be within 5 of 0. Was 255.
> >>> 
> >>> The description is mission. Can you elaborate why it is not here? Same for the other descriptions.
> >> 
> >> The description is mission. Can you elaborate why it is not here? Same for the other descriptions.
> > 
> > This is how shouldBeCloseTo works. Should I not use that function, but write my own instead?
> 
> Use debug() instead of description(). My fault. Then it will work. Then you can skip the extra mentioning of the unnecessary blend mode naming.
> 
> >>>> LayoutTests/fast/canvas/canvas-blend-solid-expected.txt:76
> >>>> +FAIL myGetImageData(1, 'color')[0] should be within 5 of 31. Was 255.
> >>> 
> >>> Why are there so many luminocity and saturation tests but not so much on others? A lot of blend modes seem to be missing.
> >> 
> >> Why are there so many luminocity and saturation tests but not so much on others? A lot of blend modes seem to be missing.
> > 
> > I don't understand. Every blend mode is tested.
> > Did you review the latest patch?
> 
> no, i didn't :/

no worries.

> 
> >>>> LayoutTests/fast/canvas/canvas-blend.html:5
> >>>> +    <meta name="DC.publisher" content="Adobe Systems, www.adobe.com">
> >>> 
> >>> This is unusual and not done in WebKit if the test is written for WebKit.
> >> 
> >> This is unusual and not done in WebKit if the test is written for WebKit.
> > 
> > OK. Why are the compositing modes tested that way?
> 
> I don't know. This test does not test anything.

OK. removed.

> 
> >>>> LayoutTests/fast/canvas/script-tests/canvas-blend-image.js:1
> >>>> +description("Series of tests to ensure correct results on applying different blend modes.");
> >>> 
> >>> Still wonder why this is not showing up in the expectation files.
> >> 
> >> Still wonder why this is not showing up in the expectation files.
> > 
> > description() is continually updated.
> > If you want to see it in the output, a different function is needed.
> 
> Yes, my fault. It is debug()

OK. Changed.

> 
> >>>> LayoutTests/fast/canvas/script-tests/canvas-blend-image.js:157
> >>>> +                var resultColor = "myGetImageData(" + i + ", '" + blendMode + "')";
> >>> 
> >>> blendMode is actually not needed if the descriptors work. Don't duplicate output but investigate in the description() function please.
> >> 
> >> blendMode is actually not needed if the descriptors work. Don't duplicate output but investigate in the description() function please.
> > 
> > Patch that you r-'ed did not have this
> 
> great!

:-)

> 
> >>>> LayoutTests/platform/chromium/TestExpectations:133
> >>>> +webkit.org/b/100071 fast/canvas/canvas-blend.html [ Skip ]
> >>> 
> >>> You are submitting with failing test results. Do you actually need to skip tests? If so, why?
> >> 
> >> You are submitting with failing test results. Do you actually need to skip tests? If so, why?
> > 
> > As far as I can tell, this is how it is done with other missing functionality.
> > What else should happen?
> 
> Shouldn't the test pass with these results on all platforms?

latest patch should have the correct test results.
On other platforms, those will fail in the short term.

[Dirk Schulze]

Comment on attachment 179986 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=179986&action=review

> LayoutTests/fast/canvas/canvas-blend-image-expected.txt:10
> +Testing blend mode "source-over"
> +solid on solid
> +solid on alpha
> +alpha on solid
> +alpha on alpha

Hmmm. The result is strange. "shouldBeCloseTo()" should give you PASS or FAIL messages, but not nothing. Guess you call the function with wrong arguments?

> LayoutTests/fast/canvas/script-tests/canvas-blend-image.js:133
> +function myGetImageData(i, blend)

You forgot to change the name here.

> LayoutTests/fast/canvas/script-tests/canvas-blend-solid.js:134
> +function myGetImageData(i, blend)

Here too.

[Rik Cabanier]

Created attachment 180268 [details]
Patch

[Rik Cabanier]

(In reply to comment #46)
> (From update of attachment 179986 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=179986&action=review
> 
> > LayoutTests/fast/canvas/canvas-blend-image-expected.txt:10
> > +Testing blend mode "source-over"
> > +solid on solid
> > +solid on alpha
> > +alpha on solid
> > +alpha on alpha
> 
> Hmmm. The result is strange. "shouldBeCloseTo()" should give you PASS or FAIL messages, but not nothing. Guess you call the function with wrong arguments?

I removed the flag that made a pass silent.

> 
> > LayoutTests/fast/canvas/script-tests/canvas-blend-image.js:133
> > +function myGetImageData(i, blend)
> 
> You forgot to change the name here.
> 
> > LayoutTests/fast/canvas/script-tests/canvas-blend-solid.js:134
> > +function myGetImageData(i, blend)
> 
> Here too.

Fixed both

[Rik Cabanier]

(In reply to comment #48)
> (In reply to comment #46)
> > (From update of attachment 179986 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=179986&action=review
> > 
> > > LayoutTests/fast/canvas/canvas-blend-image-expected.txt:10
> > > +Testing blend mode "source-over"
> > > +solid on solid
> > > +solid on alpha
> > > +alpha on solid
> > > +alpha on alpha
> > 
> > Hmmm. The result is strange. "shouldBeCloseTo()" should give you PASS or FAIL messages, but not nothing. Guess you call the function with wrong arguments?
> 
> I removed the flag that made a pass silent.
> 
> > 
> > > LayoutTests/fast/canvas/script-tests/canvas-blend-image.js:133
> > > +function myGetImageData(i, blend)
> > 
> > You forgot to change the name here.
> > 
> > > LayoutTests/fast/canvas/script-tests/canvas-blend-solid.js:134
> > > +function myGetImageData(i, blend)
> > 
> > Here too.
> 
> Fixed both

This patch only works for non-accelerated canvas.
I will open another bug for accelerated canvas. There could be an OS bug...

[Dirk Schulze]

Comment on attachment 180268 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=180268&action=review

Please read reviews a lot more carefully! I said many of the following things in previous patches or previous reviews. This just increases the iterations, takes you longer to get committer rights (assuming you want these) and reviewers will less likely review your patches.

> Source/WebCore/ChangeLog:15
> +        (WebCore::CanvasRenderingContext2D::setGlobalCompositeOperation): passing blendMode to underlying layer

Uses sentences please. Even my examples used sentences.

> Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp:1800
> +        }

The patch itself looks great IMO.

> LayoutTests/ChangeLog:8
> +        Added test files for the new blending modes in Canvas

Dot at the end of a sentence.

> LayoutTests/fast/canvas/canvas-blend-image-expected.txt:8
> +PASS myGetImageData(0, 'source-over')[0] is within 5 of 255

'source-over' this is just riddance, no output of the blend mode again please.

> LayoutTests/fast/canvas/script-tests/canvas-blend-image.js:3
> +if (self.testRunner)
> +  testRunner.overridePreference("WebKitCanvasUsesAcceleratedDrawing", 0);

Why was that necessary?

> LayoutTests/fast/canvas/script-tests/canvas-blend-image.js:136
> +function myGetImageData(i, blend)

s/myGetImageData/pixelDataAtPoint/ third time that I mention this.

> LayoutTests/fast/canvas/script-tests/canvas-blend-image.js:144
> +    var resultColor = "myGetImageData(" + i + ", '" + blendMode + "')";

Remove blendMode from output. (second time I mention this)

> LayoutTests/fast/canvas/script-tests/canvas-blend-solid.js:3
> +if (self.testRunner)
> +   testRunner.overridePreference("WebKitCanvasUsesAcceleratedDrawing", 0);

Why was that necessary? Why do you need to disable it? Shouldn't it work in HW accelerated and SW mode?

> LayoutTests/fast/canvas/script-tests/canvas-blend-solid.js:137
> +function pixelDataAtPoint(i, blend)

Here you have it right, why not in the other test?

> LayoutTests/fast/canvas/script-tests/canvas-blend-solid.js:145
> +    var resultColor = "pixelDataAtPoint(" + i + ", '" + blendMode + "')";

Remove blendMode from output in this line.

> LayoutTests/platform/chromium/TestExpectations:134
> +webkit.org/b/100071 platform/chromium/virtual/gpu/fast/canvas/canvas-blend-image.html [ Skip ]
> +webkit.org/b/100071 platform/chromium/virtual/gpu/fast/canvas/canvas-blend-solid.html [ Skip ]

What is this?

> LayoutTests/platform/qt/TestExpectations:1193
> +# The following test fail because blending is not yet implemented
> +# https://bugs.webkit.org/show_bug.cgi?id=100072
> +fast/canvas/canvas-blend-image.html
> +fast/canvas/canvas-blend-solid.html

There are TestExpectation files for gtk and efl as well. Why don't you skip the tests there as well? I mentioned it before.

[Rik Cabanier]

Created attachment 180352 [details]
Patch

[Rik Cabanier]

(In reply to comment #50)
> (From update of attachment 180268 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=180268&action=review
> 
> Please read reviews a lot more carefully! I said many of the following things in previous patches or previous reviews. This just increases the iterations, takes you longer to get committer rights (assuming you want these) and reviewers will less likely review your patches.

I don't really care.

> 
> > Source/WebCore/ChangeLog:15
> > +        (WebCore::CanvasRenderingContext2D::setGlobalCompositeOperation): passing blendMode to underlying layer
> 
> Uses sentences please. Even my examples used sentences.

done

> 
> > Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp:1800
> > +        }
> 
> The patch itself looks great IMO.
> 
> > LayoutTests/ChangeLog:8
> > +        Added test files for the new blending modes in Canvas
> 
> Dot at the end of a sentence.

done

> 
> > LayoutTests/fast/canvas/canvas-blend-image-expected.txt:8
> > +PASS myGetImageData(0, 'source-over')[0] is within 5 of 255
> 
> 'source-over' this is just riddance, no output of the blend mode again please.

done

> 
> > LayoutTests/fast/canvas/script-tests/canvas-blend-image.js:3
> > +if (self.testRunner)
> > +  testRunner.overridePreference("WebKitCanvasUsesAcceleratedDrawing", 0);
> 
> Why was that necessary?
> 
> > LayoutTests/fast/canvas/script-tests/canvas-blend-image.js:136
> > +function myGetImageData(i, blend)
> 
> s/myGetImageData/pixelDataAtPoint/ third time that I mention this.

hmm. I made those changes but I remember that my editor crashed at some point...

> 
> > LayoutTests/fast/canvas/script-tests/canvas-blend-image.js:144
> > +    var resultColor = "myGetImageData(" + i + ", '" + blendMode + "')";
> 
> Remove blendMode from output. (second time I mention this)
> 
> > LayoutTests/fast/canvas/script-tests/canvas-blend-solid.js:3
> > +if (self.testRunner)
> > +   testRunner.overridePreference("WebKitCanvasUsesAcceleratedDrawing", 0);
> 
> Why was that necessary? Why do you need to disable it? Shouldn't it work in HW accelerated and SW mode?

Some of the blend modes don't work with the accelerated canvas. I will submit a new bug for this.
This might be an operating system bug....

> 
> > LayoutTests/fast/canvas/script-tests/canvas-blend-solid.js:137
> > +function pixelDataAtPoint(i, blend)
> 
> Here you have it right, why not in the other test?

editor weirdness

> 
> > LayoutTests/fast/canvas/script-tests/canvas-blend-solid.js:145
> > +    var resultColor = "pixelDataAtPoint(" + i + ", '" + blendMode + "')";
> 
> Remove blendMode from output in this line.

done

> 
> > LayoutTests/platform/chromium/TestExpectations:134
> > +webkit.org/b/100071 platform/chromium/virtual/gpu/fast/canvas/canvas-blend-image.html [ Skip ]
> > +webkit.org/b/100071 platform/chromium/virtual/gpu/fast/canvas/canvas-blend-solid.html [ Skip ]
> 
> What is this?

weirdness in how chromium runs its tests.
You can find similar cases in the chromium testexpectations file

> 
> > LayoutTests/platform/qt/TestExpectations:1193
> > +# The following test fail because blending is not yet implemented
> > +# https://bugs.webkit.org/show_bug.cgi?id=100072
> > +fast/canvas/canvas-blend-image.html
> > +fast/canvas/canvas-blend-solid.html
> 
> There are TestExpectation files for gtk and efl as well. Why don't you skip the tests there as well? I mentioned it before.

Done.
I saw that, but I didn't understand what you meant.

[Rik Cabanier]

Created attachment 180362 [details]
Patch

[Dirk Schulze]

Comment on attachment 180362 [details]
Patch

r=me

[WebKit Review Bot]

Comment on attachment 180362 [details]
Patch

Clearing flags on attachment: 180362

Committed r138334: <http://trac.webkit.org/changeset/138334>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Ryosuke Niwa]

This patch appears to have regressed fast/canvas/canvas-blend-image.html and fast/canvas/canvas-blend-solid.html on Mac WK2:

http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=%40ToT%20-%20webkit.org&tests=fast%2Fcanvas%2Fcanvas-blend-image.html%2Cfast%2Fcanvas%2Fcanvas-blend-solid.html

http://build.webkit.org/results/Apple%20MountainLion%20Release%20WK2%20(Tests)/r138637%20(4403)/fast/canvas/canvas-blend-image-pretty-diff.html
http://build.webkit.org/results/Apple%20MountainLion%20Release%20WK2%20(Tests)/r138637%20(4403)/fast/canvas/canvas-blend-image-pretty-diff.html

[Ryosuke Niwa]

Huh, it appears that this test should be simply skipped on WK2?

[Ryosuke Niwa]

Filed https://bugs.webkit.org/show_bug.cgi?id=105943.