Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
183943003:
Verify MediaKeys parameter values consistently (Closed)
|
Created:
6 years, 9 months ago by jrummell Modified:
6 years, 9 months ago CC:
blink-reviews, feature-media-reviews_chromium.org, philipj_slow, eric.carlson_apple.com Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Visibility:
Public. |
DescriptionVerify MediaKeys parameter values consistently
Change to use same logic for verifying keySystem and contentType values when creating MediaKeys and MediaKeySessions as is used for isTypeSupported(). Removes unfinished stub code in ContentDecryptionModule.
BUG=342107
TEST=manual
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=168321
Patch Set 1 #
Total comments: 4
Patch Set 2 : Helper methods #
Total comments: 6
Patch Set 3 : rename #
Total comments: 4
Patch Set 4 : One method #
Messages
Total messages: 22 (0 generated)
PTAL. Expands on xhwang@'s change to implement isTypeSupported().
The description, especially the summary, should explain that we're hooking up the checks. "Better" is not very descriptive or accurate. You might also need to update the rest of the description based on the inline comments. https://codereview.chromium.org/183943003/diff/1/Source/modules/encryptedmedi... File Source/modules/encryptedmedia/MediaKeys.cpp (right): https://codereview.chromium.org/183943003/diff/1/Source/modules/encryptedmedi... Source/modules/encryptedmedia/MediaKeys.cpp:56: if (!isTypeSupported(keySystem, "")) { I don't think we should use the public function for either of these things. Among other things, we get a log message as if the application made the call. There might also be logic that we don't need/doesn't make sense. What we should probably do is encapsulate line 144 in a function - isSupportedKeySystem() - and call that here and there. That function can ASSERT(!keySystem.empty()) since callers should have checked that. This is a super simple wrapper that we would normally avoid, but it helps with the algorithms, avoiding calling the parent function and encapsulating what internal function we call to actually check. https://codereview.chromium.org/183943003/diff/1/Source/modules/encryptedmedi... Source/modules/encryptedmedia/MediaKeys.cpp:111: if (!isTypeSupported(m_keySystem, contentType)) { ~Ditto, except maybe step 4 of IST gets encapsulated.
looking good, just one nit comment https://codereview.chromium.org/183943003/diff/20001/Source/modules/encrypted... File Source/modules/encryptedmedia/MediaKeys.cpp (right): https://codereview.chromium.org/183943003/diff/20001/Source/modules/encrypted... Source/modules/encryptedmedia/MediaKeys.cpp:162: bool MediaKeys::isSupportedContentType(const String& keySystem, const String& contentType) nit: can we just have one helper function, e.g. isKeySystemSupportedWithContentType or just isTypeSupportedImpl(), where the "contentType" can be empty? These two helper functions looks too similar :)
Updated code and description. PTAL. https://codereview.chromium.org/183943003/diff/1/Source/modules/encryptedmedi... File Source/modules/encryptedmedia/MediaKeys.cpp (right): https://codereview.chromium.org/183943003/diff/1/Source/modules/encryptedmedi... Source/modules/encryptedmedia/MediaKeys.cpp:56: if (!isTypeSupported(keySystem, "")) { On 2014/02/27 22:44:13, ddorwin wrote: > I don't think we should use the public function for either of these things. > Among other things, we get a log message as if the application made the call. > There might also be logic that we don't need/doesn't make sense. > > What we should probably do is encapsulate line 144 in a function - > isSupportedKeySystem() - and call that here and there. That function can > ASSERT(!keySystem.empty()) since callers should have checked that. This is a > super simple wrapper that we would normally avoid, but it helps with the > algorithms, avoiding calling the parent function and encapsulating what internal > function we call to actually check. Done. https://codereview.chromium.org/183943003/diff/1/Source/modules/encryptedmedi... Source/modules/encryptedmedia/MediaKeys.cpp:111: if (!isTypeSupported(m_keySystem, contentType)) { On 2014/02/27 22:44:13, ddorwin wrote: > ~Ditto, except maybe step 4 of IST gets encapsulated. Done.
lgtm https://codereview.chromium.org/183943003/diff/20001/Source/modules/encrypted... File Source/modules/encryptedmedia/MediaKeys.cpp (right): https://codereview.chromium.org/183943003/diff/20001/Source/modules/encrypted... Source/modules/encryptedmedia/MediaKeys.cpp:162: bool MediaKeys::isSupportedContentType(const String& keySystem, const String& contentType) On 2014/02/27 23:19:51, xhwang wrote: > nit: can we just have one helper function, e.g. > isKeySystemSupportedWithContentType or just isTypeSupportedImpl(), where the > "contentType" can be empty? These two helper functions looks too similar :) Yes, but they work nicely in the algorithms and hide the implementation details (granted, within the same class). Also, we want each spec algorithm step to be specific, which makes larger shared helper functions less appealing. This function would need to be more complex if it could be called with an empty contentType. That would basically be redundant with step 3 above. https://codereview.chromium.org/183943003/diff/20001/Source/modules/encrypted... File Source/modules/encryptedmedia/MediaKeys.h (right): https://codereview.chromium.org/183943003/diff/20001/Source/modules/encrypted... Source/modules/encryptedmedia/MediaKeys.h:78: static bool isSupportedContentType(const String& keySystem, const String& contentType); This one can actually be a member and doesn't need the first parameter. Except we use it in static isTypeSupported(), so never mind...
https://codereview.chromium.org/183943003/diff/20001/Source/modules/encrypted... File Source/modules/encryptedmedia/MediaKeys.cpp (right): https://codereview.chromium.org/183943003/diff/20001/Source/modules/encrypted... Source/modules/encryptedmedia/MediaKeys.cpp:162: bool MediaKeys::isSupportedContentType(const String& keySystem, const String& contentType) On 2014/02/27 23:28:27, ddorwin wrote: > On 2014/02/27 23:19:51, xhwang wrote: > > nit: can we just have one helper function, e.g. > > isKeySystemSupportedWithContentType or just isTypeSupportedImpl(), where the > > "contentType" can be empty? These two helper functions looks too similar :) > > Yes, but they work nicely in the algorithms and hide the implementation details > (granted, within the same class). Also, we want each spec algorithm step to be > specific, which makes larger shared helper functions less appealing. > > This function would need to be more complex if it could be called with an empty > contentType. That would basically be redundant with step 3 above. If we don't need to pass in "keySystem" into isSupportedContentType() then I'd agree. Now from function signature to implementation, it's clear that isSupportedKeySystem() is just a special case of isSupportedContentType(). This is consistent with the spec, where we do progressive checks, not two independent checks. We should make this clear. So either have one helper function and pass in empty types in the first check, or we should rename isSupportedContentType to isSupportedKeySystemAndContentType to reflect this relation.
https://codereview.chromium.org/183943003/diff/20001/Source/modules/encrypted... File Source/modules/encryptedmedia/MediaKeys.cpp (right): https://codereview.chromium.org/183943003/diff/20001/Source/modules/encrypted... Source/modules/encryptedmedia/MediaKeys.cpp:162: bool MediaKeys::isSupportedContentType(const String& keySystem, const String& contentType) On 2014/02/27 23:42:36, xhwang wrote: > On 2014/02/27 23:28:27, ddorwin wrote: > > On 2014/02/27 23:19:51, xhwang wrote: > > > nit: can we just have one helper function, e.g. > > > isKeySystemSupportedWithContentType or just isTypeSupportedImpl(), where the > > > "contentType" can be empty? These two helper functions looks too similar :) > > > > Yes, but they work nicely in the algorithms and hide the implementation > details > > (granted, within the same class). Also, we want each spec algorithm step to be > > specific, which makes larger shared helper functions less appealing. > > > > This function would need to be more complex if it could be called with an > empty > > contentType. That would basically be redundant with step 3 above. > > If we don't need to pass in "keySystem" into isSupportedContentType() then I'd > agree. Now from function signature to implementation, it's clear that > isSupportedKeySystem() is just a special case of isSupportedContentType(). This > is consistent with the spec, where we do progressive checks, not two independent > checks. We should make this clear. So either have one helper function and pass > in empty types in the first check, or we should rename isSupportedContentType to > isSupportedKeySystemAndContentType to reflect this relation. Maybe. I considered that. Really, it is isKeySystemSupportedWithContentType(). I think the really problem is that we don't have an object-oriented way to execute step 4. Instead of asking whether the key system supports a type, we ask Chromium whether it supports key system with the type. As I mentioned in the .h file, it would be nice to use m_keySystem for the createSession() case. Come to think of it, these don't need to be member functions at all. We could still keep one for the createSession() case if we wanted. You end up with more functions, though. Even more fun createSession() shouldn't really have codecs, though I believe it is still allowed. Depending on the outcome of spec bugs, createSession() and isTypeSupported() might actually take different types of strings. That could mean we end up with different underlying implementations. Looking forward to that (and avoiding sharing code that shouldn't be shared), maybe createSession() should ask whether the initData type is supported and strip the codecs from the string. I believe that is actually more correct, though the spec doesn't currently say so. Only the container should matter in the createSession() algorithm. (The difference is whether codecs=foo should fail or be ignored.)
Updated function names. PTAL. https://codereview.chromium.org/183943003/diff/20001/Source/modules/encrypted... File Source/modules/encryptedmedia/MediaKeys.cpp (right): https://codereview.chromium.org/183943003/diff/20001/Source/modules/encrypted... Source/modules/encryptedmedia/MediaKeys.cpp:162: bool MediaKeys::isSupportedContentType(const String& keySystem, const String& contentType) On 2014/02/28 00:14:09, ddorwin wrote: > On 2014/02/27 23:42:36, xhwang wrote: > > On 2014/02/27 23:28:27, ddorwin wrote: > > > On 2014/02/27 23:19:51, xhwang wrote: > > > > nit: can we just have one helper function, e.g. > > > > isKeySystemSupportedWithContentType or just isTypeSupportedImpl(), where > the > > > > "contentType" can be empty? These two helper functions looks too similar > :) > > > > > > Yes, but they work nicely in the algorithms and hide the implementation > > details > > > (granted, within the same class). Also, we want each spec algorithm step to > be > > > specific, which makes larger shared helper functions less appealing. > > > > > > This function would need to be more complex if it could be called with an > > empty > > > contentType. That would basically be redundant with step 3 above. > > > > If we don't need to pass in "keySystem" into isSupportedContentType() then I'd > > agree. Now from function signature to implementation, it's clear that > > isSupportedKeySystem() is just a special case of isSupportedContentType(). > This > > is consistent with the spec, where we do progressive checks, not two > independent > > checks. We should make this clear. So either have one helper function and pass > > in empty types in the first check, or we should rename isSupportedContentType > to > > isSupportedKeySystemAndContentType to reflect this relation. > > Maybe. I considered that. Really, it is isKeySystemSupportedWithContentType(). > > I think the really problem is that we don't have an object-oriented way to > execute step 4. Instead of asking whether the key system supports a type, we ask > Chromium whether it supports key system with the type. > > As I mentioned in the .h file, it would be nice to use m_keySystem for the > createSession() case. > > Come to think of it, these don't need to be member functions at all. We could > still keep one for the createSession() case if we wanted. You end up with more > functions, though. > > Even more fun createSession() shouldn't really have codecs, though I believe it > is still allowed. Depending on the outcome of spec bugs, createSession() and > isTypeSupported() might actually take different types of strings. That could > mean we end up with different underlying implementations. > > Looking forward to that (and avoiding sharing code that shouldn't be shared), > maybe createSession() should ask whether the initData type is supported and > strip the codecs from the string. > > I believe that is actually more correct, though the spec doesn't currently say > so. Only the container should matter in the createSession() algorithm. (The > difference is whether codecs=foo should fail or be ignored.) Renamed function.
lgtm, thanks!
+acolwell@, as media blink patron
lgtm % comment https://codereview.chromium.org/183943003/diff/40001/Source/modules/encrypted... File Source/modules/encryptedmedia/MediaKeys.h (right): https://codereview.chromium.org/183943003/diff/40001/Source/modules/encrypted... Source/modules/encryptedmedia/MediaKeys.h:77: static bool isKeySystemSupported(const String& keySystem); I forgot to add here: These don't actually need to be members. They can be implemented locally in the .cc.
lgtm % nit https://codereview.chromium.org/183943003/diff/40001/Source/modules/encrypted... File Source/modules/encryptedmedia/MediaKeys.cpp (right): https://codereview.chromium.org/183943003/diff/40001/Source/modules/encrypted... Source/modules/encryptedmedia/MediaKeys.cpp:165: ASSERT(!contentType.isEmpty()); nit: I'm not sure there is a lot of value having 2 methods here. I actually find it a little confusing especially since these both ultimately bottom out in the same MIMETypeRegistry call. Also the fact that there are parameter checks before these methods are even called make the ASSERTS() here seem redundant.
+abarth@ for OWNERS review. https://codereview.chromium.org/183943003/diff/40001/Source/modules/encrypted... File Source/modules/encryptedmedia/MediaKeys.cpp (right): https://codereview.chromium.org/183943003/diff/40001/Source/modules/encrypted... Source/modules/encryptedmedia/MediaKeys.cpp:165: ASSERT(!contentType.isEmpty()); On 2014/02/28 16:50:40, acolwell wrote: > nit: I'm not sure there is a lot of value having 2 methods here. I actually find > it a little confusing especially since these both ultimately bottom out in the > same MIMETypeRegistry call. Also the fact that there are parameter checks before > these methods are even called make the ASSERTS() here seem redundant. Done. https://codereview.chromium.org/183943003/diff/40001/Source/modules/encrypted... File Source/modules/encryptedmedia/MediaKeys.h (right): https://codereview.chromium.org/183943003/diff/40001/Source/modules/encrypted... Source/modules/encryptedmedia/MediaKeys.h:77: static bool isKeySystemSupported(const String& keySystem); On 2014/02/28 05:01:09, ddorwin wrote: > I forgot to add here: > These don't actually need to be members. They can be implemented locally in the > .cc. Done.
rslgtm Should we create an OWNERS file in platform/drm so the appropriate folks can review CLs there?
The CQ bit was checked by abarth@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jrummell@chromium.org/183943003/60001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on win_layout for step(s) webkit_lint http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_layout...
On 2014/03/01 06:46:10, abarth wrote: > rslgtm > > Should we create an OWNERS file in platform/drm so the appropriate folks can > review CLs there? We plan to eliminate these files and talk directly to WebContentDecryptionModule*. jrummel, now would be a good time to do that so we don't need OWNERS approval in the future. :)
The CQ bit was checked by jrummell@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jrummell@chromium.org/183943003/60001
Message was sent while issue was closed.
Change committed as 168321 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
