Bug 91448 – Unify allowfullscreen logic in Document::webkitFullScreenEnabled and fullScreenIsAllowedForElement.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Vincent Scheib]

Simplify Document::webkitFullScreenEnabled element traversal.

[Vincent Scheib]

Created attachment 152766 [details]
Patch

[Vincent Scheib]

Pointer lock will be using the same iframe allow logic. Reducing the code here first.

[Jer Noble]

This patch subtly changes the behavior of Document::requestFullScreenForElement().  Before, a JavaScript call "iframe.webkitEnterFullScreen()" on an iframe without an "webkitallowfullscreen" attribute would allow the iframe to enter full screen mode.  This makes sense, since it's the element owner that decides whether the full screen request will be accepted.

With this patch, element->allowFullScreen() is checked against the requested element if that element is an iframe.  This causes the above case (requesting full screen on an iframe directly) to fail.  I believe the Document::fullScreenIsAllowedForElement() function could still be simplified, but there would have to be one iteration of finding the element's owner element before the do/while loop.

[Vincent Scheib]

Created attachment 152817 [details]
Patch

[Vincent Scheib]

Thanks Jer, I've written a test to identify that particular case and adjusted the patch to handle it. Please take a look.

[Vincent Scheib]

Comment on attachment 152817 [details]
Patch

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

> Source/WebCore/html/HTMLFrameElementBase.cpp:253
> +    return (hasAttribute(webkitallowfullscreenAttr) || !isFrameElementBase())

Jer, I migrated this logic of !isFrameElementBase(), but we do not have a test that detects this logic being in place. Can you describe what scenario this logic is handling?

[Build Bot]

Comment on attachment 152817 [details]
Patch

Attachment 152817 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13282237

[Vincent Scheib]

Created attachment 152830 [details]
Patch

[Vincent Scheib]

Created attachment 152872 [details]
Patch

[Vincent Scheib]

(In reply to comment #6)
> (From update of attachment 152817 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=152817&action=review
> 
> > Source/WebCore/html/HTMLFrameElementBase.cpp:253
> > +    return (hasAttribute(webkitallowfullscreenAttr) || !isFrameElementBase())
> 
> Jer, I migrated this logic of !isFrameElementBase(), but we do not have a test that detects this logic being in place. Can you describe what scenario this logic is handling?

Doh, nevermind. As I moved the implementation to FrameElementBase I've removed that.

[Jer Noble]

Comment on attachment 152872 [details]
Patch

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

> Source/WebCore/dom/Document.cpp:5371
> +    HTMLFrameOwnerElement* owner = element->document()->ownerElement();
> +    if (!owner)
> +        return true;
> +    return owner->fullscreenIsAllowedByAllOwners();

This could be more easily written as:

if (HTMLFrameOwnerElement* owner = element->document()->ownerElement())
    return owner->fullscreenIsAllowedByAllOwners();
return true;

> Source/WebCore/dom/Document.cpp:5583
>      HTMLFrameOwnerElement* owner = ownerElement();
>      if (!owner)
>          return true;
> -
> -    do {
> -        if (!owner->isFrameElementBase())
> -            continue;
> -
> -        if (!static_cast<HTMLFrameElementBase*>(owner)->allowFullScreen())
> -            return false;
> -    } while ((owner = owner->document()->ownerElement()));
> -
> -    return true;        
> +    return owner->fullscreenIsAllowedByAllOwners();

This too.

> Source/WebCore/html/HTMLFrameElementBase.cpp:253
> +    HTMLFrameOwnerElement* owner = document()->ownerElement();
> +    return hasAttribute(webkitallowfullscreenAttr) && (!owner || owner->fullscreenIsAllowedByAllOwners());

Why recurse?  It'd be trivial to walk up the owner chain instead.  This function could even exist as a free function in Document.cpp.  There's really no need for it to be a class function on HTMLFrameElementBase.

> Source/WebCore/html/HTMLFrameOwnerElement.h:61
> +#if ENABLE(FULLSCREEN_API)
> +    virtual bool fullscreenIsAllowedByAllOwners() const { return false; }
> +#endif

This is probably a good idea (to provide a base implementation in HTMLFrameOwnerElement that is overridden in HTMLFrameElementBase), but there's no need to rename the function and provide a new implementation (see above about the free function possibility).

[Vincent Scheib]

Created attachment 153041 [details]
Patch

[Vincent Scheib]

Created attachment 153044 [details]
Patch

[Jer Noble]

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

Looking good!  Just a couple of nits.

> Source/WebCore/dom/Document.cpp:591
> +    // 4. The fullscreenEnabled attribute must return true if the context object and all ancestor
> +    // browsing context's documents have their fullscreen enabled flag set, or false otherwise.

Nit: please leave this comment in Document::webkitFullscreenEnabled().

> Source/WebCore/dom/Document.cpp:594
> +    // Top-level browsing contexts are implied to have their allowFullScreen attribute set.
> +    if (owner) {

Nit: please return (true) early in this case. Otherwise the comment doesn't make (as much) sense.  (Also, other reviewers tend to prefer early returns to indented conditionals like this one.)

Unofficial r=me.

[Jer Noble]

(In reply to comment #14)
> > Source/WebCore/dom/Document.cpp:591
> > +    // 4. The fullscreenEnabled attribute must return true if the context object and all ancestor
> > +    // browsing context's documents have their fullscreen enabled flag set, or false otherwise.
> 
> Nit: please leave this comment in Document::webkitFullscreenEnabled().

Looks like your latest patch does this.  Thanks!

[Early Warning System Bot]

Comment on attachment 153044 [details]
Patch

Attachment 153044 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13271648

[Vincent Scheib]

Created attachment 153049 [details]
Patch

[Vincent Scheib]

Thanks Jer, looks nice and clean now - easy to reuse for pointer lock.
enne, take a look?

[Vincent Scheib]

Created attachment 153050 [details]
Patch

[Adrienne Walker]

Comment on attachment 153050 [details]
Patch

R=me.

[WebKit Review Bot]

Comment on attachment 153050 [details]
Patch

Clearing flags on attachment: 153050

Committed r123005: <http://trac.webkit.org/changeset/123005>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.