HTMLVideoElement::webkitEnterFullscreen does not use new Full Screen API when available.
<rdar://problem/9251239>
Created attachment 88674 [details] Patch
Attachment 88674 [details] did not build on chromium: Build output: http://queues.webkit.org/results/8346689
Comment on attachment 88674 [details] Patch Patch seems to be missing bits and pieces. Will re-upload.
Created attachment 88676 [details] Patch
Comment on attachment 88676 [details] Patch Why are these wired differently from the MediaControlFullscreenButton? Shouldn't we use the same codepaths?
(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!
(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.
Created attachment 88681 [details] Patch
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
Indeed, that would be ideal.
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.
Committed r83208: <http://trac.webkit.org/changeset/83208>
(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?
(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.
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