Generalize AesDecryptor to make it more spec compliant.

Generalize AesDecryptor to make it more spec compliant.

BUG=123260
TEST=media_unittests, encrypted-media layout tests.


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

[xhwang, 2012-06-11 17:46:16]

Hello scherkus and ddorwin,

This is the patch that generalizes the AesDecryptor to make it more spec
compliant. Please review.

xhwang

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

Design looks good.

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

http://codereview.chromium.org/10534096/diff/1/media/crypto/aes_decryptor.cc#...
media/crypto/aes_decryptor.cc:87: CHECK(init_data && key);
I think this will fail layout tests because of http://crbug.com/123265. See the
removed code in WMPI.cc.

http://codereview.chromium.org/10534096/diff/1/media/crypto/aes_decryptor.cc#...
media/crypto/aes_decryptor.cc:91: // TODO(xhwang): Added |session_id| check
after we figure out how:
s/Added/Add

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

http://codereview.chromium.org/10534096/diff/1/media/crypto/aes_decryptor.h#n...
media/crypto/aes_decryptor.h:41: // must be valid throughout the lifeime of the
AesDecryptor.
lifetime

http://codereview.chromium.org/10534096/diff/1/media/crypto/aes_decryptor.h#n...
media/crypto/aes_decryptor.h:63: void CancelKeyRequest(const std::string&
key_system,
// Cancels the key request specified by |session_id|.

http://codereview.chromium.org/10534096/diff/1/media/crypto/aes_decryptor.h#n...
media/crypto/aes_decryptor.h:77: base::Lock lock_;  // Protects the |key_map_|.
if it is specific to the key_map_, you might name it key_map_lock_.

http://codereview.chromium.org/10534096/diff/1/media/crypto/aes_decryptor.h#n...
media/crypto/aes_decryptor.h:81: uint32_t session_id_;
last_session_id_ or session_id_incrementer_

http://codereview.chromium.org/10534096/diff/1/media/crypto/aes_decryptor_uni...
File media/crypto/aes_decryptor_unittest.cc (right):

http://codereview.chromium.org/10534096/diff/1/media/crypto/aes_decryptor_uni...
media/crypto/aes_decryptor_unittest.cc:22: // Original data.
remove redundant comments here and below.

http://codereview.chromium.org/10534096/diff/1/media/crypto/aes_decryptor_uni...
media/crypto/aes_decryptor_unittest.cc:67: EXPECT_CALL(client_,
KeyError(kClearKeySystem, "", _, 0));
"" - are you not passing a session ID in the tests? You probably should.
_ - it should always be UnknownError, right?

http://codereview.chromium.org/10534096/diff/1/media/crypto/aes_decryptor_uni...
media/crypto/aes_decryptor_unittest.cc:129: const uint8 init_data[] =
"InitData";
kInitData

http://codereview.chromium.org/10534096/diff/1/media/crypto/aes_decryptor_uni...
media/crypto/aes_decryptor_unittest.cc:130: EXPECT_CALL(client_,
KeyMessageMock(kClearKeySystem, _, _, _, _));
I think you can pretty easily check for:
* non-empty string
* non-NULL pointer,
* > 0
* empty string

You can even be able to check for the actual value, but that's probably not
worthwhile.

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

http://codereview.chromium.org/10534096/diff/1/media/crypto/decryptor_client....
media/crypto/decryptor_client.h:19: virtual ~DecryptorClient() {}
You can make this private (and non-virtual) and prevent anyone from deleting the
client with a DecryptorClient pointer, which would probably be a bad idea.

http://codereview.chromium.org/10534096/diff/1/media/crypto/decryptor_client....
media/crypto/decryptor_client.h:37: virtual void KeyNeeded(const std::string&
key_system,
The proposal uses NeedKey.

http://codereview.chromium.org/10534096/diff/1/media/filters/chunk_demuxer_cl...
File media/filters/chunk_demuxer_client.h (right):

http://codereview.chromium.org/10534096/diff/1/media/filters/chunk_demuxer_cl...
media/filters/chunk_demuxer_client.h:18: virtual ~ChunkDemuxerClient() {}
Same (private)

http://codereview.chromium.org/10534096/diff/1/media/filters/chunk_demuxer_cl...
media/filters/chunk_demuxer_client.h:30: virtual void
DemuxerKeyNeeded(scoped_array<uint8> init_data,
Same (name)

http://codereview.chromium.org/10534096/diff/1/media/filters/ffmpeg_video_dec...
File media/filters/ffmpeg_video_decoder_unittest.cc (right):

http://codereview.chromium.org/10534096/diff/1/media/filters/ffmpeg_video_dec...
media/filters/ffmpeg_video_decoder_unittest.cc:41: static const uint8 kRawKey[]
= {
What is "raw key"? Should it by kRightKey like the other test?

http://codereview.chromium.org/10534096/diff/1/media/filters/ffmpeg_video_dec...
media/filters/ffmpeg_video_decoder_unittest.cc:211:
scoped_ptr<MockDecryptorClient> decryptor_client_;
Is there a reason it's not just a member like in AesDecryptor's test?

http://codereview.chromium.org/10534096/diff/1/media/filters/ffmpeg_video_dec...
media/filters/ffmpeg_video_decoder_unittest.cc:391: decryptor_->AddKey("",
kRawKey, arraysize(kRawKey),
should we use real key system and session ID strings?

http://codereview.chromium.org/10534096/diff/1/media/filters/pipeline_integra...
File media/filters/pipeline_integration_test.cc (right):

http://codereview.chromium.org/10534096/diff/1/media/filters/pipeline_integra...
media/filters/pipeline_integration_test.cc:104: class MockEncryptedMedia :
public MockDecryptorClient {
The class name is not very descriptive.

http://codereview.chromium.org/10534096/diff/1/media/filters/pipeline_integra...
media/filters/pipeline_integration_test.cc:121: decryptor_->AddKey(key_system,
init_data.get(), init_data_length,
key_system and session_id will be empty on keyneeded.
We should send valid values (as the caller would) so we can verify them below.

http://codereview.chromium.org/10534096/diff/1/webkit/media/webmediaplayer_im...
File webkit/media/webmediaplayer_impl.cc (right):

http://codereview.chromium.org/10534096/diff/1/webkit/media/webmediaplayer_im...
webkit/media/webmediaplayer_impl.cc:731:
decryptor_->GenerateKeyRequest(key_system.utf8(),
At some point, we will need to post these operations to another thread. Not sure
where.

http://codereview.chromium.org/10534096/diff/1/webkit/media/webmediaplayer_pr...
File webkit/media/webmediaplayer_proxy.h (right):

http://codereview.chromium.org/10534096/diff/1/webkit/media/webmediaplayer_pr...
webkit/media/webmediaplayer_proxy.h:131: // Methods for DecryptorClient ->
WebMediaPlayerImpl communication.
These should be private, right?

[scherkus (not reviewing), 2012-06-12 03:15:57]

http://codereview.chromium.org/10534096/diff/1/media/base/mock_filters.h
File media/base/mock_filters.h (right):

http://codereview.chromium.org/10534096/diff/1/media/base/mock_filters.h#newc...
media/base/mock_filters.h:223: // See
http://code.google.com/p/googletest/issues/detail?id=395
AFAIK this is also due to scoped_array<uint8> -- might want to mention the other
solution + bug (i.e., use std::string)

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

http://codereview.chromium.org/10534096/diff/1/media/crypto/aes_decryptor.cc#...
media/crypto/aes_decryptor.cc:72: // For now, just fire the event with the
init_data as the request.
|init_data|

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

http://codereview.chromium.org/10534096/diff/1/media/crypto/aes_decryptor.h#n...
media/crypto/aes_decryptor.h:29: UnknownError = 1,
these all get k prefix

http://codereview.chromium.org/10534096/diff/1/media/crypto/aes_decryptor.h#n...
media/crypto/aes_decryptor.h:42: void Init(DecryptorClient* client);
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

http://codereview.chromium.org/10534096/diff/1/media/crypto/aes_decryptor.h#n...
media/crypto/aes_decryptor.h:45: // client's KeyMessage or KeyError methods.
suffix fn names with () in docs here + below

http://codereview.chromium.org/10534096/diff/1/media/crypto/aes_decryptor.h#n...
media/crypto/aes_decryptor.h:81: uint32_t session_id_;
I like "next_session_id_" then inside the .cc convert to a string prior to
incrementing

http://codereview.chromium.org/10534096/diff/1/media/crypto/aes_decryptor_uni...
File media/crypto/aes_decryptor_unittest.cc (right):

http://codereview.chromium.org/10534096/diff/1/media/crypto/aes_decryptor_uni...
media/crypto/aes_decryptor_unittest.cc:23: 0x4f, 0x72, 0x69, 0x67, 0x69, 0x6e,
0x61, 0x6c,
2-space indent here + below (we treat it same as code)

http://codereview.chromium.org/10534096/diff/1/media/crypto/aes_decryptor_uni...
media/crypto/aes_decryptor_unittest.cc:57: template <int key_id_size, int
key_size>
template args use CamelCase as they're typically typenames/classes -- here
they're not but still it looks crazy confusing when they're unix_hacker style

http://codereview.chromium.org/10534096/diff/1/media/crypto/aes_decryptor_uni...
media/crypto/aes_decryptor_unittest.cc:64: template <int key_id_size, int
key_size>
ditto

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

http://codereview.chromium.org/10534096/diff/1/media/crypto/decryptor_client....
media/crypto/decryptor_client.h:21: virtual void KeyAdded(const std::string&
key_system,
docs everywhere

http://codereview.chromium.org/10534096/diff/1/media/filters/ffmpeg_video_dec...
File media/filters/ffmpeg_video_decoder_unittest.cc (right):

http://codereview.chromium.org/10534096/diff/1/media/filters/ffmpeg_video_dec...
media/filters/ffmpeg_video_decoder_unittest.cc:43: 0x41, 0x20, 0x77, 0x6f, 0x6e,
0x64, 0x65, 0x72,
2 spaace indent

http://codereview.chromium.org/10534096/diff/1/webkit/media/webmediaplayer_pr...
File webkit/media/webmediaplayer_proxy.h (right):

http://codereview.chromium.org/10534096/diff/1/webkit/media/webmediaplayer_pr...
webkit/media/webmediaplayer_proxy.h:131: // Methods for DecryptorClient ->
WebMediaPlayerImpl communication.
On 2012/06/11 21:02:40, ddorwin wrote:
> These should be private, right?

yep

[xhwang, 2012-06-12 19:01:14]

Mostly fixed on comments. I'll need to find a better name for MockEncryptedMedia
later. Please review again. Thanks!

http://codereview.chromium.org/10534096/diff/1/media/base/mock_filters.h
File media/base/mock_filters.h (right):

http://codereview.chromium.org/10534096/diff/1/media/base/mock_filters.h#newc...
media/base/mock_filters.h:223: // See
http://code.google.com/p/googletest/issues/detail?id=395
On 2012/06/12 03:15:58, scherkus wrote:
> AFAIK this is also due to scoped_array<uint8> -- might want to mention the
other
> solution + bug (i.e., use std::string)

Done.

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

http://codereview.chromium.org/10534096/diff/1/media/crypto/aes_decryptor.cc#...
media/crypto/aes_decryptor.cc:72: // For now, just fire the event with the
init_data as the request.
On 2012/06/12 03:15:58, scherkus wrote:
> |init_data|

Done.

http://codereview.chromium.org/10534096/diff/1/media/crypto/aes_decryptor.cc#...
media/crypto/aes_decryptor.cc:87: CHECK(init_data && key);
On 2012/06/11 21:02:40, ddorwin wrote:
> I think this will fail layout tests because of http://crbug.com/123265. See
the
> removed code in WMPI.cc.

Done.

http://codereview.chromium.org/10534096/diff/1/media/crypto/aes_decryptor.cc#...
media/crypto/aes_decryptor.cc:91: // TODO(xhwang): Added |session_id| check
after we figure out how:
On 2012/06/11 21:02:40, ddorwin wrote:
> s/Added/Add

Done.

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

http://codereview.chromium.org/10534096/diff/1/media/crypto/aes_decryptor.h#n...
media/crypto/aes_decryptor.h:29: UnknownError = 1,
On 2012/06/12 03:15:58, scherkus wrote:
> these all get k prefix

Done.

http://codereview.chromium.org/10534096/diff/1/media/crypto/aes_decryptor.h#n...
media/crypto/aes_decryptor.h:41: // must be valid throughout the lifeime of the
AesDecryptor.
On 2012/06/11 21:02:40, ddorwin wrote:
> lifetime

Done.

http://codereview.chromium.org/10534096/diff/1/media/crypto/aes_decryptor.h#n...
media/crypto/aes_decryptor.h:42: void Init(DecryptorClient* client);
On 2012/06/12 03:15:58, scherkus wrote:
> 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

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

http://codereview.chromium.org/10534096/diff/1/media/crypto/aes_decryptor.h#n...
media/crypto/aes_decryptor.h:45: // client's KeyMessage or KeyError methods.
On 2012/06/12 03:15:58, scherkus wrote:
> suffix fn names with () in docs here + below

Done.

http://codereview.chromium.org/10534096/diff/1/media/crypto/aes_decryptor.h#n...
media/crypto/aes_decryptor.h:63: void CancelKeyRequest(const std::string&
key_system,
On 2012/06/11 21:02:40, ddorwin wrote:
> // Cancels the key request specified by |session_id|.

Done.

http://codereview.chromium.org/10534096/diff/1/media/crypto/aes_decryptor.h#n...
media/crypto/aes_decryptor.h:77: base::Lock lock_;  // Protects the |key_map_|.
On 2012/06/11 21:02:40, ddorwin wrote:
> if it is specific to the key_map_, you might name it key_map_lock_.

Done.

http://codereview.chromium.org/10534096/diff/1/media/crypto/aes_decryptor.h#n...
media/crypto/aes_decryptor.h:81: uint32_t session_id_;
On 2012/06/11 21:02:40, ddorwin wrote:
> last_session_id_ or session_id_incrementer_ 

Done.

http://codereview.chromium.org/10534096/diff/1/media/crypto/aes_decryptor.h#n...
media/crypto/aes_decryptor.h:81: uint32_t session_id_;
On 2012/06/12 03:15:58, scherkus wrote:
> I like "next_session_id_" then inside the .cc convert to a string prior to
> incrementing

Done.

http://codereview.chromium.org/10534096/diff/1/media/crypto/aes_decryptor_uni...
File media/crypto/aes_decryptor_unittest.cc (right):

http://codereview.chromium.org/10534096/diff/1/media/crypto/aes_decryptor_uni...
media/crypto/aes_decryptor_unittest.cc:22: // Original data.
On 2012/06/11 21:02:40, ddorwin wrote:
> remove redundant comments here and below.

Done.

http://codereview.chromium.org/10534096/diff/1/media/crypto/aes_decryptor_uni...
media/crypto/aes_decryptor_unittest.cc:23: 0x4f, 0x72, 0x69, 0x67, 0x69, 0x6e,
0x61, 0x6c,
On 2012/06/12 03:15:58, scherkus wrote:
> 2-space indent here + below (we treat it same as code)

Done.

http://codereview.chromium.org/10534096/diff/1/media/crypto/aes_decryptor_uni...
media/crypto/aes_decryptor_unittest.cc:57: template <int key_id_size, int
key_size>
On 2012/06/12 03:15:58, scherkus wrote:
> template args use CamelCase as they're typically typenames/classes -- here
> they're not but still it looks crazy confusing when they're unix_hacker style

Done.

http://codereview.chromium.org/10534096/diff/1/media/crypto/aes_decryptor_uni...
media/crypto/aes_decryptor_unittest.cc:64: template <int key_id_size, int
key_size>
On 2012/06/12 03:15:58, scherkus wrote:
> ditto

Done.

http://codereview.chromium.org/10534096/diff/1/media/crypto/aes_decryptor_uni...
media/crypto/aes_decryptor_unittest.cc:67: EXPECT_CALL(client_,
KeyError(kClearKeySystem, "", _, 0));
On 2012/06/11 21:02:40, ddorwin wrote:
> "" - are you not passing a session ID in the tests? You probably should.
> _ - it should always be UnknownError, right?

Done.

http://codereview.chromium.org/10534096/diff/1/media/crypto/aes_decryptor_uni...
media/crypto/aes_decryptor_unittest.cc:129: const uint8 init_data[] =
"InitData";
On 2012/06/11 21:02:40, ddorwin wrote:
> kInitData

Done.

http://codereview.chromium.org/10534096/diff/1/media/crypto/aes_decryptor_uni...
media/crypto/aes_decryptor_unittest.cc:130: EXPECT_CALL(client_,
KeyMessageMock(kClearKeySystem, _, _, _, _));
On 2012/06/11 21:02:40, ddorwin wrote:
> I think you can pretty easily check for:
> * non-empty string
> * non-NULL pointer,
> * > 0
> * empty string
> 
> You can even be able to check for the actual value, but that's probably not
> worthwhile.

Done.

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

http://codereview.chromium.org/10534096/diff/1/media/crypto/decryptor_client....
media/crypto/decryptor_client.h:19: virtual ~DecryptorClient() {}
On 2012/06/11 21:02:40, ddorwin wrote:
> You can make this private (and non-virtual) and prevent anyone from deleting
the
> client with a DecryptorClient pointer, which would probably be a bad idea.

Good to know! Done.

http://codereview.chromium.org/10534096/diff/1/media/crypto/decryptor_client....
media/crypto/decryptor_client.h:21: virtual void KeyAdded(const std::string&
key_system,
On 2012/06/12 03:15:58, scherkus wrote:
> docs everywhere

Done.

http://codereview.chromium.org/10534096/diff/1/media/crypto/decryptor_client....
media/crypto/decryptor_client.h:37: virtual void KeyNeeded(const std::string&
key_system,
On 2012/06/11 21:02:40, ddorwin wrote:
> The proposal uses NeedKey.

Done.

http://codereview.chromium.org/10534096/diff/1/media/filters/chunk_demuxer_cl...
File media/filters/chunk_demuxer_client.h (right):

http://codereview.chromium.org/10534096/diff/1/media/filters/chunk_demuxer_cl...
media/filters/chunk_demuxer_client.h:18: virtual ~ChunkDemuxerClient() {}
On 2012/06/11 21:02:40, ddorwin wrote:
> Same (private)

Done.

http://codereview.chromium.org/10534096/diff/1/media/filters/chunk_demuxer_cl...
media/filters/chunk_demuxer_client.h:30: virtual void
DemuxerKeyNeeded(scoped_array<uint8> init_data,
On 2012/06/11 21:02:40, ddorwin wrote:
> Same (name)

Done.

http://codereview.chromium.org/10534096/diff/1/media/filters/ffmpeg_video_dec...
File media/filters/ffmpeg_video_decoder_unittest.cc (right):

http://codereview.chromium.org/10534096/diff/1/media/filters/ffmpeg_video_dec...
media/filters/ffmpeg_video_decoder_unittest.cc:41: static const uint8 kRawKey[]
= {
On 2012/06/11 21:02:40, ddorwin wrote:
> What is "raw key"? Should it by kRightKey like the other test?

Done.

http://codereview.chromium.org/10534096/diff/1/media/filters/ffmpeg_video_dec...
media/filters/ffmpeg_video_decoder_unittest.cc:43: 0x41, 0x20, 0x77, 0x6f, 0x6e,
0x64, 0x65, 0x72,
On 2012/06/12 03:15:58, scherkus wrote:
> 2 spaace indent

Done.

http://codereview.chromium.org/10534096/diff/1/media/filters/ffmpeg_video_dec...
media/filters/ffmpeg_video_decoder_unittest.cc:211:
scoped_ptr<MockDecryptorClient> decryptor_client_;
On 2012/06/11 21:02:40, ddorwin wrote:
> Is there a reason it's not just a member like in AesDecryptor's test?

Done.

http://codereview.chromium.org/10534096/diff/1/media/filters/ffmpeg_video_dec...
media/filters/ffmpeg_video_decoder_unittest.cc:391: decryptor_->AddKey("",
kRawKey, arraysize(kRawKey),
On 2012/06/11 21:02:40, ddorwin wrote:
> should we use real key system and session ID strings?

Done.

http://codereview.chromium.org/10534096/diff/1/media/filters/pipeline_integra...
File media/filters/pipeline_integration_test.cc (right):

http://codereview.chromium.org/10534096/diff/1/media/filters/pipeline_integra...
media/filters/pipeline_integration_test.cc:104: class MockEncryptedMedia :
public MockDecryptorClient {
On 2012/06/11 21:02:40, ddorwin wrote:
> The class name is not very descriptive.

Agreed, but I haven't found a good name for this. I'll keep thinking while I am
OOO this afternoon.

http://codereview.chromium.org/10534096/diff/1/media/filters/pipeline_integra...
media/filters/pipeline_integration_test.cc:121: decryptor_->AddKey(key_system,
init_data.get(), init_data_length,
On 2012/06/11 21:02:40, ddorwin wrote:
> key_system and session_id will be empty on keyneeded.
> We should send valid values (as the caller would) so we can verify them below.

Done.

http://codereview.chromium.org/10534096/diff/1/webkit/media/webmediaplayer_im...
File webkit/media/webmediaplayer_impl.cc (right):

http://codereview.chromium.org/10534096/diff/1/webkit/media/webmediaplayer_im...
webkit/media/webmediaplayer_impl.cc:731:
decryptor_->GenerateKeyRequest(key_system.utf8(),
On 2012/06/11 21:02:40, ddorwin wrote:
> At some point, we will need to post these operations to another thread. Not
sure
> where.

I suppose these key operations called on the renderer thread are all
light-weighted. This is true for AesDecryptor now. For the future CDM, I'd
expect these calls to send an IPC to the plugin process and return, which should
be pretty fast. Make extra comments about this in aes_decryptor.h

http://codereview.chromium.org/10534096/diff/1/webkit/media/webmediaplayer_pr...
File webkit/media/webmediaplayer_proxy.h (right):

http://codereview.chromium.org/10534096/diff/1/webkit/media/webmediaplayer_pr...
webkit/media/webmediaplayer_proxy.h:131: // Methods for DecryptorClient ->
WebMediaPlayerImpl communication.
On 2012/06/11 21:02:40, ddorwin wrote:
> These should be private, right?

Done.

http://codereview.chromium.org/10534096/diff/1/webkit/media/webmediaplayer_pr...
webkit/media/webmediaplayer_proxy.h:131: // Methods for DecryptorClient ->
WebMediaPlayerImpl communication.
On 2012/06/12 03:15:58, scherkus wrote:
> On 2012/06/11 21:02:40, ddorwin wrote:
> > These should be private, right?
> 
> yep

Done.

[scherkus (not reviewing), 2012-06-13 23:35:08]

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

http://codereview.chromium.org/10534096/diff/8024/media/crypto/aes_decryptor....
media/crypto/aes_decryptor.cc:130: void AesDecryptor::CancelKeyRequest(const
std::string& /* key_system */,
nit: don't bother w/ /* */

also is this NOTIMPLEMENTED()?

http://codereview.chromium.org/10534096/diff/8024/media/filters/chunk_demuxer...
File media/filters/chunk_demuxer_client.h (right):

http://codereview.chromium.org/10534096/diff/8024/media/filters/chunk_demuxer...
media/filters/chunk_demuxer_client.h:32: ~ChunkDemuxerClient() {}
virtual

http://codereview.chromium.org/10534096/diff/8024/media/filters/pipeline_inte...
File media/filters/pipeline_integration_test.cc (right):

http://codereview.chromium.org/10534096/diff/8024/media/filters/pipeline_inte...
media/filters/pipeline_integration_test.cc:109: class MockEncryptedMedia :
public MockDecryptorClient {
does this need to extend MockDecryptorClient?

if anything this is a completely legitimate implementation of a DecryptorClient,
isn't it?

http://codereview.chromium.org/10534096/diff/8024/media/filters/pipeline_inte...
media/filters/pipeline_integration_test.cc:126: EXPECT_GT(message_length, 0);
do we not care what the message is?

http://codereview.chromium.org/10534096/diff/8024/media/filters/pipeline_inte...
media/filters/pipeline_integration_test.cc:137: if (key_system_string_.empty())
{
do we fire need key w/ empty key system today?

http://codereview.chromium.org/10534096/diff/8024/media/filters/pipeline_inte...
media/filters/pipeline_integration_test.cc:143:
EXPECT_FALSE(key_system_string_.empty());
based on if statement above it looks like key_system_string_ is allowed to be
condition?

[ddorwin, 2012-06-14 21:00:13]

A few minor items.

http://codereview.chromium.org/10534096/diff/2003/media/crypto/aes_decryptor_...
File media/crypto/aes_decryptor_unittest.cc (right):

http://codereview.chromium.org/10534096/diff/2003/media/crypto/aes_decryptor_...
media/crypto/aes_decryptor_unittest.cc:55: EXPECT_CALL(client_,
KeyMessageMock(kClearKeySystem, StrNe(std::string()),
This is the type of work that would normally be done in SetUp(). However, I
don't think we should hide it here, even though it avoids code duplication. In
the future, you may, for example, want a test that calls AddKey() without GKR()
and you'll have to change all the tests.

http://codereview.chromium.org/10534096/diff/2003/media/crypto/decryptor_clie...
File media/crypto/decryptor_client.h (right):

http://codereview.chromium.org/10534096/diff/2003/media/crypto/decryptor_clie...
media/crypto/decryptor_client.h:23: // Signals that a key error happened. The
|system_code| is key system
nit: s/happened/occurred/

http://codereview.chromium.org/10534096/diff/2003/media/crypto/decryptor_clie...
media/crypto/decryptor_client.h:24: // dependent. For clear key system, the
|system_code| is always zero.
nit: add dash: system-dependent

http://codereview.chromium.org/10534096/diff/2003/media/filters/pipeline_inte...
File media/filters/pipeline_integration_test.cc (right):

http://codereview.chromium.org/10534096/diff/2003/media/filters/pipeline_inte...
media/filters/pipeline_integration_test.cc:118: class MockEncryptedMedia :
public MockDecryptorClient {
FakeEncryptedMediaApp or MockDecryptorClientImpl would seem better. There is no
such thing as an EncryptedMedia to mock.

http://codereview.chromium.org/10534096/diff/2003/media/filters/pipeline_inte...
media/filters/pipeline_integration_test.cc:133:
EXPECT_TRUE(!session_id.empty());
EXPECT_FALSE(session_id.empty());

http://codereview.chromium.org/10534096/diff/2003/media/filters/pipeline_inte...
media/filters/pipeline_integration_test.cc:147:
DCHECK(session_id_string_.empty());
Something like:
// First event - initialize a session.

http://codereview.chromium.org/10534096/diff/2003/media/filters/pipeline_inte...
media/filters/pipeline_integration_test.cc:162: std::string key_system_string_;
Both "key_system*" are strings. Maybe "current_key_system_". Same below.

http://codereview.chromium.org/10534096/diff/2003/media/filters/pipeline_inte...
media/filters/pipeline_integration_test.cc:194: EXPECT_CALL(*encrypted_media,
KeyAdded(kClearKeySystem, _));
NotNull() doesn't work?

http://codereview.chromium.org/10534096/diff/2003/webkit/media/webmediaplayer...
File webkit/media/webmediaplayer_proxy.h (right):

http://codereview.chromium.org/10534096/diff/2003/webkit/media/webmediaplayer...
webkit/media/webmediaplayer_proxy.h:157: // Notify |webmediaplayer_| that a key
error has occurred.
nit: remove "has" to be consistent with DecryptorClient.h.

[ddorwin, 2012-06-14 21:04:04]

Replies to scherkus.

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

http://codereview.chromium.org/10534096/diff/8024/media/crypto/aes_decryptor....
media/crypto/aes_decryptor.cc:130: void AesDecryptor::CancelKeyRequest(const
std::string& /* key_system */,
On 2012/06/13 23:35:08, scherkus wrote:
> nit: don't bother w/ /* */
> 
> also is this NOTIMPLEMENTED()?

It just doesn't do anything ATM. I think NOTIMPLEMENTED() will break layout
tests.

http://codereview.chromium.org/10534096/diff/8024/media/filters/pipeline_inte...
File media/filters/pipeline_integration_test.cc (right):

http://codereview.chromium.org/10534096/diff/8024/media/filters/pipeline_inte...
media/filters/pipeline_integration_test.cc:143:
EXPECT_FALSE(key_system_string_.empty());
On 2012/06/13 23:35:08, scherkus wrote:
> based on if statement above it looks like key_system_string_ is allowed to be
> condition?

This assumes KeyMessage gets called (and that AesDecryptor is synchronous). I
requested a related comment in another revision of this file. More comments
might be helpful.

[xhwang, 2012-06-15 01:41:04]

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

http://codereview.chromium.org/10534096/diff/8024/media/crypto/aes_decryptor....
media/crypto/aes_decryptor.cc:130: void AesDecryptor::CancelKeyRequest(const
std::string& /* key_system */,
On 2012/06/14 21:04:04, ddorwin wrote:
> On 2012/06/13 23:35:08, scherkus wrote:
> > nit: don't bother w/ /* */
> > 
> > also is this NOTIMPLEMENTED()?
> 
> It just doesn't do anything ATM. I think NOTIMPLEMENTED() will break layout
> tests.

Done with /* */. Can't be NOTIMPLEMENTED() as ddorwin@ commented.

http://codereview.chromium.org/10534096/diff/8024/media/filters/chunk_demuxer...
File media/filters/chunk_demuxer_client.h (right):

http://codereview.chromium.org/10534096/diff/8024/media/filters/chunk_demuxer...
media/filters/chunk_demuxer_client.h:32: ~ChunkDemuxerClient() {}
On 2012/06/13 23:35:08, scherkus wrote:
> virtual

Done. Also done for DecryptorClient::~DecryptorClient.

http://codereview.chromium.org/10534096/diff/8024/media/filters/pipeline_inte...
File media/filters/pipeline_integration_test.cc (right):

http://codereview.chromium.org/10534096/diff/8024/media/filters/pipeline_inte...
media/filters/pipeline_integration_test.cc:109: class MockEncryptedMedia :
public MockDecryptorClient {
On 2012/06/13 23:35:08, scherkus wrote:
> does this need to extend MockDecryptorClient?
> 
> if anything this is a completely legitimate implementation of a
DecryptorClient,
> isn't it?

Done.

http://codereview.chromium.org/10534096/diff/8024/media/filters/pipeline_inte...
media/filters/pipeline_integration_test.cc:126: EXPECT_GT(message_length, 0);
On 2012/06/13 23:35:08, scherkus wrote:
> do we not care what the message is?

Currently the AesDecryptor just returns the init_data passed in GKR. It's not
clear what the message should really be. So I don't think it's worthwhile
checking this.

http://codereview.chromium.org/10534096/diff/8024/media/filters/pipeline_inte...
media/filters/pipeline_integration_test.cc:137: if (key_system_string_.empty())
{
On 2012/06/13 23:35:08, scherkus wrote:
> do we fire need key w/ empty key system today?

Yes. When NeedKey is fired from the demuxer. Added comments.

http://codereview.chromium.org/10534096/diff/8024/media/filters/pipeline_inte...
media/filters/pipeline_integration_test.cc:143:
EXPECT_FALSE(key_system_string_.empty());
On 2012/06/14 21:04:04, ddorwin wrote:
> On 2012/06/13 23:35:08, scherkus wrote:
> > based on if statement above it looks like key_system_string_ is allowed to
be
> > condition?
> 
> This assumes KeyMessage gets called (and that AesDecryptor is synchronous). I
> requested a related comment in another revision of this file. More comments
> might be helpful.

Done.

http://codereview.chromium.org/10534096/diff/2003/media/crypto/aes_decryptor_...
File media/crypto/aes_decryptor_unittest.cc (right):

http://codereview.chromium.org/10534096/diff/2003/media/crypto/aes_decryptor_...
media/crypto/aes_decryptor_unittest.cc:55: EXPECT_CALL(client_,
KeyMessageMock(kClearKeySystem, StrNe(std::string()),
On 2012/06/14 21:00:13, ddorwin wrote:
> This is the type of work that would normally be done in SetUp(). However, I
> don't think we should hide it here, even though it avoids code duplication. In
> the future, you may, for example, want a test that calls AddKey() without
GKR()
> and you'll have to change all the tests.

Done.

http://codereview.chromium.org/10534096/diff/2003/media/crypto/decryptor_clie...
File media/crypto/decryptor_client.h (right):

http://codereview.chromium.org/10534096/diff/2003/media/crypto/decryptor_clie...
media/crypto/decryptor_client.h:23: // Signals that a key error happened. The
|system_code| is key system
On 2012/06/14 21:00:13, ddorwin wrote:
> nit: s/happened/occurred/

Done.

http://codereview.chromium.org/10534096/diff/2003/media/crypto/decryptor_clie...
media/crypto/decryptor_client.h:24: // dependent. For clear key system, the
|system_code| is always zero.
On 2012/06/14 21:00:13, ddorwin wrote:
> nit: add dash: system-dependent

Done.

http://codereview.chromium.org/10534096/diff/2003/media/filters/pipeline_inte...
File media/filters/pipeline_integration_test.cc (right):

http://codereview.chromium.org/10534096/diff/2003/media/filters/pipeline_inte...
media/filters/pipeline_integration_test.cc:118: class MockEncryptedMedia :
public MockDecryptorClient {
On 2012/06/14 21:00:13, ddorwin wrote:
> FakeEncryptedMediaApp or MockDecryptorClientImpl would seem better. There is
no
> such thing as an EncryptedMedia to mock.

Done.

http://codereview.chromium.org/10534096/diff/2003/media/filters/pipeline_inte...
media/filters/pipeline_integration_test.cc:133:
EXPECT_TRUE(!session_id.empty());
On 2012/06/14 21:00:13, ddorwin wrote:
> EXPECT_FALSE(session_id.empty());

Done.

http://codereview.chromium.org/10534096/diff/2003/media/filters/pipeline_inte...
media/filters/pipeline_integration_test.cc:147:
DCHECK(session_id_string_.empty());
On 2012/06/14 21:00:13, ddorwin wrote:
> Something like:
> // First event - initialize a session.

Done.

http://codereview.chromium.org/10534096/diff/2003/media/filters/pipeline_inte...
media/filters/pipeline_integration_test.cc:162: std::string key_system_string_;
On 2012/06/14 21:00:13, ddorwin wrote:
> Both "key_system*" are strings. Maybe "current_key_system_". Same below.

Done.

http://codereview.chromium.org/10534096/diff/2003/media/filters/pipeline_inte...
media/filters/pipeline_integration_test.cc:194: EXPECT_CALL(*encrypted_media,
KeyAdded(kClearKeySystem, _));
On 2012/06/14 21:00:13, ddorwin wrote:
> NotNull() doesn't work?

Done.

http://codereview.chromium.org/10534096/diff/2003/webkit/media/webmediaplayer...
File webkit/media/webmediaplayer_proxy.h (right):

http://codereview.chromium.org/10534096/diff/2003/webkit/media/webmediaplayer...
webkit/media/webmediaplayer_proxy.h:157: // Notify |webmediaplayer_| that a key
error has occurred.
On 2012/06/14 21:00:13, ddorwin wrote:
> nit: remove "has" to be consistent with DecryptorClient.h.

Done.

[scherkus (not reviewing), 2012-06-15 04:29:21]

one nit, one q

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

http://codereview.chromium.org/10534096/diff/6011/media/crypto/aes_decryptor....
media/crypto/aes_decryptor.h:79: // Since only Decrypt() is called off the
renderer thread, we only need to
nit: blank lines before comments

http://codereview.chromium.org/10534096/diff/6011/media/filters/pipeline_inte...
File media/filters/pipeline_integration_test.cc (right):

http://codereview.chromium.org/10534096/diff/6011/media/filters/pipeline_inte...
media/filters/pipeline_integration_test.cc:118: class MockDecryptorClientImpl :
public DecryptorClient {
Based on our IM chat... could other clients (i.e., player_x11)  make use of this
if they were using AesDecryptor?

[ddorwin, 2012-06-15 04:29:25]

nits

http://codereview.chromium.org/10534096/diff/8024/media/filters/chunk_demuxer...
File media/filters/chunk_demuxer_client.h (right):

http://codereview.chromium.org/10534096/diff/8024/media/filters/chunk_demuxer...
media/filters/chunk_demuxer_client.h:32: ~ChunkDemuxerClient() {}
On 2012/06/15 01:41:04, xhwang wrote:
> On 2012/06/13 23:35:08, scherkus wrote:
> > virtual
> 
> Done. Also done for DecryptorClient::~DecryptorClient.

Why does this need to be virtual? Delete is never called on this pointer type
(the destructor is protected).

http://codereview.chromium.org/10534096/diff/6011/media/crypto/decryptor_clie...
File media/crypto/decryptor_client.h (right):

http://codereview.chromium.org/10534096/diff/6011/media/crypto/decryptor_clie...
media/crypto/decryptor_client.h:45: virtual ~DecryptorClient() {}
I don't think this needs to be virtual. Remember, nobody can delete this type
(because the destructor is protected).

http://codereview.chromium.org/10534096/diff/6011/media/filters/pipeline_inte...
File media/filters/pipeline_integration_test.cc (right):

http://codereview.chromium.org/10534096/diff/6011/media/filters/pipeline_inte...
media/filters/pipeline_integration_test.cc:127: virtual void KeyAdded(const
std::string& key_system,
Why did you override these instead of using GMock matchers? Don't you get
warnings for an unexpected call? Previously, you had EXPECT_CALL().

http://codereview.chromium.org/10534096/diff/6011/media/filters/pipeline_inte...
media/filters/pipeline_integration_test.cc:129: EXPECT_EQ(key_system,
kClearKeySystem);
Expected values should be on LHS

http://codereview.chromium.org/10534096/diff/6011/media/filters/pipeline_inte...
media/filters/pipeline_integration_test.cc:137: NOTIMPLEMENTED();
Isn't unexpected call enough from GMock enough?
This usually means it hasn't yet or won't be implemented.

[scherkus (not reviewing), 2012-06-15 04:36:36]

http://codereview.chromium.org/10534096/diff/8024/media/filters/chunk_demuxer...
File media/filters/chunk_demuxer_client.h (right):

http://codereview.chromium.org/10534096/diff/8024/media/filters/chunk_demuxer...
media/filters/chunk_demuxer_client.h:32: ~ChunkDemuxerClient() {}
On 2012/06/15 04:29:25, ddorwin wrote:
> On 2012/06/15 01:41:04, xhwang wrote:
> > On 2012/06/13 23:35:08, scherkus wrote:
> > > virtual
> > 
> > Done. Also done for DecryptorClient::~DecryptorClient.
> 
> Why does this need to be virtual? Delete is never called on this pointer type
> (the destructor is protected).

Refer to both Google C++ style guide (all methods are virtual, including dtor)
and recent thread on chromium-dev that discussed this exact situation:
https://groups.google.com/a/chromium.org/d/topic/chromium-dev/n5IAT9wywPo/dis...

It was decided to not further diverge from Google C++ style

[xhwang, 2012-06-15 16:39:18]

http://codereview.chromium.org/10534096/diff/8024/media/filters/chunk_demuxer...
File media/filters/chunk_demuxer_client.h (right):

http://codereview.chromium.org/10534096/diff/8024/media/filters/chunk_demuxer...
media/filters/chunk_demuxer_client.h:32: ~ChunkDemuxerClient() {}
On 2012/06/15 04:36:36, scherkus wrote:
> On 2012/06/15 04:29:25, ddorwin wrote:
> > On 2012/06/15 01:41:04, xhwang wrote:
> > > On 2012/06/13 23:35:08, scherkus wrote:
> > > > virtual
> > > 
> > > Done. Also done for DecryptorClient::~DecryptorClient.
> > 
> > Why does this need to be virtual? Delete is never called on this pointer
type
> > (the destructor is protected).
> 
> Refer to both Google C++ style guide (all methods are virtual, including dtor)
> and recent thread on chromium-dev that discussed this exact situation:
>
https://groups.google.com/a/chromium.org/d/topic/chromium-dev/n5IAT9wywPo/dis...
> 
> It was decided to not further diverge from Google C++ style

Yeah, I discussed this with scherkus and forwarded it to ddorwin off-line.

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

http://codereview.chromium.org/10534096/diff/6011/media/crypto/aes_decryptor....
media/crypto/aes_decryptor.h:79: // Since only Decrypt() is called off the
renderer thread, we only need to
On 2012/06/15 04:29:21, scherkus wrote:
> nit: blank lines before comments

Done.

http://codereview.chromium.org/10534096/diff/6011/media/filters/pipeline_inte...
File media/filters/pipeline_integration_test.cc (right):

http://codereview.chromium.org/10534096/diff/6011/media/filters/pipeline_inte...
media/filters/pipeline_integration_test.cc:118: class MockDecryptorClientImpl :
public DecryptorClient {
On 2012/06/15 04:29:21, scherkus wrote:
> Based on our IM chat... could other clients (i.e., player_x11)  make use of
this
> if they were using AesDecryptor?

This MockDecryptorClientImpl just simulates an application which would call
AddKey() on needkey event. In this sense, it should work. But the key added is
specific to the test file bear-320x240-encrypted.webm. The player_x11 won't work
if it needs to decrypt other encrypted files.

http://codereview.chromium.org/10534096/diff/6011/media/filters/pipeline_inte...
media/filters/pipeline_integration_test.cc:127: virtual void KeyAdded(const
std::string& key_system,
On 2012/06/15 04:29:25, ddorwin wrote:
> Why did you override these instead of using GMock matchers? Don't you get
> warnings for an unexpected call? Previously, you had EXPECT_CALL().

In the IM-chat, scherkus@ points out that in pipeline integration test we use
real stuff (real demuxers, real decoders, etc) and we create a minimal
implementation of auxiliary interfaces rather than using Mock-classes. If you
look at pipeline_integration_test.cc you won't see any real Mock classes used
and EXPECT_CALL is never used.

Quote scherkus's comment: "the outcome of a pipeline integration test should be
on basic pipeline stuff (i.e., most tests play a clip until the end)
we've had to implement a few basic interfaces to get stuff working, such as a
file-based data source and a null audio sink".

http://codereview.chromium.org/10534096/diff/6011/media/filters/pipeline_inte...
media/filters/pipeline_integration_test.cc:129: EXPECT_EQ(key_system,
kClearKeySystem);
On 2012/06/15 04:29:25, ddorwin wrote:
> Expected values should be on LHS

Done.

http://codereview.chromium.org/10534096/diff/6011/media/filters/pipeline_inte...
media/filters/pipeline_integration_test.cc:137: NOTIMPLEMENTED();
On 2012/06/15 04:29:25, ddorwin wrote:
> Isn't unexpected call enough from GMock enough?
> This usually means it hasn't yet or won't be implemented.

See comment above.

[scherkus (not reviewing), 2012-06-15 16:42:44]

lgtm

[commit-bot: I haz the power, 2012-06-15 20:51:29]

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

[commit-bot: I haz the power, 2012-06-15 20:51:43]

Failed to apply patch for media/base/stream_parser.h:
While running patch -p1 --forward --force;
patching file media/base/stream_parser.h
Hunk #1 succeeded at 63 (offset 4 lines).
Hunk #2 FAILED at 69.
1 out of 2 hunks FAILED -- saving rejects to file media/base/stream_parser.h.rej

[commit-bot: I haz the power, 2012-06-15 21:16:11]

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

[commit-bot: I haz the power, 2012-06-16 01:27:44]

Change committed as 142553

[ddorwin, 2012-06-16 02:01:32]

LGTM. Thanks.
I have a few comments that can be addressed in a future CL.

http://codereview.chromium.org/10534096/diff/6011/media/filters/pipeline_inte...
File media/filters/pipeline_integration_test.cc (right):

http://codereview.chromium.org/10534096/diff/6011/media/filters/pipeline_inte...
media/filters/pipeline_integration_test.cc:127: virtual void KeyAdded(const
std::string& key_system,
On 2012/06/15 16:39:18, xhwang wrote:
> On 2012/06/15 04:29:25, ddorwin wrote:
> > Why did you override these instead of using GMock matchers? Don't you get
> > warnings for an unexpected call? Previously, you had EXPECT_CALL().
> 
> In the IM-chat, scherkus@ points out that in pipeline integration test we use
> real stuff (real demuxers, real decoders, etc) and we create a minimal
> implementation of auxiliary interfaces rather than using Mock-classes. If you
> look at pipeline_integration_test.cc you won't see any real Mock classes used
> and EXPECT_CALL is never used.
> 
> Quote scherkus's comment: "the outcome of a pipeline integration test should
be
> on basic pipeline stuff (i.e., most tests play a clip until the end)
> we've had to implement a few basic interfaces to get stuff working, such as a
> file-based data source and a null audio sink".

Ah, I missed the parent class change and the class name threw me. In a future
CL, please change the name to not include "Mock" since that implies it is a real
GMock class. FakeDecryptorClient is an appropriate name.

http://codereview.chromium.org/10534096/diff/6011/media/filters/pipeline_inte...
media/filters/pipeline_integration_test.cc:172: // In test file
bear-320x240-encrypted.webm, the decryption key is equal to
If this Fake is specific to this one file. We might as well move AddKey to
KeyMessage() and do nothing if GKR has already been requested. I think that is
more like an app implementation.

[xhwang, 2012-06-19 16:37:48]

http://codereview.chromium.org/10534096/diff/6011/media/filters/pipeline_inte...
File media/filters/pipeline_integration_test.cc (right):

http://codereview.chromium.org/10534096/diff/6011/media/filters/pipeline_inte...
media/filters/pipeline_integration_test.cc:127: virtual void KeyAdded(const
std::string& key_system,
On 2012/06/16 02:01:32, ddorwin wrote:
> On 2012/06/15 16:39:18, xhwang wrote:
> > On 2012/06/15 04:29:25, ddorwin wrote:
> > > Why did you override these instead of using GMock matchers? Don't you get
> > > warnings for an unexpected call? Previously, you had EXPECT_CALL().
> > 
> > In the IM-chat, scherkus@ points out that in pipeline integration test we
use
> > real stuff (real demuxers, real decoders, etc) and we create a minimal
> > implementation of auxiliary interfaces rather than using Mock-classes. If
you
> > look at pipeline_integration_test.cc you won't see any real Mock classes
used
> > and EXPECT_CALL is never used.
> > 
> > Quote scherkus's comment: "the outcome of a pipeline integration test should
> be
> > on basic pipeline stuff (i.e., most tests play a clip until the end)
> > we've had to implement a few basic interfaces to get stuff working, such as
a
> > file-based data source and a null audio sink".
> 
> Ah, I missed the parent class change and the class name threw me. In a future
> CL, please change the name to not include "Mock" since that implies it is a
real
> GMock class. FakeDecryptorClient is an appropriate name.

Done in http://codereview.chromium.org/10539150/, patch 4.

http://codereview.chromium.org/10534096/diff/6011/media/filters/pipeline_inte...
media/filters/pipeline_integration_test.cc:172: // In test file
bear-320x240-encrypted.webm, the decryption key is equal to
On 2012/06/16 02:01:32, ddorwin wrote:
> If this Fake is specific to this one file. We might as well move AddKey to
> KeyMessage() and do nothing if GKR has already been requested. I think that is
> more like an app implementation.

The reason I didn't put AddKey() in KeyMessage() is because in this test all
calls are synchronous. Then we'll have this reentrant calling sequence:
NeedKey() -> GKR() -> KeyMessage() -> AddKey(). I was trying to simulate the
scenario in a real application where these calls are not called in an reentrant
manner.