Override SkCanvas::drawImage() in SkRecorder.

tests/RecordDrawTest.cpp

Index: tests/RecordDrawTest.cpp
diff --git a/tests/RecordDrawTest.cpp b/tests/RecordDrawTest.cpp
index 1108af6e36503c9471ad2c10b3364a7e1f85e865..a9b151b446069eff97129e97d54ed0081f9066be 100644
--- a/tests/RecordDrawTest.cpp
+++ b/tests/RecordDrawTest.cpp
@@ -11,11 +11,13 @@
 #include "SkDebugCanvas.h"
 #include "SkDrawPictureCallback.h"
 #include "SkDropShadowImageFilter.h"
+#include "SkImagePriv.h"
 #include "SkRecord.h"
 #include "SkRecordDraw.h"
 #include "SkRecordOpts.h"
 #include "SkRecorder.h"
 #include "SkRecords.h"
+#include "SkSurface.h"
 
 static const int W = 1920, H = 1080;
 
@@ -251,3 +253,76 @@ DEF_TEST(RecordDraw_SaveLayerAffectsClipBounds, r) {
     REPORTER_ASSERT(r, sloppy_rect_eq(bbh.entries[2].bounds, SkRect::MakeLTRB(0, 0, 40, 40)));
     REPORTER_ASSERT(r, sloppy_rect_eq(bbh.entries[3].bounds, SkRect::MakeLTRB(0, 0, 70, 50)));
 }
+
+DEF_TEST(RecordDraw_drawImage, r){

[mtklein, 2014/09/27 15:41:22]

I feel like this test may be taking on a bit too much at once.  I see at least 3
parts I'd prefer were tested separately:
  1) when we call SkRecorder::drawImage()/drawImageRect() we end up with the
correct DrawImage/DrawImageRect in the SkRecord (this can go in
tests/RecorderTest.cpp, and maybe is covered enough by the tests you've added
already)
  2) when we call SkRecordDraw on an SkRecord with DrawImage/DrawImageRect, it
calls SkCanvas::drawImage()/drawImageRect() correctly (this could be here)
  3) when we call SkCanvas::drawImage()/drawImageRect(), they proxy down to
drawBitmap()/drawBitmapRectToRect() correctly (this could go in CanvasTest, and
doesn't really need to be part of this CL).

I'm not opposed to having these sort of e2e integration tests where they matter
(particularly, when they recreate a scenario that tests a regression), but the
space of e2e tests like this is very highly combinatoric, and can be hard to
manage over time if we write lots of them.  While it's all new and we still have
no indication of a need to test these pieces in this particular combination, I'd
rather we split up the tests into as focused units as possible.

[Rémi Piotaix, 2014/09/29 17:57:55]

Yes, already done in RecorderTest.cpp, but this is not tested in this particular
test.
Ok, I just have to remove the part where I check the calls to drawBitmap[Rect].
Actually, I don't even know why I did that because it will not be always the
case that a SkImage is backed by a bitmap...



Done.

+    class SkCanvasMock : public SkCanvas {
+    public:
+        SkCanvasMock(int width, int height) : INHERITED(width, height) {

[robertphillips, 2014/09/29 17:07:56]

this->

[Rémi Piotaix, 2014/09/29 17:57:55]

Done.

+            resetTestValues();
+        }
+        virtual ~SkCanvasMock() {}
+        virtual void drawImage(const SkImage* image, SkScalar left, SkScalar top,
+                                   const SkPaint* paint = NULL) SK_OVERRIDE {
+

[robertphillips, 2014/09/29 17:07:56]

fDrawImageCalled

[Rémi Piotaix, 2014/09/29 17:57:55]

Done.

+            fDrawImage_called = true;
+            fDrawingImage = image;
+            INHERITED::drawImage(image, left, top, paint);
+        }
+
+        virtual void drawImageRect(const SkImage* image, const SkRect* src,
+                               const SkRect& dst,
+                               const SkPaint* paint = NULL) SK_OVERRIDE {
+            fDrawImageRect_called = true;
+            fDrawingImage = image;
+            INHERITED::drawImageRect(image, src, dst, paint);
+        }
+
+        virtual void drawBitmap(const SkBitmap& bitmap, SkScalar left, SkScalar top,
+                                const SkPaint* paint = NULL) SK_OVERRIDE {
+            fDrawBitmap_valid = bitmap.pixelRef() == SkBitmapImageGetPixelRef(fDrawingImage);
+        }
+
+        virtual void drawBitmapRectToRect(const SkBitmap& bitmap, const SkRect* src,
+                                          const SkRect& dst,
+                                          const SkPaint* paint = NULL,
+                                          DrawBitmapRectFlags flags = kNone_DrawBitmapRectFlag)
+                                          SK_OVERRIDE {
+            fDrawBitmap_valid = bitmap.pixelRef() == SkBitmapImageGetPixelRef(fDrawingImage);
+        }
+
+        void resetTestValues() {
+            fDrawImage_called = fDrawBitmap_valid = fDrawImageRect_called = false;
+        }
+
+        const SkImage* fDrawingImage;

[robertphillips, 2014/09/29 17:07:56]

No '_'s in the names.

[Rémi Piotaix, 2014/09/29 17:57:55]

Done.

+        bool fDrawImage_called;
+        bool fDrawImageRect_called;
+        bool fDrawBitmap_valid;
+    private:
+        typedef SkCanvas INHERITED;
+    };
+
+    SkAutoTUnref<SkSurface> surface(SkSurface::NewRasterPMColor(10, 10));

[reed1, 2014/09/29 18:08:26]

1. Can we using an outer-loop or something to vary how the surface is being
created? (e.g. gpu, imagegenerators)
2. Can we perform these asserts using SkPicture public APIs, or do we really
need to use SkRecord directly?

[Rémi Piotaix, 2014/09/29 18:40:21]

Yeah, sure.
If we want a test using SkPicture public API, it is more an integration test
than a unit test. Because i added an opcode in SkRecord, i think it is
meaningful to have this test using directly SkRecord.

If we use the SkPicture API for a test, shouldn't we modify the deprecated path
as well? (the test will break otherwise)

+    surface->getCanvas()->clear(SK_ColorGREEN);
+    SkAutoTUnref<SkImage> image(surface->newImageSnapshot());
+
+    SkCanvasMock canvas(10, 10);
+
+    {
+        SkRecord record;
+        SkRecorder recorder(&record, 10, 10);
+        recorder.drawImage(image, 0, 0);
+        SkRecordDraw(record, &canvas, 0, 0);
+    }
+    REPORTER_ASSERT(r, canvas.fDrawImage_called && canvas.fDrawBitmap_valid);
+    canvas.resetTestValues();
+
+    {
+        SkRecord record;
+        SkRecorder recorder(&record, 10, 10);
+        recorder.drawImageRect(image, 0, SkRect::MakeWH(10, 10));
+        SkRecordDraw(record, &canvas, 0, 0);
+    }
+    REPORTER_ASSERT(r, canvas.fDrawImageRect_called && canvas.fDrawBitmap_valid);
+
+}