Bug 67888 – [Chromium] Add startFullscreen to WebMediaPlayer for Android

• Status: RESOLVED
• Resolution: LATER
• Component: New Bugs

[Adam Barth]

[Chromium] Add startFullscreen to WebMediaPlayer for Android

[Adam Barth]

Created attachment 106972 [details]
Patch

[Eric Seidel (no email)]

Jer is a good person to get to comment on this since he designed the fullscreen stuff.

[Jer Noble]

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?

[Jer Noble]

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

[Adam Barth]

Comment on attachment 106972 [details]
Patch

ok