67885 2011-09-09 23:35:00 -0700 Outline for the high-resolution broken image icon draws at 2x 2011-09-13 16:17:14 -0700 1 1 1 Unclassified WebKit Layout and Rendering 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 1 bdakin bdakin bdakin darin mitz oldest_to_newest 465241 0 bdakin 2011-09-09 23:35:00 -0700 https://bugs.webkit.org/show_bug.cgi?id=67819 added high resolution platform images to WebCore. The outline for the high-resolution broken image icon draws at 2x, but it should not. 465242 1 bdakin 2011-09-09 23:36:43 -0700 <rdar://problem/10104637> 466520 2 107223 bdakin 2011-09-13 14:09:19 -0700 Created attachment 107223 Patch 466523 3 bdakin 2011-09-13 14:21:40 -0700 Also: <rdar://problem/10104637>. I added that the the Changelog as well. 466536 4 107223 mitz 2011-09-13 14:31:41 -0700 Comment on attachment 107223 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=107223&action=review > Source/WebCore/rendering/RenderImage.cpp:92 > + if (deviceScaleFactor >= 2) > + imageSize.scale(0.5f); It seems strange to always multiply by 1/2 rather than by 1/deviceScaleFactor. Is this relying on some knowledge of which sizes the broken image is available at? 466546 5 bdakin 2011-09-13 14:37:04 -0700 (In reply to comment #4) > (From update of attachment 107223 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=107223&action=review > > > Source/WebCore/rendering/RenderImage.cpp:92 > > + if (deviceScaleFactor >= 2) > > + imageSize.scale(0.5f); > > It seems strange to always multiply by 1/2 rather than by 1/deviceScaleFactor. Is this relying on some knowledge of which sizes the broken image is available at? This matter came up in https://bugs.webkit.org/show_bug.cgi?id=67819 In that bug, Darin felt strongly that code similar to this in RenderLayer should use 0.5 instead of 1/deviceScaleFactor which was how I had coded it at one point. His argument was that currently we only have 2x images so (to quote him directly): "If the scale factor was 4 we would load the 2x image and then scale its size down by 4, so so display a half-sized resized. Not good! It would just get worse at higher resolutions." 466559 6 107223 mitz 2011-09-13 14:44:55 -0700 Comment on attachment 107223 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=107223&action=review >>> Source/WebCore/rendering/RenderImage.cpp:92 >>> + imageSize.scale(0.5f); >> >> It seems strange to always multiply by 1/2 rather than by 1/deviceScaleFactor. Is this relying on some knowledge of which sizes the broken image is available at? > > This matter came up in https://bugs.webkit.org/show_bug.cgi?id=67819 > > In that bug, Darin felt strongly that code similar to this in RenderLayer should use 0.5 instead of 1/deviceScaleFactor which was how I had coded it at one point. His argument was that currently we only have 2x images so (to quote him directly): "If the scale factor was 4 we would load the 2x image and then scale its size down by 4, so so display a half-sized resized. Not good! It would just get worse at higher resolutions." I see. Dividing by the scale factor would be incorrect, and doing this is correct, but the reason why it’s correct is nowhere near this code. A comment here could explain why this is the right thing to do, but still won’t help us remember to change this when we add more versions of the image. 466561 7 mitz 2011-09-13 14:46:18 -0700 A good way to have the code do the right thing would be to have a way to ask an image for its resolution (or scale factor), but that is outside the scope of this bug. 466563 8 bdakin 2011-09-13 14:49:05 -0700 (In reply to comment #6) > > I see. Dividing by the scale factor would be incorrect, and doing this is correct, but the reason why it’s correct is nowhere near this code. A comment here could explain why this is the right thing to do, but still won’t help us remember to change this when we add more versions of the image. Thanks for the review. I agree this code is problematic in that it will be tricky to find in the future, unlike the RenderLayer code which is just a few lines below the call that actually loads the image. I will definitely add a comment here. Perhaps we should make 0.5 a global constant somewhere? Called something along the lines of highResolutionArtworkScaleFactor? When we have more then one scale factor for high-resolution artwork, the global constant will be insufficient, but it would at least make all instances more find-able? What do you think? 466567 9 bdakin 2011-09-13 14:50:35 -0700 (In reply to comment #7) > A good way to have the code do the right thing would be to have a way to ask an image for its resolution (or scale factor), but that is outside the scope of this bug. Oh that's a good idea too. (I didn't see this comment before I left my last comment.) 466611 10 bdakin 2011-09-13 15:33:56 -0700 Committed this change with revision 95048…and then fixed up the ChangeLog mishap with revision 95051. 466621 11 bdakin 2011-09-13 15:40:59 -0700 (In reply to comment #10) > Committed this change with revision 95048…and then fixed up the ChangeLog mishap with revision 95051. Doh! And in revision 95053 I added a comment. Feeling spacey today. 466643 12 darin 2011-09-13 16:17:14 -0700 (In reply to comment #7) > A good way to have the code do the right thing would be to have a way to ask an image for its resolution (or scale factor), but that is outside the scope of this bug. We don’t even have to go that far. Just returning a scale factor along with the image from the brokenImage function would do for this case. 107223 2011-09-13 14:09:19 -0700 2011-09-13 14:44:55 -0700 Patch for-review.txt text/plain 2171 bdakin SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDk1MDQzKQorKysgU291cmNlL1dlYkNvcmUvQ2hhbmdl TG9nCSh3b3JraW5nIGNvcHkpCkBAIC0xLDMgKzEsMTQgQEAKKzIwMTEtMDktMTMgIEJldGggRGFr aW4gIDxiZGFraW5AYXBwbGUuY29tPgorCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3Jn L3Nob3dfYnVnLmNnaT9pZD02Nzg4NQorICAgICAgICBPdXRsaW5lIGZvciB0aGUgaGlnaC1yZXNv bHV0aW9uIGJyb2tlbiBpbWFnZSBpY29uIGRyYXdzIGF0IDJ4CisKKyAgICAgICAgUmV2aWV3ZWQg YnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgU2NhbGVkIHRoZSBpbWFnZSBzaXplIHRvIGFj Y291bnQgZm9yIHRoZSBkZXZpY2VTY2FsZUZhY3Rvci4gCisgICAgICAgICogcmVuZGVyaW5nL1Jl bmRlckltYWdlLmNwcDoKKyAgICAgICAgKFdlYkNvcmU6OlJlbmRlckltYWdlOjppbWFnZVNpemVG b3JFcnJvcik6CisKIDIwMTEtMDktMTMgIEVyaWMgU2VpZGVsICA8ZXJpY0B3ZWJraXQub3JnPgog CiAgICAgICAgIFJlbW92ZSBFTkFCTEVfU1ZHX0ZPUkVJR05fT0JKRUNUIGFzIGl0IGlzIGEgcmVx dWlyZWQgcGFydCBvZiBIVE1MNQpJbmRleDogU291cmNlL1dlYkNvcmUvcmVuZGVyaW5nL1JlbmRl ckltYWdlLmNwcAo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2ViQ29yZS9yZW5kZXJpbmcvUmVuZGVy SW1hZ2UuY3BwCShyZXZpc2lvbiA5NDk4MCkKKysrIFNvdXJjZS9XZWJDb3JlL3JlbmRlcmluZy9S ZW5kZXJJbWFnZS5jcHAJKHdvcmtpbmcgY29weSkKQEAgLTg0LDE1ICs4NCwxOCBAQCBJbnRTaXpl IFJlbmRlckltYWdlOjppbWFnZVNpemVGb3JFcnJvcihDCiAgICAgQVNTRVJUX0FSRyhuZXdJbWFn ZSwgbmV3SW1hZ2UpOwogICAgIEFTU0VSVF9BUkcobmV3SW1hZ2UsIG5ld0ltYWdlLT5pbWFnZSgp KTsKIAotICAgIEltYWdlKiBicm9rZW5JbWFnZTsKLSAgICBpZiAobmV3SW1hZ2UtPndpbGxQYWlu dEJyb2tlbkltYWdlKCkpCi0gICAgICAgIGJyb2tlbkltYWdlID0gbmV3SW1hZ2UtPmJyb2tlbklt YWdlKFBhZ2U6OmRldmljZVNjYWxlRmFjdG9yKGZyYW1lKCkpKTsKLSAgICBlbHNlCi0gICAgICAg IGJyb2tlbkltYWdlID0gbmV3SW1hZ2UtPmltYWdlKCk7CisgICAgSW50U2l6ZSBpbWFnZVNpemU7 CisgICAgaWYgKG5ld0ltYWdlLT53aWxsUGFpbnRCcm9rZW5JbWFnZSgpKSB7CisgICAgICAgIGZs b2F0IGRldmljZVNjYWxlRmFjdG9yID0gUGFnZTo6ZGV2aWNlU2NhbGVGYWN0b3IoZnJhbWUoKSk7 CisgICAgICAgIGltYWdlU2l6ZSA9IG5ld0ltYWdlLT5icm9rZW5JbWFnZShkZXZpY2VTY2FsZUZh Y3RvciktPnNpemUoKTsKKyAgICAgICAgaWYgKGRldmljZVNjYWxlRmFjdG9yID49IDIpCisgICAg ICAgICAgICBpbWFnZVNpemUuc2NhbGUoMC41Zik7ICAgICAgICAKKyAgICB9IGVsc2UKKyAgICAg ICAgaW1hZ2VTaXplID0gbmV3SW1hZ2UtPmltYWdlKCktPnNpemUoKTsKIAogICAgIC8vIGltYWdl U2l6ZSgpIHJldHVybnMgMCBmb3IgdGhlIGVycm9yIGltYWdlLiBXZSBuZWVkIHRoZSB0cnVlIHNp emUgb2YgdGhlCiAgICAgLy8gZXJyb3IgaW1hZ2UsIHNvIHdlIGhhdmUgdG8gZ2V0IGl0IGJ5IGdy YWJiaW5nIGltYWdlKCkgZGlyZWN0bHkuCi0gICAgcmV0dXJuIEludFNpemUocGFkZGluZ1dpZHRo ICsgYnJva2VuSW1hZ2UtPndpZHRoKCkgKiBzdHlsZSgpLT5lZmZlY3RpdmVab29tKCksIHBhZGRp bmdIZWlnaHQgKyBicm9rZW5JbWFnZS0+aGVpZ2h0KCkgKiBzdHlsZSgpLT5lZmZlY3RpdmVab29t KCkpOworICAgIHJldHVybiBJbnRTaXplKHBhZGRpbmdXaWR0aCArIGltYWdlU2l6ZS53aWR0aCgp ICogc3R5bGUoKS0+ZWZmZWN0aXZlWm9vbSgpLCBwYWRkaW5nSGVpZ2h0ICsgaW1hZ2VTaXplLmhl aWdodCgpICogc3R5bGUoKS0+ZWZmZWN0aXZlWm9vbSgpKTsKIH0KIAogLy8gU2V0cyB0aGUgaW1h Z2UgaGVpZ2h0IGFuZCB3aWR0aCB0byBmaXQgdGhlIGFsdCB0ZXh0LiAgUmV0dXJucyB0cnVlIGlm IHRoZQo=