More changes to SkPDFShader to eliminate multiple inheritance!

src/pdf/SkPDFCanon.cpp

 /*
  * Copyright 2015 Google Inc.
  *
  * Use of this source code is governed by a BSD-style license that can be
  * found in the LICENSE file.
  */
 
 #include "SkLazyPtr.h"
 #include "SkPDFCanon.h"
 #include "SkPDFFont.h"
@@ skipping 19 matching lines @@
 SkPDFCanon& SkPDFCanon::GetCanon() { return *singleton.get(); }
 
 ////////////////////////////////////////////////////////////////////////////////
 
 static void assert_mutex_held(const SkPDFCanon* canon, SkBaseMutex& mutex) {
     if (canon == singleton.get()) {
         mutex.assertHeld();
     }
 }
 
+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 {
     assert_mutex_held(this, gSkPDFCanonFontMutex);
     SkASSERT(relatedFontPtr);
 
     SkPDFFont* relatedFont = NULL;
     for (int i = 0; i < fFontRecords.count(); ++i) {
         SkPDFFont::Match match = SkPDFFont::IsMatch(
                 fFontRecords[i].fFont, fFontRecords[i].fFontID,
@@ skipping 22 matching lines @@
         if (fFontRecords[i].fFont == pdfFont) {
             fFontRecords.removeShuffle(i);
             return;
         }
     }
     // Not all SkPDFFonts are added to the Canon.
 }
 
 ////////////////////////////////////////////////////////////////////////////////
 
-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));
 }
 
-void SkPDFCanon::addShader(SkPDFShader* shader) {
+////////////////////////////////////////////////////////////////////////////////
+
+SkPDFAlphaFunctionShader* SkPDFCanon::findAlphaShader(
+        const SkPDFShader::State& state) const {
     assert_mutex_held(this, gSkPDFCanonShaderMutex);
-    SkASSERT(shader);
-    fShaderRecords.push(shader);
+    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::removeShader(SkPDFShader* pdfShader) {
+////////////////////////////////////////////////////////////////////////////////
+
+SkPDFImageShader* SkPDFCanon::findImageShader(
+        const SkPDFShader::State& state) const {
     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");
+    return find_item(fImageShaderRecords, state);
+}
+
+void SkPDFCanon::addImageShader(SkPDFImageShader* pdfShader) {
+    assert_mutex_held(this, gSkPDFCanonShaderMutex);
+    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));
 }