Generalize AesDecryptor to make it more spec compliant.

media/crypto/aes_decryptor.h

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef MEDIA_CRYPTO_AES_DECRYPTOR_H_
 #define MEDIA_CRYPTO_AES_DECRYPTOR_H_
 
 #include <string>
 
 #include "base/basictypes.h"
 #include "base/hash_tables.h"
 #include "base/memory/ref_counted.h"
 #include "base/synchronization/lock.h"
 #include "media/base/media_export.h"
 
 namespace crypto {
 class SymmetricKey;
 }
 
 namespace media {
 
 class DecoderBuffer;
+class DecryptorClient;
 
 // Decrypts AES encrypted buffer into unencrypted buffer.
 class MEDIA_EXPORT AesDecryptor {
  public:
+  enum KeyError {
+    UnknownError = 1,

[scherkus (not reviewing), 2012/06/12 03:15:58]

these all get k prefix

[xhwang, 2012/06/12 19:01:15]

Done.

+    ClientError,
+    ServiceError,
+    OutputError,
+    HardwareChangeError,
+    DomainError
+  };
+
   AesDecryptor();
   ~AesDecryptor();
 
-  // Add a |key_id| and |key| pair to the key system. The key is not limited to
-  // a decryption key. It can be any data that the key system accepts, such as
-  // a license. If multiple calls of this function set different keys for the
-  // same |key_id|, the older key will be replaced by the newer key.
-  void AddKey(const uint8* key_id, int key_id_size,
-              const uint8* key, int key_size);
+  // The AesDecryptor does not take ownership of the |client|. The |client|
+  // must be valid throughout the lifeime of the AesDecryptor.

[ddorwin, 2012/06/11 21:02:40]

lifetime

[xhwang, 2012/06/12 19:01:15]

Done.

+  void Init(DecryptorClient* client);

[scherkus (not reviewing), 2012/06/12 03:15:58]

why not pass into ctor?

I know you eventually want to make this an interface at some point but I have a
feeling we'll still be able to pass the client pointer into the ctor even after
the interface is generalized or at least make that change when needed

[xhwang, 2012/06/12 19:01:15]

Done. It actually make the code much cleaner. Thanks!

 
-  // Decrypt |input| buffer. The |input| should not be NULL.
-  // Return a DecoderBuffer with the decrypted data if decryption succeeded.
-  // Return NULL if decryption failed.
+  // Generates a key request. The result of this call will be reported via the
+  // client's KeyMessage or KeyError methods.

[scherkus (not reviewing), 2012/06/12 03:15:58]

suffix fn names with () in docs here + below

[xhwang, 2012/06/12 19:01:15]

Done.

+  void GenerateKeyRequest(const std::string& key_system,
+                          const uint8* init_data,
+                          int init_data_length);
+
+  // Adds a |key| to the key system. The key is not limited to a decryption key.
+  // It can be any data that the key system accepts, such as a license.
+  // If multiple calls of this function set different keys for the same
+  // |key_id|, the older key will be replaced by the newer key.
+  // The result of this call will be reported via the client's KeyAdded,
+  // KeyMessage or KeyError methods.
+  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,

[ddorwin, 2012/06/11 21:02:40]

// Cancels the key request specified by |session_id|.

[xhwang, 2012/06/12 19:01:15]

Done.

+                        const std::string& session_id);
+
+  // Decrypts the |input| buffer, which should not be NULL.
+  // Returns a DecoderBuffer with the decrypted data if decryption succeeded.
+  // Returns NULL if decryption failed.
   scoped_refptr<DecoderBuffer> Decrypt(
       const scoped_refptr<DecoderBuffer>& input);
 
  private:
   // KeyMap owns the crypto::SymmetricKey* and must delete them when they are
   // not needed any more.
   typedef base::hash_map<std::string, crypto::SymmetricKey*> KeyMap;
-  KeyMap key_map_;
-  base::Lock lock_;
+  KeyMap key_map_;  // Protected by the |lock_|.
+  base::Lock lock_;  // Protects the |key_map_|.

[ddorwin, 2012/06/11 21:02:40]

if it is specific to the key_map_, you might name it key_map_lock_.

[xhwang, 2012/06/12 19:01:15]

Done.

+
+  DecryptorClient* client_;
+
+  uint32_t session_id_;

[ddorwin, 2012/06/11 21:02:40]

last_session_id_ or session_id_incrementer_ 

[scherkus (not reviewing), 2012/06/12 03:15:58]

I like "next_session_id_" then inside the .cc convert to a string prior to
incrementing

[xhwang, 2012/06/12 19:01:15]

Done.

[xhwang, 2012/06/12 19:01:15]

Done.

 
   DISALLOW_COPY_AND_ASSIGN(AesDecryptor);
 };
 
 }  // namespace media
 
 #endif  // MEDIA_CRYPTO_AES_DECRYPTOR_H_