Swizzler changes Index8 and 565

include/core/SkTemplates.h

Index: include/core/SkTemplates.h
diff --git a/include/core/SkTemplates.h b/include/core/SkTemplates.h
index 1d22a642dae3ba959d4f7a9a9bee8fb4ff7072e7..a8b276950d91dc3e413eda568f6c6bfbc65fe40f 100644
--- a/include/core/SkTemplates.h
+++ b/include/core/SkTemplates.h
@@ -112,6 +112,14 @@ public:
     T* operator->() const { SkASSERT(fObj); return fObj; }
 
     T* detach() { T* obj = fObj; fObj = NULL; return obj; }
+    void reset(T* obj = NULL) {
+        if (fObj != obj) {
+            if (fObj) {
+                P(fObj);
+            }
+            fObj = obj;
+        }
+    }

[msarett, 2015/04/09 16:26:32]

I'm sure it is possible to implement gif rewinding without this API, but it
seemed to me that having this API was the most straightforward way to do it.

Also, it seems natural to have this API available.  An identical function exists
for SkAutoTCallVProc.  Why not for SkAutoTCallIProc?

I am a little unsure if maybe we would want it to return an int though?

[scroggo, 2015/04/09 17:37:17]

git blame is your friend. Using blame, I see we added SkAutoTCallVProc::reset in
https://codereview.chromium.org/396143004, when it was needed.

So it was probably just an oversight because we didn't need it for
SkAutoTCallIProc. I think adding this is reasonable.
That seems reasonable to me, since P returns an int. One thing I am unsure
about, though, is what to return in the following cases:

fObj == obj
!fObj

Both of these are *not* failure cases, but we will not have called P(fObj). We
do not know what return values of P are meaningful, so how can we choose what to
return?

This would be an argument against returning an int (maybe with a comment
explaining that). An alternative (ugly) approach would be to use out
parameter(s, or one out parameter + return value) to specify whether we called P
and if so, what the return value is.

I think a cleaner approach is to decide that if a client wants to know the
return value of P, they have to do the following:

T* obj = autoProc.detach();
int val = P(obj);
// Do something with val
autoProc.reset(newObj);

You'll need API approval for such a change, so you can follow up with reed@.

Looking back at your callsite, though, you do not ever care about the return
value of P(), and it's a function you wrote (SkGifCodec::CloseGif). I think you
should change it to return void, and then you can call SkAutoTCallVProc, which
already has reset().

[msarett, 2015/04/09 19:21:31]

All good points.  Seems like using SkAutoTCallCProc is a no brainer.  We don't
ever need the return value.

 private:
     T* fObj;
 };