Add class WebFullscreenMediaPlayer that interfaces between WebMediaPlayer and WebWidgetClient. Add methods for WebMediaPlayer to set the WebMediaPlayerClient. WebFullscreenMediaPlayer inherits from WebWidget (interface used by WebWidgetClient) and WebMediaPlayerClient (interface used by WebMediaPlayer). WebFullscreenMediaPlayer shortcuts the invalidate/paint cycle. This patches depends on this chromium patch (http://codereview.chromium.org/3055009/show). Will upload corresponding chromium patch.
Created attachment 63281 [details] Patch
Corresponding chromium patch: http://codereview.chromium.org/3026043
Attachment 63281 [details] did not build on chromium: Build output: http://queues.webkit.org/results/3619490
Comment on attachment 63281 [details] Patch Drive by comment. > diff --git a/WebKit/chromium/public/WebFullscreenMediaPlayer.h b/WebKit/chromium/public/WebFullscreenMediaPlayer.h > + WebFullscreenMediaPlayer(WebWidgetClient* webWidgetClient); The parameter name "webWidgetClient" shouldn't be here.
Adding Darin Fisher since this involves the chromium public api.
Created attachment 63358 [details] Patch
Attachment 63358 [details] did not build on chromium: Build output: http://queues.webkit.org/results/3632402
Comment on attachment 63358 [details] Patch WebKit/chromium/public/WebMediaPlayer.h:84 + virtual WebMediaPlayerClient* GetClient() const = 0; use webkit style here. this should be client() and setClient(), but i have to ask: why do you need the client to be accessible like this, and why do you need it to be mutable? we have so far tried to avoid exposing clients like this. what problem are you trying to solve? WebKit/chromium/public/WebPopupType.h:40 + WebPopupTypeFullscreenMediaPlayer, why does this popup type need to be media player specific? in the future, we may have other needs for a fullscreen popup, and i think the code you are adding could be used for that too. so i would drop the MediaPlayer suffix. WebKit/chromium/public/WebViewClient.h:91 + virtual WebWidget* createFullscreenWindow(WebPopupType) { return 0; } i don't see why you need to pass WebPopupType to this method. but instead of removing it, perhaps it would be better to combine this function with the createPopupMenu(WebPopupType) method from above into a single: virtual WebWidget* createPopup(WebPopupType); then, deprecate the createPopupMenu(WebPopupType) method. WebKit/chromium/public/WebFullscreenMediaPlayer.h:53 + class WebFullscreenMediaPlayer : public WebWidget, implementation classes like this should go in chromium/src. what you should do in this case is make WebFullscreenMediaPlayer be an interface with a static create function (see WebPopupMenu.h). in chromium/src, you can have an implementation of WebFullscreenMediaPlayer.
Get/SetClient is used to replace the client in order to paint into full screen. Is the general design of replacing the client for full screen okay, or should WebMediaPlayer have methods like enterFullscreen(WebMediaPlayerFullscreenClient*) and exitFullscreen? Will get the other issues fixed.
yeah the idea is to swap the client so the fullscreen RenderWidget can intercept the events bubbled up from the WebMediaPlayer (repaints, etc..)
(In reply to comment #9) > Get/SetClient is used to replace the client in order to paint into full screen. Can the client be a parameter to WebFullscreenMediaPlayer upon construction? > Is the general design of replacing the client for full screen okay, or should WebMediaPlayer have methods like enterFullscreen(WebMediaPlayerFullscreenClient*) and exitFullscreen? I don't follow this suggestion. In general, it is better for clients to be immutable and specified at object construction time.
do you need to intercept all client methods? or if it is only a subset of methods, should you define a WebFullscreenMediaPlayerClient interface?
(In reply to comment #12) > do you need to intercept all client methods? or if it is only a subset of methods, should you define a WebFullscreenMediaPlayerClient interface? Right now only the repaint call is intercepted. All other WebMediaPlayerClient calls are forwarded to the original client. However, there is no media control UI in full screen yet. The other calls will probably be needed intercepted as well when the controls are in place.
> WebKit/chromium/public/WebPopupType.h:40 > + WebPopupTypeFullscreenMediaPlayer, > why does this popup type need to be media player specific? in the future, > we may have other needs for a fullscreen popup, and i think the code you > are adding could be used for that too. so i would drop the MediaPlayer > suffix. > > WebKit/chromium/public/WebViewClient.h:91 > + virtual WebWidget* createFullscreenWindow(WebPopupType) { return 0; } > i don't see why you need to pass WebPopupType to this method. > > but instead of removing it, perhaps it would be better to combine > this function with the createPopupMenu(WebPopupType) method from > above into a single: > > virtual WebWidget* createPopup(WebPopupType); > > then, deprecate the createPopupMenu(WebPopupType) method. I thought about creating a generic full screen pop up type, and it might not work very well. Every WebPopupType match to one concrete sublcass of WebWidget. Then WebViewClient::createPopup will return a WebWidget* based on the pop up type passed in. If there is only a generic full screen type, it can only return a generic full screen subclass of WebWidget. Then the WebWidget will somehow need to be customized for full screen video, probably by introducing another interface. Is it possible for future uses of full screen to add new pop up types?
Created attachment 63389 [details] Patch
Attachment 63389 [details] did not build on chromium: Build output: http://queues.webkit.org/results/3577836
Created attachment 64644 [details] Patch Fix handling keyboard inputs
Attachment 64644 [details] did not build on chromium: Build output: http://queues.webkit.org/results/3707317
so I don't see anything glaringly wrong with this patch -- overall makes sense however I'm not a WebKit reviewer so fishd should take another glance
Created attachment 71515 [details] Patch
I'm picking up boliu's leftover fullscreen patches. The general design was to try to reuse as much as the RenderWidget IPC codepath as possible. So we have a popup type (WebPopupTypeFullscreenMediaPlayer) and an implementation of it (WebFullscreenMediaPlayerClientImpl) The general idea is to swap the RenderWidget on WebMediaPlayerImpl so that WebMediaPlayerImpl starts receiving commands from the full screen widget (i.e., seeking, pause, volume, paint) and responds with events. I'm not sure if this is still the right approach so I'm open to ideas!
Attachment 71515 [details] did not build on chromium: Build output: http://queues.webkit.org/results/4684061
Comment on attachment 71515 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=71515&action=review I'm worried that this patch introduces memory safety issues. It might well be fine, but I'm not sure it's correct. > WebKit/chromium/src/ChromeClientImpl.cpp:836 > + fullscreenClient->exitFullscreen(); When does the client get deleted? > WebKit/chromium/src/WebFullscreenMediaPlayerClientImpl.cpp:43 > +namespace WebKit { Missing blank line after this line. > WebKit/chromium/src/WebFullscreenMediaPlayerClientImpl.cpp:46 > + return new WebFullscreenMediaPlayerClientImpl(client); Is this normal memory management for the Chromium API? In WebCore, we'd use something like a PassOwnPtr. > WebKit/chromium/src/WebFullscreenMediaPlayerClientImpl.cpp:84 > + delete this; I see. This seems like a dangerous pattern for memory management, but maybe it's the norm? > WebKit/chromium/src/WebFullscreenMediaPlayerClientImpl.cpp:94 > + // Called when first created. Can this be called multiple times? I don't really understand this comment. > WebKit/chromium/src/WebFullscreenMediaPlayerClientImpl.cpp:118 > + switch (static_cast<const WebKeyboardEvent&>(event).windowsKeyCode) { Bad indent. > WebKit/chromium/src/WebFullscreenMediaPlayerClientImpl.cpp:121 > + m_mediaElement->play(m_mediaElement->processingUserGesture()); Why are we asking the media element if its processing a user gesture? Whether we're processing a user gesture is static state. > WebKit/chromium/src/WebFullscreenMediaPlayerClientImpl.h:103 > + // Weak pointer to WebWidgetClient. It's not technically a weak pointer, right? There's no notification of when the client destructs itself. > WebKit/chromium/src/WebFullscreenMediaPlayerClientImpl.h:109 > + // Pointer to the corresponding media element is saved > + // for properly exiting full screen as a result of user > + // action. > + WebCore::HTMLMediaElement* m_mediaElement; We don't need to hold a reference to the element here? What if it gets removed from the DOM and deallocated?
holy smokes I hope you didn't spend too much time reviewing this adam! I think this patch (and possibly associated ones) will be made obsolete due to recent "real" fullscreen support
(In reply to comment #24) > holy smokes I hope you didn't spend too much time reviewing this adam! No problem. I'm just trying to clear out the backlog of seemingly forgotten review requests. > I think this patch (and possibly associated ones) will be made obsolete due to recent "real" fullscreen support It would be helpful if you could mark them obsolete in the bug tracker so they don't show up in http://webkit.org/pending-review. Glad to hear we're getting real fullscreen support. :)