Verify MediaKeys parameter values consistently

Source/modules/encryptedmedia/MediaKeys.cpp

Index: Source/modules/encryptedmedia/MediaKeys.cpp
diff --git a/Source/modules/encryptedmedia/MediaKeys.cpp b/Source/modules/encryptedmedia/MediaKeys.cpp
index d0e173a96c1859077feaaf9cfdbc00005c6e1bb6..3f1c4f2776d71daa0534bdfd95ba3599f317948b 100644
--- a/Source/modules/encryptedmedia/MediaKeys.cpp
+++ b/Source/modules/encryptedmedia/MediaKeys.cpp
@@ -53,7 +53,7 @@ PassRefPtrWillBeRawPtr<MediaKeys> MediaKeys::create(ExecutionContext* context, c
     }
 
     // 2. If keySystem is not one of the user agent's supported Key Systems, throw a NotSupportedError and abort these steps.
-    if (!ContentDecryptionModule::supportsKeySystem(keySystem)) {
+    if (!isSupportedKeySystem(keySystem)) {
         exceptionState.throwDOMException(NotSupportedError, "The '" + keySystem + "' key system is not supported.");
         return nullptr;
     }
@@ -108,7 +108,7 @@ PassRefPtrWillBeRawPtr<MediaKeySession> MediaKeys::createSession(ExecutionContex
 
     // 1. If type contains a MIME type that is not supported or is not supported by the keySystem,
     // throw a NOT_SUPPORTED_ERR exception and abort these steps.
-    if (!m_cdm->supportsMIMEType(contentType)) {
+    if (!isSupportedContentType(m_keySystem, contentType)) {
         exceptionState.throwDOMException(NotSupportedError, "The type provided ('" + contentType + "') is unsupported.");
         return nullptr;
     }
@@ -141,7 +141,7 @@ bool MediaKeys::isTypeSupported(const String& keySystem, const String& contentTy
 
     // 2. If keySystem contains an unrecognized or unsupported Key System, return false and abort
     // these steps. Key system string comparison is case-sensitive.
-    if (!MIMETypeRegistry::isSupportedEncryptedMediaMIMEType(keySystem, "", ""))
+    if (!isSupportedKeySystem(keySystem))
         return false;
 
     // 3. If contentType is null or an empty string, return true and abort these steps.
@@ -150,9 +150,22 @@ bool MediaKeys::isTypeSupported(const String& keySystem, const String& contentTy
 
     // 4. If the Key System specified by keySystem does not support decrypting the container and/or
     // codec specified by contentType, return false and abort these steps.
+    return isSupportedContentType(keySystem, contentType);
+}
+
+bool MediaKeys::isSupportedKeySystem(const String& keySystem)
+{
+    ASSERT(!keySystem.isEmpty());
+    return MIMETypeRegistry::isSupportedEncryptedMediaMIMEType(keySystem, "", "");
+}
+
+bool MediaKeys::isSupportedContentType(const String& keySystem, const String& contentType)

[xhwang, 2014/02/27 23:19:51]

nit: can we just have one helper function, e.g.
isKeySystemSupportedWithContentType or just isTypeSupportedImpl(), where the
"contentType" can be empty? These two helper functions looks too similar :)

[ddorwin, 2014/02/27 23:28:27]

Yes, but they work nicely in the algorithms and hide the implementation details
(granted, within the same class). Also, we want each spec algorithm step to be
specific, which makes larger shared helper functions less appealing.

This function would need to be more complex if it could be called with an empty
contentType. That would basically be redundant with step 3 above.

[xhwang, 2014/02/27 23:42:36]

If we don't need to pass in "keySystem" into isSupportedContentType() then I'd
agree. Now from function signature to implementation, it's clear that
isSupportedKeySystem() is just a special case of isSupportedContentType(). This
is consistent with the spec, where we do progressive checks, not two independent
checks. We should make this clear. So either have one helper function and pass
in empty types in the first check, or we should rename isSupportedContentType to
isSupportedKeySystemAndContentType to reflect this relation.

[ddorwin, 2014/02/28 00:14:09]

Maybe. I considered that. Really, it is isKeySystemSupportedWithContentType().

I think the really problem is that we don't have an object-oriented way to
execute step 4. Instead of asking whether the key system supports a type, we ask
Chromium whether it supports key system with the type.

As I mentioned in the .h file, it would be nice to use m_keySystem for the
createSession() case.

Come to think of it, these don't need to be member functions at all. We could
still keep one for the createSession() case if we wanted. You end up with more
functions, though.

Even more fun createSession() shouldn't really have codecs, though I believe it
is still allowed. Depending on the outcome of spec bugs, createSession() and
isTypeSupported() might actually take different types of strings. That could
mean we end up with different underlying implementations.

Looking forward to that (and avoiding sharing code that shouldn't be shared),
maybe createSession() should ask whether the initData type is supported and
strip the codecs from the string.

I believe that is actually more correct, though the spec doesn't currently say
so. Only the container should matter in the createSession() algorithm. (The
difference is whether codecs=foo should fail or be ignored.)

[jrummell, 2014/02/28 00:25:32]

Renamed function.

+{
+    ASSERT(!keySystem.isEmpty());
+    ASSERT(!contentType.isEmpty());
+
     ContentType type(contentType);
     String codecs = type.parameter("codecs");
-
     return MIMETypeRegistry::isSupportedEncryptedMediaMIMEType(keySystem, type.type(), codecs);
 }