Bug 101100 - [BlackBerry] Automatically go fullscreen on video play
Summary: [BlackBerry] Automatically go fullscreen on video play
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit BlackBerry (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Max Feil
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-11-02 14:12 PDT by Max Feil
Modified: 2012-11-06 07:28 PST (History)
8 users (show)

See Also:


Attachments
Patch (11.91 KB, patch)
2012-11-02 14:56 PDT, Max Feil
no flags Details | Formatted Diff | Diff
Patch (11.91 KB, patch)
2012-11-05 13:13 PST, Max Feil
rwlbuis: review-
Details | Formatted Diff | Diff
Patch (11.91 KB, patch)
2012-11-05 16:55 PST, Max Feil
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Max Feil 2012-11-02 14:12:31 PDT
There is a requirement to have HTML5 video automatically enter fullscreen when the video starts playing. This patch implements this feature, with restrictions. The main restriction is adherence to WebKit's philosophy of only entering fullscreen due to a user gesture. This is important in order to avoid pop-up advertisements and other unwanted fullscreen content. One consequence of this is that video elements with the autoplay attribute will not automatically enter fullscreen.

Other caveats:
- This feature applies only to "small screen" devices where automatically going fullscreen makes more sense.
- Fullscreen will only be entered automatically when the video is played from the beginning (current time is zero). It is assumed that if the user is resuming play from a paused state and is not in fullscreen mode, then they exited fullscreen mode intentionally.
Comment 1 Max Feil 2012-11-02 14:56:54 PDT
Created attachment 172148 [details]
Patch
Comment 2 Antonio Gomes 2012-11-02 16:45:05 PDT
Comment on attachment 172148 [details]
Patch

It looks good to me. I will let Eric to have a change to look at the cross platform additions
Comment 3 Eric Carlson 2012-11-02 17:26:31 PDT
Comment on attachment 172148 [details]
Patch

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

Marking r+, but I would prefer the name change.

> Source/WebCore/html/HTMLMediaElement.cpp:4544
> +bool HTMLMediaElement::mediaPlayerIsFullscreenRestricted() const
> +{
> +    return userGestureRequiredForFullscreen() && !ScriptController::processingUserGesture();
> +}

I think it would be better to flip the meaning of the name so you ask if fullscreen is allowed instead of restricted. Maybe "mediaPlayerIsFullscreenAllowed", or "mediaPlayerIsFullscreenPermitted"?
Comment 4 Max Feil 2012-11-05 13:13:47 PST
Created attachment 172385 [details]
Patch
Comment 5 Max Feil 2012-11-05 13:17:41 PST
Comment on attachment 172385 [details]
Patch

Marking reviewed based on r+ given to previous patch. The only change in this patch is a simple one to rename boolean function mediaPlayerIsFullscreenRestricted() to mediaPlayerIsFullscreenPermitted() and change its sense.
Comment 6 Rob Buis 2012-11-05 14:52:19 PST
Comment on attachment 172385 [details]
Patch

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

> Source/WebCore/ChangeLog:38
> +        (WebCore::MediaPlayerClient::mediaPlayerIsFullscreenRestricted):

Hate to be pedantic, but this does not match the renaming.
Comment 7 Max Feil 2012-11-05 16:55:27 PST
Created attachment 172442 [details]
Patch

Good eye, Rob. I forgot about the change log.
Comment 8 WebKit Review Bot 2012-11-06 07:28:22 PST
Comment on attachment 172442 [details]
Patch

Clearing flags on attachment: 172442

Committed r133606: <http://trac.webkit.org/changeset/133606>
Comment 9 WebKit Review Bot 2012-11-06 07:28:26 PST
All reviewed patches have been landed.  Closing bug.