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..cab7bb8a1fa05bfe83cf1cf55f8623768a4a7e02 100644
--- a/media/blink/webencryptedmediaclient_impl.cc
+++ b/media/blink/webencryptedmediaclient_impl.cc
@@ -31,6 +31,119 @@ enum ConfigurationSupport {
   CONFIGURATION_SUPPORTED,
 };
 
+// Accumulates configuration rules to determine if a feature (additional
+// configuration rule) can be added to an accumulated configuration.
+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_identifier_required_(false),
+        is_identifier_recommended_(false){
+  }
+
+  bool IsPermissionGranted() const {
+    return is_permission_granted_;
+  }
+
+  // Permission is possible if it has not been denied.
+  bool IsPermissionPossible() const {
+    return is_permission_granted_ || !was_permission_requested_;
+  }
+
+  bool IsIdentifierRequired() const {
+    return is_identifier_required_;
+  }
+
+  bool IsIdentifierRecommended() const {
+    return is_identifier_recommended_;
+  }
+
+  // Checks whether a rule is compatible with all previously added rules.
+  bool IsRuleSupported(EmeConfigRule rule) const {
+    switch (rule) {
+      case EmeConfigRule::NOT_SUPPORTED:
+        return false;
+      case EmeConfigRule::IDENTIFIER_REQUIRED:
+        return IsPermissionPossible();
+      case EmeConfigRule::IDENTIFIER_RECOMMENDED:
+        return true;
+      case EmeConfigRule::SUPPORTED:
+        return true;
+    }
+    NOTREACHED();
+    return false;
+  }
+
+  // Checks whether a rule is compatible with all previously added rules, and
+  // can be accepted without needing to add it to the configuration state. This
+  // allows considering more rules after the configuration state is final (that
+  // is, after distinctiveIdentifier has been resolved).
+  bool IsRuleSupportedWithCurrentState(EmeConfigRule rule) const {

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

"CurrentState" implies that the other method (above) does not check the current
state.  Both check is_permission_granted_. The only difference is that this one
ignores was_permission_requested_.

It seems that perhaps this one is ...WithoutRequestingPermission.

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

I am trying to remove all unnecessary references to 'permission'. The goal here
is to express that if a rule passes this test, then you can add the associated
option to the accumulated config without adding the rule to the config_state and
the two will still be in sync.

+    switch (rule) {
+      case EmeConfigRule::NOT_SUPPORTED:
+        return false;
+      case EmeConfigRule::IDENTIFIER_REQUIRED:
+        return is_permission_granted_;
+      case EmeConfigRule::IDENTIFIER_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::IDENTIFIER_REQUIRED:
+        is_identifier_required_ = true;
+        return;
+      case EmeConfigRule::IDENTIFIER_RECOMMENDED:
+        is_identifier_recommended_ = true;
+        return;
+      case EmeConfigRule::SUPPORTED:
+        return;
+    }
+    NOTREACHED();
+  }
+
+ private:
+  // Was a permission to usa a distinctive identifier requested?

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

nit: Ending in '?' is a bit weird. Perhaps "Whether... was requested." or
"Whether a request... has been made."

Ditto below, especially "Do we..."

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

Done.

+  bool was_permission_requested_;

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

These first two can be const. That might help readability too.

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

Done.

+
+  // Do we know that permission to use a distinctive identifier has been
+  // granted?
+  bool is_permission_granted_;
+
+  // Have we added a configuration option that requires a distinctive
+  // identifier?
+  bool is_identifier_required_;
+
+  // Have we added a configuration option that recommends a distinctive
+  // identifier?
+  bool is_identifier_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 +179,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 +206,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;
+      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/19 18:08:35]

Suggest: It's added when the original object is pushed onto
media_type_capabilities below.

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

Done.

     }
     // 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. http://crbug.com/467779)
     // 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 +255,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 +319,13 @@ 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)) {
+  // We also reject OPTIONAL when distinctive identifiers are ALWAYS_ENABLED and
+  // permission has already been denied. This would happen anyway at step 11.
+  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 +339,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 +359,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 +378,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,20 +395,47 @@ 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)) {
-      accumulated_configuration->distinctiveIdentifier =
-          blink::WebMediaKeySystemConfiguration::Requirement::NotAllowed;
-    } else {
+    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) {
+      bool prefer_required = config_state.IsIdentifierRequired() ||
+                             (config_state.IsIdentifierRecommended() &&
+                              config_state.IsPermissionPossible());
+      if (required_supported && prefer_required) {
+        accumulated_configuration->distinctiveIdentifier =
+            blink::WebMediaKeySystemConfiguration::Requirement::Required;
+        config_state.AddRule(required_rule);
+        DCHECK(config_state.IsIdentifierRequired());
+      } else {
+        accumulated_configuration->distinctiveIdentifier =
+            blink::WebMediaKeySystemConfiguration::Requirement::NotAllowed;
+        config_state.AddRule(not_allowed_rule);
+      }
+    } else if (required_supported) {
       accumulated_configuration->distinctiveIdentifier =
           blink::WebMediaKeySystemConfiguration::Requirement::Required;
+      config_state.AddRule(required_rule);
+    } else {
+      // We should not have passed step 3.
+      NOTREACHED();
+      return CONFIGURATION_NOT_SUPPORTED;
     }
   }
 
