Implement robustness strings in requestMediaKeySystemAccess().

media/blink/webencryptedmediaclient_impl.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.

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

We really need to test this code thoroughly.
We can mock media_permission_ because it's passed into the constructor.
For Key Systems, we either need to add fake values (like some other tests) or
implement a Fake and pass that into the constructor too.

Perhaps we should file a bug to track this.

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

I've filed a bug for this, assigned to myself.

 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "webencryptedmediaclient_impl.h"
 
 #include "base/bind.h"
 #include "base/logging.h"
 #include "base/metrics/histogram.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
@@ skipping 12 matching lines @@
 // These names are used by UMA.
 const char kKeySystemSupportUMAPrefix[] =
     "Media.EME.RequestMediaKeySystemAccess.";
 
 enum ConfigurationSupport {
   CONFIGURATION_NOT_SUPPORTED,
   CONFIGURATION_REQUIRES_PERMISSION,
   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) {
   // TODO(sandersd): Move contentType parsing from Blink to here so that invalid
   // parameters can be rejected. http://crbug.com/417561
   // TODO(sandersd): Pass in the media type (audio or video) and check that the
   // container type matches. http://crbug.com/457384
   std::string container = base::StringToLowerASCII(mime_type);
 
   // Check that |codecs| are supported by the CDM. This check does not handle
@@ skipping 15 matching lines @@
   // TODO(sandersd): Reject ambiguous codecs. http://crbug.com/374751
   codec_vector.clear();
   net::ParseCodecString(codecs, &codec_vector, false);
   return net::AreSupportedMediaCodecs(codec_vector);
 }
 
 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.
   //    (Skipped as there are no configuration-based codec restrictions.)
   // 2. Let media type capabilities be empty.
   DCHECK_EQ(media_type_capabilities->size(), 0ul);
   // 3. For each value in capabilities:
   for (size_t i = 0; i < capabilities.size(); i++) {
     // 3.1. Let contentType be the value's contentType member.
     // 3.2. Let robustness be the value's robustness member.
     const blink::WebMediaKeySystemMediaCapability& capability = capabilities[i];
     // 3.3. If contentType is the empty string, return null.
     if (capability.mimeType.isEmpty())
       return false;
     // 3.4-3.11. (Implemented by IsSupportedContentType().)
     if (!base::IsStringASCII(capability.mimeType) ||
         !base::IsStringASCII(capability.codecs) ||
         !IsSupportedContentType(key_system,
                                 base::UTF16ToASCII(capability.mimeType),
                                 base::UTF16ToASCII(capability.codecs))) {
       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.
   return !media_type_capabilities->empty();
 }
 
 static EmeFeatureRequirement ConvertRequirement(
     blink::WebMediaKeySystemConfiguration::Requirement requirement) {
   switch (requirement) {
     case blink::WebMediaKeySystemConfiguration::Requirement::Required:
       return EME_FEATURE_REQUIRED;
     case blink::WebMediaKeySystemConfiguration::Requirement::Optional:
       return EME_FEATURE_OPTIONAL;
     case blink::WebMediaKeySystemConfiguration::Requirement::NotAllowed:
       return EME_FEATURE_NOT_ALLOWED;
   }
 
   NOTREACHED();
   return EME_FEATURE_NOT_ALLOWED;
 }
 
 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.)
   // 2. If candidate configuration's initDataTypes attribute is not empty, run
   //    the following steps:
   if (!candidate.initDataTypes.isEmpty()) {
     // 2.1. Let supported types be empty.
     std::vector<blink::WebEncryptedMediaInitDataType> supported_types;
 
     // 2.2. For each value in candidate configuration's initDataTypes attribute:
@@ skipping 40 matching lines @@
 
   // 3. Follow the steps for the value of candidate configuration's
   //    distinctiveIdentifier attribute from the following list:
   //     - "required": If the implementation does not support a persistent
   //       Distinctive Identifier in combination with accumulated configuration,
   //       return null.
   //     - "optional": Continue.
   //     - "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.
   accumulated_configuration->distinctiveIdentifier =
       candidate.distinctiveIdentifier;
 
   // 5. Follow the steps for the value of candidate configuration's
   //    persistentState attribute from the following list:
   //     - "required": If the implementation does not support persisting state
   //       in combination with accumulated configuration, return null.
   //     - "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.
   accumulated_configuration->persistentState = candidate.persistentState;
 
   // 7. If candidate configuration's videoCapabilities attribute is not empty,
   //    run the following steps:
   if (!candidate.videoCapabilities.isEmpty()) {
     // 7.1. Let video capabilities be the result of executing the Get Supported
     //      Capabilities for Media Type algorithm on Video, candidate
     //      configuration's videoCapabilities attribute, and accumulated
     //      configuration.
     // 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;
     }
 
     // 7.3. Add video capabilities to accumulated configuration.
     accumulated_configuration->videoCapabilities = video_capabilities;
   }
 
   // 8. If candidate configuration's audioCapabilities attribute is not empty,
   //    run the following steps:
   if (!candidate.audioCapabilities.isEmpty()) {
     // 8.1. Let audio capabilities be the result of executing the Get Supported
     //      Capabilities for Media Type algorithm on Audio, candidate
     //      configuration's audioCapabilities attribute, and accumulated
     //      configuration.
     // 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;
     }
 
     // 8.3. Add audio capabilities to accumulated configuration.
     accumulated_configuration->audioCapabilities = audio_capabilities;
   }
 
   // 9. If accumulated configuration's distinctiveIdentifier value is
   //    "optional", follow the steps for the first matching condition from the
   //    following list:
   //      - If the implementation requires a Distinctive Identifier for any of
   //        the combinations in accumulated configuration, change accumulated
   //        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.

     }
   }
 
   // 10. If accumulated configuration's persistentState value is "optional",
   //     follow the steps for the first matching condition from the following
   //     list:
   //       - If the implementation requires persisting state for any of the
   //         combinations in accumulated configuration, change accumulated
   //         configuration's persistentState value to "required".
   //       - Otherwise, change accumulated configuration's persistentState value
   //         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
   //     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)) {
