Bug 67888 - [Chromium] Add startFullscreen to WebMediaPlayer for Android
Summary: [Chromium] Add startFullscreen to WebMediaPlayer for Android
Status: RESOLVED LATER
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Adam Barth
URL:
Keywords:
Depends on:
Blocks: 66687
  Show dependency treegraph
 
Reported: 2011-09-10 01:43 PDT by Adam Barth
Modified: 2012-02-08 16:10 PST (History)
6 users (show)

See Also:


Attachments
Patch (18.66 KB, patch)
2011-09-10 01:47 PDT, Adam Barth
abarth: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Barth 2011-09-10 01:43:43 PDT
[Chromium] Add startFullscreen to WebMediaPlayer for Android
Comment 1 Adam Barth 2011-09-10 01:47:46 PDT
Created attachment 106972 [details]
Patch
Comment 2 Eric Seidel (no email) 2011-09-19 15:46:40 PDT
Jer is a good person to get to comment on this since he designed the fullscreen stuff.
Comment 3 Jer Noble 2011-09-19 16:05:39 PDT
Comment on attachment 106972 [details]
Patch

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

> Source/WebCore/ChangeLog:12
> +        When an HTMLMediaElement on Android goes fullscreen, it needs the
> +        WebMediaPlayer to help make that happen.  It's slighly unclear to me
> +        why this work can't be done by the ChromeClient (essentially
> +        special-casing HTMLMediaElement there), but this approach also seems
> +        reasonable.

Doing it at the ChromeClient level would make more sense, given that the client is probably in the best position to know whether or not the full screen request will be honored.  As it stands, the full screen request could be rejected and the startFullScreen() call would be all for naught.

> Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp:241
> -    if (m_webMediaPlayer.get())
> +    if (m_webMediaPlayer)

There are a ton of these; could they be factored out into a separate patch?

> Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp:279
> +#if OS(ANDROID)
> +void WebMediaPlayerClientImpl::startFullscreen()
> +{
> +    if (m_webMediaPlayer)
> +        m_webMediaPlayer->startFullscreen();
> +}
> +#endif

What does this do?
Comment 4 Jer Noble 2011-09-19 16:08:44 PDT
...additionally, the new Full Screen API has settled on "Full Screen" (in text), "full-screen" (in css) and "fullScreen" (in code), instead of "fullscreen".  We should try to be consistent.
Comment 5 Adam Barth 2011-09-19 16:09:32 PDT
Comment on attachment 106972 [details]
Patch

ok