Bug 43394 – [Chromium] Full screen media player interface between WebMediaPlayer and WebWidgetClient

• Status: RESOLVED
• Resolution: WONTFIX
• Component: WebKit Misc.

[Bo Liu]

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.

[Bo Liu]

Created attachment 63281 [details]
Patch

[Bo Liu]

Corresponding chromium patch: http://codereview.chromium.org/3026043

[WebKit Review Bot]

Attachment 63281 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/3619490

[David Levin]

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.

[David Levin]

Adding Darin Fisher since this involves the chromium public api.

[Bo Liu]

Created attachment 63358 [details]
Patch

[WebKit Review Bot]

Attachment 63358 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/3632402

[Darin Fisher (:fishd, Google)]

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.

[Bo Liu]

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.

[Andrew Scherkus]

yeah the idea is to swap the client so the fullscreen RenderWidget can intercept the events bubbled up from the WebMediaPlayer (repaints, etc..)

[Darin Fisher (:fishd, Google)]

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

[Darin Fisher (:fishd, Google)]

do you need to intercept all client methods?  or if it is only a subset of methods, should you define a WebFullscreenMediaPlayerClient interface?

[Bo Liu]

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

[Bo Liu]

> 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?

[Bo Liu]

Created attachment 63389 [details]
Patch

[WebKit Review Bot]

Attachment 63389 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/3577836

[Bo Liu]

Created attachment 64644 [details]
Patch

Fix handling keyboard inputs

[WebKit Review Bot]

Attachment 64644 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/3707317

[Andrew Scherkus]

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

[Andrew Scherkus]

Created attachment 71515 [details]
Patch

[Andrew Scherkus]

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!

[WebKit Review Bot]

Attachment 71515 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/4684061

[Adam Barth]

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?

[Andrew Scherkus]

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

[Adam Barth]

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