More changes to SkPDFShader to eliminate multiple inheritance!

src/pdf/SkPDFCanon.cpp

Index: src/pdf/SkPDFCanon.cpp
diff --git a/src/pdf/SkPDFCanon.cpp b/src/pdf/SkPDFCanon.cpp
index 31212f8c91564d5568ea5f56371593435463bc17..bea1400bd64ca5de40a59d78fe38dc6ea061211f 100644
--- a/src/pdf/SkPDFCanon.cpp
+++ b/src/pdf/SkPDFCanon.cpp
@@ -37,6 +37,32 @@ static void assert_mutex_held(const SkPDFCanon* canon, SkBaseMutex& mutex) {
     }
 }
 
+template <class T> T* assert_ptr(T* p) { SkASSERT(p); return p; }
+
+// TODO(halcanary):  add this method to SkTDArray.

[mtklein, 2015/01/23 14:16:11]

I'm on board with a ? at the end.

Generally I don't think it's a good idea to put methods like this on a class,
here for two reasons:
  1) you can implement it just as efficiently as the class can, 
     in a few lines of code;
  2) it's slow relative to the main methods of the class.

By 2), I mean that most important methods of SkTDArray are worst-case or
amortized O(1), so an O(N) method is slow relative to SkTDArray.  It seems to be
encouraging bad code to put an O(N) method in there among the O(1) ones unless
it's named something like slowRemove().

Centralizing convenience code like this is a maintenance burden and tends to
lock code into certain data structures and algorithms.  If you put this on
SkTDArray, other code might start calling it.  Then if you want to start sorting
your arrays, you'll either have to make sure the other callers are also sorted,
or abandon your use of the method and write a new one.  Either way you're making
extra work for our future selves, if only because it becomes that many more
entities we have to think about when working on this code.

The benefit of code reuse usually outweighs the burden of the coupling that
comes from centralization when it's offering up some fundamentally new
capability, or maybe if it is a particularly challenging or subtle method to
write.  Code size, whether lines in source or bytes of machine code, is not
important relative to these other concerns, and centralizing template methods
never helps that anyway.

