Code clean-up in AesDecryptor and test.

Code clean-up in AesDecryptor and test.

- Added GetKey() and SetKey() in AesDecryptor.
- Moved static functions out of AesDecryptorTest to make them file scope functions.

BUG=none
TEST=media_unittests

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=148984

[xhwang, 2012-07-25 16:59:22]

Hello ddorwin & fgalligan,

These are some by-products while I am working on the "resume playback" patch.
Since I am refactoring again, it make sense to move these in a separate CL.
PTAL!

xhwang

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

http://codereview.chromium.org/10822013/diff/1/media/crypto/aes_decryptor.cc
File media/crypto/aes_decryptor.cc (right):

http://codereview.chromium.org/10822013/diff/1/media/crypto/aes_decryptor.cc#...
media/crypto/aes_decryptor.cc:87: // Returns true and fill |decrypted_text| if
if decryption succeeded.
s/fill/fills/
s/if if/if/

http://codereview.chromium.org/10822013/diff/1/media/crypto/aes_decryptor.cc#...
media/crypto/aes_decryptor.cc:88: // Returns false and leave |decrypted_text| as
is if decryption failed.
s/leave/leaves/

http://codereview.chromium.org/10822013/diff/1/media/crypto/aes_decryptor.cc#...
media/crypto/aes_decryptor.cc:94: DCHECK(decrypted_text->empty());
Why does decrypted_text need to be empty()?

http://codereview.chromium.org/10822013/diff/1/media/crypto/aes_decryptor.cc#...
media/crypto/aes_decryptor.cc:119: DCHECK(decrypted_text->empty());
I don't think you need this check here if you remove the check above.

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

http://codereview.chromium.org/10822013/diff/1/media/crypto/aes_decryptor.cc
File media/crypto/aes_decryptor.cc (right):

http://codereview.chromium.org/10822013/diff/1/media/crypto/aes_decryptor.cc#...
media/crypto/aes_decryptor.cc:88: // Returns false and leave |decrypted_text| as
is if decryption failed.
I think this is pretty common practice and thus does not need to be stated in
addition to 87.

