Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
557723003:
Implement Chromium side of MediaKeys.isTypeSupported(). (Closed)
|
Created:
6 years, 3 months ago by sandersd (OOO until July 31) Modified:
6 years, 2 months ago CC:
chromium-reviews, eme-reviews_chromium.org, posciak+watch_chromium.org, jam, mcasas+watch_chromium.org, feature-media-reviews_chromium.org, darin-cc_chromium.org, wjia+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionImplement Chromium side of MediaKeys.isTypeSupported().
BUG=405731
Committed: https://crrev.com/3573622da943ccb017e1dd93791a7efad5a3bcc8
Cr-Commit-Position: refs/heads/master@{#297550}
Patch Set 1 #Patch Set 2 : Add DEP from components/cdm/renderer to content/public/common. #
Total comments: 75
Patch Set 3 : Requested changes. #
Total comments: 83
Patch Set 4 : More requested changes. #
Total comments: 24
Patch Set 5 : Small fixes. #
Total comments: 2
Patch Set 6 : Comment #
Total comments: 2
Patch Set 7 : Fix #endif comment. #
Total comments: 2
Patch Set 8 : Spelling #Patch Set 9 : Remove Blink interfaces. #Patch Set 10 : Fix key_systems_unittest. #Patch Set 11 : Work around performance warning. #Messages
Total messages: 41 (11 generated)
sandersd@chromium.org changed reviewers: + ddorwin@chromium.org, jrummell@chromium.org
Test and components DEPS are being refactored in separate CLs.
Thanks. The approach LG. Mostly minor issues and nits. https://codereview.chromium.org/557723003/diff/20001/chrome/renderer/media/ch... File chrome/renderer/media/chrome_key_systems.cc (right): https://codereview.chromium.org/557723003/diff/20001/chrome/renderer/media/ch... chrome/renderer/media/chrome_key_systems.cc:17: #include "content/public/common/eme_init_data_type.h" Does it make sense to unify these? eme_constants.h? https://codereview.chromium.org/557723003/diff/20001/chrome/renderer/media/ch... chrome/renderer/media/chrome_key_systems.cc:83: info.supported_init_data_types = content::EME_INIT_DATA_TYPE_ALL; We don't currently support keyids. How should we handle that? https://codereview.chromium.org/557723003/diff/20001/components/cdm/renderer/... File components/cdm/renderer/android_key_systems.cc (right): https://codereview.chromium.org/557723003/diff/20001/components/cdm/renderer/... components/cdm/renderer/android_key_systems.cc:71: // not really trusted to implement them in a spec-compliant way anyway. It's the key systems we're concerned about in this respect. I think you could add CENC | WEBM, and let the supported containers (represented by codecs???) determine which is actually exposed (by the logic that ensures only logical combinations are reported as supported). The current code would prevent anything beyond the key system name from being checked. https://codereview.chromium.org/557723003/diff/20001/components/cdm/renderer/... File components/cdm/renderer/widevine_key_systems.cc (right): https://codereview.chromium.org/557723003/diff/20001/components/cdm/renderer/... components/cdm/renderer/widevine_key_systems.cc:11: // TODO(sandersd): Violates DEPS. Didn't you fix this? https://codereview.chromium.org/557723003/diff/20001/components/cdm/renderer/... components/cdm/renderer/widevine_key_systems.cc:55: info.supported_init_data_types = (content::EME_INIT_DATA_TYPE_CENC | Note that the general logic will filter out unsupported types based on container. (Hopefully you can phrase that better. :) ) https://codereview.chromium.org/557723003/diff/20001/content/public/common/em... File content/public/common/eme_init_data_type.h (right): https://codereview.chromium.org/557723003/diff/20001/content/public/common/em... content/public/common/eme_init_data_type.h:15: EME_INIT_DATA_TYPE_CENC = 1 << 0, We should probably not define CENC if proprietary codecs are not supported. That avoids needing to comment on assumptions in various other places. https://codereview.chromium.org/557723003/diff/20001/content/renderer/media/c... File content/renderer/media/crypto/key_systems.cc (right): https://codereview.chromium.org/557723003/diff/20001/content/renderer/media/c... content/renderer/media/crypto/key_systems.cc:39: struct InitDataTypeMask { nit: move IDT stuff before container and codec to match ITS order. https://codereview.chromium.org/557723003/diff/20001/content/renderer/media/c... content/renderer/media/crypto/key_systems.cc:41: EmeInitDataType mask; This seems like more of a type string to enum map. I see that CodecMask is named this way, though. :( Maybe we can replace "mask" with "enum" throughout. https://codereview.chromium.org/557723003/diff/20001/content/renderer/media/c... content/renderer/media/crypto/key_systems.cc:70: // Mapping between initialization data types and their masks. ditto on moving https://codereview.chromium.org/557723003/diff/20001/content/renderer/media/c... content/renderer/media/crypto/key_systems.cc:96: info.supported_init_data_types = EME_INIT_DATA_TYPE_ALL; ditto on moving (also in other files) https://codereview.chromium.org/557723003/diff/20001/content/renderer/media/c... content/renderer/media/crypto/key_systems.cc:144: SupportedInitDataTypes supported_init_data_types, nit: move up one line to match ITS order. https://codereview.chromium.org/557723003/diff/20001/content/renderer/media/c... content/renderer/media/crypto/key_systems.cc:158: SupportedInitDataTypes supported_init_data_types; ditto https://codereview.chromium.org/557723003/diff/20001/content/renderer/media/c... content/renderer/media/crypto/key_systems.cc:165: typedef base::hash_map<std::string, EmeInitDataType> InitDataTypeMaskMap; ditto https://codereview.chromium.org/557723003/diff/20001/content/renderer/media/c... content/renderer/media/crypto/key_systems.cc:194: InitDataTypeMaskMap init_data_type_masks_; ditto https://codereview.chromium.org/557723003/diff/20001/content/renderer/media/c... content/renderer/media/crypto/key_systems.cc:217: #define BUILD_MASK(dst, src) \ Macros aren't allowed. Would a template function work? https://codereview.chromium.org/557723003/diff/20001/content/renderer/media/c... content/renderer/media/crypto/key_systems.cc:334: return concrete_key_system_map_.count(key_system); Is there a reason you made this change? Not that it matters in practice, but his requires iterating through the entire list even after one is found. https://codereview.chromium.org/557723003/diff/20001/content/renderer/media/c... content/renderer/media/crypto/key_systems.cc:398: return (parent_key_system_map_.count(key_system) || ditto. https://codereview.chromium.org/557723003/diff/20001/content/renderer/media/c... content/renderer/media/crypto/key_systems.cc:399: concrete_key_system_map_.count(key_system)); minor: We're more likely to see concrete key system queries, so check that one first. https://codereview.chromium.org/557723003/diff/20001/content/renderer/media/c... content/renderer/media/crypto/key_systems.cc:438: const std::string& key_system) { note: This ordering is odd (probably to match CPT). we should fix it sometime. We'd also re-order the checks to match the function above. https://codereview.chromium.org/557723003/diff/20001/content/renderer/media/c... content/renderer/media/crypto/key_systems.cc:441: // If |key_system| is a parent key_system, use its concrete child. Please remove second '_' https://codereview.chromium.org/557723003/diff/20001/content/renderer/media/c... content/renderer/media/crypto/key_systems.cc:442: // Otherwise, use |key_system|. 442-449 are different from 408-412. The code should at least be consistent. Better yet, maybe it makes sense to have a private function called GetConcreteKeySystem(). That replaces all of the comments and code with one line in each function. https://codereview.chromium.org/557723003/diff/20001/content/renderer/media/w... File content/renderer/media/webcontentdecryptionmodule_impl.cc (right): https://codereview.chromium.org/557723003/diff/20001/content/renderer/media/w... content/renderer/media/webcontentdecryptionmodule_impl.cc:95: // 15-18. It might help to say something like: // Continue the MediaKeys.isTypeSupported() algorithm. <empty line> https://codereview.chromium.org/557723003/diff/20001/content/renderer/media/w... content/renderer/media/webcontentdecryptionmodule_impl.cc:96: if (!capability.isEmpty()) I believe we need the following: // Not currently supported. // TODO: Implement support. https://codereview.chromium.org/557723003/diff/20001/content/renderer/media/w... content/renderer/media/webcontentdecryptionmodule_impl.cc:97: return blink::WebContentDecryptionModule::IsNotSupported; Should we move this to the end where it will be eventually? There's no reason for it to be here, right? https://codereview.chromium.org/557723003/diff/20001/content/renderer/media/w... content/renderer/media/webcontentdecryptionmodule_impl.cc:99: // 1. If keySystem is an empty string or contains an unrecognized or Half of step 1 is handled in Blink. The other half is really handled by step 2. https://codereview.chromium.org/557723003/diff/20001/content/renderer/media/w... content/renderer/media/webcontentdecryptionmodule_impl.cc:104: const std::string key_system_ascii = key_system.utf8(); Are we checking and rejecting non-ascii values in Blink? That was the plan, but I'm not sure we've implemented that yet. See [1]. If not, I'm not sure whether this is safe. This might be sufficiently high enough to do the conversion (rather than in Blink), but we should return the empty string if any of the parameters are not ASCII. See [2] for an example. On the other hand, the ASCII check fits better in step 1, which is mostly in Blink. [1] https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m... [2] https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m... https://codereview.chromium.org/557723003/diff/20001/content/renderer/media/w... content/renderer/media/webcontentdecryptionmodule_impl.cc:117: if (IsConcreteSupportedKeySystem(key_system_ascii)) nit: Convert to ? :. https://codereview.chromium.org/557723003/diff/20001/content/renderer/media/w... content/renderer/media/webcontentdecryptionmodule_impl.cc:143: if ((key_system_ascii == "mp4" && init_data_type_ascii == "webm") || This should be comparing the container. https://codereview.chromium.org/557723003/diff/20001/content/renderer/media/w... content/renderer/media/webcontentdecryptionmodule_impl.cc:144: (key_system_ascii == "webm" && init_data_type_ascii == "cenc")) Let's move this code to key_systems.cc into a function like VerifyIsSaneCombination(). Also, I would structure these as: if (container-specific-IDT) if (!matching-container) return false; The location allows us to have a single place to track these issues and update it if a new container or IDT is added. The helper function allows us to structure code as above. The name makes it clear what we're doing. https://codereview.chromium.org/557723003/diff/20001/content/renderer/media/w... content/renderer/media/webcontentdecryptionmodule_impl.cc:157: // 9. If contentType is an empty string or contains an invalid or As noted in the Blink CL, these should be there, except maybe the "unrecognized MIME type" part of 9 - but we can just assume it's handled in step 13. https://codereview.chromium.org/557723003/diff/20001/content/renderer/media/w... content/renderer/media/webcontentdecryptionmodule_impl.cc:172: bool strip_codec_suffixes = !net::IsStrictMediaMimeType(container_ascii); TODO: Do a better job validating codecs: crbug.com/374751 https://codereview.chromium.org/557723003/diff/20001/content/renderer/media/w... content/renderer/media/webcontentdecryptionmodule_impl.cc:174: net::ParseCodecString(codecs.utf8(), &codec_vector, strip_codec_suffixes); note: need to ensure we have checked ASCII before here https://codereview.chromium.org/557723003/diff/20001/content/renderer/media/w... content/renderer/media/webcontentdecryptionmodule_impl.cc:180: // IsSupported. probably_result, right? https://codereview.chromium.org/557723003/diff/20001/content/renderer/media/w... File content/renderer/media/webcontentdecryptionmodule_impl.h (right): https://codereview.chromium.org/557723003/diff/20001/content/renderer/media/w... content/renderer/media/webcontentdecryptionmodule_impl.h:61: static blink::WebContentDecryptionModule::SupportsKeySystem Are statics supposed to be grouped?
A couple more comments for discussion. https://codereview.chromium.org/557723003/diff/20001/content/renderer/media/c... File content/renderer/media/crypto/key_systems.cc (right): https://codereview.chromium.org/557723003/diff/20001/content/renderer/media/c... content/renderer/media/crypto/key_systems.cc:48: {"audio/webm", EME_CODEC_WEBM_AUDIO_ALL}, I wonder if we should just convert these to conversion functions with switch statements. This might be a bit simpler to read, but then we have to build maps and find() in them when we could just call the function. WDYT? https://codereview.chromium.org/557723003/diff/20001/content/renderer/media/c... content/renderer/media/crypto/key_systems.cc:72: {"cenc", EME_INIT_DATA_TYPE_CENC}, This is a straight conversion and never depends on the key system. Should we just do the conversion from JS string to usable value in WebCDMImpl.cc?
https://codereview.chromium.org/557723003/diff/20001/chrome/renderer/media/ch... File chrome/renderer/media/chrome_key_systems.cc (right): https://codereview.chromium.org/557723003/diff/20001/chrome/renderer/media/ch... chrome/renderer/media/chrome_key_systems.cc:17: #include "content/public/common/eme_init_data_type.h" On 2014/09/13 01:20:44, ddorwin wrote: > Does it make sense to unify these? eme_constants.h? Done. https://codereview.chromium.org/557723003/diff/20001/chrome/renderer/media/ch... chrome/renderer/media/chrome_key_systems.cc:83: info.supported_init_data_types = content::EME_INIT_DATA_TYPE_ALL; On 2014/09/13 01:20:44, ddorwin wrote: > We don't currently support keyids. How should we handle that? Done. https://codereview.chromium.org/557723003/diff/20001/components/cdm/renderer/... File components/cdm/renderer/android_key_systems.cc (right): https://codereview.chromium.org/557723003/diff/20001/components/cdm/renderer/... components/cdm/renderer/android_key_systems.cc:71: // not really trusted to implement them in a spec-compliant way anyway. On 2014/09/13 01:20:44, ddorwin wrote: > It's the key systems we're concerned about in this respect. > > I think you could add CENC | WEBM, and let the supported containers (represented > by codecs???) determine which is actually exposed (by the logic that ensures > only logical combinations are reported as supported). > > The current code would prevent anything beyond the key system name from being > checked. Done. https://codereview.chromium.org/557723003/diff/20001/components/cdm/renderer/... File components/cdm/renderer/widevine_key_systems.cc (right): https://codereview.chromium.org/557723003/diff/20001/components/cdm/renderer/... components/cdm/renderer/widevine_key_systems.cc:11: // TODO(sandersd): Violates DEPS. On 2014/09/13 01:20:44, ddorwin wrote: > Didn't you fix this? Done. https://codereview.chromium.org/557723003/diff/20001/components/cdm/renderer/... components/cdm/renderer/widevine_key_systems.cc:55: info.supported_init_data_types = (content::EME_INIT_DATA_TYPE_CENC | On 2014/09/13 01:20:44, ddorwin wrote: > Note that the general logic will filter out unsupported types based on > container. (Hopefully you can phrase that better. :) ) Done. https://codereview.chromium.org/557723003/diff/20001/content/public/common/em... File content/public/common/eme_init_data_type.h (right): https://codereview.chromium.org/557723003/diff/20001/content/public/common/em... content/public/common/eme_init_data_type.h:15: EME_INIT_DATA_TYPE_CENC = 1 << 0, On 2014/09/13 01:20:45, ddorwin wrote: > We should probably not define CENC if proprietary codecs are not supported. That > avoids needing to comment on assumptions in various other places. Done. https://codereview.chromium.org/557723003/diff/20001/content/renderer/media/c... File content/renderer/media/crypto/key_systems.cc (right): https://codereview.chromium.org/557723003/diff/20001/content/renderer/media/c... content/renderer/media/crypto/key_systems.cc:39: struct InitDataTypeMask { On 2014/09/13 01:20:45, ddorwin wrote: > nit: move IDT stuff before container and codec to match ITS order. Done. https://codereview.chromium.org/557723003/diff/20001/content/renderer/media/c... content/renderer/media/crypto/key_systems.cc:41: EmeInitDataType mask; On 2014/09/13 01:20:45, ddorwin wrote: > This seems like more of a type string to enum map. > > I see that CodecMask is named this way, though. :( > Maybe we can replace "mask" with "enum" throughout. I had a go at renaming these, hopefully it's better. https://codereview.chromium.org/557723003/diff/20001/content/renderer/media/c... content/renderer/media/crypto/key_systems.cc:48: {"audio/webm", EME_CODEC_WEBM_AUDIO_ALL}, On 2014/09/13 03:51:05, ddorwin wrote: > I wonder if we should just convert these to conversion functions with switch > statements. This might be a bit simpler to read, but then we have to build maps > and find() in them when we could just call the function. WDYT? They are mutable so that unit tests can inject new entries, but accessor methods do look better to me. https://codereview.chromium.org/557723003/diff/20001/content/renderer/media/c... content/renderer/media/crypto/key_systems.cc:70: // Mapping between initialization data types and their masks. On 2014/09/13 01:20:45, ddorwin wrote: > ditto on moving Done. https://codereview.chromium.org/557723003/diff/20001/content/renderer/media/c... content/renderer/media/crypto/key_systems.cc:72: {"cenc", EME_INIT_DATA_TYPE_CENC}, On 2014/09/13 03:51:05, ddorwin wrote: > This is a straight conversion and never depends on the key system. Should we > just do the conversion from JS string to usable value in WebCDMImpl.cc? We can, but then blink needs to know what the supported values are (and the enum needs to move there), which seems wrong. https://codereview.chromium.org/557723003/diff/20001/content/renderer/media/c... content/renderer/media/crypto/key_systems.cc:96: info.supported_init_data_types = EME_INIT_DATA_TYPE_ALL; On 2014/09/13 01:20:46, ddorwin wrote: > ditto on moving (also in other files) Done. https://codereview.chromium.org/557723003/diff/20001/content/renderer/media/c... content/renderer/media/crypto/key_systems.cc:144: SupportedInitDataTypes supported_init_data_types, On 2014/09/13 01:20:45, ddorwin wrote: > nit: move up one line to match ITS order. Done. https://codereview.chromium.org/557723003/diff/20001/content/renderer/media/c... content/renderer/media/crypto/key_systems.cc:158: SupportedInitDataTypes supported_init_data_types; On 2014/09/13 01:20:45, ddorwin wrote: > ditto Done. https://codereview.chromium.org/557723003/diff/20001/content/renderer/media/c... content/renderer/media/crypto/key_systems.cc:165: typedef base::hash_map<std::string, EmeInitDataType> InitDataTypeMaskMap; On 2014/09/13 01:20:46, ddorwin wrote: > ditto Done. https://codereview.chromium.org/557723003/diff/20001/content/renderer/media/c... content/renderer/media/crypto/key_systems.cc:194: InitDataTypeMaskMap init_data_type_masks_; On 2014/09/13 01:20:45, ddorwin wrote: > ditto Done. https://codereview.chromium.org/557723003/diff/20001/content/renderer/media/c... content/renderer/media/crypto/key_systems.cc:217: #define BUILD_MASK(dst, src) \ On 2014/09/13 01:20:45, ddorwin wrote: > Macros aren't allowed. Would a template function work? I've expanded the macro, but I do think the macro version is harder to typo. https://codereview.chromium.org/557723003/diff/20001/content/renderer/media/c... content/renderer/media/crypto/key_systems.cc:334: return concrete_key_system_map_.count(key_system); On 2014/09/13 01:20:45, ddorwin wrote: > Is there a reason you made this change? Not that it matters in practice, but his > requires iterating through the entire list even after one is found. It's a hash_map. https://codereview.chromium.org/557723003/diff/20001/content/renderer/media/c... content/renderer/media/crypto/key_systems.cc:398: return (parent_key_system_map_.count(key_system) || On 2014/09/13 01:20:45, ddorwin wrote: > ditto. Acknowledged. https://codereview.chromium.org/557723003/diff/20001/content/renderer/media/c... content/renderer/media/crypto/key_systems.cc:399: concrete_key_system_map_.count(key_system)); On 2014/09/13 01:20:45, ddorwin wrote: > minor: We're more likely to see concrete key system queries, so check that one > first. Done. https://codereview.chromium.org/557723003/diff/20001/content/renderer/media/c... content/renderer/media/crypto/key_systems.cc:438: const std::string& key_system) { On 2014/09/13 01:20:45, ddorwin wrote: > note: This ordering is odd (probably to match CPT). we should fix it sometime. > We'd also re-order the checks to match the function above. I agree, and some of the Is____ methods are probably entirely obsolete now too (especially this one). https://codereview.chromium.org/557723003/diff/20001/content/renderer/media/c... content/renderer/media/crypto/key_systems.cc:441: // If |key_system| is a parent key_system, use its concrete child. On 2014/09/13 01:20:45, ddorwin wrote: > Please remove second '_' Done. https://codereview.chromium.org/557723003/diff/20001/content/renderer/media/c... content/renderer/media/crypto/key_systems.cc:442: // Otherwise, use |key_system|. On 2014/09/13 01:20:45, ddorwin wrote: > 442-449 are different from 408-412. The code should at least be consistent. > Better yet, maybe it makes sense to have a private function called > GetConcreteKeySystem(). That replaces all of the comments and code with one line > in each function. Done. https://codereview.chromium.org/557723003/diff/20001/content/renderer/media/w... File content/renderer/media/webcontentdecryptionmodule_impl.cc (right): https://codereview.chromium.org/557723003/diff/20001/content/renderer/media/w... content/renderer/media/webcontentdecryptionmodule_impl.cc:95: // 15-18. On 2014/09/13 01:20:46, ddorwin wrote: > It might help to say something like: > // Continue the MediaKeys.isTypeSupported() algorithm. > <empty line> Done. https://codereview.chromium.org/557723003/diff/20001/content/renderer/media/w... content/renderer/media/webcontentdecryptionmodule_impl.cc:96: if (!capability.isEmpty()) On 2014/09/13 01:20:46, ddorwin wrote: > I believe we need the following: > // Not currently supported. > // TODO: Implement support. Done. https://codereview.chromium.org/557723003/diff/20001/content/renderer/media/w... content/renderer/media/webcontentdecryptionmodule_impl.cc:97: return blink::WebContentDecryptionModule::IsNotSupported; On 2014/09/13 01:20:46, ddorwin wrote: > Should we move this to the end where it will be eventually? There's no reason > for it to be here, right? The only reason was that an early exit seemed like less of a waste. (Done.) https://codereview.chromium.org/557723003/diff/20001/content/renderer/media/w... content/renderer/media/webcontentdecryptionmodule_impl.cc:99: // 1. If keySystem is an empty string or contains an unrecognized or On 2014/09/13 01:20:46, ddorwin wrote: > Half of step 1 is handled in Blink. The other half is really handled by step 2. Done. https://codereview.chromium.org/557723003/diff/20001/content/renderer/media/w... content/renderer/media/webcontentdecryptionmodule_impl.cc:104: const std::string key_system_ascii = key_system.utf8(); On 2014/09/13 01:20:46, ddorwin wrote: > Are we checking and rejecting non-ascii values in Blink? That was the plan, but > I'm not sure we've implemented that yet. See [1]. If not, I'm not sure whether > this is safe. This might be sufficiently high enough to do the conversion > (rather than in Blink), but we should return the empty string if any of the > parameters are not ASCII. See [2] for an example. > > On the other hand, the ASCII check fits better in step 1, which is mostly in > Blink. > > [1] > https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m... > > [2] > https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m... Because the spec didn't say to do that check, this CL avoids the problem by re-coding to UTF-8. It's not a great solution, but it's efficient if the strings are ASCII and I believe correct if they are not (the lookups just won't find anything). It makes sense to do an ASCII check in Blink, and then just assume it here. I've added a TODO for now. https://codereview.chromium.org/557723003/diff/20001/content/renderer/media/w... content/renderer/media/webcontentdecryptionmodule_impl.cc:117: if (IsConcreteSupportedKeySystem(key_system_ascii)) On 2014/09/13 01:20:46, ddorwin wrote: > nit: Convert to ? :. Done. https://codereview.chromium.org/557723003/diff/20001/content/renderer/media/w... content/renderer/media/webcontentdecryptionmodule_impl.cc:143: if ((key_system_ascii == "mp4" && init_data_type_ascii == "webm") || On 2014/09/13 01:20:46, ddorwin wrote: > This should be comparing the container. Whoops, thanks! https://codereview.chromium.org/557723003/diff/20001/content/renderer/media/w... content/renderer/media/webcontentdecryptionmodule_impl.cc:144: (key_system_ascii == "webm" && init_data_type_ascii == "cenc")) On 2014/09/13 01:20:46, ddorwin wrote: > Let's move this code to key_systems.cc into a function like > VerifyIsSaneCombination(). > > Also, I would structure these as: > if (container-specific-IDT) > if (!matching-container) > return false; > > The location allows us to have a single place to track these issues and update > it if a new container or IDT is added. The helper function allows us to > structure code as above. The name makes it clear what we're doing. Done. https://codereview.chromium.org/557723003/diff/20001/content/renderer/media/w... content/renderer/media/webcontentdecryptionmodule_impl.cc:157: // 9. If contentType is an empty string or contains an invalid or On 2014/09/13 01:20:46, ddorwin wrote: > As noted in the Blink CL, these should be there, except maybe the "unrecognized > MIME type" part of 9 - but we can just assume it's handled in step 13. Done. https://codereview.chromium.org/557723003/diff/20001/content/renderer/media/w... content/renderer/media/webcontentdecryptionmodule_impl.cc:172: bool strip_codec_suffixes = !net::IsStrictMediaMimeType(container_ascii); On 2014/09/13 01:20:46, ddorwin wrote: > TODO: Do a better job validating codecs: crbug.com/374751 Done. https://codereview.chromium.org/557723003/diff/20001/content/renderer/media/w... content/renderer/media/webcontentdecryptionmodule_impl.cc:174: net::ParseCodecString(codecs.utf8(), &codec_vector, strip_codec_suffixes); On 2014/09/13 01:20:46, ddorwin wrote: > note: need to ensure we have checked ASCII before here Acknowledged. https://codereview.chromium.org/557723003/diff/20001/content/renderer/media/w... content/renderer/media/webcontentdecryptionmodule_impl.cc:180: // IsSupported. On 2014/09/13 01:20:46, ddorwin wrote: > probably_result, right? That's correct, but because we don't check that the codecs are unambiguous, we can't actually return "probably" yet, so it's always "maybe". https://codereview.chromium.org/557723003/diff/20001/content/renderer/media/w... File content/renderer/media/webcontentdecryptionmodule_impl.h (right): https://codereview.chromium.org/557723003/diff/20001/content/renderer/media/w... content/renderer/media/webcontentdecryptionmodule_impl.h:61: static blink::WebContentDecryptionModule::SupportsKeySystem On 2014/09/13 01:20:47, ddorwin wrote: > Are statics supposed to be grouped? Done.
https://codereview.chromium.org/557723003/diff/20001/content/renderer/media/c... File content/renderer/media/crypto/key_systems.cc (right): https://codereview.chromium.org/557723003/diff/20001/content/renderer/media/c... content/renderer/media/crypto/key_systems.cc:48: {"audio/webm", EME_CODEC_WEBM_AUDIO_ALL}, On 2014/09/22 23:45:53, sandersd wrote: > On 2014/09/13 03:51:05, ddorwin wrote: > > I wonder if we should just convert these to conversion functions with switch > > statements. This might be a bit simpler to read, but then we have to build > maps > > and find() in them when we could just call the function. WDYT? > > They are mutable so that unit tests can inject new entries, but accessor methods > do look better to me. :( Okay, let's keep this in mind. If we enabled container support without software codec support, we could probably use real types in those tests. (There is a bug for that, but nobody is working on it.) https://codereview.chromium.org/557723003/diff/20001/content/renderer/media/c... content/renderer/media/crypto/key_systems.cc:72: {"cenc", EME_INIT_DATA_TYPE_CENC}, On 2014/09/22 23:45:53, sandersd wrote: > On 2014/09/13 03:51:05, ddorwin wrote: > > This is a straight conversion and never depends on the key system. Should we > > just do the conversion from JS string to usable value in WebCDMImpl.cc? > > We can, but then blink needs to know what the supported values are (and the enum > needs to move there), which seems wrong. I think it's probably reasonable for Blink to know the possibly supported values. If one is registered, it can be added to that interface. It's still up to Chromium _whether_ it is supported. https://codereview.chromium.org/557723003/diff/20001/content/renderer/media/w... File content/renderer/media/webcontentdecryptionmodule_impl.cc (right): https://codereview.chromium.org/557723003/diff/20001/content/renderer/media/w... content/renderer/media/webcontentdecryptionmodule_impl.cc:104: const std::string key_system_ascii = key_system.utf8(); On 2014/09/22 23:45:54, sandersd wrote: > On 2014/09/13 01:20:46, ddorwin wrote: > > Are we checking and rejecting non-ascii values in Blink? That was the plan, > but > > I'm not sure we've implemented that yet. See [1]. If not, I'm not sure whether > > this is safe. This might be sufficiently high enough to do the conversion > > (rather than in Blink), but we should return the empty string if any of the > > parameters are not ASCII. See [2] for an example. > > > > On the other hand, the ASCII check fits better in step 1, which is mostly in > > Blink. > > > > [1] > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m... > > > > [2] > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m... > > Because the spec didn't say to do that check, this CL avoids the problem by > re-coding to UTF-8. It's not a great solution, but it's efficient if the strings > are ASCII and I believe correct if they are not (the lookups just won't find > anything). > > It makes sense to do an ASCII check in Blink, and then just assume it here. I've > added a TODO for now. FWIW, there is a note in the spec that lower-case ascii is recommended. https://codereview.chromium.org/557723003/diff/40001/components/cdm/renderer/... File components/cdm/renderer/android_key_systems.cc (right): https://codereview.chromium.org/557723003/diff/40001/components/cdm/renderer/... components/cdm/renderer/android_key_systems.cc:69: // Assume that WebM and CENC are supported by platform CDMs. |KeySystems| I think '|' is usually used for parameters and maybe members. Not classes, AFAIK. https://codereview.chromium.org/557723003/diff/40001/components/cdm/renderer/... components/cdm/renderer/android_key_systems.cc:69: // Assume that WebM and CENC are supported by platform CDMs. |KeySystems| ... CDMs that support their respective containers. KeySystems will remove the type if the MIME type is not supported. ^ Is that correct? Or is it filtered somewhere else? https://codereview.chromium.org/557723003/diff/40001/components/cdm/renderer/... File components/cdm/renderer/widevine_key_systems.cc (right): https://codereview.chromium.org/557723003/diff/40001/components/cdm/renderer/... components/cdm/renderer/widevine_key_systems.cc:53: // |KeySystems| handles validating |init_data_type|/|container| mappings. ditto. https://codereview.chromium.org/557723003/diff/40001/components/cdm/renderer/... components/cdm/renderer/widevine_key_systems.cc:53: // |KeySystems| handles validating |init_data_type|/|container| mappings. // Support for a container implies support for the Initialization Data Type. https://codereview.chromium.org/557723003/diff/40001/components/cdm/renderer/... components/cdm/renderer/widevine_key_systems.cc:54: info.supported_init_data_types = content::EME_INIT_DATA_TYPE_NONE; The constructor should handle this. https://codereview.chromium.org/557723003/diff/40001/content/public/common/em... File content/public/common/eme_constants.h (right): https://codereview.chromium.org/557723003/diff/40001/content/public/common/em... content/public/common/eme_constants.h:18: EME_INIT_DATA_TYPE_WEBM = 1 << 1, nit: WEBM appears first in the other enum - be consistent. https://codereview.chromium.org/557723003/diff/40001/content/public/common/em... content/public/common/eme_constants.h:24: enum EmeCodec { // *_ALL should only be used for checking the contents of a mask. Do not use it to specify codec support because additional codecs could be added here. https://codereview.chromium.org/557723003/diff/40001/content/renderer/media/c... File content/renderer/media/crypto/key_systems.cc (right): https://codereview.chromium.org/557723003/diff/40001/content/renderer/media/c... content/renderer/media/crypto/key_systems.cc:38: // Mapping between initialization data types names and enum values. Add a comment to update IsSaneInitDataTypeWithContainer() if necessary when adding new types. https://codereview.chromium.org/557723003/diff/40001/content/renderer/media/c... content/renderer/media/crypto/key_systems.cc:40: {"cenc", EME_INIT_DATA_TYPE_CENC}, ifdef - this will fail to compile. Also, remove the trailing ',' once reordered (below). https://codereview.chromium.org/557723003/diff/40001/content/renderer/media/c... content/renderer/media/crypto/key_systems.cc:41: {"webm", EME_INIT_DATA_TYPE_WEBM}, ditto on swapping order https://codereview.chromium.org/557723003/diff/40001/content/renderer/media/c... content/renderer/media/crypto/key_systems.cc:158: Is there a reason you removed the typedefs? They were shorter and easier to locate. https://codereview.chromium.org/557723003/diff/40001/content/renderer/media/c... content/renderer/media/crypto/key_systems.cc:162: EmeInitDataType LookupInitDataType(const std::string& init_data_type) const; nit: GetInitDataTypeForString()? https://codereview.chromium.org/557723003/diff/40001/content/renderer/media/c... content/renderer/media/crypto/key_systems.cc:163: SupportedCodecs LookupContainer(const std::string& container) const; In the future, it'd be nice to have a container enum too and be able to GetCodecMaskForContainer(), which would return the _ALL codecs value. https://codereview.chromium.org/557723003/diff/40001/content/renderer/media/c... content/renderer/media/crypto/key_systems.cc:163: SupportedCodecs LookupContainer(const std::string& container) const; Is this more GetCodecsMaskForContainer()? https://codereview.chromium.org/557723003/diff/40001/content/renderer/media/c... content/renderer/media/crypto/key_systems.cc:164: EmeCodec LookupCodec(const std::string& codec) const; nit: GetCodecForString() or ConvertStringToCodec()? https://codereview.chromium.org/557723003/diff/40001/content/renderer/media/c... content/renderer/media/crypto/key_systems.cc:166: const std::string& GetConcreteKeySystem(const std::string& key_system) const; nit: ...Name https://codereview.chromium.org/557723003/diff/40001/content/renderer/media/c... content/renderer/media/crypto/key_systems.cc:190: base::hash_map<std::string, SupportedCodecs> container_codecs_; container_to_codecs_map_? https://codereview.chromium.org/557723003/diff/40001/content/renderer/media/c... content/renderer/media/crypto/key_systems.cc:191: base::hash_map<std::string, EmeCodec> codecs_; codec_string_map_? https://codereview.chromium.org/557723003/diff/40001/content/renderer/media/c... content/renderer/media/crypto/key_systems.cc:192: base::hash_map<std::string, EmeInitDataType> init_data_types_; ...map_ https://codereview.chromium.org/557723003/diff/40001/content/renderer/media/c... content/renderer/media/crypto/key_systems.cc:192: base::hash_map<std::string, EmeInitDataType> init_data_types_; make this one first. https://codereview.chromium.org/557723003/diff/40001/content/renderer/media/c... content/renderer/media/crypto/key_systems.cc:214: for (size_t i = 0; i < arraysize(kContainerCodecs); ++i) { On 2014/09/22 23:45:52, sandersd wrote: > On 2014/09/13 01:20:45, ddorwin wrote: > > Macros aren't allowed. Would a template function work? > > I've expanded the macro, but I do think the macro version is harder to typo. You might be able to use a template function. However, a better solution is probably to eliminate these tables in a future CL. https://codereview.chromium.org/557723003/diff/40001/content/renderer/media/c... content/renderer/media/crypto/key_systems.cc:224: for (size_t i = 0; i < arraysize(kInitDataTypes); ++i) { move to line 214 https://codereview.chromium.org/557723003/diff/40001/content/renderer/media/c... content/renderer/media/crypto/key_systems.cc:339: SupportedInitDataTypes supported_init_data_types, move up https://codereview.chromium.org/557723003/diff/40001/content/renderer/media/c... content/renderer/media/crypto/key_systems.cc:356: properties.supported_init_data_types = supported_init_data_types; ditto https://codereview.chromium.org/557723003/diff/40001/content/renderer/media/c... content/renderer/media/crypto/key_systems.cc:386: if (container.find("audio/") == 0) Note: If we also use enums for container types in the Blink layer, we may need to pass the major type too so we can reject video codecs in "audio containers." https://codereview.chromium.org/557723003/diff/40001/content/renderer/media/c... content/renderer/media/crypto/key_systems.cc:407: EmeCodec codec = LookupCodec(codecs[i]); The "codec.empty()" logic was for a specific reason: https://codereview.chromium.org/254913002. We will get 0 now, which will cause us to fall into 411, changing the behavior. It appears that I was unsure whether we wanted it to succeed or fail. For ITS, we can probably be more strict. This might be breaking a layout test, though. xhwang, do you recall more details? https://codereview.chromium.org/557723003/diff/40001/content/renderer/media/c... content/renderer/media/crypto/key_systems.cc:447: // TODO(sandersd): Remove if possible, otherwise reorganize in the same order as See comments below. canPlayType() calls this directly [1]. We might be able to replace that, but we need to maintain the UMAs. (Changing the structure might help us report consistent values for both prefixed and unprefixed.) [1] https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/r... https://codereview.chromium.org/557723003/diff/40001/content/renderer/media/c... content/renderer/media/crypto/key_systems.cc:460: key_systems_support_uma_.ReportKeySystemQuery(key_system, has_type); We now have some paths, including the function above, that do not handle the UMA. We might want to somehow differentiate the two APIs too. xhwang, please advise on the best path. https://codereview.chromium.org/557723003/diff/40001/content/renderer/media/c... content/renderer/media/crypto/key_systems.cc:470: if (!has_type) { Note: The canPlayType() flow would always end up here, even if we really just wanted to check IsSupportedKeySystem. Note that this also helps us handle UMAs correctly. https://codereview.chromium.org/557723003/diff/40001/content/renderer/media/c... content/renderer/media/crypto/key_systems.cc:560: return container != "audio/webm" && container != "video/webm"; Why not positive checks? This is actually wrong if someone decided to add video/ogg or whatever. https://codereview.chromium.org/557723003/diff/40001/content/renderer/media/c... content/renderer/media/crypto/key_systems.cc:564: return true; Related to enums, it would be nice to be able to have a switch with a NOTREACHED here. On some platforms, the lack of a default would also force new types to be considered here. https://codereview.chromium.org/557723003/diff/40001/content/renderer/media/c... content/renderer/media/crypto/key_systems.cc:572: bool IsSupportedKeySystem(const std::string& key_system) { Related to UMAs, if someone refactored the code such that prefixed flowed through here, our UMAs might be broken. https://codereview.chromium.org/557723003/diff/40001/content/renderer/media/c... File content/renderer/media/crypto/key_systems.h (right): https://codereview.chromium.org/557723003/diff/40001/content/renderer/media/c... content/renderer/media/crypto/key_systems.h:27: bool IsSaneInitDataTypeWithContainer( This probably deserves a comment: // Returns false if a container-specific |init_data_type| is specified with an inappropriate container. https://codereview.chromium.org/557723003/diff/40001/content/renderer/media/c... content/renderer/media/crypto/key_systems.h:27: bool IsSaneInitDataTypeWithContainer( This is really an implementation detail of this class - we don't add every combination in our table. Thus, this should probably be handled internally. However, we don't have a function that checks the type and container, so I guess we'll have to leave it exposed (unless/until the spec is updated to separate them :) ). https://codereview.chromium.org/557723003/diff/40001/content/renderer/media/c... content/renderer/media/crypto/key_systems.h:28: const std::string& init_data_type, I was thinking we should convert IDT (and maybe container) to an enum to pass around. (In a separate CL/effort.) WDYT? https://codereview.chromium.org/557723003/diff/40001/content/renderer/media/c... File content/renderer/media/crypto/key_systems_unittest.cc (right): https://codereview.chromium.org/557723003/diff/40001/content/renderer/media/c... content/renderer/media/crypto/key_systems_unittest.cc:111: ext.supported_init_data_types = EME_INIT_DATA_TYPE_ALL; TODO: Add tests for IDT. The reason I say we can do this later is that the structure of ITS, the use of fac codecs, and maybe even the existence of these tests is TBD. https://codereview.chromium.org/557723003/diff/40001/content/renderer/media/w... File content/renderer/media/webcontentdecryptionmodule_impl.cc (right): https://codereview.chromium.org/557723003/diff/40001/content/renderer/media/w... content/renderer/media/webcontentdecryptionmodule_impl.cc:109: (IsConcreteSupportedKeySystem(key_system_ascii) ? nit: () is not strictly necessary. https://codereview.chromium.org/557723003/diff/40001/content/renderer/media/w... content/renderer/media/webcontentdecryptionmodule_impl.cc:110: blink::WebContentDecryptionModule::ProbablySupported : nit: I'm not sure what the style guide says or formatter does, but I'd find this easier to read if these two lines were indented 4 spaces. WDYT? https://codereview.chromium.org/557723003/diff/40001/content/renderer/media/w... content/renderer/media/webcontentdecryptionmodule_impl.cc:165: bool strip_codec_suffixes = !net::IsStrictMediaMimeType(container_ascii); http://crbug.com/374751 also affects this line, so I think we should have an explicit note.
ddorwin@chromium.org changed reviewers: + xhwang@chromium.org
xhwang, please see the two comments for you in https://codereview.chromium.org/557723003/diff/40001/content/renderer/media/c....
https://codereview.chromium.org/557723003/diff/40001/components/cdm/renderer/... File components/cdm/renderer/android_key_systems.cc (right): https://codereview.chromium.org/557723003/diff/40001/components/cdm/renderer/... components/cdm/renderer/android_key_systems.cc:69: // Assume that WebM and CENC are supported by platform CDMs. |KeySystems| On 2014/09/23 22:48:15, ddorwin wrote: > ... CDMs that support their respective containers. KeySystems will remove the > type if the MIME type is not supported. > ^ Is that correct? Or is it filtered somewhere else? No, it just returns false for IsSupported queries, which is all that this data is used for so far. https://codereview.chromium.org/557723003/diff/40001/components/cdm/renderer/... components/cdm/renderer/android_key_systems.cc:69: // Assume that WebM and CENC are supported by platform CDMs. |KeySystems| On 2014/09/23 22:48:15, ddorwin wrote: > I think '|' is usually used for parameters and maybe members. Not classes, > AFAIK. Done. https://codereview.chromium.org/557723003/diff/40001/components/cdm/renderer/... File components/cdm/renderer/widevine_key_systems.cc (right): https://codereview.chromium.org/557723003/diff/40001/components/cdm/renderer/... components/cdm/renderer/widevine_key_systems.cc:53: // |KeySystems| handles validating |init_data_type|/|container| mappings. On 2014/09/23 22:48:15, ddorwin wrote: > ditto. Done. https://codereview.chromium.org/557723003/diff/40001/components/cdm/renderer/... components/cdm/renderer/widevine_key_systems.cc:53: // |KeySystems| handles validating |init_data_type|/|container| mappings. On 2014/09/23 22:48:15, ddorwin wrote: > // Support for a container implies support for the Initialization Data Type. Done. https://codereview.chromium.org/557723003/diff/40001/components/cdm/renderer/... components/cdm/renderer/widevine_key_systems.cc:54: info.supported_init_data_types = content::EME_INIT_DATA_TYPE_NONE; On 2014/09/23 22:48:15, ddorwin wrote: > The constructor should handle this. Done. https://codereview.chromium.org/557723003/diff/40001/content/public/common/em... File content/public/common/eme_constants.h (right): https://codereview.chromium.org/557723003/diff/40001/content/public/common/em... content/public/common/eme_constants.h:18: EME_INIT_DATA_TYPE_WEBM = 1 << 1, On 2014/09/23 22:48:16, ddorwin wrote: > nit: WEBM appears first in the other enum - be consistent. Done. https://codereview.chromium.org/557723003/diff/40001/content/public/common/em... content/public/common/eme_constants.h:24: enum EmeCodec { On 2014/09/23 22:48:16, ddorwin wrote: > // *_ALL should only be used for checking the contents of a mask. Do not use it > to specify codec support because additional codecs could be added here. Done. https://codereview.chromium.org/557723003/diff/40001/content/renderer/media/c... File content/renderer/media/crypto/key_systems.cc (right): https://codereview.chromium.org/557723003/diff/40001/content/renderer/media/c... content/renderer/media/crypto/key_systems.cc:38: // Mapping between initialization data types names and enum values. On 2014/09/23 22:48:16, ddorwin wrote: > Add a comment to update IsSaneInitDataTypeWithContainer() if necessary when > adding new types. Done. https://codereview.chromium.org/557723003/diff/40001/content/renderer/media/c... content/renderer/media/crypto/key_systems.cc:40: {"cenc", EME_INIT_DATA_TYPE_CENC}, On 2014/09/23 22:48:16, ddorwin wrote: > ifdef - this will fail to compile. > Also, remove the trailing ',' once reordered (below). Done. https://codereview.chromium.org/557723003/diff/40001/content/renderer/media/c... content/renderer/media/crypto/key_systems.cc:41: {"webm", EME_INIT_DATA_TYPE_WEBM}, On 2014/09/23 22:48:17, ddorwin wrote: > ditto on swapping order Done. https://codereview.chromium.org/557723003/diff/40001/content/renderer/media/c... content/renderer/media/crypto/key_systems.cc:158: On 2014/09/23 22:48:16, ddorwin wrote: > Is there a reason you removed the typedefs? They were shorter and easier to > locate. Only one of them was used more than once in the code, but separated from their uses making it a pain to go back and forth. I've put them back as requested. https://codereview.chromium.org/557723003/diff/40001/content/renderer/media/c... content/renderer/media/crypto/key_systems.cc:162: EmeInitDataType LookupInitDataType(const std::string& init_data_type) const; On 2014/09/23 22:48:16, ddorwin wrote: > nit: GetInitDataTypeForString()? Done. https://codereview.chromium.org/557723003/diff/40001/content/renderer/media/c... content/renderer/media/crypto/key_systems.cc:163: SupportedCodecs LookupContainer(const std::string& container) const; On 2014/09/23 22:48:16, ddorwin wrote: > In the future, it'd be nice to have a container enum too and be able to > GetCodecMaskForContainer(), which would return the _ALL codecs value. Done. https://codereview.chromium.org/557723003/diff/40001/content/renderer/media/c... content/renderer/media/crypto/key_systems.cc:163: SupportedCodecs LookupContainer(const std::string& container) const; On 2014/09/23 22:48:17, ddorwin wrote: > Is this more GetCodecsMaskForContainer()? Done. https://codereview.chromium.org/557723003/diff/40001/content/renderer/media/c... content/renderer/media/crypto/key_systems.cc:164: EmeCodec LookupCodec(const std::string& codec) const; On 2014/09/23 22:48:17, ddorwin wrote: > nit: GetCodecForString() or ConvertStringToCodec()? Done. https://codereview.chromium.org/557723003/diff/40001/content/renderer/media/c... content/renderer/media/crypto/key_systems.cc:166: const std::string& GetConcreteKeySystem(const std::string& key_system) const; On 2014/09/23 22:48:17, ddorwin wrote: > nit: ...Name Done. https://codereview.chromium.org/557723003/diff/40001/content/renderer/media/c... content/renderer/media/crypto/key_systems.cc:190: base::hash_map<std::string, SupportedCodecs> container_codecs_; On 2014/09/23 22:48:16, ddorwin wrote: > container_to_codecs_map_? Done. https://codereview.chromium.org/557723003/diff/40001/content/renderer/media/c... content/renderer/media/crypto/key_systems.cc:191: base::hash_map<std::string, EmeCodec> codecs_; On 2014/09/23 22:48:17, ddorwin wrote: > codec_string_map_? Done. https://codereview.chromium.org/557723003/diff/40001/content/renderer/media/c... content/renderer/media/crypto/key_systems.cc:192: base::hash_map<std::string, EmeInitDataType> init_data_types_; On 2014/09/23 22:48:16, ddorwin wrote: > ...map_ Done. https://codereview.chromium.org/557723003/diff/40001/content/renderer/media/c... content/renderer/media/crypto/key_systems.cc:192: base::hash_map<std::string, EmeInitDataType> init_data_types_; On 2014/09/23 22:48:17, ddorwin wrote: > make this one first. Done. https://codereview.chromium.org/557723003/diff/40001/content/renderer/media/c... content/renderer/media/crypto/key_systems.cc:214: for (size_t i = 0; i < arraysize(kContainerCodecs); ++i) { On 2014/09/23 22:48:17, ddorwin wrote: > On 2014/09/22 23:45:52, sandersd wrote: > > On 2014/09/13 01:20:45, ddorwin wrote: > > > Macros aren't allowed. Would a template function work? > > > > I've expanded the macro, but I do think the macro version is harder to typo. > > You might be able to use a template function. However, a better solution is > probably to eliminate these tables in a future CL. Sounds good, I've mentioned that in the bug for refactoring key_systems_unittest.cc https://codereview.chromium.org/557723003/diff/40001/content/renderer/media/c... content/renderer/media/crypto/key_systems.cc:224: for (size_t i = 0; i < arraysize(kInitDataTypes); ++i) { On 2014/09/23 22:48:16, ddorwin wrote: > move to line 214 Done. https://codereview.chromium.org/557723003/diff/40001/content/renderer/media/c... content/renderer/media/crypto/key_systems.cc:339: SupportedInitDataTypes supported_init_data_types, On 2014/09/23 22:48:16, ddorwin wrote: > move up Alright, now I'm sad that the compiler didn't warn about this one. https://codereview.chromium.org/557723003/diff/40001/content/renderer/media/c... content/renderer/media/crypto/key_systems.cc:356: properties.supported_init_data_types = supported_init_data_types; On 2014/09/23 22:48:17, ddorwin wrote: > ditto Done. https://codereview.chromium.org/557723003/diff/40001/content/renderer/media/c... content/renderer/media/crypto/key_systems.cc:386: if (container.find("audio/") == 0) On 2014/09/23 22:48:16, ddorwin wrote: > Note: If we also use enums for container types in the Blink layer, we may need > to pass the major type too so we can reject video codecs in "audio containers." Yes, we'll need both to be able to keep doing the container -> codec mask map lookup. https://codereview.chromium.org/557723003/diff/40001/content/renderer/media/c... content/renderer/media/crypto/key_systems.cc:447: // TODO(sandersd): Remove if possible, otherwise reorganize in the same order as On 2014/09/23 22:48:16, ddorwin wrote: > See comments below. > canPlayType() calls this directly [1]. We might be able to replace that, but we > need to maintain the UMAs. (Changing the structure might help us report > consistent values for both prefixed and unprefixed.) > > [1] > https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/r... Done. https://codereview.chromium.org/557723003/diff/40001/content/renderer/media/c... content/renderer/media/crypto/key_systems.cc:560: return container != "audio/webm" && container != "video/webm"; On 2014/09/23 22:48:17, ddorwin wrote: > Why not positive checks? > This is actually wrong if someone decided to add video/ogg or whatever. I was assuming the goal was to exclude bad combinations, but either way is fine. https://codereview.chromium.org/557723003/diff/40001/content/renderer/media/c... content/renderer/media/crypto/key_systems.cc:564: return true; On 2014/09/23 22:48:17, ddorwin wrote: > Related to enums, it would be nice to be able to have a switch with a NOTREACHED > here. On some platforms, the lack of a default would also force new types to be > considered here. Sounds reasonable, I've just added the NOTREACHED for now. https://codereview.chromium.org/557723003/diff/40001/content/renderer/media/c... content/renderer/media/crypto/key_systems.cc:572: bool IsSupportedKeySystem(const std::string& key_system) { On 2014/09/23 22:48:16, ddorwin wrote: > Related to UMAs, if someone refactored the code such that prefixed flowed > through here, our UMAs might be broken. I added a comment to the implementation. https://codereview.chromium.org/557723003/diff/40001/content/renderer/media/c... File content/renderer/media/crypto/key_systems.h (right): https://codereview.chromium.org/557723003/diff/40001/content/renderer/media/c... content/renderer/media/crypto/key_systems.h:27: bool IsSaneInitDataTypeWithContainer( On 2014/09/23 22:48:17, ddorwin wrote: > This probably deserves a comment: > // Returns false if a container-specific |init_data_type| is specified with an > inappropriate container. Done. https://codereview.chromium.org/557723003/diff/40001/content/renderer/media/c... content/renderer/media/crypto/key_systems.h:27: bool IsSaneInitDataTypeWithContainer( On 2014/09/23 22:48:17, ddorwin wrote: > This is really an implementation detail of this class - we don't add every > combination in our table. Thus, this should probably be handled internally. > However, we don't have a function that checks the type and container, so I guess > we'll have to leave it exposed (unless/until the spec is updated to separate > them :) ). Acknowledged. https://codereview.chromium.org/557723003/diff/40001/content/renderer/media/c... content/renderer/media/crypto/key_systems.h:28: const std::string& init_data_type, On 2014/09/23 22:48:17, ddorwin wrote: > I was thinking we should convert IDT (and maybe container) to an enum to pass > around. (In a separate CL/effort.) WDYT? This makes sense; we could add APIs here to turn the string into enum values, then use those values everywhere else. For this particular method it doesn't make much difference. https://codereview.chromium.org/557723003/diff/40001/content/renderer/media/c... File content/renderer/media/crypto/key_systems_unittest.cc (right): https://codereview.chromium.org/557723003/diff/40001/content/renderer/media/c... content/renderer/media/crypto/key_systems_unittest.cc:111: ext.supported_init_data_types = EME_INIT_DATA_TYPE_ALL; On 2014/09/23 22:48:17, ddorwin wrote: > TODO: Add tests for IDT. > > The reason I say we can do this later is that the structure of ITS, the use of > fac codecs, and maybe even the existence of these tests is TBD. Done. https://codereview.chromium.org/557723003/diff/40001/content/renderer/media/w... File content/renderer/media/webcontentdecryptionmodule_impl.cc (right): https://codereview.chromium.org/557723003/diff/40001/content/renderer/media/w... content/renderer/media/webcontentdecryptionmodule_impl.cc:109: (IsConcreteSupportedKeySystem(key_system_ascii) ? On 2014/09/23 22:48:18, ddorwin wrote: > nit: () is not strictly necessary. Done. https://codereview.chromium.org/557723003/diff/40001/content/renderer/media/w... content/renderer/media/webcontentdecryptionmodule_impl.cc:110: blink::WebContentDecryptionModule::ProbablySupported : On 2014/09/23 22:48:17, ddorwin wrote: > nit: I'm not sure what the style guide says or formatter does, but I'd find this > easier to read if these two lines were indented 4 spaces. WDYT? Done. https://codereview.chromium.org/557723003/diff/40001/content/renderer/media/w... content/renderer/media/webcontentdecryptionmodule_impl.cc:165: bool strip_codec_suffixes = !net::IsStrictMediaMimeType(container_ascii); On 2014/09/23 22:48:17, ddorwin wrote: > http://crbug.com/374751 also affects this line, so I think we should have an > explicit note. Done.
Looks pretty good. After you make these changes, let's get input from xhwang on my questions in the previous PS (mostly related to UMA). https://codereview.chromium.org/557723003/diff/40001/content/renderer/media/c... File content/renderer/media/crypto/key_systems.cc (right): https://codereview.chromium.org/557723003/diff/40001/content/renderer/media/c... content/renderer/media/crypto/key_systems.cc:339: SupportedInitDataTypes supported_init_data_types, On 2014/09/24 22:22:34, sandersd wrote: > On 2014/09/23 22:48:16, ddorwin wrote: > > move up > > Alright, now I'm sad that the compiler didn't warn about this one. The danger of using masks! https://codereview.chromium.org/557723003/diff/40001/content/renderer/media/c... content/renderer/media/crypto/key_systems.cc:470: if (!has_type) { On 2014/09/23 22:48:17, ddorwin wrote: > Note: The canPlayType() flow would always end up here, even if we really just > wanted to check IsSupportedKeySystem. Note that this also helps us handle UMAs > correctly. +xhwang https://codereview.chromium.org/557723003/diff/40001/content/renderer/media/c... File content/renderer/media/crypto/key_systems.h (right): https://codereview.chromium.org/557723003/diff/40001/content/renderer/media/c... content/renderer/media/crypto/key_systems.h:28: const std::string& init_data_type, On 2014/09/24 22:22:34, sandersd wrote: > On 2014/09/23 22:48:17, ddorwin wrote: > > I was thinking we should convert IDT (and maybe container) to an enum to pass > > around. (In a separate CL/effort.) WDYT? > > This makes sense; we could add APIs here to turn the string into enum values, > then use those values everywhere else. > > For this particular method it doesn't make much difference. Even better, the strings should never reach Chromium. :) https://codereview.chromium.org/557723003/diff/60001/components/cdm/renderer/... File components/cdm/renderer/android_key_systems.cc (right): https://codereview.chromium.org/557723003/diff/60001/components/cdm/renderer/... components/cdm/renderer/android_key_systems.cc:70: // CDMs. KeySystems validates that they match with the chosen container. Check the container like the next file. The fact that ITS() will check is too fragile and may not always be true. https://codereview.chromium.org/557723003/diff/60001/content/public/common/em... File content/public/common/eme_constants.h (right): https://codereview.chromium.org/557723003/diff/60001/content/public/common/em... content/public/common/eme_constants.h:48: typedef uint32_t SupportedInitDataTypes; FYI, the "enum class" C++11 thread had some discussion relevant to such bit mask uses of enums. https://codereview.chromium.org/557723003/diff/60001/content/public/renderer/... File content/public/renderer/key_system_info.cc (right): https://codereview.chromium.org/557723003/diff/60001/content/public/renderer/... content/public/renderer/key_system_info.cc:5: #include "content/public/common/eme_constants.h" You don't need this if it's already in the header file for the .cc file. https://codereview.chromium.org/557723003/diff/60001/content/public/renderer/... content/public/renderer/key_system_info.cc:13: supported_init_data_types(EME_INIT_DATA_TYPE_NONE), swap order https://codereview.chromium.org/557723003/diff/60001/content/public/renderer/... File content/public/renderer/key_system_info.h (right): https://codereview.chromium.org/557723003/diff/60001/content/public/renderer/... content/public/renderer/key_system_info.h:45: SupportedInitDataTypes supported_init_data_types; Hmm. This order is inconsistent with all the uses, right? We should probably change it here too. https://codereview.chromium.org/557723003/diff/60001/content/renderer/media/c... File content/renderer/media/crypto/key_systems.cc (right): https://codereview.chromium.org/557723003/diff/60001/content/renderer/media/c... content/renderer/media/crypto/key_systems.cc:38: // Mapping between initialization data types names and enum values. Make sure to nit: When adding entries, update... https://codereview.chromium.org/557723003/diff/60001/content/renderer/media/c... content/renderer/media/crypto/key_systems.cc:162: typedef base::hash_map<std::string, KeySystemProperties> On 2014/09/24 22:22:34, sandersd wrote: > On 2014/09/23 22:48:16, ddorwin wrote: > > Is there a reason you removed the typedefs? They were shorter and easier to > > locate. > > Only one of them was used more than once in the code, but separated from their > uses making it a pain to go back and forth. > > I've put them back as requested. If you don't like these, you can use auto and for_each to eliminate some of the uses and thus eliminate the typedefs. (In another CL.) https://codereview.chromium.org/557723003/diff/60001/content/renderer/media/c... content/renderer/media/crypto/key_systems.cc:341: key_system_info.supported_init_data_types, Move up. Yikes - the danger of using masks! https://codereview.chromium.org/557723003/diff/60001/content/renderer/media/c... content/renderer/media/crypto/key_systems.cc:467: // TODO(sandersd): Reforganize to be more similar to Reorganize https://codereview.chromium.org/557723003/diff/60001/content/renderer/media/c... content/renderer/media/crypto/key_systems.cc:583: NOTREACHED(); It's not clear whether we check for invalid IDTs elsewhere. Either way, this could rot when a new IDT is added. (We don't yet force new IDTs to be added here.) Thus, restore just "return true". https://codereview.chromium.org/557723003/diff/60001/content/renderer/media/c... content/renderer/media/crypto/key_systems.cc:592: bool IsSupportedKeySystem(const std::string& key_system) { On 2014/09/24 22:22:34, sandersd wrote: > On 2014/09/23 22:48:16, ddorwin wrote: > > Related to UMAs, if someone refactored the code such that prefixed flowed > > through here, our UMAs might be broken. > > I added a comment to the implementation. Hmm. Might be better in the header file since that's what callers should be looking at. https://codereview.chromium.org/557723003/diff/60001/content/renderer/media/w... File content/renderer/media/webcontentdecryptionmodule_impl.cc (right): https://codereview.chromium.org/557723003/diff/60001/content/renderer/media/w... content/renderer/media/webcontentdecryptionmodule_impl.cc:165: // TODO(sandersd): Check that the codecs are unambiguous. //crbug.com/374751 "Check the suffixes rather than stripping them." Remove the "//" before crbug.
https://codereview.chromium.org/557723003/diff/60001/components/cdm/renderer/... File components/cdm/renderer/android_key_systems.cc (right): https://codereview.chromium.org/557723003/diff/60001/components/cdm/renderer/... components/cdm/renderer/android_key_systems.cc:70: // CDMs. KeySystems validates that they match with the chosen container. On 2014/09/27 00:41:24, ddorwin wrote: > Check the container like the next file. The fact that ITS() will check is too > fragile and may not always be true. Done. https://codereview.chromium.org/557723003/diff/60001/content/public/common/em... File content/public/common/eme_constants.h (right): https://codereview.chromium.org/557723003/diff/60001/content/public/common/em... content/public/common/eme_constants.h:48: typedef uint32_t SupportedInitDataTypes; On 2014/09/27 00:41:24, ddorwin wrote: > FYI, the "enum class" C++11 thread had some discussion relevant to such bit mask > uses of enums. Acknowledged. https://codereview.chromium.org/557723003/diff/60001/content/public/renderer/... File content/public/renderer/key_system_info.cc (right): https://codereview.chromium.org/557723003/diff/60001/content/public/renderer/... content/public/renderer/key_system_info.cc:5: #include "content/public/common/eme_constants.h" On 2014/09/27 00:41:24, ddorwin wrote: > You don't need this if it's already in the header file for the .cc file. Done. https://codereview.chromium.org/557723003/diff/60001/content/public/renderer/... content/public/renderer/key_system_info.cc:13: supported_init_data_types(EME_INIT_DATA_TYPE_NONE), On 2014/09/27 00:41:24, ddorwin wrote: > swap order Done. https://codereview.chromium.org/557723003/diff/60001/content/public/renderer/... File content/public/renderer/key_system_info.h (right): https://codereview.chromium.org/557723003/diff/60001/content/public/renderer/... content/public/renderer/key_system_info.h:45: SupportedInitDataTypes supported_init_data_types; On 2014/09/27 00:41:24, ddorwin wrote: > Hmm. This order is inconsistent with all the uses, right? We should probably > change it here too. Done. https://codereview.chromium.org/557723003/diff/60001/content/renderer/media/c... File content/renderer/media/crypto/key_systems.cc (right): https://codereview.chromium.org/557723003/diff/60001/content/renderer/media/c... content/renderer/media/crypto/key_systems.cc:38: // Mapping between initialization data types names and enum values. Make sure to On 2014/09/27 00:41:24, ddorwin wrote: > nit: When adding entries, update... Done. https://codereview.chromium.org/557723003/diff/60001/content/renderer/media/c... content/renderer/media/crypto/key_systems.cc:162: typedef base::hash_map<std::string, KeySystemProperties> On 2014/09/27 00:41:25, ddorwin wrote: > On 2014/09/24 22:22:34, sandersd wrote: > > On 2014/09/23 22:48:16, ddorwin wrote: > > > Is there a reason you removed the typedefs? They were shorter and easier to > > > locate. > > > > Only one of them was used more than once in the code, but separated from their > > uses making it a pain to go back and forth. > > > > I've put them back as requested. > > If you don't like these, you can use auto and for_each to eliminate some of the > uses and thus eliminate the typedefs. (In another CL.) Acknowledged. https://codereview.chromium.org/557723003/diff/60001/content/renderer/media/c... content/renderer/media/crypto/key_systems.cc:341: key_system_info.supported_init_data_types, On 2014/09/27 00:41:24, ddorwin wrote: > Move up. Yikes - the danger of using masks! Done. https://codereview.chromium.org/557723003/diff/60001/content/renderer/media/c... content/renderer/media/crypto/key_systems.cc:467: // TODO(sandersd): Reforganize to be more similar to On 2014/09/27 00:41:25, ddorwin wrote: > Reorganize Done. https://codereview.chromium.org/557723003/diff/60001/content/renderer/media/c... content/renderer/media/crypto/key_systems.cc:583: NOTREACHED(); On 2014/09/27 00:41:25, ddorwin wrote: > It's not clear whether we check for invalid IDTs elsewhere. > Either way, this could rot when a new IDT is added. (We don't yet force new IDTs > to be added here.) Thus, restore just "return true". Done. https://codereview.chromium.org/557723003/diff/60001/content/renderer/media/c... content/renderer/media/crypto/key_systems.cc:592: bool IsSupportedKeySystem(const std::string& key_system) { On 2014/09/27 00:41:24, ddorwin wrote: > On 2014/09/24 22:22:34, sandersd wrote: > > On 2014/09/23 22:48:16, ddorwin wrote: > > > Related to UMAs, if someone refactored the code such that prefixed flowed > > > through here, our UMAs might be broken. > > > > I added a comment to the implementation. > > Hmm. Might be better in the header file since that's what callers should be > looking at. Done. https://codereview.chromium.org/557723003/diff/60001/content/renderer/media/w... File content/renderer/media/webcontentdecryptionmodule_impl.cc (right): https://codereview.chromium.org/557723003/diff/60001/content/renderer/media/w... content/renderer/media/webcontentdecryptionmodule_impl.cc:165: // TODO(sandersd): Check that the codecs are unambiguous. //crbug.com/374751 On 2014/09/27 00:41:25, ddorwin wrote: > "Check the suffixes rather than stripping them." > Remove the "//" before crbug. Done.
LG % nit. xhwang, please see my three comments for you in https://codereview.chromium.org/557723003/diff/40001/content/renderer/media/c... (an older PS). https://codereview.chromium.org/557723003/diff/80001/content/renderer/media/c... File content/renderer/media/crypto/key_systems.h (right): https://codereview.chromium.org/557723003/diff/80001/content/renderer/media/c... content/renderer/media/crypto/key_systems.h:36: // Note: Shouldn't be used for prefixed API as the old path reports UMAs. "old path" isn't clear to future readers. I assume you mean "IsSupportedKeySystemWithMediaMimeType()."
https://codereview.chromium.org/557723003/diff/80001/content/renderer/media/c... File content/renderer/media/crypto/key_systems.h (right): https://codereview.chromium.org/557723003/diff/80001/content/renderer/media/c... content/renderer/media/crypto/key_systems.h:36: // Note: Shouldn't be used for prefixed API as the old path reports UMAs. On 2014/09/29 18:53:52, ddorwin wrote: > "old path" isn't clear to future readers. I assume you mean > "IsSupportedKeySystemWithMediaMimeType()." Done.
I didn't fully review this CL. Just provide some info/comments... https://codereview.chromium.org/557723003/diff/40001/content/renderer/media/c... File content/renderer/media/crypto/key_systems.cc (right): https://codereview.chromium.org/557723003/diff/40001/content/renderer/media/c... content/renderer/media/crypto/key_systems.cc:407: EmeCodec codec = LookupCodec(codecs[i]); On 2014/09/23 22:48:16, ddorwin wrote: > The "codec.empty()" logic was for a specific reason: > https://codereview.chromium.org/254913002. > > We will get 0 now, which will cause us to fall into 411, changing the behavior. > > It appears that I was unsure whether we wanted it to succeed or fail. For ITS, > we can probably be more strict. This might be breaking a layout test, though. > xhwang, do you recall more details? Yeah, see the CL description of https://codereview.chromium.org/254913002 ',vorbis' will be split into "" and "vorbis": https://code.google.com/p/chromium/codesearch#chromium/src/net/base/mime_util... https://code.google.com/p/chromium/codesearch#chromium/src/base/strings/strin... I am not a fan of this but this is what it is. And currently we have layout tests covering this behavior. I'll be happy to enforce what the spec says. For now, we should probably just keep the old code. An extra comment would be nice. https://codereview.chromium.org/557723003/diff/40001/content/renderer/media/c... content/renderer/media/crypto/key_systems.cc:460: key_systems_support_uma_.ReportKeySystemQuery(key_system, has_type); I checked the current UMA stats (under Counts) and don't find those stats very interesting. We probably want to think more about what's worth reporting. For, I think it's okay to leave alone the UMA stuff and worry about it later. https://codereview.chromium.org/557723003/diff/40001/content/renderer/media/c... content/renderer/media/crypto/key_systems.cc:470: if (!has_type) { On 2014/09/27 00:41:24, ddorwin wrote: > On 2014/09/23 22:48:17, ddorwin wrote: > > Note: The canPlayType() flow would always end up here, even if we really just > > wanted to check IsSupportedKeySystem. Note that this also helps us handle UMAs > > correctly. > > +xhwang Acknowledged. https://codereview.chromium.org/557723003/diff/100001/content/public/common/e... File content/public/common/eme_constants.h (right): https://codereview.chromium.org/557723003/diff/100001/content/public/common/e... content/public/common/eme_constants.h:53: #endif // CONTENT_CONSTANTS_H_ CONTENT_PUBLIC_COMMON_EME_CONSTANTS_H_
https://codereview.chromium.org/557723003/diff/40001/content/renderer/media/c... File content/renderer/media/crypto/key_systems.cc (right): https://codereview.chromium.org/557723003/diff/40001/content/renderer/media/c... content/renderer/media/crypto/key_systems.cc:407: EmeCodec codec = LookupCodec(codecs[i]); On 2014/09/29 21:27:17, xhwang wrote: > On 2014/09/23 22:48:16, ddorwin wrote: > > The "codec.empty()" logic was for a specific reason: > > https://codereview.chromium.org/254913002. > > > > We will get 0 now, which will cause us to fall into 411, changing the > behavior. > > > > It appears that I was unsure whether we wanted it to succeed or fail. For ITS, > > we can probably be more strict. This might be breaking a layout test, though. > > xhwang, do you recall more details? > > Yeah, see the CL description of https://codereview.chromium.org/254913002 > > ',vorbis' will be split into "" and "vorbis": > > https://code.google.com/p/chromium/codesearch#chromium/src/net/base/mime_util... > > https://code.google.com/p/chromium/codesearch#chromium/src/base/strings/strin... > > I am not a fan of this but this is what it is. And currently we have layout > tests covering this behavior. I'll be happy to enforce what the spec says. > > For now, we should probably just keep the old code. An extra comment would be > nice. Acknowledged. (There is a bug reference comment now) https://codereview.chromium.org/557723003/diff/40001/content/renderer/media/c... content/renderer/media/crypto/key_systems.cc:460: key_systems_support_uma_.ReportKeySystemQuery(key_system, has_type); On 2014/09/29 21:27:17, xhwang wrote: > I checked the current UMA stats (under Counts) and don't find those stats very > interesting. We probably want to think more about what's worth reporting. > > For, I think it's okay to leave alone the UMA stuff and worry about it later. Acknowledged. https://codereview.chromium.org/557723003/diff/100001/content/public/common/e... File content/public/common/eme_constants.h (right): https://codereview.chromium.org/557723003/diff/100001/content/public/common/e... content/public/common/eme_constants.h:53: #endif // CONTENT_CONSTANTS_H_ On 2014/09/29 21:27:18, xhwang wrote: > CONTENT_PUBLIC_COMMON_EME_CONSTANTS_H_ Done.
LGTM. We'll need to revisit UMAs for unprefixed. Do we have a bug for that?
On 2014/09/29 22:13:22, ddorwin wrote: > LGTM. We'll need to revisit UMAs for unprefixed. Do we have a bug for that? We have http://crbug.com/351501
sandersd@chromium.org changed reviewers: + ben@chromium.org, nasko@chromium.org
nasko@chromium.org: Please review changes in components/cdm/common. ben@chromium.org: Please review changes in content/public and content/renderer.
sandersd@chromium.org changed reviewers: + avi@chromium.org - ben@chromium.org
bin: Ignore previous. avi: Please review changes in content/public and content/renderer.
components/cdm/common/cdm_messages_android.h LGTM
lgtm stamp https://codereview.chromium.org/557723003/diff/120001/content/renderer/media/... File content/renderer/media/crypto/key_systems.h (right): https://codereview.chromium.org/557723003/diff/120001/content/renderer/media/... content/renderer/media/crypto/key_systems.h:29: // TODO(sandersd): Remove this essentailly internal detail if the spec is spelling of essentailly
https://codereview.chromium.org/557723003/diff/120001/content/renderer/media/... File content/renderer/media/crypto/key_systems.h (right): https://codereview.chromium.org/557723003/diff/120001/content/renderer/media/... content/renderer/media/crypto/key_systems.h:29: // TODO(sandersd): Remove this essentailly internal detail if the spec is On 2014/09/30 05:13:54, Avi wrote: > spelling of essentailly Done.
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/557723003/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_gn_dbg on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by sandersd@chromium.org
FYI, I have removed the portions of this CL that expose it to Blink, as the interface specification is being reworked.
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/557723003/160001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_dbg_tests_recipe on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)
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/557723003/180001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/67568) win_chromium_rel_swarming on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
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/557723003/200001
Message was sent while issue was closed.
Committed patchset #11 (id:200001) as 78021a0ed663fc3f62b10751eb0c4f8806d0cdab
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/3573622da943ccb017e1dd93791a7efad5a3bcc8 Cr-Commit-Position: refs/heads/master@{#297550} |
