Bug 34942 - Fullscreen API naming is inconsistent
: Fullscreen API naming is inconsistent
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: Media Elements
: 528+ (Nightly build)
: PC OS X 10.6
: P2 Normal
Assigned To: Nobody
: InRadar
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-02-15 07:26 PST by Antoine Quint
Modified: 2010-03-12 18:15 PST (History)
5 users (show)

See Also:


Attachments
Patch (17.95 KB, patch)
2010-03-12 17:05 PST, Beth Dakin
simon.fraser: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Antoine Quint 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.
Comment 1 Chris Marrin 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.
Comment 2 Simon Fraser (smfr) 2010-03-08 12:32:07 PST
<rdar://problem/7729165>
Comment 3 Beth Dakin 2010-03-12 17:05:17 PST
Created attachment 50643 [details]
Patch

I polled the audience and "fullscreen" won.
Comment 4 Simon Fraser (smfr) 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
Comment 5 Beth Dakin 2010-03-12 17:41:15 PST
Fixed with r55946.
Comment 6 Eric Seidel 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.