Adjust src and dest rects when drawing images instead of using a clip

third_party/WebKit/Source/core/paint/ImagePainter.cpp

Index: third_party/WebKit/Source/core/paint/ImagePainter.cpp
diff --git a/third_party/WebKit/Source/core/paint/ImagePainter.cpp b/third_party/WebKit/Source/core/paint/ImagePainter.cpp
index 203c68807df637f623020f6c6248286269e58b1b..14f67aa61ef8dbc020bdb22bdd0e77753c0ee658 100644
--- a/third_party/WebKit/Source/core/paint/ImagePainter.cpp
+++ b/third_party/WebKit/Source/core/paint/ImagePainter.cpp
@@ -19,7 +19,6 @@
 #include "core/paint/PaintInfo.h"
 #include "platform/geometry/LayoutPoint.h"
 #include "platform/graphics/Path.h"
-#include "platform/graphics/paint/ClipRecorder.h"
 
 namespace blink {
 
@@ -102,46 +101,50 @@ void ImagePainter::paintReplaced(const PaintInfo& paintInfo, const LayoutPoint&
             context.drawRect(paintRect);
         }
     } else if (cWidth > 0 && cHeight > 0) {
+        if (LayoutObjectDrawingRecorder::useCachedDrawingIfPossible(context, m_layoutImage, paintInfo.phase))
+            return;
+
         LayoutRect contentRect = m_layoutImage.contentBoxRect();
         contentRect.moveBy(paintOffset);
         LayoutRect paintRect = m_layoutImage.replacedContentRect();
         paintRect.moveBy(paintOffset);
 
-        Optional<ClipRecorder> clipRecorder;
-        if (!contentRect.contains(paintRect)) {
-            // TODO(fmalita): can we get rid of this clip and adjust the image src/dst rect instead?
-            clipRecorder.emplace(context, m_layoutImage, paintInfo.displayItemTypeForClipping(), pixelSnappedIntRect(contentRect));
-        }
-
-        if (LayoutObjectDrawingRecorder::useCachedDrawingIfPossible(context, m_layoutImage, paintInfo.phase))
-            return;
-
         LayoutObjectDrawingRecorder drawingRecorder(context, m_layoutImage, paintInfo.phase, contentRect);
-        paintIntoRect(context, paintRect);
+        paintIntoRect(context, paintRect, contentRect);
     }
 }
 
-void ImagePainter::paintIntoRect(GraphicsContext& context, const LayoutRect& rect)
+void ImagePainter::paintIntoRect(GraphicsContext& context, const LayoutRect& destRect, const LayoutRect& contentRect)
 {
     if (!m_layoutImage.imageResource()->hasImage() || m_layoutImage.imageResource()->errorOccurred())
         return; // FIXME: should we just ASSERT these conditions? (audit all callers).
 
-    IntRect alignedRect = pixelSnappedIntRect(rect);
-    if (alignedRect.width() <= 0 || alignedRect.height() <= 0)
+    IntRect snappedDest = pixelSnappedIntRect(destRect);

[chrishtr, 2016/07/11 18:11:24]

pixelSnappedDestRect

[pdr., 2016/07/11 19:58:17]

Done.

+    if (snappedDest.isEmpty())
         return;
 
-    RefPtr<Image> image = m_layoutImage.imageResource()->image(alignedRect.size(), m_layoutImage.style()->effectiveZoom());
+    RefPtr<Image> image = m_layoutImage.imageResource()->image(snappedDest.size(), m_layoutImage.style()->effectiveZoom());
     if (!image || image->isNull())
         return;
 
     // FIXME: why is interpolation quality selection not included in the Instrumentation reported cost of drawing an image?
-    InterpolationQuality interpolationQuality = BoxPainter::chooseInterpolationQuality(m_layoutImage, image.get(), image.get(), LayoutSize(alignedRect.size()));
+    InterpolationQuality interpolationQuality = BoxPainter::chooseInterpolationQuality(m_layoutImage, image.get(), image.get(), LayoutSize(snappedDest.size()));
+
+    FloatRect srcRect = image->rect();
+    // If the content rect requires clipping, adjust |srcRect| and |snappedDest| over using a clip.
+    if (!contentRect.contains(destRect)) {
+        IntRect snappedContentRect = pixelSnappedIntRect(contentRect);
+        snappedContentRect.intersect(snappedDest);

[f(malita), 2016/07/11 19:43:00]

Can the intersection be empty?  Should we bail in that case?

[pdr., 2016/07/11 19:58:17]

I don't think so. Added a DCHECK to assert it.

+        FloatRect snappedBoundsInSrc = mapRect(snappedContentRect, snappedDest, srcRect);

[f(malita), 2016/07/11 19:43:00]

Nit: snappedBoundsInSrc local doesn't add much value - assign directly to
srcRect?

[pdr., 2016/07/11 19:58:17]

I renamed this pixelSnappedContentRectInSrc and refactored it to be used in the
DCHECK below. The compiler will optimize out any unneeded locals.

+        srcRect = snappedBoundsInSrc;

[f(malita), 2016/07/11 19:43:00]

Nit: would this stick

DECHECK(image->rect().contains(srcRect));

+        snappedDest = snappedContentRect;
+    }
 
     TRACE_EVENT1(TRACE_DISABLED_BY_DEFAULT("devtools.timeline"), "PaintImage", "data", InspectorPaintImageEvent::data(m_layoutImage));
 
     InterpolationQuality previousInterpolationQuality = context.imageInterpolationQuality();
     context.setImageInterpolationQuality(interpolationQuality);
-    context.drawImage(image.get(), alignedRect, nullptr, SkXfermode::kSrcOver_Mode,
+    context.drawImage(image.get(), snappedDest, &srcRect, SkXfermode::kSrcOver_Mode,

[f(malita), 2016/07/11 18:57:09]

So previously we were always scaling/fitting the full image (srcRect* ==
nullptr) into dest, regardless of clipping.

But now, when the clip condition kicks in, we're no longer scaling the image but
extracting a subset and drawing 1:1.

^ looks like a potential functional change - is it intended and do we have any
test coverage for it?

         LayoutObject::shouldRespectImageOrientation(&m_layoutImage));
     context.setImageInterpolationQuality(previousInterpolationQuality);
 }