WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
Patch
(2.18 KB, patch)
2011-04-07 13:12 PDT
,
Jer Noble
no flags
Details
Formatted Diff
Diff
Patch
(2.39 KB, patch)
2011-04-07 13:26 PDT
,
Jer Noble
eric.carlson
: review+
Details
Formatted Diff
Diff
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
Jer Noble
Comment 1
2011-04-07 12:41:09 PDT
<
rdar://problem/9251239
>
Jer Noble
Comment 2
2011-04-07 12:45:20 PDT
Created
attachment 88674
[details]
Patch
WebKit Review Bot
Comment 3
2011-04-07 12:53:12 PDT
Attachment 88674
[details]
did not build on chromium: Build output:
http://queues.webkit.org/results/8346689
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
Created
attachment 88676
[details]
Patch
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
Created
attachment 88681
[details]
Patch
Dimitri Glazkov (Google)
Comment 10
2011-04-07 13:28:47 PDT
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
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
Committed
r83208
: <
http://trac.webkit.org/changeset/83208
>
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.
Top of Page
Format For Printing
XML
Clone This Bug