http://codereview.chromium.org/10822013/diff/1/media/crypto/aes_decryptor.cc#...
media/crypto/aes_decryptor.cc:93: std::string* decrypted_text) {
nit: Odd that the output is a different type from the input.
Also odd that the names don't match (data vs. text).

http://codereview.chromium.org/10822013/diff/1/media/crypto/aes_decryptor.cc#...
media/crypto/aes_decryptor.cc:105: base::StringPiece
counter_block(reinterpret_cast<const char*>(iv),iv_size);
space after ,

http://codereview.chromium.org/10822013/diff/1/media/crypto/aes_decryptor.cc#...
media/crypto/aes_decryptor.cc:130: // Fires the |decrypt_cb| with kError and
NULL if decryption failed.
Otherwise fires ... NULL.

http://codereview.chromium.org/10822013/diff/1/media/crypto/aes_decryptor.cc#...
media/crypto/aes_decryptor.cc:241: void AesDecryptor::Decrypt(const
scoped_refptr<DecoderBuffer>& encrypted,
"encrypted" isn't really a noun. I realize changing it will make this not fit (
:( ).

http://codereview.chromium.org/10822013/diff/1/media/crypto/aes_decryptor.cc#...
media/crypto/aes_decryptor.cc:252: DVLOG(1) << "Could not find a matching key
for the given key ID.";
This is still temporary, right? So there should still be a TODO (maybe for
xhwang).

http://codereview.chromium.org/10822013/diff/1/media/crypto/aes_decryptor.cc#...
media/crypto/aes_decryptor.cc:273: scoped_ptr<DecryptionKey> decryption_key) {
Why not just a raw pointer with a note in .h that this function takes ownership?

http://codereview.chromium.org/10822013/diff/1/media/crypto/aes_decryptor.cc#...
media/crypto/aes_decryptor.cc:287: if (found != key_map_.end())
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 !=.)

http://codereview.chromium.org/10822013/diff/1/media/crypto/aes_decryptor.h
File media/crypto/aes_decryptor.h (right):

http://codereview.chromium.org/10822013/diff/1/media/crypto/aes_decryptor.h#n...
media/crypto/aes_decryptor.h:89: void SetKey(const std::string& key_id,
scoped_ptr<DecryptionKey> key);
Should note that this object takes ownership of key.

http://codereview.chromium.org/10822013/diff/1/media/crypto/aes_decryptor.h#n...
media/crypto/aes_decryptor.h:90: DecryptionKey* GetKey(const std::string&
key_id) const;
Who owns the key? If it's not a copy, should that be a const ref? Might then
need a "null" DecryptionKey.

[ddorwin, 2012-07-26 22:26:39]

Please add () to function names in the description. The first bullet could be
read as referring to data structures.

[xhwang, 2012-07-26 23:36:09]

On 2012/07/26 22:26:39, ddorwin wrote:
> Please add () to function names in the description. The first bullet could be
> read as referring to data structures.

Done.

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

Comments mostly resolved. PTAL!

I'll start rebasing shortly.

http://codereview.chromium.org/10822013/diff/1/media/crypto/aes_decryptor.cc
File media/crypto/aes_decryptor.cc (right):

http://codereview.chromium.org/10822013/diff/1/media/crypto/aes_decryptor.cc#...
media/crypto/aes_decryptor.cc:87: // Returns true and fill |decrypted_text| if
if decryption succeeded.
On 2012/07/25 19:33:50, fgalligan1 wrote:
> s/fill/fills/
> s/if if/if/

Done.

http://codereview.chromium.org/10822013/diff/1/media/crypto/aes_decryptor.cc#...
media/crypto/aes_decryptor.cc:88: // Returns false and leave |decrypted_text| as
is if decryption failed.
On 2012/07/26 21:24:07, ddorwin wrote:
> I think this is pretty common practice and thus does not need to be stated in
> addition to 87.

Done.

http://codereview.chromium.org/10822013/diff/1/media/crypto/aes_decryptor.cc#...
media/crypto/aes_decryptor.cc:93: std::string* decrypted_text) {
On 2012/07/26 21:24:07, ddorwin wrote:
> nit: Odd that the output is a different type from the input.
> Also odd that the names don't match (data vs. text).

Done.

http://codereview.chromium.org/10822013/diff/1/media/crypto/aes_decryptor.cc#...
media/crypto/aes_decryptor.cc:94: DCHECK(decrypted_text->empty());
On 2012/07/25 19:33:50, fgalligan1 wrote:
> Why does decrypted_text need to be empty()?

Done.

http://codereview.chromium.org/10822013/diff/1/media/crypto/aes_decryptor.cc#...
media/crypto/aes_decryptor.cc:105: base::StringPiece
counter_block(reinterpret_cast<const char*>(iv),iv_size);
On 2012/07/26 21:24:07, ddorwin wrote:
> space after ,

Done.

http://codereview.chromium.org/10822013/diff/1/media/crypto/aes_decryptor.cc#...
media/crypto/aes_decryptor.cc:119: DCHECK(decrypted_text->empty());
On 2012/07/25 19:33:50, fgalligan1 wrote:
> I don't think you need this check here if you remove the check above.

Done.

http://codereview.chromium.org/10822013/diff/1/media/crypto/aes_decryptor.cc#...
media/crypto/aes_decryptor.cc:130: // Fires the |decrypt_cb| with kError and
NULL if decryption failed.
On 2012/07/26 21:24:07, ddorwin wrote:
> Otherwise fires ... NULL.

Done.

http://codereview.chromium.org/10822013/diff/1/media/crypto/aes_decryptor.cc#...
media/crypto/aes_decryptor.cc:241: void AesDecryptor::Decrypt(const
scoped_refptr<DecoderBuffer>& encrypted,
On 2012/07/26 21:24:07, ddorwin wrote:
> "encrypted" isn't really a noun. I realize changing it will make this not fit
(
> :( ).

Chatted with ddorwin@ offline. We'll use:
a) "encrypted_data" for string, StringPiece and scoped_array<uint8> and uint8*,
b) "encrypted_buffer" for scoped_refptr<DecoderBuffer>.

I'll make the change in another CL though.

http://codereview.chromium.org/10822013/diff/1/media/crypto/aes_decryptor.cc#...
media/crypto/aes_decryptor.cc:252: DVLOG(1) << "Could not find a matching key
for the given key ID.";
On 2012/07/26 21:24:07, ddorwin wrote:
> This is still temporary, right? So there should still be a TODO (maybe for
> xhwang).

Done.

http://codereview.chromium.org/10822013/diff/1/media/crypto/aes_decryptor.cc#...
media/crypto/aes_decryptor.cc:273: scoped_ptr<DecryptionKey> decryption_key) {
On 2012/07/26 21:24:07, ddorwin wrote:
> Why not just a raw pointer with a note in .h that this function takes
ownership?

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

http://codereview.chromium.org/10822013/diff/1/media/crypto/aes_decryptor.cc#...
media/crypto/aes_decryptor.cc:287: if (found != key_map_.end())
On 2012/07/26 21:24:07, ddorwin wrote:
> 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 !=.)

