[webkit-reviews] review denied: [Bug 100070] Add canvas blending modes using Core Graphics : [Attachment 180268] Patch

[bugzilla-daemon at webkit.org]

Dirk Schulze <krit at webkit.org> has denied Rik Cabanier <cabanier at adobe.com>'s
request for review:
Bug 100070: Add canvas blending modes using Core Graphics
https://bugs.webkit.org/show_bug.cgi?id=100070

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

------- Additional Comments from Dirk Schulze <krit at webkit.org>
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.