RESOLVED FIXED 58070
HTMLVideoElement::webkitEnterFullscreen does not use new Full Screen API when available.
https://bugs.webkit.org/show_bug.cgi?id=58070
Summary HTMLVideoElement::webkitEnterFullscreen does not use new Full Screen API when...
Jer Noble
Reported 2011-04-07 12:40:26 PDT
HTMLVideoElement::webkitEnterFullscreen does not use new Full Screen API when available.
Attachments
Patch (2.15 KB, patch)
2011-04-07 12:45 PDT, Jer Noble
no flags
Patch (2.18 KB, patch)
2011-04-07 13:12 PDT, Jer Noble
no flags
Patch (2.39 KB, patch)
2011-04-07 13:26 PDT, Jer Noble
eric.carlson: review+
Jer Noble
Comment 1 2011-04-07 12:41:09 PDT
Jer Noble
Comment 2 2011-04-07 12:45:20 PDT
WebKit Review Bot
Comment 3 2011-04-07 12:53:12 PDT
Jer Noble
Comment 4 2011-04-07 13:01:17 PDT
Comment on attachment 88674 [details] Patch Patch seems to be missing bits and pieces. Will re-upload.
Jer Noble
Comment 5 2011-04-07 13:12:26 PDT
Dimitri Glazkov (Google)
Comment 6 2011-04-07 13:14:14 PDT
Comment on attachment 88676 [details] Patch Why are these wired differently from the MediaControlFullscreenButton? Shouldn't we use the same codepaths?
Jer Noble
Comment 7 2011-04-07 13:18:28 PDT
(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)
Comment 8 2011-04-07 13:26:07 PDT
(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
Comment 9 2011-04-07 13:26:30 PDT
Jer Noble
Comment 11 2011-04-07 13:35:42 PDT
Indeed, that would be ideal.
Eric Carlson
Comment 12 2011-04-07 14:04:36 PDT
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
Comment 13 2011-04-07 14:12:26 PDT
Dimitri Glazkov (Google)
Comment 14 2011-04-07 14:44:43 PDT
(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
Comment 15 2011-04-07 15:01:06 PDT
(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
Comment 16 2011-04-07 15:38:51 PDT
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
Note You need to log in before you can comment on or make changes to this bug.