| Summary: | Use high resolution platform images when the deviceScaleFactor > 1 | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Beth Dakin <bdakin> | ||||||||||||
| Component: | Layout and Rendering | Assignee: | Beth Dakin <bdakin> | ||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||
| Severity: | Normal | CC: | aroben, bdakin, dglazkov, rniwa, webkit.review.bot | ||||||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||
| Hardware: | Unspecified | ||||||||||||||
| OS: | Unspecified | ||||||||||||||
| Bug Depends on: | |||||||||||||||
| Bug Blocks: | 67898 | ||||||||||||||
| Attachments: |
|
||||||||||||||
|
Description
Beth Dakin
2011-09-08 16:48:50 PDT
Created attachment 106816 [details]
Patch
Attachment 106816 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/loader/cache/CachedImage.cpp:123: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5]
Total errors found: 1 in 6 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 106818 [details]
Patch with style fix
Comment on attachment 106818 [details] Patch with style fix Attachment 106818 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/9624432 Comment on attachment 106818 [details] Patch with style fix Attachment 106818 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/9623495 Comment on attachment 106818 [details] Patch with style fix View in context: https://bugs.webkit.org/attachment.cgi?id=106818&action=review > Source/WebCore/rendering/RenderLayer.cpp:2467 > + cornerResizerWidth = resizeCornerImageHiRes->width() / 2; > + cornerResizerHeight = resizeCornerImageHiRes->height() / 2; Isn't this assuming that a non-1 deviceScaleFactor is always 2? Comment on attachment 106818 [details] Patch with style fix Attachment 106818 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9624597 New failing tests: fast/encoding/utf-16-big-endian.html fast/text/whitespace/normal-after-nowrap-breaking.html tables/mozilla/bugs/bug215629.html tables/mozilla/bugs/bug3977.html fast/encoding/utf-16-little-endian.html (In reply to comment #6) > (From update of attachment 106818 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=106818&action=review > > > Source/WebCore/rendering/RenderLayer.cpp:2467 > > + cornerResizerWidth = resizeCornerImageHiRes->width() / 2; > > + cornerResizerHeight = resizeCornerImageHiRes->height() / 2; > > Isn't this assuming that a non-1 deviceScaleFactor is always 2? Ah, yes that's dumb. I can just divide by the devicePixelRatio and that will actually be the correct thing. I will post a new patch that does that and attempts to fix the broken builds shortly. Comment on attachment 106818 [details] Patch with style fix View in context: https://bugs.webkit.org/attachment.cgi?id=106818&action=review Do we have code that will cause us to update to the right image when the page's device scale factor changes? > Source/WebCore/WebCore.xcodeproj/project.pbxproj:-575 > - 1C14E76B0AD8C81C00B6158B /* deleteButtonPressed.tiff in Resources */ = {isa = PBXBuildFile; fileRef = 1C14E7690AD8C81C00B6158B /* deleteButtonPressed.tiff */; }; > - 1C14E76C0AD8C81C00B6158B /* deleteButton.tiff in Resources */ = {isa = PBXBuildFile; fileRef = 1C14E76A0AD8C81C00B6158B /* deleteButton.tiff */; }; Are these files still used by other ports? Or can we delete them? > Source/WebCore/editing/DeleteButtonController.cpp:244 > + unsigned deviceScaleFactor = 1; Since we use float everywhere else, I'd recommend using it here, too. > Source/WebCore/editing/DeleteButtonController.cpp:252 > + RefPtr<Image> buttonImage; > + if (deviceScaleFactor == 1) > + buttonImage = Image::loadPlatformResource("deleteButton"); > + else > + buttonImage = Image::loadPlatformResource("deleteButton@2x"); I think our normal practice has been to use @2x images when the scale factor is >= 2. So I'd write it like this: if (deviceScaleFactor >= 2) use @2x else use normal image > Source/WebCore/loader/cache/CachedImage.cpp:117 > +static Image* brokenImage(unsigned deviceScaleFactor) Again, I think you should use float here. > Source/WebCore/loader/cache/CachedImage.cpp:122 > + // If deviceScaleFactor is 0, that means we couldn't access the Page through m_request this time, > + // so use the lastDeviceScaleFactor to determine the appropriate resolution. In what kinds of cases will we be unable to get to a Page? I think you can make the logic in this function simpler by doing this: static float lastDeviceScaleFactor = 1; if (!deviceScaleFactor) deviceScaleFactor = lastDeviceScaleFactor; else lastDeviceScaleFactor = deviceScaleFactor; // From here on you just use deviceScaleFactor and never mention lastDeviceScaleFactor again. > Source/WebCore/loader/cache/CachedImage.cpp:123 > + if (deviceScaleFactor == 1 || (!deviceScaleFactor && lastDeviceScaleFactor == 1)) { Again, I think you should use the >= 2 logic as above. > Source/WebCore/loader/cache/CachedImage.cpp:125 > + DEFINE_STATIC_LOCAL(RefPtr<Image>, brokenImageLoRes, (Image::loadPlatformResource("missingImage"))); You can make this a bare pointer if you add a .leakRef() to the loadPlatformResource expression. > Source/WebCore/rendering/RenderLayer.cpp:2458 > + if (deviceScaleFactor == 1) { Again, I recommend you use the >= 2 logic as above. >>> Source/WebCore/rendering/RenderLayer.cpp:2467 >>> + cornerResizerHeight = resizeCornerImageHiRes->height() / 2; >> >> Isn't this assuming that a non-1 deviceScaleFactor is always 2? > > Ah, yes that's dumb. I can just divide by the devicePixelRatio and that will actually be the correct thing. I will post a new patch that does that and attempts to fix the broken builds shortly. If we were using IntSize instead of two ints you could even use cornerResizerSize.scale(1 / devicePixelRatio). (In reply to comment #9) > (From update of attachment 106818 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=106818&action=review > > Do we have code that will cause us to update to the right image when the page's device scale factor changes? There are some follow-up bugs in this area. The RenderLayer resize corner image does everything correctly, so no follow-up bugs there. The delete button does not update correctly when the scale factor changes, so that's follow-up bug #1. And the high-res broken-image image always draws at 2x the size it should, so that's follow-up #2. > > > Source/WebCore/WebCore.xcodeproj/project.pbxproj:-575 > > - 1C14E76B0AD8C81C00B6158B /* deleteButtonPressed.tiff in Resources */ = {isa = PBXBuildFile; fileRef = 1C14E7690AD8C81C00B6158B /* deleteButtonPressed.tiff */; }; > > - 1C14E76C0AD8C81C00B6158B /* deleteButton.tiff in Resources */ = {isa = PBXBuildFile; fileRef = 1C14E76A0AD8C81C00B6158B /* deleteButton.tiff */; }; > > Are these files still used by other ports? Or can we delete them? > Not sure. I thought I would be conservative for now and just leave them around. I will do a quick grep to see if I can answer that though. > > Source/WebCore/editing/DeleteButtonController.cpp:244 > > + unsigned deviceScaleFactor = 1; > > Since we use float everywhere else, I'd recommend using it here, too. > Ah, silly. Will fix. > > Source/WebCore/editing/DeleteButtonController.cpp:252 > > + RefPtr<Image> buttonImage; > > + if (deviceScaleFactor == 1) > > + buttonImage = Image::loadPlatformResource("deleteButton"); > > + else > > + buttonImage = Image::loadPlatformResource("deleteButton@2x"); > > I think our normal practice has been to use @2x images when the scale factor is >= 2. So I'd write it like this: > > if (deviceScaleFactor >= 2) > use @2x > else > use normal image > Sounds good. Will fix. > > Source/WebCore/loader/cache/CachedImage.cpp:117 > > +static Image* brokenImage(unsigned deviceScaleFactor) > > Again, I think you should use float here. > Silly again. Will fix. > > Source/WebCore/loader/cache/CachedImage.cpp:122 > > + // If deviceScaleFactor is 0, that means we couldn't access the Page through m_request this time, > > + // so use the lastDeviceScaleFactor to determine the appropriate resolution. > > In what kinds of cases will we be unable to get to a Page? > I never actually set a breakpoint to get a backtrace, but I was doing printf debugging and saw that this function would be called about 4 times on any load or re-load of the page. The first 3 times there was a Page. The last, there was never a Page. > I think you can make the logic in this function simpler by doing this: > > static float lastDeviceScaleFactor = 1; > > if (!deviceScaleFactor) > deviceScaleFactor = lastDeviceScaleFactor; > else > lastDeviceScaleFactor = deviceScaleFactor; > > // From here on you just use deviceScaleFactor and never mention lastDeviceScaleFactor again. > Okay. > > Source/WebCore/loader/cache/CachedImage.cpp:123 > > + if (deviceScaleFactor == 1 || (!deviceScaleFactor && lastDeviceScaleFactor == 1)) { > > Again, I think you should use the >= 2 logic as above. > Will do. > > Source/WebCore/loader/cache/CachedImage.cpp:125 > > + DEFINE_STATIC_LOCAL(RefPtr<Image>, brokenImageLoRes, (Image::loadPlatformResource("missingImage"))); > > You can make this a bare pointer if you add a .leakRef() to the loadPlatformResource expression. > Will do. > > Source/WebCore/rendering/RenderLayer.cpp:2458 > > + if (deviceScaleFactor == 1) { > > Again, I recommend you use the >= 2 logic as above. > Will do. > >>> Source/WebCore/rendering/RenderLayer.cpp:2467 > >>> + cornerResizerHeight = resizeCornerImageHiRes->height() / 2; > >> > >> Isn't this assuming that a non-1 deviceScaleFactor is always 2? > > > > Ah, yes that's dumb. I can just divide by the devicePixelRatio and that will actually be the correct thing. I will post a new patch that does that and attempts to fix the broken builds shortly. > > If we were using IntSize instead of two ints you could even use cornerResizerSize.scale(1 / devicePixelRatio). That works! Created attachment 106897 [details]
Updated Patch
Comment on attachment 106897 [details] Updated Patch Attachment 106897 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/9622941 Comment on attachment 106818 [details] Patch with style fix View in context: https://bugs.webkit.org/attachment.cgi?id=106818&action=review >>> Source/WebCore/loader/cache/CachedImage.cpp:122 >>> + // so use the lastDeviceScaleFactor to determine the appropriate resolution. >> >> In what kinds of cases will we be unable to get to a Page? >> >> I think you can make the logic in this function simpler by doing this: >> >> static float lastDeviceScaleFactor = 1; >> >> if (!deviceScaleFactor) >> deviceScaleFactor = lastDeviceScaleFactor; >> else >> lastDeviceScaleFactor = deviceScaleFactor; >> >> // From here on you just use deviceScaleFactor and never mention lastDeviceScaleFactor again. > > I never actually set a breakpoint to get a backtrace, but I was doing printf debugging and saw that this function would be called about 4 times on any load or re-load of the page. The first 3 times there was a Page. The last, there was never a Page. It would be nice to know why it's called so much. Maybe we can call it less. Using a static for this isn't super-great, since if two Pages with different device scale factors are loading concurrently you could end up with the calls to brokenImage() from each Page being interleaved, which could eventually result in the wrong image being shown in a Page. Comment on attachment 106897 [details] Updated Patch View in context: https://bugs.webkit.org/attachment.cgi?id=106897&action=review Is there any way to write a test for this? Perhaps a pixel test that uses your setCustomBackingScaleFactor API? > Source/WebCore/loader/cache/CachedImage.cpp:126 > + if (!deviceScaleFactor) > + deviceScaleFactor = lastDeviceScaleFactor; > + else > + lastDeviceScaleFactor = deviceScaleFactor; Even though I suggested it, I guess the "else" technically isn't needed here. I can't decide whether it's clearer with it or without it! > Source/WebCore/loader/cache/CachedImage.cpp:148 > + float deviceScaleFactor = 0; > + Frame* frame = m_request ? m_request->cachedResourceLoader()->frame() : 0; > + if (frame) { > + if (Page* page = frame->page()) > + deviceScaleFactor = page->deviceScaleFactor(); > + } It would be a little clearer to split this logic out into a helper function. > Source/WebCore/rendering/RenderLayer.cpp:2461 > + DEFINE_STATIC_LOCAL(RefPtr<Image>, resizeCornerImageHiRes, (Image::loadPlatformResource("textAreaResizeCorner@2x"))); > + resizeCornerImage = resizeCornerImageHiRes; > + } else { > + DEFINE_STATIC_LOCAL(RefPtr<Image>, resizeCornerImageLoRes, (Image::loadPlatformResource("textAreaResizeCorner"))); You can use bare pointers here, too, using the same trick as in CachedImage.cpp. > Source/WebCore/rendering/RenderLayer.cpp:2468 > + IntRect imageRect(absRect.maxX() - cornerResizerSize.width(), absRect.maxY() - cornerResizerSize.height(), cornerResizerSize.width(), cornerResizerSize.height()); I think this can be further simplified so: IntRect imageRect(absRect.maxXmaxYCorner() - cornerResizerSize, cornerResizerSize); (In reply to comment #13) > > It would be nice to know why it's called so much. Maybe we can call it less. > > Using a static for this isn't super-great, since if two Pages with different device scale factors are loading concurrently you could end up with the calls to brokenImage() from each Page being interleaved, which could eventually result in the wrong image being shown in a Page. Okay, so I debugged this a bit. As you may recall from the patch, the Page is accessed through m_request. When m_request is non-null, we can always get to Page (in my debugging anyway). When it is null, my patch doesn't do anything else to get to Page, and I don't think there is another way there with what is available in the class. So, when is m_request null and when is it non-null you ask? It seems to be non-null in the first bunch of calls to CachedImage::image() because those happen through the loader while the load is still happening. Once the load is complete, m_request is cleared. But then CachedImage::image() is accessed through painting code paths, and that's when we can't get to Page because m_request is null. (Specifically, RenderImage::paintReplaced() calls RenderImageResource::image() which calls CachedImage::image().) To improve this, I can see stashing the imageScaleFactor as a member variable on CachedImage. It would represent the CachedImage's current deviceScaleFactor, and it could update itself in CachedImage::image() if the Page has a different deviceScaleFactor. Of course this has some limitations still. And it's a member variable that's only currently relevant to the brokenImage-image, which is kind of strange. But it might come in handy fixing the follow-up bug I mentioned before (where the broken-image-image in hi-res draws at double the size it should). We could also have CachedImage::image() take the deviceScaleFactor as an optional parameter. Thoughts? (In reply to comment #14) > (From update of attachment 106897 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=106897&action=review > > Is there any way to write a test for this? Perhaps a pixel test that uses your setCustomBackingScaleFactor API? > > > Source/WebCore/loader/cache/CachedImage.cpp:126 > > + if (!deviceScaleFactor) > > + deviceScaleFactor = lastDeviceScaleFactor; > > + else > > + lastDeviceScaleFactor = deviceScaleFactor; > > Even though I suggested it, I guess the "else" technically isn't needed here. I can't decide whether it's clearer with it or without it! > Haha, I considered removing it too! Maybe it is clearer without it… I'll think on it a little more. > > Source/WebCore/loader/cache/CachedImage.cpp:148 > > + float deviceScaleFactor = 0; > > + Frame* frame = m_request ? m_request->cachedResourceLoader()->frame() : 0; > > + if (frame) { > > + if (Page* page = frame->page()) > > + deviceScaleFactor = page->deviceScaleFactor(); > > + } > > It would be a little clearer to split this logic out into a helper function. > Okay! > > Source/WebCore/rendering/RenderLayer.cpp:2461 > > + DEFINE_STATIC_LOCAL(RefPtr<Image>, resizeCornerImageHiRes, (Image::loadPlatformResource("textAreaResizeCorner@2x"))); > > + resizeCornerImage = resizeCornerImageHiRes; > > + } else { > > + DEFINE_STATIC_LOCAL(RefPtr<Image>, resizeCornerImageLoRes, (Image::loadPlatformResource("textAreaResizeCorner"))); > > You can use bare pointers here, too, using the same trick as in CachedImage.cpp. > Ah, right. > > Source/WebCore/rendering/RenderLayer.cpp:2468 > > + IntRect imageRect(absRect.maxX() - cornerResizerSize.width(), absRect.maxY() - cornerResizerSize.height(), cornerResizerSize.width(), cornerResizerSize.height()); > > I think this can be further simplified so: > > IntRect imageRect(absRect.maxXmaxYCorner() - cornerResizerSize, cornerResizerSize); Cool. Will do. (In reply to comment #15) > (In reply to comment #13) > > > > It would be nice to know why it's called so much. Maybe we can call it less. > > > > Using a static for this isn't super-great, since if two Pages with different device scale factors are loading concurrently you could end up with the calls to brokenImage() from each Page being interleaved, which could eventually result in the wrong image being shown in a Page. > > Okay, so I debugged this a bit. As you may recall from the patch, the Page is accessed through m_request. When m_request is non-null, we can always get to Page (in my debugging anyway). When it is null, my patch doesn't do anything else to get to Page, and I don't think there is another way there with what is available in the class. > > So, when is m_request null and when is it non-null you ask? It seems to be non-null in the first bunch of calls to CachedImage::image() because those happen through the loader while the load is still happening. Once the load is complete, m_request is cleared. But then CachedImage::image() is accessed through painting code paths, and that's when we can't get to Page because m_request is null. (Specifically, RenderImage::paintReplaced() calls RenderImageResource::image() which calls CachedImage::image().) Thanks for debugging this! > We could also have CachedImage::image() take the deviceScaleFactor as an optional parameter. That sounds like it might be the cleanest option, if the caller always has access to the device scale factor. (In reply to comment #17) > > Thanks for debugging this! > > > We could also have CachedImage::image() take the deviceScaleFactor as an optional parameter. > > That sounds like it might be the cleanest option, if the caller always has access to the device scale factor. I think you are right. I will do this now. Created attachment 106921 [details]
More patch
Comment on attachment 106921 [details] More patch View in context: https://bugs.webkit.org/attachment.cgi?id=106921&action=review Looking pretty close! I still think it would be good to have a test case for this. > Source/WebCore/editing/DeleteButtonController.cpp:250 > + RefPtr<Image> buttonImage; > + if (deviceScaleFactor >= 2) > + buttonImage = Image::loadPlatformResource("deleteButton@2x"); Is it a problem that this code is compiled on all platforms but only Mac has the @2x images for now? > Source/WebCore/loader/cache/CachedImage.h:47 > + Image* image(float deviceScaleFactor = 1) const; // Returns the nullImage() if the image is not available yet. How many callers would have to change if this were made non-optional? Having it be optional makes it seem easy to introduce bugs by forgetting to pass a scale factor. > Source/WebCore/rendering/RenderImage.cpp:88 > + if (frame) { > + if (Page* page = frame->page()) > + deviceScaleFactor = page->deviceScaleFactor(); > + } This might be clearer if it used early returns. > Source/WebCore/rendering/RenderImageResource.cpp:105 > + float deviceScaleFactor = 1; > + if (m_renderer) { > + if (Frame* frame = m_renderer->frame()) { > + if (Page* page = frame->page()) > + deviceScaleFactor = page->deviceScaleFactor(); > + } > + } This might be clearer as a helper function. Comment on attachment 106921 [details] More patch View in context: https://bugs.webkit.org/attachment.cgi?id=106921&action=review > Source/WebCore/rendering/RenderImage.cpp:101 > + return IntSize(paddingWidth + newImage->image(scaleFactor)->width() * style()->effectiveZoom(), paddingHeight + newImage->image(scaleFactor)->height() * style()->effectiveZoom()); Seems like you could just grab brokenImage directly, and then you wouldn't have to patch the generic image accessor. > Source/WebCore/rendering/RenderImage.cpp:109 > + if (newImage && newImage->image(deviceScaleFactor(frame()))) Scale factor here doesn't matter does it? You're just null checking? Comment on attachment 106921 [details] More patch View in context: https://bugs.webkit.org/attachment.cgi?id=106921&action=review > Source/WebCore/rendering/RenderLayer.cpp:2469 > + float deviceScaleFactor = 1; > + if (Frame* frame = renderer()->frame()) { > + if (Page* page = frame->page()) > + deviceScaleFactor = page->deviceScaleFactor(); > + } > + > + RefPtr<Image> resizeCornerImage; > + if (deviceScaleFactor >= 2) { > + DEFINE_STATIC_LOCAL(Image*, resizeCornerImageHiRes, (Image::loadPlatformResource("textAreaResizeCorner@2x").leakRef())); > + resizeCornerImage = resizeCornerImageHiRes; > + } else { > + DEFINE_STATIC_LOCAL(Image*, resizeCornerImageLoRes, (Image::loadPlatformResource("textAreaResizeCorner").leakRef())); > + resizeCornerImage = resizeCornerImageLoRes; > + } > + > + IntSize cornerResizerSize = resizeCornerImage->size(); > + cornerResizerSize.scale(1 / deviceScaleFactor); > + > + IntRect imageRect(absRect.maxXMaxYCorner() - cornerResizerSize, cornerResizerSize); > + context->drawImage(resizeCornerImage.get(), box->style()->colorSpace(), imageRect); Also, pull this into a helper now that it's gotten this big. Comment on attachment 106921 [details] More patch Attachment 106921 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/9624872 Created attachment 106942 [details]
Another new patch
This patch should address all of Adam's and Hyatt's comments except for the request for a test.
Comment on attachment 106942 [details] Another new patch View in context: https://bugs.webkit.org/attachment.cgi?id=106942&action=review > Source/WebCore/loader/cache/CachedImage.cpp:124 > + if (deviceScaleFactor >= 2) { > + DEFINE_STATIC_LOCAL(Image*, brokenImageHiRes, (Image::loadPlatformResource("missingImage@2x").leakRef())); > + return brokenImageHiRes; > + } > + > + DEFINE_STATIC_LOCAL(Image*, brokenImageLoRes, (Image::loadPlatformResource("missingImage").leakRef())); > + return brokenImageLoRes; I think we need a comment here explaining that these two images need to be the same size, because the 1x image will be used for layout even if the 2x image is the one actually drawn. > Source/WebCore/loader/cache/CachedImage.cpp:132 > - return brokenImage(); > + return brokenImage(1); Why is this 1 OK? Don’t we need the correct image? I’m guessing it’s OK because of the code in RenderReplaced::paintReplaced. If that’s so, then a comment is required here to explain that. I think we are in a situation where there is an image returned by this function that simply indicates “this is a broken image, not intended to be drawn”. It might be better to be clearer about that and even report an error if we draw it, rather than just always using the 1x image for that purpose? > Source/WebCore/loader/cache/CachedImage.h:50 > + Image* brokenImage(float deviceScaleFactor = 1) const; Why is the default still here? > Source/WebCore/rendering/RenderImage.cpp:82 > +static float deviceScaleFactor(Frame* frame) Too bad we can’t share this with RenderLayer::drawPlatformResizerImage. Maybe we can find a place to put this where it can be shared. > Source/WebCore/rendering/RenderImage.cpp:84 > + if (!frame) Can frame be 0 here? If not, can we leave this check out? > Source/WebCore/rendering/RenderImage.cpp:101 > + return IntSize(paddingWidth + newImage->brokenImage(scaleFactor)->width() * style()->effectiveZoom(), paddingHeight + newImage->brokenImage(scaleFactor)->height() * style()->effectiveZoom()); Seems unfortunate to fetch the image twice. A local variable would be better. The old code was different because image() was just an inlined getter function. > Source/WebCore/rendering/RenderLayer.cpp:2430 > + if (Frame* frame = renderer()->frame()) { Can frame be 0 here? If not, can we leave this check out? > Source/WebCore/rendering/RenderLayer.cpp:2445 > + RefPtr<Image> resizeCornerImage; > + if (deviceScaleFactor >= 2) { > + DEFINE_STATIC_LOCAL(Image*, resizeCornerImageHiRes, (Image::loadPlatformResource("textAreaResizeCorner@2x").leakRef())); > + resizeCornerImage = resizeCornerImageHiRes; > + } else { > + DEFINE_STATIC_LOCAL(Image*, resizeCornerImageLoRes, (Image::loadPlatformResource("textAreaResizeCorner").leakRef())); > + resizeCornerImage = resizeCornerImageLoRes; > + } > + > + IntSize cornerResizerSize = resizeCornerImage->size(); > + cornerResizerSize.scale(1 / deviceScaleFactor); This seems too generally written. The 2X image’s size should be divided by 2, and the 1-pixel image not divided at all. There’s no good reason to do the math with the actual scale factor. Also, rounding to an integer would be a problem in the general case. (In reply to comment #25) > (From update of attachment 106942 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=106942&action=review > > > Source/WebCore/loader/cache/CachedImage.cpp:124 > > + if (deviceScaleFactor >= 2) { > > + DEFINE_STATIC_LOCAL(Image*, brokenImageHiRes, (Image::loadPlatformResource("missingImage@2x").leakRef())); > > + return brokenImageHiRes; > > + } > > + > > + DEFINE_STATIC_LOCAL(Image*, brokenImageLoRes, (Image::loadPlatformResource("missingImage").leakRef())); > > + return brokenImageLoRes; > > I think we need a comment here explaining that these two images need to be the same size, because the 1x image will be used for layout even if the 2x image is the one actually drawn. > I'm not sure I agree that is necessary here. There is no similar comment in the other places where we load a 2X image. > > Source/WebCore/loader/cache/CachedImage.cpp:132 > > - return brokenImage(); > > + return brokenImage(1); > > Why is this 1 OK? Don’t we need the correct image? I’m guessing it’s OK because of the code in RenderReplaced::paintReplaced. If that’s so, then a comment is required here to explain that. > I think you are right about a comment being in order. > I think we are in a situation where there is an image returned by this function that simply indicates “this is a broken image, not intended to be drawn”. It might be better to be clearer about that and even report an error if we draw it, rather than just always using the 1x image for that purpose? > Hyatt thinks it's weird that the broken image is entangled in all of this anyway. He didn't want CachedImage::image() to take a parameter for the deviceScaleFactor, which is why I re-wrote the code this way. The code could fetch the deviceScaleFactor from the Page itself, but it would only get it right sometimes, which Adam didn't care for. I'm feeling a bit of strain here from 3 reviewers wanting 3 different things. I feel like I am being pulled in many directions here. > > Source/WebCore/loader/cache/CachedImage.h:50 > > + Image* brokenImage(float deviceScaleFactor = 1) const; > > Why is the default still here? It shouldn't be. I will remove this. > > > Source/WebCore/rendering/RenderImage.cpp:82 > > +static float deviceScaleFactor(Frame* frame) > > Too bad we can’t share this with RenderLayer::drawPlatformResizerImage. Maybe we can find a place to put this where it can be shared. > I agree that would be nice. > > Source/WebCore/rendering/RenderImage.cpp:84 > > + if (!frame) > > Can frame be 0 here? If not, can we leave this check out? > Frame is often null-checked in rendering code. If frame can be null during painting or loading, then this check needs to be here. Other painting code null-checks Frame, but I am not sure it needs to. > > Source/WebCore/rendering/RenderImage.cpp:101 > > + return IntSize(paddingWidth + newImage->brokenImage(scaleFactor)->width() * style()->effectiveZoom(), paddingHeight + newImage->brokenImage(scaleFactor)->height() * style()->effectiveZoom()); > > Seems unfortunate to fetch the image twice. A local variable would be better. The old code was different because image() was just an inlined getter function. > The old image function was not an inlined getter. Please see CachedImage::image(). > > Source/WebCore/rendering/RenderLayer.cpp:2430 > > + if (Frame* frame = renderer()->frame()) { > > Can frame be 0 here? If not, can we leave this check out? > Same comment as above. > > Source/WebCore/rendering/RenderLayer.cpp:2445 > > + RefPtr<Image> resizeCornerImage; > > + if (deviceScaleFactor >= 2) { > > + DEFINE_STATIC_LOCAL(Image*, resizeCornerImageHiRes, (Image::loadPlatformResource("textAreaResizeCorner@2x").leakRef())); > > + resizeCornerImage = resizeCornerImageHiRes; > > + } else { > > + DEFINE_STATIC_LOCAL(Image*, resizeCornerImageLoRes, (Image::loadPlatformResource("textAreaResizeCorner").leakRef())); > > + resizeCornerImage = resizeCornerImageLoRes; > > + } > > + > > + IntSize cornerResizerSize = resizeCornerImage->size(); > > + cornerResizerSize.scale(1 / deviceScaleFactor); > > This seems too generally written. The 2X image’s size should be divided by 2, and the 1-pixel image not divided at all. There’s no good reason to do the math with the actual scale factor. Also, rounding to an integer would be a problem in the general case. Adam requested using the actual scale factor instead of 2, which I did in my original patch. As I expressed earlier, this patch is feeling very over-reviewed to me at this point. Comment on attachment 106942 [details] Another new patch View in context: https://bugs.webkit.org/attachment.cgi?id=106942&action=review >>> Source/WebCore/loader/cache/CachedImage.cpp:124 >>> + return brokenImageLoRes; >> >> I think we need a comment here explaining that these two images need to be the same size, because the 1x image will be used for layout even if the 2x image is the one actually drawn. > > I'm not sure I agree that is necessary here. There is no similar comment in the other places where we load a 2X image. As I understand it, in the other places we just use the 2x image. But in this case, the layout code uses the 1x image and then drawing code substitutes the 2x image. That's a bit different. But maybe a comment will just be confusing. >>> Source/WebCore/rendering/RenderImage.cpp:82 >>> +static float deviceScaleFactor(Frame* frame) >> >> Too bad we can’t share this with RenderLayer::drawPlatformResizerImage. Maybe we can find a place to put this where it can be shared. > > I agree that would be nice. I’m thinking that Page.h would be a reasonable place for the helper function. >>> Source/WebCore/rendering/RenderImage.cpp:101 >>> + return IntSize(paddingWidth + newImage->brokenImage(scaleFactor)->width() * style()->effectiveZoom(), paddingHeight + newImage->brokenImage(scaleFactor)->height() * style()->effectiveZoom()); >> >> Seems unfortunate to fetch the image twice. A local variable would be better. The old code was different because image() was just an inlined getter function. > > The old image function was not an inlined getter. Please see CachedImage::image(). OK, then even the old code should have used a local variable! >>> Source/WebCore/rendering/RenderLayer.cpp:2445 >>> + cornerResizerSize.scale(1 / deviceScaleFactor); >> >> This seems too generally written. The 2X image’s size should be divided by 2, and the 1-pixel image not divided at all. There’s no good reason to do the math with the actual scale factor. Also, rounding to an integer would be a problem in the general case. > > Adam requested using the actual scale factor instead of 2, which I did in my original patch. > > As I expressed earlier, this patch is feeling very over-reviewed to me at this point. I missed your over-reviewed comment; I’m not sure I agree. If Adam suggested that, then I believe he was wrong. 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. (In reply to comment #27) > (From update of attachment 106942 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=106942&action=review > > >>> Source/WebCore/loader/cache/CachedImage.cpp:124 > >>> + return brokenImageLoRes; > >> > >> I think we need a comment here explaining that these two images need to be the same size, because the 1x image will be used for layout even if the 2x image is the one actually drawn. > > > > I'm not sure I agree that is necessary here. There is no similar comment in the other places where we load a 2X image. > > As I understand it, in the other places we just use the 2x image. But in this case, the layout code uses the 1x image and then drawing code substitutes the 2x image. That's a bit different. But maybe a comment will just be confusing. > I don't think that is accurate. Based on my (admittedly limited) debugging, the 1x image is only being returned for things like null-checks. > >>> Source/WebCore/rendering/RenderImage.cpp:82 > >>> +static float deviceScaleFactor(Frame* frame) > >> > >> Too bad we can’t share this with RenderLayer::drawPlatformResizerImage. Maybe we can find a place to put this where it can be shared. > > > > I agree that would be nice. > > I’m thinking that Page.h would be a reasonable place for the helper function. > Okay! I will add that. > >>> Source/WebCore/rendering/RenderImage.cpp:101 > >>> + return IntSize(paddingWidth + newImage->brokenImage(scaleFactor)->width() * style()->effectiveZoom(), paddingHeight + newImage->brokenImage(scaleFactor)->height() * style()->effectiveZoom()); > >> > >> Seems unfortunate to fetch the image twice. A local variable would be better. The old code was different because image() was just an inlined getter function. > > > > The old image function was not an inlined getter. Please see CachedImage::image(). > > OK, then even the old code should have used a local variable! > That I can agree with! > >>> Source/WebCore/rendering/RenderLayer.cpp:2445 > >>> + cornerResizerSize.scale(1 / deviceScaleFactor); > >> > >> This seems too generally written. The 2X image’s size should be divided by 2, and the 1-pixel image not divided at all. There’s no good reason to do the math with the actual scale factor. Also, rounding to an integer would be a problem in the general case. > > > > Adam requested using the actual scale factor instead of 2, which I did in my original patch. > > > > As I expressed earlier, this patch is feeling very over-reviewed to me at this point. > > I missed your over-reviewed comment; I’m not sure I agree. > You are probably right. It can just be frustrating when reviewers have conflicting suggestions. Especially when I feel a strong case could be made for either side and there is not an obviously better choice. Anyway, I do feel like the patch has continued to get better, so as I initially said…you are probably right. > If Adam suggested that, then I believe he was wrong. 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. Yeah, I hear that. Back to 2 it is! Thanks for the review. Comment on attachment 106942 [details] Another new patch Attachment 106942 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/9625871 Comment on attachment 106942 [details] Another new patch Attachment 106942 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/9632054 Sorry for sending things in the wrong direction here. I'm glad Hyatt and Darin and Simon have been looking at this; they are more qualified to review a patch like this than I am. (In reply to comment #31) > Sorry for sending things in the wrong direction here. I'm glad Hyatt and Darin and Simon have been looking at this; they are more qualified to review a patch like this than I am. No, you were really very helpful Adam!! Reconciling all of the different opinions was challenging for me, but in the end, I am glad to have them. I committed the change with revision 94900. Follow-up bugs: https://bugs.webkit.org/show_bug.cgi?id=67884 Delete button icon does not properly update when the device resolution changes dynamically https://bugs.webkit.org/show_bug.cgi?id=67885 Outline for the high-resolution broken image icon draws at 2x Adam, I promise to add test infrastructure with those patches as well! And this is a follow-up for Chrome folks: https://bugs.webkit.org/show_bug.cgi?id=67886 Chromium: Need to add high resolution platform images to Chrome WebCore bundle 10 tests started failing on Snow Leopard after this patch: http://build.webkit.org/results/SnowLeopard%20Intel%20Debug%20(Tests)/r94900%20(2089)/results.html http://build.webkit.org/results/SnowLeopard%20Intel%20Release%20(Tests)/r94900%20(32987)/results.html And that's because you've added a printf:
2417 void RenderLayer::drawPlatformResizerImage(GraphicsContext* context, LayoutRect resizerCornerRect)
2418 {
2419 float deviceScaleFactor = Page::deviceScaleFactor(renderer()->frame());
2420 printf("RenderLayer deviceScaleFactor=%f\n", deviceScaleFactor);
Removed printf in http://trac.webkit.org/changeset/94910 for now. fast/images/support-broken-image-delegate.html is still failing on Leopard, Snow Leopard, & Lion: http://build.webkit.org/results/SnowLeopard%20Intel%20Debug%20(Tests)/r94911%20(2096)/results.html http://build.webkit.org/results/Leopard%20Intel%20Debug%20(Tests)/r94909%20(33500)/results.html http://build.webkit.org/results/Lion%20Intel%20Debug%20(Tests)/r94910%20(535)/results.html Per Darin's suggestion, I've filed https://bugs.webkit.org/show_bug.cgi?id=67898. I'm going to temporarily skip the test on the bots for the sake of keeping them green while Beth & Darin investigate the issue. |