[webkit-reviews] review granted: [Bug 74779] Drawing an animated image to a canvas via drawImage should draw the first frame : [Attachment 376066] Patch

[bugzilla-daemon at webkit.org]

Said Abou-Hallawa <sabouhallawa at apple.com> has granted Carlos Alberto Lopez
Perez <clopez at igalia.com>'s request for review:
Bug 74779: Drawing an animated image to a canvas via drawImage should draw the
first frame
https://bugs.webkit.org/show_bug.cgi?id=74779

Attachment 376066: Patch

https://bugs.webkit.org/attachment.cgi?id=376066&action=review




--- Comment #35 from Said Abou-Hallawa <sabouhallawa at apple.com> ---
Comment on attachment 376066
  --> https://bugs.webkit.org/attachment.cgi?id=376066
Patch

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

> Source/WebCore/ChangeLog:10
> +	   This patch passes as image to be drawn into the canvas context a
> +	   new image with the contents of the first frame of the animated
image.

This statement is a little bit hard to read.

>
LayoutTests/fast/canvas/drawImage-animated-gif-draws-first-frame-and-no-reset-i
mage.html:11
> +	   description("This tests that when drawing an animated image to a
canvas it draws the first frame and that the playing image doesn't get
reseted.");

Testing the loop count is covered by other tests.

>
LayoutTests/fast/canvas/drawImage-animated-gif-draws-first-frame-and-no-reset-i
mage.html:17
> +	   var canvas = document.getElementsByTagName("canvas")[0];
> +	   var previousFrameIndex = 0;
> +	   var currentFrameIndex = 0;
> +	   var canvasAlreadyDrawed = false;
> +	   var imgData = [];

These variables can be just local variables or just not needed.

>
LayoutTests/fast/canvas/drawImage-animated-gif-draws-first-frame-and-no-reset-i
mage.html:41
> +	       canvasAlreadyDrawed = true;

No need to track whether an image is to drawn to the canvas. The caller should
call this function only once: when the current frame is the last frame.

>
LayoutTests/fast/canvas/drawImage-animated-gif-draws-first-frame-and-no-reset-i
mage.html:46
> +	       shouldBe("internals.imageFrameIndex(image)", "3");

What is "3"? Did you mean to use imageLastFrameIndex instead?

>
LayoutTests/fast/canvas/drawImage-animated-gif-draws-first-frame-and-no-reset-i
mage.html:51
> +	   function imageCheckDrawFrames()
> +	   {

The curly brace should be moved to the previous line.

>
LayoutTests/fast/canvas/drawImage-animated-gif-draws-first-frame-and-no-reset-i
mage.html:54
> +	       if (currentFrameIndex == 3) {

What does 3 mean here? Did you mean to use imageLastFrameIndex instead?

>
LayoutTests/fast/canvas/drawImage-animated-gif-draws-first-frame-and-no-reset-i
mage.html:61
> +	       if (currentFrameIndex < previousFrameIndex) {
> +		   testFailed("The GIF animation should not be resetted.");
> +		   finishJSTest();
> +	       }

No need to test the loop count here.

>
LayoutTests/fast/canvas/drawImage-animated-gif-draws-first-frame-and-no-reset-i
mage.html:64
> +		       testPassed("The GIF animation has completed playing and
the Canvas has been drawn.");
> +		       finishJSTest();

Wrong indentation.

>
LayoutTests/fast/canvas/drawImage-animated-gif-draws-first-frame-and-no-reset-i
mage.html:66
> +	       previousFrameIndex = currentFrameIndex;

I am a little puzzled by the logic in this function. Can't the logic be
simplified like this:

image.addEventListener("webkitImageFrameReady", () => {
    if (internals.imageFrameIndex(image) == internals.imageFrameCount(image) -
1) {
	drawAndCheckCanvas();
	finishJSTest();
    }
}

> LayoutTests/fast/canvas/drawImage-animated-gif.html:2
> +<title>This tests that when drawing an animated image to a canvas it draws
the first frame.</title>

There is no much difference between this test and
drawImage-animated-gif-draws-first-frame-and-no-reset-image.html. In fact the
other test is timed much better than this one. You can also convert the other
test to ref-test if this is the goal of a second test.

> LayoutTests/fast/canvas/drawImage-animated-gif.html:19
> +    function runTest() {
> +	   // wait for the gif loop to be completed
> +	   setTimeout(drawCanvas, 500);
> +    }

This is bad timing technique. The other test is timed much better than this
which relies on setTimeout(). If the EWS bots are overloaded, drawing the
frames of the animated image might take longer than 500ms. In this case, the
test will be flaky.