Code clean-up in AesDecryptor and test.

media/crypto/aes_decryptor.cc

Index: media/crypto/aes_decryptor.cc
diff --git a/media/crypto/aes_decryptor.cc b/media/crypto/aes_decryptor.cc
index e69f4062a8afb3f00ee9d609a19785da40269efe..8cbd58db84da6354c52163e87155b1ea4781f24a 100644
--- a/media/crypto/aes_decryptor.cc
+++ b/media/crypto/aes_decryptor.cc
@@ -83,53 +83,84 @@ static bool CheckData(const DecoderBuffer& input,
   return true;
 }
 
-// Decrypts |input| using |key|. |encrypted_data_offset| is the number of bytes
-// into |input| that the encrypted data starts.
-// Returns a DecoderBuffer with the decrypted data if decryption succeeded or
-// NULL if decryption failed.
-static scoped_refptr<DecoderBuffer> DecryptData(const DecoderBuffer& input,
-                                                crypto::SymmetricKey* key,
-                                                int encrypted_data_offset) {
-  CHECK(input.GetDataSize());
-  CHECK(input.GetDecryptConfig());
-  CHECK(key);
+// Decrypts |encrypted_data| using |key|.
+// Returns true and fill |decrypted_text| if if decryption succeeded.

[fgalligan1, 2012/07/25 19:33:50]

s/fill/fills/
s/if if/if/

[xhwang, 2012/07/26 23:36:47]

Done.

+// Returns false and leave |decrypted_text| as is if decryption failed.

[fgalligan1, 2012/07/25 19:33:50]

s/leave/leaves/

[ddorwin, 2012/07/26 21:24:07]

I think this is pretty common practice and thus does not need to be stated in
addition to 87.

[xhwang, 2012/07/26 23:36:47]

Done.

+static bool DecryptData(
+    const uint8* encrypted_data, int encrypted_data_size,
+    const uint8* iv, int iv_size,
+    crypto::SymmetricKey* key,
+    std::string* decrypted_text) {

[ddorwin, 2012/07/26 21:24:07]

nit: Odd that the output is a different type from the input.
Also odd that the names don't match (data vs. text).

[xhwang, 2012/07/26 23:36:47]

Done.

+  DCHECK(decrypted_text->empty());

[fgalligan1, 2012/07/25 19:33:50]

Why does decrypted_text need to be empty()?

[xhwang, 2012/07/26 23:36:47]

Done.

 
   // Initialize decryptor.
   crypto::Encryptor encryptor;
   if (!encryptor.Init(key, crypto::Encryptor::CTR, "")) {
     DVLOG(1) << "Could not initialize decryptor.";
-    return NULL;
+    return false;
   }
 
-  DCHECK_EQ(input.GetDecryptConfig()->iv_size(),
-            DecryptConfig::kDecryptionKeySize);
+  DCHECK_EQ(iv_size, DecryptConfig::kDecryptionKeySize);
   // Set the counter block.
-  base::StringPiece counter_block(
-      reinterpret_cast<const char*>(input.GetDecryptConfig()->iv()),
-      input.GetDecryptConfig()->iv_size());
+  base::StringPiece counter_block(reinterpret_cast<const char*>(iv),iv_size);

[ddorwin, 2012/07/26 21:24:07]

space after ,

[xhwang, 2012/07/26 23:36:47]

Done.

   if (counter_block.empty()) {
     DVLOG(1) << "Could not generate counter block.";
-    return NULL;
+    return false;
   }
   if (!encryptor.SetCounter(counter_block)) {
     DVLOG(1) << "Could not set counter block.";
-    return NULL;
+    return false;
   }
 
-  std::string decrypted_text;
-  const char* frame =
-      reinterpret_cast<const char*>(input.GetData() + encrypted_data_offset);
-  int frame_size = input.GetDataSize() - encrypted_data_offset;
-  base::StringPiece encrypted_text(frame, frame_size);
-  if (!encryptor.Decrypt(encrypted_text, &decrypted_text)) {
+  base::StringPiece encrypted_text(
+      reinterpret_cast<const char*>(encrypted_data), encrypted_data_size);
+  if (!encryptor.Decrypt(encrypted_text, decrypted_text)) {
     DVLOG(1) << "Could not decrypt data.";
-    return NULL;
+    DCHECK(decrypted_text->empty());

[fgalligan1, 2012/07/25 19:33:50]

I don't think you need this check here if you remove the check above.

[xhwang, 2012/07/26 23:36:47]

Done.

+    return false;
+  }
+
+  DCHECK(!decrypted_text->empty());
+  return true;
+}
+
+// Decrypts |encrypted_buffer| using |key|.
+// Fires the |decrypt_cb| with kSuccess and the decrypted buffer if decryption
+// succeeded.
+// Fires the |decrypt_cb| with kError and NULL if decryption failed.

[ddorwin, 2012/07/26 21:24:07]

Otherwise fires ... NULL.

[xhwang, 2012/07/26 23:36:47]

Done.

+static void DecryptBuffer(const Decryptor::DecryptCB& decrypt_cb,
+                          const scoped_refptr<DecoderBuffer>& encrypted_buffer,
+                          crypto::SymmetricKey* key) {
+  CHECK(encrypted_buffer->GetDataSize());
+  CHECK(encrypted_buffer->GetDecryptConfig());
+  CHECK(key);
+
+  std::string decrypted_text;
+  int encrypted_data_offset =
+      encrypted_buffer->GetDecryptConfig()->encrypted_frame_offset();
+
+  bool succeed = DecryptData(
+      encrypted_buffer->GetData() + encrypted_data_offset,
+      encrypted_buffer->GetDataSize() - encrypted_data_offset,
+      encrypted_buffer->GetDecryptConfig()->iv(),
+      encrypted_buffer->GetDecryptConfig()->iv_size(),
+      key,
+      &decrypted_text);
+
+  if (!succeed) {
+    DVLOG(1) << "Decryption failed.";
+    decrypt_cb.Run(Decryptor::kError, NULL);
+    return;
   }
 
   // TODO(xhwang): Find a way to avoid this data copy.
-  return DecoderBuffer::CopyFrom(
+  scoped_refptr<DecoderBuffer> decrypted_buffer = DecoderBuffer::CopyFrom(
       reinterpret_cast<const uint8*>(decrypted_text.data()),
       decrypted_text.size());
+
+  decrypted_buffer->SetTimestamp(encrypted_buffer->GetTimestamp());
+  decrypted_buffer->SetDuration(encrypted_buffer->GetDuration());
+  decrypt_cb.Run(Decryptor::kSuccess, decrypted_buffer);
 }
 
 AesDecryptor::AesDecryptor(DecryptorClient* client)
@@ -199,16 +230,7 @@ void AesDecryptor::AddKey(const std::string& key_system,
     return;
   }
 
-  {
-    base::AutoLock auto_lock(key_map_lock_);
-    KeyMap::iterator found = key_map_.find(key_id_string);
-    if (found != key_map_.end()) {
-      delete found->second;
-      key_map_.erase(found);
-    }
-    key_map_[key_id_string] = decryption_key.release();
-  }
-
+  SetKey(key_id_string, decryption_key.Pass());
   client_->KeyAdded(key_system, session_id);
 }
 
@@ -224,18 +246,10 @@ void AesDecryptor::Decrypt(const scoped_refptr<DecoderBuffer>& encrypted,
 
   // TODO(xhwang): Avoid always constructing a string with StringPiece?
   std::string key_id_string(reinterpret_cast<const char*>(key_id), key_id_size);
-
-  DecryptionKey* key = NULL;
-  {
-    base::AutoLock auto_lock(key_map_lock_);
-    KeyMap::const_iterator found = key_map_.find(key_id_string);
-    if (found != key_map_.end())
-      key = found->second;
-  }
+  DecryptionKey* key = GetKey(key_id_string);
 
   if (!key) {
-    // TODO(fgalligan): Fire a need_key event here and add a test.
-    DVLOG(1) << "Could not find a matching key for given key ID.";
+    DVLOG(1) << "Could not find a matching key for the given key ID.";

[ddorwin, 2012/07/26 21:24:07]

This is still temporary, right? So there should still be a TODO (maybe for
xhwang).

[xhwang, 2012/07/26 23:36:47]

Done.

     decrypt_cb.Run(kError, NULL);
     return;
   }
@@ -252,23 +266,30 @@ void AesDecryptor::Decrypt(const scoped_refptr<DecoderBuffer>& encrypted,
     return;
   }
 
-  scoped_refptr<DecoderBuffer> decrypted =
-      DecryptData(*encrypted,
-                  key->decryption_key(),
-                  encrypted->GetDecryptConfig()->encrypted_frame_offset());
-  if (!decrypted) {
-    DVLOG(1) << "Decryption failed.";
-    decrypt_cb.Run(kError, NULL);
-    return;
+  DecryptBuffer(decrypt_cb, encrypted, key->decryption_key());
+}
+
+void AesDecryptor::SetKey(const std::string& key_id,
+                          scoped_ptr<DecryptionKey> decryption_key) {

[ddorwin, 2012/07/26 21:24:07]

Why not just a raw pointer with a note in .h that this function takes ownership?

[xhwang, 2012/07/26 23:36:47]

Passing in scoped_ptr<> enforces that the AesDecryptor takes the ownership,
which IMHO is better.

+  base::AutoLock auto_lock(key_map_lock_);
+  KeyMap::iterator found = key_map_.find(key_id);
+  if (found != key_map_.end()) {
+    delete found->second;
+    key_map_.erase(found);
   }
+  key_map_[key_id] = decryption_key.release();
+}
 
-  decrypted->SetTimestamp(encrypted->GetTimestamp());
-  decrypted->SetDuration(encrypted->GetDuration());
-  decrypt_cb.Run(kSuccess, decrypted);
+AesDecryptor::DecryptionKey* AesDecryptor::GetKey(
+    const std::string& key_id) const {
+  base::AutoLock auto_lock(key_map_lock_);
+  KeyMap::const_iterator found = key_map_.find(key_id);
+  if (found != key_map_.end())

[ddorwin, 2012/07/26 21:24:07]

I'd swap such that the success case is at the end of the function. (That's a
more common pattern and uses == instead of !=.)

[xhwang, 2012/07/26 23:36:47]

Done.

+    return found->second;
+  return NULL;
 }
 
-AesDecryptor::DecryptionKey::DecryptionKey(
-    const std::string& secret)
+AesDecryptor::DecryptionKey::DecryptionKey(const std::string& secret)
     : secret_(secret) {
 }