Bug 41295 – [Chromium] Add chromium WebMediaPlayer to PlatformMedia

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Bo Liu]

Also add methods in WebNode to retrieve the WebMediaPlayer of a HTMLMediaElement

[Bo Liu]

Created attachment 59934 [details]
Patch

[Jeremy Orlow]

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

[Bo Liu]

Created attachment 60259 [details]
Patch

[Bo Liu]

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.

[Darin Fisher (:fishd, Google)]

r- because of the WebKit namespace usage in WebCore.  that other ports are doing it is not a valid excuse.

[Bo Liu]

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?

[Bo Liu]

Created attachment 61257 [details]
Patch

[Darin Fisher (:fishd, Google)]

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.

[Bo Liu]

Created attachment 61276 [details]
Patch

[Bo Liu]

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.

[Darin Fisher (:fishd, Google)]

(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 :)

[Darin Fisher (:fishd, Google)]

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();
}

[Bo Liu]

Created attachment 61537 [details]
Patch

[Bo Liu]

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.

[Darin Fisher (:fishd, Google)]

(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/.

[Darin Fisher (:fishd, Google)]

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

[Bo Liu]

Created attachment 61542 [details]
Patch

[Darin Fisher (:fishd, Google)]

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

[Bo Liu]

Created attachment 62216 [details]
Patch

[WebKit Commit Bot]

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

[Andrew Scherkus]

I'll fix the ChangeLog and land the patch.

[Andrew Scherkus]

Landed as http://trac.webkit.org/changeset/63859