Bug 34942

Summary: Fullscreen API naming is inconsistent
Product: WebKit Reporter: Antoine Quint <ml>
Component: MediaAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: bdakin, cmarrin, eric.carlson, eric, simon.fraser
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.6   
Attachments:
Description Flags
Patch simon.fraser: review+

Antoine Quint
Reported 2010-02-15 07:26:51 PST
We currently have the following custom APIs for the <video> element in order to enter full-screen: readonly attribute boolean webkitSupportsFullscreen; readonly attribute boolean webkitDisplayingFullscreen; void webkitEnterFullScreen() raises (DOMException); void webkitExitFullScreen(); Note that the properties use a lower-case "s" for "screen" while the methods use an uppercase "S" for "Screen". I believe the "S" should be uppercase in all instances, but more importantly, the naming should be consistent.
Attachments
Patch (17.95 KB, patch)
2010-03-12 17:05 PST, Beth Dakin 		simon.fraser: review+
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Chris Marrin
Comment 1 2010-02-17 09:18:55 PST
This inconsistency extends to the rest of the code, so I'd like us to go through and make it consistent throughout when fixing this. When doing fullscreen for Windows, I was careful to use Fullscreen throughout. That seems to be the more common usage in the rest of the code (although not by much) and I think it makes sense if you consider "fullscreen" to be one word rather than two.
Simon Fraser (smfr)
Comment 2 2010-03-08 12:32:07 PST
<rdar://problem/7729165>
Beth Dakin
Comment 3 2010-03-12 17:05:17 PST
Created attachment 50643 [details] Patch I polled the audience and "fullscreen" won.
Simon Fraser (smfr)
Comment 4 2010-03-12 17:17:15 PST
Comment on attachment 50643 [details] Patch > Index: WebCore/html/HTMLVideoElement.idl > =================================================================== > --- WebCore/html/HTMLVideoElement.idl (revision 55928) > +++ WebCore/html/HTMLVideoElement.idl (working copy) > @@ -33,6 +33,10 @@ module html { > > readonly attribute boolean webkitSupportsFullscreen; > readonly attribute boolean webkitDisplayingFullscreen; > + > + [NeedsUserGestureCheck] void webkitEnterFullscreen() > + raises (DOMException); > + void webkitExitFullscreen(); > > [NeedsUserGestureCheck] void webkitEnterFullScreen() > raises (DOMException); I think you file a bugzilla to remove the old versions, and add a comment referencing it. > Index: WebKit/mac/ChangeLog > =================================================================== > --- WebKit/mac/ChangeLog (revision 55944) > +++ WebKit/mac/ChangeLog (working copy) > @@ -1,3 +1,31 @@ > +2010-03-12 Beth Dakin <bdakin@apple.com> > + > + Reviewed by NOBODY (OOPS!). > + > + Fix for https://bugs.webkit.org/show_bug.cgi?id=34942 Fullscreen > + API naming is inconsistent > + -and corresponding- > + <rdar://problem/7729165> > + > + This patch changes all occurrences of "fullScreen" to the more > + popular "fullscreen." > + > + * Plugins/Hosted/NetscapePluginHostProxy.h: > + (WebKit::NetscapePluginHostProxy::isFullscreenWindowShowing): > + * Plugins/Hosted/NetscapePluginHostProxy.mm: > + (WebKit::NetscapePluginHostProxy::NetscapePluginHostProxy): > + (WebKit::NetscapePluginHostProxy::didEnterFullscreen): > + (WebKit::NetscapePluginHostProxy::didExitFullscreen): > + (WebKit::NetscapePluginHostProxy::setFullscreenWindowIsShowing): > + (WKPCSetFullscreenWindowIsShowing): > + * Plugins/Hosted/WebKitPluginClient.defs: > + * Plugins/WebNetscapePluginView.mm: > + (-[WebNetscapePluginView _workaroundSilverlightFullscreenBug:]): > + (-[WebNetscapePluginView _createPlugin]): > + (-[WebNetscapePluginView _destroyPlugin]): > + * WebView/WebVideoFullscreenHUDWindowController.mm: > + (-[WebVideoFullscreenHUDWindowController windowDidLoad]): I think it's fine if you don't change the plugin-related code in this patch; up to you. r=me
Beth Dakin
Comment 5 2010-03-12 17:41:15 PST
Fixed with r55946.
Eric Seidel (no email)
Comment 6 2010-03-12 18:15:50 PST
This may have broken video tests on Snow Leopard: media/video-source-none-supported.html expected actual diff pretty diff plugins/keyboard-events.html expected actual diff pretty diff http/tests/media/video-cancel-load.html expected actual diff pretty diff http://build.webkit.org/results/SnowLeopard%20Intel%20Release%20(Tests)/r55947%20(6631)/results.html But the Snow Leopard Bot it so flakey it's difficult to tell.
Note You need to log in before you can comment on or make changes to this bug.