make LayerIter private, and remove skipClip option

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2257203002

Review-Url: https://codereview.chromium.org/2257203002

include/core/SkCanvas.h

@@ -1325,44 +1325,6 @@ protected:
     sk_sp<SkLights> fLights;
 #endif
 
-    /** After calling saveLayer(), there can be any number of devices that make
-        up the top-most drawing area. LayerIter can be used to iterate through
-        those devices. Note that the iterator is only valid until the next API
-        call made on the canvas. Ownership of all pointers in the iterator stays
-        with the canvas, so none of them should be modified or deleted.
-    */
-    class LayerIter /*: SkNoncopyable*/ {
-    public:
-        /** Initialize iterator with canvas, and set values for 1st device */
-        LayerIter(SkCanvas*, bool skipEmptyClips);
-        ~LayerIter();
-
-        /** Return true if the iterator is done */
-        bool done() const { return fDone; }
-        /** Cycle to the next device */
-        void next();
-
-        // These reflect the current device in the iterator
-
-        SkBaseDevice*   device() const;
-        const SkMatrix& matrix() const;
-        const SkRasterClip& clip() const;
-        const SkPaint&  paint() const;
-        int             x() const;
-        int             y() const;
-
-    private:
-        // used to embed the SkDrawIter object directly in our instance, w/o
-        // having to expose that class def to the public. There is an assert
-        // in our constructor to ensure that fStorage is large enough
-        // (though needs to be a compile-time-assert!). We use intptr_t to work
-        // safely with 32 and 64 bit machines (to ensure the storage is enough)
-        intptr_t          fStorage[32];
-        class SkDrawIter* fImpl;    // this points at fStorage
-        SkPaint           fDefaultPaint;
-        bool              fDone;
-    };
-
     // default impl defers to getDevice()->newSurface(info)
     virtual sk_sp<SkSurface> onNewSurface(const SkImageInfo&, const SkSurfaceProps&);
 
@@ -1484,6 +1446,44 @@ protected:
                         const SkImageFilter* imageFilter = NULL);
 
 private:
+    /** After calling saveLayer(), there can be any number of devices that make
+     up the top-most drawing area. LayerIter can be used to iterate through
+     those devices. Note that the iterator is only valid until the next API
+     call made on the canvas. Ownership of all pointers in the iterator stays
+     with the canvas, so none of them should be modified or deleted.
+     */
+    class LayerIter /*: SkNoncopyable*/ {
+    public:
+        /** Initialize iterator with canvas, and set values for 1st device */
+        LayerIter(SkCanvas*);
+        ~LayerIter();
+
+        /** Return true if the iterator is done */
+        bool done() const { return fDone; }
+        /** Cycle to the next device */
+        void next();
+
+        // These reflect the current device in the iterator
+
+        SkBaseDevice*   device() const;
+        const SkMatrix& matrix() const;
+        const SkRasterClip& clip() const;
+        const SkPaint&  paint() const;
+        int             x() const;
+        int             y() const;
+
+    private:
+        // used to embed the SkDrawIter object directly in our instance, w/o
+        // having to expose that class def to the public. There is an assert
+        // in our constructor to ensure that fStorage is large enough
+        // (though needs to be a compile-time-assert!). We use intptr_t to work
+        // safely with 32 and 64 bit machines (to ensure the storage is enough)
+        intptr_t          fStorage[32];
+        class SkDrawIter* fImpl;    // this points at fStorage
+        SkPaint           fDefaultPaint;
+        bool              fDone;
+    };
+    
     static bool BoundsAffectsClip(SaveLayerFlags);
     static SaveLayerFlags LegacySaveFlagsToSaveLayerFlags(uint32_t legacySaveFlags);
 
@@ -1542,7 +1542,6 @@ private:
     void checkForDeferredSave();
     void internalSetMatrix(const SkMatrix&);
 
