VideoDecodeAccelerator now SupportsWeakPtr instead of being RefCountedThreadSafe.

media/video/video_decode_accelerator.h

Index: media/video/video_decode_accelerator.h
diff --git a/media/video/video_decode_accelerator.h b/media/video/video_decode_accelerator.h
index 83d0b95631ca775fdfcb9ec34ef515568481f7dc..b7297ac056816a4dc532f61e9cadc97799635c33 100644
--- a/media/video/video_decode_accelerator.h
+++ b/media/video/video_decode_accelerator.h
@@ -8,6 +8,7 @@
 #include <vector>
 
 #include "base/basictypes.h"
+#include "base/memory/weak_ptr.h"
 #include "media/base/bitstream_buffer.h"
 #include "media/base/video_decoder_config.h"
 #include "media/video/picture.h"
@@ -18,12 +19,11 @@ namespace media {
 // Video decoder interface.
 // This interface is extended by the various components that ultimately
 // implement the backend of PPB_VideoDecode_Dev.
-//
-// No thread-safety guarantees are implied by the use of RefCountedThreadSafe
-// below.
 class MEDIA_EXPORT VideoDecodeAccelerator
-    : public base::RefCountedThreadSafe<VideoDecodeAccelerator> {
+    : public base::SupportsWeakPtr<VideoDecodeAccelerator> {
  public:
+  virtual ~VideoDecodeAccelerator();
+
   // Enumeration of potential errors generated by the API.
   // Note: Keep these in sync with PP_VideoDecodeError_Dev.
   enum Error {
@@ -127,11 +127,11 @@ class MEDIA_EXPORT VideoDecodeAccelerator
   // component is freed.  This call may asynchornously free system resources,
   // but its client-visible effects are synchronous.  After this method returns
   // no more callbacks will be made on the client.
-  virtual void Destroy() = 0;
-
- protected:
-  friend class base::RefCountedThreadSafe<VideoDecodeAccelerator>;
-  virtual ~VideoDecodeAccelerator();
+  // Because the client interface is synchronous but implementations may be
+  // asynchronous, the caller (who may own |this|) can pass ownership of the VDA
+  // *to* the VDA for hidden shenaniganary.  |self| may be NULL if ownership
+  // doesn't need to transfer.
+  virtual void Destroy(scoped_ptr<VideoDecodeAccelerator> self) = 0;

[piman, 2012/07/16 22:16:01]

I can't say I'm a fan of this pattern (commenting here because it impacts all
the implementations)...
Among others, it means the Task takes ownership of self, meaning it may delete
it without calling Destroy, or even delete it on the wrong thread if the message
loop is destroyed before executing the task.
Also, it seems like an implementation detail is leaking in the interface which
is a bit of a code smell...

Is there a way we could either ensure that no inappropriate usage will happen,
or change the interface to avoid this?

[Ami GONE FROM CHROMIUM, 2012/07/16 22:34:04]

Interesting thoughts.
In practice this is only posted to the ChildThread (render thread for host, GPU
thread for impl), so I don't think this is a problem (these are the threads on
which the WeakPtr<>'s would have been tested for validity).
Agreed it smells, but also that it's only a /bit/ of a smell.
I'm not sure what you mean by inappropriate usage.
Easiest way to avoid this would be to make Destroy() async again (I made it sync
(fire-and-forget) in http://codereview.chromium.org/7361010).  I don't remember
the details of why this was necessary now [:(], but IIRC it had to do with
flapper requirements.
I think keeping Destroy() sync presents a better API, even with the need to pass
ownership here.
So, I think I'm happiest leaving it as-is.

Overall I value this CL's improvement in lifecycle clarity significantly more
than I mind this aspect of the API.  WDYT?

 };
 
 }  // namespace media