[webkit-reviews] review denied: [Bug 106358] [EFL][WebGL] WebGL content is not painted after resizing the viewport. : [Attachment 184855] Patch

[bugzilla-daemon at webkit.org]

Benjamin Poulain <benjamin at webkit.org> has denied Viatcheslav Ostapenko
<ostap73 at gmail.com>'s request for review:
Bug 106358: [EFL][WebGL] WebGL content is not painted after resizing the
viewport.
https://bugs.webkit.org/show_bug.cgi?id=106358

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

------- Additional Comments from Benjamin Poulain <benjamin at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=184855&action=review


I sign off on this for WebKit2. Please get Noam to review the changes for
correctness.

In my opinion, the test is not good enough. Tests have a cost, especially with
timers.

> LayoutTests/ChangeLog:14
> +	   Add test checking that canvas painting is correct if layer
parameters were changed,
> +	   but webgl canvas didn't change.
> +
> +	   * fast/canvas/webgl/webgl-composite-modes-update-expected.png:
Added.
> +	   * fast/canvas/webgl/webgl-composite-modes-update-expected.txt:
Added.
> +	   * fast/canvas/webgl/webgl-composite-modes-update.html: Added.
> +

I think this is not a great test. You should focus on the use case at hand, not
have complex rendering for the sake of it. Drawing simple rect should be enough
to reproduce the bug with a pixel test.

The test rely on two timers. It is unlikely required as most canvas rendering
can be forced.

> LayoutTests/fast/canvas/webgl/webgl-composite-modes-update.html:28
> +This test is only useful as a pixel test. You should see two rows with 4
canvases in each. The top row of canvases should have a black background, the
bottom row should have a dark blue background.
> +Each canvas should have a green triangle in it. If multisampling is
supported, the odd columns should have smooth edges instead of jagged
stair-stepping. Otherwise, all canvases within a row should be identical to
each other.

This is not an accurate description of the test. You should have some context
about what is being tested.