Make unprefixed EME APIs use platform and Chromium.

Make unprefixed EME APIs use platform APIs.

This CL replaces much of the previous implementation and removes MockCDM.

BUG=224791
TEST=v2 layout tests pass using org.w3.clearkey and Chromium's stubbed implementation.

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=152520

[ddorwin, 2013-06-06 23:22:38]

See also the corresponding Chromium CL:
https://codereview.chromium.org/16583004/

To avoid breaking the existing v2 layout tests, I will need to land the
public/platform/ files from this CL, then the Chromium CL, and finally this CL.

[ddorwin, 2013-06-06 23:29:50]

Added questions I have as comments.

https://codereview.chromium.org/16387015/diff/1/Source/core/platform/graphics...
File Source/core/platform/graphics/CDMSession.h (right):

https://codereview.chromium.org/16387015/diff/1/Source/core/platform/graphics...
Source/core/platform/graphics/CDMSession.h:48: class CDMSessionClient {
Should we prefer [un]nested client classes?

https://codereview.chromium.org/16387015/diff/1/Source/core/platform/graphics...
File Source/core/platform/graphics/CDMSessionChromium.cpp (right):

https://codereview.chromium.org/16387015/diff/1/Source/core/platform/graphics...
Source/core/platform/graphics/CDMSessionChromium.cpp:78: // FIXME: Should we be
using types from core/html/MediaKeyError.h here?
abarth: Can you help me answer these two questions?

https://codereview.chromium.org/16387015/diff/1/Source/modules/encryptedmedia...
File Source/modules/encryptedmedia/MediaKeySession.h (right):

https://codereview.chromium.org/16387015/diff/1/Source/modules/encryptedmedia...
Source/modules/encryptedmedia/MediaKeySession.h:66: PassRefPtr<MediaKeyError>
error() const;
abarth: Which way should we prefer?
I changed this to be like HTMLMediaElement, though I'm not sure which way is
"correct". Interestingly, the generated V8 code is different between the two
classes but did not change when I changed this line.

https://codereview.chromium.org/16387015/diff/1/public/platform/WebCDMSession.h
File public/platform/WebCDMSession.h (right):

https://codereview.chromium.org/16387015/diff/1/public/platform/WebCDMSession...
public/platform/WebCDMSession.h:43: class Client {
Should we prefer [un]nested client classes?

[abarth-chromium, 2013-06-07 00:08:23]

Sorry, I was distracted while reading this CL.  Here are some rough comments.

https://codereview.chromium.org/16387015/diff/1/Source/core/platform/graphics...
File Source/core/platform/graphics/CDM.h (right):

https://codereview.chromium.org/16387015/diff/1/Source/core/platform/graphics...
Source/core/platform/graphics/CDM.h:40: }
We usually just #include String.  It gets included often anyway.

https://codereview.chromium.org/16387015/diff/1/Source/core/platform/graphics...
Source/core/platform/graphics/CDM.h:48: static bool supportsKeySystem(const
WTF::String&);
Then you won't need the WTF:: prefix.

https://codereview.chromium.org/16387015/diff/1/Source/core/platform/graphics...
File Source/core/platform/graphics/CDMChromium.cpp (right):

https://codereview.chromium.org/16387015/diff/1/Source/core/platform/graphics...
Source/core/platform/graphics/CDMChromium.cpp:53: PassOwnPtr<CDM>
CDM::create(const WTF::String& keySystem)
No need for the WTF:: prefix

https://codereview.chromium.org/16387015/diff/1/Source/core/platform/graphics...
Source/core/platform/graphics/CDMChromium.cpp:58: return PassOwnPtr<CDM>();
You can just write:

    return nullptr;

https://codereview.chromium.org/16387015/diff/1/Source/core/platform/graphics...
File Source/core/platform/graphics/CDMChromium.h (right):

https://codereview.chromium.org/16387015/diff/1/Source/core/platform/graphics...
Source/core/platform/graphics/CDMChromium.h:40: class CDMChromium : public CDM {
There's no need for a CDMChromium anymore.  You can just put this stuff into the
main CDM class.

Also, would you be willing to use complete words in your class names?  Rather
than CDM, perhaps ContentDecryptionModule (or whatever CDM stands for).

https://codereview.chromium.org/16387015/diff/1/Source/core/platform/graphics...
File Source/core/platform/graphics/CDMSessionChromium.cpp (right):

https://codereview.chromium.org/16387015/diff/1/Source/core/platform/graphics...
Source/core/platform/graphics/CDMSessionChromium.cpp:78: // FIXME: Should we be
using types from core/html/MediaKeyError.h here?
On 2013/06/06 23:29:50, ddorwin wrote:
> abarth: Can you help me answer these two questions?

Files inside core/platform cannot include headers from core outside of
core/platform.

https://codereview.chromium.org/16387015/diff/1/Source/core/platform/graphics...
File Source/core/platform/graphics/CDMSessionChromium.h (right):

https://codereview.chromium.org/16387015/diff/1/Source/core/platform/graphics...
Source/core/platform/graphics/CDMSessionChromium.h:45: class CDMSessionChromium
: public CDMSession, private WebKit::WebCDMSession::Client {
There's no need for any of these Chromium files.  You can just fold them into
the base class.

https://codereview.chromium.org/16387015/diff/1/Source/modules/encryptedmedia...
File Source/modules/encryptedmedia/MediaKeySession.h (right):

https://codereview.chromium.org/16387015/diff/1/Source/modules/encryptedmedia...
Source/modules/encryptedmedia/MediaKeySession.h:66: PassRefPtr<MediaKeyError>
error() const;
On 2013/06/06 23:29:50, ddorwin wrote:
> abarth: Which way should we prefer?

Do you mean between returning a MediaKeyError* and a PassRefPtr<MediaKeyError> ?
 We prefer MediaKeyError* in this case because we're not passing ownership of
the object.

[ddorwin, 2013-06-10 22:52:34]

PTAL. I renamed the classes/files and collapsed the Chromium classes into the
base class.

https://codereview.chromium.org/16387015/diff/1/Source/core/platform/graphics...
File Source/core/platform/graphics/CDM.h (right):

https://codereview.chromium.org/16387015/diff/1/Source/core/platform/graphics...
Source/core/platform/graphics/CDM.h:40: }
On 2013/06/07 00:08:23, abarth wrote:
> We usually just #include String.  It gets included often anyway.

Done.

https://codereview.chromium.org/16387015/diff/1/Source/core/platform/graphics...
Source/core/platform/graphics/CDM.h:48: static bool supportsKeySystem(const
WTF::String&);
On 2013/06/07 00:08:23, abarth wrote:
> Then you won't need the WTF:: prefix.

Done.

https://codereview.chromium.org/16387015/diff/1/Source/core/platform/graphics...
File Source/core/platform/graphics/CDMChromium.cpp (right):

https://codereview.chromium.org/16387015/diff/1/Source/core/platform/graphics...
Source/core/platform/graphics/CDMChromium.cpp:53: PassOwnPtr<CDM>
CDM::create(const WTF::String& keySystem)
On 2013/06/07 00:08:23, abarth wrote:
> No need for the WTF:: prefix

Done.

https://codereview.chromium.org/16387015/diff/1/Source/core/platform/graphics...
Source/core/platform/graphics/CDMChromium.cpp:58: return PassOwnPtr<CDM>();
On 2013/06/07 00:08:23, abarth wrote:
> You can just write:
> 
>     return nullptr;

Done.

https://codereview.chromium.org/16387015/diff/1/Source/core/platform/graphics...
File Source/core/platform/graphics/CDMChromium.h (right):

https://codereview.chromium.org/16387015/diff/1/Source/core/platform/graphics...
Source/core/platform/graphics/CDMChromium.h:40: class CDMChromium : public CDM {
On 2013/06/07 00:08:23, abarth wrote:
> There's no need for a CDMChromium anymore.  You can just put this stuff into
the
> main CDM class.
> 
> Also, would you be willing to use complete words in your class names?  Rather
> than CDM, perhaps ContentDecryptionModule (or whatever CDM stands for).

Done.

https://codereview.chromium.org/16387015/diff/1/Source/core/platform/graphics...
File Source/core/platform/graphics/CDMSessionChromium.cpp (right):

https://codereview.chromium.org/16387015/diff/1/Source/core/platform/graphics...
Source/core/platform/graphics/CDMSessionChromium.cpp:78: // FIXME: Should we be
using types from core/html/MediaKeyError.h here?
On 2013/06/07 00:08:23, abarth wrote:
> On 2013/06/06 23:29:50, ddorwin wrote:
> > abarth: Can you help me answer these two questions?
> 
> Files inside core/platform cannot include headers from core outside of
> core/platform.

Done.

https://codereview.chromium.org/16387015/diff/1/Source/core/platform/graphics...
File Source/core/platform/graphics/CDMSessionChromium.h (right):

https://codereview.chromium.org/16387015/diff/1/Source/core/platform/graphics...
Source/core/platform/graphics/CDMSessionChromium.h:45: class CDMSessionChromium
: public CDMSession, private WebKit::WebCDMSession::Client {
On 2013/06/07 00:08:23, abarth wrote:
> There's no need for any of these Chromium files.  You can just fold them into
> the base class.

Done.

https://codereview.chromium.org/16387015/diff/1/Source/modules/encryptedmedia...
File Source/modules/encryptedmedia/MediaKeySession.h (right):

https://codereview.chromium.org/16387015/diff/1/Source/modules/encryptedmedia...
Source/modules/encryptedmedia/MediaKeySession.h:66: PassRefPtr<MediaKeyError>
error() const;
On 2013/06/07 00:08:23, abarth wrote:
> On 2013/06/06 23:29:50, ddorwin wrote:
> > abarth: Which way should we prefer?
> 
> Do you mean between returning a MediaKeyError* and a PassRefPtr<MediaKeyError>
?
>  We prefer MediaKeyError* in this case because we're not passing ownership of
> the object.

Done.

Should we change HTMLMediaElement too?

[scherkus (not reviewing), 2013-06-12 00:51:15]

(lgtm, but I'm not a WK reviewer)

[ddorwin, 2013-06-12 19:55:55]

abarth, friendly ping to please take another look.

[abarth-chromium, 2013-06-12 20:45:18]

Thanks.  This is on my list for today.

[abarth-chromium, 2013-06-12 23:03:51]

LGTM.  Some very minor nits below.

https://codereview.chromium.org/16387015/diff/46002/Source/core/platform/grap...
File Source/core/platform/graphics/ContentDecryptionModule.cpp (right):

https://codereview.chromium.org/16387015/diff/46002/Source/core/platform/grap...
Source/core/platform/graphics/ContentDecryptionModule.cpp:48: return false;
You can just write:

return keySystem == "org.w3.clearkey";

https://codereview.chromium.org/16387015/diff/46002/Source/core/platform/grap...
Source/core/platform/graphics/ContentDecryptionModule.cpp:51: // Factory method:
Chromium-implementation
No need for this comment.

https://codereview.chromium.org/16387015/diff/46002/Source/core/platform/grap...
Source/core/platform/graphics/ContentDecryptionModule.cpp:54:
ASSERT(!keySystem.isNull() && !keySystem.isEmpty());
isEmpty is a superset of isNull. You can just write:

ASSERT(!keySystem.isEmpty());

https://codereview.chromium.org/16387015/diff/46002/Source/core/platform/grap...
Source/core/platform/graphics/ContentDecryptionModule.cpp:58: return
adoptPtr(new ContentDecryptionModule(cdm.release()));
No need to call adoptPtr here.  You've already adopted the object. 
cdm.release() should work correctly.  (I'm surprised what you've written here
compiles, actually.)

https://codereview.chromium.org/16387015/diff/46002/Source/core/platform/grap...
File Source/core/platform/graphics/ContentDecryptionModuleSession.cpp (right):

https://codereview.chromium.org/16387015/diff/46002/Source/core/platform/grap...
Source/core/platform/graphics/ContentDecryptionModuleSession.cpp:43:
ContentDecryptionModuleSession::ContentDecryptionModuleSession(WebKit::WebContentDecryptionModule&
ContentDecryptionModule, ContentDecryptionModuleSessionClient* client)
WebKit::WebContentDecryptionModule&   ->   WebKit::WebContentDecryptionModule*

I generally prefer if we pass heap objects by pointer rather than by reference. 
I know that in Chromium using a reference is an idiom for a non-null pointer,
but that's not enforced by the compiler and can be misleading.

https://codereview.chromium.org/16387015/diff/46002/Source/core/platform/grap...
File Source/core/platform/graphics/ContentDecryptionModuleSession.h (right):

https://codereview.chromium.org/16387015/diff/46002/Source/core/platform/grap...
Source/core/platform/graphics/ContentDecryptionModuleSession.h:66: WTF::String
sessionId() const;
No need for the WTF:: prefix

https://codereview.chromium.org/16387015/diff/46002/Source/modules/encryptedm...
File Source/modules/encryptedmedia/MediaKeySession.cpp (right):

https://codereview.chromium.org/16387015/diff/46002/Source/modules/encryptedm...
Source/modules/encryptedmedia/MediaKeySession.cpp:152:
m_asyncEventQueue->enqueueEvent(event);
event -> event.release()

https://codereview.chromium.org/16387015/diff/46002/Source/modules/encryptedm...
Source/modules/encryptedmedia/MediaKeySession.cpp:167: ASSERT_NOT_REACHED();
We usually leave off the default case and have the compiler tell us when we've
forgotten an enum value.

https://codereview.chromium.org/16387015/diff/46002/Source/modules/encryptedm...
Source/modules/encryptedmedia/MediaKeySession.cpp:193:
m_asyncEventQueue->enqueueEvent(event);
event.release()

https://codereview.chromium.org/16387015/diff/46002/Source/modules/encryptedm...
File Source/modules/encryptedmedia/MediaKeySession.h (right):

https://codereview.chromium.org/16387015/diff/46002/Source/modules/encryptedm...
Source/modules/encryptedmedia/MediaKeySession.h:57: static
PassRefPtr<MediaKeySession> create(ScriptExecutionContext*,
ContentDecryptionModule&, MediaKeys*);
same nit about * versus &

https://codereview.chromium.org/16387015/diff/46002/Source/modules/encryptedm...
Source/modules/encryptedmedia/MediaKeySession.h:92: const String m_keySystem;
We typically don't use const for member variables.  It's not harmful, we just
don't usually bother with that level of const-correctness.

https://codereview.chromium.org/16387015/diff/46002/public/platform/Platform.h
File public/platform/Platform.h (right):

https://codereview.chromium.org/16387015/diff/46002/public/platform/Platform....
public/platform/Platform.h:149: 
Please maintain two blank lines between sections.

https://codereview.chromium.org/16387015/diff/46002/public/platform/WebConten...
File public/platform/WebContentDecryptionModule.h (right):

https://codereview.chromium.org/16387015/diff/46002/public/platform/WebConten...
public/platform/WebContentDecryptionModule.h:38: // Abstract interface to the
Chromium content decryption modules.
This comment doesn't really add any value.

[abarth-chromium, 2013-06-12 23:26:38]

https://codereview.chromium.org/16387015/diff/46002/Source/core/platform/grap...
File Source/core/platform/graphics/ContentDecryptionModule.cpp (right):

https://codereview.chromium.org/16387015/diff/46002/Source/core/platform/grap...
Source/core/platform/graphics/ContentDecryptionModule.cpp:58: return
adoptPtr(new ContentDecryptionModule(cdm.release()));
On 2013/06/12 23:03:51, abarth wrote:
> No need to call adoptPtr here.  You've already adopted the object. 
> cdm.release() should work correctly.  (I'm surprised what you've written here
> compiles, actually.)

Oops, sorry.  I misread the code.  You're right.  ;)

[ddorwin, 2013-06-12 23:55:54]

Thanks.

https://codereview.chromium.org/16387015/diff/46002/Source/core/platform/grap...
File Source/core/platform/graphics/ContentDecryptionModule.cpp (right):

https://codereview.chromium.org/16387015/diff/46002/Source/core/platform/grap...
Source/core/platform/graphics/ContentDecryptionModule.cpp:48: return false;
On 2013/06/12 23:03:51, abarth wrote:
> You can just write:
> 
> return keySystem == "org.w3.clearkey";

Done.

https://codereview.chromium.org/16387015/diff/46002/Source/core/platform/grap...
Source/core/platform/graphics/ContentDecryptionModule.cpp:51: // Factory method:
Chromium-implementation
On 2013/06/12 23:03:51, abarth wrote:
> No need for this comment.

Done.

https://codereview.chromium.org/16387015/diff/46002/Source/core/platform/grap...
Source/core/platform/graphics/ContentDecryptionModule.cpp:54:
ASSERT(!keySystem.isNull() && !keySystem.isEmpty());
On 2013/06/12 23:03:51, abarth wrote:
> isEmpty is a superset of isNull. You can just write:
> 
> ASSERT(!keySystem.isEmpty());

Done here and in MediaKeys.cpp.

https://codereview.chromium.org/16387015/diff/46002/Source/core/platform/grap...
File Source/core/platform/graphics/ContentDecryptionModuleSession.cpp (right):

https://codereview.chromium.org/16387015/diff/46002/Source/core/platform/grap...
Source/core/platform/graphics/ContentDecryptionModuleSession.cpp:43:
ContentDecryptionModuleSession::ContentDecryptionModuleSession(WebKit::WebContentDecryptionModule&
ContentDecryptionModule, ContentDecryptionModuleSessionClient* client)
On 2013/06/12 23:03:51, abarth wrote:
> WebKit::WebContentDecryptionModule&   ->   WebKit::WebContentDecryptionModule*
> 
> I generally prefer if we pass heap objects by pointer rather than by
reference. 
> I know that in Chromium using a reference is an idiom for a non-null pointer,
> but that's not enforced by the compiler and can be misleading.

Done. I was trying to avoid calling leakPtr() at the call site (and at the same
time indicate that it wouldn't hang on to that pointer). I agree it is unusual,
but leakPtr() seems like a red flag too.

https://codereview.chromium.org/16387015/diff/46002/Source/core/platform/grap...
File Source/core/platform/graphics/ContentDecryptionModuleSession.h (right):

https://codereview.chromium.org/16387015/diff/46002/Source/core/platform/grap...
Source/core/platform/graphics/ContentDecryptionModuleSession.h:66: WTF::String
sessionId() const;
On 2013/06/12 23:03:51, abarth wrote:
> No need for the WTF:: prefix

Done.

https://codereview.chromium.org/16387015/diff/46002/Source/modules/encryptedm...
File Source/modules/encryptedmedia/MediaKeySession.cpp (right):

https://codereview.chromium.org/16387015/diff/46002/Source/modules/encryptedm...
Source/modules/encryptedmedia/MediaKeySession.cpp:152:
m_asyncEventQueue->enqueueEvent(event);
On 2013/06/12 23:03:51, abarth wrote:
> event -> event.release()

Done.

https://codereview.chromium.org/16387015/diff/46002/Source/modules/encryptedm...
Source/modules/encryptedmedia/MediaKeySession.cpp:167: ASSERT_NOT_REACHED();
On 2013/06/12 23:03:51, abarth wrote:
> We usually leave off the default case and have the compiler tell us when we've
> forgotten an enum value.

Done. Excellent! (I always forget where we actually have this compiler flag
turned on.)

https://codereview.chromium.org/16387015/diff/46002/Source/modules/encryptedm...
Source/modules/encryptedmedia/MediaKeySession.cpp:193:
m_asyncEventQueue->enqueueEvent(event);
On 2013/06/12 23:03:51, abarth wrote:
> event.release()

Done.

https://codereview.chromium.org/16387015/diff/46002/Source/modules/encryptedm...
File Source/modules/encryptedmedia/MediaKeySession.h (right):

https://codereview.chromium.org/16387015/diff/46002/Source/modules/encryptedm...
Source/modules/encryptedmedia/MediaKeySession.h:57: static
PassRefPtr<MediaKeySession> create(ScriptExecutionContext*,
ContentDecryptionModule&, MediaKeys*);
On 2013/06/12 23:03:51, abarth wrote:
> same nit about * versus &

Done. (Same.)

https://codereview.chromium.org/16387015/diff/46002/Source/modules/encryptedm...
Source/modules/encryptedmedia/MediaKeySession.h:92: const String m_keySystem;
On 2013/06/12 23:03:51, abarth wrote:
> We typically don't use const for member variables.  It's not harmful, we just
> don't usually bother with that level of const-correctness.

Done. (It was more an indication that the value is always valid.)

https://codereview.chromium.org/16387015/diff/46002/public/platform/Platform.h
File public/platform/Platform.h (right):

https://codereview.chromium.org/16387015/diff/46002/public/platform/Platform....
public/platform/Platform.h:149: 
On 2013/06/12 23:03:51, abarth wrote:
> Please maintain two blank lines between sections.

Done.

https://codereview.chromium.org/16387015/diff/46002/public/platform/WebConten...
File public/platform/WebContentDecryptionModule.h (right):

https://codereview.chromium.org/16387015/diff/46002/public/platform/WebConten...
public/platform/WebContentDecryptionModule.h:38: // Abstract interface to the
Chromium content decryption modules.
On 2013/06/12 23:03:51, abarth wrote:
> This comment doesn't really add any value.

Done.

[abarth-chromium, 2013-06-12 23:57:38]

Why do you need to call leakPtr() ?  Can't you just call get()?

[ddorwin, 2013-06-13 00:17:53]

Yes. Done.

[ddorwin, 2013-06-13 00:20:07]

The public/platform files from <= PS6 are now in
https://codereview.chromium.org/16841007/.

[commit-bot: I haz the power, 2013-06-17 01:29:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ddorwin@chromium.org/16387015/76002

[commit-bot: I haz the power, 2013-06-17 03:40:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ddorwin@chromium.org/16387015/91001

[commit-bot: I haz the power, 2013-06-17 05:13:19]

Change committed as 152520