+template <typename T>
+bool remove_item(SkTDArray<T>* array, const T& elem) {
+    int i = array->find(elem);
+    if (i >= 0) {
+        array->removeShuffle(i);
+        return true;
+    }
+    return false;
+}
+
+// requires `bool T::equals(const U&) const`
+template <typename T, typename U>
+T* find_item(const SkTDArray<T*>& ptrArray, const U& object) {

[mtklein, 2015/01/23 14:16:11]

consider object -> key ?

+    for (int i = 0; i < ptrArray.count(); ++i) {
+        if (ptrArray[i]->equals(object)) {
+            return ptrArray[i];
+        }
+    }
+    return NULL;
+}
+
+////////////////////////////////////////////////////////////////////////////////
+
 SkPDFFont* SkPDFCanon::findFont(uint32_t fontID,
                                 uint16_t glyphID,
                                 SkPDFFont** relatedFontPtr) const {
@@ -79,58 +105,67 @@ void SkPDFCanon::removeFont(SkPDFFont* pdfFont) {
 
 ////////////////////////////////////////////////////////////////////////////////
 
-SkPDFShader* SkPDFCanon::findShader(const SkPDFShader::State& state) const {
+SkPDFFunctionShader* SkPDFCanon::findFunctionShader(
+        const SkPDFShader::State& state) const {
     assert_mutex_held(this, gSkPDFCanonShaderMutex);
-    for (int i = 0; i < fShaderRecords.count(); ++i) {
-        if (fShaderRecords[i]->equals(state)) {
-            return fShaderRecords[i];
-        }
-    }
-    return NULL;
+    return find_item(fFunctionShaderRecords, state);
+}
+void SkPDFCanon::addFunctionShader(SkPDFFunctionShader* pdfShader) {
+    assert_mutex_held(this, gSkPDFCanonShaderMutex);
+    fFunctionShaderRecords.push(assert_ptr(pdfShader));
+}
+void SkPDFCanon::removeFunctionShader(SkPDFFunctionShader* pdfShader) {
+    assert_mutex_held(this, gSkPDFCanonShaderMutex);
+    SkAssertResult(remove_item(&fFunctionShaderRecords, pdfShader));
+}
+
+////////////////////////////////////////////////////////////////////////////////
+
+SkPDFAlphaFunctionShader* SkPDFCanon::findAlphaShader(
+        const SkPDFShader::State& state) const {
+    assert_mutex_held(this, gSkPDFCanonShaderMutex);
+    return find_item(fAlphaShaderRecords, state);
+}
+void SkPDFCanon::addAlphaShader(SkPDFAlphaFunctionShader* pdfShader) {
+    assert_mutex_held(this, gSkPDFCanonShaderMutex);
+    fAlphaShaderRecords.push(assert_ptr(pdfShader));
+}
+void SkPDFCanon::removeAlphaShader(SkPDFAlphaFunctionShader* pdfShader) {
+    assert_mutex_held(this, gSkPDFCanonShaderMutex);
+    SkAssertResult(remove_item(&fAlphaShaderRecords, pdfShader));
 }
 
-void SkPDFCanon::addShader(SkPDFShader* shader) {
+////////////////////////////////////////////////////////////////////////////////
+
+SkPDFImageShader* SkPDFCanon::findImageShader(
+        const SkPDFShader::State& state) const {
     assert_mutex_held(this, gSkPDFCanonShaderMutex);
-    SkASSERT(shader);
-    fShaderRecords.push(shader);
+    return find_item(fImageShaderRecords, state);
 }
 
-void SkPDFCanon::removeShader(SkPDFShader* pdfShader) {
+void SkPDFCanon::addImageShader(SkPDFImageShader* pdfShader) {
     assert_mutex_held(this, gSkPDFCanonShaderMutex);
-    for (int i = 0; i < fShaderRecords.count(); ++i) {
-        if (fShaderRecords[i] == pdfShader) {
-            fShaderRecords.removeShuffle(i);
-            return;
-        }
-    }
-    SkDEBUGFAIL("pdfShader not found");
+    fImageShaderRecords.push(assert_ptr(pdfShader));
+}
+
+void SkPDFCanon::removeImageShader(SkPDFImageShader* pdfShader) {
+    assert_mutex_held(this, gSkPDFCanonShaderMutex);
+    SkAssertResult(remove_item(&fImageShaderRecords, pdfShader));
 }
 
 ////////////////////////////////////////////////////////////////////////////////
 
 SkPDFGraphicState* SkPDFCanon::findGraphicState(const SkPaint& paint) const {
     assert_mutex_held(this, gSkPDFCanonPaintMutex);
-    for (int i = 0; i < fGraphicStateRecords.count(); ++i) {
-        if (fGraphicStateRecords[i]->equals(paint)) {
-            return fGraphicStateRecords[i];
-        }
-    }
-    return NULL;
+    return find_item(fGraphicStateRecords, paint);
 }
 
 void SkPDFCanon::addGraphicState(SkPDFGraphicState* state) {
     assert_mutex_held(this, gSkPDFCanonPaintMutex);
-    SkASSERT(state);
-    fGraphicStateRecords.push(state);
+    fGraphicStateRecords.push(assert_ptr(state));
 }
 
 void SkPDFCanon::removeGraphicState(SkPDFGraphicState* pdfGraphicState) {
     assert_mutex_held(this, gSkPDFCanonPaintMutex);
-    for (int i = 0; i < fGraphicStateRecords.count(); ++i) {
-        if (fGraphicStateRecords[i] == pdfGraphicState) {
-            fGraphicStateRecords.removeShuffle(i);
-            return;
-        }
-    }
-    SkDEBUGFAIL("pdfGraphicState not found");
+    SkAssertResult(remove_item(&fGraphicStateRecords, pdfGraphicState));
 }