53951 2011-02-07 15:05:03 -0800 drawImageBuffer in GraphicsContext passes magic (-1,-1) sized rects 2011-02-08 09:06:07 -0800 1 1 1 Unclassified WebKit WebCore Misc. 528+ (Nightly build) PC All RESOLVED FIXED P2 Normal --- 0 gwright webkit-unassigned commit-queue darin oldest_to_newest 346847 0 gwright 2011-02-07 15:05:03 -0800 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. 346853 1 81536 gwright 2011-02-07 15:09:03 -0800 Created attachment 81536 Send correctly sized rects in drawImageBuffer Send the correct sized rects in drawImageBuffer to the ImageBuffer. 346857 2 81536 darin 2011-02-07 15:09:53 -0800 Comment on attachment 81536 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. 346883 3 gwright 2011-02-07 15:20:30 -0800 (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. 346887 4 darin 2011-02-07 15:21:26 -0800 (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? 346894 5 gwright 2011-02-07 15:24:54 -0800 (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 346895 6 darin 2011-02-07 15:29:39 -0800 (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. 346897 7 darin 2011-02-07 15:30:16 -0800 (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. 346898 8 81536 darin 2011-02-07 15:30:54 -0800 Comment on attachment 81536 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 346899 9 gwright 2011-02-07 15:34:50 -0800 (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! 347367 10 81536 paroga 2011-02-08 07:45:33 -0800 Comment on attachment 81536 Send correctly sized rects in drawImageBuffer cq+ as requested by gw280 on IRC 347428 11 81536 commit-queue 2011-02-08 09:06:02 -0800 Comment on attachment 81536 Send correctly sized rects in drawImageBuffer Clearing flags on attachment: 81536 Committed r77945: <http://trac.webkit.org/changeset/77945> 347429 12 commit-queue 2011-02-08 09:06:07 -0800 All reviewed patches have been landed. Closing bug. 81536 2011-02-07 15:09:03 -0800 2011-02-08 09:06:01 -0800 Send correctly sized rects in drawImageBuffer drawImageBuffer.patch text/plain 2358 gwright Y29tbWl0IGU4MmIxMzE0MGVjMDQ4YmQwN2Y3ZTBiZmMzYWE3Y2UwOWJhMGJkMDYKQXV0aG9yOiBH ZW9yZ2UgV3JpZ2h0IDxnd3JpZ2h0QHJpbS5jb20+CkRhdGU6ICAgTW9uIEZlYiA3IDE3OjU5OjQw IDIwMTEgLTA1MDAKCiAgICAgMjAxMS0wMi0wNyAgR2VvcmdlIFdyaWdodCAgPGd3cmlnaHRAcmlt LmNvbT4KICAgIAogICAgICAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCiAgICAK ICAgICAgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD01Mzk1 MQogICAgCiAgICAgICAgICAgICBFbnN1cmUgd2UgZG8gbm90IHBhc3MgKC0xLCAtMSkgc2l6ZWQg cmVjdHMgdG8gSW1hZ2VCdWZmZXIgYnV0IGluc3RlYWQKICAgICAgICAgICAgIHBhc3MgdGhlIGZ1 bGwgaW1hZ2UgZGltZW5zaW9ucy4KICAgIAogICAgICAgICAgICAgKiBwbGF0Zm9ybS9ncmFwaGlj cy9HcmFwaGljc0NvbnRleHQuY3BwOgogICAgICAgICAgICAgKFdlYkNvcmU6OkdyYXBoaWNzQ29u dGV4dDo6ZHJhd0ltYWdlQnVmZmVyKToKCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9DaGFu Z2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggYmMzZDE4MS4uNTQzYTBmMCAx MDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1NvdXJjZS9XZWJDb3Jl L0NoYW5nZUxvZwpAQCAtMSwzICsxLDE1IEBACisyMDExLTAyLTA3ICBHZW9yZ2UgV3JpZ2h0ICA8 Z3dyaWdodEByaW0uY29tPgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgor CisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD01Mzk1MQor CisgICAgICAgIEVuc3VyZSB3ZSBkbyBub3QgcGFzcyAoLTEsIC0xKSBzaXplZCByZWN0cyB0byBJ bWFnZUJ1ZmZlciBidXQgaW5zdGVhZAorICAgICAgICBwYXNzIHRoZSBmdWxsIGltYWdlIGRpbWVu c2lvbnMuCisKKyAgICAgICAgKiBwbGF0Zm9ybS9ncmFwaGljcy9HcmFwaGljc0NvbnRleHQuY3Bw OgorICAgICAgICAoV2ViQ29yZTo6R3JhcGhpY3NDb250ZXh0OjpkcmF3SW1hZ2VCdWZmZXIpOgor CiAyMDExLTAyLTA3ICBTYW0gV2VpbmlnICA8c2FtQHdlYmtpdC5vcmc+CiAKICAgICAgICAgRml4 IGJ1aWxkLgpkaWZmIC0tZ2l0IGEvU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vZ3JhcGhpY3MvR3Jh cGhpY3NDb250ZXh0LmNwcCBiL1NvdXJjZS9XZWJDb3JlL3BsYXRmb3JtL2dyYXBoaWNzL0dyYXBo aWNzQ29udGV4dC5jcHAKaW5kZXggMjk4YTg5ZC4uMTZlZmE3MCAxMDA2NDQKLS0tIGEvU291cmNl L1dlYkNvcmUvcGxhdGZvcm0vZ3JhcGhpY3MvR3JhcGhpY3NDb250ZXh0LmNwcAorKysgYi9Tb3Vy Y2UvV2ViQ29yZS9wbGF0Zm9ybS9ncmFwaGljcy9HcmFwaGljc0NvbnRleHQuY3BwCkBAIC01NDQs MTAgKzU0NCwxMCBAQCB2b2lkIEdyYXBoaWNzQ29udGV4dDo6ZHJhd0ltYWdlQnVmZmVyKEltYWdl QnVmZmVyKiBpbWFnZSwgQ29sb3JTcGFjZSBzdHlsZUNvbG9yUwogICAgICAgICBJbnRlcnBvbGF0 aW9uUXVhbGl0eSBwcmV2aW91c0ludGVycG9sYXRpb25RdWFsaXR5ID0gaW1hZ2VJbnRlcnBvbGF0 aW9uUXVhbGl0eSgpOwogICAgICAgICAvLyBGSVhNRTogU2hvdWxkIGJlIEludGVycG9sYXRpb25M b3cKICAgICAgICAgc2V0SW1hZ2VJbnRlcnBvbGF0aW9uUXVhbGl0eShJbnRlcnBvbGF0aW9uTm9u ZSk7Ci0gICAgICAgIGltYWdlLT5kcmF3KHRoaXMsIHN0eWxlQ29sb3JTcGFjZSwgZGVzdCwgc3Jj LCBvcCwgdXNlTG93UXVhbGl0eVNjYWxlKTsKKyAgICAgICAgaW1hZ2UtPmRyYXcodGhpcywgc3R5 bGVDb2xvclNwYWNlLCBGbG9hdFJlY3QoZGVzdC5sb2NhdGlvbigpLCBGbG9hdFNpemUodHcsIHRo KSksIEZsb2F0UmVjdChzcmMubG9jYXRpb24oKSwgRmxvYXRTaXplKHRzdywgdHNoKSksIG9wLCB1 c2VMb3dRdWFsaXR5U2NhbGUpOwogICAgICAgICBzZXRJbWFnZUludGVycG9sYXRpb25RdWFsaXR5 KHByZXZpb3VzSW50ZXJwb2xhdGlvblF1YWxpdHkpOwogICAgIH0gZWxzZQotICAgICAgICBpbWFn ZS0+ZHJhdyh0aGlzLCBzdHlsZUNvbG9yU3BhY2UsIGRlc3QsIHNyYywgb3AsIHVzZUxvd1F1YWxp dHlTY2FsZSk7CisgICAgICAgIGltYWdlLT5kcmF3KHRoaXMsIHN0eWxlQ29sb3JTcGFjZSwgRmxv YXRSZWN0KGRlc3QubG9jYXRpb24oKSwgRmxvYXRTaXplKHR3LCB0aCkpLCBGbG9hdFJlY3Qoc3Jj LmxvY2F0aW9uKCksIEZsb2F0U2l6ZSh0c3csIHRzaCkpLCBvcCwgdXNlTG93UXVhbGl0eVNjYWxl KTsKIH0KIAogdm9pZCBHcmFwaGljc0NvbnRleHQ6OmFkZFJvdW5kZWRSZWN0Q2xpcChjb25zdCBS b3VuZGVkSW50UmVjdCYgcmVjdCkK