Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
157423003:
Remove the dependency on encryptedmedia from HTMLMediaElement. (Closed)
|
Created:
6 years, 10 months ago by c.shu Modified:
6 years, 9 months ago CC:
blink-reviews, nessy, philipj_slow, gasubic, fs, eric.carlson_apple.com, feature-media-reviews_chromium.org, dglazkov+blink, adamk+blink_chromium.org, vcarbune.chromium, jrummell Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Visibility:
Public. |
DescriptionThis patch removes the dependency on EncryptedMedia from HTMLMediaElement.
BUG=242754, 341883
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=168704
Patch Set 1 #
Total comments: 3
Patch Set 2 : this is the 2nd patch. But more changes are expected based on bug 342036. #Patch Set 3 : The 3rd patch that extracts the MediaPlayerClient::mediaPlayerKeyNeeded function. #Patch Set 4 : Move EME completely from core::HTMLMediaElement to modules::HTMLMediaElementEncryptedMedia (Based o… #Patch Set 5 : Move EME completely from core::HTMLMediaElement to modules::HTMLMediaElementEncryptedMedia (Based o… #
Total comments: 8
Patch Set 6 : Address review comments. #
Total comments: 51
Patch Set 7 : Address ddorwin's review comments. #
Total comments: 7
Patch Set 8 : Address more review comments. #
Total comments: 3
Patch Set 9 : minor changes. #
Total comments: 5
Patch Set 10 : merge latest trunk #Patch Set 11 : Fix compilation error on linux #Messages
Total messages: 56 (0 generated)
would you review it? thanks. Btw, we may want to create new files and give classes/functions better names. I just to want to start the discussion.
[set]MediaKeys and onneedkey also need to be extracted. Is there a more generic solution, such as partial interface? I believe this is how MSE was separated from core.
https://codereview.chromium.org/157423003/diff/1/Source/core/html/HTMLMediaEl... File Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/157423003/diff/1/Source/core/html/HTMLMediaEl... Source/core/html/HTMLMediaElement.cpp:1774: } This whole function should move into the encryptedmedia module. HTMLMediaElement shouldn't be asking whether RuntimeEnabledFeatures::encryptedMediaEnabled() or RuntimeEnabledFeatures::prefixedEncryptedMediaEnabled(). That knowledge should only be useful in the encryptedmedia module.
On 2014/02/07 18:46:58, ddorwin wrote: > [set]MediaKeys and onneedkey also need to be extracted. Is there a more generic > solution, such as partial interface? I believe this is how MSE was separated > from core. It looks to me the cause of the dependency is that HTMLMediaElement inherits MediaPlayerClient and implements the mediaPlayerKeyNeeded virtual function. They are not a part of APIs defined in IDLs.
On 2014/02/07 18:55:35, c.shu wrote: > It looks to me the cause of the dependency is that HTMLMediaElement inherits > MediaPlayerClient and implements the mediaPlayerKeyNeeded virtual function. They > are not a part of APIs defined in IDLs. Maybe we need to split MediaPlayerClient into smaller pieces and implement different functions in different classes?
https://codereview.chromium.org/157423003/diff/1/Source/core/html/HTMLMediaEl... File Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/157423003/diff/1/Source/core/html/HTMLMediaEl... Source/core/html/HTMLMediaElement.cpp:1774: } On 2014/02/07 18:52:50, abarth wrote: > This whole function should move into the encryptedmedia module. What about the m_asyncEventQueue->enqueueEvent call? m_asyncEventQueue is a private member.
https://codereview.chromium.org/157423003/diff/1/Source/core/html/HTMLMediaEl... File Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/157423003/diff/1/Source/core/html/HTMLMediaEl... Source/core/html/HTMLMediaElement.cpp:1774: } On 2014/02/07 19:08:29, c.shu wrote: > On 2014/02/07 18:52:50, abarth wrote: > > This whole function should move into the encryptedmedia module. > What about the m_asyncEventQueue->enqueueEvent call? m_asyncEventQueue is a > private member. I agree with abarth. The EME logic should be moved to the encryptedmedia module and implemented as a partial interface much like HTMLVideoElementMediaSource is. We can expose a public variant of scheduleEvent() to allow the partial implementation to schedule events.
On 2014/02/07 18:57:35, abarth wrote: > On 2014/02/07 18:55:35, c.shu wrote: > > It looks to me the cause of the dependency is that HTMLMediaElement inherits > > MediaPlayerClient and implements the mediaPlayerKeyNeeded virtual function. > They > > are not a part of APIs defined in IDLs. > > Maybe we need to split MediaPlayerClient into smaller pieces and implement > different functions in different classes? I like this idea. How about we do this after all encryptedmedia has been taken out?
On 2014/02/07 20:00:54, c.shu wrote: > On 2014/02/07 18:57:35, abarth wrote: > > On 2014/02/07 18:55:35, c.shu wrote: > > > It looks to me the cause of the dependency is that HTMLMediaElement inherits > > > MediaPlayerClient and implements the mediaPlayerKeyNeeded virtual function. > > They > > > are not a part of APIs defined in IDLs. > > > > Maybe we need to split MediaPlayerClient into smaller pieces and implement > > different functions in different classes? > > I like this idea. How about we do this after all encryptedmedia has been taken > out? Ideally, I'd like to split out the EME methods on WebMediaPlayer and move them to a separate WebPrefixedEMEClient(needs a better name) interface. Initialy WebMediaPlayer could inherit from this interface to maintain compatability, but the ultimate goal would be to have a setEMEClient(WebPrefixedEMEClient*) method on WebMediaPlayer that the code in the EME module could call. I think this would be the easiest way to get this code out of core and avoid introducing code into the MediaPlayer(Client) code, which I'm trying to get rid of.
On 2014/02/07 20:37:18, acolwell wrote: > On 2014/02/07 20:00:54, c.shu wrote: > > On 2014/02/07 18:57:35, abarth wrote: > > > On 2014/02/07 18:55:35, c.shu wrote: > > > > It looks to me the cause of the dependency is that HTMLMediaElement > inherits > > > > MediaPlayerClient and implements the mediaPlayerKeyNeeded virtual > function. > > > They > > > > are not a part of APIs defined in IDLs. > > > > > > Maybe we need to split MediaPlayerClient into smaller pieces and implement > > > different functions in different classes? > > > > I like this idea. How about we do this after all encryptedmedia has been taken > > out? > > Ideally, I'd like to split out the EME methods on WebMediaPlayer and move them > to a separate WebPrefixedEMEClient(needs a better name) interface. Initialy > WebMediaPlayer could inherit from this interface to maintain compatability, but > the ultimate goal would be to have a setEMEClient(WebPrefixedEMEClient*) method > on WebMediaPlayer that the code in the EME module could call. I think this would > be the easiest way to get this code out of core and avoid introducing code into > the MediaPlayer(Client) code, which I'm trying to get rid of. Prefixed EME is a different issue - and something I'm not sure is worth the time to extract since we want to remove it this year. (The same idea _might_ apply to unprefixed EME, but without the "Prefixed" name.)
On 2014/02/07 21:24:03, ddorwin wrote: > On 2014/02/07 20:37:18, acolwell wrote: > > On 2014/02/07 20:00:54, c.shu wrote: > > > On 2014/02/07 18:57:35, abarth wrote: > > > > On 2014/02/07 18:55:35, c.shu wrote: > > > > > It looks to me the cause of the dependency is that HTMLMediaElement > > inherits > > > > > MediaPlayerClient and implements the mediaPlayerKeyNeeded virtual > > function. > > > > They > > > > > are not a part of APIs defined in IDLs. > > > > > > > > Maybe we need to split MediaPlayerClient into smaller pieces and implement > > > > different functions in different classes? > > > > > > I like this idea. How about we do this after all encryptedmedia has been > taken > > > out? > > > > Ideally, I'd like to split out the EME methods on WebMediaPlayer and move them > > to a separate WebPrefixedEMEClient(needs a better name) interface. Initialy > > WebMediaPlayer could inherit from this interface to maintain compatability, > but > > the ultimate goal would be to have a setEMEClient(WebPrefixedEMEClient*) > method > > on WebMediaPlayer that the code in the EME module could call. I think this > would > > be the easiest way to get this code out of core and avoid introducing code > into > > the MediaPlayer(Client) code, which I'm trying to get rid of. > > Prefixed EME is a different issue - and something I'm not sure is worth the time > to extract since we want to remove it this year. (The same idea _might_ apply to > unprefixed EME, but without the "Prefixed" name.) But I don't think we should introduce this MediaEventCreator business in HTMLMediaElement code. We should refactor enough to allow this piece to completely live in the encryptedmedia module.
> But I don't think we should introduce this MediaEventCreator business in > HTMLMediaElement code. We should refactor enough to allow this piece to > completely live in the encryptedmedia module. I agree this MediaEventCreator does not look good. But splitting MediaPlayerClient could be a separate task due to its complexity.
On 2014/02/07 22:22:24, c.shu wrote: > I agree this MediaEventCreator does not look good. But splitting > MediaPlayerClient could be a separate task due to its complexity. I don't think we should shy away from the better design because it's difficult to make the transition. The goal of this work is to improve the structure of the code, which can be difficult.
Hi, guys. This 3rd patch extracts the mediaPlayerKeyNeeded from MediaPlayerClient successfully. Please take a look and see if the approach is ok. We can take care the other functions in an updated patch or in a new issue.
Based on acolwell's initial work, I have moved all the EME code out from HTMLMediaElement.
Looks pretty good. Just a few comments. https://codereview.chromium.org/157423003/diff/270001/Source/modules/encrypte... File Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp (right): https://codereview.chromium.org/157423003/diff/270001/Source/modules/encrypte... Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp:42: , m_mediaElement(element) nit: I don't think you need this. In most cases, it looks like you just need to pass a hasPlayer bool from the static method to the non-static version. In the other cases, it might just be better to pass in the WebMediaPlayer* derived from the HTMLMediaElement passed to the static method. https://codereview.chromium.org/157423003/diff/270001/Source/modules/encrypte... Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp:354: blink::WebMediaPlayer* HTMLMediaElementEncryptedMedia::webMediaPlayer() nit: I think this should be on HTMLMediaElement instead so code can just do m_mediaElement->webMediaPlayer(). https://codereview.chromium.org/157423003/diff/270001/Source/modules/encrypte... File Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.h (right): https://codereview.chromium.org/157423003/diff/270001/Source/modules/encrypte... Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.h:15: typedef blink::WebMediaPlayerClient::MediaKeyErrorCode MediaKeyErrorCode; nit: I don't think this should be here. Nothing in this file appears to reference this typedef. https://codereview.chromium.org/157423003/diff/270001/Source/modules/encrypte... Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.h:42: static void keyAdded(HTMLMediaElement&, const blink::WebString& keySystem, const blink::WebString& sessionId); nit: I think you should change the blink::WebString to String in all these keyXXX methods since this is WebCore code. https://codereview.chromium.org/157423003/diff/270001/Source/modules/encrypte... Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.h:76: MediaPlayer* player(); nit: Remove. It doesn't look like this is used/defined anywhere. https://codereview.chromium.org/157423003/diff/270001/Source/web/WebMediaPlay... File Source/web/WebMediaPlayerClientImpl.cpp (right): https://codereview.chromium.org/157423003/diff/270001/Source/web/WebMediaPlay... Source/web/WebMediaPlayerClientImpl.cpp:74: playerDestroyed(); This looks like the only call to this method so I think we should just inline the HMEEM::playerDestroyed() here. https://codereview.chromium.org/157423003/diff/270001/Source/web/WebMediaPlay... Source/web/WebMediaPlayerClientImpl.cpp:135: HTMLMediaElementEncryptedMedia::keyAdded(*static_cast<HTMLMediaElement*>(m_client), keySystem, sessionId); nit: Please put the static_cast into a HTMLMediaElement& mediaElement() helper so you don't have to copy this everywhere. https://codereview.chromium.org/157423003/diff/270001/Source/web/WebMediaPlay... Source/web/WebMediaPlayerClientImpl.cpp:238: Frame* frame = static_cast<HTMLMediaElement*>(m_client)->document().frame(); nit: Change this to use the the new mediaElement() helper mentioned above.
Thanks for the review, acolwell.
https://codereview.chromium.org/157423003/diff/290001/Source/core/html/HTMLMe... File Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/157423003/diff/290001/Source/core/html/HTMLMe... Source/core/html/HTMLMediaElement.cpp:324: closeMediaSource(); acolwell: Are you intending to do something similar for MSE? https://codereview.chromium.org/157423003/diff/290001/Source/core/html/HTMLMe... Source/core/html/HTMLMediaElement.cpp:325: When do we end up calling setMediaKeysInternal()? I assume from playerDestroyed(). We need to make sure the ordering is correct. The call was intentionally here to be before clearMediaPlayerAndAudioSourceProviderClient() (and consistent with MSE above). It appears playerDestroyed() will be called when WebMediaPlayerClientImpl is destroyed but before WebMediaPlayerImpl is destroyed. That _may_ be okay. https://codereview.chromium.org/157423003/diff/290001/Source/core/html/HTMLMe... Source/core/html/HTMLMediaElement.cpp:524: void HTMLMediaElement::scheduleEvent(const AtomicString& eventName) Should we be overloading here? Maybe this should be scheduleNamedEvent() or something like that. https://codereview.chromium.org/157423003/diff/290001/Source/core/html/HTMLMe... Source/core/html/HTMLMediaElement.cpp:534: m_asyncEventQueue->enqueueEvent(event); IMO, we should either setTarget(this) here and remove all the callers OR ASSERT(target() == this). (Note: This class also fires events at track objects, but that doesn't come through here.) https://codereview.chromium.org/157423003/diff/290001/Source/core/html/HTMLMe... Source/core/html/HTMLMediaElement.cpp:3120: closeMediaSource(); ditto https://codereview.chromium.org/157423003/diff/290001/Source/core/html/HTMLMe... Source/core/html/HTMLMediaElement.cpp:3419: Where does the removed code get called now? There's no playerCreated() method. Since only WMPI knows about the CDM, it appears that this is entirely handled in WebMediaPlayerClientImpl::loadInternal() when WMPI is created. https://codereview.chromium.org/157423003/diff/290001/Source/core/html/HTMLMe... File Source/core/html/HTMLMediaElement.h (right): https://codereview.chromium.org/157423003/diff/290001/Source/core/html/HTMLMe... Source/core/html/HTMLMediaElement.h:84: blink::WebMediaPlayer* webMediaPlayer() const { return m_player ? m_player->webMediaPlayer() : 0; } This seems fishy and like a layering violation, though I know we want to get rid of the MediaPlayer layer. Why not just force the caller to get the WMP from the result of player()? https://codereview.chromium.org/157423003/diff/290001/Source/modules/encrypte... File Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp (right): https://codereview.chromium.org/157423003/diff/290001/Source/modules/encrypte... Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp:56: HTMLMediaElementEncryptedMedia* supplement = static_cast<HTMLMediaElementEncryptedMedia*>(Supplement<HTMLMediaElement>::from(element, supplementName())); Wouldn't it be better to do the case to this Supplement after we have a supplement? https://codereview.chromium.org/157423003/diff/290001/Source/modules/encrypte... Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp:87: WTF_LOG(Media, "HTMLMediaElementEncryptedMedia::setMediaKeys"); Update function name and/or move to 105. https://codereview.chromium.org/157423003/diff/290001/Source/modules/encrypte... Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp:123: return MediaKeyNeededEvent::create(EventTypeNames::needkey, initializer); Blink experts: Is it possible to make the event name part of the event type? It's sort of redundant in this case. https://codereview.chromium.org/157423003/diff/290001/Source/modules/encrypte... Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp:139: void HTMLMediaElementEncryptedMedia::webkitGenerateKeyRequest(HTMLMediaElement& element, const String& keySystem, PassRefPtr<Uint8Array> initData, ExceptionState& exceptionState) These lines seem long even for Blink. https://codereview.chromium.org/157423003/diff/290001/Source/modules/encrypte... Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp:141: HTMLMediaElementEncryptedMedia::from(element).generateKeyRequest(element.player(), element.webMediaPlayer(), keySystem, initData, exceptionState); Would it be better to just pass the element and let the member function decide what to do with it (rather than passing various players)? https://codereview.chromium.org/157423003/diff/290001/Source/modules/encrypte... Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp:144: void HTMLMediaElementEncryptedMedia::generateKeyRequest(bool hasPlayer, blink::WebMediaPlayer* webMediaPlayer, const String& keySystem, PassRefPtr<Uint8Array> initData, ExceptionState& exceptionState) I don't think we need |hasPlayer| - see below. https://codereview.chromium.org/157423003/diff/290001/Source/modules/encrypte... Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp:156: if (!hasPlayer) { Do we really need this parameter? If we instead check webMediaPlayer, we should be fine (assuming we keep the layering violations). https://codereview.chromium.org/157423003/diff/290001/Source/modules/encrypte... Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp:168: blink::WebMediaPlayer::MediaKeyException result = webMediaPlayer ? webMediaPlayer->generateKeyRequest(keySystem, initDataPointer, initDataLength) : blink::WebMediaPlayer::MediaKeyExceptionInvalidPlayerState; This is too confusing and long. If we don't have a WMP, thrown an invalid state exception directly (as in 156). The fact that we have to check both layers makes all this feel wrong. https://codereview.chromium.org/157423003/diff/290001/Source/modules/encrypte... Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp:204: if (!hasPlayer) { ditto here and below. https://codereview.chromium.org/157423003/diff/290001/Source/modules/encrypte... Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp:242: if (!hasPlayer) { ditto https://codereview.chromium.org/157423003/diff/290001/Source/platform/graphic... File Source/platform/graphics/media/MediaPlayer.h (right): https://codereview.chromium.org/157423003/diff/290001/Source/platform/graphic... Source/platform/graphics/media/MediaPlayer.h:188: virtual blink::WebMediaPlayer* webMediaPlayer() const = 0; This seems like a layering violation. Why can't this interface know about EME? platform/ has a CDM insterface. https://codereview.chromium.org/157423003/diff/290001/Source/web/WebMediaPlay... File Source/web/WebMediaPlayerClientImpl.cpp (right): https://codereview.chromium.org/157423003/diff/290001/Source/web/WebMediaPlay... Source/web/WebMediaPlayerClientImpl.cpp:172: void WebMediaPlayerClientImpl::closeHelperPluginSoon(WebFrame*) That's not what the FIXME meant. You should just revert this because there are CLs in flight that will conflict. https://codereview.chromium.org/157423003/diff/290001/Source/web/WebMediaPlay... Source/web/WebMediaPlayerClientImpl.cpp:233: Frame* frame = static_cast<HTMLMediaElement*>(m_client)->document().frame(); FYI, you didn't address acolwell's nit from PS5. You also didn't publish any responses to his comments. https://codereview.chromium.org/157423003/diff/290001/Source/web/WebMediaPlay... Source/web/WebMediaPlayerClientImpl.cpp:653: return *static_cast<WebCore::HTMLMediaElement*>(m_client); WebCore:: is unnecessary. https://codereview.chromium.org/157423003/diff/290001/Source/web/WebMediaPlay... File Source/web/WebMediaPlayerClientImpl.h (right): https://codereview.chromium.org/157423003/diff/290001/Source/web/WebMediaPlay... Source/web/WebMediaPlayerClientImpl.h:84: // FIXME: Remove these once the WebMediaPlayer implementation stop making these calls on the WebMediaPlayerClient object. Where should it be making them?
It looks like the summary was updated but not the description. Also, is this just BUG=242754 now? I think the other bug can probably just be closed as a duplicate of this one.
We have 3 bugs: 1) 341883: Remove dependency on MediaKeyNeededEvent for HTMLMediaElement 2) 342036: Extract EcryptedMediaEvent-related functions in MediaPlayerClient into a new class 3) 242754: Files in Source/core/ should not reference files in Source/modules/encryptedmedia/ Does this address 2? If so, can we close 1 as a duplicate of 2? Is this everything we need for 3? If so, we can update Source/core/DEPS and include #3 in the BUG= entry.
https://codereview.chromium.org/157423003/diff/290001/Source/core/html/HTMLMe... File Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/157423003/diff/290001/Source/core/html/HTMLMe... Source/core/html/HTMLMediaElement.cpp:324: closeMediaSource(); On 2014/02/27 00:37:59, ddorwin wrote: > acolwell: Are you intending to do something similar for MSE? I believe this call is actually inert and will probably be removed. https://codereview.chromium.org/157423003/diff/290001/Source/core/html/HTMLMe... Source/core/html/HTMLMediaElement.cpp:325: On 2014/02/27 00:37:59, ddorwin wrote: > When do we end up calling setMediaKeysInternal()? I assume from > playerDestroyed(). We need to make sure the ordering is correct. The call was > intentionally here to be before clearMediaPlayerAndAudioSourceProviderClient() > (and consistent with MSE above). > > It appears playerDestroyed() will be called when WebMediaPlayerClientImpl is > destroyed but before WebMediaPlayerImpl is destroyed. That _may_ be okay. I'd prefer to tie the EME code to WebMediaPlayer destruction than here. Being consistent with MSE isn't a good reason. I plan on doing some cleanup on the MSE code as well since, I believe there are closeMediaSource() calls that don't actually do anything now. https://codereview.chromium.org/157423003/diff/290001/Source/core/html/HTMLMe... Source/core/html/HTMLMediaElement.cpp:3120: closeMediaSource(); On 2014/02/27 00:37:59, ddorwin wrote: > ditto Why do you actually need to clear it here vs in the WMPCI destructor? https://codereview.chromium.org/157423003/diff/290001/Source/core/html/HTMLMe... Source/core/html/HTMLMediaElement.cpp:3419: On 2014/02/27 00:37:59, ddorwin wrote: > Where does the removed code get called now? There's no playerCreated() method. > > Since only WMPI knows about the CDM, it appears that this is entirely handled in > WebMediaPlayerClientImpl::loadInternal() when WMPI is created. It's handled in loadInternal(). Why do you need it to happen any earlier since before that, the actual WMPI doesn't exist? https://codereview.chromium.org/157423003/diff/290001/Source/core/html/HTMLMe... File Source/core/html/HTMLMediaElement.h (right): https://codereview.chromium.org/157423003/diff/290001/Source/core/html/HTMLMe... Source/core/html/HTMLMediaElement.h:84: blink::WebMediaPlayer* webMediaPlayer() const { return m_player ? m_player->webMediaPlayer() : 0; } On 2014/02/27 00:37:59, ddorwin wrote: > This seems fishy and like a layering violation, though I know we want to get rid > of the MediaPlayer layer. Why not just force the caller to get the WMP from the > result of player()? I asked him to put this here. The MediaPlayer layer is going away. I see no reason to keep the methods in the MediaPlayer interface. Ultimately all the code in HTMLMediaElement will be using a WebMediaPlayer* anyways so this is a step in that direction. https://codereview.chromium.org/157423003/diff/290001/Source/modules/encrypte... File Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp (right): https://codereview.chromium.org/157423003/diff/290001/Source/modules/encrypte... Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp:168: blink::WebMediaPlayer::MediaKeyException result = webMediaPlayer ? webMediaPlayer->generateKeyRequest(keySystem, initDataPointer, initDataLength) : blink::WebMediaPlayer::MediaKeyExceptionInvalidPlayerState; On 2014/02/27 00:37:59, ddorwin wrote: > This is too confusing and long. If we don't have a WMP, thrown an invalid state > exception directly (as in 156). > > The fact that we have to check both layers makes all this feel wrong. Ultimately, I think this code should just care about webMediaPlayer(). I'm guessing these hoops are just here to preserve exact behavior. If you don't care about exception order, this stuff could be handled in the static method instead of in here. https://codereview.chromium.org/157423003/diff/290001/Source/platform/graphic... File Source/platform/graphics/media/MediaPlayer.h (right): https://codereview.chromium.org/157423003/diff/290001/Source/platform/graphic... Source/platform/graphics/media/MediaPlayer.h:188: virtual blink::WebMediaPlayer* webMediaPlayer() const = 0; On 2014/02/27 00:37:59, ddorwin wrote: > This seems like a layering violation. Why can't this interface know about EME? > platform/ has a CDM insterface. This layer is going away. There is no point in keeping these. https://codereview.chromium.org/157423003/diff/290001/Source/web/WebMediaPlay... File Source/web/WebMediaPlayerClientImpl.h (right): https://codereview.chromium.org/157423003/diff/290001/Source/web/WebMediaPlay... Source/web/WebMediaPlayerClientImpl.h:84: // FIXME: Remove these once the WebMediaPlayer implementation stop making these calls on the WebMediaPlayerClient object. On 2014/02/27 00:37:59, ddorwin wrote: > Where should it be making them? This was left over from the original patch I sent him. This comment should be removed.
That's right, ddorwin. We can remove the encryptedmedia entries in core/DEPS. I will include this in the next patch. I am closing 2) and update the description in 1). Thanks Chang On 2/26/14 7:43 PM, "ddorwin@chromium.org" <ddorwin@chromium.org> wrote: >We have 3 bugs: >1) 341883: Remove dependency on MediaKeyNeededEvent for HTMLMediaElement >2) 342036: Extract EcryptedMediaEvent-related functions in >MediaPlayerClient >into a new class >3) 242754: Files in Source/core/ should not reference files in >Source/modules/encryptedmedia/ > >Does this address 2? If so, can we close 1 as a duplicate of 2? >Is this everything we need for 3? If so, we can update Source/core/DEPS >and >include #3 in the BUG= entry. > >https://codereview.chromium.org/157423003/ To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
Thanks for the review, guys. Most of the issues have been addressed. But a couple of them are pending. Can we go through them again? https://codereview.chromium.org/157423003/diff/290001/Source/core/html/HTMLMe... File Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/157423003/diff/290001/Source/core/html/HTMLMe... Source/core/html/HTMLMediaElement.cpp:325: So I won't touch MSE this time. https://codereview.chromium.org/157423003/diff/290001/Source/core/html/HTMLMe... Source/core/html/HTMLMediaElement.cpp:524: void HTMLMediaElement::scheduleEvent(const AtomicString& eventName) On 2014/02/27 00:37:59, ddorwin wrote: > Should we be overloading here? Maybe this should be scheduleNamedEvent() or > something like that. Done. https://codereview.chromium.org/157423003/diff/290001/Source/core/html/HTMLMe... Source/core/html/HTMLMediaElement.cpp:534: m_asyncEventQueue->enqueueEvent(event); All the scheduleNamedEvent() now go through this function. I don't see setTarget() in the original code path. Shall we change the behavior? https://codereview.chromium.org/157423003/diff/290001/Source/core/html/HTMLMe... Source/core/html/HTMLMediaElement.cpp:3419: Right. All the tests are passing, too. https://codereview.chromium.org/157423003/diff/290001/Source/modules/encrypte... File Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp (right): https://codereview.chromium.org/157423003/diff/290001/Source/modules/encrypte... Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp:56: HTMLMediaElementEncryptedMedia* supplement = static_cast<HTMLMediaElementEncryptedMedia*>(Supplement<HTMLMediaElement>::from(element, supplementName())); On 2014/02/27 00:37:59, ddorwin wrote: > Wouldn't it be better to do the case to this Supplement after we have a > supplement? Done. https://codereview.chromium.org/157423003/diff/290001/Source/modules/encrypte... Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp:87: WTF_LOG(Media, "HTMLMediaElementEncryptedMedia::setMediaKeys"); On 2014/02/27 00:37:59, ddorwin wrote: > Update function name and/or move to 105. Done. https://codereview.chromium.org/157423003/diff/290001/Source/modules/encrypte... Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp:156: if (!hasPlayer) { On 2014/02/27 00:37:59, ddorwin wrote: > Do we really need this parameter? If we instead check webMediaPlayer, we should > be fine (assuming we keep the layering violations). There are cases when the player is not null but webMediaPlayer is null. If we only check webMediaPlayer, the following test will fail. LayoutTests/media/encrypted-media/encrypted-media-not-loaded.html https://codereview.chromium.org/157423003/diff/290001/Source/modules/encrypte... Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp:168: blink::WebMediaPlayer::MediaKeyException result = webMediaPlayer ? webMediaPlayer->generateKeyRequest(keySystem, initDataPointer, initDataLength) : blink::WebMediaPlayer::MediaKeyExceptionInvalidPlayerState; On 2014/02/27 01:11:11, acolwell wrote: > On 2014/02/27 00:37:59, ddorwin wrote: > > This is too confusing and long. If we don't have a WMP, thrown an invalid > state > > exception directly (as in 156). > > > > The fact that we have to check both layers makes all this feel wrong. > > Ultimately, I think this code should just care about webMediaPlayer(). I'm > guessing these hoops are just here to preserve exact behavior. If you don't care > about exception order, this stuff could be handled in the static method instead > of in here. Right. If we can update the expected result, the code will be simpler. https://codereview.chromium.org/157423003/diff/290001/Source/web/WebMediaPlay... File Source/web/WebMediaPlayerClientImpl.cpp (right): https://codereview.chromium.org/157423003/diff/290001/Source/web/WebMediaPlay... Source/web/WebMediaPlayerClientImpl.cpp:172: void WebMediaPlayerClientImpl::closeHelperPluginSoon(WebFrame*) On 2014/02/27 00:37:59, ddorwin wrote: > That's not what the FIXME meant. You should just revert this because there are > CLs in flight that will conflict. Done. https://codereview.chromium.org/157423003/diff/290001/Source/web/WebMediaPlay... Source/web/WebMediaPlayerClientImpl.cpp:233: Frame* frame = static_cast<HTMLMediaElement*>(m_client)->document().frame(); On 2014/02/27 00:37:59, ddorwin wrote: > FYI, you didn't address acolwell's nit from PS5. You also didn't publish any > responses to his comments. Done. https://codereview.chromium.org/157423003/diff/290001/Source/web/WebMediaPlay... Source/web/WebMediaPlayerClientImpl.cpp:653: return *static_cast<WebCore::HTMLMediaElement*>(m_client); On 2014/02/27 00:37:59, ddorwin wrote: > WebCore:: is unnecessary. Done. https://codereview.chromium.org/157423003/diff/290001/Source/web/WebMediaPlay... File Source/web/WebMediaPlayerClientImpl.h (right): https://codereview.chromium.org/157423003/diff/290001/Source/web/WebMediaPlay... Source/web/WebMediaPlayerClientImpl.h:84: // FIXME: Remove these once the WebMediaPlayer implementation stop making these calls on the WebMediaPlayerClient object. On 2014/02/27 01:11:11, acolwell wrote: > On 2014/02/27 00:37:59, ddorwin wrote: > > Where should it be making them? > > This was left over from the original patch I sent him. This comment should be > removed. Done.
The new patch addressed all the comments from ddorwin that I am sure about. A couple of review comments are still pending for discussion.
Looking good. Thanks. https://codereview.chromium.org/157423003/diff/290001/Source/core/html/HTMLMe... File Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/157423003/diff/290001/Source/core/html/HTMLMe... Source/core/html/HTMLMediaElement.cpp:534: m_asyncEventQueue->enqueueEvent(event); On 2014/02/27 14:49:46, c.shu wrote: > All the scheduleNamedEvent() now go through this function. I don't see > setTarget() in the original code path. Shall we change the behavior? This was an unrelated comment, other than the fact that we're adding more schedule* methods. Don't worry about it in this CL. https://codereview.chromium.org/157423003/diff/290001/Source/core/html/HTMLMe... Source/core/html/HTMLMediaElement.cpp:3120: closeMediaSource(); On 2014/02/27 01:11:11, acolwell wrote: > On 2014/02/27 00:37:59, ddorwin wrote: > > ditto > Why do you actually need to clear it here vs in the WMPCI destructor? We used to have to clear m_mediaKeys. I believe this now happens as a callback when WMPCI is destroyed. https://codereview.chromium.org/157423003/diff/290001/Source/core/html/HTMLMe... Source/core/html/HTMLMediaElement.cpp:3419: On 2014/02/27 01:11:11, acolwell wrote: > On 2014/02/27 00:37:59, ddorwin wrote: > > Where does the removed code get called now? There's no playerCreated() method. > > > > Since only WMPI knows about the CDM, it appears that this is entirely handled > in > > WebMediaPlayerClientImpl::loadInternal() when WMPI is created. > > It's handled in loadInternal(). Why do you need it to happen any earlier since > before that, the actual WMPI doesn't exist? I was just checking my understanding. I should have been clearer. https://codereview.chromium.org/157423003/diff/290001/Source/modules/encrypte... File Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp (right): https://codereview.chromium.org/157423003/diff/290001/Source/modules/encrypte... Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp:156: if (!hasPlayer) { On 2014/02/27 14:49:46, c.shu wrote: > On 2014/02/27 00:37:59, ddorwin wrote: > > Do we really need this parameter? If we instead check webMediaPlayer, we > should > > be fine (assuming we keep the layering violations). > > There are cases when the player is not null but webMediaPlayer is null. If we > only check webMediaPlayer, the following test will fail. > LayoutTests/media/encrypted-media/encrypted-media-not-loaded.html In what way? Is the exception message different or something else? (It looks like it would at least change.) As far as the implementation is concerned, I don't think it matters. If it's just the exception message, please simplify per these comments and update the expected.txt file with the new message so we can review that. https://codereview.chromium.org/157423003/diff/290001/Source/modules/encrypte... Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp:168: blink::WebMediaPlayer::MediaKeyException result = webMediaPlayer ? webMediaPlayer->generateKeyRequest(keySystem, initDataPointer, initDataLength) : blink::WebMediaPlayer::MediaKeyExceptionInvalidPlayerState; On 2014/02/27 14:49:46, c.shu wrote: > On 2014/02/27 01:11:11, acolwell wrote: > > On 2014/02/27 00:37:59, ddorwin wrote: > > > This is too confusing and long. If we don't have a WMP, thrown an invalid > > state > > > exception directly (as in 156). > > > > > > The fact that we have to check both layers makes all this feel wrong. > > > > Ultimately, I think this code should just care about webMediaPlayer(). I'm > > guessing these hoops are just here to preserve exact behavior. If you don't > care > > about exception order, this stuff could be handled in the static method > instead > > of in here. > > Right. If we can update the expected result, the code will be simpler. Yes, please do. See above. https://codereview.chromium.org/157423003/diff/310001/Source/core/html/HTMLMe... File Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/157423003/diff/310001/Source/core/html/HTMLMe... Source/core/html/HTMLMediaElement.cpp:524: void HTMLMediaElement::scheduleNamedEvent(const AtomicString& eventName) Thank you for changing the name, but this introduced a lot of unrelated changes (that I didn't anticipate). Please revert this and add a FIXME to rename it. https://codereview.chromium.org/157423003/diff/310001/Source/core/html/HTMLMe... File Source/core/html/HTMLMediaElement.h (right): https://codereview.chromium.org/157423003/diff/310001/Source/core/html/HTMLMe... Source/core/html/HTMLMediaElement.h:83: MediaPlayer* player() const { return m_player.get(); } Should we add: // Do not use player(). // FIXME: Replace all uses with webMediaPlayer() and move this API. https://codereview.chromium.org/157423003/diff/310001/Source/modules/encrypte... File Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp (right): https://codereview.chromium.org/157423003/diff/310001/Source/modules/encrypte... Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp:58: supplement = new HTMLMediaElementEncryptedMedia(); Returning a reference to a value just created on the heap is weird. Who is going to delete this? Is this common among Supplementables? https://codereview.chromium.org/157423003/diff/310001/Source/modules/encrypte... Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp:156: if (!hasPlayer) { Reminder to myself: I still think we should get rid of this parameter. See comments in the previous PS.
FYI, we use a single BUG= entry with each number separated by commas. I fixed the description for you.
addressed more review comments. thanks. https://codereview.chromium.org/157423003/diff/290001/Source/modules/encrypte... File Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp (right): https://codereview.chromium.org/157423003/diff/290001/Source/modules/encrypte... Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp:141: HTMLMediaElementEncryptedMedia::from(element).generateKeyRequest(element.player(), element.webMediaPlayer(), keySystem, initData, exceptionState); On 2014/02/27 00:37:59, ddorwin wrote: > Would it be better to just pass the element and let the member function decide > what to do with it (rather than passing various players)? Done. https://codereview.chromium.org/157423003/diff/290001/Source/modules/encrypte... Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp:144: void HTMLMediaElementEncryptedMedia::generateKeyRequest(bool hasPlayer, blink::WebMediaPlayer* webMediaPlayer, const String& keySystem, PassRefPtr<Uint8Array> initData, ExceptionState& exceptionState) On 2014/02/27 00:37:59, ddorwin wrote: > I don't think we need |hasPlayer| - see below. Done. https://codereview.chromium.org/157423003/diff/290001/Source/modules/encrypte... Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp:156: if (!hasPlayer) { On 2014/03/01 01:08:47, ddorwin wrote: > On 2014/02/27 14:49:46, c.shu wrote: > > On 2014/02/27 00:37:59, ddorwin wrote: > > > Do we really need this parameter? If we instead check webMediaPlayer, we > > should > > > be fine (assuming we keep the layering violations). > > > > There are cases when the player is not null but webMediaPlayer is null. If we > > only check webMediaPlayer, the following test will fail. > > LayoutTests/media/encrypted-media/encrypted-media-not-loaded.html > > In what way? Is the exception message different or something else? (It looks > like it would at least change.) As far as the implementation is concerned, I > don't think it matters. If it's just the exception message, please simplify per > these comments and update the expected.txt file with the new message so we can > review that. Done. https://codereview.chromium.org/157423003/diff/290001/Source/modules/encrypte... Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp:168: blink::WebMediaPlayer::MediaKeyException result = webMediaPlayer ? webMediaPlayer->generateKeyRequest(keySystem, initDataPointer, initDataLength) : blink::WebMediaPlayer::MediaKeyExceptionInvalidPlayerState; On 2014/03/01 01:08:47, ddorwin wrote: > On 2014/02/27 14:49:46, c.shu wrote: > > On 2014/02/27 01:11:11, acolwell wrote: > > > On 2014/02/27 00:37:59, ddorwin wrote: > > > > This is too confusing and long. If we don't have a WMP, thrown an invalid > > > state > > > > exception directly (as in 156). > > > > > > > > The fact that we have to check both layers makes all this feel wrong. > > > > > > Ultimately, I think this code should just care about webMediaPlayer(). I'm > > > guessing these hoops are just here to preserve exact behavior. If you don't > > care > > > about exception order, this stuff could be handled in the static method > > instead > > > of in here. > > > > Right. If we can update the expected result, the code will be simpler. > > Yes, please do. See above. Done. https://codereview.chromium.org/157423003/diff/310001/Source/core/html/HTMLMe... File Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/157423003/diff/310001/Source/core/html/HTMLMe... Source/core/html/HTMLMediaElement.cpp:524: void HTMLMediaElement::scheduleNamedEvent(const AtomicString& eventName) On 2014/03/01 01:08:48, ddorwin wrote: > Thank you for changing the name, but this introduced a lot of unrelated changes > (that I didn't anticipate). Please revert this and add a FIXME to rename it. Done. https://codereview.chromium.org/157423003/diff/310001/Source/core/html/HTMLMe... File Source/core/html/HTMLMediaElement.h (right): https://codereview.chromium.org/157423003/diff/310001/Source/core/html/HTMLMe... Source/core/html/HTMLMediaElement.h:83: MediaPlayer* player() const { return m_player.get(); } On 2014/03/01 01:08:48, ddorwin wrote: > Should we add: > // Do not use player(). > // FIXME: Replace all uses with webMediaPlayer() and move this API. Done. https://codereview.chromium.org/157423003/diff/310001/Source/modules/encrypte... File Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp (right): https://codereview.chromium.org/157423003/diff/310001/Source/modules/encrypte... Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp:58: supplement = new HTMLMediaElementEncryptedMedia(); On 2014/03/01 01:08:48, ddorwin wrote: > Returning a reference to a value just created on the heap is weird. Who is going > to delete this? Is this common among Supplementables? The pointer is passed to provideTo() as a passOwnPtr and Supplementable saves it in its map. I see all other instances of ::from() are coded in this way.
LGTM. Thank you!!
The CQ bit was checked by c.shu@samsung.com
The CQ bit was unchecked by c.shu@samsung.com
lgtm % nits https://codereview.chromium.org/157423003/diff/330001/Source/modules/encrypte... File Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp (right): https://codereview.chromium.org/157423003/diff/330001/Source/modules/encrypte... Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp:90: ASSERT(m_emeMode = EmeModeUnprefixed); nit: s/=/==/? https://codereview.chromium.org/157423003/diff/330001/Source/modules/encrypte... Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp:217: throwExceptionForMediaKeyException(keySystem, sessionId, result, exceptionState); nit: This is a little deceiving because the name implies to me that an exception is always thrown by this method. How about s/throwExceptionForMediaKeyException/throwExceptionIfMediaKeyExceptionOccurred/? Either that or call this method only when result != MediaKeyExceptionNoError. Same goes for other instances in this file.
https://codereview.chromium.org/157423003/diff/330001/Source/modules/encrypte... File Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp (right): https://codereview.chromium.org/157423003/diff/330001/Source/modules/encrypte... Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp:217: throwExceptionForMediaKeyException(keySystem, sessionId, result, exceptionState); On 2014/03/06 01:31:53, acolwell wrote: > nit: This is a little deceiving because the name implies to me that an exception > is always thrown by this method. How about > s/throwExceptionForMediaKeyException/throwExceptionIfMediaKeyExceptionOccurred/? > Either that or call this method only when result != MediaKeyExceptionNoError. > Same goes for other instances in this file. This was copied from the old code. Shall we keep the name and add a FIXME so the amount of changes in this move are minimized?
Addressed some minor issues.
On 2014/03/06 02:01:18, ddorwin wrote: > https://codereview.chromium.org/157423003/diff/330001/Source/modules/encrypte... > File Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp (right): > > https://codereview.chromium.org/157423003/diff/330001/Source/modules/encrypte... > Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp:217: > throwExceptionForMediaKeyException(keySystem, sessionId, result, > exceptionState); > On 2014/03/06 01:31:53, acolwell wrote: > > nit: This is a little deceiving because the name implies to me that an > exception > > is always thrown by this method. How about > > > s/throwExceptionForMediaKeyException/throwExceptionIfMediaKeyExceptionOccurred/? > > Either that or call this method only when result != MediaKeyExceptionNoError. > > Same goes for other instances in this file. > > This was copied from the old code. Shall we keep the name and add a FIXME so the > amount of changes in this move are minimized? I made the change anyway. This function is local so the impact is minor.
Source/core and Source/web LGTM Thanks! https://codereview.chromium.org/157423003/diff/350001/Source/core/DEPS File Source/core/DEPS (left): https://codereview.chromium.org/157423003/diff/350001/Source/core/DEPS#oldcode13 Source/core/DEPS:13: "!modules/encryptedmedia/MediaKeys.h", Yay! https://codereview.chromium.org/157423003/diff/350001/Source/core/html/HTMLMe... File Source/core/html/HTMLMediaElement.h (right): https://codereview.chromium.org/157423003/diff/350001/Source/core/html/HTMLMe... Source/core/html/HTMLMediaElement.h:83: // Do not use player(). Should we rename player() to deprecatedPlayer()? We can do that in a separate CL. https://codereview.chromium.org/157423003/diff/350001/Source/core/html/HTMLMe... Source/core/html/HTMLMediaElement.h:363: void scheduleEvent(const AtomicString& eventName); // FIXME: Rename to scheduleNamedEvent for clarity. Are you planning to do this in a followup CL?
The CQ bit was checked by abarth@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/c.shu@samsung.com/157423003/350001
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for
LayoutTests/media/encrypted-media/encrypted-media-not-loaded-expected.txt:
While running patch -p1 --forward --force --no-backup-if-mismatch;
can't find file to patch at input line 6
Perhaps you used the wrong -p or --strip option?
The text leading up to this was:
--------------------------
|Index:
LayoutTests/media/encrypted-media/encrypted-media-not-loaded-expected.txt
|diff --git
a/LayoutTests/media/encrypted-media/encrypted-media-not-loaded-expected.txt
b/LayoutTests/media/encrypted-media/encrypted-media-not-loaded-expected.txt
|index
5c0f9bfe846c028c65e59a9faa71fa55de3f2185..820d648e258ea8dd4c4b6d0f82d9a31ed20d73fc
100644
|---
a/LayoutTests/media/encrypted-media/encrypted-media-not-loaded-expected.txt
|+++
b/LayoutTests/media/encrypted-media/encrypted-media-not-loaded-expected.txt
--------------------------
No file to patch. Skipping patch.
1 out of 1 hunk ignored
Patch:
LayoutTests/media/encrypted-media/encrypted-media-not-loaded-expected.txt
Index: LayoutTests/media/encrypted-media/encrypted-media-not-loaded-expected.txt
diff --git
a/LayoutTests/media/encrypted-media/encrypted-media-not-loaded-expected.txt
b/LayoutTests/media/encrypted-media/encrypted-media-not-loaded-expected.txt
index
5c0f9bfe846c028c65e59a9faa71fa55de3f2185..820d648e258ea8dd4c4b6d0f82d9a31ed20d73fc
100644
--- a/LayoutTests/media/encrypted-media/encrypted-media-not-loaded-expected.txt
+++ b/LayoutTests/media/encrypted-media/encrypted-media-not-loaded-expected.txt
@@ -11,9 +11,9 @@ RUN(video.canPlayType('audio/wav', 'webkit-org.w3.clearkey'))
'src' has been set, but loading has not yet started. Verify that all methods
except canPlayType() throw INVALID_STATE_ERR.
EXPECTED (video.networkState == '3') OK
EXPECTED (video.currentSrc == '') OK
-TEST(video.webkitGenerateKeyRequest('webkit-org.w3.clearkey'))
THROWS(DOMException.INVALID_STATE_ERR: Failed to execute
'webkitGenerateKeyRequest' on 'HTMLMediaElement': The player is in an invalid
state.) OK
-TEST(video.webkitAddKey('webkit-org.w3.clearkey', key))
THROWS(DOMException.INVALID_STATE_ERR: Failed to execute 'webkitAddKey' on
'HTMLMediaElement': The player is in an invalid state.) OK
-TEST(video.webkitCancelKeyRequest('webkit-org.w3.clearkey'))
THROWS(DOMException.INVALID_STATE_ERR: Failed to execute
'webkitCancelKeyRequest' on 'HTMLMediaElement': The player is in an invalid
state.) OK
+TEST(video.webkitGenerateKeyRequest('webkit-org.w3.clearkey'))
THROWS(DOMException.INVALID_STATE_ERR: Failed to execute
'webkitGenerateKeyRequest' on 'HTMLMediaElement': No media has been loaded.) OK
+TEST(video.webkitAddKey('webkit-org.w3.clearkey', key))
THROWS(DOMException.INVALID_STATE_ERR: Failed to execute 'webkitAddKey' on
'HTMLMediaElement': No media has been loaded.) OK
+TEST(video.webkitCancelKeyRequest('webkit-org.w3.clearkey'))
THROWS(DOMException.INVALID_STATE_ERR: Failed to execute
'webkitCancelKeyRequest' on 'HTMLMediaElement': No media has been loaded.) OK
RUN(video.canPlayType('audio/wav', 'webkit-org.w3.clearkey'))
EVENT(loadstart)
Thanks for the review. https://codereview.chromium.org/157423003/diff/350001/Source/core/html/HTMLMe... File Source/core/html/HTMLMediaElement.h (right): https://codereview.chromium.org/157423003/diff/350001/Source/core/html/HTMLMe... Source/core/html/HTMLMediaElement.h:83: // Do not use player(). On 2014/03/06 17:27:14, abarth wrote: > Should we rename player() to deprecatedPlayer()? We can do that in a separate > CL. Good suggestion! https://codereview.chromium.org/157423003/diff/350001/Source/core/html/HTMLMe... Source/core/html/HTMLMediaElement.h:363: void scheduleEvent(const AtomicString& eventName); // FIXME: Rename to scheduleNamedEvent for clarity. On 2014/03/06 17:27:14, abarth wrote: > Are you planning to do this in a followup CL? sure, I can. I will coordinate with acolwel and ddorwin on the follow-ups.
The CQ bit was checked by c.shu@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/c.shu@samsung.com/157423003/370001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_blink_rel
The CQ bit was checked by c.shu@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/c.shu@samsung.com/157423003/380001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_blink
The CQ bit was checked by c.shu@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/c.shu@samsung.com/157423003/380001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_blink
The CQ bit was checked by abarth@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/c.shu@samsung.com/157423003/380001
Message was sent while issue was closed.
Change committed as 168704 |