-    friend class CanvasTestingAccess; // for testing
     friend class SkDrawIter;        // needs setupDrawForLayerDevice()
     friend class AutoDrawLooper;
     friend class SkLua;             // needs top layer size and offset

src/core/SkCanvas.cpp

@@ -318,22 +318,19 @@ public:
 
 class SkDrawIter : public SkDraw {
 public:
-    SkDrawIter(SkCanvas* canvas, bool skipEmptyClips = true) {
+    SkDrawIter(SkCanvas* canvas) {
         canvas = canvas->canvasForDrawIter();
         fCanvas = canvas;
         canvas->updateDeviceCMCache();
 
         fClipStack = canvas->fClipStack;
         fCurrLayer = canvas->fMCRec->fTopLayer;
-        fSkipEmptyClips = skipEmptyClips;
     }
 
     bool next() {
         // skip over recs with empty clips
-        if (fSkipEmptyClips) {
+        while (fCurrLayer && fCurrLayer->fClip.isEmpty()) {
-            while (fCurrLayer && fCurrLayer->fClip.isEmpty()) {
+            fCurrLayer = fCurrLayer->fNext;
-                fCurrLayer = fCurrLayer->fNext;
-            }
         }
 
         const DeviceCM* rec = fCurrLayer;
@@ -367,7 +364,6 @@ private:
     SkCanvas*       fCanvas;
     const DeviceCM* fCurrLayer;
     const SkPaint*  fPaint;     // May be null.
-    SkBool8         fSkipEmptyClips;
 
     typedef SkDraw INHERITED;
 };
@@ -432,8 +428,7 @@ public:
     // "rawBounds" is the original bounds of the primitive about to be drawn, unmodified by the
     // paint. It's used to determine the size of the offscreen layer for filters.
     // If null, the clip will be used instead.
-    AutoDrawLooper(SkCanvas* canvas, const SkSurfaceProps& props, const SkPaint& paint,
+    AutoDrawLooper(SkCanvas* canvas, const SkPaint& paint, bool skipLayerForImageFilter = false,
-                   bool skipLayerForImageFilter = false,
                    const SkRect* rawBounds = nullptr) : fOrigPaint(paint) {
         fCanvas = canvas;
 #ifdef SK_SUPPORT_LEGACY_DRAWFILTER
@@ -580,28 +575,28 @@ bool AutoDrawLooper::doNext(SkDrawFilter::Type drawType) {
 
 ////////// macros to place around the internal draw calls //////////////////
 
-#define LOOPER_BEGIN_DRAWBITMAP(paint, skipLayerForFilter, bounds)          \
+#define LOOPER_BEGIN_DRAWBITMAP(paint, skipLayerForFilter, bounds)  \
-    this->predrawNotify();                                                  \
+    this->predrawNotify();                                          \
-    AutoDrawLooper looper(this, fProps, paint, skipLayerForFilter, bounds); \
+    AutoDrawLooper looper(this, paint, skipLayerForFilter, bounds); \
-    while (looper.next(SkDrawFilter::kBitmap_Type)) {                       \
+    while (looper.next(SkDrawFilter::kBitmap_Type)) {               \
         SkDrawIter iter(this);
 
 
 #define LOOPER_BEGIN_DRAWDEVICE(paint, type)                        \
     this->predrawNotify();                                          \
-    AutoDrawLooper  looper(this, fProps, paint, true);              \
+    AutoDrawLooper  looper(this, paint, true);                      \
     while (looper.next(type)) {                                     \
         SkDrawIter          iter(this);
 
 #define LOOPER_BEGIN(paint, type, bounds)                           \
     this->predrawNotify();                                          \
-    AutoDrawLooper  looper(this, fProps, paint, false, bounds);     \
+    AutoDrawLooper  looper(this, paint, false, bounds);             \
     while (looper.next(type)) {                                     \
         SkDrawIter          iter(this);
 
 #define LOOPER_BEGIN_CHECK_COMPLETE_OVERWRITE(paint, type, bounds, auxOpaque)  \
     this->predrawNotify(bounds, &paint, auxOpaque);                 \
-    AutoDrawLooper  looper(this, fProps, paint, false, bounds);     \
+    AutoDrawLooper  looper(this, paint, false, bounds);             \
     while (looper.next(type)) {                                     \
         SkDrawIter          iter(this);
 
@@ -3263,12 +3258,12 @@ void SkCanvas::onDrawShadowedPicture(const SkPicture* picture,
 ///////////////////////////////////////////////////////////////////////////////
 ///////////////////////////////////////////////////////////////////////////////
 
-SkCanvas::LayerIter::LayerIter(SkCanvas* canvas, bool skipEmptyClips) {
+SkCanvas::LayerIter::LayerIter(SkCanvas* canvas) {
     static_assert(sizeof(fStorage) >= sizeof(SkDrawIter), "fStorage_too_small");
 
     SkASSERT(canvas);
 
-    fImpl = new (fStorage) SkDrawIter(canvas, skipEmptyClips);
+    fImpl = new (fStorage) SkDrawIter(canvas);
     fDone = !fImpl->next();
 }
 

src/utils/SkCanvasStateUtils.cpp

@@ -222,7 +222,7 @@ SkCanvasState* SkCanvasStateUtils::CaptureCanvasState(SkCanvas* canvas) {
      */
     SkSWriter32<3*sizeof(SkCanvasLayerState)> layerWriter;
     int layerCount = 0;
-    for (SkCanvas::LayerIter layer(canvas, true/*skipEmptyClips*/); !layer.done(); layer.next()) {
+    for (SkCanvas::LayerIter layer(canvas); !layer.done(); layer.next()) {
 
         // we currently only work for bitmap backed devices
         SkPixmap pmap;

tests/CanvasTest.cpp

@@ -169,20 +169,6 @@ private:
     }
 };
 
-static bool equal_clips(const SkCanvas& a, const SkCanvas& b) {
-    if (a.isClipEmpty()) {
-        return b.isClipEmpty();
-    }
-    if (!a.isClipRect()) {
-        // this is liberally true, since we don't expose a way to know this exactly (for non-rects)
-        return !b.isClipRect();
-    }
-    SkIRect ar, br;
-    a.getClipDeviceBounds(&ar);
-    b.getClipDeviceBounds(&br);
-    return ar == br;
-}
-
 class Canvas2CanvasClipVisitor : public SkCanvas::ClipVisitor {
 public:
     Canvas2CanvasClipVisitor(SkCanvas* target) : fTarget(target) {}
@@ -201,19 +187,6 @@ private:
     SkCanvas* fTarget;
 };
 
-static void test_clipVisitor(skiatest::Reporter* reporter, SkCanvas* canvas) {
-    SkISize size = canvas->getDeviceSize();
-
-    SkBitmap bm;
-    bm.setInfo(SkImageInfo::MakeN32Premul(size.width(), size.height()));
-    SkCanvas c(bm);
-
-    Canvas2CanvasClipVisitor visitor(&c);
-    canvas->replayClips(&visitor);
-
-    REPORTER_ASSERT(reporter, equal_clips(c, *canvas));
-}
-
 static void test_clipstack(skiatest::Reporter* reporter) {
     // The clipstack is refcounted, and needs to be able to out-live the canvas if a client has
     // ref'd it.
@@ -234,14 +207,6 @@ static void test_clipstack(skiatest::Reporter* reporter) {
 static const char* const kDefaultAssertMessageFormat = "%s";
 static const char* const kCanvasDrawAssertMessageFormat =
     "Drawing test step %s with SkCanvas";
-static const char* const kNWayDrawAssertMessageFormat =
-    "Drawing test step %s with SkNWayCanvas";
-static const char* const kNWayStateAssertMessageFormat =
-    "test step %s, SkNWayCanvas state consistency";
-static const char* const kNWayIndirect1StateAssertMessageFormat =
-    "test step %s, SkNWayCanvas indirect canvas 1 state consistency";
-static const char* const kNWayIndirect2StateAssertMessageFormat =
-    "test step %s, SkNWayCanvas indirect canvas 2 state consistency";
 static const char* const kPdfAssertMessageFormat =
     "PDF sanity check failed %s";
 
@@ -538,78 +503,7 @@ static void DescribeTopLayerTestStep(SkCanvas* canvas,
 TEST_STEP(DescribeTopLayer, DescribeTopLayerTestStep);
 
 
-class CanvasTestingAccess {
+static void TestPdfDevice(skiatest::Reporter* reporter, const TestData& d, CanvasTestStep* step) {
-public:
-    static bool SameState(const SkCanvas* canvas1, const SkCanvas* canvas2) {
-        SkCanvas::LayerIter layerIter1(const_cast<SkCanvas*>(canvas1), false);
-        SkCanvas::LayerIter layerIter2(const_cast<SkCanvas*>(canvas2), false);
-        while (!layerIter1.done() && !layerIter2.done()) {
-            if (layerIter1.matrix() != layerIter2.matrix()) {
-                return false;
-            }
-            if (layerIter1.clip() != layerIter2.clip()) {
-                return false;
-            }
-            if (layerIter1.paint() != layerIter2.paint()) {
-                return false;
-            }
-            if (layerIter1.x() != layerIter2.x()) {
-                return false;
-            }
-            if (layerIter1.y() != layerIter2.y()) {
-                return false;
-            }
-            layerIter1.next();
-            layerIter2.next();
-        }
-        if (!layerIter1.done()) {
-            return false;
-        }
-        if (!layerIter2.done()) {
-            return false;
-        }
-        return true;
-    }
-};
-
-static void AssertCanvasStatesEqual(skiatest::Reporter* reporter, const TestData& d,
-                                    const SkCanvas* canvas1, const SkCanvas* canvas2,
-                                    CanvasTestStep* testStep) {
-    REPORTER_ASSERT_MESSAGE(reporter, canvas1->getDeviceSize() ==
-        canvas2->getDeviceSize(), testStep->assertMessage());
-    REPORTER_ASSERT_MESSAGE(reporter, canvas1->getSaveCount() ==
-        canvas2->getSaveCount(), testStep->assertMessage());
-
-    SkRect bounds1, bounds2;
-    REPORTER_ASSERT_MESSAGE(reporter,
-        canvas1->getClipBounds(&bounds1) == canvas2->getClipBounds(&bounds2),
-        testStep->assertMessage());
-    REPORTER_ASSERT_MESSAGE(reporter, bounds1 == bounds2,
-                            testStep->assertMessage());
-
-#ifdef SK_SUPPORT_LEGACY_DRAWFILTER
-    REPORTER_ASSERT_MESSAGE(reporter, canvas1->getDrawFilter() ==
-        canvas2->getDrawFilter(), testStep->assertMessage());
-#endif
-
-    SkIRect deviceBounds1, deviceBounds2;
-    REPORTER_ASSERT_MESSAGE(reporter,
-        canvas1->getClipDeviceBounds(&deviceBounds1) ==
-        canvas2->getClipDeviceBounds(&deviceBounds2),
-        testStep->assertMessage());
-    REPORTER_ASSERT_MESSAGE(reporter, deviceBounds1 == deviceBounds2, testStep->assertMessage());
-    REPORTER_ASSERT_MESSAGE(reporter, canvas1->getTotalMatrix() ==
-        canvas2->getTotalMatrix(), testStep->assertMessage());
-    REPORTER_ASSERT_MESSAGE(reporter, equal_clips(*canvas1, *canvas2), testStep->assertMessage());
-
-    REPORTER_ASSERT_MESSAGE(reporter,
-                            CanvasTestingAccess::SameState(canvas1, canvas2),
-                            testStep->assertMessage());
-}
-
-static void TestPdfDevice(skiatest::Reporter* reporter,
-                          const TestData& d,
-                          CanvasTestStep* testStep) {
     SkDynamicMemoryWStream outStream;
     sk_sp<SkDocument> doc(SkDocument::MakePDF(&outStream));
     REPORTER_ASSERT(reporter, doc);
@@ -619,44 +513,12 @@ static void TestPdfDevice(skiatest::Reporter* reporter,
     SkCanvas* canvas = doc->beginPage(SkIntToScalar(d.fWidth),
                                       SkIntToScalar(d.fHeight));
     REPORTER_ASSERT(reporter, canvas);
-    testStep->setAssertMessageFormat(kPdfAssertMessageFormat);
+    step->setAssertMessageFormat(kPdfAssertMessageFormat);
-    testStep->draw(canvas, d, reporter);
+    step->draw(canvas, d, reporter);
 
     REPORTER_ASSERT(reporter, doc->close());
 }
 
-// unused
-static void TestNWayCanvasStateConsistency(
-    skiatest::Reporter* reporter,
-    const TestData& d,
-    CanvasTestStep* testStep,
-    const SkCanvas& referenceCanvas) {
-
-    SkBitmap indirectStore1;
-    createBitmap(&indirectStore1, 0xFFFFFFFF);
-    SkCanvas indirectCanvas1(indirectStore1);
-
-    SkBitmap indirectStore2;
-    createBitmap(&indirectStore2, 0xFFFFFFFF);
-    SkCanvas indirectCanvas2(indirectStore2);
-
-    SkISize canvasSize = referenceCanvas.getDeviceSize();
-    SkNWayCanvas nWayCanvas(canvasSize.width(), canvasSize.height());
-    nWayCanvas.addCanvas(&indirectCanvas1);
-    nWayCanvas.addCanvas(&indirectCanvas2);
-
-    testStep->setAssertMessageFormat(kNWayDrawAssertMessageFormat);
-    testStep->draw(&nWayCanvas, d, reporter);
-    // Verify that the SkNWayCanvas reports consitent state
-    testStep->setAssertMessageFormat(kNWayStateAssertMessageFormat);
-    AssertCanvasStatesEqual(reporter, d, &nWayCanvas, &referenceCanvas, testStep);
-    // Verify that the indirect canvases report consitent state
-    testStep->setAssertMessageFormat(kNWayIndirect1StateAssertMessageFormat);
-    AssertCanvasStatesEqual(reporter, d, &indirectCanvas1, &referenceCanvas, testStep);
-    testStep->setAssertMessageFormat(kNWayIndirect2StateAssertMessageFormat);
-    AssertCanvasStatesEqual(reporter, d, &indirectCanvas2, &referenceCanvas, testStep);
-}
-
 /*
  * This sub-test verifies that the test step passes when executed
  * with SkCanvas and with classes derrived from SkCanvas. It also verifies
@@ -671,17 +533,6 @@ static void TestOverrideStateConsistency(skiatest::Reporter* reporter, const Tes
     testStep->setAssertMessageFormat(kCanvasDrawAssertMessageFormat);
     testStep->draw(&referenceCanvas, d, reporter);
 
-    // The following test code is disabled because SkNWayCanvas does not
-    // report correct clipping and device bounds information
-    // Issue: http://code.google.com/p/skia/issues/detail?id=501
-
-    if (false) { // avoid bit rot, suppress warning
-        TestNWayCanvasStateConsistency(reporter, d, testStep, referenceCanvas);
-    }
-
-    if (false) { // avoid bit rot, suppress warning
-        test_clipVisitor(reporter, &referenceCanvas);
-    }
     test_clipstack(reporter);
 }