Bug 51133 – Provide an interface to force using fullscreen mediaplayer

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Yi Shen]

Based on the worked done by Tor Arne Vestbo ([e-mail]), we try to add a function shouldForceFullScreenVideoPlayback in ChromeClient to force webkit to launch full screen media player for playing the html5 video. The idea is that a browser vendor can specify this behavior through the platform plugin. This patch is only the WebCore-part of forcing fullscreen player.

[Yi Shen]

Created attachment 76686 [details]
first try

[Antonio Gomes]

> 10 idea is that a browser vendor can specify this behavior through the
> 11 platform plugin. 

Do all platforms have a platform plugin-like thing available?

[Jer Noble]

Why are the changes to MediaPlayer necessary?  Why not call the chrome() client methods from HTMLMediaElement?

[Yi Shen]

Created attachment 76764 [details]
minor changes

[Yi Shen]

(In reply to comment #2)
> > 10 idea is that a browser vendor can specify this behavior through the
> > 11 platform plugin. 
> 
> Do all platforms have a platform plugin-like thing available?
Good question! I am not sure about the answer but just like qtwebkit did for the HitTestResult padding -- first, it hacks webcore, then qtwebkit extends its platform plugin to define the paddings.

So, I think it makes sense to add a new interface in webcore::chromeclient to hack the webkit, then for Qt, we can extend the platform plugin to force fullscreen; and for other platforms, they also can force fullscreen by changing the implementation of their own chrome client.

[Yi Shen]

(In reply to comment #3)
> Why are the changes to MediaPlayer necessary?  Why not call the chrome() client methods from HTMLMediaElement?

Just want to make it readable and clear :) I can change the code if you guys don't like it.

[Eric Carlson]

Comment on attachment 76764 [details]
minor changes

View in context: https://bugs.webkit.org/attachment.cgi?id=76764&action=review

r- for the layering problem.


> WebCore/ChangeLog:5
> +        Provide an interface to force using fullscreen mediaplayer

"force" isn't right if the method names changes as I suggest below.

> WebCore/ChangeLog:13
> +        No new tests. as the changes should not change functionality.

Typo, either replace the "." with a "," or capitalize "as".

> WebCore/page/ChromeClient.h:268
> +#if ENABLE(VIDEO) 
> +        virtual bool shouldForceFullScreenVideoPlayback() { return false; } 
> +#endif

I am not sure about this name, maybe "requiresFullscreenForVideoPlayback()"?

> WebCore/platform/graphics/MediaPlayer.cpp:443
> +bool MediaPlayer::shouldForceFullscreenVideoPlayback() const 
> +{ 
> +    Document* doc = document(); 
> +    if (!doc || !doc->page()) 
> +        return false; 
> +
> +    return doc->page()->chrome()->client()->shouldForceFullScreenVideoPlayback();
> +}
> +

This layering doesn't make sense. MediaPlayer is the bridge between the platform specific media engine and the rest of WebCore, so this method should only be here if the answer comes from the media engine. If the answer comes from the chrome client, HTMLMediaElement should call it directly - as it already does to enter and exit fullscreen.

[Yi Shen]

(In reply to comment #7)
> (From update of attachment 76764 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=76764&action=review
> 
> r- for the layering problem.
> 
> 
> > WebCore/ChangeLog:5
> > +        Provide an interface to force using fullscreen mediaplayer
> 
> "force" isn't right if the method names changes as I suggest below.
> 
> > WebCore/ChangeLog:13
> > +        No new tests. as the changes should not change functionality.
> 
> Typo, either replace the "." with a "," or capitalize "as".
> 
> > WebCore/page/ChromeClient.h:268
> > +#if ENABLE(VIDEO) 
> > +        virtual bool shouldForceFullScreenVideoPlayback() { return false; } 
> > +#endif
> 
> I am not sure about this name, maybe "requiresFullscreenForVideoPlayback()"?
> 
> > WebCore/platform/graphics/MediaPlayer.cpp:443
> > +bool MediaPlayer::shouldForceFullscreenVideoPlayback() const 
> > +{ 
> > +    Document* doc = document(); 
> > +    if (!doc || !doc->page()) 
> > +        return false; 
> > +
> > +    return doc->page()->chrome()->client()->shouldForceFullScreenVideoPlayback();
> > +}
> > +
> 
> This layering doesn't make sense. MediaPlayer is the bridge between the platform specific media engine and the rest of WebCore, so this method should only be here if the answer comes from the media engine. If the answer comes from the chrome client, HTMLMediaElement should call it directly - as it already does to enter and exit fullscreen.

Thanks for reviewing, Eric. I know the interface name is really confusing :(
This interface means once it returns true, all the html 5 video should be played in fullscreen mode only (no inline playing mode allowed). So, how about we change it to fullScreenVideoPlaybackOnly? Does it make sense?

[Yi Shen]

Created attachment 76770 [details]
updated with eric's suggestion

[Antonio Gomes]

Comment on attachment 76770 [details]
updated with eric's suggestion

View in context: https://bugs.webkit.org/attachment.cgi?id=76770&action=review

> WebCore/ChangeLog:13
> +        No new tests, as the changes should not change functionality.

It this reads better: "New new tests needed since there is not functionality change."

> WebCore/html/HTMLMediaElement.cpp:2112
> +            if (document() && document()->page() && document()->page()->chrome()->client()->requiresFullscreenForVideoPlayback() && !m_isFullscreen)

I think the right way to do it is adding a method to Chrome, and Chrome calls ChromeClient. Look at Chrome.cpp methods.

[Yi Shen]

Created attachment 76785 [details]
add a new function to chrome, and make it calls chromeclient::requiresFullscreenForVideoPlayback

[Jer Noble]

Comment on attachment 76785 [details]
add a new function to chrome, and make it calls chromeclient::requiresFullscreenForVideoPlayback

View in context: https://bugs.webkit.org/attachment.cgi?id=76785&action=review

Overall, the new patch is better than the first one, but moving enterFullscreenForNode() and exitFullscreenFroNode() from ChromeClient into Chrome is not strictly needed here.  If you believe they should be exposed through Chrome, that deserves a separate patch.

> WebCore/html/HTMLMediaElement.cpp:2417
> -        document()->page()->chrome()->client()->enterFullscreenForNode(this);
> +        document()->page()->chrome()->enterFullscreenForNode(this);

These changes don't seem necessary for your patch.

> WebCore/html/HTMLMediaElement.cpp:-2427
> -        document()->page()->chrome()->client()->exitFullscreenForNode(this);

Ditto.

> WebCore/page/Chrome.cpp:532
> +void Chrome::enterFullscreenForNode(Node* node)
> +{
> +    m_client->enterFullscreenForNode(node);
> +}
> +
> +void Chrome::exitFullscreenForNode(Node* node)
> +{
> +    m_client->exitFullscreenForNode(node);
> +}

Ditto.

> WebCore/page/Chrome.h:153
> +        void enterFullscreenForNode(Node*);
> +        void exitFullscreenForNode(Node*);

Ditto.

[Antonio Gomes]

(In reply to comment #12)
> (From update of attachment 76785 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=76785&action=review
> 
> Overall, the new patch is better than the first one, but moving enterFullscreenForNode() and exitFullscreenFroNode() from ChromeClient into Chrome is not strictly needed here.  If you believe they should be exposed through Chrome, that deserves a separate patch.

If you think they are not needed, sorry Yi, since I suggested them.

Could you please say why not though?

[Yi Shen]

(In reply to comment #13)
> (In reply to comment #12)
> > (From update of attachment 76785 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=76785&action=review
> > 
> > Overall, the new patch is better than the first one, but moving enterFullscreenForNode() and exitFullscreenFroNode() from ChromeClient into Chrome is not strictly needed here.  If you believe they should be exposed through Chrome, that deserves a separate patch.
> 
> If you think they are not needed, sorry Yi, since I suggested them.
> 
> Could you please say why not though?
That's all right, Antonio. I am always with you :) 
I think what Jer suggested is that we should make a separated patch for moving enterFullscreenForNode() and exitFullscreenFroNode() from ChromeClient into Chrome.

[Yi Shen]

Created attachment 76802 [details]
updated with Jer's suggestion

[Jer Noble]

(In reply to comment #15)
> Created an attachment (id=76802) [details]
> updated with Jer's suggestion

Much better, thanks. :)

[Antonio Gomes]

Comment on attachment 76802 [details]
updated with Jer's suggestion

looks good to me too. Eric?

[Eric Carlson]

Comment on attachment 76802 [details]
updated with Jer's suggestion

View in context: https://bugs.webkit.org/attachment.cgi?id=76802&action=review

r+, but clearing the cq flag because I would like the ChangeLog comment to be accurate. 

Thanks for the change!

> WebCore/ChangeLog:13
> +        No new tests needed since there is no functionality change.

This is not true, there is definitely new functionality and it will be possible to create new tests once a platform implements requiresFullscreenForVideoPlayback. Probably better to say something like "No new tests because no client implements requiresFullscreenForVideoPlayback yet".

[Yi Shen]

Created attachment 76812 [details]
fix changelog

[WebKit Commit Bot]

Comment on attachment 76812 [details]
fix changelog

Rejecting attachment 76812 [details] from commit-queue.

Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-3', 'apply-attachment', '--non-interactive', 76812]" exit_code: 2
Last 500 characters of output:
ailed to merge in the changes.
Patch failed at 0001 2010-12-16  Yury Semikhatsky  <[e-mail]>

When you have resolved this problem run "git rebase --continue".
If you would prefer to skip this patch, instead run "git rebase --skip".
To restore the original branch and stop rebasing run "git rebase --abort".

rebase refs/remotes/origin/master: command returned error: 1

Died at WebKitTools/Scripts/update-webkit line 132.

Failed to run "['WebKitTools/Scripts/update-webkit']" exit_code: 2

Full output: http://queues.webkit.org/results/7180068

[Eric Seidel (no email)]

Comment on attachment 76812 [details]
fix changelog

Bot's git repo got corrupted.

[WebKit Commit Bot]

Comment on attachment 76812 [details]
fix changelog

Clearing flags on attachment: 76812

Committed r74228: <http://trac.webkit.org/changeset/74228>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Yi Shen]

Created attachment 77842 [details]
a small fix for video to enter the fullscreen mode

[Kenneth Rohde Christiansen]

I think you need to reopen.

Simon Hausmann, can you have a look at the patch?

[Yi Shen]

According to the interface name, only video should be displayed in fullscreen when requiresFullscreenForVideoPlayback is specified.

[Yi Shen]

Created attachment 77843 [details]
fix typo

[Simon Hausmann]

Comment on attachment 77843 [details]
fix typo

That surely corrects the logic and brings it in line with the name requiresFullscreenFor_Video_Playback(), but I have the distinct feeling that in the future we're going to remove the _Video_ part from the function ;-)

[WebKit Commit Bot]

Comment on attachment 77843 [details]
fix typo

Clearing flags on attachment: 77843

Committed r74965: <http://trac.webkit.org/changeset/74965>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/74965 might have broken GTK Linux 32-bit Release
The following tests are not passing:
media/audio-delete-while-slider-thumb-clicked.html

[Ademar Reis]

Revision r74228 cherry-picked into qtwebkit-2.2 with commit 6f4a70e <http://gitorious.org/webkit/qtwebkit/commit/6f4a70e>
Revision r74965 cherry-picked into qtwebkit-2.2 with commit 26e1be3 <http://gitorious.org/webkit/qtwebkit/commit/26e1be3>