Verify MediaKeys parameter values consistently

Source/modules/encryptedmedia/MediaKeys.cpp

 /*
  * Copyright (C) 2013 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
  * are met:
  * 1. Redistributions of source code must retain the above copyright
  *    notice, this list of conditions and the following disclaimer.
  * 2. Redistributions in binary form must reproduce the above copyright
  *    notice, this list of conditions and the following disclaimer in the
@@ skipping 35 matching lines @@
     // From <http://dvcs.w3.org/hg/html-media/raw-file/default/encrypted-media/encrypted-media.html#dom-media-keys-constructor>:
     // The MediaKeys(keySystem) constructor must run the following steps:
 
     // 1. If keySystem is null or an empty string, throw an InvalidAccessError exception and abort these steps.
     if (keySystem.isEmpty()) {
         exceptionState.throwDOMException(InvalidAccessError, "The key system provided is invalid.");
         return nullptr;
     }
 
     // 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;
     }
 
     // 3. Let cdm be the content decryption module corresponding to keySystem.
     // 4. Load cdm if necessary.
     OwnPtr<ContentDecryptionModule> cdm = ContentDecryptionModule::create(keySystem);
     if (!cdm) {
         exceptionState.throwDOMException(NotSupportedError, "A content decryption module could not be loaded for the '" + keySystem + "' key system.");
         return nullptr;
@@ skipping 34 matching lines @@
         return nullptr;
     }
 
     if (!initData || !initData->length()) {
         exceptionState.throwDOMException(InvalidAccessError, "The initData provided is null or empty.");
         return nullptr;
     }
 
     // 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;
     }
 
     // 2. Create a new MediaKeySession object.
     RefPtrWillBeRawPtr<MediaKeySession> session = MediaKeySession::create(context, m_cdm.get(), m_weakFactory.createWeakPtr());
     // 2.1 Let the keySystem attribute be keySystem.
     ASSERT(!session->keySystem().isEmpty());
     // FIXME: 2.2 Let the state of the session be CREATED.
 
@@ skipping 12 matching lines @@
 bool MediaKeys::isTypeSupported(const String& keySystem, const String& contentType)
 {
     WTF_LOG(Media, "MediaKeys::isTypeSupported(%s, %s)", keySystem.ascii().data(), contentType.ascii().data());
 
     // 1. If keySystem is null or an empty string, return false and abort these steps.
     if (keySystem.isEmpty())
         return false;
 
     // 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.
     if (contentType.isEmpty())
         return true;
 
     // 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);
 }
 
 void MediaKeys::setMediaElement(HTMLMediaElement* element)
 {
     // FIXME: Cause HTMLMediaElement::setMediaKeys() to throw an exception if m_mediaElement is not 0
     // and remove the code that prevents the assert below in HTMLMediaElement.
     ASSERT(!m_mediaElement != !element);
     m_mediaElement = element;
 }
@@ skipping 26 matching lines @@
 
 void MediaKeys::contextDestroyed()
 {
     ContextLifecycleObserver::contextDestroyed();
 
     // We don't need the CDM anymore.
     m_cdm.clear();
 }
 
 }