Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1005903003:
Implement robustness strings in requestMediaKeySystemAccess(). (Closed)
|
Created:
5 years, 9 months ago by sandersd (OOO until July 31) Modified:
5 years, 9 months ago CC:
chromium-reviews, eme-reviews_chromium.org, feature-media-reviews_chromium.org, lcwu1 Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionImplement robustness strings in requestMediaKeySystemAccess().
BUG=442586, 460616
Committed: https://crrev.com/a8a9143d9373e1ae47f30d3da14c051d16188b3e
Cr-Commit-Position: refs/heads/master@{#322219}
Patch Set 1 #
Total comments: 17
Patch Set 2 : #Patch Set 3 : Rename all the things. #
Total comments: 80
Patch Set 4 : Make distinctiveIdentifier and permission equivalent. #Patch Set 5 : Clarify CrOS Widevine robustness rules. #
Total comments: 14
Patch Set 6 : Rename PERMISSION to IDENTIFIER. #
Total comments: 20
Patch Set 7 : Update key system registration. #Patch Set 8 : Improve comments. #
Total comments: 8
Patch Set 9 : #Patch Set 10 : Change parameter names. #
Total comments: 9
Patch Set 11 : Move ConvertRobustness() into KeySystems. #
Total comments: 2
Patch Set 12 : #Patch Set 13 : Rebase #Patch Set 14 : #Patch Set 15 : Fix layout tests. #Messages
Total messages: 74 (34 generated)
sandersd@chromium.org changed reviewers: + ddorwin@chromium.org, jrummell@chromium.org
Reviewed through the first part of key_systems.cc. https://codereview.chromium.org/1005903003/diff/1/components/cdm/renderer/and... File components/cdm/renderer/android_key_systems.cc (right): https://codereview.chromium.org/1005903003/diff/1/components/cdm/renderer/and... components/cdm/renderer/android_key_systems.cc:53: codecs = static_cast<SupportedCodecs>(response.non_compositing_codecs); Note: Using non_compositing_codecs here matches the actual behavior (comment below). Does it make sense to instead use the logic at 65 rather than checking the switch here? https://codereview.chromium.org/1005903003/diff/1/components/cdm/renderer/and... components/cdm/renderer/android_key_systems.cc:66: // When creating the WIDEVINE_HR_NON_COMPOSITING key system, This is the "L1 key system". It's actually confusing that the one above will be L1. Perhaps this comment belongs up there. Do we have a bug to fix that code? https://codereview.chromium.org/1005903003/diff/1/media/base/eme_constants.h File media/base/eme_constants.h (right): https://codereview.chromium.org/1005903003/diff/1/media/base/eme_constants.h#... media/base/eme_constants.h:78: // Used to query support for distinctive identifier and persistent state. Not so much to query support, but to request a specific behavior. Although, if this is used for querying whether you can match the app's request, then this might be accurate. https://codereview.chromium.org/1005903003/diff/1/media/base/key_systems.cc File media/base/key_systems.cc (right): https://codereview.chromium.org/1005903003/diff/1/media/base/key_systems.cc#n... media/base/key_systems.cc:78: static EmeRequirementSupport ConvertSessionTypeSupport( Should we just eliminate EmeSessionTypeSupport? https://codereview.chromium.org/1005903003/diff/1/media/base/key_systems.cc#n... media/base/key_systems.cc:417: DCHECK(info.persistent_state_support != EME_FEATURE_INVALID); Note: We've only addressed the TODO for desktop. Ditto below. This happens to be fine for all known registrations. https://codereview.chromium.org/1005903003/diff/1/media/base/key_systems.h File media/base/key_systems.h (right): https://codereview.chromium.org/1005903003/diff/1/media/base/key_systems.h#ne... media/base/key_systems.h:89: // indicates how support should interacts with the encrypted media permission. Fix: "support should interacts" https://codereview.chromium.org/1005903003/diff/1/media/base/key_systems.h#ne... media/base/key_systems.h:90: // TODO(sandersd): Also take a list of codecs, as they affect support in some ...as they may affect the result?
https://codereview.chromium.org/1005903003/diff/1/components/cdm/renderer/and... File components/cdm/renderer/android_key_systems.cc (right): https://codereview.chromium.org/1005903003/diff/1/components/cdm/renderer/and... components/cdm/renderer/android_key_systems.cc:53: codecs = static_cast<SupportedCodecs>(response.non_compositing_codecs); On 2015/03/13 19:24:47, ddorwin wrote: > Note: Using non_compositing_codecs here matches the actual behavior (comment > below). > > Does it make sense to instead use the logic at 65 rather than checking the > switch here? I rearranged this to make it more correct, but the short answer is yes, the flag is separate from platform support for the codecs. https://codereview.chromium.org/1005903003/diff/1/components/cdm/renderer/and... components/cdm/renderer/android_key_systems.cc:66: // When creating the WIDEVINE_HR_NON_COMPOSITING key system, On 2015/03/13 19:24:47, ddorwin wrote: > This is the "L1 key system". It's actually confusing that the one above will be > L1. Perhaps this comment belongs up there. Do we have a bug to fix that code? Turns out we do. Done. https://codereview.chromium.org/1005903003/diff/1/media/base/eme_constants.h File media/base/eme_constants.h (right): https://codereview.chromium.org/1005903003/diff/1/media/base/eme_constants.h#... media/base/eme_constants.h:78: // Used to query support for distinctive identifier and persistent state. On 2015/03/13 19:24:47, ddorwin wrote: > Not so much to query support, but to request a specific behavior. Although, if > this is used for querying whether you can match the app's request, then this > might be accurate. The requests are blink::WebMediaKeySystemConfiguration::Requirement values, but they do map 1:1 with these enum values. (ConvertRequirement() does the mapping.) The only actual difference is that I also query things that were not directly requested (when resolving Optional). https://codereview.chromium.org/1005903003/diff/1/media/base/key_systems.cc File media/base/key_systems.cc (right): https://codereview.chromium.org/1005903003/diff/1/media/base/key_systems.cc#n... media/base/key_systems.cc:78: static EmeRequirementSupport ConvertSessionTypeSupport( On 2015/03/13 19:24:47, ddorwin wrote: > Should we just eliminate EmeSessionTypeSupport? Probably. I really don't want to have an INVALID EmeRequirementSupport value though, so I'd want to rip that check out. Is that okay? https://codereview.chromium.org/1005903003/diff/1/media/base/key_systems.cc#n... media/base/key_systems.cc:417: DCHECK(info.persistent_state_support != EME_FEATURE_INVALID); On 2015/03/13 19:24:47, ddorwin wrote: > Note: We've only addressed the TODO for desktop. Ditto below. > This happens to be fine for all known registrations. Done. https://codereview.chromium.org/1005903003/diff/1/media/base/key_systems.h File media/base/key_systems.h (right): https://codereview.chromium.org/1005903003/diff/1/media/base/key_systems.h#ne... media/base/key_systems.h:89: // indicates how support should interacts with the encrypted media permission. On 2015/03/13 19:24:47, ddorwin wrote: > Fix: "support should interacts" Done. https://codereview.chromium.org/1005903003/diff/1/media/base/key_systems.h#ne... media/base/key_systems.h:90: // TODO(sandersd): Also take a list of codecs, as they affect support in some On 2015/03/13 19:24:47, ddorwin wrote: > ...as they may affect the result? Done.
Patchset #3 (id:40001) has been deleted
Patchset #3 (id:60001) has been deleted
Patchset #4 (id:100001) has been deleted
Patchset #3 (id:80001) has been deleted
Patchset #3 (id:120001) has been deleted
https://codereview.chromium.org/1005903003/diff/1/components/cdm/renderer/and... File components/cdm/renderer/android_key_systems.cc (right): https://codereview.chromium.org/1005903003/diff/1/components/cdm/renderer/and... components/cdm/renderer/android_key_systems.cc:66: // When creating the WIDEVINE_HR_NON_COMPOSITING key system, On 2015/03/13 20:11:56, sandersd wrote: > On 2015/03/13 19:24:47, ddorwin wrote: > > This is the "L1 key system". It's actually confusing that the one above will > be > > L1. Perhaps this comment belongs up there. Do we have a bug to fix that code? > > Turns out we do. Done. As I noted, I don't think that bug covers this issue. https://codereview.chromium.org/1005903003/diff/1/media/base/key_systems.cc File media/base/key_systems.cc (right): https://codereview.chromium.org/1005903003/diff/1/media/base/key_systems.cc#n... media/base/key_systems.cc:78: static EmeRequirementSupport ConvertSessionTypeSupport( On 2015/03/13 20:11:56, sandersd wrote: > On 2015/03/13 19:24:47, ddorwin wrote: > > Should we just eliminate EmeSessionTypeSupport? > > Probably. I really don't want to have an INVALID EmeRequirementSupport value > though, so I'd want to rip that check out. Is that okay? Acknowledged. We also have PERMISSION_PREFERRED, which may not make sense for session types. https://codereview.chromium.org/1005903003/diff/140001/components/cdm/rendere... File components/cdm/renderer/android_key_systems.cc (right): https://codereview.chromium.org/1005903003/diff/140001/components/cdm/rendere... components/cdm/renderer/android_key_systems.cc:48: // requested robustness instead of the flag. http://crbug.com/459400 That's probably not exactly this bug. https://codereview.chromium.org/1005903003/diff/140001/components/cdm/rendere... components/cdm/renderer/android_key_systems.cc:50: if (base::CommandLine::ForCurrentProcess()->HasSwitch( My point was that we might want to use if (response.non_compositing_codecs != media::EME_CODEC_NONE) instead so that the switch only controls one location and everything else is based on the results. https://codereview.chromium.org/1005903003/diff/140001/media/base/eme_constan... File media/base/eme_constants.h (right): https://codereview.chromium.org/1005903003/diff/140001/media/base/eme_constan... media/base/eme_constants.h:102: // Configuration rules are recipes for determining if a feature is supported, Not really "recipes"? https://codereview.chromium.org/1005903003/diff/140001/media/base/eme_constan... media/base/eme_constants.h:106: enum class EmeConfigRule { "Rule" doesn't seem appropriate. SupportStatus? SupportCondition? https://codereview.chromium.org/1005903003/diff/140001/media/base/eme_constan... media/base/eme_constants.h:115: SUPPORTED, This does not indicate whether it can be disabled. Perhaps the questions are "does support not using" to which SUPPORTED would be returned. If so, then the comment should probably be updated - "feature" can also be not doing something. https://codereview.chromium.org/1005903003/diff/140001/media/base/key_systems.cc File media/base/key_systems.cc (right): https://codereview.chromium.org/1005903003/diff/140001/media/base/key_systems... media/base/key_systems.cc:428: // Do not allow registration because it is not currently supported: http://crbug.com/448888. DCHECK(info.persistent_release_message_support == EME_FEATURE_NOT_SUPPORTED); https://codereview.chromium.org/1005903003/diff/140001/media/base/key_systems... media/base/key_systems.cc:433: // for PPAPI key systems. nit: s/PPAPI/Pepper-hosted/ new sentence - something like: Do not allow registration with non-absolute values on other platforms. https://codereview.chromium.org/1005903003/diff/140001/media/base/key_systems... media/base/key_systems.cc:440: DCHECK(info.distinctive_identifier_support == EME_FEATURE_NOT_SUPPORTED || Should we be checking persistent state too? https://codereview.chromium.org/1005903003/diff/140001/media/base/key_systems... media/base/key_systems.cc:676: return (robustness <= max_robustness) You could get rid of this by having #else recommendation = EmeConfigRule::SUPPORTED; and replacing line 683. https://codereview.chromium.org/1005903003/diff/140001/media/base/key_systems... media/base/key_systems.cc:678: : EmeConfigRule::PERMISSION_REQUIRED; Otherwise: NOT_SUPPORTED, right? https://codereview.chromium.org/1005903003/diff/140001/media/base/key_systems... media/base/key_systems.cc:733: requirement == EME_FEATURE_NOT_ALLOWED) { It is line 440 that ensures that EME_FEATURE_REQUESTABLE* is either not allowed or is properly blockable for not-allowed, right? It's probably worth commenting that here. Another option would be to put the code at line 440 into a function and call it here and there. https://codereview.chromium.org/1005903003/diff/140001/media/base/key_systems... media/base/key_systems.cc:739: // for optional requirements will be handled when they are resolved to Just so I understand, EME_FEATURE_OPTIONAL could get here and fall through to line 745 (on the first pass), right? https://codereview.chromium.org/1005903003/diff/140001/media/base/key_systems... media/base/key_systems.cc:767: requirement == EME_FEATURE_NOT_ALLOWED) { ditto https://codereview.chromium.org/1005903003/diff/140001/media/base/key_systems.h File media/base/key_systems.h (right): https://codereview.chromium.org/1005903003/diff/140001/media/base/key_systems... media/base/key_systems.h:88: // Return which configuration rule to use for a robustness requirement. "Return_s_" here and below. https://codereview.chromium.org/1005903003/diff/140001/media/base/key_systems... media/base/key_systems.h:88: // Return which configuration rule to use for a robustness requirement. Does the caller really "use" the rule? https://codereview.chromium.org/1005903003/diff/140001/media/blink/webencrypt... File media/blink/webencryptedmediaclient_impl.cc (right): https://codereview.chromium.org/1005903003/diff/140001/media/blink/webencrypt... media/blink/webencryptedmediaclient_impl.cc:1: // Copyright 2014 The Chromium Authors. All rights reserved. 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. https://codereview.chromium.org/1005903003/diff/140001/media/blink/webencrypt... media/blink/webencryptedmediaclient_impl.cc:47: is_permission_required_(false), Use a tri-state enum instead of two bools? https://codereview.chromium.org/1005903003/diff/140001/media/blink/webencrypt... media/blink/webencryptedmediaclient_impl.cc:54: bool IsValid() { const here and below - all except AddRule(). https://codereview.chromium.org/1005903003/diff/140001/media/blink/webencrypt... media/blink/webencryptedmediaclient_impl.cc:67: bool IsPermissionRecommended() { 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. https://codereview.chromium.org/1005903003/diff/140001/media/blink/webencrypt... media/blink/webencryptedmediaclient_impl.cc:74: bool IsPermissionRequestRequired() { 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()? https://codereview.chromium.org/1005903003/diff/140001/media/blink/webencrypt... media/blink/webencryptedmediaclient_impl.cc:75: return is_permission_required_ && !is_permission_granted_ && 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.) https://codereview.chromium.org/1005903003/diff/140001/media/blink/webencrypt... media/blink/webencryptedmediaclient_impl.cc:95: // Checks whether a rule is compatible with all previously added rules, This doesn't seem consistent with the function name. Also, I wouldn't assume that such a function would alter any state. https://codereview.chromium.org/1005903003/diff/140001/media/blink/webencrypt... media/blink/webencryptedmediaclient_impl.cc:97: // rules after the permission request, the correct solution is to always Why don't we do that? What's the reason it is this way? https://codereview.chromium.org/1005903003/diff/140001/media/blink/webencrypt... media/blink/webencryptedmediaclient_impl.cc:99: bool IsRuleSupportedWithoutPrompt(EmeConfigRule rule) { 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? https://codereview.chromium.org/1005903003/diff/140001/media/blink/webencrypt... media/blink/webencryptedmediaclient_impl.cc:132: bool was_permission_requested_; It might help to explain what this means? I assume we will only resume use of this class after the request is requested/denied. https://codereview.chromium.org/1005903003/diff/140001/media/blink/webencrypt... media/blink/webencryptedmediaclient_impl.cc:223: continue; 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. https://codereview.chromium.org/1005903003/diff/140001/media/blink/webencrypt... media/blink/webencryptedmediaclient_impl.cc:226: // (It's already added, the original object is used directly.) by the caller? https://codereview.chromium.org/1005903003/diff/140001/media/blink/webencrypt... media/blink/webencryptedmediaclient_impl.cc:234: // based on robustness.) Bug? https://codereview.chromium.org/1005903003/diff/140001/media/blink/webencrypt... media/blink/webencryptedmediaclient_impl.cc:411: if (not_allowed_supported && required_supported) { 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: https://codereview.chromium.org/1005903003/diff/140001/media/blink/webencrypt... media/blink/webencryptedmediaclient_impl.cc:415: config_state.AddRule(required_rule); Should we add "DCHECK(IsPermissionRequestRequired());"? It was unclear initially how we get from the above to IsPermissionRequestRequired(). https://codereview.chromium.org/1005903003/diff/140001/media/blink/webencrypt... media/blink/webencryptedmediaclient_impl.cc:430: return CONFIGURATION_NOT_SUPPORTED; Is this reachable? It seems we should have failed earlier if neither required nor not allowed are supported. If we think so, add NOTREACHED(). https://codereview.chromium.org/1005903003/diff/140001/media/blink/webencrypt... media/blink/webencryptedmediaclient_impl.cc:459: return CONFIGURATION_NOT_SUPPORTED; ditto https://codereview.chromium.org/1005903003/diff/140001/media/blink/webencrypt... media/blink/webencryptedmediaclient_impl.cc:470: // TODO(sandersd): Prove that this can't happen. Step 1: Add NOTREACHED() :) https://codereview.chromium.org/1005903003/diff/140001/media/blink/webencrypt... media/blink/webencryptedmediaclient_impl.cc:474: // If a permission is required but has not been requested, prompt. prompt is one possible outcome. Replace with "request permission". https://codereview.chromium.org/1005903003/diff/140001/media/blink/webencrypt... media/blink/webencryptedmediaclient_impl.cc:475: if (config_state.IsPermissionRequestRequired()) Initially I wondered, How do we get from preferred to requesting the permission? Now, I see we change the config above. https://codereview.chromium.org/1005903003/diff/140001/media/blink/webencrypt... media/blink/webencryptedmediaclient_impl.cc:485: // TODO(sandersd): This should happen sooner so that it can affect the Is this solved by updating the spec? We would only want to do this based on a request, though, which requires the spec update. https://codereview.chromium.org/1005903003/diff/140001/media/blink/webencrypt... media/blink/webencryptedmediaclient_impl.cc:590: // TODO(sandersd): Query the permission first. Why? It might shortcut some of the code execution, but always running the same code is also nice.
https://codereview.chromium.org/1005903003/diff/1/components/cdm/renderer/and... File components/cdm/renderer/android_key_systems.cc (right): https://codereview.chromium.org/1005903003/diff/1/components/cdm/renderer/and... components/cdm/renderer/android_key_systems.cc:66: // When creating the WIDEVINE_HR_NON_COMPOSITING key system, On 2015/03/16 23:25:07, ddorwin wrote: > On 2015/03/13 20:11:56, sandersd wrote: > > On 2015/03/13 19:24:47, ddorwin wrote: > > > This is the "L1 key system". It's actually confusing that the one above will > > be > > > L1. Perhaps this comment belongs up there. Do we have a bug to fix that > code? > > > > Turns out we do. Done. > > As I noted, I don't think that bug covers this issue. Acknowledged. https://codereview.chromium.org/1005903003/diff/140001/components/cdm/rendere... File components/cdm/renderer/android_key_systems.cc (right): https://codereview.chromium.org/1005903003/diff/140001/components/cdm/rendere... components/cdm/renderer/android_key_systems.cc:48: // requested robustness instead of the flag. http://crbug.com/459400 On 2015/03/16 23:25:08, ddorwin wrote: > That's probably not exactly this bug. Done. https://codereview.chromium.org/1005903003/diff/140001/components/cdm/rendere... components/cdm/renderer/android_key_systems.cc:50: if (base::CommandLine::ForCurrentProcess()->HasSwitch( On 2015/03/16 23:25:08, ddorwin wrote: > My point was that we might want to use > if (response.non_compositing_codecs != media::EME_CODEC_NONE) > instead so that the switch only controls one location and everything else is > based on the results. Unfortunately the com.widevine.alpha key system may operate in L1 mode (based on the flag), and the flag does not affect the list of supported codecs. The implementation I have matches what a user will actually get if they request the named key systems. https://codereview.chromium.org/1005903003/diff/140001/media/base/eme_constan... File media/base/eme_constants.h (right): https://codereview.chromium.org/1005903003/diff/140001/media/base/eme_constan... media/base/eme_constants.h:102: // Configuration rules are recipes for determining if a feature is supported, On 2015/03/16 23:25:08, ddorwin wrote: > Not really "recipes"? Done. https://codereview.chromium.org/1005903003/diff/140001/media/base/eme_constan... media/base/eme_constants.h:106: enum class EmeConfigRule { On 2015/03/16 23:25:08, ddorwin wrote: > "Rule" doesn't seem appropriate. SupportStatus? SupportCondition? Condition is accurate. Let me know if it still needs to be changed now that the comments are improved. https://codereview.chromium.org/1005903003/diff/140001/media/base/eme_constan... media/base/eme_constants.h:115: SUPPORTED, On 2015/03/16 23:25:08, ddorwin wrote: > This does not indicate whether it can be disabled. Perhaps the questions are > "does support not using" to which SUPPORTED would be returned. If so, then the > comment should probably be updated - "feature" can also be not doing something. Done. https://codereview.chromium.org/1005903003/diff/140001/media/base/key_systems.cc File media/base/key_systems.cc (right): https://codereview.chromium.org/1005903003/diff/140001/media/base/key_systems... media/base/key_systems.cc:428: On 2015/03/16 23:25:08, ddorwin wrote: > // Do not allow registration because it is not currently supported: > http://crbug.com/448888. > DCHECK(info.persistent_release_message_support == EME_FEATURE_NOT_SUPPORTED); Done. https://codereview.chromium.org/1005903003/diff/140001/media/base/key_systems... media/base/key_systems.cc:433: // for PPAPI key systems. On 2015/03/16 23:25:08, ddorwin wrote: > nit: s/PPAPI/Pepper-hosted/ > > new sentence - something like: Do not allow registration with non-absolute > values on other platforms. Done. https://codereview.chromium.org/1005903003/diff/140001/media/base/key_systems... media/base/key_systems.cc:440: DCHECK(info.distinctive_identifier_support == EME_FEATURE_NOT_SUPPORTED || On 2015/03/16 23:25:08, ddorwin wrote: > Should we be checking persistent state too? Done. https://codereview.chromium.org/1005903003/diff/140001/media/base/key_systems... media/base/key_systems.cc:676: return (robustness <= max_robustness) On 2015/03/16 23:25:08, ddorwin wrote: > You could get rid of this by having > #else > recommendation = EmeConfigRule::SUPPORTED; > > and replacing line 683. See next comment. https://codereview.chromium.org/1005903003/diff/140001/media/base/key_systems... media/base/key_systems.cc:678: : EmeConfigRule::PERMISSION_REQUIRED; On 2015/03/16 23:25:08, ddorwin wrote: > Otherwise: NOT_SUPPORTED, right? This is confusing because I am imagining that max_robustness will be L3. (Thus everything above max_robustness is in fact supported, as long as RA succeeds.) It may make more sense to hardcode both limits, WDYT? https://codereview.chromium.org/1005903003/diff/140001/media/base/key_systems... media/base/key_systems.cc:733: requirement == EME_FEATURE_NOT_ALLOWED) { On 2015/03/16 23:25:08, ddorwin wrote: > It is line 440 that ensures that EME_FEATURE_REQUESTABLE* is either not allowed > or is properly blockable for not-allowed, right? > > It's probably worth commenting that here. Another option would be to put the > code at line 440 into a function and call it here and there. It turns out that the state table is really simple, there are only two cases that are NOT_SUPPORTED; the rest are always supported in some capacity (how exactly is different for PS and DI). If we didn't block those options at registration, they would still be handled correctly here. https://codereview.chromium.org/1005903003/diff/140001/media/base/key_systems... media/base/key_systems.cc:739: // for optional requirements will be handled when they are resolved to On 2015/03/16 23:25:08, ddorwin wrote: > Just so I understand, EME_FEATURE_OPTIONAL could get here and fall through to > line 745 (on the first pass), right? Indeed. https://codereview.chromium.org/1005903003/diff/140001/media/base/key_systems... media/base/key_systems.cc:767: requirement == EME_FEATURE_NOT_ALLOWED) { On 2015/03/16 23:25:08, ddorwin wrote: > ditto Acknowledged. https://codereview.chromium.org/1005903003/diff/140001/media/base/key_systems.h File media/base/key_systems.h (right): https://codereview.chromium.org/1005903003/diff/140001/media/base/key_systems... media/base/key_systems.h:88: // Return which configuration rule to use for a robustness requirement. On 2015/03/16 23:25:08, ddorwin wrote: > "Return_s_" here and below. Done. https://codereview.chromium.org/1005903003/diff/140001/media/base/key_systems... media/base/key_systems.h:88: // Return which configuration rule to use for a robustness requirement. On 2015/03/16 23:25:08, ddorwin wrote: > Does the caller really "use" the rule? Done. https://codereview.chromium.org/1005903003/diff/140001/media/blink/webencrypt... File media/blink/webencryptedmediaclient_impl.cc (right): https://codereview.chromium.org/1005903003/diff/140001/media/blink/webencrypt... media/blink/webencryptedmediaclient_impl.cc:47: is_permission_required_(false), On 2015/03/16 23:25:09, ddorwin wrote: > Use a tri-state enum instead of two bools? They are actually handled separately, although it is true that if permission is required then it is also in some sense recommended. https://codereview.chromium.org/1005903003/diff/140001/media/blink/webencrypt... media/blink/webencryptedmediaclient_impl.cc:54: bool IsValid() { On 2015/03/16 23:25:09, ddorwin wrote: > const here and below - all except AddRule(). Done. https://codereview.chromium.org/1005903003/diff/140001/media/blink/webencrypt... media/blink/webencryptedmediaclient_impl.cc:67: bool IsPermissionRecommended() { On 2015/03/16 23:25:08, ddorwin wrote: > 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. I went a different direction here but hopefully it is better for you too. https://codereview.chromium.org/1005903003/diff/140001/media/blink/webencrypt... media/blink/webencryptedmediaclient_impl.cc:74: bool IsPermissionRequestRequired() { On 2015/03/16 23:25:08, ddorwin wrote: > 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()? Done. https://codereview.chromium.org/1005903003/diff/140001/media/blink/webencrypt... media/blink/webencryptedmediaclient_impl.cc:75: return is_permission_required_ && !is_permission_granted_ && On 2015/03/16 23:25:10, ddorwin wrote: > 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.) Acknowledged. https://codereview.chromium.org/1005903003/diff/140001/media/blink/webencrypt... media/blink/webencryptedmediaclient_impl.cc:95: // Checks whether a rule is compatible with all previously added rules, On 2015/03/16 23:25:09, ddorwin wrote: > This doesn't seem consistent with the function name. > Also, I wouldn't assume that such a function would alter any state. Done. https://codereview.chromium.org/1005903003/diff/140001/media/blink/webencrypt... media/blink/webencryptedmediaclient_impl.cc:97: // rules after the permission request, the correct solution is to always On 2015/03/16 23:25:09, ddorwin wrote: > Why don't we do that? What's the reason it is this way? This is because we don't want session types to affect config state until the exact behavior is spec'd. https://codereview.chromium.org/1005903003/diff/140001/media/blink/webencrypt... media/blink/webencryptedmediaclient_impl.cc:99: bool IsRuleSupportedWithoutPrompt(EmeConfigRule rule) { On 2015/03/16 23:25:09, ddorwin wrote: > 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? Done. https://codereview.chromium.org/1005903003/diff/140001/media/blink/webencrypt... media/blink/webencryptedmediaclient_impl.cc:132: bool was_permission_requested_; On 2015/03/16 23:25:09, ddorwin wrote: > It might help to explain what this means? I assume we will only resume use of > this class after the request is requested/denied. I'm not certain what you mean; a temporary class is created for evaluating each configuration, and it is thrown away immediately. https://codereview.chromium.org/1005903003/diff/140001/media/blink/webencrypt... media/blink/webencryptedmediaclient_impl.cc:226: // (It's already added, the original object is used directly.) On 2015/03/16 23:25:09, ddorwin wrote: > by the caller? No, when we push the capability on to media_type_capabilities. https://codereview.chromium.org/1005903003/diff/140001/media/blink/webencrypt... media/blink/webencryptedmediaclient_impl.cc:234: // based on robustness.) On 2015/03/16 23:25:09, ddorwin wrote: > Bug? Done. https://codereview.chromium.org/1005903003/diff/140001/media/blink/webencrypt... media/blink/webencryptedmediaclient_impl.cc:411: if (not_allowed_supported && required_supported) { On 2015/03/16 23:25:09, ddorwin wrote: > 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: Done. https://codereview.chromium.org/1005903003/diff/140001/media/blink/webencrypt... media/blink/webencryptedmediaclient_impl.cc:415: config_state.AddRule(required_rule); On 2015/03/16 23:25:10, ddorwin wrote: > Should we add "DCHECK(IsPermissionRequestRequired());"? It was unclear initially > how we get from the above to IsPermissionRequestRequired(). I've changed the definition of AddRule() to set |is_permission_required_| instead of separately tracking the recommendation. https://codereview.chromium.org/1005903003/diff/140001/media/blink/webencrypt... media/blink/webencryptedmediaclient_impl.cc:430: return CONFIGURATION_NOT_SUPPORTED; On 2015/03/16 23:25:10, ddorwin wrote: > Is this reachable? It seems we should have failed earlier if neither required > nor not allowed are supported. > > If we think so, add NOTREACHED(). Done. https://codereview.chromium.org/1005903003/diff/140001/media/blink/webencrypt... media/blink/webencryptedmediaclient_impl.cc:430: return CONFIGURATION_NOT_SUPPORTED; On 2015/03/16 23:25:10, ddorwin wrote: > Is this reachable? It seems we should have failed earlier if neither required > nor not allowed are supported. > > If we think so, add NOTREACHED(). It was before, but I've fixed up GetDistinctiveIdentifierConfigRule() to guarantee it won't be. https://codereview.chromium.org/1005903003/diff/140001/media/blink/webencrypt... media/blink/webencryptedmediaclient_impl.cc:459: return CONFIGURATION_NOT_SUPPORTED; On 2015/03/16 23:25:10, ddorwin wrote: > ditto Done. https://codereview.chromium.org/1005903003/diff/140001/media/blink/webencrypt... media/blink/webencryptedmediaclient_impl.cc:470: // TODO(sandersd): Prove that this can't happen. On 2015/03/16 23:25:09, ddorwin wrote: > Step 1: Add NOTREACHED() :) Done. https://codereview.chromium.org/1005903003/diff/140001/media/blink/webencrypt... media/blink/webencryptedmediaclient_impl.cc:474: // If a permission is required but has not been requested, prompt. On 2015/03/16 23:25:09, ddorwin wrote: > prompt is one possible outcome. Replace with "request permission". Done. https://codereview.chromium.org/1005903003/diff/140001/media/blink/webencrypt... media/blink/webencryptedmediaclient_impl.cc:475: if (config_state.IsPermissionRequestRequired()) On 2015/03/16 23:25:10, ddorwin wrote: > Initially I wondered, How do we get from preferred to requesting the permission? > Now, I see we change the config above. Acknowledged. https://codereview.chromium.org/1005903003/diff/140001/media/blink/webencrypt... media/blink/webencryptedmediaclient_impl.cc:485: // TODO(sandersd): This should happen sooner so that it can affect the On 2015/03/16 23:25:09, ddorwin wrote: > Is this solved by updating the spec? > > We would only want to do this based on a request, though, which requires the > spec update. It will be, yes. https://codereview.chromium.org/1005903003/diff/140001/media/blink/webencrypt... media/blink/webencryptedmediaclient_impl.cc:590: // TODO(sandersd): Query the permission first. On 2015/03/16 23:25:09, ddorwin wrote: > Why? It might shortcut some of the code execution, but always running the same > code is also nice. Done.
Reviewed all but last file. https://codereview.chromium.org/1005903003/diff/140001/components/cdm/rendere... File components/cdm/renderer/android_key_systems.cc (right): https://codereview.chromium.org/1005903003/diff/140001/components/cdm/rendere... components/cdm/renderer/android_key_systems.cc:50: if (base::CommandLine::ForCurrentProcess()->HasSwitch( On 2015/03/17 22:10:38, sandersd wrote: > On 2015/03/16 23:25:08, ddorwin wrote: > > My point was that we might want to use > > if (response.non_compositing_codecs != media::EME_CODEC_NONE) > > instead so that the switch only controls one location and everything else is > > based on the results. > > Unfortunately the com.widevine.alpha key system may operate in L1 mode (based on > the flag), and the flag does not affect the list of supported codecs. The > implementation I have matches what a user will actually get if they request the > named key systems. Oh. :( Thanks. https://codereview.chromium.org/1005903003/diff/170001/media/base/eme_constan... File media/base/eme_constants.h (right): https://codereview.chromium.org/1005903003/diff/170001/media/base/eme_constan... media/base/eme_constants.h:54: // that it is equivalent to remote attestation succeeding. (Remote attestation This only applies to Chrome OS. That should be clear. Instead, you could just say this is to the best of our knowledge and/or based on the expected outcome. https://codereview.chromium.org/1005903003/diff/170001/media/base/key_systems.cc File media/base/key_systems.cc (right): https://codereview.chromium.org/1005903003/diff/170001/media/base/key_systems... media/base/key_systems.cc:427: if (info.persistent_state_support == nit: else if. You could also make this a switch statement. https://codereview.chromium.org/1005903003/diff/170001/media/base/key_systems... media/base/key_systems.cc:695: if (robustness > max_robustness) You _could_ move this to 692 and simplify 714. https://codereview.chromium.org/1005903003/diff/170001/media/base/key_systems... media/base/key_systems.cc:709: return (robustness <= EmeRobustness::SW_SECURE_DECODE) It might be possible to simplify this logic with a few if(x) return y. For example, put "if > SW_S_D, return REQUIRED" at line 698. Then "if (logic), return RECOMMENDED". And finally, return SUPPORTED. https://codereview.chromium.org/1005903003/diff/170001/media/base/key_systems... media/base/key_systems.cc:761: // return the lest restrictive of the two rules; this guarantees that if s/lest/least/ https://codereview.chromium.org/1005903003/diff/170001/media/base/key_systems... media/base/key_systems.cc:815: requirement == EME_FEATURE_REQUIRED) { This might be easier to follow if it was a switch() on requirement. Also, we check for EME_FEATURE_REQUIRED twice. Or maybe the current order has other advantages. Same above. https://codereview.chromium.org/1005903003/diff/170001/media/base/key_systems.h File media/base/key_systems.h (right): https://codereview.chromium.org/1005903003/diff/170001/media/base/key_systems... media/base/key_systems.h:88: // Returns the configuration rule for supporting a robustness requirement. s/rule/option/ like the comments in the definition?
https://codereview.chromium.org/1005903003/diff/170001/media/base/eme_constan... File media/base/eme_constants.h (right): https://codereview.chromium.org/1005903003/diff/170001/media/base/eme_constan... media/base/eme_constants.h:54: // that it is equivalent to remote attestation succeeding. (Remote attestation On 2015/03/18 01:08:04, ddorwin wrote: > This only applies to Chrome OS. That should be clear. > Instead, you could just say this is to the best of our knowledge and/or based on > the expected outcome. Done. https://codereview.chromium.org/1005903003/diff/170001/media/base/key_systems.cc File media/base/key_systems.cc (right): https://codereview.chromium.org/1005903003/diff/170001/media/base/key_systems... media/base/key_systems.cc:427: if (info.persistent_state_support == On 2015/03/18 01:08:04, ddorwin wrote: > nit: else if. > > You could also make this a switch statement. Done. https://codereview.chromium.org/1005903003/diff/170001/media/base/key_systems... media/base/key_systems.cc:695: if (robustness > max_robustness) On 2015/03/18 01:08:04, ddorwin wrote: > You _could_ move this to 692 and simplify 714. Done. https://codereview.chromium.org/1005903003/diff/170001/media/base/key_systems... media/base/key_systems.cc:709: return (robustness <= EmeRobustness::SW_SECURE_DECODE) On 2015/03/18 01:08:04, ddorwin wrote: > It might be possible to simplify this logic with a few if(x) return y. For > example, put "if > SW_S_D, return REQUIRED" at line 698. Then "if (logic), > return RECOMMENDED". And finally, return SUPPORTED. Done. https://codereview.chromium.org/1005903003/diff/170001/media/base/key_systems... media/base/key_systems.cc:761: // return the lest restrictive of the two rules; this guarantees that if On 2015/03/18 01:08:04, ddorwin wrote: > s/lest/least/ Done. https://codereview.chromium.org/1005903003/diff/170001/media/base/key_systems... media/base/key_systems.cc:815: requirement == EME_FEATURE_REQUIRED) { On 2015/03/18 01:08:04, ddorwin wrote: > This might be easier to follow if it was a switch() on requirement. Also, we > check for EME_FEATURE_REQUIRED twice. > > Or maybe the current order has other advantages. > > Same above. Done. https://codereview.chromium.org/1005903003/diff/170001/media/base/key_systems.h File media/base/key_systems.h (right): https://codereview.chromium.org/1005903003/diff/170001/media/base/key_systems... media/base/key_systems.h:88: // Returns the configuration rule for supporting a robustness requirement. On 2015/03/18 01:08:04, ddorwin wrote: > s/rule/option/ like the comments in the definition? (media_type, robustness) is the option, the conditions to support that option is the rule.
I've now reviewed everything through PS6. LG. There are a couple comments in PS3. (All new comments are in the last file.) https://codereview.chromium.org/1005903003/diff/140001/media/blink/webencrypt... File media/blink/webencryptedmediaclient_impl.cc (right): https://codereview.chromium.org/1005903003/diff/140001/media/blink/webencrypt... media/blink/webencryptedmediaclient_impl.cc:132: bool was_permission_requested_; On 2015/03/17 22:10:40, sandersd wrote: > On 2015/03/16 23:25:09, ddorwin wrote: > > It might help to explain what this means? I assume we will only resume use of > > this class after the request is requested/denied. > > I'm not certain what you mean; a temporary class is created for evaluating each > configuration, and it is thrown away immediately. 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. https://codereview.chromium.org/1005903003/diff/140001/media/blink/webencrypt... media/blink/webencryptedmediaclient_impl.cc:415: config_state.AddRule(required_rule); On 2015/03/17 22:10:40, sandersd wrote: > On 2015/03/16 23:25:10, ddorwin wrote: > > Should we add "DCHECK(IsPermissionRequestRequired());"? It was unclear > initially > > how we get from the above to IsPermissionRequestRequired(). > > I've changed the definition of AddRule() to set |is_permission_required_| > instead of separately tracking the recommendation. 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. https://codereview.chromium.org/1005903003/diff/190001/media/blink/webencrypt... File media/blink/webencryptedmediaclient_impl.cc (right): https://codereview.chromium.org/1005903003/diff/190001/media/blink/webencrypt... media/blink/webencryptedmediaclient_impl.cc:82: bool IsRuleSupportedWithCurrentState(EmeConfigRule rule) const { "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. https://codereview.chromium.org/1005903003/diff/190001/media/blink/webencrypt... media/blink/webencryptedmediaclient_impl.cc:115: // Was a permission to usa a distinctive identifier requested? nit: Ending in '?' is a bit weird. Perhaps "Whether... was requested." or "Whether a request... has been made." Ditto below, especially "Do we..." https://codereview.chromium.org/1005903003/diff/190001/media/blink/webencrypt... media/blink/webencryptedmediaclient_impl.cc:116: bool was_permission_requested_; These first two can be const. That might help readability too. https://codereview.chromium.org/1005903003/diff/190001/media/blink/webencrypt... media/blink/webencryptedmediaclient_impl.cc:219: // (It's already added, the original object is used directly.) Suggest: It's added when the original object is pushed onto media_type_capabilities below. https://codereview.chromium.org/1005903003/diff/190001/media/blink/webencrypt... media/blink/webencryptedmediaclient_impl.cc:454: config_state.IsRuleSupportedWithCurrentState(not_allowed_rule); 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? https://codereview.chromium.org/1005903003/diff/190001/media/blink/webencrypt... media/blink/webencryptedmediaclient_impl.cc:479: return CONFIGURATION_REQUIRES_PERMISSION; 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? https://codereview.chromium.org/1005903003/diff/190001/media/blink/webencrypt... media/blink/webencryptedmediaclient_impl.cc:490: if (config_state.IsRuleSupportedWithCurrentState( This one is more clear that we're not going to request permission because we would have exited at 479. https://codereview.chromium.org/1005903003/diff/190001/media/blink/webencrypt... media/blink/webencryptedmediaclient_impl.cc:626: if (was_permission_requested) This is the code corresponding to my question at line 479.
https://codereview.chromium.org/1005903003/diff/140001/media/blink/webencrypt... File media/blink/webencryptedmediaclient_impl.cc (right): https://codereview.chromium.org/1005903003/diff/140001/media/blink/webencrypt... media/blink/webencryptedmediaclient_impl.cc:1: // Copyright 2014 The Chromium Authors. All rights reserved. On 2015/03/16 23:25:10, ddorwin wrote: > 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. I've filed a bug for this, assigned to myself. https://codereview.chromium.org/1005903003/diff/140001/media/blink/webencrypt... media/blink/webencryptedmediaclient_impl.cc:132: bool was_permission_requested_; On 2015/03/19 18:08:35, ddorwin wrote: > On 2015/03/17 22:10:40, sandersd wrote: > > On 2015/03/16 23:25:09, ddorwin wrote: > > > It might help to explain what this means? I assume we will only resume use > of > > > this class after the request is requested/denied. > > > > I'm not certain what you mean; a temporary class is created for evaluating > each > > configuration, and it is thrown away immediately. > > 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. Done. https://codereview.chromium.org/1005903003/diff/140001/media/blink/webencrypt... media/blink/webencryptedmediaclient_impl.cc:223: continue; On 2015/03/16 23:25:09, ddorwin wrote: > 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. Done. https://codereview.chromium.org/1005903003/diff/140001/media/blink/webencrypt... media/blink/webencryptedmediaclient_impl.cc:415: config_state.AddRule(required_rule); On 2015/03/19 18:08:35, ddorwin wrote: > On 2015/03/17 22:10:40, sandersd wrote: > > On 2015/03/16 23:25:10, ddorwin wrote: > > > Should we add "DCHECK(IsPermissionRequestRequired());"? It was unclear > > initially > > > how we get from the above to IsPermissionRequestRequired(). > > > > I've changed the definition of AddRule() to set |is_permission_required_| > > instead of separately tracking the recommendation. > > 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. Acknowledged. https://codereview.chromium.org/1005903003/diff/190001/media/blink/webencrypt... File media/blink/webencryptedmediaclient_impl.cc (right): https://codereview.chromium.org/1005903003/diff/190001/media/blink/webencrypt... media/blink/webencryptedmediaclient_impl.cc:82: bool IsRuleSupportedWithCurrentState(EmeConfigRule rule) const { On 2015/03/19 18:08:35, ddorwin wrote: > "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. 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. https://codereview.chromium.org/1005903003/diff/190001/media/blink/webencrypt... media/blink/webencryptedmediaclient_impl.cc:115: // Was a permission to usa a distinctive identifier requested? On 2015/03/19 18:08:35, ddorwin wrote: > nit: Ending in '?' is a bit weird. Perhaps "Whether... was requested." or > "Whether a request... has been made." > > Ditto below, especially "Do we..." Done. https://codereview.chromium.org/1005903003/diff/190001/media/blink/webencrypt... media/blink/webencryptedmediaclient_impl.cc:116: bool was_permission_requested_; On 2015/03/19 18:08:35, ddorwin wrote: > These first two can be const. That might help readability too. Done. https://codereview.chromium.org/1005903003/diff/190001/media/blink/webencrypt... media/blink/webencryptedmediaclient_impl.cc:219: // (It's already added, the original object is used directly.) On 2015/03/19 18:08:35, ddorwin wrote: > Suggest: It's added when the original object is pushed onto > media_type_capabilities below. Done. https://codereview.chromium.org/1005903003/diff/190001/media/blink/webencrypt... media/blink/webencryptedmediaclient_impl.cc:454: config_state.IsRuleSupportedWithCurrentState(not_allowed_rule); On 2015/03/19 18:08:35, ddorwin wrote: > 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? 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. https://codereview.chromium.org/1005903003/diff/190001/media/blink/webencrypt... media/blink/webencryptedmediaclient_impl.cc:479: return CONFIGURATION_REQUIRES_PERMISSION; On 2015/03/19 18:08:35, ddorwin wrote: > 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? 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. https://codereview.chromium.org/1005903003/diff/190001/media/blink/webencrypt... media/blink/webencryptedmediaclient_impl.cc:490: if (config_state.IsRuleSupportedWithCurrentState( On 2015/03/19 18:08:35, ddorwin wrote: > This one is more clear that we're not going to request permission because we > would have exited at 479. Acknowledged. https://codereview.chromium.org/1005903003/diff/190001/media/blink/webencrypt... media/blink/webencryptedmediaclient_impl.cc:626: if (was_permission_requested) On 2015/03/19 18:08:35, ddorwin wrote: > This is the code corresponding to my question at line 479. Acknowledged.
LG. Two comments in the last file are in PS6. https://codereview.chromium.org/1005903003/diff/190001/media/blink/webencrypt... File media/blink/webencryptedmediaclient_impl.cc (right): https://codereview.chromium.org/1005903003/diff/190001/media/blink/webencrypt... media/blink/webencryptedmediaclient_impl.cc:454: config_state.IsRuleSupportedWithCurrentState(not_allowed_rule); On 2015/03/19 20:05:09, sandersd wrote: > On 2015/03/19 18:08:35, ddorwin wrote: > > 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? > > 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. Should we add a comment pointing this out, either here or somewhere above after the "point of no return?" https://codereview.chromium.org/1005903003/diff/190001/media/blink/webencrypt... media/blink/webencryptedmediaclient_impl.cc:479: return CONFIGURATION_REQUIRES_PERMISSION; On 2015/03/19 20:05:10, sandersd wrote: > On 2015/03/19 18:08:35, ddorwin wrote: > > 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? > > 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. Okay. Perhaps note that this is handled by the caller here. https://codereview.chromium.org/1005903003/diff/230001/media/base/key_systems.cc File media/base/key_systems.cc (right): https://codereview.chromium.org/1005903003/diff/230001/media/base/key_systems... media/base/key_systems.cc:698: (max_robustness == EmeRobustness::SW_SECURE_DECODE && Isn't this covered by line 700? https://codereview.chromium.org/1005903003/diff/230001/media/blink/webencrypt... File media/blink/webencryptedmediaclient_impl.cc (right): https://codereview.chromium.org/1005903003/diff/230001/media/blink/webencrypt... media/blink/webencryptedmediaclient_impl.cc:238: if (media_type_capabilities->empty()) { Move up under 4. https://codereview.chromium.org/1005903003/diff/230001/media/blink/webencrypt... media/blink/webencryptedmediaclient_impl.cc:243: return !media_type_capabilities->empty(); Just return true. https://codereview.chromium.org/1005903003/diff/230001/media/blink/webencrypt... media/blink/webencryptedmediaclient_impl.cc:452: << "distinctiveIdentifier was both not allowed and " nit: I think the logic above is in the opposite order. Please change one of them.
https://codereview.chromium.org/1005903003/diff/190001/media/blink/webencrypt... File media/blink/webencryptedmediaclient_impl.cc (right): https://codereview.chromium.org/1005903003/diff/190001/media/blink/webencrypt... media/blink/webencryptedmediaclient_impl.cc:454: config_state.IsRuleSupportedWithCurrentState(not_allowed_rule); On 2015/03/19 22:11:57, ddorwin wrote: > On 2015/03/19 20:05:09, sandersd wrote: > > On 2015/03/19 18:08:35, ddorwin wrote: > > > 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? > > > > 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. > > Should we add a comment pointing this out, either here or somewhere above after > the "point of no return?" Done. https://codereview.chromium.org/1005903003/diff/190001/media/blink/webencrypt... media/blink/webencryptedmediaclient_impl.cc:479: return CONFIGURATION_REQUIRES_PERMISSION; On 2015/03/19 22:11:57, ddorwin wrote: > On 2015/03/19 20:05:10, sandersd wrote: > > On 2015/03/19 18:08:35, ddorwin wrote: > > > 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? > > > > 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. > > Okay. Perhaps note that this is handled by the caller here. Done. https://codereview.chromium.org/1005903003/diff/230001/media/base/key_systems.cc File media/base/key_systems.cc (right): https://codereview.chromium.org/1005903003/diff/230001/media/base/key_systems... media/base/key_systems.cc:698: (max_robustness == EmeRobustness::SW_SECURE_DECODE && On 2015/03/19 22:11:57, ddorwin wrote: > Isn't this covered by line 700? It is, but explaining that took more way more words than just having the redundant test. https://codereview.chromium.org/1005903003/diff/230001/media/blink/webencrypt... File media/blink/webencryptedmediaclient_impl.cc (right): https://codereview.chromium.org/1005903003/diff/230001/media/blink/webencrypt... media/blink/webencryptedmediaclient_impl.cc:238: if (media_type_capabilities->empty()) { On 2015/03/19 22:11:57, ddorwin wrote: > Move up under 4. Done. https://codereview.chromium.org/1005903003/diff/230001/media/blink/webencrypt... media/blink/webencryptedmediaclient_impl.cc:243: return !media_type_capabilities->empty(); On 2015/03/19 22:11:57, ddorwin wrote: > Just return true. Done. https://codereview.chromium.org/1005903003/diff/230001/media/blink/webencrypt... media/blink/webencryptedmediaclient_impl.cc:452: << "distinctiveIdentifier was both not allowed and " On 2015/03/19 22:11:57, ddorwin wrote: > nit: I think the logic above is in the opposite order. Please change one of > them. Done.
lgtm Thanks!
sandersd@chromium.org changed reviewers: + lcwu@chromium.org
lcwu@chromium.org: Please review changes in chromecast/renderer/key_systems_cast.cc
Patchset #10 (id:270001) has been deleted
https://codereview.chromium.org/1005903003/diff/290001/chromecast/renderer/ke... File chromecast/renderer/key_systems_cast.cc (right): https://codereview.chromium.org/1005903003/diff/290001/chromecast/renderer/ke... chromecast/renderer/key_systems_cast.cc:29: info.max_video_robustness = ::media::EmeRobustness::EMPTY; From the spec, it appears that the robustness level is key system dependent. However, the current robustness level enum you propose/use appears to be Widevine specific. How do you envision other key systems (say PlayReady) use this enum?
lcwu@chromium.org changed reviewers: + gunsch@chromium.org
Substitute gunsch@ for me as Chromecast reviewer as he can provide more in-depth discussions at this time.
https://codereview.chromium.org/1005903003/diff/290001/chromecast/renderer/ke... File chromecast/renderer/key_systems_cast.cc (right): https://codereview.chromium.org/1005903003/diff/290001/chromecast/renderer/ke... chromecast/renderer/key_systems_cast.cc:29: info.max_video_robustness = ::media::EmeRobustness::EMPTY; On 2015/03/23 23:23:54, lcwu1 wrote: > From the spec, it appears that the robustness level is key system dependent. > However, the current robustness level enum you propose/use appears to be > Widevine specific. How do you envision other key systems (say PlayReady) use > this enum? I envision that other values would be added to the enum and that the comparison method would be updated to understand them.
https://codereview.chromium.org/1005903003/diff/290001/media/base/key_systems.cc File media/base/key_systems.cc (right): https://codereview.chromium.org/1005903003/diff/290001/media/base/key_systems... media/base/key_systems.cc:695: // robustness requirement is not supported. This seems awkward---isn't there an implicit assumption that EmeRobustness is ordered? As I read the CL, it seems potentially more appropriate to separate the concept of "decode robustness" and "crypto robustness".
https://codereview.chromium.org/1005903003/diff/290001/media/base/eme_constan... File media/base/eme_constants.h (right): https://codereview.chromium.org/1005903003/diff/290001/media/base/eme_constan... media/base/eme_constants.h:103: Just chatted with ddorwin@ off-line. I felt that this enum should be in Widevine-specific code instead of here in eme_constants.h. (David provide some rationals for doing it this way. I will let him respond with his answers.)
https://codereview.chromium.org/1005903003/diff/290001/media/base/key_systems.cc File media/base/key_systems.cc (right): https://codereview.chromium.org/1005903003/diff/290001/media/base/key_systems... media/base/key_systems.cc:695: // robustness requirement is not supported. On 2015/03/24 00:12:00, gunsch wrote: > This seems awkward---isn't there an implicit assumption that EmeRobustness is > ordered? As I read the CL, it seems potentially more appropriate to separate the > concept of "decode robustness" and "crypto robustness". There is no such implicit assumption, and that assumption would already be invalid. The reality is that robustness values are a partially ordered set, and we need an implementation of less-than for it. This comment is about the specific implementation of less-than for the particular list of values currently supported. Treating the values as ordered in some cases is a shortcut that works when only considering Widevine values. I expect that if other values are added, there will be a separate check to ensure that the requested robustness is valid for the key system.
https://codereview.chromium.org/1005903003/diff/290001/media/base/eme_constan... File media/base/eme_constants.h (right): https://codereview.chromium.org/1005903003/diff/290001/media/base/eme_constan... media/base/eme_constants.h:103: On 2015/03/24 00:19:49, lcwu1 wrote: > Just chatted with ddorwin@ off-line. I felt that this enum should be in > Widevine-specific code instead of here in eme_constants.h. (David provide some > rationals for doing it this way. I will let him respond with his answers.) I was just saying that we would have to pass strings around and convert them repeatedly, whereas today the conversion happens once at the higher levels. It's possible things will change when KeySystems is refactored (issue 457438). It's also possible that other systems would map onto a common set of levels.
https://codereview.chromium.org/1005903003/diff/290001/media/base/eme_constan... File media/base/eme_constants.h (right): https://codereview.chromium.org/1005903003/diff/290001/media/base/eme_constan... media/base/eme_constants.h:103: On 2015/03/24 00:24:37, ddorwin wrote: > On 2015/03/24 00:19:49, lcwu1 wrote: > > Just chatted with ddorwin@ off-line. I felt that this enum should be in > > Widevine-specific code instead of here in eme_constants.h. (David provide some > > rationals for doing it this way. I will let him respond with his answers.) > > I was just saying that we would have to pass strings around and convert them > repeatedly, whereas today the conversion happens once at the higher levels. It's > possible things will change when KeySystems is refactored (issue 457438). It's > also possible that other systems would map onto a common set of levels. If refactoring does not address this, we probably need to move ConvertRobustness() into Key Systems so that we don't have key system-specific strings in webencryptedmediaclient_impl.cc. For now, this CL is a step forward in spec compliance in Chrome. Support for other robustness level-supporting key systems may require additional changes.
https://codereview.chromium.org/1005903003/diff/290001/media/blink/webencrypt... File media/blink/webencryptedmediaclient_impl.cc (right): https://codereview.chromium.org/1005903003/diff/290001/media/blink/webencrypt... media/blink/webencryptedmediaclient_impl.cc:218: key_system, media_type, ConvertRobustness(capability.robustness)); On second thought, regardless of how we define or use the EmeRobustness enum, we should not do key system-specific string processing (ConvertRobustness()) here. We only use capability.robustness here. Let's convert it to ASCII (as in the step above) and pass it as a string to GetRobustnessConfigRule(). This allows us to move ConvertRobustness() to be internal to KeySystems. From there, we can refactor as necessary in future CLs.
https://codereview.chromium.org/1005903003/diff/290001/media/blink/webencrypt... File media/blink/webencryptedmediaclient_impl.cc (right): https://codereview.chromium.org/1005903003/diff/290001/media/blink/webencrypt... media/blink/webencryptedmediaclient_impl.cc:218: key_system, media_type, ConvertRobustness(capability.robustness)); On 2015/03/24 18:11:37, ddorwin wrote: > On second thought, regardless of how we define or use the EmeRobustness enum, we > should not do key system-specific string processing (ConvertRobustness()) here. > > We only use capability.robustness here. Let's convert it to ASCII (as in the > step above) and pass it as a string to GetRobustnessConfigRule(). This allows us > to move ConvertRobustness() to be internal to KeySystems. > > From there, we can refactor as necessary in future CLs. Done.
Patchset uploaded, sorry!
lgtm % nit https://codereview.chromium.org/1005903003/diff/310001/media/base/key_systems.cc File media/base/key_systems.cc (right): https://codereview.chromium.org/1005903003/diff/310001/media/base/key_systems... media/base/key_systems.cc:687: EmeRobustness robustness_value = ConvertRobustness(robustness); nit: Since you need to make this longer anyway, perhaps requested_robustness would be clearer. I always thought "robustness" was ambiguous anyway.
https://codereview.chromium.org/1005903003/diff/310001/media/base/key_systems.cc File media/base/key_systems.cc (right): https://codereview.chromium.org/1005903003/diff/310001/media/base/key_systems... media/base/key_systems.cc:687: EmeRobustness robustness_value = ConvertRobustness(robustness); On 2015/03/24 20:00:09, ddorwin wrote: > nit: Since you need to make this longer anyway, perhaps requested_robustness > would be clearer. I always thought "robustness" was ambiguous anyway. Done.
chromecast lgtm It still seems a little too tied to Widevine specifically but sounds like that's KI for now---I recognize there isn't a good way to do much in the way of key-system-specific behavior in the current structure.
The CQ bit was checked by sandersd@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ddorwin@chromium.org Link to the patchset: https://codereview.chromium.org/1005903003/#ps330001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1005903003/330001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by sandersd@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ddorwin@chromium.org, gunsch@chromium.org Link to the patchset: https://codereview.chromium.org/1005903003/#ps350001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1005903003/350001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)
The CQ bit was checked by sandersd@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ddorwin@chromium.org, gunsch@chromium.org Link to the patchset: https://codereview.chromium.org/1005903003/#ps370001 (title: "Android build.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1005903003/370001
The CQ bit was checked by sandersd@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ddorwin@chromium.org, gunsch@chromium.org Link to the patchset: https://codereview.chromium.org/1005903003/#ps390001 (title: "Register ClearKey correctly.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1005903003/390001
The CQ bit was unchecked by sandersd@chromium.org
Patchset #15 (id:390001) has been deleted
Patchset #14 (id:370001) has been deleted
The CQ bit was checked by sandersd@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ddorwin@chromium.org, gunsch@chromium.org Link to the patchset: https://codereview.chromium.org/1005903003/#ps410001 (title: " ")
The CQ bit was unchecked by sandersd@chromium.org
The CQ bit was checked by sandersd@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1005903003/410001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by sandersd@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1005903003/410001
The CQ bit was checked by sandersd@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ddorwin@chromium.org, gunsch@chromium.org Link to the patchset: https://codereview.chromium.org/1005903003/#ps430001 (title: "Fix LayoutTests.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1005903003/430001
The CQ bit was unchecked by sandersd@chromium.org
Patchset #15 (id:430001) has been deleted
The CQ bit was checked by sandersd@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ddorwin@chromium.org, gunsch@chromium.org Link to the patchset: https://codereview.chromium.org/1005903003/#ps450001 (title: "Fix layout tests.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1005903003/450001
Message was sent while issue was closed.
Committed patchset #15 (id:450001)
Message was sent while issue was closed.
Patchset 15 (id:??) landed as https://crrev.com/a8a9143d9373e1ae47f30d3da14c051d16188b3e Cr-Commit-Position: refs/heads/master@{#322219} |
