[webkit-reviews] review granted: [Bug 58638] Full Screen from within an <iframe> does not cause <iframe> to resize. : [Attachment 90306] Patch

[bugzilla-daemon at webkit.org]

Daniel Bates <dbates at webkit.org> has granted Jer Noble <jer.noble at apple.com>'s
request for review:
Bug 58638: Full Screen from within an <iframe> does not cause <iframe> to
resize.
https://bugs.webkit.org/show_bug.cgi?id=58638

Attachment 90306: Patch
https://bugs.webkit.org/attachment.cgi?id=90306&action=review

------- Additional Comments from Daniel Bates <dbates at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=90306&action=review

Thanks Jer for the additional test cases. This patch looks sane to me. I'm not
as familiar with the changes to WKFullScreenWindowController.mm/Core Animation
but they look reasonable. If you have any doubt about this change then feel
free to ask someone more familiar with this area of the code to review it.

r=me

> LayoutTests/fullscreen/full-screen-frameset-not-allowed.html:22
> +	       consoleWrite("SUCCEED - did not enter full screen!"); 

What you have here is sufficient as is. That being said, could we strengthen
this test case by checking that
"document.getElementById('frame').contentDocument.width != document.width" here
as opposed to just assuming we succeeded on timeout? I mean, outputting success
here would be a false positive if the webkitfullscreenchange event isn't
dispatched.

Note, if the webkitfullscreenchange event isn't dispatched then we would detect
such a regression using the test case
LayoutTests/fullscreen/full-screen-frameset-allowed.html (because it only
succeeds when the webkitfullscreenchange event is dispatched among other
criterion).

> LayoutTests/fullscreen/resources/not-allowed.html:7
> +<p>Test for <a href="http://bugs.webkit.org/show_bug.cgi?id=56264">bug
56264</a>: 
> +Handle entering full screen security restrictions</p>
> +<p>To test manually, click the "Go full screen" button - the page should not
enter full screen mode.</p>
> +<div id="console"></div>
> +<script>
> +    top.console = document.getElementById('console');
> +</script>

This is sufficient as is.

If we really wanted to, we could have one file, called
frame-test-description-and-console.html, that accepts a query string for the
text such that we don't have to have two files,
LayoutTests/fullscreen/resources/allowed.html and
LayoutTests/fullscreen/resources/not-allowed.html, which are almost identical.

> Source/WebCore/ChangeLog:21
> +	   (WebCore::Document::fullScreenIsAllowedForElement): Use the new
Element::isFrameElement

Nit: isFrameElement() => isFrameElementBase()

> Source/WebCore/ChangeLog:27
> +	   (WebCore::Element::isFrameElement): Added.

Ditto.

> Source/WebCore/ChangeLog:32
> +	   (WebCore::HTMLFrameElementBase::isFrameElement): Added.

Ditto.

> Source/WebCore/css/CSSStyleSelector.cpp:2907
>		   // that element. Also, an <iframe>, <object> or <embed>
element whose child browsing 

We should update this comment since this logic also applies to <frame>.