Add MediaKeys::GetCdmId().

Add MediaKeys::GetCdmId().

BUG=422730
Committed: https://crrev.com/fa26b63208db8afbdc41d68dc2aa1243690d8325
Cr-Commit-Position: refs/heads/master@{#300401}

[xhwang, 2014-10-17 05:15:35]

xhwang@chromium.org changed reviewers:
+ ddorwin@chromium.org, jrummell@chromium.org

[xhwang, 2014-10-17 05:15:36]

xhwang@chromium.org changed required reviewers:
+ ddorwin@chromium.org

[xhwang, 2014-10-17 05:15:36]

PTAL

[ddorwin, 2014-10-17 16:38:56]

Yay, so much cleaner!

Don't we need an implementation in ProxyMediaKeys?

https://codereview.chromium.org/665563002/diff/20001/media/base/media_keys.h
File media/base/media_keys.h (right):

https://codereview.chromium.org/665563002/diff/20001/media/base/media_keys.h#...
media/base/media_keys.h:132: int GetCdmId() const;
Should this be pure virtual to force implementations to implement it rather than
having a default implementation?

[jrummell, 2014-10-17 17:53:26]

lgtm w/nits.

https://codereview.chromium.org/665563002/diff/20001/content/renderer/media/c...
File content/renderer/media/cdm_session_adapter.cc (right):

https://codereview.chromium.org/665563002/diff/20001/content/renderer/media/c...
content/renderer/media/cdm_session_adapter.cc:135: DCHECK(media_keys_);
I thought the convention was not to check if the code would blow up anyway.

https://codereview.chromium.org/665563002/diff/20001/content/renderer/media/c...
File content/renderer/media/crypto/render_cdm_factory.cc (right):

https://codereview.chromium.org/665563002/diff/20001/content/renderer/media/c...
content/renderer/media/crypto/render_cdm_factory.cc:68: return
scoped_ptr<ProxyMediaKeys>(
Should this be media::MediaKeys like the other two? It looks like it doesn't
make any difference to the compilers, but it's not consistent.

[xhwang, 2014-10-17 20:32:06]

ddorwin: You are right. ProxyMediaKeys has GetCdmId but I should make it
"override". PTAL again.

https://codereview.chromium.org/665563002/diff/20001/content/renderer/media/c...
File content/renderer/media/cdm_session_adapter.cc (right):

https://codereview.chromium.org/665563002/diff/20001/content/renderer/media/c...
content/renderer/media/cdm_session_adapter.cc:135: DCHECK(media_keys_);
On 2014/10/17 17:53:25, jrummell wrote:
> I thought the convention was not to check if the code would blow up anyway.

Done.

https://codereview.chromium.org/665563002/diff/20001/content/renderer/media/c...
File content/renderer/media/crypto/render_cdm_factory.cc (right):

https://codereview.chromium.org/665563002/diff/20001/content/renderer/media/c...
content/renderer/media/crypto/render_cdm_factory.cc:68: return
scoped_ptr<ProxyMediaKeys>(
On 2014/10/17 17:53:26, jrummell wrote:
> Should this be media::MediaKeys like the other two? It looks like it doesn't
> make any difference to the compilers, but it's not consistent.

Good catch. Done.

https://codereview.chromium.org/665563002/diff/20001/media/base/media_keys.h
File media/base/media_keys.h (right):

https://codereview.chromium.org/665563002/diff/20001/media/base/media_keys.h#...
media/base/media_keys.h:132: int GetCdmId() const;
On 2014/10/17 16:38:56, ddorwin wrote:
> Should this be pure virtual to force implementations to implement it rather
than
> having a default implementation?

Well, most MediaKeys doesn't need this, hence the default implementation. It's
the same as GetDecryptor() above.

[ddorwin, 2014-10-18 17:05:43]

https://codereview.chromium.org/665563002/diff/20001/media/base/media_keys.h
File media/base/media_keys.h (right):

https://codereview.chromium.org/665563002/diff/20001/media/base/media_keys.h#...
media/base/media_keys.h:132: int GetCdmId() const;
On 2014/10/17 20:32:06, xhwang wrote:
> On 2014/10/17 16:38:56, ddorwin wrote:
> > Should this be pure virtual to force implementations to implement it rather
> than
> > having a default implementation?
> 
> Well, most MediaKeys doesn't need this, hence the default implementation. It's
> the same as GetDecryptor() above.

But all implementations that have this define do (or the define is too broad),
right?

https://codereview.chromium.org/665563002/diff/40001/media/base/media_keys.h
File media/base/media_keys.h (right):

https://codereview.chromium.org/665563002/diff/40001/media/base/media_keys.h#...
media/base/media_keys.h:132: int GetCdmId() const;
virtual

Also, as I replied in PS2, all implementations that define ENABLE_BROWSER_CDMS
use CDM ID (at least for now), so I don't see why we need a default
implementation.

[xhwang, 2014-10-20 17:59:53]

Thanks for the comments. PTAL again.

https://codereview.chromium.org/665563002/diff/20001/media/base/media_keys.h
File media/base/media_keys.h (right):

https://codereview.chromium.org/665563002/diff/20001/media/base/media_keys.h#...
media/base/media_keys.h:132: int GetCdmId() const;
On 2014/10/18 17:05:43, ddorwin wrote:
> On 2014/10/17 20:32:06, xhwang wrote:
> > On 2014/10/17 16:38:56, ddorwin wrote:
> > > Should this be pure virtual to force implementations to implement it
rather
> > than
> > > having a default implementation?
> > 
> > Well, most MediaKeys doesn't need this, hence the default implementation.
It's
> > the same as GetDecryptor() above.
> 
> But all implementations that have this define do (or the define is too broad),
> right?

AesDecryptor is an exception. But I agree it's clearer not to have the default
impl.

Done.

https://codereview.chromium.org/665563002/diff/40001/media/base/media_keys.h
File media/base/media_keys.h (right):

https://codereview.chromium.org/665563002/diff/40001/media/base/media_keys.h#...
media/base/media_keys.h:132: int GetCdmId() const;
On 2014/10/18 17:05:43, ddorwin wrote:
> virtual
> 
> Also, as I replied in PS2, all implementations that define ENABLE_BROWSER_CDMS
> use CDM ID (at least for now), so I don't see why we need a default
> implementation.

Done.

[ddorwin, 2014-10-20 18:16:07]

Thanks

[ddorwin, 2014-10-20 18:16:13]

lgtm

[xhwang, 2014-10-20 23:24:18]

The CQ bit was checked by xhwang@chromium.org

[commit-bot: I haz the power, 2014-10-20 23:26:37]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/665563002/60001

[commit-bot: I haz the power, 2014-10-21 02:00:07]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2014-10-21 02:03:09]

Patchset 4 (id:??) landed as
https://crrev.com/fa26b63208db8afbdc41d68dc2aa1243690d8325
Cr-Commit-Position: refs/heads/master@{#300401}