SkRemote: refactoring

  - Cache becomes CachingEncoder that wraps another Encoder
  - Encoders provide IDs
  - syntaxy improvements to Client
  - ID isn't really protocol sensitive.
  - I don't think we need Type::kNone.

No diffs.
https://gold.skia.org/search2?issue=1418863002&unt=true&query=source_type%3Dgm&master=false&include=true

BUG=skia:

Review URL: https://codereview.chromium.org/1418863002

3 files changed, 162 insertions, 257 deletions

diff --git a/src/core/SkRemote.cpp b/src/core/SkRemote.cpp
index 7c662d3233..21fbdf3d97 100644
--- a/src/core/SkRemote.cpp
+++ b/src/core/SkRemote.cpp
@@ -72,210 +72,141 @@ namespace SkRemote {
 
     // ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ //
 
-    class LookupScope {
+    class CachingEncoder final : public Encoder {
     public:
-        LookupScope(Cache* cache, Encoder* encoder) : fCache(cache), fEncoder(encoder) {}
-        ~LookupScope() { for (ID id : fToUndefine) { fEncoder->undefine(id); } }
-        void undefineWhenDone(ID id) { fToUndefine.push_back(id); }
-
-        template <typename T>
-        ID lookup(const T& val) {
-            ID id;
-            if (!fCache->lookup(val, &id, this)) {
-                fEncoder->define(id, val);
-            }
-            return id;
-        }
+        explicit CachingEncoder(Encoder* wrapped) : fWrapped(wrapped) {}
 
     private:
-        Cache*   fCache;
-        Encoder* fEncoder;
-        SkSTArray<4, ID> fToUndefine;
-    };
+        struct Undef {
+            Encoder* fEncoder;
+            template <typename T>
+            void operator()(const T&, ID* id) const { fEncoder->undefine(*id); }
+        };
 
+        ~CachingEncoder() override {
+            Undef undef{fWrapped};
+            fMatrix  .foreach(undef);
+            fMisc    .foreach(undef);
+            fPath    .foreach(undef);
+            fStroke  .foreach(undef);
+            fShader  .foreach(undef);
+            fXfermode.foreach(undef);
+        }
 
-    Cache* Cache::CreateNeverCache() {
-        struct NeverCache final : public Cache {
-            NeverCache()
-                : fNextMatrix  (Type::kMatrix)
-                , fNextMisc    (Type::kMisc)
-                , fNextPath    (Type::kPath)
-                , fNextStroke  (Type::kStroke)
-                , fNextShader  (Type::kShader)
-                , fNextXfermode(Type::kXfermode)
-            {}
-            void cleanup(Encoder*) override {}
-
-            static bool Helper(ID* next, ID* id, LookupScope* ls) {
-                *id = ++(*next);
-                ls->undefineWhenDone(*id);
-                return false;
+        template <typename Map, typename T>
+        ID define(Map* map, const T& v) {
+            if (const ID* id = map->find(v)) {
+                return *id;
             }
+            ID id = fWrapped->define(v);
+            map->set(v, id);
+            return id;
+        }
 
-            bool lookup(const SkMatrix&, ID* id, LookupScope* ls) override {
-                return Helper(&fNextMatrix, id, ls);
-            }
-            bool lookup(const Misc&, ID* id, LookupScope* ls) override {
-                return Helper(&fNextMisc, id, ls);
-            }
-            bool lookup(const SkPath&, ID* id, LookupScope* ls) override {
-                return Helper(&fNextPath, id, ls);
-            }
-            bool lookup(const Stroke&, ID* id, LookupScope* ls) override {
-                return Helper(&fNextStroke, id, ls);
-            }
-            bool lookup(const SkShader* shader, ID* id, LookupScope* ls) override {
-                if (!shader) {
-                    *id = ID(Type::kShader);
-                    return true;  // Null IDs are always defined.
-                }
-                return Helper(&fNextShader, id, ls);
-            }
-            bool lookup(const SkXfermode* xfermode, ID* id, LookupScope* ls) override {
-                if (!xfermode) {
-                    *id = ID(Type::kXfermode);
-                    return true;  // Null IDs are always defined.
-                }
-                return Helper(&fNextXfermode, id, ls);
-            }
+        ID define(const SkMatrix& v) override { return this->define(&fMatrix,   v); }
+        ID define(const Misc&     v) override { return this->define(&fMisc,     v); }
+        ID define(const SkPath&   v) override { return this->define(&fPath,     v); }
+        ID define(const Stroke&   v) override { return this->define(&fStroke,   v); }
+        ID define(SkShader*       v) override { return this->define(&fShader,   v); }
+        ID define(SkXfermode*     v) override { return this->define(&fXfermode, v); }
 
-            ID fNextMatrix,
-               fNextMisc,
-               fNextPath,
-               fNextStroke,
-               fNextShader,
-               fNextXfermode;
-        };
-        return new NeverCache;
-    }
+        void undefine(ID) override {}
 
-    // These can't be declared locally inside AlwaysCache because of the templating.  :(
-    namespace {
-        template <typename T, typename Map>
-        static bool always_cache_lookup(const T& val, Map* map, ID* next, ID* id) {
-            if (const ID* found = map->find(val)) {
-                *id = *found;
-                return true;
-            }
-            *id = ++(*next);
-            map->set(val, *id);
-            return false;
-        }
+        void    save() override { fWrapped->   save(); }
+        void restore() override { fWrapped->restore(); }
 
-        struct Undefiner {
-            Encoder* fEncoder;
+        void setMatrix(ID matrix) override { fWrapped->setMatrix(matrix); }
 
-            template <typename T>
-            void operator()(const T&, ID* id) const { fEncoder->undefine(*id); }
-        };
+        void clipPath(ID path, SkRegion::Op op, bool aa) override {
+            fWrapped->clipPath(path, op, aa);
+        }
+        void fillPath(ID path, ID misc, ID shader, ID xfermode) override {
+            fWrapped->fillPath(path, misc, shader, xfermode);
+        }
+        void strokePath(ID path, ID misc, ID shader, ID xfermode, ID stroke) override {
+            fWrapped->strokePath(path, misc, shader, xfermode, stroke);
+        }
 
-        // Maps const T* -> ID, and refs the key.  nullptr always maps to ID(kType).
+        // Maps const T* -> ID, and refs the key.
         template <typename T, Type kType>
         class RefKeyMap {
         public:
             RefKeyMap() {}
-            ~RefKeyMap() { fMap.foreach([](const T* key, ID*) { key->unref(); }); }
+            ~RefKeyMap() { fMap.foreach([](const T* key, ID*) { SkSafeUnref(key); }); }
 
-            void set(const T* key, const ID& id) {
-                SkASSERT(key && id.type() == kType);
-                fMap.set(SkRef(key), id);
+            void set(const T* key, ID id) {
+                SkASSERT(id.type() == kType);
+                fMap.set(SkSafeRef(key), id);
             }
 
             void remove(const T* key) {
-                SkASSERT(key);
                 fMap.remove(key);
-                key->unref();
+                SkSafeUnref(key);
             }
 
             const ID* find(const T* key) const {
-                static const ID nullID(kType);
-                return key ? fMap.find(key) : &nullID;
+                return fMap.find(key);
             }
 
             template <typename Fn>
-            void foreach(const Fn& fn) { fMap.foreach(fn); }
+            void foreach(const Fn& fn) {
+                fMap.foreach(fn);
+            }
         private:
             SkTHashMap<const T*, ID> fMap;
         };
-    } // namespace
-
-    Cache* Cache::CreateAlwaysCache() {
-        struct AlwaysCache final : public Cache {
-            AlwaysCache()
-                : fNextMatrix  (Type::kMatrix)
-                , fNextMisc    (Type::kMisc)
-                , fNextPath    (Type::kPath)
-                , fNextStroke  (Type::kStroke)
-                , fNextShader  (Type::kShader)
-                , fNextXfermode(Type::kXfermode)
-            {}
-
-            void cleanup(Encoder* encoder) override {
-                Undefiner undef{encoder};
-                fMatrix  .foreach(undef);
-                fMisc    .foreach(undef);
-                fPath    .foreach(undef);
-                fStroke  .foreach(undef);
-                fShader  .foreach(undef);
-                fXfermode.foreach(undef);
-            }
 
+        SkTHashMap<SkMatrix, ID>               fMatrix;
+        SkTHashMap<Misc, ID, MiscHash>         fMisc;
+        SkTHashMap<SkPath, ID>                 fPath;
+        SkTHashMap<Stroke, ID>                 fStroke;
+        RefKeyMap<SkShader, Type::kShader>     fShader;
+        RefKeyMap<SkXfermode, Type::kXfermode> fXfermode;
 
-            bool lookup(const SkMatrix& matrix, ID* id, LookupScope*) override {
-                return always_cache_lookup(matrix, &fMatrix, &fNextMatrix, id);
-            }
-            bool lookup(const Misc& misc, ID* id, LookupScope*) override {
-                return always_cache_lookup(misc, &fMisc, &fNextMisc, id);
-            }
-            bool lookup(const SkPath& path, ID* id, LookupScope*) override {
-                return always_cache_lookup(path, &fPath, &fNextPath, id);
-            }
-            bool lookup(const Stroke& stroke, ID* id, LookupScope*) override {
-                return always_cache_lookup(stroke, &fStroke, &fNextStroke, id);
-            }
-            bool lookup(const SkShader* shader, ID* id, LookupScope*) override {
-                return always_cache_lookup(shader, &fShader, &fNextShader, id);
-            }
-            bool lookup(const SkXfermode* xfermode, ID* id, LookupScope*) override {
-                return always_cache_lookup(xfermode, &fXfermode, &fNextXfermode, id);
-            }
+        Encoder* fWrapped;
+    };
 
-            SkTHashMap<SkMatrix, ID>               fMatrix;
-            SkTHashMap<Misc,     ID, MiscHash>     fMisc;
-            SkTHashMap<SkPath,   ID>               fPath;
-            SkTHashMap<Stroke,   ID>               fStroke;
-            RefKeyMap<SkShader,   Type::kShader>   fShader;
-            RefKeyMap<SkXfermode, Type::kXfermode> fXfermode;
-
-            ID fNextMatrix,
-               fNextMisc,
-               fNextPath,
-               fNextStroke,
-               fNextShader,
-               fNextXfermode;
-        };
-        return new AlwaysCache;
-    }
+    Encoder* Encoder::CreateCachingEncoder(Encoder* wrapped) { return new CachingEncoder(wrapped); }
+
+    // ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ //
+
+    // Calls Encoder::define() when created, Encoder::undefine() when destroyed.
+    class Client::AutoID : ::SkNoncopyable {
+    public:
+        template <typename T>
+        explicit AutoID(Encoder* encoder, const T& val)
+            : fEncoder(encoder)
+            , fID(encoder->define(val)) {}
+        ~AutoID() { if (fEncoder) fEncoder->undefine(fID); }
+
+        AutoID(AutoID&& o) : fEncoder(o.fEncoder), fID(o.fID) {
+            o.fEncoder = nullptr;
+        }
+        AutoID& operator=(AutoID&&) = delete;
+
+        operator ID () const { return fID; }
+
+    private:
+        Encoder* fEncoder;
+        const ID fID;
+    };
+
+    template <typename T>
+    Client::AutoID Client::id(const T& val) { return AutoID(fEncoder, val); }
 
     // ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ //
 
-    Client::Client(Cache* cache, Encoder* encoder)
+    Client::Client(Encoder* encoder)
         : SkCanvas(1,1)
-        , fCache(cache)
         , fEncoder(encoder)
     {}
 
-    Client::~Client() {
-        fCache->cleanup(fEncoder);
-    }
-
     void Client::willSave()   { fEncoder->save(); }
     void Client::didRestore() { fEncoder->restore(); }
 
     void Client::didConcat   (const SkMatrix&) { this->didSetMatrix(this->getTotalMatrix()); }
     void Client::didSetMatrix(const SkMatrix& matrix) {
-        LookupScope ls(fCache, fEncoder);
-        fEncoder->setMatrix(ls.lookup(matrix));
+        fEncoder->setMatrix(this->id(matrix));
     }
 
     void Client::onDrawOval(const SkRect& oval, const SkPaint& paint) {
@@ -306,17 +237,16 @@ namespace SkRemote {
     }
 
     void Client::onDrawPath(const SkPath& path, const SkPaint& paint) {
-        LookupScope ls(fCache, fEncoder);
-        ID p = ls.lookup(path),
-           m = ls.lookup(Misc::CreateFrom(paint)),
-           s = ls.lookup(paint.getShader()),
-           x = ls.lookup(paint.getXfermode());
+        auto p = this->id(path),
+             m = this->id(Misc::CreateFrom(paint)),
+             s = this->id(paint.getShader()),
+             x = this->id(paint.getXfermode());
 
         if (paint.getStyle() == SkPaint::kFill_Style) {
             fEncoder->fillPath(p, m, s, x);
         } else {
             // TODO: handle kStrokeAndFill_Style
-            fEncoder->strokePath(p, m, s, x, ls.lookup(Stroke::CreateFrom(paint)));
+            fEncoder->strokePath(p, m, s, x, this->id(Stroke::CreateFrom(paint)));
         }
     }
 
@@ -368,20 +298,26 @@ namespace SkRemote {
     }
 
     void Client::onClipPath(const SkPath& path, SkRegion::Op op, ClipEdgeStyle edgeStyle) {
-        LookupScope ls(fCache, fEncoder);
-        fEncoder->clipPath(ls.lookup(path), op, edgeStyle == kSoft_ClipEdgeStyle);
+        fEncoder->clipPath(this->id(path), op, edgeStyle == kSoft_ClipEdgeStyle);
     }
 
     // ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ //
 
     Server::Server(SkCanvas* canvas) : fCanvas(canvas) {}
 
-    void Server::define(ID id, const SkMatrix& v) { fMatrix  .set(id, v); }
-    void Server::define(ID id, const Misc&     v) { fMisc    .set(id, v); }
-    void Server::define(ID id, const SkPath&   v) { fPath    .set(id, v); }
-    void Server::define(ID id, const Stroke&   v) { fStroke  .set(id, v); }
-    void Server::define(ID id, SkShader*       v) { fShader  .set(id, v); }
-    void Server::define(ID id, SkXfermode*     v) { fXfermode.set(id, v); }
+    template <typename Map, typename T>
+    ID Server::define(Type type, Map* map, const T& val) {
+        ID id(type, fNextID++);
+        map->set(id, val);
+        return id;
+    }
+
+    ID Server::define(const SkMatrix& v) { return this->define(Type::kMatrix,   &fMatrix,   v); }
+    ID Server::define(const Misc&     v) { return this->define(Type::kMisc,     &fMisc,     v); }
+    ID Server::define(const SkPath&   v) { return this->define(Type::kPath,     &fPath,     v); }
+    ID Server::define(const Stroke&   v) { return this->define(Type::kStroke,   &fStroke,   v); }
+    ID Server::define(SkShader*       v) { return this->define(Type::kShader,   &fShader,   v); }
+    ID Server::define(SkXfermode*     v) { return this->define(Type::kXfermode, &fXfermode, v); }
 
     void Server::undefine(ID id) {
         switch(id.type()) {
@@ -391,8 +327,6 @@ namespace SkRemote {
             case Type::kStroke:   return fStroke  .remove(id);
             case Type::kShader:   return fShader  .remove(id);
             case Type::kXfermode: return fXfermode.remove(id);
-
-            case Type::kNone: SkASSERT(false);
         };
     }
 
diff --git a/src/core/SkRemote.h b/src/core/SkRemote.h
index 6e95f3a158..cbfcf9b381 100644
--- a/src/core/SkRemote.h
+++ b/src/core/SkRemote.h
@@ -18,7 +18,26 @@
 // TODO: document
 
 namespace SkRemote {
-    // TODO: document
+
+    // General purpose identifier.  Holds a Type and a 56-bit value.
+    class ID {
+    public:
+        ID() {}
+        ID(Type type, uint64_t val) {
+            fVal = (uint64_t)type << 56 | val;
+            SkASSERT(this->type() == type && this->val() == val);
+        }
+
+        Type    type() const { return (Type)(fVal >> 56); }
+        uint64_t val() const { return fVal & ~((uint64_t)0xFF << 56); }
+
+        bool operator==(ID o) const { return fVal == o.fVal; }
+
+    private:
+        uint64_t fVal;
+    };
+
+    // Fields from SkPaint used by stroke, fill, and text draws.
     struct Misc {
         SkColor         fColor;
         SkFilterQuality fFilterQuality;
@@ -28,7 +47,7 @@ namespace SkRemote {
         void applyTo(SkPaint*) const;
     };
 
-    // TODO: document
+    // Fields from SkPaint used by stroke draws only.
     struct Stroke {
         SkScalar fWidth, fMiter;
         SkPaint::Cap  fCap;
@@ -42,12 +61,14 @@ namespace SkRemote {
     struct Encoder {
         virtual ~Encoder() {}
 
-        virtual void define(ID, const SkMatrix&) = 0;
-        virtual void define(ID, const Misc&)     = 0;
-        virtual void define(ID, const SkPath&)   = 0;
-        virtual void define(ID, const Stroke&)   = 0;
-        virtual void define(ID, SkShader*)       = 0;
-        virtual void define(ID, SkXfermode*)     = 0;
+        static Encoder* CreateCachingEncoder(Encoder*);
+
+        virtual ID define(const SkMatrix&) = 0;
+        virtual ID define(const Misc&)     = 0;
+        virtual ID define(const SkPath&)   = 0;
+        virtual ID define(const Stroke&)   = 0;
+        virtual ID define(SkShader*)       = 0;
+        virtual ID define(SkXfermode*)     = 0;
 
         virtual void undefine(ID) = 0;
 
@@ -64,41 +85,17 @@ namespace SkRemote {
         virtual void strokePath(ID path, ID misc, ID shader, ID xfermode, ID stroke) = 0;
     };
 
-    class LookupScope;
-
-    // The Cache interface encapsulates the caching logic of the Client.
-    //
-    // Each lookup() method must always fill ID* with a valid value,
-    // but ID may be cached.  If so, the lookup() method returns true;
-    // if not the lookup() method returns false and the Client must
-    // then define() this ID -> Thing mapping before using the ID.
-    //
-    // The Caches may also add IDs to the LookupScope's list of IDs to
-    // undefine() on destruction.  This lets the Cache purge IDs.
-    struct Cache {
-        virtual ~Cache() {}
-
-        static Cache* CreateNeverCache();   // Never caches anything.
-        static Cache* CreateAlwaysCache();  // Caches by value (not deep pointer equality).
-        // TODO: static Cache* CreateDeepCache();  // Caches by deep value.
-
-        virtual bool lookup(const SkMatrix&,   ID*, LookupScope*) = 0;
-        virtual bool lookup(const Misc&,       ID*, LookupScope*) = 0;
-        virtual bool lookup(const SkPath&,     ID*, LookupScope*) = 0;
-        virtual bool lookup(const Stroke&,     ID*, LookupScope*) = 0;
-        virtual bool lookup(const SkShader*,   ID*, LookupScope*) = 0;
-        virtual bool lookup(const SkXfermode*, ID*, LookupScope*) = 0;
-
-        virtual void cleanup(Encoder*) = 0;
-    };
-
-    // TODO: document
+    // An SkCanvas that translates to Encoder calls.
     class Client final : public SkCanvas {
     public:
-        Client(Cache*, Encoder*);
-        ~Client();
+        explicit Client(Encoder*);
 
     private:
+        class AutoID;
+
+        template <typename T>
+        AutoID id(const T&);
+
         void   willSave() override;
         void didRestore() override;
 
@@ -123,22 +120,21 @@ namespace SkRemote {
         void onDrawPosTextH(const void*, size_t, const SkScalar[], SkScalar,
                             const SkPaint&) override;
 
-        Cache*   fCache;
         Encoder* fEncoder;
     };
 
-    // TODO: document
+    // An Encoder that translates back to SkCanvas calls.
     class Server final : public Encoder {
     public:
         explicit Server(SkCanvas*);
 
     private:
-        void define(ID, const SkMatrix&) override;
-        void define(ID, const Misc&)     override;
-        void define(ID, const SkPath&)   override;
-        void define(ID, const Stroke&)   override;
-        void define(ID, SkShader*)       override;
-        void define(ID, SkXfermode*)     override;
+        ID define(const SkMatrix&) override;
+        ID define(const Misc&)     override;
+        ID define(const SkPath&)   override;
+        ID define(const Stroke&)   override;
+        ID define(SkShader*)       override;
+        ID define(SkXfermode*)     override;
 
         void undefine(ID) override;
 
@@ -185,25 +181,22 @@ namespace SkRemote {
         template <typename T, Type kType>
         class ReffedIDMap {
         public:
-            ReffedIDMap() {
-                // A null ID always maps to nullptr.
-                fMap.set(ID(kType), nullptr);
-            }
+            ReffedIDMap() {}
             ~ReffedIDMap() {
                 // A well-behaved client always cleans up its definitions.
-                SkASSERT(fMap.count() == 1);
+                SkASSERT(fMap.count() == 0);
             }
 
             void set(const ID& id, T* val) {
-                SkASSERT(id.type() == kType && val);
-                fMap.set(id, SkRef(val));
+                SkASSERT(id.type() == kType);
+                fMap.set(id, SkSafeRef(val));
             }
 
             void remove(const ID& id) {
                 SkASSERT(id.type() == kType);
                 T** val = fMap.find(id);
-                SkASSERT(val && *val);
-                (*val)->unref();
+                SkASSERT(val);
+                SkSafeUnref(*val);
                 fMap.remove(id);
             }
 
@@ -218,6 +211,9 @@ namespace SkRemote {
             SkTHashMap<ID, T*> fMap;
         };
 
+        template <typename Map, typename T>
+        ID define(Type, Map*, const T&);
+
         IDMap<SkMatrix, Type::kMatrix>           fMatrix;
         IDMap<Misc    , Type::kMisc  >           fMisc;
         IDMap<SkPath  , Type::kPath  >           fPath;
@@ -226,6 +222,7 @@ namespace SkRemote {
         ReffedIDMap<SkXfermode, Type::kXfermode> fXfermode;
 
         SkCanvas* fCanvas;
+        uint64_t fNextID = 0;
     };
 
 }  // namespace SkRemote
diff --git a/src/core/SkRemote_protocol.h b/src/core/SkRemote_protocol.h
index d34a4b6c4b..b89d491379 100644
--- a/src/core/SkRemote_protocol.h
+++ b/src/core/SkRemote_protocol.h
@@ -15,8 +15,6 @@ namespace SkRemote {
     // It is safe to append to this enum without breaking protocol compatibility.
     // Resorting, deleting, or inserting anywhere but the end will break compatibility.
     enum class Type : uint8_t {
-        kNone,
-
         kMatrix,
         kMisc,
         kPath,
@@ -25,30 +23,6 @@ namespace SkRemote {
         kXfermode,
     };
 
-    class ID {
-    public:
-        explicit ID(Type type = Type::kNone) : fVal((uint64_t)type << 56) {}
-        ID(Type type, uint64_t val) {
-            fVal = (uint64_t)type << 56 | val;
-            SkASSERT(this->type() == type && this->val() == val);
-        }
-
-        Type    type() const { return (Type)(fVal >> 56); }
-        uint64_t val() const { return fVal & ~((uint64_t)0xFF << 56); }
-
-        bool operator==(ID o) const { return fVal == o.fVal; }
-        ID operator++() {
-            ++fVal;
-            SkASSERT(this->val() != 0);  // Overflow is particularly bad as it'd change our Type.
-            return *this;
-        }
-
-    private:
-        // High 8 bits hold a Type.  Low 56 bits are unique within that Type.
-        // Any change to this format will break protocol compatibility.
-        uint64_t fVal;
-    };
-
 }  // namespace SkRemote
 
 #endif//SkRemote_protocol_DEFINED