Clean up RenderImage and a RenderImageResource function
authorsimon.fraser@apple.com <simon.fraser@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 4 Mar 2017 01:45:39 +0000 (01:45 +0000)
committersimon.fraser@apple.com <simon.fraser@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 4 Mar 2017 01:45:39 +0000 (01:45 +0000)
https://bugs.webkit.org/show_bug.cgi?id=169153

Reviewed by Zalan Bujtas.

Change all calls to imageResource().cachedImage() in RenderImage to use the inline
cachedImage() function.

In RenderImage::paintReplaced(), early return after the broken image block (and no need
to test imageResource().hasImage() again in the second condition). Convert height/width to size,
which also forces us to be explicit about using flooredIntSize() when fetching the image
(perhaps this should be a roundedIntSize, but I didn't want to change behavior).

Change RenderImageResource::image() to take an IntSize, rather than int height and width.

No behavior change.

* rendering/RenderImage.cpp:
(WebCore::RenderImage::styleDidChange):
(WebCore::RenderImage::imageChanged):
(WebCore::RenderImage::notifyFinished):
(WebCore::RenderImage::paintReplaced):
(WebCore::RenderImage::paintIntoRect):
(WebCore::RenderImage::foregroundIsKnownToBeOpaqueInRect):
(WebCore::RenderImage::embeddedContentBox):
* rendering/RenderImageResource.cpp:
(WebCore::RenderImageResource::image):
* rendering/RenderImageResource.h:
(WebCore::RenderImageResource::image):
* rendering/RenderImageResourceStyleImage.cpp:
(WebCore::RenderImageResourceStyleImage::image):
* rendering/RenderImageResourceStyleImage.h:

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@213404 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WebCore/ChangeLog
Source/WebCore/rendering/RenderImage.cpp
Source/WebCore/rendering/RenderImageResource.cpp
Source/WebCore/rendering/RenderImageResource.h
Source/WebCore/rendering/RenderImageResourceStyleImage.cpp
Source/WebCore/rendering/RenderImageResourceStyleImage.h

index b06d25cac94c9717e3c6aafb2264aee1928f81ce..4958c092790946f8941a84249ed4291cf3fba7e1 100644 (file)
@@ -1,3 +1,38 @@
+2017-03-03  Simon Fraser  <simon.fraser@apple.com>
+
+        Clean up RenderImage and a RenderImageResource function
+        https://bugs.webkit.org/show_bug.cgi?id=169153
+
+        Reviewed by Zalan Bujtas.
+        
+        Change all calls to imageResource().cachedImage() in RenderImage to use the inline
+        cachedImage() function.
+
+        In RenderImage::paintReplaced(), early return after the broken image block (and no need
+        to test imageResource().hasImage() again in the second condition). Convert height/width to size,
+        which also forces us to be explicit about using flooredIntSize() when fetching the image
+        (perhaps this should be a roundedIntSize, but I didn't want to change behavior).
+
+        Change RenderImageResource::image() to take an IntSize, rather than int height and width.
+
+        No behavior change.
+
+        * rendering/RenderImage.cpp:
+        (WebCore::RenderImage::styleDidChange):
+        (WebCore::RenderImage::imageChanged):
+        (WebCore::RenderImage::notifyFinished):
+        (WebCore::RenderImage::paintReplaced):
+        (WebCore::RenderImage::paintIntoRect):
+        (WebCore::RenderImage::foregroundIsKnownToBeOpaqueInRect):
+        (WebCore::RenderImage::embeddedContentBox):
+        * rendering/RenderImageResource.cpp:
+        (WebCore::RenderImageResource::image):
+        * rendering/RenderImageResource.h:
+        (WebCore::RenderImageResource::image):
+        * rendering/RenderImageResourceStyleImage.cpp:
+        (WebCore::RenderImageResourceStyleImage::image):
+        * rendering/RenderImageResourceStyleImage.h:
+
 2017-03-03  Antoine Quint  <graouts@apple.com>
 
         [Modern Media Controls] Improve media documents across macOS, iPhone and iPad
index 95f18dfe3babc5d921113eb72b6f5fa28afce4ab..7a33bab8a9c92c019c4fcbb9cf54414ca353384a 100644 (file)
@@ -210,7 +210,7 @@ void RenderImage::styleDidChange(StyleDifference diff, const RenderStyle* oldSty
 {
     RenderReplaced::styleDidChange(diff, oldStyle);
     if (m_needsToSetSizeForAltText) {
-        if (!m_altText.isEmpty() && setImageSizeForAltText(imageResource().cachedImage()))
+        if (!m_altText.isEmpty() && setImageSizeForAltText(cachedImage()))
             repaintOrMarkForLayout(ImageSizeChangeForAltText);
         m_needsToSetSizeForAltText = false;
     }
@@ -257,7 +257,7 @@ void RenderImage::imageChanged(WrappedImagePtr newImage, const IntRect* rect)
             }
             return;
         }
-        imageSizeChange = setImageSizeForAltText(imageResource().cachedImage());
+        imageSizeChange = setImageSizeForAltText(cachedImage());
     }
 
     if (UNLIKELY(AXObjectCache::accessibilityEnabled())) {
@@ -342,7 +342,7 @@ void RenderImage::notifyFinished(CachedResource& newImage)
 
     invalidateBackgroundObscurationStatus();
 
-    if (&newImage == imageResource().cachedImage()) {
+    if (&newImage == cachedImage()) {
         // tell any potential compositing layers
         // that the image is done and they can reference it directly.
         contentChanged(ImageChanged);
@@ -351,12 +351,7 @@ void RenderImage::notifyFinished(CachedResource& newImage)
 
 void RenderImage::paintReplaced(PaintInfo& paintInfo, const LayoutPoint& paintOffset)
 {
-    LayoutUnit cWidth = contentWidth();
-    LayoutUnit cHeight = contentHeight();
-    LayoutUnit leftBorder = borderLeft();
-    LayoutUnit topBorder = borderTop();
-    LayoutUnit leftPad = paddingLeft();
-    LayoutUnit topPad = paddingTop();
+    LayoutSize contentSize = this->contentSize();
 
     GraphicsContext& context = paintInfo.context();
     float deviceScaleFactor = document().deviceScaleFactor();
@@ -368,35 +363,39 @@ void RenderImage::paintReplaced(PaintInfo& paintInfo, const LayoutPoint& paintOf
         if (paintInfo.phase == PaintPhaseForeground)
             page().addRelevantUnpaintedObject(this, visualOverflowRect());
 
-        if (cWidth > 2 && cHeight > 2) {
+        if (contentSize.width() > 2 && contentSize.height() > 2) {
             LayoutUnit borderWidth = LayoutUnit(1 / deviceScaleFactor);
 
+            LayoutUnit leftBorder = borderLeft();
+            LayoutUnit topBorder = borderTop();
+            LayoutUnit leftPad = paddingLeft();
+            LayoutUnit topPad = paddingTop();
+
             // Draw an outline rect where the image should be.
             context.setStrokeStyle(SolidStroke);
             context.setStrokeColor(Color::lightGray);
             context.setFillColor(Color::transparent);
-            context.drawRect(snapRectToDevicePixels(LayoutRect(paintOffset.x() + leftBorder + leftPad, paintOffset.y() + topBorder + topPad, cWidth, cHeight), deviceScaleFactor), borderWidth);
+            context.drawRect(snapRectToDevicePixels(LayoutRect({ paintOffset.x() + leftBorder + leftPad, paintOffset.y() + topBorder + topPad }, contentSize), deviceScaleFactor), borderWidth);
 
             bool errorPictureDrawn = false;
             LayoutSize imageOffset;
             // When calculating the usable dimensions, exclude the pixels of
             // the ouline rect so the error image/alt text doesn't draw on it.
-            LayoutUnit usableWidth = cWidth - 2 * borderWidth;
-            LayoutUnit usableHeight = cHeight - 2 * borderWidth;
+            LayoutSize usableSize = contentSize - LayoutSize(2 * borderWidth, 2 * borderWidth);
 
             RefPtr<Image> image = imageResource().image();
 
-            if (imageResource().errorOccurred() && !image->isNull() && usableWidth >= image->width() && usableHeight >= image->height()) {
+            if (imageResource().errorOccurred() && !image->isNull() && usableSize.width() >= image->width() && usableSize.height() >= image->height()) {
                 // Call brokenImage() explicitly to ensure we get the broken image icon at the appropriate resolution.
-                std::pair<Image*, float> brokenImageAndImageScaleFactor = imageResource().cachedImage()->brokenImage(document().deviceScaleFactor());
+                std::pair<Image*, float> brokenImageAndImageScaleFactor = cachedImage()->brokenImage(document().deviceScaleFactor());
                 image = brokenImageAndImageScaleFactor.first;
                 FloatSize imageSize = image->size();
                 imageSize.scale(1 / brokenImageAndImageScaleFactor.second);
                 // Center the error image, accounting for border and padding.
-                LayoutUnit centerX = (usableWidth - imageSize.width()) / 2;
+                LayoutUnit centerX = (usableSize.width() - imageSize.width()) / 2;
                 if (centerX < 0)
                     centerX = 0;
-                LayoutUnit centerY = (usableHeight - imageSize.height()) / 2;
+                LayoutUnit centerY = (usableSize.height() - imageSize.height()) / 2;
                 if (centerY < 0)
                     centerY = 0;
                 imageOffset = LayoutSize(leftBorder + leftPad + centerX + borderWidth, topBorder + topPad + centerY + borderWidth);
@@ -423,40 +422,44 @@ void RenderImage::paintReplaced(PaintInfo& paintInfo, const LayoutPoint& paintOf
                 TextRun textRun = RenderBlock::constructTextRun(text, style());
                 LayoutUnit textWidth = font.width(textRun);
                 if (errorPictureDrawn) {
-                    if (usableWidth >= textWidth && fontMetrics.height() <= imageOffset.height())
+                    if (usableSize.width() >= textWidth && fontMetrics.height() <= imageOffset.height())
                         context.drawText(font, textRun, altTextOffset);
-                } else if (usableWidth >= textWidth && usableHeight >= fontMetrics.height())
+                } else if (usableSize.width() >= textWidth && usableSize.height() >= fontMetrics.height())
                     context.drawText(font, textRun, altTextOffset);
             }
         }
-    } else if (imageResource().hasImage() && cWidth > 0 && cHeight > 0) {
-        RefPtr<Image> img = imageResource().image(cWidth, cHeight);
-        if (!img || img->isNull()) {
-            if (paintInfo.phase == PaintPhaseForeground)
-                page().addRelevantUnpaintedObject(this, visualOverflowRect());
-            return;
-        }
+        return;
+    }
+    
+    if (contentSize.isEmpty())
+        return;
 
-        LayoutRect contentBoxRect = this->contentBoxRect();
-        contentBoxRect.moveBy(paintOffset);
-        LayoutRect replacedContentRect = this->replacedContentRect(intrinsicSize());
-        replacedContentRect.moveBy(paintOffset);
-        bool clip = !contentBoxRect.contains(replacedContentRect);
-        GraphicsContextStateSaver stateSaver(context, clip);
-        if (clip)
-            context.clip(contentBoxRect);
+    RefPtr<Image> img = imageResource().image(flooredIntSize(contentSize));
+    if (!img || img->isNull()) {
+        if (paintInfo.phase == PaintPhaseForeground)
+            page().addRelevantUnpaintedObject(this, visualOverflowRect());
+        return;
+    }
 
-        paintIntoRect(context, snapRectToDevicePixels(replacedContentRect, deviceScaleFactor));
-        
-        if (cachedImage() && paintInfo.phase == PaintPhaseForeground) {
-            // For now, count images as unpainted if they are still progressively loading. We may want 
-            // to refine this in the future to account for the portion of the image that has painted.
-            LayoutRect visibleRect = intersection(replacedContentRect, contentBoxRect);
-            if (cachedImage()->isLoading())
-                page().addRelevantUnpaintedObject(this, visibleRect);
-            else
-                page().addRelevantRepaintedObject(this, visibleRect);
-        }
+    LayoutRect contentBoxRect = this->contentBoxRect();
+    contentBoxRect.moveBy(paintOffset);
+    LayoutRect replacedContentRect = this->replacedContentRect(intrinsicSize());
+    replacedContentRect.moveBy(paintOffset);
+    bool clip = !contentBoxRect.contains(replacedContentRect);
+    GraphicsContextStateSaver stateSaver(context, clip);
+    if (clip)
+        context.clip(contentBoxRect);
+
+    paintIntoRect(context, snapRectToDevicePixels(replacedContentRect, deviceScaleFactor));
+    
+    if (cachedImage() && paintInfo.phase == PaintPhaseForeground) {
+        // For now, count images as unpainted if they are still progressively loading. We may want 
+        // to refine this in the future to account for the portion of the image that has painted.
+        LayoutRect visibleRect = intersection(replacedContentRect, contentBoxRect);
+        if (cachedImage()->isLoading())
+            page().addRelevantUnpaintedObject(this, visibleRect);
+        else
+            page().addRelevantRepaintedObject(this, visibleRect);
     }
 }
 
@@ -536,7 +539,7 @@ void RenderImage::paintIntoRect(GraphicsContext& context, const FloatRect& rect)
     if (!imageResource().hasImage() || imageResource().errorOccurred() || rect.width() <= 0 || rect.height() <= 0)
         return;
 
-    RefPtr<Image> img = imageResource().image(rect.width(), rect.height());
+    RefPtr<Image> img = imageResource().image(flooredIntSize(rect.size()));
     if (!img || img->isNull())
         return;
 
@@ -569,7 +572,7 @@ bool RenderImage::foregroundIsKnownToBeOpaqueInRect(const LayoutRect& localRect,
     UNUSED_PARAM(maxDepthToTest);
     if (!imageResource().hasImage() || imageResource().errorOccurred())
         return false;
-    if (imageResource().cachedImage() && !imageResource().cachedImage()->isLoaded())
+    if (cachedImage() && !cachedImage()->isLoaded())
         return false;
     if (!contentBoxRect().contains(localRect))
         return false;
@@ -590,7 +593,7 @@ bool RenderImage::foregroundIsKnownToBeOpaqueInRect(const LayoutRect& localRect,
         return false;
 
     // Check for image with alpha.
-    return imageResource().cachedImage() && imageResource().cachedImage()->currentFrameKnownToBeOpaque(this);
+    return cachedImage() && cachedImage()->currentFrameKnownToBeOpaque(this);
 }
 
 bool RenderImage::computeBackgroundIsKnownToBeObscured(const LayoutPoint& paintOffset)
@@ -741,7 +744,7 @@ bool RenderImage::needsPreferredWidthsRecalculation() const
 
 RenderBox* RenderImage::embeddedContentBox() const
 {
-    CachedImage* cachedImage = imageResource().cachedImage();
+    CachedImage* cachedImage = this->cachedImage();
     if (cachedImage && is<SVGImage>(cachedImage->image()))
         return downcast<SVGImage>(*cachedImage->image()).embeddedContentBox();
 
index b94f159a63ec80e3d07aac791d3534047acd20da..bd1e55e4075790b7610fd59cbbd9eb8a6054b70f 100644 (file)
@@ -92,7 +92,7 @@ void RenderImageResource::resetAnimation()
         m_renderer->repaint();
 }
 
-RefPtr<Image> RenderImageResource::image(int, int) const
+RefPtr<Image> RenderImageResource::image(const IntSize&) const
 {
     return m_cachedImage ? m_cachedImage->imageForRenderer(m_renderer) : Image::nullImage();
 }
index 7dfeb157091ef82a5bd8e6b2de9d3538096e785d..8c6f81907668685e774e89181382306c80373413 100644 (file)
@@ -49,7 +49,7 @@ public:
 
     void resetAnimation();
 
-    virtual RefPtr<Image> image(int width = 0, int height = 0) const;
+    virtual RefPtr<Image> image(const IntSize& size = { }) const;
     virtual bool errorOccurred() const;
 
     virtual void setContainerSizeForRenderer(const IntSize&);
index 28e36b1998547b0f4002013c928f0569d0c384ff..ac3f4a092b52ba1b4aa0b0e96862ee6ae9527b98 100644 (file)
@@ -63,12 +63,12 @@ void RenderImageResourceStyleImage::shutdown()
     }
 }
 
-RefPtr<Image> RenderImageResourceStyleImage::image(int width, int height) const
+RefPtr<Image> RenderImageResourceStyleImage::image(const IntSize& size) const
 {
     // Generated content may trigger calls to image() while we're still pending, don't assert but gracefully exit.
     if (m_styleImage->isPending())
         return nullptr;
-    return m_styleImage->image(m_renderer, IntSize(width, height));
+    return m_styleImage->image(m_renderer, size);
 }
 
 void RenderImageResourceStyleImage::setContainerSizeForRenderer(const IntSize& size)
index 2ab8ebd9fc9d9b9b35cea4a8ea20c5818ff01040..584055ce2aff8ec1a198ec5c30e69eb80e34298a 100644 (file)
@@ -43,7 +43,7 @@ private:
     void shutdown() override;
 
     bool hasImage() const override { return true; }
-    RefPtr<Image> image(int width = 0, int height = 0) const override;
+    RefPtr<Image> image(const IntSize& = { }) const override;
     bool errorOccurred() const override { return m_styleImage->errorOccurred(); }
 
     void setContainerSizeForRenderer(const IntSize&) override;