Bug 79566 – Fixed a typo in CanvasRenderingContext2D::drawImage(HTMLCanvasElement); incorrect source and destination rect used

• Component: Canvas
• Status: RESOLVED FIXED

[Dongseong Hwang]

Fixed a type of Canvas drawImage(canvas).

Exchanged dstRect and bufferSrcRect when calling fullCanvasCompositedDrawImage.

[Dongseong Hwang]

Created attachment 128860 [details]
patch

[Dongseong Hwang]

It is from bug 71537.

[Dongseong Hwang]

This bug appear explicitly following test case.
https://bug-66920-attachments.webkit.org/attachment.cgi?id=105127

[Ben Wells]

The test case referenced does look wrong, as does the code. The source-is-image and source-is-canvas cases should look the same.

The fix looks right. (I'm not a reviewer so cannot r+).

This makes me wonder what is special about the test I added originally to test this case. LayoutTests/fast/canvas/canvas-composite-image.html and canvas-composite-canvas.html are the tests for source-is-image and source-is-canvas respectively, and I think both produce the same / correct results.

[Dongseong Hwang]

The source-is-image and source-is-canvas cases of source-in, source-out, destination-in and destination-atop in the test case referenced are different.
The source-is-canvas does not have offset, so is rendered on left-top without padding.

I think the layout tests, LayoutTests/fast/canvas/canvas-composite-image.html and canvas-composite-canvas.html, do not reproduce this bug because those may not have offset for image that is rendered into canvas.

[Ben Wells]

(In reply to comment #5)
> The source-is-image and source-is-canvas cases of source-in, source-out, destination-in and destination-atop in the test case referenced are different.
> The source-is-canvas does not have offset, so is rendered on left-top without padding.

Yes, agreed. And as mentioned your code looks like the correct fix.

> 
> I think the layout tests, LayoutTests/fast/canvas/canvas-composite-image.html and canvas-composite-canvas.html, do not reproduce this bug because those may not have offset for image that is rendered into canvas.

It would be good to update these tests to cover this case.

[Dongseong Hwang]

OK, I will update in this bug. :)

[Dongseong Hwang]

Created attachment 129002 [details]
patch v.2

[Dongseong Hwang]

I amended canvas-composite-image and canvas-composite-canvas to cover this bug.

[Kenneth Russell]

senorblanco, is this something you could review?

[Daniel Bates]

Comment on attachment 129002 [details]
patch v.2

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

This patch looks good to me, but we need a change log entry for the layout test changes.

You may also want to add a remark in the layout test change log entry that this patch only affects the results of  LayoutTests/fast/canvas/canvas-composite-canvas.html and that the changes made to canvas-composite-image.html are a side effect of the change made to LayoutTests/fast/canvas/resources/canvas-composite-image-common.js.

> LayoutTests/ChangeLog:3
> +        Fixed a typo of Canvas drawImage(canvas).

Nit: I updated the title of this bug to elaborate on the typo. For consistency, we should update this line to match the updated bug title. The updated title is a bit verbose. Feel free to come up with an alternative bug title.

[Daniel Bates]

Comment on attachment 129002 [details]
patch v.2

Disregard my remark about missing a change log entry for the layout tests. Before landing I suggest updating the bug title in the change log entries and adding the remark about the reason for updating fast/canvas/canvas-composite-image.html (see previous comment).

[Dongseong Hwang]

Created attachment 129112 [details]
patch v.3

[Dongseong Hwang]

Thanks for good title. :)

[Daniel Bates]

Comment on attachment 129112 [details]
patch v.3

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

> Source/WebCore/ChangeLog:11
> +        I amended canvas-composite-canvas to cover this bug, and I also amended
> +        canvas-composite-image because I changed
> +        LayoutTests/fast/canvas/resources/canvas-composite-image-common.js.
> +        However, the change of canvas-composite-image is reasonable because
> +        drawImage(HTMLImageElement) will be able to have a similar bug.

Did you mean to use the same description in the WebCore change log? I suggest writing something specific to the WebCore change in this change log entry, like:
"Pass dstRect and bufferSrcRect to CanvasRenderingContext2D::fullCanvasCompositedDrawImage() for the destination and source rect, respectively."

[Dongseong Hwang]

Created attachment 129114 [details]
patch v.4

I had misunderstanding that changelogs should be consistent.

[Daniel Bates]

Comment on attachment 129114 [details]
patch v.4

Thanks Huang for the updated patch.

[Dongseong Hwang]

You're welcome. ;)

[WebKit Review Bot]

Comment on attachment 129114 [details]
patch v.4

Clearing flags on attachment: 129114

Committed r109062: <http://trac.webkit.org/changeset/109062>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.