Add warning message for empty robustness.

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 eb3cf8122c8b651414ad6b409b242c2ccb98b701..84770a91e58d52e0f509daf4f346d3eea53d3db0 100644
--- a/third_party/WebKit/Source/modules/encryptedmedia/NavigatorRequestMediaKeySystemAccess.cpp
+++ b/third_party/WebKit/Source/modules/encryptedmedia/NavigatorRequestMediaKeySystemAccess.cpp
@@ -12,6 +12,7 @@
 #include "core/dom/ExceptionCode.h"
 #include "core/frame/OriginsUsingFeatures.h"
 #include "core/frame/UseCounter.h"
+#include "core/inspector/ConsoleMessage.h"
 #include "modules/encryptedmedia/EncryptedMediaUtils.h"
 #include "modules/encryptedmedia/MediaKeySession.h"
 #include "modules/encryptedmedia/MediaKeySystemAccess.h"
@@ -78,6 +79,18 @@ static WebVector<WebEncryptedMediaSessionType> convertSessionTypes(const Vector<
     return result;
 }
 
+static bool isWidevine(const String& keySystem)
+{
+    return keySystem == "com.widevine.alpha";

[ddorwin, 2015/10/01 21:23:47]

This should be in Chromium, though I don't know how easy it is to write to the
console. Also, we wouldn't go to the trouble to abstract it correctly. Since
this is temporary, it's probably fine.

[xhwang, 2015/10/01 22:49:43]

Acknowledged.

+}
+
+// Returns true if the |config| has only one audio or video capability and the robustness in that capability is empty.
+static bool hasOneCapabilityAndRobustnessIsEmpty(const WebMediaKeySystemConfiguration& config)
+{
+    return (config.audioCapabilities.size() == 1 && config.audioCapabilities[0].robustness.isEmpty())

[sandersd (OOO until July 31), 2015/10/01 20:40:24]

I'm not certain that we should restrict to the size() == 1 case. I see three
other options that make more sense:
  - Warn if the first returned capability has no robustness.
  - Warn if any returned capability has no robustness.
  - Warn if there are capabilities, but none of them have robustness.

The last one is probably the right choice, since it's a close match for where
behavior will change.

[ddorwin, 2015/10/01 21:23:47]

Good point. My assumption was that the empty one would be last, but there's no
guarantee of that. However, that is probably the _only_ case where we might want
to not warn about the empty string. (To do this entirely correctly, we'd have to
check the input array, not just the output, to see if we rejected subsequent
entries.)

The real question is whether the empty string should _ever_ be provided for
video and Widevine. The only use case I can think of is supporting some legacy
system that didn't implement robustness values. However, Chromium implemented
these in ~M43, so I would expect most UAs to support them.

Thus, perhaps we should just check for empty string in any entry. If that turns
out to be too noisy (i.e. there is a legit reason of which we're not aware), we
can always fix it. WDYT?

[ddorwin, 2015/10/01 21:23:47]

Empty might be legit for audio. For example, it's unclear what value to use if
the audio is not encrypted. Empty string seems appropriate rather than requiring
use of a minimal decryption capability. Thus, we should probably drop this one.
WDYT?

[sandersd (OOO until July 31), 2015/10/01 21:34:45]

The ideal design for compatibility would be to list the empty capabilities in a
separate configuration entirely, so we can reasonably expect that all accepted
capabilities should have a robustness. That said, other browsers are broken and
I don't know what gymnastics providers will actually implement.

I agree that there are no obvious use cases for video without a robustness, but
your audio use case makes sense to me and is even likely.

You get to make the call on 'any' vs 'all' though, I have no data on which to
make a call. (There may even be an argument for continuing to warn for the
'none' case, even after the change.)

[xhwang, 2015/10/01 22:49:43]

I am going to do "any". 

+        || (config.videoCapabilities.size() == 1 && config.videoCapabilities[0].robustness.isEmpty());
+}
+
 // This class allows capabilities to be checked and a MediaKeySystemAccess
 // object to be created asynchronously.
 class MediaKeySystemAccessInitializer final : public EncryptedMediaRequest {
@@ -144,6 +157,14 @@ MediaKeySystemAccessInitializer::MediaKeySystemAccessInitializer(ScriptState* sc
 
 void MediaKeySystemAccessInitializer::requestSucceeded(WebContentDecryptionModuleAccess* access)
 {
+    // Check the config and generate warnings on empty robustness string.
+    // TODO(xhwang): Remove after we handle empty robustness correctly. See http://crbug.com/482277
+    if (isWidevine(keySystem()) && hasOneCapabilityAndRobustnessIsEmpty(access->getConfiguration())) {
+        m_resolver->scriptState()->executionContext()->addConsoleMessage(ConsoleMessage::create(JSMessageSource, WarningMessageLevel,

[ddorwin, 2015/10/01 21:23:47]

Are we guaranteed that the EC will be valid?

[xhwang, 2015/10/01 22:49:43]

Good question. Actually I don't know. But we have a precedent at l.106.

+            "It is recommended that a robustness level be specified. In a future version, Chromium will be made "

[sandersd (OOO until July 31), 2015/10/01 20:40:24]

While this message is the one recommended by ddorwin@, it seems overly long and
doesn't have a clear message. In fact, the real result is that we may provide a
*higher* robustness level after the change than we do now. I don't know what the
proposed spec change is, so it's hard to recommend a precise wording.

[ddorwin, 2015/10/01 21:23:47]

I think we generally use "Chrome" in console messages. At least for deprecation
messages in Blink.

[ddorwin, 2015/10/01 21:23:47]

There aren't any plans to change the spec beyond what is there today: "The empty
string indicates that any ability to decrypt and decode the content type is
acceptable." Of course, suggestions welcome.

I agree with your points, though, and the wording was just a suggestion. :)
Perhaps the real message is that not specifying the robustness level could
result in unexpected behavior, potentially including failure to play in future
versions. Of course, what we really want is "you should specify a robustness
along with each contentType." Those are both clearer messages, but I'm not sure
how to shrink that message.

[sandersd (OOO until July 31), 2015/10/01 21:34:45]

Perhaps we should just say that the behavior will change, without clarification.

[xhwang, 2015/10/01 22:49:43]

Updated the text... 

+            "spec compliant such that the empty string is interpreted as the minimum robustness being acceptable."));
+    }
+
     m_resolver->resolve(new MediaKeySystemAccess(m_keySystem, adoptPtr(access)));
     m_resolver.clear();
 }