WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
41295
[Chromium] Add chromium WebMediaPlayer to PlatformMedia
https://bugs.webkit.org/show_bug.cgi?id=41295
Summary
[Chromium] Add chromium WebMediaPlayer to PlatformMedia
Bo Liu
Reported
2010-06-28 12:41:51 PDT
Also add methods in WebNode to retrieve the WebMediaPlayer of a HTMLMediaElement
Attachments
Patch
(5.65 KB, patch)
2010-06-28 13:47 PDT
,
Bo Liu
no flags
Details
Formatted Diff
Diff
Patch
(5.65 KB, patch)
2010-07-01 09:38 PDT
,
Bo Liu
no flags
Details
Formatted Diff
Diff
Patch
(5.62 KB, patch)
2010-07-12 12:50 PDT
,
Bo Liu
no flags
Details
Formatted Diff
Diff
Patch
(10.52 KB, patch)
2010-07-12 15:14 PDT
,
Bo Liu
no flags
Details
Formatted Diff
Diff
Patch
(11.55 KB, patch)
2010-07-14 10:34 PDT
,
Bo Liu
no flags
Details
Formatted Diff
Diff
Patch
(11.55 KB, patch)
2010-07-14 11:11 PDT
,
Bo Liu
no flags
Details
Formatted Diff
Diff
Patch
(11.76 KB, patch)
2010-07-21 12:05 PDT
,
Bo Liu
fishd
: review+
commit-queue
: commit-queue-
Details
Formatted Diff
Diff
Show Obsolete
(6)
View All
Add attachment
proposed patch, testcase, etc.
Bo Liu
Comment 1
2010-06-28 13:47:54 PDT
Created
attachment 59934
[details]
Patch
Jeremy Orlow
Comment 2
2010-06-30 22:45:57 PDT
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
Comment 3
2010-07-01 09:38:03 PDT
Created
attachment 60259
[details]
Patch
Bo Liu
Comment 4
2010-07-01 09:47:08 PDT
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)
Comment 5
2010-07-01 16:37:47 PDT
r- because of the WebKit namespace usage in WebCore. that other ports are doing it is not a valid excuse.
Bo Liu
Comment 6
2010-07-02 11:02:58 PDT
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
Comment 7
2010-07-12 12:50:06 PDT
Created
attachment 61257
[details]
Patch
Darin Fisher (:fishd, Google)
Comment 8
2010-07-12 13:07:43 PDT
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
Comment 9
2010-07-12 15:14:57 PDT
Created
attachment 61276
[details]
Patch
Bo Liu
Comment 10
2010-07-13 17:28:47 PDT
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)
Comment 11
2010-07-13 23:57:11 PDT
(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 12
2010-07-14 00:03:37 PDT
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
Comment 13
2010-07-14 10:34:39 PDT
Created
attachment 61537
[details]
Patch
Bo Liu
Comment 14
2010-07-14 10:40:39 PDT
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)
Comment 15
2010-07-14 10:54:03 PDT
(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 16
2010-07-14 10:55:51 PDT
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
Comment 17
2010-07-14 11:11:46 PDT
Created
attachment 61542
[details]
Patch
Darin Fisher (:fishd, Google)
Comment 18
2010-07-21 11:38:31 PDT
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
Comment 19
2010-07-21 12:05:51 PDT
Created
attachment 62216
[details]
Patch
WebKit Commit Bot
Comment 20
2010-07-21 14:24:06 PDT
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
Comment 21
2010-07-21 14:52:56 PDT
I'll fix the ChangeLog and land the patch.
Andrew Scherkus
Comment 22
2010-07-21 15:12:46 PDT
Landed as
http://trac.webkit.org/changeset/63859
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug