Fixed a type of Canvas drawImage(canvas). Exchanged dstRect and bufferSrcRect when calling fullCanvasCompositedDrawImage.
Created attachment 128860 [details] patch
It is from bug 71537.
This bug appear explicitly following test case. https://bug-66920-attachments.webkit.org/attachment.cgi?id=105127
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.
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.
(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.
OK, I will update in this bug. :)
Created attachment 129002 [details] patch v.2
I amended canvas-composite-image and canvas-composite-canvas to cover this bug.
senorblanco, is this something you could review?
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.
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).
Created attachment 129112 [details] patch v.3
Thanks for good title. :)
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."
Created attachment 129114 [details] patch v.4 I had misunderstanding that changelogs should be consistent.
Comment on attachment 129114 [details] patch v.4 Thanks Huang for the updated patch.
You're welcome. ;)
Comment on attachment 129114 [details] patch v.4 Clearing flags on attachment: 129114 Committed r109062: <http://trac.webkit.org/changeset/109062>
All reviewed patches have been landed. Closing bug.