Bug 117001 - WebKit's Enter Full Screen for <video> elements isn't working
Summary: WebKit's Enter Full Screen for <video> elements isn't working
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2013-05-29 14:39 PDT by Ruth Fong
Modified: 2013-05-30 19:31 PDT (History)
9 users (show)

See Also:

Attachments
Patch (1.27 KB, patch)
2013-05-30 12:05 PDT, Ruth Fong 		no flags Details | Formatted Diff | Diff
Patch (1.61 KB, patch)
2013-05-30 16:45 PDT, Ruth Fong 		no flags Details | Formatted Diff | Diff
Patch (1.99 KB, patch)
2013-05-30 17:13 PDT, Ruth Fong 		no flags Details | Formatted Diff | Diff
Show Obsolete (2) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Ruth Fong 2013-05-29 14:39:26 PDT 
When hooking into WK's context menu action for entering into full screen for <video> elements, nothing happens (as opposed to hooking into WK's other media context menu actions, such as mute).
Comment 1 Ruth Fong 2013-05-29 14:41:58 PDT 
<rdar://problem/14006095>
Comment 2 Ruth Fong 2013-05-30 12:05:25 PDT 
Created attachment 203372 [details]
Patch
Comment 3 Darin Adler 2013-05-30 12:31:56 PDT 
Comment on attachment 203372 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        No new tests. Covered by existing tests for fullscreen.

If this is covered by existing tests, why aren’t those tests failing?
Comment 4 Jer Noble 2013-05-30 12:59:58 PDT 
Comment on attachment 203372 [details]
Patch

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

> Source/WebCore/dom/Document.cpp:4980
> +    UserGestureIndicator indicator(DefinitelyProcessingNewUserGesture);
> +    

By inserting this UserGestureIndicator, this patch would disable the security requirement that full screen requests must begin with an event handler.  The appropriate place to put this line is in your context menu handler, immediately before calling element->webkitRequestFullScreen(), not inside requestFullScreenForElement() itself.
Comment 5 Ruth Fong 2013-05-30 16:45:27 PDT 
Created attachment 203396 [details]
Patch
Comment 6 Jer Noble 2013-05-30 16:55:11 PDT 
Comment on attachment 203396 [details]
Patch

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

> Source/WebCore/ChangeLog:11
> +        No new tests.
> +
> +        * rendering/HitTestResult.cpp:
> +        (WebCore::HitTestResult::enterFullscreenForVideo):

Please describe the problem which this patch is trying to solve, and why the approach here fixes the problem.

You should also explain why no new tests were added: i.e, there is an existing test, media/context-menu-actions.html, but it is disabled due to bug #116651.
Comment 7 Ruth Fong 2013-05-30 17:13:07 PDT 
Created attachment 203400 [details]
Patch
Comment 8 WebKit Commit Bot 2013-05-30 19:31:52 PDT 
Comment on attachment 203400 [details]
Patch

Clearing flags on attachment: 203400

Committed r151003: <http://trac.webkit.org/changeset/151003>
Comment 9 WebKit Commit Bot 2013-05-30 19:31:55 PDT 
All reviewed patches have been landed.  Closing bug.