Separate MediaKeys interface from Decryptor interface.

webkit/media/crypto/proxy_decryptor.h

Index: webkit/media/crypto/proxy_decryptor.h
diff --git a/webkit/media/crypto/proxy_decryptor.h b/webkit/media/crypto/proxy_decryptor.h
index 3e81ce37e7b5bbae46e8f7d5c8a2cb981789f74b..5e3d3d7fd6c916c0b8525b2a4db262987f9c3849 100644
--- a/webkit/media/crypto/proxy_decryptor.h
+++ b/webkit/media/crypto/proxy_decryptor.h
@@ -11,6 +11,7 @@
 #include "base/memory/weak_ptr.h"
 #include "base/synchronization/lock.h"
 #include "media/base/decryptor.h"
+#include "media/base/media_keys.h"
 
 namespace WebKit {
 class WebFrame;
@@ -19,11 +20,13 @@ class WebMediaPlayerClient;
 
 namespace webkit_media {
 
+class PpapiDecryptor;
+
 // A decryptor proxy that creates a real decryptor object on demand and
 // forwards decryptor calls to it.
 // TODO(xhwang): Currently we don't support run-time switching among decryptor
 // objects. Fix this when needed.
-class ProxyDecryptor {
+class ProxyDecryptor : public media::MediaKeys {

[ddorwin, 2013/05/24 20:39:12]

We should probably rename this class in a future CL.

[xhwang, 2013/05/24 23:15:56]

Any idea on the new name? How about MediaKeysManager or DecryptorManager?

[ddorwin, 2013/05/28 19:44:33]

I'm not sure. It still hides the separation of Decryptor/MediaKeys from WMPI.
Maybe it will fade away with the new APIs.

At the very least, it's not a Decryptor - it contains one. DecryptorProxy or
DecryptorAdapter would be a big improvement. I don't think it's worth changing
at this point since we don't know what it should be. However, you might add a
TODO here that notes we need to change the name or delete this.

[xhwang, 2013/05/28 23:50:39]

Added TODO.

  public:
   ProxyDecryptor(WebKit::WebMediaPlayerClient* web_media_player_client,
                  WebKit::WebFrame* web_frame,
@@ -39,29 +42,31 @@ class ProxyDecryptor {
   // NULL immediately and reset.
   void SetDecryptorReadyCB(const media::DecryptorReadyCB& decryptor_ready_cb);
 
-  bool GenerateKeyRequest(const std::string& key_system,
-                          const std::string& type,
-                          const uint8* init_data, int init_data_length);
-  void AddKey(const std::string& key_system,
-              const uint8* key, int key_length,
-              const uint8* init_data, int init_data_length,
-              const std::string& session_id);
-  void CancelKeyRequest(const std::string& key_system,
-                        const std::string& session_id);
+  // MediaKeys implementation.
+  virtual bool GenerateKeyRequest(const std::string& key_system,
+                                  const std::string& type,
+                                  const uint8* init_data,
+                                  int init_data_length) OVERRIDE;
+  virtual void AddKey(const std::string& key_system,
+                      const uint8* key, int key_length,
+                      const uint8* init_data, int init_data_length,
+                      const std::string& session_id) OVERRIDE;
+  virtual void CancelKeyRequest(const std::string& key_system,
+                                const std::string& session_id) OVERRIDE;
 
  private:
   // Helper functions to create decryptors to handle the given |key_system|.
 #if defined(ENABLE_PEPPER_CDMS)
-  scoped_ptr<media::Decryptor> CreatePpapiDecryptor(
+  scoped_ptr<PpapiDecryptor> CreatePpapiDecryptor(
       const std::string& key_system);
 #endif  // defined(ENABLE_PEPPER_CDMS)
-  scoped_ptr<media::Decryptor> CreateDecryptor(const std::string& key_system);
+  bool CreateDecryptor(const std::string& key_system);
 
   // Callbacks for firing key events.
   void KeyAdded(const std::string& key_system, const std::string& session_id);
   void KeyError(const std::string& key_system,
                 const std::string& session_id,
-                media::Decryptor::KeyError error_code,
+                media::MediaKeys::KeyError error_code,
                 int system_code);
   void KeyMessage(const std::string& key_system,
                   const std::string& session_id,
@@ -89,9 +94,12 @@ class ProxyDecryptor {
 
   media::DecryptorReadyCB decryptor_ready_cb_;
 
-  // The real decryptor that does decryption for the ProxyDecryptor.
-  // This pointer is protected by the |lock_|.
-  scoped_ptr<media::Decryptor> decryptor_;
+  // The real MediaKeys and Decryptor objects that perform key operations and
+  // decryption for the ProxyDecryptor. These two pointers refer to the same
+  // object and |media_keys_| takes the ownership. They are protected by the
+  // |lock_|.
+  scoped_ptr<media::MediaKeys> media_keys_;
+  media::Decryptor* decryptor_;

[xhwang, 2013/05/24 00:25:12]

I like keeping MediaKeys and Decryptor separate. But this is the ugly part of
this CL that I don't like.

I am thinking: In desktop chromium's implementation the real Decryptor
(AesDecryptor and PpapiDecryptor) always implements both MediaKeys and Decryptor
interface. So maybe Decryptor interface could also inherit MediaKeys interface.
Then we don't have this problem (two pointers for the same object).

WDYT?

[ddorwin, 2013/05/24 20:39:12]

The inheritance would be for convenience and not really any real relationship.
What about exposing MediaKeys from the Decryptor? (I really like the idea of a
Decryptor(MediaKeys*) constructor, but that may not actually be practical. This
is close.

See my comment in the .cc file, which I think makes an argument for exposing it
in this way.

[xhwang, 2013/05/24 23:15:56]

Done.

 
   base::WeakPtrFactory<ProxyDecryptor> weak_ptr_factory_;