Also add methods in WebNode to retrieve the WebMediaPlayer of a HTMLMediaElement
Created attachment 59934 [details] Patch
Comment on attachment 59934 [details] Patch Drive by comments. No r- because someone else should still take a look at this. WebCore/platform/graphics/MediaPlayer.h:82 + WebKit::WebMediaPlayer* chromiumWebMediaPlayer; Isn't this a layering violation? WebKit/chromium/src/WebNode.cpp:188 + { goes on previous line WebKit/chromium/src/WebNode.cpp:190 + return (element->platformMedia()).media.chromiumWebMediaPlayer; why the ()'s? WebKit/chromium/public/WebNode.h:109 + // 0 if |m_private| is another type. no need to wrap
Created attachment 60259 [details] Patch
About the layering violation: Other platforms are already forward declaring their media classes in WebCore/platform/graphics/MediaPlayer.h, so I thought adding the chromium one is okay (I could be wrong). Also I think the forward declaration does not introduce additional build dependencies.
r- because of the WebKit namespace usage in WebCore. that other ports are doing it is not a valid excuse.
This patch is part of implementing full screen video. The client is passed a WebNode wrapping a HTMLVideoElement to full screen. My idea was to poke a hole through HTMLVideoElement to retrieve the WebMediaPlayer and cast it down to WebMediaPlayerImpl. Then the full screen changes can be made mostly in Chromium code. Are there any suggestions for achieving this without the layering violation? Add an interface in WebCore for platform players? Use a void* pointer in struct PlatformMedia?
Created attachment 61257 [details] Patch
Comment on attachment 61257 [details] Patch WebCore/platform/graphics/MediaPlayer.h:78 + // Using void* instead of WebKit::WebMediaPlayer this comment introduces WebKit::WebMediaPlayer into WebCore in a way that the compiler won't even help us notice if we ever rename WebKit::WebMediaPlayer. that seems bad. HTMLMediaElement has a reference to a MediaPlayer object. A MediaPlayer has a MediaPlayerPrivateInterface member as well as a MediaPlayerClient member, and WebMediaPlayerClientImpl implements both of those interfaces. You should be able to static_cast from one of those interfaces to WebMediaPlayerClientImpl and gets the WebMediaPlayer from there. This casting can be done at the WebKit/chromium/src level. BTW, I think you need to invent a WebMediaElement subclass of WebElement instead of adding a method on WebNode.
Created attachment 61276 [details] Patch
Thanks so much for the pointers!! Should other methods in HTMLMediaElement be ported to WebMediaElement or only when they are used? Also MediaPlayerClient is implemented by HTMLMediaElement, so the only choice was to use MediaPlayerPrivateInterface, which seems kind of bad to expose since it has the word private in it.
(In reply to comment #10) > Thanks so much for the pointers!! > > Should other methods in HTMLMediaElement be ported to WebMediaElement or only > when they are used? I would only do it as needed. > Also MediaPlayerClient is implemented by HTMLMediaElement, so the only choice > was to use MediaPlayerPrivateInterface, which seems kind of bad to expose since > it has the word private in it. Oh, right. I think that is fine. WebMediaPlayerClientImpl.cpp is the implementation of MediaPlayerPrivateInterface, so it can know about it :)
Comment on attachment 61276 [details] Patch WebKit/chromium/public/WebMediaElement.h:46 + WEBKIT_API WebMediaPlayer* getWebMediaPlayer() const; webkit style is to avoid the "get" prefix for simple getter functions. how about just calling this method mediaPlayer()? we drop the "web" prefix for method names that return a Web* type. WebKit/chromium/src/WebMediaElement.cpp:46 + PlatformMedia pm = constUnwrap<HTMLMediaElement>()->platformMedia(); I think it would be better to create a method named fromMediaElement on WebMediaPlayerClientImpl. That hides the details of how a WebMediaPlayerClientImpl can be extracted from a HTMLMediaElement in the WebMediaPlayerClientImpl class. You should also create a public accessor on WebMediaPlayerClientImpl named mediaPlayer() that returns m_webMediaPlayer.get(). That way you do not need to use friend to access private data. So, for this method you should end up with: WebMediaPlayer* WebMediaElement::getWebMediaPlayer() const { return WebMediaPlayerClientImpl::fromMediaElement(this)->mediaPlayer(); }
Created attachment 61537 [details] Patch
Renamed chromiumWebMediaPlayer to chromiumMediaPlayer. Renamed WebMediaElement::getWebMediaPlayer() to player() to match HTMLMediaElement::player(). Is the use of constUnwrap without a #if WEBKIT_IMPLEMENTATION guard okay? It seems it used without the guard at other places, but declaration of constUnwrap is under the guard.
(In reply to comment #14) > Renamed chromiumWebMediaPlayer to chromiumMediaPlayer. Renamed > WebMediaElement::getWebMediaPlayer() to player() to match > HTMLMediaElement::player(). ^^^ good idea > Is the use of constUnwrap without a #if WEBKIT_IMPLEMENTATION guard okay? It > seems it used without the guard at other places, but declaration of constUnwrap > is under the guard. constUnwrap is intended to be used in chromium/src/. The WEBKIT_IMPLEMENTATION macro is defined when compiling code in chromium/src/. The code in chromium/public/ however can be compiled with and without WEBKIT_IMPLEMENTATION. That's why you should only see that macro used in chromium/public/.
Comment on attachment 61537 [details] Patch WebCore/platform/graphics/MediaPlayer.h:71 + ChromiumWebMediaPlayerType, nit: ChromiumWebMediaPlayerType should probably be renamed ChromiumMediaPlayerType to match the variable name. R=me otherwise
Created attachment 61542 [details] Patch
Comment on attachment 61542 [details] Patch WebKit/chromium/src/WebMediaPlayerClientImpl.cpp:77 > + WebCore::PlatformMedia pm = element->constUnwrap<HTMLMediaElement>()->platformMedia(); nit: no need for WebCore:: prefix in this file since there is a 'using namespace WebCore' at the top of the file. > + WebCore::PlatformMedia pm; ditto R=me, but please fix that before committing.WebKit/chromium/src/WebMediaPlayerClientImpl.cpp:174
Created attachment 62216 [details] Patch
Comment on attachment 62216 [details] Patch Rejecting patch 62216 from commit-queue. Failed to run "[u'git', u'svn', u'dcommit']" exit_code: 1 Last 500 characters of output: src/WebMediaElement.cpp M WebKit/chromium/src/WebMediaPlayerClientImpl.cpp M WebKit/chromium/src/WebMediaPlayerClientImpl.h A repository hook failed: MERGE request failed on '/repository/webkit/trunk': Commit blocked by pre-commit hook (exit code 1) with output: svnlook: Can't write to stream: Broken pipe The following ChangeLog files contain OOPS: trunk/WebCore/ChangeLog Please don't ever say "OOPS" in a ChangeLog file. at /usr/local/git/libexec/git-core/git-svn line 570 Full output: http://queues.webkit.org/results/3598250
I'll fix the ChangeLog and land the patch.
Landed as http://trac.webkit.org/changeset/63859