Clarify GraphicsContext::beginLayer()/endLayer() have unexpected behaviors.

Source/platform/graphics/GraphicsContext.cpp

Index: Source/platform/graphics/GraphicsContext.cpp
diff --git a/Source/platform/graphics/GraphicsContext.cpp b/Source/platform/graphics/GraphicsContext.cpp
index c75c0cf53327cf7d8f54e8286c2eb75ceeb637f3..d40eff4a59e3c033a37fbb437f088f6032dae257 100644
--- a/Source/platform/graphics/GraphicsContext.cpp
+++ b/Source/platform/graphics/GraphicsContext.cpp
@@ -88,7 +88,6 @@ GraphicsContext::GraphicsContext(SkCanvas* canvas, DisabledMode disableContextOr
     , m_annotationMode(0)
 #if ENABLE(ASSERT)
     , m_annotationCount(0)
-    , m_layerCount(0)
     , m_disableDestructionChecks(false)
 #endif
     , m_disabledState(disableContextOrPainting)
@@ -114,7 +113,7 @@ GraphicsContext::~GraphicsContext()
         ASSERT(!m_paintStateIndex);
         ASSERT(!m_paintState->saveCount());
         ASSERT(!m_annotationCount);
-        ASSERT(!m_layerCount);
+        ASSERT(m_clipStackPointersOfLayerStack.isEmpty());
         ASSERT(m_recordingStateStack.isEmpty());
         ASSERT(!saveCount());
     }
@@ -182,6 +181,18 @@ unsigned GraphicsContext::saveCount() const
 }
 #endif
 
+static unsigned getClipStackPointer(const SkCanvas* canvas)
+{
+    SkClipStack::B2TIter iter(*canvas->getClipStack());
+    const SkClipStack::Element* element = iter.next();
+    unsigned stackPointer = 0;
+    while (element) {
+        stackPointer++;
+        element = iter.next();
+    }
+    return stackPointer;
+}
+
 void GraphicsContext::saveLayer(const SkRect* bounds, const SkPaint* paint)
 {
     if (contextDisabled())
@@ -192,6 +203,7 @@ void GraphicsContext::saveLayer(const SkRect* bounds, const SkPaint* paint)
     m_canvas->saveLayer(bounds, paint);
     if (regionTrackingEnabled())
         m_trackedRegion.pushCanvasLayer(paint);
+    m_clipStackPointersOfLayerStack.append(getClipStackPointer(m_canvas));
 }
 
 void GraphicsContext::restoreLayer()
@@ -200,10 +212,48 @@ void GraphicsContext::restoreLayer()
         return;
 
     ASSERT(m_canvas);
+    ASSERT(m_clipStackPointersOfLayerStack.size() > 0);
+
+    // beginLayer()/endLayer() must not expose states stacking mechanism.
+    // Currently SkCanvas::save()/restore() in GraphicsContext affected only ctm and clip.
+    SkMatrix currentCTM = m_canvas->getTotalMatrix();
+    SkClipStack currentSkClipStack(*m_canvas->getClipStack());

[Justin Novosad, 2014/10/16 17:51:58]

Seems unfortunate to duplicate the entire clip stack when all you care about is
elements from m_clipStackPointersOfLayerStack.last() to the end.

 
     m_canvas->restore();
     if (regionTrackingEnabled())
         m_trackedRegion.popCanvasLayer(this);
+
+    // SkCanvas::clipXXX() transforms a rect/path by CTM.
+    m_canvas->resetMatrix();
+
+    static const SkRect kEmptyRect = { 0, 0, 0, 0 };
+    SkClipStack::B2TIter iter(currentSkClipStack);
+    const SkClipStack::Element* element = nullptr;
+    unsigned stackPointer = m_clipStackPointersOfLayerStack.last();
+    m_clipStackPointersOfLayerStack.removeLast();
+    for (unsigned i = 0; i < stackPointer; i++) {
+        iter.next();
+    }
+    element = iter.next();
+    while (element) {
+        switch (element->getType()) {
+        case SkClipStack::Element::kPath_Type:
+            m_canvas->clipPath(element->getPath(), element->getOp(), element->isAA());
+            break;
+        case SkClipStack::Element::kRRect_Type:
+            m_canvas->clipRRect(element->getRRect(), element->getOp(), element->isAA());
+            break;
+        case SkClipStack::Element::kRect_Type:
+            m_canvas->clipRect(element->getRect(), element->getOp(), element->isAA());
+            break;
+        case SkClipStack::Element::kEmpty_Type:
+            m_canvas->clipRect(kEmptyRect, SkRegion::kIntersect_Op, false);
+            break;
+        }
+        element = iter.next();
+    }
+
+    m_canvas->setMatrix(currentCTM);
 }

[dshwang, 2014/10/16 15:58:20]

Feel like using implementation detail of skia, but I cannot find a better way.
I hope new unittests in GraphicsContextTest is to catch it if clip
implementation of SkCanvas is updated incompatible to this CL.

[Justin Novosad, 2014/10/16 17:51:58]

I agree. reverse engineering the state seems unfortunate. Have you considered
making a change to skia? Perhaps adding a flavor of saveLayer that does not save
the matrix and clip?

 
 void GraphicsContext::beginAnnotation(const AnnotationList& annotations)
@@ -462,10 +512,6 @@ void GraphicsContext::beginLayer(float opacity, CompositeOperator op, const Floa
     } else {
         saveLayer(0, &layerPaint);
     }
-
-#if ENABLE(ASSERT)
-    ++m_layerCount;
-#endif
 }
 
 void GraphicsContext::endLayer()
@@ -474,11 +520,6 @@ void GraphicsContext::endLayer()
         return;
 
     restoreLayer();
-
-    ASSERT(m_layerCount > 0);
-#if ENABLE(ASSERT)
-    --m_layerCount;
-#endif
 }
 
 void GraphicsContext::beginRecording(const FloatRect& bounds, uint32_t recordFlags)