Done.

http://codereview.chromium.org/10822013/diff/1/media/crypto/aes_decryptor.h
File media/crypto/aes_decryptor.h (right):

http://codereview.chromium.org/10822013/diff/1/media/crypto/aes_decryptor.h#n...
media/crypto/aes_decryptor.h:89: void SetKey(const std::string& key_id,
scoped_ptr<DecryptionKey> key);
On 2012/07/26 21:24:07, ddorwin wrote:
> Should note that this object takes ownership of key.

Since scopted_ptr<> is move only, by passing in a scoped_ptr<DecryptionKey>, the
AesDecryptor has to take the ownership or the key is released. But I agree a
comment is clearer. Done.

http://codereview.chromium.org/10822013/diff/1/media/crypto/aes_decryptor.h#n...
media/crypto/aes_decryptor.h:90: DecryptionKey* GetKey(const std::string&
key_id) const;
On 2012/07/26 21:24:07, ddorwin wrote:
> Who owns the key? If it's not a copy, should that be a const ref? Might then
> need a "null" DecryptionKey.

Added a comment that the AesDecryptor still owns the key.

[ddorwin, 2012-07-27 00:00:35]

lgtm

[xhwang, 2012-07-27 15:49:23]

Ah, that's a painful rebase. Now DecryptData() looks good so I'll leave it as
is. Added some minor style changes, PTAL!

[ddorwin, 2012-07-28 05:21:06]

lgtm

[Ryan Sleevi, 2012-07-30 06:52:49]

drive by

http://codereview.chromium.org/10822013/diff/2004/media/crypto/aes_decryptor.cc
File media/crypto/aes_decryptor.cc (right):

http://codereview.chromium.org/10822013/diff/2004/media/crypto/aes_decryptor....
media/crypto/aes_decryptor.cc:20: 
FWIW, please see
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/qSrl_UAqAkQ/SGDZG...
for the general sentiment regarding 'using' declarations

[xhwang, 2012-07-30 16:22:30]

http://codereview.chromium.org/10822013/diff/2004/media/crypto/aes_decryptor.cc
File media/crypto/aes_decryptor.cc (right):

http://codereview.chromium.org/10822013/diff/2004/media/crypto/aes_decryptor....
media/crypto/aes_decryptor.cc:20: 
On 2012/07/30 06:52:50, Ryan Sleevi wrote:
> FWIW, please see
>
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/qSrl_UAqAkQ/SGDZG...
> for the general sentiment regarding 'using' declarations

Thanks for the info. Personally I think either way is okay. But since a lot of
people prefer to not using "using", I'll just remove them. Done.

[commit-bot: I haz the power, 2012-07-30 16:22:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/xhwang@chromium.org/10822013/3005

[commit-bot: I haz the power, 2012-07-30 18:41:31]

Change committed as 148984