Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10575026:
Add ProxyDecryptor which wraps concrete Decryptor implementations. (Closed)
|
Created:
8 years, 6 months ago by xhwang Modified:
8 years, 5 months ago CC:
chromium-reviews, feature-media-reviews_chromium.org, darin-cc_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd ProxyDecryptor which wraps concrete Decryptor implementations.
For now it only wraps AesDecryptor. In the future it will also wrap
PpapiDecryptor.
BUG=123260
TEST=media_unittests, media layout tests.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=144869
Patch Set 1 #
Total comments: 1
Patch Set 2 : Move proxy_decryptor.* into webkit/media/crypto/ #
Total comments: 16
Patch Set 3 : Revise on comments. #
Total comments: 22
Patch Set 4 : Add CreateDecryptor in key_system.* #
Total comments: 9
Patch Set 5 : Fixed nits and rebased. #
Messages
Total messages: 24 (0 generated)
Hello scherkus and ddorwin, This is the CL that introduces the ProxyDecryptor. Please review. xhwang http://codereview.chromium.org/10575026/diff/1/webkit/media/webmediaplayer_im... File webkit/media/webmediaplayer_impl.cc (right): http://codereview.chromium.org/10575026/diff/1/webkit/media/webmediaplayer_im... webkit/media/webmediaplayer_impl.cc:747: return WebKit::WebMediaPlayer::MediaKeyExceptionInvalidPlayerState; This will break all demos that call AddKey() before calling GenerateKeyRequest().
http://codereview.chromium.org/10575026/diff/3001/webkit/media/crypto/proxy_d... File webkit/media/crypto/proxy_decryptor.cc (right): http://codereview.chromium.org/10575026/diff/3001/webkit/media/crypto/proxy_d... webkit/media/crypto/proxy_decryptor.cc:16: const char kClearKeyKeySystem[] = "webkit-org.w3.clearkey"; this is declared in webkit/media/key_systems.cc -- can we dedupe? http://codereview.chromium.org/10575026/diff/3001/webkit/media/crypto/proxy_d... webkit/media/crypto/proxy_decryptor.cc:38: decryptor_.reset(new media::AesDecryptor(client_)); is there a disadvantage to creating this in the ctor instead? http://codereview.chromium.org/10575026/diff/3001/webkit/media/crypto/proxy_d... webkit/media/crypto/proxy_decryptor.cc:41: NOTREACHED(); can these DCHECKs be triggered via JS? i.e., I call gKR() w/ "foo.blah.mycrypto" ? http://codereview.chromium.org/10575026/diff/3001/webkit/media/crypto/proxy_d... webkit/media/crypto/proxy_decryptor.cc:54: DCHECK(decryptor_.get() && key_system == current_key_system_); instead of && split the DCHECKs http://codereview.chromium.org/10575026/diff/3001/webkit/media/crypto/proxy_d... webkit/media/crypto/proxy_decryptor.cc:61: DCHECK(decryptor_.get() && key_system == current_key_system_); instead of && split the DCHECKs http://codereview.chromium.org/10575026/diff/3001/webkit/media/webmediaplayer... File webkit/media/webmediaplayer_impl.cc (right): http://codereview.chromium.org/10575026/diff/3001/webkit/media/webmediaplayer... webkit/media/webmediaplayer_impl.cc:746: if (current_key_system_.isEmpty() || key_system != current_key_system_) couldn't this state be kept inside of decryptor_? WMPI creates a ProxyDecryptor -- it can hold onto one as well instead of Decryptor
http://codereview.chromium.org/10575026/diff/3001/webkit/media/crypto/proxy_d... File webkit/media/crypto/proxy_decryptor.cc (right): http://codereview.chromium.org/10575026/diff/3001/webkit/media/crypto/proxy_d... webkit/media/crypto/proxy_decryptor.cc:16: const char kClearKeyKeySystem[] = "webkit-org.w3.clearkey"; On 2012/06/21 21:17:16, scherkus wrote: > this is declared in webkit/media/key_systems.cc -- can we dedupe? Will do. http://codereview.chromium.org/10575026/diff/3001/webkit/media/crypto/proxy_d... webkit/media/crypto/proxy_decryptor.cc:38: decryptor_.reset(new media::AesDecryptor(client_)); On 2012/06/21 21:17:16, scherkus wrote: > is there a disadvantage to creating this in the ctor instead? Currently we don't allow run-time switching between selected decryptors. This is to simplify the locking scheme we use and minimize the locking time (maybe it's pre-mature optimization :P). By creating this in the ctor, we'll need to switch from AesDecryptor to e.g. PpapiDecryptor at run-time. http://codereview.chromium.org/10575026/diff/3001/webkit/media/crypto/proxy_d... webkit/media/crypto/proxy_decryptor.cc:41: NOTREACHED(); On 2012/06/21 21:17:16, scherkus wrote: > can these DCHECKs be triggered via JS? i.e., I call gKR() w/ "foo.blah.mycrypto" > ? No. The WMPI will check IsSupportedKeySystem() and rule out unsupported key system strings. http://codereview.chromium.org/10575026/diff/3001/webkit/media/crypto/proxy_d... webkit/media/crypto/proxy_decryptor.cc:54: DCHECK(decryptor_.get() && key_system == current_key_system_); On 2012/06/21 21:17:16, scherkus wrote: > instead of && split the DCHECKs Will do. http://codereview.chromium.org/10575026/diff/3001/webkit/media/crypto/proxy_d... webkit/media/crypto/proxy_decryptor.cc:61: DCHECK(decryptor_.get() && key_system == current_key_system_); On 2012/06/21 21:17:16, scherkus wrote: > instead of && split the DCHECKs Will do. http://codereview.chromium.org/10575026/diff/3001/webkit/media/webmediaplayer... File webkit/media/webmediaplayer_impl.cc (right): http://codereview.chromium.org/10575026/diff/3001/webkit/media/webmediaplayer... webkit/media/webmediaplayer_impl.cc:746: if (current_key_system_.isEmpty() || key_system != current_key_system_) On 2012/06/21 21:17:16, scherkus wrote: > couldn't this state be kept inside of decryptor_? Yeah, I thought about that too. The only reason I didn't do it is that we need to return MediaKeyExceptionInvalidPlayerState. But I agree that this code looks awkward. I'll discuss with ddorwin@ to find a better solution. > > WMPI creates a ProxyDecryptor -- it can hold onto one as well instead of > Decryptor That's right. Will do!
ping -- did you mean to upload a new copy of the code?
On 2012/06/26 03:12:27, scherkus wrote: > ping -- did you mean to upload a new copy of the code? Sorry for the delay. I was working on something else and need to talk to ddorwin@ about where we should check those conditions. Will upload a new patch asap.
New patch uploaded. Please review again. http://codereview.chromium.org/10575026/diff/3001/webkit/media/crypto/proxy_d... File webkit/media/crypto/proxy_decryptor.cc (right): http://codereview.chromium.org/10575026/diff/3001/webkit/media/crypto/proxy_d... webkit/media/crypto/proxy_decryptor.cc:16: const char kClearKeyKeySystem[] = "webkit-org.w3.clearkey"; On 2012/06/22 16:57:30, xhwang wrote: > On 2012/06/21 21:17:16, scherkus wrote: > > this is declared in webkit/media/key_systems.cc -- can we dedupe? > > Will do. Done. http://codereview.chromium.org/10575026/diff/3001/webkit/media/crypto/proxy_d... webkit/media/crypto/proxy_decryptor.cc:54: DCHECK(decryptor_.get() && key_system == current_key_system_); On 2012/06/22 16:57:30, xhwang wrote: > On 2012/06/21 21:17:16, scherkus wrote: > > instead of && split the DCHECKs > > Will do. Done. http://codereview.chromium.org/10575026/diff/3001/webkit/media/crypto/proxy_d... webkit/media/crypto/proxy_decryptor.cc:61: DCHECK(decryptor_.get() && key_system == current_key_system_); On 2012/06/22 16:57:30, xhwang wrote: > On 2012/06/21 21:17:16, scherkus wrote: > > instead of && split the DCHECKs > > Will do. Done. http://codereview.chromium.org/10575026/diff/3001/webkit/media/webmediaplayer... File webkit/media/webmediaplayer_impl.cc (right): http://codereview.chromium.org/10575026/diff/3001/webkit/media/webmediaplayer... webkit/media/webmediaplayer_impl.cc:746: if (current_key_system_.isEmpty() || key_system != current_key_system_) On 2012/06/22 16:57:30, xhwang wrote: > On 2012/06/21 21:17:16, scherkus wrote: > > couldn't this state be kept inside of decryptor_? > > Yeah, I thought about that too. The only reason I didn't do it is that we need > to return MediaKeyExceptionInvalidPlayerState. But I agree that this code looks > awkward. I'll discuss with ddorwin@ to find a better solution. > > > > WMPI creates a ProxyDecryptor -- it can hold onto one as well instead of > > Decryptor > > That's right. Will do! Done.
http://codereview.chromium.org/10575026/diff/19001/webkit/media/crypto/key_sy... File webkit/media/crypto/key_systems.h (right): http://codereview.chromium.org/10575026/diff/19001/webkit/media/crypto/key_sy... webkit/media/crypto/key_systems.h:17: const char kClearKeyKeySystem[] = "webkit-org.w3.clearkey"; this can be declared here but initialized in the .cc http://codereview.chromium.org/10575026/diff/19001/webkit/media/webmediaplaye... File webkit/media/webmediaplayer_impl.cc (right): http://codereview.chromium.org/10575026/diff/19001/webkit/media/webmediaplaye... webkit/media/webmediaplayer_impl.cc:718: if (!current_key_system_.isEmpty() && key_system != current_key_system_) you can move this into a helper of ProxyDecryptor i.e., decryptor_->current_key_system()
LG overall http://codereview.chromium.org/10575026/diff/19001/webkit/media/crypto/DEPS File webkit/media/crypto/DEPS (right): http://codereview.chromium.org/10575026/diff/19001/webkit/media/crypto/DEPS#n... webkit/media/crypto/DEPS:2: "+media/base", Do we not inherit +media from the parent file? I'm not familiar with the syntax. If so, we might want to -media. http://codereview.chromium.org/10575026/diff/19001/webkit/media/crypto/proxy_... File webkit/media/crypto/proxy_decryptor.cc (right): http://codereview.chromium.org/10575026/diff/19001/webkit/media/crypto/proxy_... webkit/media/crypto/proxy_decryptor.cc:30: if (!decryptor_.get()) { Do we assume that decryptor_ is set always on the same thread (and thus do not need to lock it)? If so, that's worth commenting. http://codereview.chromium.org/10575026/diff/19001/webkit/media/crypto/proxy_... webkit/media/crypto/proxy_decryptor.cc:32: base::AutoLock auto_lock(lock_); If we don't need to lock above, why are we locking here? If there is no decryptor, nobody is using it, right? Or can the decoder be using it before we create one. I'm guessing that's the reason. If so, this is also worth a comment. http://codereview.chromium.org/10575026/diff/19001/webkit/media/crypto/proxy_... webkit/media/crypto/proxy_decryptor.cc:49: DCHECK(decryptor_.get()); // WMPI ensures GenerateKeyRequest() has been called. Is that right? Same below. http://codereview.chromium.org/10575026/diff/19001/webkit/media/crypto/proxy_... File webkit/media/crypto/proxy_decryptor.h (right): http://codereview.chromium.org/10575026/diff/19001/webkit/media/crypto/proxy_... webkit/media/crypto/proxy_decryptor.h:21: // forwards decryptor calls to it. Currently we don't support run-time Make the second sentence a TODO so we don't forget to delete it later? http://codereview.chromium.org/10575026/diff/19001/webkit/media/webmediaplaye... File webkit/media/webmediaplayer_impl.cc (right): http://codereview.chromium.org/10575026/diff/19001/webkit/media/webmediaplaye... webkit/media/webmediaplayer_impl.cc:718: if (!current_key_system_.isEmpty() && key_system != current_key_system_) On 2012/06/27 01:10:18, scherkus wrote: > you can move this into a helper of ProxyDecryptor > > i.e., decryptor_->current_key_system() Or even is_current_key_system(string) and is_key_system_set() http://codereview.chromium.org/10575026/diff/19001/webkit/media/webmediaplaye... webkit/media/webmediaplayer_impl.cc:907: #define COMPILE_ASSERT_MATCHING_ENUM(name) \ Should all these be at the top of the file together? http://codereview.chromium.org/10575026/diff/19001/webkit/media/webmediaplaye... File webkit/media/webmediaplayer_impl.h (right): http://codereview.chromium.org/10575026/diff/19001/webkit/media/webmediaplaye... webkit/media/webmediaplayer_impl.h:297: scoped_ptr<webkit_media::ProxyDecryptor> decryptor_; This could just be a member - just initialize it in the initializers list.
http://codereview.chromium.org/10575026/diff/19001/webkit/media/crypto/key_sy... File webkit/media/crypto/key_systems.h (right): http://codereview.chromium.org/10575026/diff/19001/webkit/media/crypto/key_sy... webkit/media/crypto/key_systems.h:17: const char kClearKeyKeySystem[] = "webkit-org.w3.clearkey"; On 2012/06/27 01:10:18, scherkus wrote: > this can be declared here but initialized in the .cc This is only used to determine the CDM. We eventually want a static registry that tells us what to create. Rather than move this string and expose it elsewhere, I suggest adding UseAesDecryptor(string) or something to this file. (I'm not exactly sure how we will specify AES vs. external CDMs, but this seems close enough for now.)
http://codereview.chromium.org/10575026/diff/19001/webkit/media/crypto/key_sy... File webkit/media/crypto/key_systems.h (right): http://codereview.chromium.org/10575026/diff/19001/webkit/media/crypto/key_sy... webkit/media/crypto/key_systems.h:17: const char kClearKeyKeySystem[] = "webkit-org.w3.clearkey"; On 2012/06/27 04:07:15, ddorwin wrote: > On 2012/06/27 01:10:18, scherkus wrote: > > this can be declared here but initialized in the .cc > > This is only used to determine the CDM. We eventually want a static registry > that tells us what to create. Rather than move this string and expose it > elsewhere, I suggest adding UseAesDecryptor(string) or something to this file. > (I'm not exactly sure how we will specify AES vs. external CDMs, but this seems > close enough for now.) Could you explain the UseAesDecryptor() approach a bit more? Not sure I understand. Also is this something that needs to be done now? Seems like something that could wait until we have a proper registry.
http://codereview.chromium.org/10575026/diff/19001/webkit/media/crypto/key_sy... File webkit/media/crypto/key_systems.h (right): http://codereview.chromium.org/10575026/diff/19001/webkit/media/crypto/key_sy... webkit/media/crypto/key_systems.h:17: const char kClearKeyKeySystem[] = "webkit-org.w3.clearkey"; On 2012/06/27 04:22:23, scherkus wrote: > On 2012/06/27 04:07:15, ddorwin wrote: > > On 2012/06/27 01:10:18, scherkus wrote: > > > this can be declared here but initialized in the .cc > > > > This is only used to determine the CDM. We eventually want a static registry > > that tells us what to create. Rather than move this string and expose it > > elsewhere, I suggest adding UseAesDecryptor(string) or something to this file. > > (I'm not exactly sure how we will specify AES vs. external CDMs, but this > seems > > close enough for now.) > > Could you explain the UseAesDecryptor() approach a bit more? Not sure I > understand. The string that is being exposed is only used in the .cc file and proxy_decryptor.cc line 33, which will use this registry in the future. However, AES decryptor/clear key will be different than most/all items in the registry, so the registry might need something like UseAesDecryptor() anyway. If we expose the necessary function, we don't need to expose the string. > > Also is this something that needs to be done now? Seems like something that > could wait until we have a proper registry. It could wait, but I'd rather not expose the string now (and allow other uses to get added). Exposing a potentially temporary function seems better than exposing a string that should not be exposed.
http://codereview.chromium.org/10575026/diff/19001/webkit/media/crypto/key_sy... File webkit/media/crypto/key_systems.h (right): http://codereview.chromium.org/10575026/diff/19001/webkit/media/crypto/key_sy... webkit/media/crypto/key_systems.h:17: const char kClearKeyKeySystem[] = "webkit-org.w3.clearkey"; On 2012/06/27 05:24:41, ddorwin wrote: > On 2012/06/27 04:22:23, scherkus wrote: > > On 2012/06/27 04:07:15, ddorwin wrote: > > > On 2012/06/27 01:10:18, scherkus wrote: > > > > this can be declared here but initialized in the .cc > > > > > > This is only used to determine the CDM. We eventually want a static registry > > > that tells us what to create. Rather than move this string and expose it > > > elsewhere, I suggest adding UseAesDecryptor(string) or something to this > file. > > > (I'm not exactly sure how we will specify AES vs. external CDMs, but this > > seems > > > close enough for now.) > > > > Could you explain the UseAesDecryptor() approach a bit more? Not sure I > > understand. > The string that is being exposed is only used in the .cc file and > proxy_decryptor.cc line 33, which will use this registry in the future. However, > AES decryptor/clear key will be different than most/all items in the registry, > so the registry might need something like UseAesDecryptor() anyway. > > If we expose the necessary function, we don't need to expose the string. > > > > Also is this something that needs to be done now? Seems like something that > > could wait until we have a proper registry. > It could wait, but I'd rather not expose the string now (and allow other uses to > get added). Exposing a potentially temporary function seems better than exposing > a string that should not be exposed. I don't think it's really that big of an issue but if you feel strongly about it I'd be OK implementing a crude registry now Considering ProxyDecryptor only references a Decryptor (and not an AesDecryptor) we can: 1) define a function here that returns a Decryptor or NULL given a key system 2) update ProxyDecryptor to know nothing about clear key WDYT?
Updated as discussed. Please review again. scherkus: please pay attention to webkit/media/crypto/DEPS. I was following the pattern of some DPPS files I found. Please help double check. http://codereview.chromium.org/10575026/diff/19001/webkit/media/crypto/key_sy... File webkit/media/crypto/key_systems.h (right): http://codereview.chromium.org/10575026/diff/19001/webkit/media/crypto/key_sy... webkit/media/crypto/key_systems.h:17: const char kClearKeyKeySystem[] = "webkit-org.w3.clearkey"; On 2012/06/27 05:34:11, scherkus wrote: > On 2012/06/27 05:24:41, ddorwin wrote: > > On 2012/06/27 04:22:23, scherkus wrote: > > > On 2012/06/27 04:07:15, ddorwin wrote: > > > > On 2012/06/27 01:10:18, scherkus wrote: > > > > > this can be declared here but initialized in the .cc > > > > > > > > This is only used to determine the CDM. We eventually want a static > registry > > > > that tells us what to create. Rather than move this string and expose it > > > > elsewhere, I suggest adding UseAesDecryptor(string) or something to this > > file. > > > > (I'm not exactly sure how we will specify AES vs. external CDMs, but this > > > seems > > > > close enough for now.) > > > > > > Could you explain the UseAesDecryptor() approach a bit more? Not sure I > > > understand. > > The string that is being exposed is only used in the .cc file and > > proxy_decryptor.cc line 33, which will use this registry in the future. > However, > > AES decryptor/clear key will be different than most/all items in the registry, > > so the registry might need something like UseAesDecryptor() anyway. > > > > If we expose the necessary function, we don't need to expose the string. > > > > > > Also is this something that needs to be done now? Seems like something that > > > could wait until we have a proper registry. > > It could wait, but I'd rather not expose the string now (and allow other uses > to > > get added). Exposing a potentially temporary function seems better than > exposing > > a string that should not be exposed. > > I don't think it's really that big of an issue but if you feel strongly about it > I'd be OK implementing a crude registry now > > Considering ProxyDecryptor only references a Decryptor (and not an AesDecryptor) > we can: > 1) define a function here that returns a Decryptor or NULL given a key system > 2) update ProxyDecryptor to know nothing about clear key > > WDYT? Done. http://codereview.chromium.org/10575026/diff/19001/webkit/media/crypto/proxy_... File webkit/media/crypto/proxy_decryptor.cc (right): http://codereview.chromium.org/10575026/diff/19001/webkit/media/crypto/proxy_... webkit/media/crypto/proxy_decryptor.cc:30: if (!decryptor_.get()) { On 2012/06/27 04:00:08, ddorwin wrote: > Do we assume that decryptor_ is set always on the same thread (and thus do not > need to lock it)? If so, that's worth commenting. Added comment about |lock_| in .h file. http://codereview.chromium.org/10575026/diff/19001/webkit/media/crypto/proxy_... webkit/media/crypto/proxy_decryptor.cc:32: base::AutoLock auto_lock(lock_); On 2012/06/27 04:00:08, ddorwin wrote: > If we don't need to lock above, why are we locking here? If there is no > decryptor, nobody is using it, right? Or can the decoder be using it before we > create one. I'm guessing that's the reason. If so, this is also worth a comment. Added comment about |lock_| in .h file. http://codereview.chromium.org/10575026/diff/19001/webkit/media/crypto/proxy_... webkit/media/crypto/proxy_decryptor.cc:49: DCHECK(decryptor_.get()); On 2012/06/27 04:00:08, ddorwin wrote: > // WMPI ensures GenerateKeyRequest() has been called. > Is that right? > Same below. Done. http://codereview.chromium.org/10575026/diff/19001/webkit/media/crypto/proxy_... File webkit/media/crypto/proxy_decryptor.h (right): http://codereview.chromium.org/10575026/diff/19001/webkit/media/crypto/proxy_... webkit/media/crypto/proxy_decryptor.h:21: // forwards decryptor calls to it. Currently we don't support run-time On 2012/06/27 04:00:08, ddorwin wrote: > Make the second sentence a TODO so we don't forget to delete it later? Done. http://codereview.chromium.org/10575026/diff/19001/webkit/media/webmediaplaye... File webkit/media/webmediaplayer_impl.cc (right): http://codereview.chromium.org/10575026/diff/19001/webkit/media/webmediaplaye... webkit/media/webmediaplayer_impl.cc:718: if (!current_key_system_.isEmpty() && key_system != current_key_system_) On 2012/06/27 04:00:08, ddorwin wrote: > On 2012/06/27 01:10:18, scherkus wrote: > > you can move this into a helper of ProxyDecryptor > > > > i.e., decryptor_->current_key_system() > > Or even is_current_key_system(string) and is_key_system_set() As discussed, ProxyDecryptor will know nothing about key system strings. Removed the DCHECK in ProxyDecryptor and keep the checking here. http://codereview.chromium.org/10575026/diff/19001/webkit/media/webmediaplaye... webkit/media/webmediaplayer_impl.cc:907: #define COMPILE_ASSERT_MATCHING_ENUM(name) \ On 2012/06/27 04:00:08, ddorwin wrote: > Should all these be at the top of the file together? I am following the current style (at least in webkit_media code): checking before it's used. http://codereview.chromium.org/10575026/diff/19001/webkit/media/webmediaplaye... File webkit/media/webmediaplayer_impl.h (right): http://codereview.chromium.org/10575026/diff/19001/webkit/media/webmediaplaye... webkit/media/webmediaplayer_impl.h:297: scoped_ptr<webkit_media::ProxyDecryptor> decryptor_; On 2012/06/27 04:00:08, ddorwin wrote: > This could just be a member - just initialize it in the initializers list. Done.
LGTM w/ nits http://codereview.chromium.org/10575026/diff/35001/webkit/media/crypto/DEPS File webkit/media/crypto/DEPS (right): http://codereview.chromium.org/10575026/diff/35001/webkit/media/crypto/DEPS#n... webkit/media/crypto/DEPS:1: include_rules = [ yeah we don't need this file -- just delete it http://codereview.chromium.org/10575026/diff/35001/webkit/media/crypto/key_sy... File webkit/media/crypto/key_systems.h (right): http://codereview.chromium.org/10575026/diff/35001/webkit/media/crypto/key_sy... webkit/media/crypto/key_systems.h:35: // Creates a decryptor that corresponds to the |key_system|. "returns NULL if blah blah blah" http://codereview.chromium.org/10575026/diff/35001/webkit/media/webmediaplaye... File webkit/media/webmediaplayer_impl.h (right): http://codereview.chromium.org/10575026/diff/35001/webkit/media/webmediaplaye... webkit/media/webmediaplayer_impl.h:345: scoped_ptr<webkit_media::ProxyDecryptor> decryptor_; this doesn't even have to be a scoped_ptr<> :)
LGTM w/ comments. http://codereview.chromium.org/10575026/diff/35001/webkit/media/crypto/key_sy... File webkit/media/crypto/key_systems.cc (right): http://codereview.chromium.org/10575026/diff/35001/webkit/media/crypto/key_sy... webkit/media/crypto/key_systems.cc:8: #include "media/base/decryptor_client.h" Is this one necessary? http://codereview.chromium.org/10575026/diff/35001/webkit/media/webmediaplaye... File webkit/media/webmediaplayer_impl.h (right): http://codereview.chromium.org/10575026/diff/35001/webkit/media/webmediaplaye... webkit/media/webmediaplayer_impl.h:345: scoped_ptr<webkit_media::ProxyDecryptor> decryptor_; On 2012/06/28 18:32:52, scherkus wrote: > this doesn't even have to be a scoped_ptr<> :) Yes, that's what I was trying to say. Just do: webkit_media::ProxyDecryptor decryptor_
http://codereview.chromium.org/10575026/diff/35001/webkit/media/crypto/DEPS File webkit/media/crypto/DEPS (right): http://codereview.chromium.org/10575026/diff/35001/webkit/media/crypto/DEPS#n... webkit/media/crypto/DEPS:1: include_rules = [ On 2012/06/28 18:32:52, scherkus wrote: > yeah we don't need this file -- just delete it Done. http://codereview.chromium.org/10575026/diff/35001/webkit/media/crypto/key_sy... File webkit/media/crypto/key_systems.cc (right): http://codereview.chromium.org/10575026/diff/35001/webkit/media/crypto/key_sy... webkit/media/crypto/key_systems.cc:8: #include "media/base/decryptor_client.h" On 2012/06/28 22:15:38, ddorwin wrote: > Is this one necessary? Removed. http://codereview.chromium.org/10575026/diff/35001/webkit/media/crypto/key_sy... File webkit/media/crypto/key_systems.h (right): http://codereview.chromium.org/10575026/diff/35001/webkit/media/crypto/key_sy... webkit/media/crypto/key_systems.h:35: // Creates a decryptor that corresponds to the |key_system|. On 2012/06/28 18:32:52, scherkus wrote: > "returns NULL if blah blah blah" Done. http://codereview.chromium.org/10575026/diff/35001/webkit/media/webmediaplaye... File webkit/media/webmediaplayer_impl.h (right): http://codereview.chromium.org/10575026/diff/35001/webkit/media/webmediaplaye... webkit/media/webmediaplayer_impl.h:345: scoped_ptr<webkit_media::ProxyDecryptor> decryptor_; On 2012/06/28 22:15:38, ddorwin wrote: > On 2012/06/28 18:32:52, scherkus wrote: > > this doesn't even have to be a scoped_ptr<> :) > > Yes, that's what I was trying to say. Just do: > webkit_media::ProxyDecryptor decryptor_ Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/xhwang@chromium.org/10575026/41002
Presubmit check for 10575026-41002 failed and returned exit status 1.
Running presubmit commit checks ...
** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
webkit
Hello brettw, I need your OWNERS' review on this file: webkit/glue/simple_webmimeregistry_impl.cc It's just an include path change. Thanks, xhwang
LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/xhwang@chromium.org/10575026/41002
Try job failure for 10575026-41002 (retry) on mac_rel for step "compile" (clobber build). It's a second try, previously, step "compile" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/xhwang@chromium.org/10575026/41002
Change committed as 144869 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
