Bug 61461 – REGRESSION: Fullscreen button on embedded Vimeo videos does nothing

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Jer Noble]

REGRESSION: Fullscreen button on embedded Vimeo videos does nothing

[Jer Noble]

<rdar://problem/9495908>

[Jer Noble]

Created attachment 94845 [details]
Patch

[Darin Adler]

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.

[Jer Noble]

(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.

[Jer Noble]

Committed r87322: <http://trac.webkit.org/changeset/87322>

[Ademar Reis]

Revision r87322 cherry-picked into qtwebkit-2.2 with commit 10c0887 <http://gitorious.org/webkit/qtwebkit/commit/10c0887>