HTMLVideoElement::webkitEnterFullscreen does not use new Full Screen API when available.

• Component: Media
• Status: RESOLVED FIXED

[Jer Noble]

HTMLVideoElement::webkitEnterFullscreen does not use new Full Screen API when available.

[Jer Noble]

<rdar://problem/9251239>

[Jer Noble]

Created attachment 88674 [details]
Patch

[WebKit Review Bot]

Attachment 88674 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8346689

[Jer Noble]

Comment on attachment 88674 [details]
Patch

Patch seems to be missing bits and pieces.  Will re-upload.

[Jer Noble]

Created attachment 88676 [details]
Patch

[Dimitri Glazkov (Google)]

Comment on attachment 88676 [details]
Patch

Why are these wired differently from the MediaControlFullscreenButton? Shouldn't we use the same codepaths?

[Jer Noble]

(In reply to comment #6)
> (From update of attachment 88676 [details])
> Why are these wired differently from the MediaControlFullscreenButton? Shouldn't we use the same code paths?

Good point.  I should check to make sure the settings are enabled first.  Thanks!

[Dimitri Glazkov (Google)]

(In reply to comment #7)
> (In reply to comment #6)
> > (From update of attachment 88676 [details] [details])
> > Why are these wired differently from the MediaControlFullscreenButton? Shouldn't we use the same code paths?
> 
> Good point.  I should check to make sure the settings are enabled first.  Thanks!

No worries -- I was just looking at this code :). Sounds like ideally, the code from here: http://codesearch.google.com/codesearch/p#OAMlx_jo-ck/src/third_party/WebKit/Source/WebCore/rendering/MediaControlElements.cpp&l=950&exact_package=chromium

should just migrate to enterFullScreen.

[Jer Noble]

Created attachment 88681 [details]
Patch

[Dimitri Glazkov (Google)]

Comment on attachment 88681 [details]
Patch

Can we also eliminate the same code here: http://codesearch.google.com/codesearch/p#OAMlx_jo-ck/src/third_party/WebKit/Source/WebCore/rendering/MediaControlElements.cpp&l=950&exact_package=chromium

[Jer Noble]

Indeed, that would be ideal.

[Eric Carlson]

Comment on attachment 88681 [details]
Patch

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

> Source/WebCore/html/HTMLMediaElement.cpp:2464
> +#if ENABLE(FULLSCREEN_API)
> +    if (document()->settings() && document()->settings()->fullScreenEnabled()) {
> +        webkitRequestFullScreen(0);
> +        return;
> +    }
> +#else
>      ASSERT(!m_isFullscreen);
>      m_isFullscreen = true;
>      if (document() && document()->page()) {
> +
>          document()->page()->chrome()->client()->enterFullscreenForNode(this);
>          scheduleEvent(eventNames().webkitbeginfullscreenEvent);
>      }
> +#endif

There isn't any need for the #else ... #endif, we want to continue to support the old API even when ENABLE_FULLSCREEN_API is defined.

> Source/WebCore/html/HTMLMediaElement.cpp:2475
> +#if ENABLE(FULLSCREEN_API)
> +    if (document()->settings() && document()->settings()->fullScreenEnabled()) {
> +        document()->webkitCancelFullScreen();
> +        return;
> +    }
> +#else

Same here.

[Jer Noble]

Committed r83208: <http://trac.webkit.org/changeset/83208>

[Dimitri Glazkov (Google)]

(In reply to comment #13)
> Committed r83208: <http://trac.webkit.org/changeset/83208>

Whoops. Don't you do this twice now, since MediaControlFullscreenButtonElement::defaultEventHandler calls enterFullScreen?

[Jer Noble]

(In reply to comment #14)
> (In reply to comment #13)
> > Committed r83208: <http://trac.webkit.org/changeset/83208>
> 
> Whoops. Don't you do this twice now, since MediaControlFullscreenButtonElement::defaultEventHandler calls enterFullScreen?

I filed bug #58087 to get rid of the duplicated code.

[WebKit Review Bot]

http://trac.webkit.org/changeset/83208 might have broken SnowLeopard Intel Release (Tests), GTK Linux 32-bit Release, and GTK Linux 64-bit Debug
The following tests are not passing:
media/context-menu-actions.html
media/media-fullscreen-inline.html
media/media-fullscreen-not-in-document.html