+  // If permission is required but we couldn't enable it, reject the
+  // configuration.
+  if (config_state.IsIdentifierRequired() &&
+      accumulated_configuration->distinctiveIdentifier !=
+      blink::WebMediaKeySystemConfiguration::Requirement::Required) {
+    return CONFIGURATION_NOT_SUPPORTED;
+  }
+
   // 10. If accumulated configuration's persistentState value is "optional",
   //     follow the steps for the first matching condition from the following
   //     list:
@@ -298,59 +446,54 @@ 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.IsRuleSupportedWithCurrentState(not_allowed_rule);

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

It's not obvious to me why we use this method here and the other one above. Is
it because we've already decided whether to prompt?

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

That's right. At this point we may accept a rule that requires a distinctive
identifier (note that we block that in KeySystems, so this concern is
theoretical), but it's too late to change the value. Using this method reliably
implements your intended behavior of requiring the user to supply a requirement
for persistentState to work.

The alternative is to provide a way to 'seal' the state, so that the regular
IsRuleSupported() can reject it. I'm not a big fan of this as it complicates the
state machine.

[ddorwin, 2015/03/19 22:11:57]

Should we add a comment pointing this out, either here or somewhere above after
the "point of no return?"

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

Done.

+    bool required_supported =
+        config_state.IsRuleSupportedWithCurrentState(required_rule);
+    if (not_allowed_supported) {
       accumulated_configuration->persistentState =
           blink::WebMediaKeySystemConfiguration::Requirement::NotAllowed;
-    } else {
+    } else if (required_supported) {
       accumulated_configuration->persistentState =
           blink::WebMediaKeySystemConfiguration::Requirement::Required;
+    } else {
+      // We should not have passed step 5.
+      NOTREACHED();
+      return CONFIGURATION_NOT_SUPPORTED;
     }
   }
 
   // 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 {
+  //
+  //     If accumulated configuration's distinctiveIdentifier value is
+  //     "required", [request user consent].
+  if (accumulated_configuration->distinctiveIdentifier ==
+      blink::WebMediaKeySystemConfiguration::Requirement::Required) {
+    if (!config_state.IsPermissionGranted())
       return CONFIGURATION_REQUIRES_PERMISSION;

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

Is the case where we've already requested the permission handled by the caller?

If so, is that the best solution? We have all the information here, right?

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

It turned out to remove a lot of cases (and invalid states) to push that
decision up one level, making the code both more readable and more similar to
the spec. I'm happy with this tradeoff.

[ddorwin, 2015/03/19 22:11:57]

Okay. Perhaps note that this is handled by the caller here.

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

Done.

-    }
-  }
-
-  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.
-    return CONFIGURATION_REQUIRES_PERMISSION;
   }
 
   // 12. Return accumulated configuration.
-  //     (As an extra step, we record the available session types so that
-  //     createSession() can be synchronous.)
+  //
+  // We also record the available session types so that createSession() can be
+  // synchronous.
   std::vector<blink::WebEncryptedMediaSessionType> session_types;
   session_types.push_back(blink::WebEncryptedMediaSessionType::Temporary);
   if (accumulated_configuration->persistentState ==
       blink::WebMediaKeySystemConfiguration::Requirement::Required) {
-    if (IsPersistentLicenseSessionSupported(key_system,
-                                            is_permission_granted)) {
+    if (config_state.IsRuleSupportedWithCurrentState(

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

This one is more clear that we're not going to request permission because we
would have exited at 479.

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

Acknowledged.

+            GetPersistentLicenseSessionConfigRule(key_system))) {
       session_types.push_back(
           blink::WebEncryptedMediaSessionType::PersistentLicense);
     }
-    if (IsPersistentReleaseMessageSessionSupported(key_system,
-                                                   is_permission_granted)) {
+    if (config_state.IsRuleSupportedWithCurrentState(
+            GetPersistentReleaseMessageSessionConfigRule(key_system))) {
       session_types.push_back(
           blink::WebEncryptedMediaSessionType::PersistentReleaseMessage);
     }
@@ -474,13 +617,14 @@ 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;
       case CONFIGURATION_REQUIRES_PERMISSION:
-        DCHECK(!was_permission_requested);
+        if (was_permission_requested)

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

This is the code corresponding to my question at line 479.

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

Acknowledged.

+          continue;
         media_permission_->RequestPermission(
             MediaPermission::PROTECTED_MEDIA_IDENTIFIER,
             GURL(request.securityOrigin().toString()),