PDF: Now threadsafe!

src/pdf/SkPDFGraphicState.cpp

Index: src/pdf/SkPDFGraphicState.cpp
diff --git a/src/pdf/SkPDFGraphicState.cpp b/src/pdf/SkPDFGraphicState.cpp
index 46aa1d1ea40aa3d85ba85fb72ea899eabefbd68d..d8fa1c5157efa912c31ef9eabe5a0190d0178c75 100644
--- a/src/pdf/SkPDFGraphicState.cpp
+++ b/src/pdf/SkPDFGraphicState.cpp
@@ -113,9 +113,9 @@ bool SkPDFGraphicState::equals(const SkPaint& paint) const {
 }
 
 SkPDFGraphicState::~SkPDFGraphicState() {
-    SkAutoMutexAcquire lock(SkPDFCanon::GetPaintMutex());
+    SkASSERT((fSMask || fCanon) && !(fSMask && fCanon));

[mtklein, 2015/02/19 23:50:38]

My head asplode.  Can we write this as

if (fCanon) {
   SkASSERT(!fSMask);
   fCanon->removeGraphicState(this);
}

[hal.canary, 2015/02/20 01:22:28]

Not exactly.  Is this more readable?

  SkASSERT((fSMask == NULL) != (fCanon == NULL));

[mtklein, 2015/02/20 14:55:41]

Yes that was actually my first thought too, until I realized fSMask is a bool. 
Then I couldn't figure out any way to write it that wasn't either unclear or
misleading.

Why don't we just drop the fSMask checks altogether here?  Seems like mixing
that in is just sort of confusing things.

if (fCanon) {
    fCanon->removeGraphicState(this);
}

     if (!fSMask) {
-        SkPDFCanon::GetCanon().removeGraphicState(this);
+        fCanon->removeGraphicState(this);
     }
 }
 
@@ -126,15 +126,14 @@ void SkPDFGraphicState::emitObject(SkWStream* stream, SkPDFCatalog* catalog) {
 
 // static
 SkPDFGraphicState* SkPDFGraphicState::GetGraphicStateForPaint(
-        const SkPaint& paint) {
-    SkAutoMutexAcquire lock(SkPDFCanon::GetPaintMutex());
-    SkPDFGraphicState* pdfGraphicState =
-            SkPDFCanon::GetCanon().findGraphicState(paint);
+        SkPDFCanon* canon, const SkPaint& paint) {
+    SkASSERT(canon);
+    SkPDFGraphicState* pdfGraphicState = canon->findGraphicState(paint);
     if (pdfGraphicState) {
         return SkRef(pdfGraphicState);
     }
-    pdfGraphicState = new SkPDFGraphicState(paint);
-    SkPDFCanon::GetCanon().addGraphicState(pdfGraphicState);
+    pdfGraphicState = new SkPDFGraphicState(canon, paint);
+    canon->addGraphicState(pdfGraphicState);
     return pdfGraphicState;
 }
 
@@ -211,15 +210,10 @@ SkPDFGraphicState* SkPDFGraphicState::GetNoSMaskGraphicState() {
 }
 
 SkPDFGraphicState::SkPDFGraphicState()
-    : fPopulated(false),
-      fSMask(false) {
-}
+    : fCanon(NULL), fPopulated(false), fSMask(false) {}
 
-SkPDFGraphicState::SkPDFGraphicState(const SkPaint& paint)
-    : fPaint(paint),
-      fPopulated(false),
-      fSMask(false) {
-}
+SkPDFGraphicState::SkPDFGraphicState(SkPDFCanon* canon, const SkPaint& paint)
+    : fCanon(canon), fPaint(paint), fPopulated(false), fSMask(false) {}
 
 // populateDict and operator== have to stay in sync with each other.
 void SkPDFGraphicState::populateDict() {