Bug 53951 - drawImageBuffer in GraphicsContext passes magic (-1,-1) sized rects
Summary: drawImageBuffer in GraphicsContext passes magic (-1,-1) sized rects
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-02-07 15:05 PST by George Wright
Modified: 2011-02-08 09:06 PST (History)
2 users (show)

See Also:


Attachments
Send correctly sized rects in drawImageBuffer (2.30 KB, patch)
2011-02-07 15:09 PST, George Wright
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description George Wright 2011-02-07 15:05:03 PST
GraphicsContext::drawImageBuffer has logic inside it to set up a FloatSize properly based on (tw, th) and (tsw, tsh) if the dimensions are -1, -1 for the source rect. However, these variables are never used and the (-1, -1) rect is passed to ImageBuffer anyway. This is clearly in contrast with, for example, drawImage.
Comment 1 George Wright 2011-02-07 15:09:03 PST
Created attachment 81536 [details]
Send correctly sized rects in drawImageBuffer

Send the correct sized rects in drawImageBuffer to the ImageBuffer.
Comment 2 Darin Adler 2011-02-07 15:09:53 PST
Comment on attachment 81536 [details]
Send correctly sized rects in drawImageBuffer

Does this change have any effect? How can we test that the fix is effective? Normally we do not accept code changes that fix bugs without regression tests.
Comment 3 George Wright 2011-02-07 15:20:30 PST
(In reply to comment #2)
> (From update of attachment 81536 [details])
> Does this change have any effect? How can we test that the fix is effective? Normally we do not accept code changes that fix bugs without regression tests.

It's exactly the same logic as is applied in drawImage. This is clearly what the original author of drawImageBuffer intended, but forgot to set up the FloatRects accordingly (evident by the fact that drawImageBuffer already calculates th, tw, tsh and tsw but never actually uses them).

This fixes rendering issues with certain versions of Skia.
Comment 4 Darin Adler 2011-02-07 15:21:26 PST
(In reply to comment #3)
> This fixes rendering issues with certain versions of Skia.

Can you make a regression test which shows those issues, or are they with some version of Skia not available to us on buildbots?
Comment 5 George Wright 2011-02-07 15:24:54 PST
(In reply to comment #4)
> (In reply to comment #3)
> > This fixes rendering issues with certain versions of Skia.
> 
> Can you make a regression test which shows those issues, or are they with some version of Skia not available to us on buildbots?

yes, it's a version of skia that's not available on the build bots
Comment 6 Darin Adler 2011-02-07 15:29:39 PST
(In reply to comment #3)
> (In reply to comment #2)
> > Does this change have any effect? How can we test that the fix is effective? Normally we do not accept code changes that fix bugs without regression tests.
> 
> It's exactly the same logic as is applied in drawImage. This is clearly what the original author of drawImageBuffer intended [...]
> 
> This fixes rendering issues with certain versions of Skia.

For the record, the requirement for a regression test is not any reflection on how good or obvious a fix is, or how silly the original error was.

I think it’s OK for us to take this change that has no effect with any current configurations and is useful to Google in your work with newer fancier versions of Skia. But in the future, change log should explain why there is no test when you propose a change without a test.
Comment 7 Darin Adler 2011-02-07 15:30:16 PST
(In reply to comment #6)
> I think it’s OK for us to take this change that has no effect with any current configurations and is useful to Google in your work with newer fancier versions of Skia. But in the future, change log should explain why there is no test when you propose a change without a test.

Heh, RIM, not Google.
Comment 8 Darin Adler 2011-02-07 15:30:54 PST
Comment on attachment 81536 [details]
Send correctly sized rects in drawImageBuffer

review+ given that George assures me this causes no problems with any other ports, and is useful to RIM in some improved version of Skia they are working with
Comment 9 George Wright 2011-02-07 15:34:50 PST
(In reply to comment #6)
> For the record, the requirement for a regression test is not any reflection on how good or obvious a fix is, or how silly the original error was.
> 
> I think it’s OK for us to take this change that has no effect with any current configurations and is useful to Google in your work with newer fancier versions of Skia. But in the future, change log should explain why there is no test when you propose a change without a test.

Noted. Thanks!
Comment 10 Patrick R. Gansterer 2011-02-08 07:45:33 PST
Comment on attachment 81536 [details]
Send correctly sized rects in drawImageBuffer

cq+ as requested by gw280 on IRC
Comment 11 WebKit Commit Bot 2011-02-08 09:06:02 PST
Comment on attachment 81536 [details]
Send correctly sized rects in drawImageBuffer

Clearing flags on attachment: 81536

Committed r77945: <http://trac.webkit.org/changeset/77945>
Comment 12 WebKit Commit Bot 2011-02-08 09:06:07 PST
All reviewed patches have been landed.  Closing bug.