Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1055503002:
Eliminate MediaPlayer & MediaPlayerClient abstractions (Closed)
|
Created:
5 years, 8 months ago by Srirama Modified:
5 years, 5 months ago CC:
blink-reviews, blink-reviews-html_chromium.org, Rik, danakj, dglazkov+blink, Dominik Röttsches, dshwang, krit, eric.carlson_apple.com, feature-media-reviews_chromium.org, f(malita), gasubic, jbroman, Justin Novosad, pdr+graphicswatchlist_chromium.org, rwlbuis, Raymond Toy, Stephen Chennney, nessy, vcarbune.chromium, wolenetz, mlamouri (slow - plz ping), oilpan-reviews Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
DescriptionEliminate MediaPlayer & MediaPlayerClient abstractions
This patch is to remove MediaPlayer and MediaPlayerClient interfaces
completely and let HTMLMediaElement talk directly to webmediaplayer
implementations by implementing the WebMediaPlayerClient interface.
BUG=350571
Chromium CL: https://codereview.chromium.org/1133033003/
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=199375
Patch Set 1 #
Total comments: 8
Patch Set 2 : Move createMediaPlayer() to FrameLoaderClient to avoid deps rule violation #
Total comments: 2
Patch Set 3 : Added separate interface for EncryptedMedia #
Total comments: 44
Patch Set 4 : Patch cleanup and review comments fix #
Total comments: 4
Patch Set 5 : added new path for createMediaPlayer #
Total comments: 6
Patch Set 6 : Fixed review comments #
Total comments: 1
Patch Set 7 : Fixed layout test failures #Patch Set 8 : Rebase and code cleanup #
Total comments: 39
Patch Set 9 : Address comments #
Total comments: 26
Patch Set 10 : fixed review comments #Patch Set 11 : Comments fix and Rebase #Patch Set 12 : Fix for compile issue in win_blink_compile_dgb #Patch Set 13 : Rebase #Patch Set 14 : Use BLINK_PLATFORM_EXPORT for WebMediaPlayerEncryptedMediaClient class #Patch Set 15 : Fix linking error #
Total comments: 10
Patch Set 16 : Addressed philip's comments #
Total comments: 3
Patch Set 17 : address comments #Messages
Total messages: 93 (20 generated)
srirama.m@samsung.com changed reviewers: + philipj@opera.com
Hi Philip, can you please have a look at high level and tell me if this is the way to go. I just made these changes to make sure it compiles and runs well, though there are some issues. I will be out of office for 4 days and will be back on monday to resume it. So please just have a quick glance whenever you get time.
https://codereview.chromium.org/1055503002/diff/1/Source/core/html/HTMLMediaE... File Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/1055503002/diff/1/Source/core/html/HTMLMediaE... Source/core/html/HTMLMediaElement.cpp:66: #include "modules/encryptedmedia/HTMLMediaElementEncryptedMedia.h" This is violation of include_rules, so is it the right way and do we need to change the include rules or do i need to change the design to obey the existing include_rules?
Sorry for the slow review, I was home sick for the past two weeks. This looks really promising, finally getting rid of this layer. Getting the EME dependency right is the blocker. https://codereview.chromium.org/1055503002/diff/1/Source/core/html/HTMLMediaE... File Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/1055503002/diff/1/Source/core/html/HTMLMediaE... Source/core/html/HTMLMediaElement.cpp:66: #include "modules/encryptedmedia/HTMLMediaElementEncryptedMedia.h" On 2015/04/01 13:03:06, Srirama wrote: > This is violation of include_rules, so is it the right way and do we need to > change the include rules or do i need to change the design to obey the existing > include_rules? This will need to be refactored in some way avoid includes modules from core. I don't exactly know how, though. Perhaps the EME bits need a separate client interface implemented by HTMLMediaElementEncryptedMedia so that these calls don't go through HTMLMediaElement at all. Perhaps ddworin has ideas for this. https://codereview.chromium.org/1055503002/diff/1/Source/core/testing/Interna... File Source/core/testing/Internals.cpp (right): https://codereview.chromium.org/1055503002/diff/1/Source/core/testing/Interna... Source/core/testing/Internals.cpp:1705: mediaElement->requestFullscreen(); What is this change for? Does LayoutTests/media/media-player-request-fullscreen.html still pass? If so that test must be broken because it should verify that it goes fullscreen even without a user gesture.
Thanks for the quick review. I will clean up the patch by removing commented code and other unnecessary stuff and will look into EME thing a bit more before asking ddorwin. https://codereview.chromium.org/1055503002/diff/1/Source/core/html/HTMLMediaE... File Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/1055503002/diff/1/Source/core/html/HTMLMediaE... Source/core/html/HTMLMediaElement.cpp:66: #include "modules/encryptedmedia/HTMLMediaElementEncryptedMedia.h" On 2015/04/14 08:42:54, philipj_UTC2 wrote: > On 2015/04/01 13:03:06, Srirama wrote: > > This is violation of include_rules, so is it the right way and do we need to > > change the include rules or do i need to change the design to obey the > existing > > include_rules? > > This will need to be refactored in some way avoid includes modules from core. I > don't exactly know how, though. Perhaps the EME bits need a separate client > interface implemented by HTMLMediaElementEncryptedMedia so that these calls > don't go through HTMLMediaElement at all. Perhaps ddworin has ideas for this. ok, i will check with ddorwin for this. https://codereview.chromium.org/1055503002/diff/1/Source/core/testing/Interna... File Source/core/testing/Internals.cpp (right): https://codereview.chromium.org/1055503002/diff/1/Source/core/testing/Interna... Source/core/testing/Internals.cpp:1705: mediaElement->requestFullscreen(); On 2015/04/14 08:42:54, philipj_UTC2 wrote: > What is this change for? Does > LayoutTests/media/media-player-request-fullscreen.html still pass? If so that > test must be broken because it should verify that it goes fullscreen even > without a user gesture. This is just renaming of the API (mediaPlayerRequestFullscreen is renamed to requestFullscreen as HTMLMediaElement is now implementing WebMediaPlayerClient instead of MediaPlayerClient)
https://codereview.chromium.org/1055503002/diff/1/Source/core/html/HTMLMediaE... File Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/1055503002/diff/1/Source/core/html/HTMLMediaE... Source/core/html/HTMLMediaElement.cpp:81: #include "public/web/WebFrameClient.h" Even public/web/ and web/ are excluded from code module. I was checking the deps files if there is any other way of passing the webframe to chromium side but couldn't get any. Do i need to figure out a way to remove the dependency on render_frame_impl.cc as well or i need to keep that as it is and find a way to go from HTMLMediaElement to render_frame_impl by obeying the include rules? Your quick thoughts on this please
fs@opera.com changed reviewers: + fs@opera.com
https://codereview.chromium.org/1055503002/diff/1/Source/core/html/HTMLMediaE... File Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/1055503002/diff/1/Source/core/html/HTMLMediaE... Source/core/html/HTMLMediaElement.cpp:81: #include "public/web/WebFrameClient.h" On 2015/04/22 07:42:11, Srirama wrote: > Even public/web/ and web/ are excluded from code module. I was checking the deps > files if there is any other way of passing the webframe to chromium side but > couldn't get any. Do i need to figure out a way to remove the dependency on > render_frame_impl.cc as well or i need to keep that as it is and find a way to > go from HTMLMediaElement to render_frame_impl by obeying the include rules? > Your quick thoughts on this please I think the "Right Way(TM)" here is to move this bit (createMediaPlayer) to FrameLoaderClient. Does mean some plumbing.
https://codereview.chromium.org/1055503002/diff/1/Source/core/html/HTMLMediaE... File Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/1055503002/diff/1/Source/core/html/HTMLMediaE... Source/core/html/HTMLMediaElement.cpp:81: #include "public/web/WebFrameClient.h" On 2015/04/22 08:41:53, fs wrote: > On 2015/04/22 07:42:11, Srirama wrote: > > Even public/web/ and web/ are excluded from code module. I was checking the > deps > > files if there is any other way of passing the webframe to chromium side but > > couldn't get any. Do i need to figure out a way to remove the dependency on > > render_frame_impl.cc as well or i need to keep that as it is and find a way to > > go from HTMLMediaElement to render_frame_impl by obeying the include rules? > > Your quick thoughts on this please > > I think the "Right Way(TM)" here is to move this bit (createMediaPlayer) to > FrameLoaderClient. Does mean some plumbing. Thanks for the quick reply. I will do that way.
Patchset #2 (id:20001) has been deleted
srirama.m@samsung.com changed reviewers: + ddorwin@chromium.org
@ddorwin: can you please have a quick look and suggest me how to separate out the APIs related to HTMLMediaElementEncryptedMedia from WebMediaPlayerClient.
https://codereview.chromium.org/1055503002/diff/40001/Source/core/html/HTMLMe... File Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/1055503002/diff/40001/Source/core/html/HTMLMe... Source/core/html/HTMLMediaElement.cpp:67: #include "modules/encryptedmedia/HTMLMediaElementEncryptedMedia.h" This is not allowed. https://codereview.chromium.org/1055503002/diff/40001/Source/core/html/HTMLMe... File Source/core/html/HTMLMediaElement.h (right): https://codereview.chromium.org/1055503002/diff/40001/Source/core/html/HTMLMe... Source/core/html/HTMLMediaElement.h:73: class CORE_EXPORT HTMLMediaElement : public HTMLElement, public WillBeHeapSupplementable<HTMLMediaElement>, public WebMediaPlayerClient, public ActiveDOMObject { The problem is that WebMediaPlayerClient includes methods for features that are in implemented in both core/ and modules/. I don't know what the preferred Blink solution for this is. Separate interfaces is one, but it seems silly to have to expose the modules concept (and exact module boundaries) to Chromium in this way.
Patchset #3 (id:60001) has been deleted
Patchset #3 (id:80001) has been deleted
1) Added a new interface WebMediaPlayerEncryptedMediaClient.h 2) Implemented it in HTMLMediaElementEncryptedMedia.cpp 3) Added a new client parameter to createMediaPlayer API to make a direct link from WebMediaPlayer to HTMLMediaElementEncryptedMedia Please have a quick look and tell me if this is fine so that i can go ahead.
Some comments. I haven't gotten through all of the changes yet. https://codereview.chromium.org/1055503002/diff/100001/Source/core/html/HTMLM... File Source/core/html/HTMLMediaElement.cpp (left): https://codereview.chromium.org/1055503002/diff/100001/Source/core/html/HTMLM... Source/core/html/HTMLMediaElement.cpp:1036: m_player->setPreload(m_preload); The code doesn't looks quite like this any more, so you need to rebase. https://codereview.chromium.org/1055503002/diff/100001/Source/core/html/HTMLM... File Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/1055503002/diff/100001/Source/core/html/HTMLM... Source/core/html/HTMLMediaElement.cpp:555: if (!autoplay()) Is dropping the m_player bit entirely equivalent? The startDeferredLoad() call could now happen in cases where it couldn't before. It looks OK to me, but if you're feeling risk-averse you may want to do simplifications like this separately, before or after this CL. https://codereview.chromium.org/1055503002/diff/100001/Source/core/html/HTMLM... Source/core/html/HTMLMediaElement.cpp:1789: // FIXME: remove m_webMediaPlayer check once we figure out how m_webMediaPlayer is going It seems somewhat likely that this problem will magically go away with your changes. https://codereview.chromium.org/1055503002/diff/100001/Source/core/html/HTMLM... Source/core/html/HTMLMediaElement.cpp:1861: if (potentiallyPlaying()) This doesn't quite jive with the FIXME in HTMLMediaElement::duration(). There the problem is that readyState is out of sync with the player, but here we're assuming that they are in sync and will crash otherwise. To be conservative, maybe keep the player check and add a FIXME to be dealt with at the same time as the above. https://codereview.chromium.org/1055503002/diff/100001/Source/core/html/HTMLM... Source/core/html/HTMLMediaElement.cpp:2727: // FIXME: Change MediaPlayerClient & WebMediaPlayer to convey MediaPlayerClient is gone. https://codereview.chromium.org/1055503002/diff/100001/Source/core/html/HTMLM... Source/core/html/HTMLMediaElement.cpp:2768: void HTMLMediaElement::requestFullscreen() This name is a bit tricky, it could be seen as an override for Element::requestFullscreen() which it certainly is not. The old name is better if there's no problem in keeping it. https://codereview.chromium.org/1055503002/diff/100001/Source/core/html/HTMLM... Source/core/html/HTMLMediaElement.cpp:2779: void HTMLMediaElement::requestSeek(double time) I think in this case too the old name is good because it makes it clear that it's a request from the Chromium side as opposed to internal is normally the case for seeking (and fullscreen). https://codereview.chromium.org/1055503002/diff/100001/Source/core/html/HTMLM... Source/core/html/HTMLMediaElement.cpp:3651: void HTMLMediaElement::loadMediaPlayer(const WTF::String& url) loadMediaPlayer() as a name isn't only part of what this does. How about createMediaPlayer() or prepareMediaPlayer(), with the actual load() call pulled out to after the call to this method? That makes the enterFullscreen() call look strange (before the load) but in practice it's already strange. Maybe it should be delayed until there's actually something to play. https://codereview.chromium.org/1055503002/diff/100001/Source/core/html/HTMLM... File Source/core/html/HTMLMediaElement.h (right): https://codereview.chromium.org/1055503002/diff/100001/Source/core/html/HTMLM... Source/core/html/HTMLMediaElement.h:69: // FIXME: The inheritance from MediaPlayerClient here should be private inheritance. This FIXME can be removed now. Yay! https://codereview.chromium.org/1055503002/diff/100001/Source/core/html/HTMLM... Source/core/html/HTMLMediaElement.h:86: blink::WebMediaPlayer* webMediaPlayer() const blink:: prefix not needed: https://codereview.chromium.org/1115553002 https://codereview.chromium.org/1055503002/diff/100001/Source/core/html/HTMLM... Source/core/html/HTMLMediaElement.h:218: bool hasSingleSecurityOrigin() const { return webMediaPlayer() && webMediaPlayer()->hasSingleSecurityOrigin(); } I think we've discussed this in another review. This is a change for the case where there is no media player, but it shouldn't matter and the new code is more readable IMHO. https://codereview.chromium.org/1055503002/diff/100001/Source/core/html/HTMLM... Source/core/html/HTMLMediaElement.h:630: OwnPtr<WebMediaPlayer> m_webMediaPlayer; Putting this where m_player was would be better I think. (Ordering is hard, but adding new stuff to the end is often not the best place.)
Thanks, I assume the approach is fine to go ahead :). I will address all the comments. I am going to start the below changes as well, which are required for this change. 1) Add a new createMediaPlayer() method in WebFrameClient.h with extra parameter of WebMediaPlayerEncryptedMediaClient* 2) Implement the above method in render_frame_impl.cc 3) Add new constructors in WebMediaPlayer_impl and WebMediaPlayer_Android to accept additional client for WebMediaPlayerEncryptedMediaClient*
https://codereview.chromium.org/1055503002/diff/100001/Source/modules/encrypt... File Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp (right): https://codereview.chromium.org/1055503002/diff/100001/Source/modules/encrypt... Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp:267: supplement = new HTMLMediaElementEncryptedMedia(); Can we pass the element here and store it as a member to avoid having to pass the client and static_cast it? https://codereview.chromium.org/1055503002/diff/100001/Source/modules/encrypt... Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp:453: HTMLMediaElement& element = *(static_cast<HTMLMediaElement*>(client)); Hopefully we can avoid this. See above. https://codereview.chromium.org/1055503002/diff/100001/Source/modules/encrypt... Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp:545: void HTMLMediaElementEncryptedMedia::didBlockPlaybackWaitingForKey(WebMediaPlayerClient* client) Don't need this parameter. https://codereview.chromium.org/1055503002/diff/100001/Source/modules/encrypt... Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp:554: HTMLMediaElement& element = *(static_cast<HTMLMediaElement*>(client)); We don't need any of this because this is no longer static. https://codereview.chromium.org/1055503002/diff/100001/Source/modules/encrypt... Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp:573: void HTMLMediaElementEncryptedMedia::didResumePlaybackBlockedForKey(WebMediaPlayerClient* client) ditto https://codereview.chromium.org/1055503002/diff/100001/Source/modules/encrypt... File Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.h (right): https://codereview.chromium.org/1055503002/diff/100001/Source/modules/encrypt... Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.h:13: #include "public/platform/WebContentDecryptionModule.h" Can we fwd declare instead? https://codereview.chromium.org/1055503002/diff/100001/Source/modules/encrypt... Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.h:15: #include "public/platform/WebMediaPlayer.h" What is this used for? https://codereview.chromium.org/1055503002/diff/100001/Source/modules/encrypt... Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.h:54: static WebContentDecryptionModule* contentDecryptionModule(HTMLMediaElement&); Why is this still static? The call site makes the strangeness clearer. https://codereview.chromium.org/1055503002/diff/100001/Source/web/FrameLoader... File Source/web/FrameLoaderClientImpl.cpp (right): https://codereview.chromium.org/1055503002/diff/100001/Source/web/FrameLoader... Source/web/FrameLoaderClientImpl.cpp:803: return adoptPtr(webFrame->client()->createMediaPlayer(webFrame, url, static_cast<WebMediaPlayerClient*>(htmlMediaElement), nit: Put this complex last parameter on its own line, similar to below. https://codereview.chromium.org/1055503002/diff/100001/Source/web/FrameLoader... Source/web/FrameLoaderClientImpl.cpp:805: HTMLMediaElementEncryptedMedia::contentDecryptionModule(*htmlMediaElement))); As noted at the definition, I think it makes sense to get the HMEEM using from() then use it to call cDM().
Going to blinkOn4. I will address all the comments once i am back.
Patchset #4 (id:120001) has been deleted
Patchset #4 (id:140001) has been deleted
Patchset #4 (id:160001) has been deleted
Thanks for the review. I have cleaned up the patch and addressed all the comments to the best of my ability. This is a complete patch and i have uploaded the chromium changes as well. https://codereview.chromium.org/1133033003/ With all these changes, there are few layout issues which i will work and fix them. Regressions: Unexpected image-only failures (1) media/controls-after-unload.html [ ImageOnlyFailure ] Regressions: Unexpected crashes (6) media/avtrack/forget-on-load.html [ Crash ] media/video-source-error.html [ Crash ] media/video-source-load.html [ Crash ] media/video-source-media.html [ Crash ] media/video-source-moved.html [ Crash ] media/video-source-removed.html [ Crash ] Regressions: Unexpected timeouts (1) media/encrypted-media/encrypted-media-playback-encrypted-and-clear-sources.html [ Timeout ] https://codereview.chromium.org/1055503002/diff/100001/Source/core/html/HTMLM... File Source/core/html/HTMLMediaElement.cpp (left): https://codereview.chromium.org/1055503002/diff/100001/Source/core/html/HTMLM... Source/core/html/HTMLMediaElement.cpp:1036: m_player->setPreload(m_preload); On 2015/05/06 15:39:33, philipj_UTC2 wrote: > The code doesn't looks quite like this any more, so you need to rebase. Rebased to latest. https://codereview.chromium.org/1055503002/diff/100001/Source/core/html/HTMLM... File Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/1055503002/diff/100001/Source/core/html/HTMLM... Source/core/html/HTMLMediaElement.cpp:555: if (!autoplay()) On 2015/05/06 15:39:32, philipj_UTC2 wrote: > Is dropping the m_player bit entirely equivalent? The startDeferredLoad() call > could now happen in cases where it couldn't before. It looks OK to me, but if > you're feeling risk-averse you may want to do simplifications like this > separately, before or after this CL. I have added the webmediaplayer check to be on safe side, i will work on these kind of things after finishing this CL. https://codereview.chromium.org/1055503002/diff/100001/Source/core/html/HTMLM... Source/core/html/HTMLMediaElement.cpp:1789: // FIXME: remove m_webMediaPlayer check once we figure out how m_webMediaPlayer is going On 2015/05/06 15:39:33, philipj_UTC2 wrote: > It seems somewhat likely that this problem will magically go away with your > changes. keeping it for time being as per the below comment. https://codereview.chromium.org/1055503002/diff/100001/Source/core/html/HTMLM... Source/core/html/HTMLMediaElement.cpp:1861: if (potentiallyPlaying()) On 2015/05/06 15:39:32, philipj_UTC2 wrote: > This doesn't quite jive with the FIXME in HTMLMediaElement::duration(). There > the problem is that readyState is out of sync with the player, but here we're > assuming that they are in sync and will crash otherwise. To be conservative, > maybe keep the player check and add a FIXME to be dealt with at the same time as > the above. Logically what you were saying is correct, in both cases the check should be there, but previously when i was working on these changes we got a clusterfuzz crash in the above case so we have added a check there only. Anyway i have done the same thing here as well to be consistent. https://codereview.chromium.org/1055503002/diff/100001/Source/core/html/HTMLM... Source/core/html/HTMLMediaElement.cpp:2727: // FIXME: Change MediaPlayerClient & WebMediaPlayer to convey On 2015/05/06 15:39:32, philipj_UTC2 wrote: > MediaPlayerClient is gone. Done. https://codereview.chromium.org/1055503002/diff/100001/Source/core/html/HTMLM... Source/core/html/HTMLMediaElement.cpp:2768: void HTMLMediaElement::requestFullscreen() On 2015/05/06 15:39:32, philipj_UTC2 wrote: > This name is a bit tricky, it could be seen as an override for > Element::requestFullscreen() which it certainly is not. The old name is better > if there's no problem in keeping it. It is like this because we are overriding the function from WebMediaPlayerClient. Now I have changed the name in both places. Now these two functions start with mediaPlayer* where as all other functions don't in WebMediaPlayerClient. Is it ok? https://codereview.chromium.org/1055503002/diff/100001/Source/core/html/HTMLM... Source/core/html/HTMLMediaElement.cpp:2779: void HTMLMediaElement::requestSeek(double time) On 2015/05/06 15:39:32, philipj_UTC2 wrote: > I think in this case too the old name is good because it makes it clear that > it's a request from the Chromium side as opposed to internal is normally the > case for seeking (and fullscreen). Same as above. https://codereview.chromium.org/1055503002/diff/100001/Source/core/html/HTMLM... Source/core/html/HTMLMediaElement.cpp:3651: void HTMLMediaElement::loadMediaPlayer(const WTF::String& url) On 2015/05/06 15:39:33, philipj_UTC2 wrote: > loadMediaPlayer() as a name isn't only part of what this does. How about > createMediaPlayer() or prepareMediaPlayer(), with the actual load() call pulled > out to after the call to this method? > > That makes the enterFullscreen() call look strange (before the load) but in > practice it's already strange. Maybe it should be delayed until there's actually > something to play. I renamed it to prepareAndLoadMediaPlayer to be consistent with what it does. I will take up the simplifications later if requried as i don't want to change the order of load() and enterFullScreen() just to be on safe side. https://codereview.chromium.org/1055503002/diff/100001/Source/core/html/HTMLM... File Source/core/html/HTMLMediaElement.h (right): https://codereview.chromium.org/1055503002/diff/100001/Source/core/html/HTMLM... Source/core/html/HTMLMediaElement.h:69: // FIXME: The inheritance from MediaPlayerClient here should be private inheritance. On 2015/05/06 15:39:33, philipj_UTC2 wrote: > This FIXME can be removed now. Yay! Done. https://codereview.chromium.org/1055503002/diff/100001/Source/core/html/HTMLM... Source/core/html/HTMLMediaElement.h:86: blink::WebMediaPlayer* webMediaPlayer() const On 2015/05/06 15:39:33, philipj_UTC2 wrote: > blink:: prefix not needed: https://codereview.chromium.org/1115553002 Done. https://codereview.chromium.org/1055503002/diff/100001/Source/core/html/HTMLM... Source/core/html/HTMLMediaElement.h:218: bool hasSingleSecurityOrigin() const { return webMediaPlayer() && webMediaPlayer()->hasSingleSecurityOrigin(); } On 2015/05/06 15:39:33, philipj_UTC2 wrote: > I think we've discussed this in another review. This is a change for the case > where there is no media player, but it shouldn't matter and the new code is more > readable IMHO. Acknowledged. https://codereview.chromium.org/1055503002/diff/100001/Source/core/html/HTMLM... Source/core/html/HTMLMediaElement.h:630: OwnPtr<WebMediaPlayer> m_webMediaPlayer; On 2015/05/06 15:39:33, philipj_UTC2 wrote: > Putting this where m_player was would be better I think. (Ordering is hard, but > adding new stuff to the end is often not the best place.) Done. https://codereview.chromium.org/1055503002/diff/100001/Source/modules/encrypt... File Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp (right): https://codereview.chromium.org/1055503002/diff/100001/Source/modules/encrypt... Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp:267: supplement = new HTMLMediaElementEncryptedMedia(); On 2015/05/08 17:49:41, ddorwin wrote: > Can we pass the element here and store it as a member to avoid having to pass > the client and static_cast it? Done. https://codereview.chromium.org/1055503002/diff/100001/Source/modules/encrypt... Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp:453: HTMLMediaElement& element = *(static_cast<HTMLMediaElement*>(client)); On 2015/05/08 17:49:41, ddorwin wrote: > Hopefully we can avoid this. See above. Done. https://codereview.chromium.org/1055503002/diff/100001/Source/modules/encrypt... Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp:545: void HTMLMediaElementEncryptedMedia::didBlockPlaybackWaitingForKey(WebMediaPlayerClient* client) On 2015/05/08 17:49:41, ddorwin wrote: > Don't need this parameter. Done. https://codereview.chromium.org/1055503002/diff/100001/Source/modules/encrypt... Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp:554: HTMLMediaElement& element = *(static_cast<HTMLMediaElement*>(client)); On 2015/05/08 17:49:41, ddorwin wrote: > We don't need any of this because this is no longer static. Done. https://codereview.chromium.org/1055503002/diff/100001/Source/modules/encrypt... Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp:573: void HTMLMediaElementEncryptedMedia::didResumePlaybackBlockedForKey(WebMediaPlayerClient* client) On 2015/05/08 17:49:41, ddorwin wrote: > ditto Done. https://codereview.chromium.org/1055503002/diff/100001/Source/modules/encrypt... File Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.h (right): https://codereview.chromium.org/1055503002/diff/100001/Source/modules/encrypt... Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.h:13: #include "public/platform/WebContentDecryptionModule.h" On 2015/05/08 17:49:41, ddorwin wrote: > Can we fwd declare instead? Done. https://codereview.chromium.org/1055503002/diff/100001/Source/modules/encrypt... Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.h:15: #include "public/platform/WebMediaPlayer.h" On 2015/05/08 17:49:41, ddorwin wrote: > What is this used for? WebMediaPlayer is referenced which is being included previously from WebMediaPlayerClient. Fwd declaration should be enough now. https://codereview.chromium.org/1055503002/diff/100001/Source/modules/encrypt... Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.h:54: static WebContentDecryptionModule* contentDecryptionModule(HTMLMediaElement&); On 2015/05/08 17:49:42, ddorwin wrote: > Why is this still static? The call site makes the strangeness clearer. Done. https://codereview.chromium.org/1055503002/diff/100001/Source/web/FrameLoader... File Source/web/FrameLoaderClientImpl.cpp (right): https://codereview.chromium.org/1055503002/diff/100001/Source/web/FrameLoader... Source/web/FrameLoaderClientImpl.cpp:803: return adoptPtr(webFrame->client()->createMediaPlayer(webFrame, url, static_cast<WebMediaPlayerClient*>(htmlMediaElement), On 2015/05/08 17:49:42, ddorwin wrote: > nit: Put this complex last parameter on its own line, similar to below. Done. https://codereview.chromium.org/1055503002/diff/100001/Source/web/FrameLoader... Source/web/FrameLoaderClientImpl.cpp:805: HTMLMediaElementEncryptedMedia::contentDecryptionModule(*htmlMediaElement))); On 2015/05/08 17:49:42, ddorwin wrote: > As noted at the definition, I think it makes sense to get the HMEEM using from() > then use it to call cDM(). Done.
https://codereview.chromium.org/1055503002/diff/180001/Source/modules/encrypt... File Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp (right): https://codereview.chromium.org/1055503002/diff/180001/Source/modules/encrypt... Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp:607: HTMLMediaElementEncryptedMedia& thisElement = HTMLMediaElementEncryptedMedia::from(m_mediaElement); |thisElement| is just |this|. Access members directly. https://codereview.chromium.org/1055503002/diff/180001/Source/modules/encrypt... Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp:631: HTMLMediaElementEncryptedMedia& thisElement = HTMLMediaElementEncryptedMedia::from(m_mediaElement); ditto https://codereview.chromium.org/1055503002/diff/180001/Source/modules/encrypt... File Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.h (right): https://codereview.chromium.org/1055503002/diff/180001/Source/modules/encrypt... Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.h:58: WebContentDecryptionModule* contentDecryptionModule(HTMLMediaElement&); The parameter is unnecessary. https://codereview.chromium.org/1055503002/diff/180001/Source/web/FrameLoader... File Source/web/FrameLoaderClientImpl.cpp (right): https://codereview.chromium.org/1055503002/diff/180001/Source/web/FrameLoader... Source/web/FrameLoaderClientImpl.cpp:807: return adoptPtr(webFrame->client()->createMediaPlayer(webFrame, url, static_cast<WebMediaPlayerClient*>(htmlMediaElement), On 2015/05/19 10:39:47, Srirama wrote: > On 2015/05/08 17:49:42, ddorwin wrote: > > nit: Put this complex last parameter on its own line, similar to below. > > Done. It doesn't look like this was done. I was referring to the static_cast...
On 2015/05/19 17:01:50, ddorwin wrote: > https://codereview.chromium.org/1055503002/diff/180001/Source/modules/encrypt... > File Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp (right): > > https://codereview.chromium.org/1055503002/diff/180001/Source/modules/encrypt... > Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp:607: > HTMLMediaElementEncryptedMedia& thisElement = > HTMLMediaElementEncryptedMedia::from(m_mediaElement); > |thisElement| is just |this|. Access members directly. > > https://codereview.chromium.org/1055503002/diff/180001/Source/modules/encrypt... > Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp:631: > HTMLMediaElementEncryptedMedia& thisElement = > HTMLMediaElementEncryptedMedia::from(m_mediaElement); > ditto > > https://codereview.chromium.org/1055503002/diff/180001/Source/modules/encrypt... > File Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.h (right): > > https://codereview.chromium.org/1055503002/diff/180001/Source/modules/encrypt... > Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.h:58: > WebContentDecryptionModule* contentDecryptionModule(HTMLMediaElement&); > The parameter is unnecessary. > > https://codereview.chromium.org/1055503002/diff/180001/Source/web/FrameLoader... > File Source/web/FrameLoaderClientImpl.cpp (right): > > https://codereview.chromium.org/1055503002/diff/180001/Source/web/FrameLoader... > Source/web/FrameLoaderClientImpl.cpp:807: return > adoptPtr(webFrame->client()->createMediaPlayer(webFrame, url, > static_cast<WebMediaPlayerClient*>(htmlMediaElement), > On 2015/05/19 10:39:47, Srirama wrote: > > On 2015/05/08 17:49:42, ddorwin wrote: > > > nit: Put this complex last parameter on its own line, similar to below. > > > > Done. > > It doesn't look like this was done. I was referring to the static_cast... Sorry, i got it wrong. I have moved it to new line now. And i have added a new createMediaPlayer method to maintain both the approaches as discussed in chromium CL.
Several comments in PS4's HTMLMediaElementEncryptedMedia.* were unaddressed. https://codereview.chromium.org/1055503002/diff/200001/Source/core/html/HTMLM... File Source/core/html/HTMLMediaElement.h (right): https://codereview.chromium.org/1055503002/diff/200001/Source/core/html/HTMLM... Source/core/html/HTMLMediaElement.h:341: // TODO (srirama): Remove these methods once blink updated. This seems wrong even if temporary. Is there another ordering that avoids pulling these out of modules? https://codereview.chromium.org/1055503002/diff/200001/public/platform/WebMed... File public/platform/WebMediaPlayerClient.h (right): https://codereview.chromium.org/1055503002/diff/200001/public/platform/WebMed... public/platform/WebMediaPlayerClient.h:46: enum MediaKeyErrorCode { Ditto on avoiding this. https://codereview.chromium.org/1055503002/diff/200001/public/web/WebFrameCli... File public/web/WebFrameClient.h (right): https://codereview.chromium.org/1055503002/diff/200001/public/web/WebFrameCli... public/web/WebFrameClient.h:112: virtual WebMediaPlayer* createMediaPlayer(blink::WebLocalFrame*, const blink::WebURL&, blink::WebMediaPlayerClient*, blink::WebContentDecryptionModule*) { return 0; } blink:: is unnecessary
Sorry for overlooking your comments in patchset#4. I will address them in my next patchset. And regarding your concerns in this patch and chromium patch... Should i do the following way. Step 1 [BlinkCL] : Add a new interface WebMediaPlayerEncryptedMediaClient(WMPEMC) with encrypted media related functions and let HTMLMediaElementEncryptedMedia(HMEM) implement it. Step 2 [Chromium CL] : Add a new createMediaPlayer method with additional param for WMPEMC in render_frame_impl. Step 3 [BlinkCL]: Pass WMPEMC to createMediaPalyer funtion till WebMediaPlayerClientImpl (WMPCI) and let WMPCI invoke the new createMediaPlayer function of chromium side. Step 4 [Chromium CL] : On chromium side Pass this new client to WebMediaPlayer_Impl and WebMediaPlayer_Android(may be few more CLs for android port) and Encrypted_Media_Player_Support and invoke the encrypted related function through new client. At this point the link from chromium to HMEM will be established which will enable us to knock off WMPCI. Step 5 [Blink CL] : Probably this is the current CL with required modifications. https://codereview.chromium.org/1055503002/diff/200001/Source/core/html/HTMLM... File Source/core/html/HTMLMediaElement.h (right): https://codereview.chromium.org/1055503002/diff/200001/Source/core/html/HTMLM... Source/core/html/HTMLMediaElement.h:341: // TODO (srirama): Remove these methods once blink updated. On 2015/05/26 21:33:41, ddorwin wrote: > This seems wrong even if temporary. Is there another ordering that avoids > pulling these out of modules? As i explained in my chromium CL, at some point there will be both client and encrypted_client active at chromium side so i just added dummy implementatin here without having to include modules headers here. https://codereview.chromium.org/1055503002/diff/200001/public/platform/WebMed... File public/platform/WebMediaPlayerClient.h (right): https://codereview.chromium.org/1055503002/diff/200001/public/platform/WebMed... public/platform/WebMediaPlayerClient.h:46: enum MediaKeyErrorCode { On 2015/05/26 21:33:41, ddorwin wrote: > Ditto on avoiding this. Acknowledged. https://codereview.chromium.org/1055503002/diff/200001/public/web/WebFrameCli... File public/web/WebFrameClient.h (right): https://codereview.chromium.org/1055503002/diff/200001/public/web/WebFrameCli... public/web/WebFrameClient.h:112: virtual WebMediaPlayer* createMediaPlayer(blink::WebLocalFrame*, const blink::WebURL&, blink::WebMediaPlayerClient*, blink::WebContentDecryptionModule*) { return 0; } On 2015/05/26 21:33:41, ddorwin wrote: > blink:: is unnecessary Acknowledged.
In order to do the multi-sided patch dance, I think you'll need to land a Blink CL that adds WebMediaPlayerEncryptedMediaClient, makes WebMediaPlayerClient implement it, and adds a create method that takes it. Once that rolls into Chromium, you can start using the new interface in place of WebMediaPlayerClient and implement the new create method. For the old create method, you'll need to have a way to convert from WebMediaPlayerClient to WebMediaPlayerEncryptedMediaClient. Perhaps you'll need a temporary getAs() method. I haven't thought through in detail about whether this is the best approach, but I think this will avoid many of the issues identified so far.
On 2015/05/29 05:18:40, ddorwin wrote: > In order to do the multi-sided patch dance, I think you'll need to land a Blink > CL that adds WebMediaPlayerEncryptedMediaClient, makes WebMediaPlayerClient > implement it, and adds a create method that takes it. Once that rolls into > Chromium, you can start using the new interface in place of WebMediaPlayerClient > and implement the new create method. For the old create method, you'll need to > have a way to convert from WebMediaPlayerClient to > WebMediaPlayerEncryptedMediaClient. Perhaps you'll need a temporary getAs() > method. > > I haven't thought through in detail about whether this is the best approach, but > I think this will avoid many of the issues identified so far. OK, i will work on the blink CL to add the interface which will make things bit easier for me.
PTAL. Addressed all the review comments from previous patchsets. After getting approval for this, i will start splitting it for landing. Updated the chromium CL as well with latest changes https://codereview.chromium.org/1133033003/. Ran all the media test cases 5 times each. With these changes, right now two test cases are failing. 1) encrypted-media-change-mediakeys-and-src.html - flaky (crash, pass) 2) encrypted-media-playback-encrypted-and-clear-sources.html - crashing always Will look into these failures. https://codereview.chromium.org/1055503002/diff/220001/Source/core/html/HTMLM... File Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/1055503002/diff/220001/Source/core/html/HTMLM... Source/core/html/HTMLMediaElement.cpp:1046: if (m_webMediaPlayer) this is a duplicate condition when coming from parseAttribute(), i will check it how to avoid.
Is there now a set of 2 or 3 CLs that cover the entire change that will be required to get rid of MediaPlayer and MediaPlayerClient? I'd like to review it as a whole.
On 2015/06/17 12:53:58, philipj wrote: > Is there now a set of 2 or 3 CLs that cover the entire change that will be > required to get rid of MediaPlayer and MediaPlayerClient? I'd like to review it > as a whole. Thanks Philip. This is the CL for complete changes on blink side. And https://codereview.chromium.org/1133033003/ covers all the changes on chromium side. So please review these two CLs. Once these two are approved i will start breaking them into multi sided patches.
Patchset #8 (id:260001) has been deleted
I have fixed the layout test failures. I ran all the media test cases with 10 iterations each and the results are fine. PTAL.
philipj@opera.com changed reviewers: + rtoy@chromium.org
Sorry for the very slow review. This looks like it's going to work out, which makes me very happy! Just a few nits and questions, and I'd like ddorwin@ and rtoy@ to look at the EME and Web Audio bits of this. https://codereview.chromium.org/1055503002/diff/280001/Source/core/html/HTMLM... File Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/1055503002/diff/280001/Source/core/html/HTMLM... Source/core/html/HTMLMediaElement.cpp:2254: this, id.latin1().data(), kindString.ascii().data(), label.latin1().data(), language.latin1().data(), enabled); I think calling utf8() here is probably more appropriate. It's just for logging, but the label in particular can contain anything, and there's no reason to log it as latin1. https://codereview.chromium.org/1055503002/diff/280001/Source/core/html/HTMLM... Source/core/html/HTMLMediaElement.cpp:2298: this, id.latin1().data(), kindString.ascii().data(), label.latin1().data(), language.latin1().data(), selected); Ditto. https://codereview.chromium.org/1055503002/diff/280001/Source/core/html/HTMLM... Source/core/html/HTMLMediaElement.cpp:3040: m_audioSourceProvider.wrap(0); Use nullptr here, and why not in the above setClient(0) call as well assuming that is in fact a pointer. https://www.chromium.org/blink/coding-style#TOC-Null-false-and-0 https://codereview.chromium.org/1055503002/diff/280001/Source/core/html/HTMLM... Source/core/html/HTMLMediaElement.cpp:3450: m_audioSourceProvider.wrap(0); Ditto. https://codereview.chromium.org/1055503002/diff/280001/Source/core/html/HTMLM... Source/core/html/HTMLMediaElement.cpp:3678: void HTMLMediaElement::prepareAndLoadMediaPlayer(const WTF::String& url) Maybe this can simply be folded into startPlayerLoad() since that's the only call site and it would go quite well together? Otherwise, remove the WTF:: prefix. https://codereview.chromium.org/1055503002/diff/280001/Source/core/html/HTMLM... Source/core/html/HTMLMediaElement.cpp:3682: WebURL poster = mediaPlayerPosterURL(); Inline this into the setPoster call as that's the only place it's used. https://codereview.chromium.org/1055503002/diff/280001/Source/core/html/HTMLM... Source/core/html/HTMLMediaElement.cpp:3684: KURL kurl(ParsedURLString, url); Move this down to where it is used, or maybe inline it. https://codereview.chromium.org/1055503002/diff/280001/Source/core/html/HTMLM... Source/core/html/HTMLMediaElement.cpp:3686: ASSERT(frame); In the original code the case !webFrame case was handled. Perhaps we've already discussed this, but is ASSERT guaranteed to hold? It is in the HTMLMediaElement::loadResource case, but how about HTMLMediaElement::executeDeferredLoad? If you can get a HTMLMediaElement in a state where document().frame() is null, it seems like you could fail this assert by setting the preload attribute and triggering a deferred load. Can you investigate? https://codereview.chromium.org/1055503002/diff/280001/Source/core/html/HTMLM... Source/core/html/HTMLMediaElement.cpp:3688: if (!m_webMediaPlayer) Can this happen, and should it be treated as a decoding error, network error, or something else? https://codereview.chromium.org/1055503002/diff/280001/Source/core/html/HTMLM... Source/core/html/HTMLMediaElement.cpp:3716: void HTMLMediaElement::AudioSourceProviderImpl::wrap(WebAudioSourceProvider* provider) I'd like someone who knows Web Audio to take a look at these bits. I think you can ask rtoy@chromium.org. https://codereview.chromium.org/1055503002/diff/280001/Source/core/html/HTMLM... File Source/core/html/HTMLMediaElement.h (right): https://codereview.chromium.org/1055503002/diff/280001/Source/core/html/HTMLM... Source/core/html/HTMLMediaElement.h:155: void mediaPlayerDurationChanged(double duration, bool requestSeek); It doesn't look like this name change is necessary, as the existing durationChanged() takes no arguments. https://codereview.chromium.org/1055503002/diff/280001/Source/core/html/HTMLM... Source/core/html/HTMLMediaElement.h:180: void mediaPlayerAddTextTrack(TextTrack*); I tried undoing these name changes and it compiles fine, because the arguments types are enough to disambiguate the two methods. https://codereview.chromium.org/1055503002/diff/280001/Source/core/html/HTMLM... Source/core/html/HTMLMediaElement.h:191: virtual KURL mediaPlayerPosterURL() { return KURL(); } This FIXME doesn't seem like it's actually going to happen, the poster URL is actually used on the Chromium side with little chance of it going away I think. I think you can simply rename this to posterImageURL(), make it protected and then overload it in HTMLVideoElement. And remove the FIXME. https://codereview.chromium.org/1055503002/diff/280001/Source/core/html/HTMLM... Source/core/html/HTMLMediaElement.h:458: void prepareAndLoadMediaPlayer(const WTF::String& url); The WTF:: prefix ought not be needed here. https://codereview.chromium.org/1055503002/diff/280001/Source/web/FrameLoader... File Source/web/FrameLoaderClientImpl.cpp (right): https://codereview.chromium.org/1055503002/diff/280001/Source/web/FrameLoader... Source/web/FrameLoaderClientImpl.cpp:795: static_cast<WebMediaPlayerClient*>(htmlMediaElement), It compiles for me without this cast, but I guess it's for documentation purposes? https://codereview.chromium.org/1055503002/diff/280001/public/platform/WebMed... File public/platform/WebMediaPlayerClient.h (right): https://codereview.chromium.org/1055503002/diff/280001/public/platform/WebMed... public/platform/WebMediaPlayerClient.h:79: virtual void mediaPlayerRequestSeek(double) = 0; I tried ondoing this rename to see what would break, and nothing did! It looks like requestSeek is never called and can be removed, or am I missing something?
Thanks Philip. I have addressed your comments. PTAL. https://codereview.chromium.org/1055503002/diff/280001/Source/core/html/HTMLM... File Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/1055503002/diff/280001/Source/core/html/HTMLM... Source/core/html/HTMLMediaElement.cpp:2254: this, id.latin1().data(), kindString.ascii().data(), label.latin1().data(), language.latin1().data(), enabled); On 2015/07/01 13:49:19, philipj wrote: > I think calling utf8() here is probably more appropriate. It's just for logging, > but the label in particular can contain anything, and there's no reason to log > it as latin1. Done. https://codereview.chromium.org/1055503002/diff/280001/Source/core/html/HTMLM... Source/core/html/HTMLMediaElement.cpp:2298: this, id.latin1().data(), kindString.ascii().data(), label.latin1().data(), language.latin1().data(), selected); On 2015/07/01 13:49:19, philipj wrote: > Ditto. Done. https://codereview.chromium.org/1055503002/diff/280001/Source/core/html/HTMLM... Source/core/html/HTMLMediaElement.cpp:3040: m_audioSourceProvider.wrap(0); On 2015/07/01 13:49:19, philipj wrote: > Use nullptr here, and why not in the above setClient(0) call as well assuming > that is in fact a pointer. > https://www.chromium.org/blink/coding-style#TOC-Null-false-and-0 Done. Thanks for the info, generally i used to look around the code and follow same. May be i should look at the coding style guidelines when in doubt. https://codereview.chromium.org/1055503002/diff/280001/Source/core/html/HTMLM... Source/core/html/HTMLMediaElement.cpp:3450: m_audioSourceProvider.wrap(0); On 2015/07/01 13:49:18, philipj wrote: > Ditto. Done. https://codereview.chromium.org/1055503002/diff/280001/Source/core/html/HTMLM... Source/core/html/HTMLMediaElement.cpp:3678: void HTMLMediaElement::prepareAndLoadMediaPlayer(const WTF::String& url) On 2015/07/01 13:49:19, philipj wrote: > Maybe this can simply be folded into startPlayerLoad() since that's the only > call site and it would go quite well together? > > Otherwise, remove the WTF:: prefix. Done. Moved it to startPlayerLoad https://codereview.chromium.org/1055503002/diff/280001/Source/core/html/HTMLM... Source/core/html/HTMLMediaElement.cpp:3682: WebURL poster = mediaPlayerPosterURL(); On 2015/07/01 13:49:19, philipj wrote: > Inline this into the setPoster call as that's the only place it's used. Done. https://codereview.chromium.org/1055503002/diff/280001/Source/core/html/HTMLM... Source/core/html/HTMLMediaElement.cpp:3684: KURL kurl(ParsedURLString, url); On 2015/07/01 13:49:18, philipj wrote: > Move this down to where it is used, or maybe inline it. Done. Moved it down, but didn't inline it as it is used in two places. https://codereview.chromium.org/1055503002/diff/280001/Source/core/html/HTMLM... Source/core/html/HTMLMediaElement.cpp:3686: ASSERT(frame); On 2015/07/01 13:49:18, philipj wrote: > In the original code the case !webFrame case was handled. Perhaps we've already > discussed this, but is ASSERT guaranteed to hold? It is in the > HTMLMediaElement::loadResource case, but how about > HTMLMediaElement::executeDeferredLoad? If you can get a HTMLMediaElement in a > state where document().frame() is null, it seems like you could fail this assert > by setting the preload attribute and triggering a deferred load. Can you > investigate? Added similar check in executeDeferredLoad() to be in sync with loadResource() for time being. I can investigate it separately later if you want, may be with a TODO or FIXME? https://codereview.chromium.org/1055503002/diff/280001/Source/core/html/HTMLM... Source/core/html/HTMLMediaElement.cpp:3688: if (!m_webMediaPlayer) On 2015/07/01 13:49:19, philipj wrote: > Can this happen, and should it be treated as a decoding error, network error, or > something else? It can previously because of webframe null case but now we have handled it before. But for android it can be still null when gpu_channel_host and context_provider fails. https://codereview.chromium.org/1055503002/diff/280001/Source/core/html/HTMLM... Source/core/html/HTMLMediaElement.cpp:3716: void HTMLMediaElement::AudioSourceProviderImpl::wrap(WebAudioSourceProvider* provider) On 2015/07/01 13:49:19, philipj wrote: > I'd like someone who knows Web Audio to take a look at these bits. I think you > can ask mailto:rtoy@chromium.org. Acknowledged. https://codereview.chromium.org/1055503002/diff/280001/Source/core/html/HTMLM... File Source/core/html/HTMLMediaElement.h (right): https://codereview.chromium.org/1055503002/diff/280001/Source/core/html/HTMLM... Source/core/html/HTMLMediaElement.h:155: void mediaPlayerDurationChanged(double duration, bool requestSeek); On 2015/07/01 13:49:19, philipj wrote: > It doesn't look like this name change is necessary, as the existing > durationChanged() takes no arguments. Done. https://codereview.chromium.org/1055503002/diff/280001/Source/core/html/HTMLM... Source/core/html/HTMLMediaElement.h:180: void mediaPlayerAddTextTrack(TextTrack*); On 2015/07/01 13:49:19, philipj wrote: > I tried undoing these name changes and it compiles fine, because the arguments > types are enough to disambiguate the two methods. Done. https://codereview.chromium.org/1055503002/diff/280001/Source/core/html/HTMLM... Source/core/html/HTMLMediaElement.h:191: virtual KURL mediaPlayerPosterURL() { return KURL(); } On 2015/07/01 13:49:19, philipj wrote: > This FIXME doesn't seem like it's actually going to happen, the poster URL is > actually used on the Chromium side with little chance of it going away I think. > I think you can simply rename this to posterImageURL(), make it protected and > then overload it in HTMLVideoElement. And remove the FIXME. Done. https://codereview.chromium.org/1055503002/diff/280001/Source/core/html/HTMLM... Source/core/html/HTMLMediaElement.h:458: void prepareAndLoadMediaPlayer(const WTF::String& url); On 2015/07/01 13:49:19, philipj wrote: > The WTF:: prefix ought not be needed here. Removed the function https://codereview.chromium.org/1055503002/diff/280001/Source/web/FrameLoader... File Source/web/FrameLoaderClientImpl.cpp (right): https://codereview.chromium.org/1055503002/diff/280001/Source/web/FrameLoader... Source/web/FrameLoaderClientImpl.cpp:795: static_cast<WebMediaPlayerClient*>(htmlMediaElement), On 2015/07/01 13:49:19, philipj wrote: > It compiles for me without this cast, but I guess it's for documentation > purposes? Done. I would have added it in my initial stages of patch, not required now. https://codereview.chromium.org/1055503002/diff/280001/public/platform/WebMed... File public/platform/WebMediaPlayerClient.h (right): https://codereview.chromium.org/1055503002/diff/280001/public/platform/WebMed... public/platform/WebMediaPlayerClient.h:79: virtual void mediaPlayerRequestSeek(double) = 0; On 2015/07/01 13:49:19, philipj wrote: > I tried ondoing this rename to see what would break, and nothing did! It looks > like requestSeek is never called and can be removed, or am I missing something? It is requried for android case. It is used in WebMediaPlayerAndroid::OnSeekRequest().
https://codereview.chromium.org/1055503002/diff/280001/Source/core/html/HTMLM... File Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/1055503002/diff/280001/Source/core/html/HTMLM... Source/core/html/HTMLMediaElement.cpp:3040: m_audioSourceProvider.wrap(0); On 2015/07/03 12:59:01, Srirama wrote: > On 2015/07/01 13:49:19, philipj wrote: > > Use nullptr here, and why not in the above setClient(0) call as well assuming > > that is in fact a pointer. > > https://www.chromium.org/blink/coding-style#TOC-Null-false-and-0 > > Done. Thanks for the info, generally i used to look around the code and follow > same. May be i should look at the coding style guidelines when in doubt. Following local style is always the most important, but when it's simple one might also change the local style :) https://codereview.chromium.org/1055503002/diff/280001/Source/core/html/HTMLM... Source/core/html/HTMLMediaElement.cpp:3686: ASSERT(frame); On 2015/07/03 12:59:01, Srirama wrote: > On 2015/07/01 13:49:18, philipj wrote: > > In the original code the case !webFrame case was handled. Perhaps we've > already > > discussed this, but is ASSERT guaranteed to hold? It is in the > > HTMLMediaElement::loadResource case, but how about > > HTMLMediaElement::executeDeferredLoad? If you can get a HTMLMediaElement in a > > state where document().frame() is null, it seems like you could fail this > assert > > by setting the preload attribute and triggering a deferred load. Can you > > investigate? > > Added similar check in executeDeferredLoad() to be in sync with loadResource() > for time being. I can investigate it separately later if you want, may be with a > TODO or FIXME? See new comment on the moved code. https://codereview.chromium.org/1055503002/diff/280001/Source/core/html/HTMLM... Source/core/html/HTMLMediaElement.cpp:3688: if (!m_webMediaPlayer) On 2015/07/03 12:59:02, Srirama wrote: > On 2015/07/01 13:49:19, philipj wrote: > > Can this happen, and should it be treated as a decoding error, network error, > or > > something else? > > It can previously because of webframe null case but now we have handled it > before. > But for android it can be still null when gpu_channel_host and context_provider > fails. Acknowledged. https://codereview.chromium.org/1055503002/diff/280001/public/platform/WebMed... File public/platform/WebMediaPlayerClient.h (right): https://codereview.chromium.org/1055503002/diff/280001/public/platform/WebMed... public/platform/WebMediaPlayerClient.h:79: virtual void mediaPlayerRequestSeek(double) = 0; On 2015/07/03 12:59:02, Srirama wrote: > On 2015/07/01 13:49:19, philipj wrote: > > I tried ondoing this rename to see what would break, and nothing did! It looks > > like requestSeek is never called and can be removed, or am I missing > something? > > It is requried for android case. It is used in > WebMediaPlayerAndroid::OnSeekRequest(). OK, so how about the name, is there some ambiguity with the variable requestSeek or why is it renamed? https://codereview.chromium.org/1055503002/diff/300001/Source/core/html/HTMLM... File Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/1055503002/diff/300001/Source/core/html/HTMLM... Source/core/html/HTMLMediaElement.cpp:1048: ASSERT(!m_webMediaPlayer); Perhaps move this to the top of the function. https://codereview.chromium.org/1055503002/diff/300001/Source/core/html/HTMLM... Source/core/html/HTMLMediaElement.cpp:1050: LocalFrame* frame = document().frame(); I think handling the null case here instead of in executeDeferredLoad() would be more clear, even if there's one caller when it's known to never happen. Perhaps also add a comment about when it can be null here, if you were able to figure that out. https://codereview.chromium.org/1055503002/diff/300001/Source/core/html/HTMLV... File Source/core/html/HTMLVideoElement.h (right): https://codereview.chromium.org/1055503002/diff/300001/Source/core/html/HTMLV... Source/core/html/HTMLVideoElement.h:76: // FIXME: Remove this when HTMLMediaElement::prepareAndLoadMediaPlayer does not depend on it. I guess remove this FIXME as well, don't think it's actually going to be fixed.
EME stuff LG. I assume this is the "final state" after the multi-sided patch dance. https://codereview.chromium.org/1055503002/diff/300001/Source/core/html/HTMLM... File Source/core/html/HTMLMediaElement.h (right): https://codereview.chromium.org/1055503002/diff/300001/Source/core/html/HTMLM... Source/core/html/HTMLMediaElement.h:69: class CORE_EXPORT HTMLMediaElement : public HTMLElement, public WillBeHeapSupplementable<HTMLMediaElement>, public WebMediaPlayerClient, public ActiveDOMObject { Can WebMediaPlayerClient be private (similar to the FIXME that was removed)? https://codereview.chromium.org/1055503002/diff/300001/Source/core/html/HTMLM... Source/core/html/HTMLMediaElement.h:326: virtual void networkStateChanged() override final; Should we label these as: // <interface> implementation. as we do in Chromium? https://codereview.chromium.org/1055503002/diff/300001/Source/modules/encrypt... File Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp (right): https://codereview.chromium.org/1055503002/diff/300001/Source/modules/encrypt... Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp:597: // This is nothing but |this| element. nit: This could be more concise. However, you could probably just remove 597-598 and have steps 1-2 together. We know what the HME objec is. https://codereview.chromium.org/1055503002/diff/300001/public/platform/WebMed... File public/platform/WebMediaPlayerClient.h (right): https://codereview.chromium.org/1055503002/diff/300001/public/platform/WebMed... public/platform/WebMediaPlayerClient.h:79: virtual void mediaPlayerRequestSeek(double) = 0; Is there a reason this name was changed? https://codereview.chromium.org/1055503002/diff/300001/public/platform/WebMed... File public/platform/WebMediaPlayerEncryptedMediaClient.h (right): https://codereview.chromium.org/1055503002/diff/300001/public/platform/WebMed... public/platform/WebMediaPlayerEncryptedMediaClient.h:2: * Copyright (C) 2009 Google Inc. All rights reserved. nit: 2009 is wrong. These methods were added in 2012. https://codereview.chromium.org/1055503002/diff/300001/public/web/WebFrameCli... File public/web/WebFrameClient.h (right): https://codereview.chromium.org/1055503002/diff/300001/public/web/WebFrameCli... public/web/WebFrameClient.h:113: virtual WebMediaPlayer* createMediaPlayer(WebLocalFrame*, const WebURL&, WebMediaPlayerClient*, WebMediaPlayerEncryptedMediaClient*, WebContentDecryptionModule*) { return 0; } You'll need to have two versions of this method, right? (Perhaps this is just showing the final result for review.)
https://codereview.chromium.org/1055503002/diff/300001/Source/modules/encrypt... File Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp (right): https://codereview.chromium.org/1055503002/diff/300001/Source/modules/encrypt... Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp:597: // This is nothing but |this| element. On 2015/07/06 20:43:01, ddorwin wrote: > nit: This could be more concise. However, you could probably just remove 597-598 > and have steps 1-2 together. We know what the HME objec is. Isn't the comment incorrect? |this| will be a HTMLMediaElementEncryptedMedia supplement, not a HTMLMediaElement.
https://codereview.chromium.org/1055503002/diff/300001/Source/modules/encrypt... File Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp (right): https://codereview.chromium.org/1055503002/diff/300001/Source/modules/encrypt... Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp:597: // This is nothing but |this| element. On 2015/07/06 20:51:17, philipj wrote: > On 2015/07/06 20:43:01, ddorwin wrote: > > nit: This could be more concise. However, you could probably just remove > 597-598 > > and have steps 1-2 together. We know what the HME objec is. > > Isn't the comment incorrect? |this| will be a HTMLMediaElementEncryptedMedia > supplement, not a HTMLMediaElement. Yes. I should have included that in my comment, but just skipped over to eliminating it. _If_ we are going to keep this comment, it should refer to m_mediaElement. Something like "|m_mediaElement| is that object."
Thanks Philip and ddorwin for the review. I will address the nits while landing the changes. I think the changes are fine except for AudioSourceProvider code which Raymond will look into. So shall i wait for Raymond's review or shall i go ahead and split the changes and land one by one, starting with https://codereview.chromium.org/1125353004/ ?
Splitting into separate CLs now and asking for final review of those sounds good.
To simplify the review, please try to reuse the existing CLs as much as possible. Breaking a few files out is fine, but please use this CL for the majority of the files. Is there anything other than the three public/ files that will be broken out? IMO, it would be ideal if the nits were addressed before breaking it up and copying the results to the other CL. Preparing to commit also means adding backwards compatibility for the existing createMediaPlayer() method to this and the Chromium CL, right? I'd suggest uploading a patch set with those changes to make sure it compiles and the public/ CL actually has the correct changes for the next commit.
PTAL. https://codereview.chromium.org/1055503002/diff/280001/public/platform/WebMed... File public/platform/WebMediaPlayerClient.h (right): https://codereview.chromium.org/1055503002/diff/280001/public/platform/WebMed... public/platform/WebMediaPlayerClient.h:79: virtual void mediaPlayerRequestSeek(double) = 0; On 2015/07/06 11:07:10, philipj wrote: > On 2015/07/03 12:59:02, Srirama wrote: > > On 2015/07/01 13:49:19, philipj wrote: > > > I tried ondoing this rename to see what would break, and nothing did! It > looks > > > like requestSeek is never called and can be removed, or am I missing > > something? > > > > It is requried for android case. It is used in > > WebMediaPlayerAndroid::OnSeekRequest(). > > OK, so how about the name, is there some ambiguity with the variable requestSeek > or why is it renamed? In patchset3, you suggested to use this name in HTMLMediaElement.cpp to signify that it is a call from the player, so i have changed it. Probably you misunderstood the renaming that time? https://codereview.chromium.org/1055503002/diff/300001/Source/core/html/HTMLM... File Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/1055503002/diff/300001/Source/core/html/HTMLM... Source/core/html/HTMLMediaElement.cpp:1048: ASSERT(!m_webMediaPlayer); On 2015/07/06 11:07:10, philipj wrote: > Perhaps move this to the top of the function. Done. https://codereview.chromium.org/1055503002/diff/300001/Source/core/html/HTMLM... Source/core/html/HTMLMediaElement.cpp:1050: LocalFrame* frame = document().frame(); On 2015/07/06 11:07:10, philipj wrote: > I think handling the null case here instead of in executeDeferredLoad() would be > more clear, even if there's one caller when it's known to never happen. Perhaps > also add a comment about when it can be null here, if you were able to figure > that out. Done. Added TODO for time being, will investigate it once i successfully land this if you don't mind. https://codereview.chromium.org/1055503002/diff/300001/Source/core/html/HTMLM... File Source/core/html/HTMLMediaElement.h (right): https://codereview.chromium.org/1055503002/diff/300001/Source/core/html/HTMLM... Source/core/html/HTMLMediaElement.h:69: class CORE_EXPORT HTMLMediaElement : public HTMLElement, public WillBeHeapSupplementable<HTMLMediaElement>, public WebMediaPlayerClient, public ActiveDOMObject { On 2015/07/06 20:43:01, ddorwin wrote: > Can WebMediaPlayerClient be private (similar to the FIXME that was removed)? It is giving problem in frameloaderclientimpl while type casting mediaelement to webmediaplayerclient. https://codereview.chromium.org/1055503002/diff/300001/Source/core/html/HTMLM... Source/core/html/HTMLMediaElement.h:326: virtual void networkStateChanged() override final; On 2015/07/06 20:43:01, ddorwin wrote: > Should we label these as: > // <interface> implementation. > as we do in Chromium? Done. https://codereview.chromium.org/1055503002/diff/300001/Source/core/html/HTMLV... File Source/core/html/HTMLVideoElement.h (right): https://codereview.chromium.org/1055503002/diff/300001/Source/core/html/HTMLV... Source/core/html/HTMLVideoElement.h:76: // FIXME: Remove this when HTMLMediaElement::prepareAndLoadMediaPlayer does not depend on it. On 2015/07/06 11:07:10, philipj wrote: > I guess remove this FIXME as well, don't think it's actually going to be fixed. Done. https://codereview.chromium.org/1055503002/diff/300001/Source/modules/encrypt... File Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp (right): https://codereview.chromium.org/1055503002/diff/300001/Source/modules/encrypt... Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp:597: // This is nothing but |this| element. On 2015/07/06 20:53:41, ddorwin wrote: > On 2015/07/06 20:51:17, philipj wrote: > > On 2015/07/06 20:43:01, ddorwin wrote: > > > nit: This could be more concise. However, you could probably just remove > > 597-598 > > > and have steps 1-2 together. We know what the HME objec is. > > > > Isn't the comment incorrect? |this| will be a HTMLMediaElementEncryptedMedia > > supplement, not a HTMLMediaElement. > > Yes. I should have included that in my comment, but just skipped over to > eliminating it. _If_ we are going to keep this comment, it should refer to > m_mediaElement. Something like "|m_mediaElement| is that object." Removed it. https://codereview.chromium.org/1055503002/diff/300001/public/platform/WebMed... File public/platform/WebMediaPlayerClient.h (right): https://codereview.chromium.org/1055503002/diff/300001/public/platform/WebMed... public/platform/WebMediaPlayerClient.h:79: virtual void mediaPlayerRequestSeek(double) = 0; On 2015/07/06 20:43:01, ddorwin wrote: > Is there a reason this name was changed? As per Philip's suggestion in patchset3, i have changed it. Even he has the same doubt now, probably he would have misunderstood the renaming that time. https://codereview.chromium.org/1055503002/diff/300001/public/platform/WebMed... File public/platform/WebMediaPlayerEncryptedMediaClient.h (right): https://codereview.chromium.org/1055503002/diff/300001/public/platform/WebMed... public/platform/WebMediaPlayerEncryptedMediaClient.h:2: * Copyright (C) 2009 Google Inc. All rights reserved. On 2015/07/06 20:43:01, ddorwin wrote: > nit: 2009 is wrong. These methods were added in 2012. Done. https://codereview.chromium.org/1055503002/diff/300001/public/web/WebFrameCli... File public/web/WebFrameClient.h (right): https://codereview.chromium.org/1055503002/diff/300001/public/web/WebFrameCli... public/web/WebFrameClient.h:113: virtual WebMediaPlayer* createMediaPlayer(WebLocalFrame*, const WebURL&, WebMediaPlayerClient*, WebMediaPlayerEncryptedMediaClient*, WebContentDecryptionModule*) { return 0; } On 2015/07/06 20:43:01, ddorwin wrote: > You'll need to have two versions of this method, right? (Perhaps this is just > showing the final result for review.) Yes, that will be done in separate CL (that will be the first CL to land https://codereview.chromium.org/1125353004/).
On 2015/07/08 20:19:04, ddorwin wrote: > To simplify the review, please try to reuse the existing CLs as much as > possible. Breaking a few files out is fine, but please use this CL for the > majority of the files. Is there anything other than the three public/ files that > will be broken out? Yes, i will use the existing two CLs. But initially public/ changes will have WebMediaPlayerClient implement WebMediaPlayerEncryptedMediaClient, so unless i move that implementation from WebMediaPlayerClient to HTMLMediaElementEncryptedmedia in intermediate CLs, I cannot come to this CL as it will not allow HTMLMediaElement to implement WebMediaPlayerClient. > IMO, it would be ideal if the nits were addressed before breaking it up and > copying the results to the other CL. Done. > Preparing to commit also means adding backwards compatibility for the existing > createMediaPlayer() method to this and the Chromium CL, right? I'd suggest > uploading a patch set with those changes to make sure it compiles and the > public/ CL actually has the correct changes for the next commit. I will add backward compatibility as we discussed earlier. But I can come to this CL only after landing the interface changes https://codereview.chromium.org/1125353004/ which you have already reviewed, and then land the chromium side changes to support two paths for createMediaPlayer(), then shift to new createMediaPlayer() path on blink side, remove the old path on chromium side and then finally land this CL.
https://codereview.chromium.org/1055503002/diff/280001/public/platform/WebMed... File public/platform/WebMediaPlayerClient.h (right): https://codereview.chromium.org/1055503002/diff/280001/public/platform/WebMed... public/platform/WebMediaPlayerClient.h:79: virtual void mediaPlayerRequestSeek(double) = 0; On 2015/07/09 15:19:37, Srirama wrote: > On 2015/07/06 11:07:10, philipj wrote: > > On 2015/07/03 12:59:02, Srirama wrote: > > > On 2015/07/01 13:49:19, philipj wrote: > > > > I tried ondoing this rename to see what would break, and nothing did! It > > looks > > > > like requestSeek is never called and can be removed, or am I missing > > > something? > > > > > > It is requried for android case. It is used in > > > WebMediaPlayerAndroid::OnSeekRequest(). > > > > OK, so how about the name, is there some ambiguity with the variable > requestSeek > > or why is it renamed? > > In patchset3, you suggested to use this name in HTMLMediaElement.cpp to signify > that it is a call from the player, so i have changed it. Probably you > misunderstood the renaming that time? Yeah, it's been a long series of patches and I've probably had conflicting opinions over time, and forgotten about it. As it stands, this is the only remaining case of mediaPlayerFoo, and it's a rename, so if it's possible to not rename it I think I'd like that better, and will try to stick to that opinion ;)
On 2015/07/09 15:42:34, philipj wrote: > https://codereview.chromium.org/1055503002/diff/280001/public/platform/WebMed... > File public/platform/WebMediaPlayerClient.h (right): > > https://codereview.chromium.org/1055503002/diff/280001/public/platform/WebMed... > public/platform/WebMediaPlayerClient.h:79: virtual void > mediaPlayerRequestSeek(double) = 0; > On 2015/07/09 15:19:37, Srirama wrote: > > On 2015/07/06 11:07:10, philipj wrote: > > > On 2015/07/03 12:59:02, Srirama wrote: > > > > On 2015/07/01 13:49:19, philipj wrote: > > > > > I tried ondoing this rename to see what would break, and nothing did! It > > > looks > > > > > like requestSeek is never called and can be removed, or am I missing > > > > something? > > > > > > > > It is requried for android case. It is used in > > > > WebMediaPlayerAndroid::OnSeekRequest(). > > > > > > OK, so how about the name, is there some ambiguity with the variable > > requestSeek > > > or why is it renamed? > > > > In patchset3, you suggested to use this name in HTMLMediaElement.cpp to > signify > > that it is a call from the player, so i have changed it. Probably you > > misunderstood the renaming that time? > > Yeah, it's been a long series of patches and I've probably had conflicting > opinions over time, and forgotten about it. As it stands, this is the only > remaining case of mediaPlayerFoo, and it's a rename, so if it's possible to not > rename it I think I'd like that better, and will try to stick to that opinion ;) I was about to revert it, but then i thought lets have your final opinion. I will change it, thanks.
1 comment about private inheritance in PS9. On 2015/07/09 15:33:20, Srirama wrote: > On 2015/07/08 20:19:04, ddorwin wrote: > > To simplify the review, please try to reuse the existing CLs as much as > > possible. Breaking a few files out is fine, but please use this CL for the > > majority of the files. Is there anything other than the three public/ files > that > > will be broken out? > > Yes, i will use the existing two CLs. But initially public/ changes will have > WebMediaPlayerClient implement WebMediaPlayerEncryptedMediaClient, so unless i > move that implementation from WebMediaPlayerClient to > HTMLMediaElementEncryptedmedia in intermediate CLs, I cannot come to this CL as > it will not allow HTMLMediaElement to implement WebMediaPlayerClient. > > > IMO, it would be ideal if the nits were addressed before breaking it up and > > copying the results to the other CL. > > Done. > > > Preparing to commit also means adding backwards compatibility for the existing > > createMediaPlayer() method to this and the Chromium CL, right? I'd suggest > > uploading a patch set with those changes to make sure it compiles and the > > public/ CL actually has the correct changes for the next commit. > > I will add backward compatibility as we discussed earlier. But I can come to > this CL only after landing the interface changes > https://codereview.chromium.org/1125353004/ which you have already reviewed, and > then land the chromium side changes to support two paths for > createMediaPlayer(), then shift to new createMediaPlayer() path on blink side, > remove the old path on chromium side and then finally land this CL. Oh, so this is the fourth CL to land? And the third will be to pass WebMediaPlayerClient as WebMediaPlayerEncryptedMediaClient? Okay, that makes sense. (I was thinking this was the third.) Then this CL might include removal of the old createMediaPlayer() signature. You can probably just leave the public/ files in this CL. Then, when you rebase this CL on the other CLs, there should be no changes from the current patch set. (I think two of the public/ files will have diffs from tip of tree.) If there are any review feedback changes to public/ in this CL that aren't in https://codereview.chromium.org/1125353004/, please be sure to include them there. Other than that, LG to move forward with the other CLs. We can figure out the private inheritance issue in parallel. Thanks! https://codereview.chromium.org/1055503002/diff/300001/Source/core/html/HTMLM... File Source/core/html/HTMLMediaElement.h (right): https://codereview.chromium.org/1055503002/diff/300001/Source/core/html/HTMLM... Source/core/html/HTMLMediaElement.h:69: class CORE_EXPORT HTMLMediaElement : public HTMLElement, public WillBeHeapSupplementable<HTMLMediaElement>, public WebMediaPlayerClient, public ActiveDOMObject { On 2015/07/09 15:19:37, Srirama wrote: > On 2015/07/06 20:43:01, ddorwin wrote: > > Can WebMediaPlayerClient be private (similar to the FIXME that was removed)? > > It is giving problem in frameloaderclientimpl while type casting mediaelement to > webmediaplayerclient. I don't see such a cast (or even "webmediaplayerclient") in that file. Can you point me to the specific line (maybe with a "HERE" comment on that line)? philipj: Should this indeed be private inheritance as indicated by the FIXME?
https://codereview.chromium.org/1055503002/diff/300001/Source/core/html/HTMLM... File Source/core/html/HTMLMediaElement.h (right): https://codereview.chromium.org/1055503002/diff/300001/Source/core/html/HTMLM... Source/core/html/HTMLMediaElement.h:69: class CORE_EXPORT HTMLMediaElement : public HTMLElement, public WillBeHeapSupplementable<HTMLMediaElement>, public WebMediaPlayerClient, public ActiveDOMObject { On 2015/07/09 18:37:42, ddorwin wrote: > On 2015/07/09 15:19:37, Srirama wrote: > > On 2015/07/06 20:43:01, ddorwin wrote: > > > Can WebMediaPlayerClient be private (similar to the FIXME that was removed)? > > > > It is giving problem in frameloaderclientimpl while type casting mediaelement > to > > webmediaplayerclient. > > I don't see such a cast (or even "webmediaplayerclient") in that file. Can you > point me to the specific line (maybe with a "HERE" comment on that line)? > > philipj: Should this indeed be private inheritance as indicated by the FIXME? It's the implicit cast from HTMLMediaElement* to WebMediaPlayerClient* in FrameLoaderClientImpl::createWebMediaPlayer that's now a problem. The original FIXME around MediaPlayerClient, where WebMediaPlayerClientImpl had a cast from MediaPlayerClient* to HTMLMediaElement* was IMHO worse, because there HTMLMediaElement* was used to bypass the layering in ugly ways, especially in WebMediaPlayerClientImpl::load(). However, the layering is still not perfect. What remains is in order to avoid HTMLMediaElement depending on HTMLMediaElementEncryptedMedia, a core-modules dependency violation. The most direct path forward I can see is some method in web/ that can return a WebMediaPlayerEncryptedMediaClient* and WebContentDecryptionModule* from a HTMLMediaElement*, and then having HTMLMediaElement call WebFrameClient::createMediaPlayer() itself, removing the createWebMediaPlayer helper. Do you think that would be OK, and if so what's a good place to put those converters?
https://codereview.chromium.org/1055503002/diff/300001/Source/core/html/HTMLM... File Source/core/html/HTMLMediaElement.h (right): https://codereview.chromium.org/1055503002/diff/300001/Source/core/html/HTMLM... Source/core/html/HTMLMediaElement.h:69: class CORE_EXPORT HTMLMediaElement : public HTMLElement, public WillBeHeapSupplementable<HTMLMediaElement>, public WebMediaPlayerClient, public ActiveDOMObject { On 2015/07/09 19:50:15, philipj wrote: > On 2015/07/09 18:37:42, ddorwin wrote: > > On 2015/07/09 15:19:37, Srirama wrote: > > > On 2015/07/06 20:43:01, ddorwin wrote: > > > > Can WebMediaPlayerClient be private (similar to the FIXME that was > removed)? > > > > > > It is giving problem in frameloaderclientimpl while type casting > mediaelement > > to > > > webmediaplayerclient. > > > > I don't see such a cast (or even "webmediaplayerclient") in that file. Can you > > point me to the specific line (maybe with a "HERE" comment on that line)? > > > > philipj: Should this indeed be private inheritance as indicated by the FIXME? > > It's the implicit cast from HTMLMediaElement* to WebMediaPlayerClient* in > FrameLoaderClientImpl::createWebMediaPlayer that's now a problem. > > The original FIXME around MediaPlayerClient, where WebMediaPlayerClientImpl had > a cast from MediaPlayerClient* to HTMLMediaElement* was IMHO worse, because > there HTMLMediaElement* was used to bypass the layering in ugly ways, especially > in WebMediaPlayerClientImpl::load(). > > However, the layering is still not perfect. What remains is in order to avoid > HTMLMediaElement depending on HTMLMediaElementEncryptedMedia, a core-modules > dependency violation. > > The most direct path forward I can see is some method in web/ that can return a > WebMediaPlayerEncryptedMediaClient* and WebContentDecryptionModule* from a > HTMLMediaElement*, and then having HTMLMediaElement call > WebFrameClient::createMediaPlayer() itself, removing the createWebMediaPlayer > helper. Do you think that would be OK, and if so what's a good place to put > those converters? Ah, thanks. I'm not an expert in where to put such things or whether that would be better. Other options might include friending or getWebMediaPlayerClient(). For now, perhaps we should just leave a TODO (or pick a simple solution to avoid people using WebMediaPlayerClient APIs).
https://codereview.chromium.org/1055503002/diff/300001/Source/core/html/HTMLM... File Source/core/html/HTMLMediaElement.h (right): https://codereview.chromium.org/1055503002/diff/300001/Source/core/html/HTMLM... Source/core/html/HTMLMediaElement.h:69: class CORE_EXPORT HTMLMediaElement : public HTMLElement, public WillBeHeapSupplementable<HTMLMediaElement>, public WebMediaPlayerClient, public ActiveDOMObject { On 2015/07/09 21:15:11, ddorwin wrote: > On 2015/07/09 19:50:15, philipj wrote: > > On 2015/07/09 18:37:42, ddorwin wrote: > > > On 2015/07/09 15:19:37, Srirama wrote: > > > > On 2015/07/06 20:43:01, ddorwin wrote: > > > > > Can WebMediaPlayerClient be private (similar to the FIXME that was > > removed)? > > > > > > > > It is giving problem in frameloaderclientimpl while type casting > > mediaelement > > > to > > > > webmediaplayerclient. > > > > > > I don't see such a cast (or even "webmediaplayerclient") in that file. Can > you > > > point me to the specific line (maybe with a "HERE" comment on that line)? > > > > > > philipj: Should this indeed be private inheritance as indicated by the > FIXME? > > > > It's the implicit cast from HTMLMediaElement* to WebMediaPlayerClient* in > > FrameLoaderClientImpl::createWebMediaPlayer that's now a problem. > > > > The original FIXME around MediaPlayerClient, where WebMediaPlayerClientImpl > had > > a cast from MediaPlayerClient* to HTMLMediaElement* was IMHO worse, because > > there HTMLMediaElement* was used to bypass the layering in ugly ways, > especially > > in WebMediaPlayerClientImpl::load(). > > > > However, the layering is still not perfect. What remains is in order to avoid > > HTMLMediaElement depending on HTMLMediaElementEncryptedMedia, a core-modules > > dependency violation. > > > > The most direct path forward I can see is some method in web/ that can return > a > > WebMediaPlayerEncryptedMediaClient* and WebContentDecryptionModule* from a > > HTMLMediaElement*, and then having HTMLMediaElement call > > WebFrameClient::createMediaPlayer() itself, removing the createWebMediaPlayer > > helper. Do you think that would be OK, and if so what's a good place to put > > those converters? > > Ah, thanks. I'm not an expert in where to put such things or whether that would > be better. Other options might include friending or getWebMediaPlayerClient(). > > For now, perhaps we should just leave a TODO (or pick a simple solution to avoid > people using WebMediaPlayerClient APIs). I think it would be OK to leave this as // TODO(srirama.m): Make the WebMediaPlayerClient inheritance private by adding a means for getting a WebMediaPlayerEncryptedMediaClient and WebContentDecryptionModule from an HTMLMediaElement and calling WebFrameClient::createMediaPlayer() directly. Or if you want to try it out now, that'd also be OK, Srirama.
On 2015/07/09 21:41:03, philipj wrote: > https://codereview.chromium.org/1055503002/diff/300001/Source/core/html/HTMLM... > File Source/core/html/HTMLMediaElement.h (right): > > https://codereview.chromium.org/1055503002/diff/300001/Source/core/html/HTMLM... > Source/core/html/HTMLMediaElement.h:69: class CORE_EXPORT HTMLMediaElement : > public HTMLElement, public WillBeHeapSupplementable<HTMLMediaElement>, public > WebMediaPlayerClient, public ActiveDOMObject { > On 2015/07/09 21:15:11, ddorwin wrote: > > On 2015/07/09 19:50:15, philipj wrote: > > > On 2015/07/09 18:37:42, ddorwin wrote: > > > > On 2015/07/09 15:19:37, Srirama wrote: > > > > > On 2015/07/06 20:43:01, ddorwin wrote: > > > > > > Can WebMediaPlayerClient be private (similar to the FIXME that was > > > removed)? > > > > > > > > > > It is giving problem in frameloaderclientimpl while type casting > > > mediaelement > > > > to > > > > > webmediaplayerclient. > > > > > > > > I don't see such a cast (or even "webmediaplayerclient") in that file. Can > > you > > > > point me to the specific line (maybe with a "HERE" comment on that line)? > > > > > > > > philipj: Should this indeed be private inheritance as indicated by the > > FIXME? > > > > > > It's the implicit cast from HTMLMediaElement* to WebMediaPlayerClient* in > > > FrameLoaderClientImpl::createWebMediaPlayer that's now a problem. > > > > > > The original FIXME around MediaPlayerClient, where WebMediaPlayerClientImpl > > had > > > a cast from MediaPlayerClient* to HTMLMediaElement* was IMHO worse, because > > > there HTMLMediaElement* was used to bypass the layering in ugly ways, > > especially > > > in WebMediaPlayerClientImpl::load(). > > > > > > However, the layering is still not perfect. What remains is in order to > avoid > > > HTMLMediaElement depending on HTMLMediaElementEncryptedMedia, a core-modules > > > dependency violation. > > > > > > The most direct path forward I can see is some method in web/ that can > return > > a > > > WebMediaPlayerEncryptedMediaClient* and WebContentDecryptionModule* from a > > > HTMLMediaElement*, and then having HTMLMediaElement call > > > WebFrameClient::createMediaPlayer() itself, removing the > createWebMediaPlayer > > > helper. Do you think that would be OK, and if so what's a good place to put > > > those converters? > > > > Ah, thanks. I'm not an expert in where to put such things or whether that > would > > be better. Other options might include friending or getWebMediaPlayerClient(). > > > > For now, perhaps we should just leave a TODO (or pick a simple solution to > avoid > > people using WebMediaPlayerClient APIs). > > I think it would be OK to leave this as > > // TODO(srirama.m): Make the WebMediaPlayerClient inheritance private by adding > a means for getting a WebMediaPlayerEncryptedMediaClient and > WebContentDecryptionModule from an HTMLMediaElement and calling > WebFrameClient::createMediaPlayer() directly. > > Or if you want to try it out now, that'd also be OK, Srirama. Thanks Philip and ddorwin for the review. I would like to keep it simple for me, so i will go with TODO and first finish the elimination of MediaPlayer and MediaPlayerClient. @ddorwin: you are right, the third CL is to shift to new createMediaPlayer method by passing WebMediaPlayerClient for WebMediaPlayerEncryptedMediaClient.
Patchset #11 (id:340001) has been deleted
Patchset #11 (id:360001) has been deleted
Rebased and addressed review comments. There seems to be an issue with win_blink_compile_dbg bot with respect to WebMediaPlayerClient inheritance. htmlmediaelement.h(73) : error C2220: warning treated as error - no 'object' file generated htmlmediaelement.h(73) : warning C4275: non dll-interface class 'blink::WebMediaPlayerClient' used as base for dll-interface class 'blink::HTMLMediaElement'. May be other platforms have suppressed the warning. Any suggestion what to do to resolve this? https://codereview.chromium.org/1055503002/diff/280001/public/platform/WebMed... File public/platform/WebMediaPlayerClient.h (right): https://codereview.chromium.org/1055503002/diff/280001/public/platform/WebMed... public/platform/WebMediaPlayerClient.h:79: virtual void mediaPlayerRequestSeek(double) = 0; On 2015/07/09 15:42:34, philipj wrote: > On 2015/07/09 15:19:37, Srirama wrote: > > On 2015/07/06 11:07:10, philipj wrote: > > > On 2015/07/03 12:59:02, Srirama wrote: > > > > On 2015/07/01 13:49:19, philipj wrote: > > > > > I tried ondoing this rename to see what would break, and nothing did! It > > > looks > > > > > like requestSeek is never called and can be removed, or am I missing > > > > something? > > > > > > > > It is requried for android case. It is used in > > > > WebMediaPlayerAndroid::OnSeekRequest(). > > > > > > OK, so how about the name, is there some ambiguity with the variable > > requestSeek > > > or why is it renamed? > > > > In patchset3, you suggested to use this name in HTMLMediaElement.cpp to > signify > > that it is a call from the player, so i have changed it. Probably you > > misunderstood the renaming that time? > > Yeah, it's been a long series of patches and I've probably had conflicting > opinions over time, and forgotten about it. As it stands, this is the only > remaining case of mediaPlayerFoo, and it's a rename, so if it's possible to not > rename it I think I'd like that better, and will try to stick to that opinion ;) Done. https://codereview.chromium.org/1055503002/diff/300001/Source/core/html/HTMLM... File Source/core/html/HTMLMediaElement.h (right): https://codereview.chromium.org/1055503002/diff/300001/Source/core/html/HTMLM... Source/core/html/HTMLMediaElement.h:69: class CORE_EXPORT HTMLMediaElement : public HTMLElement, public WillBeHeapSupplementable<HTMLMediaElement>, public WebMediaPlayerClient, public ActiveDOMObject { On 2015/07/09 21:41:03, philipj wrote: > On 2015/07/09 21:15:11, ddorwin wrote: > > On 2015/07/09 19:50:15, philipj wrote: > > > On 2015/07/09 18:37:42, ddorwin wrote: > > > > On 2015/07/09 15:19:37, Srirama wrote: > > > > > On 2015/07/06 20:43:01, ddorwin wrote: > > > > > > Can WebMediaPlayerClient be private (similar to the FIXME that was > > > removed)? > > > > > > > > > > It is giving problem in frameloaderclientimpl while type casting > > > mediaelement > > > > to > > > > > webmediaplayerclient. > > > > > > > > I don't see such a cast (or even "webmediaplayerclient") in that file. Can > > you > > > > point me to the specific line (maybe with a "HERE" comment on that line)? > > > > > > > > philipj: Should this indeed be private inheritance as indicated by the > > FIXME? > > > > > > It's the implicit cast from HTMLMediaElement* to WebMediaPlayerClient* in > > > FrameLoaderClientImpl::createWebMediaPlayer that's now a problem. > > > > > > The original FIXME around MediaPlayerClient, where WebMediaPlayerClientImpl > > had > > > a cast from MediaPlayerClient* to HTMLMediaElement* was IMHO worse, because > > > there HTMLMediaElement* was used to bypass the layering in ugly ways, > > especially > > > in WebMediaPlayerClientImpl::load(). > > > > > > However, the layering is still not perfect. What remains is in order to > avoid > > > HTMLMediaElement depending on HTMLMediaElementEncryptedMedia, a core-modules > > > dependency violation. > > > > > > The most direct path forward I can see is some method in web/ that can > return > > a > > > WebMediaPlayerEncryptedMediaClient* and WebContentDecryptionModule* from a > > > HTMLMediaElement*, and then having HTMLMediaElement call > > > WebFrameClient::createMediaPlayer() itself, removing the > createWebMediaPlayer > > > helper. Do you think that would be OK, and if so what's a good place to put > > > those converters? > > > > Ah, thanks. I'm not an expert in where to put such things or whether that > would > > be better. Other options might include friending or getWebMediaPlayerClient(). > > > > For now, perhaps we should just leave a TODO (or pick a simple solution to > avoid > > people using WebMediaPlayerClient APIs). > > I think it would be OK to leave this as > > // TODO(srirama.m): Make the WebMediaPlayerClient inheritance private by adding > a means for getting a WebMediaPlayerEncryptedMediaClient and > WebContentDecryptionModule from an HTMLMediaElement and calling > WebFrameClient::createMediaPlayer() directly. > > Or if you want to try it out now, that'd also be OK, Srirama. Done. Added TODO for now. https://codereview.chromium.org/1055503002/diff/300001/public/platform/WebMed... File public/platform/WebMediaPlayerClient.h (right): https://codereview.chromium.org/1055503002/diff/300001/public/platform/WebMed... public/platform/WebMediaPlayerClient.h:79: virtual void mediaPlayerRequestSeek(double) = 0; On 2015/07/09 15:19:37, Srirama wrote: > On 2015/07/06 20:43:01, ddorwin wrote: > > Is there a reason this name was changed? > > As per Philip's suggestion in patchset3, i have changed it. Even he has the same > doubt now, probably he would have misunderstood the renaming that time. Done.
On 2015/07/15 13:25:24, Srirama wrote: > Rebased and addressed review comments. There seems to be an issue with > win_blink_compile_dbg bot with respect to WebMediaPlayerClient inheritance. > htmlmediaelement.h(73) : error C2220: warning treated as error - no 'object' > file generated > htmlmediaelement.h(73) : warning C4275: non dll-interface class > 'blink::WebMediaPlayerClient' used as base for dll-interface class > 'blink::HTMLMediaElement'. > May be other platforms have suppressed the warning. > Any suggestion what to do to resolve this? Maybe this has something to do with which classes are marked with CORE_EXPORT?
On 2015/07/15 13:45:38, philipj wrote:
> On 2015/07/15 13:25:24, Srirama wrote:
> > Rebased and addressed review comments. There seems to be an issue with
> > win_blink_compile_dbg bot with respect to WebMediaPlayerClient inheritance.
> > htmlmediaelement.h(73) : error C2220: warning treated as error - no 'object'
> > file generated
> > htmlmediaelement.h(73) : warning C4275: non dll-interface class
> > 'blink::WebMediaPlayerClient' used as base for dll-interface class
> > 'blink::HTMLMediaElement'.
> > May be other platforms have suppressed the warning.
> > Any suggestion what to do to resolve this?
>
> Maybe this has something to do with which classes are marked with CORE_EXPORT?
yes, it is done as part of blink componentization. There is a special handling
for win32 in CoreExport.h, not sure if that is causing problem.
win32:
#define CORE_EXPORT __declspec(dllexport)
!win32:
#define CORE_EXPORT __attribute__((visibility("default"))).
On 2015/07/15 13:49:40, Srirama wrote:
> On 2015/07/15 13:45:38, philipj wrote:
> > On 2015/07/15 13:25:24, Srirama wrote:
> > > Rebased and addressed review comments. There seems to be an issue with
> > > win_blink_compile_dbg bot with respect to WebMediaPlayerClient
inheritance.
> > > htmlmediaelement.h(73) : error C2220: warning treated as error - no
'object'
> > > file generated
> > > htmlmediaelement.h(73) : warning C4275: non dll-interface class
> > > 'blink::WebMediaPlayerClient' used as base for dll-interface class
> > > 'blink::HTMLMediaElement'.
> > > May be other platforms have suppressed the warning.
> > > Any suggestion what to do to resolve this?
> >
> > Maybe this has something to do with which classes are marked with
CORE_EXPORT?
>
> yes, it is done as part of blink componentization. There is a special handling
> for win32 in CoreExport.h, not sure if that is causing problem.
> win32:
> #define CORE_EXPORT __declspec(dllexport)
> !win32:
> #define CORE_EXPORT __attribute__((visibility("default"))).
Isn't it just that WebMediaPlayer* needs PLATFORM_EXPORT (or BLINK_EXPORT?) just
like MediaPlayer* had?
On 2015/07/15 13:58:37, philipj wrote:
> On 2015/07/15 13:49:40, Srirama wrote:
> > On 2015/07/15 13:45:38, philipj wrote:
> > > On 2015/07/15 13:25:24, Srirama wrote:
> > > > Rebased and addressed review comments. There seems to be an issue with
> > > > win_blink_compile_dbg bot with respect to WebMediaPlayerClient
> inheritance.
> > > > htmlmediaelement.h(73) : error C2220: warning treated as error - no
> 'object'
> > > > file generated
> > > > htmlmediaelement.h(73) : warning C4275: non dll-interface class
> > > > 'blink::WebMediaPlayerClient' used as base for dll-interface class
> > > > 'blink::HTMLMediaElement'.
> > > > May be other platforms have suppressed the warning.
> > > > Any suggestion what to do to resolve this?
> > >
> > > Maybe this has something to do with which classes are marked with
> CORE_EXPORT?
> >
> > yes, it is done as part of blink componentization. There is a special
handling
> > for win32 in CoreExport.h, not sure if that is causing problem.
> > win32:
> > #define CORE_EXPORT __declspec(dllexport)
> > !win32:
> > #define CORE_EXPORT __attribute__((visibility("default"))).
>
> Isn't it just that WebMediaPlayer* needs PLATFORM_EXPORT (or BLINK_EXPORT?)
just
> like MediaPlayer* had?
May be as per this
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
I can try with BLINK_PLATFORM_EXPORT
Patchset #13 (id:420001) has been deleted
Patchset #13 (id:440001) has been deleted
Tried with BLINK_PLATFORM_EXPORT, compiled successfully but there is a linking error. webcore_shared.HTMLMediaElement.obj : error LNK2019: unresolved external symbol "__declspec(dllimport) public: virtual __thiscall blink::WebMediaPlayerClient::~WebMediaPlayerClient(void)" (__imp_??1WebMediaPlayerClient@blink@@UAE@XZ) referenced in function "protected: virtual __thiscall blink::HTMLMediaElement::~HTMLMediaElement(void)" (??1HTMLMediaElement@blink@@MAE@XZ) webcore_shared.HTMLMediaElement.obj : error LNK2019: unresolved external symbol "__declspec(dllimport) public: __thiscall blink::WebMediaPlayerClient::WebMediaPlayerClient(void)" (__imp_??0WebMediaPlayerClient@blink@@QAE@XZ) referenced in function "protected: __thiscall blink::HTMLMediaElement::HTMLMediaElement(class blink::QualifiedName const &,class blink::Document &)" (??0HTMLMediaElement@blink@@IAE@ABVQualifiedName@1@AAVDocument@1@@Z) webcore_shared.dll : fatal error LNK1120: 2 unresolved externals Not sure how Animation.cpp in "core" is able to implement WebCompositorAnimationPlayerClient.
philipj@opera.com changed reviewers: + tasak@google.com
tasak@, I know you've worked a bit with these CORE_EXPORT and MODULES_EXPORT things, can you see at a glance what's wrong with this CL, why it isn't linking?
On 2015/07/16 11:02:45, Srirama wrote: > Tried with BLINK_PLATFORM_EXPORT, compiled successfully but there is a linking > error. > webcore_shared.HTMLMediaElement.obj : error LNK2019: unresolved external symbol > "__declspec(dllimport) public: virtual __thiscall > blink::WebMediaPlayerClient::~WebMediaPlayerClient(void)" > (__imp_??1WebMediaPlayerClient@blink@@UAE@XZ) referenced in function "protected: > virtual __thiscall blink::HTMLMediaElement::~HTMLMediaElement(void)" > (??1HTMLMediaElement@blink@@MAE@XZ) > > webcore_shared.HTMLMediaElement.obj : error LNK2019: unresolved external symbol > "__declspec(dllimport) public: __thiscall > blink::WebMediaPlayerClient::WebMediaPlayerClient(void)" > (__imp_??0WebMediaPlayerClient@blink@@QAE@XZ) referenced in function "protected: > __thiscall blink::HTMLMediaElement::HTMLMediaElement(class blink::QualifiedName > const &,class blink::Document &)" > (??0HTMLMediaElement@blink@@IAE@ABVQualifiedName@1@AAVDocument@1@@Z) > webcore_shared.dll : fatal error LNK1120: 2 unresolved externals > > Not sure how Animation.cpp in "core" is able to implement > WebCompositorAnimationPlayerClient. Try creating a "empty" .cpp file in platform/exported/ which just includes WebMediaPlayerClient.h (and config.h). WebCompositorAnimationPlayerClient probably has one you can copy and edit...
On 2015/07/16 11:48:48, fs wrote: > On 2015/07/16 11:02:45, Srirama wrote: > > Tried with BLINK_PLATFORM_EXPORT, compiled successfully but there is a linking > > error. > > webcore_shared.HTMLMediaElement.obj : error LNK2019: unresolved external > symbol > > "__declspec(dllimport) public: virtual __thiscall > > blink::WebMediaPlayerClient::~WebMediaPlayerClient(void)" > > (__imp_??1WebMediaPlayerClient@blink@@UAE@XZ) referenced in function > "protected: > > virtual __thiscall blink::HTMLMediaElement::~HTMLMediaElement(void)" > > (??1HTMLMediaElement@blink@@MAE@XZ) > > > > webcore_shared.HTMLMediaElement.obj : error LNK2019: unresolved external > symbol > > "__declspec(dllimport) public: __thiscall > > blink::WebMediaPlayerClient::WebMediaPlayerClient(void)" > > (__imp_??0WebMediaPlayerClient@blink@@QAE@XZ) referenced in function > "protected: > > __thiscall blink::HTMLMediaElement::HTMLMediaElement(class > blink::QualifiedName > > const &,class blink::Document &)" > > (??0HTMLMediaElement@blink@@IAE@ABVQualifiedName@1@AAVDocument@1@@Z) > > webcore_shared.dll : fatal error LNK1120: 2 unresolved externals > > > > Not sure how Animation.cpp in "core" is able to implement > > WebCompositorAnimationPlayerClient. > > Try creating a "empty" .cpp file in platform/exported/ which just includes > WebMediaPlayerClient.h (and config.h). WebCompositorAnimationPlayerClient > probably has one you can copy and edit... Thanks fs, i will try this.
Patchset #15 (id:500001) has been deleted
Thanks fs and philip, the issue is resolved now(Linux bot failure is unrelated). Hopefully it is ready for landing now. PTAL.
LGTM, though I didn't closely review HTMLMediaElement.*. philipj has to review and approve that anyway.
On 2015/07/16 17:24:01, ddorwin wrote: > LGTM, though I didn't closely review HTMLMediaElement.*. philipj has to review > and approve that anyway. @philip, can you please review one final time.
I'm still not sure about the Web Audio stuff, can you take a look, rtoy@? https://codereview.chromium.org/1055503002/diff/520001/Source/core/html/HTMLM... File Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/1055503002/diff/520001/Source/core/html/HTMLM... Source/core/html/HTMLMediaElement.cpp:3500: void HTMLMediaElement::createMediaPlayer() This is now incorrectly named, as it never creates a media player but rather clears the existing one. There's already a clearMediaPlayer(), so I don't know what to call this. Have you investigated which parts of this method are actually relevant at the two call sites? It sure would be nice if there were a single place to create a WebMediaPlayer (HTMLMediaElement::startPlayerLoad()) and a single place to destroy it. If the consequences of such refactoring are uncertain, renaming this to something that makes more sense and adding a TODO would be reasonable. https://codereview.chromium.org/1055503002/diff/520001/Source/core/html/HTMLM... Source/core/html/HTMLMediaElement.cpp:3517: // When creating the player, make sure its AudioSourceProvider knows about the client. This comment and the condition doesn't seem to make sense any longer. Will the audioSourceProvider() be null here, or an object wrapping a null pointer? https://codereview.chromium.org/1055503002/diff/520001/Source/core/html/HTMLM... File Source/core/html/HTMLMediaElement.h (right): https://codereview.chromium.org/1055503002/diff/520001/Source/core/html/HTMLM... Source/core/html/HTMLMediaElement.h:572: Remove blank lines between documentation here and for AudioSourceProviderImpl. https://codereview.chromium.org/1055503002/diff/520001/Source/core/html/HTMLM... Source/core/html/HTMLMediaElement.h:597: : m_webAudioSourceProvider(0) nullptr
lgtm for webaudio/MediaElementAudioSourceNode.cpp
On 2015/07/21 19:46:10, Raymond Toy wrote: > lgtm for webaudio/MediaElementAudioSourceNode.cpp Thanks Raymond, not sure if you have taken a look at the webaudio code in HTMLMediaElement.*. The webaudio related code in WebMediaPlayerClientImpl.* has been moved to HTMLMediaElement.*(at the end of the file). Please have a look if you have not reviewed it.
On 2015/07/22 03:41:58, Srirama wrote: > On 2015/07/21 19:46:10, Raymond Toy wrote: > > lgtm for webaudio/MediaElementAudioSourceNode.cpp > > Thanks Raymond, not sure if you have taken a look at the webaudio code in > HTMLMediaElement.*. > The webaudio related code in WebMediaPlayerClientImpl.* has been moved to > HTMLMediaElement.*(at the end of the file). > Please have a look if you have not reviewed it. Yes indeed, I would like a second opinion on these bits.
https://codereview.chromium.org/1055503002/diff/520001/Source/core/html/HTMLM... File Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/1055503002/diff/520001/Source/core/html/HTMLM... Source/core/html/HTMLMediaElement.cpp:3500: void HTMLMediaElement::createMediaPlayer() On 2015/07/21 08:05:10, philipj wrote: > This is now incorrectly named, as it never creates a media player but rather > clears the existing one. There's already a clearMediaPlayer(), so I don't know > what to call this. > > Have you investigated which parts of this method are actually relevant at the > two call sites? It sure would be nice if there were a single place to create a > WebMediaPlayer (HTMLMediaElement::startPlayerLoad()) and a single place to > destroy it. > > If the consequences of such refactoring are uncertain, renaming this to > something that makes more sense and adding a TODO would be reasonable. Done. Couldn't think of a better name than this. https://codereview.chromium.org/1055503002/diff/520001/Source/core/html/HTMLM... Source/core/html/HTMLMediaElement.cpp:3517: // When creating the player, make sure its AudioSourceProvider knows about the client. On 2015/07/21 08:05:10, philipj wrote: > This comment and the condition doesn't seem to make sense any longer. Will the > audioSourceProvider() be null here, or an object wrapping a null pointer? Done. It will not be null. https://codereview.chromium.org/1055503002/diff/520001/Source/core/html/HTMLM... File Source/core/html/HTMLMediaElement.h (right): https://codereview.chromium.org/1055503002/diff/520001/Source/core/html/HTMLM... Source/core/html/HTMLMediaElement.h:572: On 2015/07/21 08:05:10, philipj wrote: > Remove blank lines between documentation here and for AudioSourceProviderImpl. Done. https://codereview.chromium.org/1055503002/diff/520001/Source/core/html/HTMLM... Source/core/html/HTMLMediaElement.h:597: : m_webAudioSourceProvider(0) On 2015/07/21 08:05:10, philipj wrote: > nullptr Done.
OK, Web Audio remains the only point of doubt. Have you looked into whether both the AudioSourceProvider and WebAudioSourceProvider layers are still going to be needed, or there's an extra layer here too? I don't propose to remove them in this CL, just curious since locally in HTMLMediaElement the extra indirection doesn't look very meaningful. https://codereview.chromium.org/1055503002/diff/520001/Source/core/html/HTMLM... File Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/1055503002/diff/520001/Source/core/html/HTMLM... Source/core/html/HTMLMediaElement.cpp:3500: void HTMLMediaElement::createMediaPlayer() On 2015/07/22 14:42:09, Srirama wrote: > On 2015/07/21 08:05:10, philipj wrote: > > This is now incorrectly named, as it never creates a media player but rather > > clears the existing one. There's already a clearMediaPlayer(), so I don't know > > what to call this. > > > > Have you investigated which parts of this method are actually relevant at the > > two call sites? It sure would be nice if there were a single place to create a > > WebMediaPlayer (HTMLMediaElement::startPlayerLoad()) and a single place to > > destroy it. > > > > If the consequences of such refactoring are uncertain, renaming this to > > something that makes more sense and adding a TODO would be reasonable. > > Done. Couldn't think of a better name than this. That works well enough, thanks! https://codereview.chromium.org/1055503002/diff/520001/Source/core/html/HTMLM... Source/core/html/HTMLMediaElement.cpp:3517: // When creating the player, make sure its AudioSourceProvider knows about the client. On 2015/07/22 14:42:09, Srirama wrote: > On 2015/07/21 08:05:10, philipj wrote: > > This comment and the condition doesn't seem to make sense any longer. Will the > > audioSourceProvider() be null here, or an object wrapping a null pointer? > > Done. It will not be null. The extra {} can now be removed too.
On 2015/07/22 14:54:06, philipj wrote: > OK, Web Audio remains the only point of doubt. Have you looked into whether both > the AudioSourceProvider and WebAudioSourceProvider layers are still going to be > needed, or there's an extra layer here too? I don't propose to remove them in > this CL, just curious since locally in HTMLMediaElement the extra indirection > doesn't look very meaningful. > > https://codereview.chromium.org/1055503002/diff/520001/Source/core/html/HTMLM... > File Source/core/html/HTMLMediaElement.cpp (right): > > https://codereview.chromium.org/1055503002/diff/520001/Source/core/html/HTMLM... > Source/core/html/HTMLMediaElement.cpp:3500: void > HTMLMediaElement::createMediaPlayer() > On 2015/07/22 14:42:09, Srirama wrote: > > On 2015/07/21 08:05:10, philipj wrote: > > > This is now incorrectly named, as it never creates a media player but rather > > > clears the existing one. There's already a clearMediaPlayer(), so I don't > know > > > what to call this. > > > > > > Have you investigated which parts of this method are actually relevant at > the > > > two call sites? It sure would be nice if there were a single place to create > a > > > WebMediaPlayer (HTMLMediaElement::startPlayerLoad()) and a single place to > > > destroy it. > > > > > > If the consequences of such refactoring are uncertain, renaming this to > > > something that makes more sense and adding a TODO would be reasonable. > > > > Done. Couldn't think of a better name than this. > > That works well enough, thanks! > > https://codereview.chromium.org/1055503002/diff/520001/Source/core/html/HTMLM... > Source/core/html/HTMLMediaElement.cpp:3517: // When creating the player, make > sure its AudioSourceProvider knows about the client. > On 2015/07/22 14:42:09, Srirama wrote: > > On 2015/07/21 08:05:10, philipj wrote: > > > This comment and the condition doesn't seem to make sense any longer. Will > the > > > audioSourceProvider() be null here, or an object wrapping a null pointer? > > > > Done. It will not be null. > > The extra {} can now be removed too. Infact i thought about the same when i initially started this work, so i thought i will take it up after finishing this as i don't have much of a knowledge on that part. I will look into it after successfully landing this :)
I've compared the AudioSourceProvider bits in the removed and new code and only found one small difference, so LGTM it is. https://codereview.chromium.org/1055503002/diff/540001/Source/core/html/HTMLM... File Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/1055503002/diff/540001/Source/core/html/HTMLM... Source/core/html/HTMLMediaElement.cpp:3770: if (!bus) In the original code there was an ASSERT(bus) before this, which was strange. Did you determine that it is in fact possible for bus to be null here?
On 2015/07/22 15:14:53, philipj wrote: > I've compared the AudioSourceProvider bits in the removed and new code and only > found one small difference, so LGTM it is. > > https://codereview.chromium.org/1055503002/diff/540001/Source/core/html/HTMLM... > File Source/core/html/HTMLMediaElement.cpp (right): > > https://codereview.chromium.org/1055503002/diff/540001/Source/core/html/HTMLM... > Source/core/html/HTMLMediaElement.cpp:3770: if (!bus) > In the original code there was an ASSERT(bus) before this, which was strange. > Did you determine that it is in fact possible for bus to be null here? I haven't checked if it can be null or not. In any case both are not required. So i just removed assert infavour of this. Should i do the reverse?
Patchset #17 (id:560001) has been deleted
https://codereview.chromium.org/1055503002/diff/540001/Source/core/html/HTMLM... File Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/1055503002/diff/540001/Source/core/html/HTMLM... Source/core/html/HTMLMediaElement.cpp:3770: if (!bus) On 2015/07/22 15:14:53, philipj wrote: > In the original code there was an ASSERT(bus) before this, which was strange. > Did you determine that it is in fact possible for bus to be null here? That depends on whether or not it can actually be null. The original code was confused about this. If you don't want to investigate it now, you can put the assert back together with a TODO to either remove the assert of remove the null check.
https://codereview.chromium.org/1055503002/diff/540001/Source/core/html/HTMLM... File Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/1055503002/diff/540001/Source/core/html/HTMLM... Source/core/html/HTMLMediaElement.cpp:3770: if (!bus) On 2015/07/23 07:25:40, philipj wrote: > On 2015/07/22 15:14:53, philipj wrote: > > In the original code there was an ASSERT(bus) before this, which was strange. > > Did you determine that it is in fact possible for bus to be null here? > > That depends on whether or not it can actually be null. The original code was > confused about this. If you don't want to investigate it now, you can put the > assert back together with a TODO to either remove the assert of remove the null > check. Done. Added TODO with original code.
The CQ bit was checked by philipj@opera.com
lgtm Sending to CQ, thanks for sticking with through all of the problems!
The patchset sent to the CQ was uploaded after l-g-t-m from ddorwin@chromium.org, rtoy@chromium.org Link to the patchset: https://codereview.chromium.org/1055503002/#ps580001 (title: "address comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1055503002/580001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1055503002/580001
Message was sent while issue was closed.
Committed patchset #17 (id:580001) as https://src.chromium.org/viewvc/blink?view=rev&revision=199375
Message was sent while issue was closed.
Thanks Philip and David for your patient reviews and valuable suggestions. Thanks fs@ for your valuable tips when i got stuck at times.
Message was sent while issue was closed.
Sorry for being slow in reviewing. The HTMLMedia stuff looks ok, but I'm not very familiar with that code. dalecurtis@ might be more familiar.
Message was sent while issue was closed.
Nope :)
Message was sent while issue was closed.
On 2015/07/23 21:03:39, DaleCurtis wrote: > Nope :) @Raymond: no problem, philip had a look at it and it is just a mere movement of code between files. So it should not be a problem. |