+    // 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);
     }
   }
   accumulated_configuration->sessionTypes = session_types;
 
   return CONFIGURATION_SUPPORTED;
 }
 
 // Report usage of key system to UMA. There are 2 different counts logged:
@@ skipping 76 matching lines @@
   // Report this request to the UMA.
   std::string key_system = base::UTF16ToASCII(request.keySystem());
   GetReporter(key_system)->ReportRequested();
 
   if (!IsSupportedKeySystem(key_system)) {
     request.requestNotSupported("Unsupported keySystem");
     return;
   }
 
   // 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);
 }
 
 void WebEncryptedMediaClientImpl::SelectSupportedConfiguration(
     blink::WebEncryptedMediaRequest request,
     bool was_permission_requested,
     bool is_permission_granted) {
   // Continued from requestMediaKeySystemAccess(), step 7.1, from
   // https://w3c.github.io/encrypted-media/#requestmediakeysystemaccess
   //
   // 7.2. Let implementation be the implementation of keySystem.
   std::string key_system = base::UTF16ToASCII(request.keySystem());
 
   // 7.3. For each value in supportedConfigurations:
   const blink::WebVector<blink::WebMediaKeySystemConfiguration>&
       configurations = request.supportedConfigurations();
   for (size_t i = 0; i < configurations.size(); i++) {
     // 7.3.1. Let candidate configuration be the value.
     const blink::WebMediaKeySystemConfiguration& candidate_configuration =
         configurations[i];
     // 7.3.2. Let supported configuration be the result of executing the Get
     //        Supported Configuration algorithm on implementation, candidate
     //        configuration, and origin.
     // 7.3.3. If supported configuration is not null, [initialize and return a
     //        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);
         media_permission_->RequestPermission(
             MediaPermission::PROTECTED_MEDIA_IDENTIFIER,
             GURL(request.securityOrigin().toString()),
             // Try again with |was_permission_requested| true and
             // |is_permission_granted| the value of the permission.
@@ skipping 38 matching lines @@
     return reporter;
 
   // Reporter not found, so create one.
   auto result =
       reporters_.add(uma_name, make_scoped_ptr(new Reporter(uma_name)));
   DCHECK(result.second);
   return result.first->second;
 }
 
 }  // namespace media