EME: Correctly handle container-only contentType strings

third_party/WebKit/Source/modules/encryptedmedia/NavigatorRequestMediaKeySystemAccess.cpp

Index: third_party/WebKit/Source/modules/encryptedmedia/NavigatorRequestMediaKeySystemAccess.cpp
diff --git a/third_party/WebKit/Source/modules/encryptedmedia/NavigatorRequestMediaKeySystemAccess.cpp b/third_party/WebKit/Source/modules/encryptedmedia/NavigatorRequestMediaKeySystemAccess.cpp
index fbeffbbd610019dfd1d6c4bb317d754e1ea229bb..9755602d3707b92b53f457a288af7708db543008 100644
--- a/third_party/WebKit/Source/modules/encryptedmedia/NavigatorRequestMediaKeySystemAccess.cpp
+++ b/third_party/WebKit/Source/modules/encryptedmedia/NavigatorRequestMediaKeySystemAccess.cpp
@@ -111,6 +111,12 @@ private:
     // See http://crbug.com/482277
     void checkVideoCapabilityRobustness() const;
 
+    // Generate warning and report to UMA if |config| contains a container-only

[xhwang, 2016/04/25 22:14:08]

Can't find |config| in the declaration :)

[ddorwin, 2016/04/25 22:58:52]

nit: I think it's clearer with "to" removed.

[jrummell, 2016/04/26 21:23:56]

Done.

[jrummell, 2016/04/26 21:23:56]

presubmit checks made me remove |config| as the parameter name. Fixed.

+    // contentType string.
+    // TODO(jrummell): Remove once this is no longer allowed.
+    // See http://crbug.com/605661.
+    void checkNoContainerOnlyContentTypeStrings(const WebMediaKeySystemConfiguration&);

[xhwang, 2016/04/25 22:14:08]

The function name is a bit hard to read.. How about checkEmptyCodecs()?

[jrummell, 2016/04/26 21:23:56]

Done.

+
     Member<ScriptPromiseResolver> m_resolver;
     const String m_keySystem;
     WebVector<WebMediaKeySystemConfiguration> m_supportedConfigurations;
@@ -154,6 +160,8 @@ MediaKeySystemAccessInitializer::MediaKeySystemAccessInitializer(ScriptState* sc
 
 void MediaKeySystemAccessInitializer::requestSucceeded(WebContentDecryptionModuleAccess* access)
 {
+    checkNoContainerOnlyContentTypeStrings(access->getConfiguration());
+
     m_resolver->resolve(new MediaKeySystemAccess(m_keySystem, adoptPtr(access)));
     m_resolver.clear();
 }
@@ -202,6 +210,46 @@ void MediaKeySystemAccessInitializer::checkVideoCapabilityRobustness() const
     }
 }
 
+void MediaKeySystemAccessInitializer::checkNoContainerOnlyContentTypeStrings(const WebMediaKeySystemConfiguration& config)
+{
+    // This is only checking for empty codecs in the selected configuration,
+    // as users may pass container only contentType strings for compatibility

[ddorwin, 2016/04/25 22:58:51]

s/users/apps/

[jrummell, 2016/04/26 21:23:56]

Done.

+    // with other systems.

[ddorwin, 2016/04/25 22:58:51]

s/systems/implementations/

[jrummell, 2016/04/26 21:23:56]

Done.

+    bool emptyAudioCodecs = false;

[ddorwin, 2016/04/25 22:58:51]

How about isAudioCodecsEmpty?

[jrummell, 2016/04/26 21:23:56]

Done.

+    if (!config.audioCapabilities.isEmpty()) {

[xhwang, 2016/04/25 22:14:08]

The logic here is good.

Just to be clear, this check covers both the case where audioCapabilities
doesn't exist, and audioCapabilities exists but is empty:

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

[jrummell, 2016/04/26 21:23:56]

Added additional check to make it clearer.

+        for (const auto& audio : config.audioCapabilities) {
+            if (audio.codecs.isEmpty()) {
+                emptyAudioCodecs = true;

[ddorwin, 2016/04/25 22:58:51]

This will currently trigger if there is capabilities with codecs followed by one
without codecs. This seems like it might be a useful mechanism for compatibility
with other implementations^ without having to duplicate the entire
configuration.

Should we not warn in that case? Or maybe we should have Chromium reject
codec-less strings once a one supported item has been found. Then, we could
leave this code as is.


^ This does not work for Edge
(https://developer.microsoft.com/en-us/microsoft-edge/platform/issues/7223299/),
and I'm not sure what other user agents there might be an issue with. Perhaps
this isn't a useful pattern. Still, the proposal above seems reasonable.

[jrummell, 2016/04/26 21:23:56]

Changed to only track if all returned capabilities don't supply codecs=. Thus
configurations that have both accepted won't get a message, since they will
continue to succeed once the code enforces the check.

+                break;
+            }
+        }
+
+        DEFINE_THREAD_SAFE_STATIC_LOCAL(EnumerationHistogram, emptyAudioCodecsHistogram,
+            new EnumerationHistogram("Media.EME.RequestMediaKeySystemAccess.HasEmptyAudioCodecs", 2));

[ddorwin, 2016/04/25 22:58:51]

We might want to rename this to something like
SelectedAudioContentTypeWithoutCodecs based on how the implementation changes.

[jrummell, 2016/04/26 21:23:56]

Done.

+        emptyAudioCodecsHistogram.count(emptyAudioCodecs);
+    }
+
+    bool emptyVideoCodecs = false;
+    if (!config.videoCapabilities.isEmpty()) {
+        for (const auto& video : config.videoCapabilities) {
+            if (video.codecs.isEmpty()) {
+                emptyVideoCodecs = true;
+                break;
+            }
+        }
+
+        DEFINE_THREAD_SAFE_STATIC_LOCAL(EnumerationHistogram, emptyVideoCodecsHistogram,
+            new EnumerationHistogram("Media.EME.RequestMediaKeySystemAccess.HasEmptyVideoCodecs", 2));
+        emptyVideoCodecsHistogram.count(emptyVideoCodecs);
+    }
+
+    if (emptyAudioCodecs || emptyVideoCodecs) {
+        m_resolver->getExecutionContext()->addConsoleMessage(ConsoleMessage::create(JSMessageSource, WarningMessageLevel,
+            "Container only contentType strings that do not imply a specific set of codecs will not be "

[ddorwin, 2016/04/25 22:58:51]

Since this is a deprecation message, we can be more specific:
contentType strings without codecs will...

[jrummell, 2016/04/26 21:23:56]

Done.

+            "supported in the future. Please specify the desired codec as part of the contentType."));

[ddorwin, 2016/04/25 22:58:51]

"codec(s)"

[jrummell, 2016/04/26 21:23:56]

Done.

+    }
+}
+
 } // namespace
 
 ScriptPromise NavigatorRequestMediaKeySystemAccess::requestMediaKeySystemAccess(