Implement robustness strings in requestMediaKeySystemAccess().

media/blink/webencryptedmediaclient_impl.cc

Index: media/blink/webencryptedmediaclient_impl.cc
diff --git a/media/blink/webencryptedmediaclient_impl.cc b/media/blink/webencryptedmediaclient_impl.cc
index e770c4ec78786e5d5c7fade96f484b240b35d5a9..3acebacec2024390ed25fbfecad81f295de7b8f9 100644
--- a/media/blink/webencryptedmediaclient_impl.cc
+++ b/media/blink/webencryptedmediaclient_impl.cc
@@ -31,6 +31,126 @@ enum ConfigurationSupport {
   CONFIGURATION_SUPPORTED,
 };
 
+// Accumulates configuration rules to determine if a feature (additional
+// configuration rule) can be added to an accumulated configuration.
+//
+// Note: We consider the encrypted media permission to be equivalent to access
+// to a distinctive identifier, even though they are conceptually distinct. We
+// may want to separate the concepts in the future to make it possible to
+// recommend or require permission without providing access to a distinctive
+// identifier.
+class ConfigState {
+ public:
+  ConfigState(bool was_permission_requested, bool is_permission_granted)
+      : was_permission_requested_(was_permission_requested),
+        is_permission_granted_(is_permission_granted),
+        is_permission_required_(false),

[ddorwin, 2015/03/16 23:25:09]

Use a tri-state enum instead of two bools?

[sandersd (OOO until July 31), 2015/03/17 22:10:40]

They are actually handled separately, although it is true that if permission is
required then it is also in some sense recommended.

+        is_permission_recommended_(false) {
+  }
+
+  // The state is invalid if permission is required but has been denied.
+  // It should not be possible to enter an invalid state if only supported rules
+  // are added.
+  bool IsValid() {

[ddorwin, 2015/03/16 23:25:09]

const here and below - all except AddRule().

[sandersd (OOO until July 31), 2015/03/17 22:10:39]

Done.

+    return !is_permission_required_ || is_permission_granted_ ||
+        !was_permission_requested_;
+  }
+
+  // Permission is possible it has not been denied.
+  bool IsPermissionPossible() {
+    return is_permission_granted_ || !was_permission_requested_;
+  }
+
+  // Permission is recommended (read 'should be used') if it is explicitly
+  // required by a requirement, or if it has been recommended and has not
+  // denied.
+  bool IsPermissionRecommended() {

[ddorwin, 2015/03/16 23:25:08]

These two methods are odd and difficult to reason about out of context.

Is this really ShouldPreferRequestPermission? The presence of
is_permission_required_ means that it's not just recommended.

IsPermissionRequiredOrRecommended() would be more accurate.

[sandersd (OOO until July 31), 2015/03/17 22:10:39]

I went a different direction here but hopefully it is better for you too.

+    return is_permission_required_ || (is_permission_recommended_ &&
+                                       IsPermissionPossible());
+  }
+
+  // A permission request is required if permission is required but has not
+  // been requested yet.
+  bool IsPermissionRequestRequired() {

[ddorwin, 2015/03/16 23:25:08]

This name is also confusing. It's not just that the request is required but that
it has not been granted and has not already been requested.

ShouldRequestPermission()?

[sandersd (OOO until July 31), 2015/03/17 22:10:39]

Done.

+    return is_permission_required_ && !is_permission_granted_ &&

[ddorwin, 2015/03/16 23:25:10]

Continued from the IsPermissionRecommended() comment:
In particular, one wonders how we get from recommended to prompting, which is
controlled by this function. I believe it is because we change the state after
running the function above. (See below.)

[sandersd (OOO until July 31), 2015/03/17 22:10:40]

Acknowledged.

+        !was_permission_requested_;
+  }
+
+  // Checks whether a rule is compatible with all previously added rules.
+  bool IsRuleSupported(EmeConfigRule rule) {
+    switch (rule) {
+      case EmeConfigRule::NOT_SUPPORTED:
+        return false;
+      case EmeConfigRule::PERMISSION_REQUIRED:
+        return IsPermissionPossible();
+      case EmeConfigRule::PERMISSION_RECOMMENDED:
+        return true;
+      case EmeConfigRule::SUPPORTED:
+        return true;
+    }
+    NOTREACHED();
+    return false;
+  }
+
+  // Checks whether a rule is compatible with all previously added rules,

[ddorwin, 2015/03/16 23:25:09]

This doesn't seem consistent with the function name.
Also, I wouldn't assume that such a function would alter any state.

[sandersd (OOO until July 31), 2015/03/17 22:10:39]

Done.

+  // without altering permission state. (This is a hack to allow adding more
+  // rules after the permission request, the correct solution is to always

[ddorwin, 2015/03/16 23:25:09]

Why don't we do that? What's the reason it is this way?

[sandersd (OOO until July 31), 2015/03/17 22:10:40]

This is because we don't want session types to affect config state until the
exact behavior is spec'd.

+  // request permission last.)
+  bool IsRuleSupportedWithoutPrompt(EmeConfigRule rule) {

[ddorwin, 2015/03/16 23:25:09]

s/Prompt/Permission/

WithoutAnotherRequest? The important part is that we're using the existing
granted_ state.

It looks like we can eliminate this once the spec is updated. Right?

[sandersd (OOO until July 31), 2015/03/17 22:10:39]

Done.

+    switch (rule) {
+      case EmeConfigRule::NOT_SUPPORTED:
+        return false;
+      case EmeConfigRule::PERMISSION_REQUIRED:
+        return is_permission_granted_;
+      case EmeConfigRule::PERMISSION_RECOMMENDED:
+        return true;
+      case EmeConfigRule::SUPPORTED:
+        return true;
+    }
+    NOTREACHED();
+    return false;
+  }
+
+  // Add a rule to the accumulated configuration state.
+  void AddRule(EmeConfigRule rule) {
+    switch (rule) {
+      case EmeConfigRule::NOT_SUPPORTED:
+        return;
+      case EmeConfigRule::PERMISSION_REQUIRED:
+        is_permission_required_ = true;
+        return;
+      case EmeConfigRule::PERMISSION_RECOMMENDED:
+        is_permission_recommended_ = true;
+        return;
+      case EmeConfigRule::SUPPORTED:
+        return;
+    }
+    NOTREACHED();
+  }
+
+ private:
+  bool was_permission_requested_;

[ddorwin, 2015/03/16 23:25:09]

It might help to explain what this means? I assume we will only resume use of
this class after the request is requested/denied.

[sandersd (OOO until July 31), 2015/03/17 22:10:40]

I'm not certain what you mean; a temporary class is created for evaluating each
configuration, and it is thrown away immediately.

[ddorwin, 2015/03/19 18:08:35]

I was asking when this would be true. As I noted in PS6, this should be const,
which answers the question somewhat.

However, you might also note that this would be true if this is the second time
through the loop because a permission was requested and rejected. That's the
type of explanation I was looking for, even though it's not strictly related to
this class. Alternatively, this could be a comment on the constructor.

[sandersd (OOO until July 31), 2015/03/19 20:05:09]

Done.

+  bool is_permission_granted_;
+  bool is_permission_required_;
+  bool is_permission_recommended_;
+};
+
+static EmeRobustness ConvertRobustness(const blink::WebString& robustness) {
+  if (robustness.isEmpty())
+    return EmeRobustness::EMPTY;
+  if (robustness == "SW_SECURE_CRYPTO")
+    return EmeRobustness::SW_SECURE_CRYPTO;
+  if (robustness == "SW_SECURE_DECODE")
+    return EmeRobustness::SW_SECURE_DECODE;
+  if (robustness == "HW_SECURE_CRYPTO")
+    return EmeRobustness::HW_SECURE_CRYPTO;
+  if (robustness == "HW_SECURE_DECODE")
+    return EmeRobustness::HW_SECURE_DECODE;
+  if (robustness == "HW_SECURE_ALL")
+    return EmeRobustness::HW_SECURE_ALL;
+  return EmeRobustness::INVALID;
+}
+
 static bool IsSupportedContentType(const std::string& key_system,
                                    const std::string& mime_type,
                                    const std::string& codecs) {
@@ -66,8 +186,10 @@ static bool GetSupportedCapabilities(
     const std::string& key_system,
     const blink::WebVector<blink::WebMediaKeySystemMediaCapability>&
         capabilities,
+    EmeMediaType media_type,
     std::vector<blink::WebMediaKeySystemMediaCapability>*
-        media_type_capabilities) {
+        media_type_capabilities,
+    ConfigState* config_state) {
   // From
   // https://w3c.github.io/encrypted-media/#get-supported-capabilities-for-media-type
   // 1. Let accumulated capabilities be partial configuration.
@@ -91,22 +213,31 @@ static bool GetSupportedCapabilities(
       continue;
     }
     // 3.12. If robustness is not the empty string, run the following steps:
-    //       (Robustness is not supported.)
-    // TODO(sandersd): Implement robustness. http://crbug.com/442586
     if (!capability.robustness.isEmpty()) {
-      LOG(WARNING) << "Configuration rejected because rubustness strings are "
-                   << "not yet supported.";
-      continue;
+      // 3.12.1. If robustness is an unrecognized value or not supported by
+      //         implementation, continue to the next iteration. String
+      //         comparison is case-sensitive.
+      EmeConfigRule robustness_rule = GetRobustnessConfigRule(
+          key_system, media_type, ConvertRobustness(capability.robustness));
+      if (!config_state->IsRuleSupported(robustness_rule))
+        continue;

[ddorwin, 2015/03/16 23:25:09]

It would be nice to expose such results to developers, but that would spam the
console. Perhaps we should DVLOG() to simplify things for devs and/or ourselves
when we really need to take a look.

[sandersd (OOO until July 31), 2015/03/19 20:05:09]

Done.

+      config_state->AddRule(robustness_rule);
+      // 3.12.2. Add robustness to configuration.
+      //         (It's already added, the original object is used directly.)

[ddorwin, 2015/03/16 23:25:09]

by the caller?

[sandersd (OOO until July 31), 2015/03/17 22:10:40]

No, when we push the capability on to media_type_capabilities.

     }
     // 3.13. If the user agent and implementation do not support playback of
     //       encrypted media data as specified by configuration, including all
     //       media types, in combination with accumulated capabilities,
     //       continue to the next iteration.
-    //       (Skipped as there are no configuration-based codec restrictions.)
+    //       (Skipped as there are no configuration-based codec restrictions.
+    //       There will be when the Android security level becomes configurable
+    //       based on robustness.)

[ddorwin, 2015/03/16 23:25:09]

Bug?

[sandersd (OOO until July 31), 2015/03/17 22:10:40]

Done.

     // 3.14. Add configuration to media type capabilities.
     media_type_capabilities->push_back(capability);
     // 3.15. Add configuration to accumulated capabilities.
-    //       (Skipped as there are no configuration-based codec restrictions.)
+    //       (Skipped as there are no configuration-based codec restrictions.
+    //       Note that this is the local accumulated capabilities, the global
+    //       one is updated by the caller.)
   }
   // 4. If media type capabilities is empty, return null.
   // 5. Return media type capabilities.
@@ -131,15 +262,10 @@ static EmeFeatureRequirement ConvertRequirement(
 static ConfigurationSupport GetSupportedConfiguration(
     const std::string& key_system,
     const blink::WebMediaKeySystemConfiguration& candidate,
-    blink::WebMediaKeySystemConfiguration* accumulated_configuration,
     bool was_permission_requested,
-    bool is_permission_granted) {
-  DCHECK(was_permission_requested || !is_permission_granted);
-
-  // It is possible to obtain user permission unless permission was already
-  // requested and denied.
-  bool is_permission_possible =
-      !was_permission_requested || is_permission_granted;
+    bool is_permission_granted,
+    blink::WebMediaKeySystemConfiguration* accumulated_configuration) {
+  ConfigState config_state(was_permission_requested, is_permission_granted);
 
   // From https://w3c.github.io/encrypted-media/#get-supported-configuration
   // 1. Let accumulated configuration be empty. (Done by caller.)
@@ -200,12 +326,11 @@ static ConfigurationSupport GetSupportedConfiguration(
   //     - "not-allowed": If the implementation requires a Distinctive
   //       Identifier in combination with accumulated configuration, return
   //       null.
-  EmeFeatureRequirement di_requirement =
-      ConvertRequirement(candidate.distinctiveIdentifier);
-  if (!IsDistinctiveIdentifierRequirementSupported(key_system, di_requirement,
-                                                   is_permission_possible)) {
+  EmeConfigRule di_rule = GetDistinctiveIdentifierConfigRule(
+      key_system, ConvertRequirement(candidate.distinctiveIdentifier));
+  if (!config_state.IsRuleSupported(di_rule))
     return CONFIGURATION_NOT_SUPPORTED;
-  }
+  config_state.AddRule(di_rule);
 
   // 4. Add the value of the candidate configuration's distinctiveIdentifier
   //    attribute to accumulated configuration.
@@ -219,12 +344,11 @@ static ConfigurationSupport GetSupportedConfiguration(
   //     - "optional": Continue.
   //     - "not-allowed": If the implementation requires persisting state in
   //       combination with accumulated configuration, return null.
-  EmeFeatureRequirement ps_requirement =
-      ConvertRequirement(candidate.persistentState);
-  if (!IsPersistentStateRequirementSupported(key_system, ps_requirement,
-                                             is_permission_possible)) {
+  EmeConfigRule ps_rule = GetPersistentStateConfigRule(
+      key_system, ConvertRequirement(candidate.persistentState));
+  if (!config_state.IsRuleSupported(ps_rule))
     return CONFIGURATION_NOT_SUPPORTED;
-  }
+  config_state.AddRule(ps_rule);
 
   // 6. Add the value of the candidate configuration's persistentState
   //    attribute to accumulated configuration.
@@ -240,7 +364,8 @@ static ConfigurationSupport GetSupportedConfiguration(
     // 7.2. If video capabilities is null, return null.
     std::vector<blink::WebMediaKeySystemMediaCapability> video_capabilities;
     if (!GetSupportedCapabilities(key_system, candidate.videoCapabilities,
-                                  &video_capabilities)) {
+                                  EmeMediaType::VIDEO, &video_capabilities,
+                                  &config_state)) {
       return CONFIGURATION_NOT_SUPPORTED;
     }
 
@@ -258,7 +383,8 @@ static ConfigurationSupport GetSupportedConfiguration(
     // 8.2. If audio capabilities is null, return null.
     std::vector<blink::WebMediaKeySystemMediaCapability> audio_capabilities;
     if (!GetSupportedCapabilities(key_system, candidate.audioCapabilities,
-                                  &audio_capabilities)) {
+                                  EmeMediaType::AUDIO, &audio_capabilities,
+                                  &config_state)) {
       return CONFIGURATION_NOT_SUPPORTED;
     }
 
@@ -274,17 +400,34 @@ static ConfigurationSupport GetSupportedConfiguration(
   //        configuration's distinctiveIdentifier value to "required".
   //      - Otherwise, change accumulated configuration's distinctiveIdentifier
   //        value to "not-allowed".
-  //    (Without robustness support, capabilities do not affect this.)
-  // TODO(sandersd): Implement robustness. http://crbug.com/442586
   if (accumulated_configuration->distinctiveIdentifier ==
       blink::WebMediaKeySystemConfiguration::Requirement::Optional) {
-    if (IsDistinctiveIdentifierRequirementSupported(
-            key_system, EME_FEATURE_NOT_ALLOWED, is_permission_possible)) {
+    EmeConfigRule not_allowed_rule =
+        GetDistinctiveIdentifierConfigRule(key_system, EME_FEATURE_NOT_ALLOWED);
+    EmeConfigRule required_rule =
+        GetDistinctiveIdentifierConfigRule(key_system, EME_FEATURE_REQUIRED);
+    bool not_allowed_supported = config_state.IsRuleSupported(not_allowed_rule);
+    bool required_supported = config_state.IsRuleSupported(required_rule);
+    if (not_allowed_supported && required_supported) {

[ddorwin, 2015/03/16 23:25:09]

Since we check these both below, this can probably be collapsed a little more.
For example:

if not_allowed_supported:
 if required_supported && config_state.IsPermissionRecommended():
   ...
 else:

[sandersd (OOO until July 31), 2015/03/17 22:10:39]

Done.

+      if (config_state.IsPermissionRecommended()) {
+        accumulated_configuration->distinctiveIdentifier =
+            blink::WebMediaKeySystemConfiguration::Requirement::Required;
+        config_state.AddRule(required_rule);

[ddorwin, 2015/03/16 23:25:10]

Should we add "DCHECK(IsPermissionRequestRequired());"? It was unclear initially
how we get from the above to IsPermissionRequestRequired().

[sandersd (OOO until July 31), 2015/03/17 22:10:40]

I've changed the definition of AddRule() to set |is_permission_required_|
instead of separately tracking the recommendation.

[ddorwin, 2015/03/19 18:08:35]

This reply does not seem to match the change (in PS6, at least). Perhaps you
were referring to the removal of IsPermissionRecommended().

PS6 seems fine, though.

[sandersd (OOO until July 31), 2015/03/19 20:05:09]

Acknowledged.

+      } else {
+        accumulated_configuration->distinctiveIdentifier =
+            blink::WebMediaKeySystemConfiguration::Requirement::NotAllowed;
+        config_state.AddRule(not_allowed_rule);
+      }
+    } else if (not_allowed_supported) {
       accumulated_configuration->distinctiveIdentifier =
           blink::WebMediaKeySystemConfiguration::Requirement::NotAllowed;
-    } else {
+      config_state.AddRule(not_allowed_rule);
+    } else if (required_supported) {
       accumulated_configuration->distinctiveIdentifier =
           blink::WebMediaKeySystemConfiguration::Requirement::Required;
+      config_state.AddRule(required_rule);
+    } else {
+      return CONFIGURATION_NOT_SUPPORTED;

[ddorwin, 2015/03/16 23:25:10]

Is this reachable? It seems we should have failed earlier if neither required
nor not allowed are supported.

If we think so, add NOTREACHED().

[sandersd (OOO until July 31), 2015/03/17 22:10:40]

It was before, but I've fixed up GetDistinctiveIdentifierConfigRule() to
guarantee it won't be.

[sandersd (OOO until July 31), 2015/03/17 22:10:40]

Done.

     }
   }
 
@@ -298,44 +441,39 @@ static ConfigurationSupport GetSupportedConfiguration(
   //         to "not-allowed".
   if (accumulated_configuration->persistentState ==
       blink::WebMediaKeySystemConfiguration::Requirement::Optional) {
-    if (IsPersistentStateRequirementSupported(
-            key_system, EME_FEATURE_NOT_ALLOWED, is_permission_possible)) {
+    EmeConfigRule not_allowed_rule =
+        GetPersistentStateConfigRule(key_system, EME_FEATURE_NOT_ALLOWED);
+    EmeConfigRule required_rule =
+        GetPersistentStateConfigRule(key_system, EME_FEATURE_REQUIRED);
+    bool not_allowed_supported = config_state.IsRuleSupported(not_allowed_rule);
+    bool required_supported = config_state.IsRuleSupported(required_rule);
+    if (not_allowed_supported) {
       accumulated_configuration->persistentState =
           blink::WebMediaKeySystemConfiguration::Requirement::NotAllowed;
-    } else {
+      config_state.AddRule(not_allowed_rule);
+    } else if (required_supported) {
       accumulated_configuration->persistentState =
           blink::WebMediaKeySystemConfiguration::Requirement::Required;
+      config_state.AddRule(required_rule);
+    } else {
+      return CONFIGURATION_NOT_SUPPORTED;

[ddorwin, 2015/03/16 23:25:10]

ditto

[sandersd (OOO until July 31), 2015/03/17 22:10:40]

Done.

     }
   }
 
   // 11. If implementation in the configuration specified by the combination of
   //     the values in accumulated configuration is not supported or not allowed
   //     in the origin, return null.
-  di_requirement =
-      ConvertRequirement(accumulated_configuration->distinctiveIdentifier);
-  if (!IsDistinctiveIdentifierRequirementSupported(key_system, di_requirement,
-                                                   is_permission_granted)) {
-    if (was_permission_requested) {
-      // The optional permission was requested and denied.
-      // TODO(sandersd): Avoid the need for this logic - crbug.com/460616.
-      DCHECK(candidate.distinctiveIdentifier ==
-             blink::WebMediaKeySystemConfiguration::Requirement::Optional);
-      DCHECK(di_requirement == EME_FEATURE_REQUIRED);
-      DCHECK(!is_permission_granted);
-      accumulated_configuration->distinctiveIdentifier =
-          blink::WebMediaKeySystemConfiguration::Requirement::NotAllowed;
-    } else {
-      return CONFIGURATION_REQUIRES_PERMISSION;
-    }
-  }
+  //     If accumulated configuration's distinctiveIdentifier value is
+  //     "required", [request user consent].
 
-  ps_requirement =
-      ConvertRequirement(accumulated_configuration->persistentState);
-  if (!IsPersistentStateRequirementSupported(key_system, ps_requirement,
-                                             is_permission_granted)) {
-    DCHECK(!was_permission_requested);  // Should have failed at step 5.
+  // If the combination of permissions cannot be satisfied, give up.
+  // TODO(sandersd): Prove that this can't happen.

[ddorwin, 2015/03/16 23:25:09]

Step 1: Add NOTREACHED() :)

[sandersd (OOO until July 31), 2015/03/17 22:10:40]

Done.

+  if (!config_state.IsValid())
+    return CONFIGURATION_NOT_SUPPORTED;
+
+  // If a permission is required but has not been requested, prompt.

[ddorwin, 2015/03/16 23:25:09]

prompt is one possible outcome. Replace with "request permission".

[sandersd (OOO until July 31), 2015/03/17 22:10:40]

Done.

+  if (config_state.IsPermissionRequestRequired())

[ddorwin, 2015/03/16 23:25:10]

Initially I wondered, How do we get from preferred to requesting the permission?
Now, I see we change the config above.

[sandersd (OOO until July 31), 2015/03/17 22:10:40]

Acknowledged.

     return CONFIGURATION_REQUIRES_PERMISSION;
-  }
 
   // 12. Return accumulated configuration.
   //     (As an extra step, we record the available session types so that
@@ -344,13 +482,15 @@ static ConfigurationSupport GetSupportedConfiguration(
   session_types.push_back(blink::WebEncryptedMediaSessionType::Temporary);
   if (accumulated_configuration->persistentState ==
       blink::WebMediaKeySystemConfiguration::Requirement::Required) {
-    if (IsPersistentLicenseSessionSupported(key_system,
-                                            is_permission_granted)) {
+    // TODO(sandersd): This should happen sooner so that it can affect the

[ddorwin, 2015/03/16 23:25:09]

Is this solved by updating the spec?

We would only want to do this based on a request, though, which requires the
spec update.

[sandersd (OOO until July 31), 2015/03/17 22:10:39]

It will be, yes.

+    // permission state.
+    if (config_state.IsRuleSupportedWithoutPrompt(
+            GetPersistentLicenseSessionConfigRule(key_system))) {
       session_types.push_back(
           blink::WebEncryptedMediaSessionType::PersistentLicense);
     }
-    if (IsPersistentReleaseMessageSessionSupported(key_system,
-                                                   is_permission_granted)) {
+    if (config_state.IsRuleSupportedWithoutPrompt(
+            GetPersistentReleaseMessageSessionConfigRule(key_system))) {
       session_types.push_back(
           blink::WebEncryptedMediaSessionType::PersistentReleaseMessage);
     }
@@ -447,6 +587,7 @@ void WebEncryptedMediaClientImpl::requestMediaKeySystemAccess(
   }
 
   // 7.2-7.4. Implemented by SelectSupportedConfiguration().
+  // TODO(sandersd): Query the permission first.

[ddorwin, 2015/03/16 23:25:09]

Why? It might shortcut some of the code execution, but always running the same
code is also nice.

[sandersd (OOO until July 31), 2015/03/17 22:10:40]

Done.

   SelectSupportedConfiguration(request, false, false);
 }
 
@@ -474,8 +615,8 @@ void WebEncryptedMediaClientImpl::SelectSupportedConfiguration(
     //        new MediaKeySystemAccess object.]
     blink::WebMediaKeySystemConfiguration accumulated_configuration;
     ConfigurationSupport supported = GetSupportedConfiguration(
-        key_system, candidate_configuration, &accumulated_configuration,
-        was_permission_requested, is_permission_granted);
+        key_system, candidate_configuration, was_permission_requested,
+        is_permission_granted, &accumulated_configuration);
     switch (supported) {
       case CONFIGURATION_NOT_SUPPORTED:
         continue;