Bug 51133 - Provide an interface to force using fullscreen mediaplayer
Summary: Provide an interface to force using fullscreen mediaplayer
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media Elements (show other bugs)
Version: 528+ (Nightly build)
Hardware: All OS X 10.5
: P2 Enhancement
Assignee: Yi Shen
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-12-15 14:04 PST by Yi Shen
Modified: 2011-01-04 07:08 PST (History)
13 users (show)

See Also:


Attachments
first try (5.21 KB, patch)
2010-12-15 14:14 PST, Yi Shen
no flags Details | Formatted Diff | Diff
minor changes (5.22 KB, patch)
2010-12-16 06:12 PST, Yi Shen
eric.carlson: review-
eric.carlson: commit-queue-
Details | Formatted Diff | Diff
updated with eric's suggestion (3.00 KB, patch)
2010-12-16 08:17 PST, Yi Shen
no flags Details | Formatted Diff | Diff
add a new function to chrome, and make it calls chromeclient::requiresFullscreenForVideoPlayback (4.89 KB, patch)
2010-12-16 10:50 PST, Yi Shen
no flags Details | Formatted Diff | Diff
updated with Jer's suggestion (3.80 KB, patch)
2010-12-16 12:43 PST, Yi Shen
eric.carlson: review+
eric.carlson: commit-queue-
Details | Formatted Diff | Diff
fix changelog (deleted)
2010-12-16 13:50 PST, Yi Shen
no flags Details | Formatted Diff | Diff
a small fix for video to enter the fullscreen mode (1.53 KB, patch)
2011-01-03 13:40 PST, Yi Shen
no flags Details | Formatted Diff | Diff
fix typo (1.53 KB, patch)
2011-01-03 13:45 PST, Yi Shen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yi Shen 2010-12-15 14:04:38 PST
Based on the worked done by Tor Arne Vestbo (tor.arne.vestbo@nokia.com), 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.
Comment 1 Yi Shen 2010-12-15 14:14:34 PST
Created attachment 76686 [details]
first try
Comment 2 Antonio Gomes 2010-12-15 16:43:29 PST
> 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?
Comment 3 Jer Noble 2010-12-15 20:51:21 PST
Why are the changes to MediaPlayer necessary?  Why not call the chrome() client methods from HTMLMediaElement?
Comment 4 Yi Shen 2010-12-16 06:12:33 PST
Created attachment 76764 [details]
minor changes
Comment 5 Yi Shen 2010-12-16 06:34:46 PST
(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.
Comment 6 Yi Shen 2010-12-16 06:36:04 PST
(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.
Comment 7 Eric Carlson 2010-12-16 07:18:59 PST
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.
Comment 8 Yi Shen 2010-12-16 07:37:22 PST
(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?
Comment 9 Yi Shen 2010-12-16 08:17:46 PST
Created attachment 76770 [details]
updated with eric's suggestion
Comment 10 Antonio Gomes 2010-12-16 08:36:19 PST
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.
Comment 11 Yi Shen 2010-12-16 10:50:38 PST
Created attachment 76785 [details]
add a new function to chrome, and make it calls chromeclient::requiresFullscreenForVideoPlayback
Comment 12 Jer Noble 2010-12-16 11:23:02 PST
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.
Comment 13 Antonio Gomes 2010-12-16 11:32:43 PST
(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?
Comment 14 Yi Shen 2010-12-16 11:37:28 PST
(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.
Comment 15 Yi Shen 2010-12-16 12:43:04 PST
Created attachment 76802 [details]
updated with Jer's suggestion
Comment 16 Jer Noble 2010-12-16 13:24:33 PST
(In reply to comment #15)
> Created an attachment (id=76802) [details]
> updated with Jer's suggestion

Much better, thanks. :)
Comment 17 Antonio Gomes 2010-12-16 13:27:23 PST
Comment on attachment 76802 [details]
updated with Jer's suggestion

looks good to me too. Eric?
Comment 18 Eric Carlson 2010-12-16 13:37:12 PST
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".
Comment 19 Yi Shen 2010-12-16 13:50:16 PST
Created attachment 76812 [details]
fix changelog
Comment 20 WebKit Commit Bot 2010-12-16 13:59:18 PST
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  <yurys@chromium.org>

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
Comment 21 Eric Seidel 2010-12-16 14:22:23 PST
Comment on attachment 76812 [details]
fix changelog

Bot's git repo got corrupted.
Comment 22 WebKit Commit Bot 2010-12-16 19:29:05 PST
Comment on attachment 76812 [details]
fix changelog

Clearing flags on attachment: 76812

Committed r74228: <http://trac.webkit.org/changeset/74228>
Comment 23 WebKit Commit Bot 2010-12-16 19:29:12 PST
All reviewed patches have been landed.  Closing bug.
Comment 24 Yi Shen 2011-01-03 13:40:35 PST
Created attachment 77842 [details]
a small fix for video to enter the fullscreen mode
Comment 25 Kenneth Rohde Christiansen 2011-01-03 13:43:07 PST
I think you need to reopen.

Simon Hausmann, can you have a look at the patch?
Comment 26 Yi Shen 2011-01-03 13:44:31 PST
According to the interface name, only video should be displayed in fullscreen when requiresFullscreenForVideoPlayback is specified.
Comment 27 Yi Shen 2011-01-03 13:45:13 PST
Created attachment 77843 [details]
fix typo
Comment 28 Simon Hausmann 2011-01-04 04:48:02 PST
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 ;-)
Comment 29 WebKit Commit Bot 2011-01-04 05:17:59 PST
Comment on attachment 77843 [details]
fix typo

Clearing flags on attachment: 77843

Committed r74965: <http://trac.webkit.org/changeset/74965>
Comment 30 WebKit Commit Bot 2011-01-04 05:18:07 PST
All reviewed patches have been landed.  Closing bug.
Comment 31 WebKit Review Bot 2011-01-04 06:08:09 PST
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
Comment 32 Ademar Reis 2011-01-04 07:08:13 PST
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>