Bug 61461 - REGRESSION: Fullscreen button on embedded Vimeo videos does nothing
Summary: REGRESSION: Fullscreen button on embedded Vimeo videos does nothing
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media Elements (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jer Noble
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2011-05-25 12:42 PDT by Jer Noble
Modified: 2011-06-03 14:06 PDT (History)
1 user (show)

See Also:


Attachments
Patch (8.64 KB, patch)
2011-05-25 13:06 PDT, Jer Noble
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jer Noble 2011-05-25 12:42:07 PDT
REGRESSION: Fullscreen button on embedded Vimeo videos does nothing
Comment 1 Jer Noble 2011-05-25 12:42:33 PDT
<rdar://problem/9495908>
Comment 2 Jer Noble 2011-05-25 13:06:10 PDT
Created attachment 94845 [details]
Patch
Comment 3 Darin Adler 2011-05-25 14:02:17 PDT
Comment on attachment 94845 [details]
Patch

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

I think the term “legacy full screen check” here is a little confusing; we might have been able to find a more specific name that would be clearer.

I am puzzled by all the webkit prefixes on the function names. Those should be present only for functions exported to the DOM. There’s no point in having those prefixes in our own internal function names.

> Source/WebCore/dom/Document.h:1062
> +    typedef enum {
> +        UseStrictFullScreenCheck,
> +        UseLegacyFullScreenCheck,
> +    } FullScreenCheckType;

The preferred C++ syntax is just:

    enum X { A, B, C };

The syntax you’re using here is C-ism.
Comment 4 Jer Noble 2011-05-25 14:08:27 PDT
(In reply to comment #3)
> (From update of attachment 94845 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=94845&action=review
> 
> I think the term “legacy full screen check” here is a little confusing; we might have been able to find a more specific name that would be clearer.

Hm.  How about "EnforceIFrameAllowFulScreenRequirement"?

> I am puzzled by all the webkit prefixes on the function names. Those should be present only for functions exported to the DOM. There’s no point in having those prefixes in our own internal function names.

The spec has changed a few times since I first added that function; webkitRequestFullScreenForElement used to be in the Document.idl, but has since been moved into Element.  But you're right, these should have their prefixes snipped.

> > Source/WebCore/dom/Document.h:1062
> > +    typedef enum {
> > +        UseStrictFullScreenCheck,
> > +        UseLegacyFullScreenCheck,
> > +    } FullScreenCheckType;
> 
> The preferred C++ syntax is just:
> 
>     enum X { A, B, C };
> 
> The syntax you’re using here is C-ism.

You're right.  Reflex.  Changed.
Comment 5 Jer Noble 2011-05-25 14:38:42 PDT
Committed r87322: <http://trac.webkit.org/changeset/87322>
Comment 6 Ademar Reis 2011-06-03 14:06:10 PDT
Revision r87322 cherry-picked into qtwebkit-2.2 with commit 10c0887 <http://gitorious.org/webkit/qtwebkit/commit/10c0887>