This bug will add the canvas blending modes using CG
Created attachment 177626 [details] First try. Not ready for review
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
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
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
Created attachment 177660 [details] not for review
Comment on attachment 177660 [details] not for review Attachment 177660 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/15147313
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
Created attachment 177662 [details] not for review
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.
Comment on attachment 177662 [details] not for review Attachment 177662 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/15138464
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
Created attachment 177665 [details] not for review
Created attachment 177668 [details] not for review
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
Comment on attachment 177668 [details] not for review Attachment 177668 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/15154267
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
Comment on attachment 177668 [details] not for review Attachment 177668 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/15154307
Created attachment 178334 [details] Patch
Created attachment 178335 [details] Added QT testexpectations file
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.
(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.
(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.
Created attachment 178693 [details] Updated bug per Dirk's comments
(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).
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.
Created attachment 179508 [details] fixed per Dirk's comments
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
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
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
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
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
Created attachment 179523 [details] Patch
(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.
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
Created attachment 179535 [details] fixed chromium testexpectations
Created attachment 179872 [details] Patch
Created attachment 179874 [details] Patch
(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(']'); }
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, cabanier@adob.com"> > + <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.
Comment on attachment 179874 [details] Patch See previous comments. r-
(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, cabanier@adob.com"> > > + <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.
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?
Created attachment 179986 [details] Patch
(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.
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.
Created attachment 180268 [details] Patch
(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
(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...
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.
Created attachment 180352 [details] Patch
(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.
Created attachment 180362 [details] Patch
Comment on attachment 180362 [details] Patch r=me
Comment on attachment 180362 [details] Patch Clearing flags on attachment: 180362 Committed r138334: <http://trac.webkit.org/changeset/138334>
All reviewed patches have been landed. Closing bug.
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
Huh, it appears that this test should be simply skipped on WK2?
Filed https://bugs.webkit.org/show_bug.cgi?id=105943.