[webkit-reviews] review granted: [Bug 43099] Add JavaScript API to allow a page to go fullscreen : [Attachment 64997] Part 7: FullScreen-LayoutTests

Thu Aug 26 11:59:36 PDT 2010

Eric Carlson <eric.carlson at apple.com> has granted Jer Noble
<jer.noble at apple.com>'s request for review:
Bug 43099: Add JavaScript API to allow a page to go fullscreen
https://bugs.webkit.org/show_bug.cgi?id=43099

Attachment 64997: Part 7: FullScreen-LayoutTests
https://bugs.webkit.org/attachment.cgi?id=64997&action=review

------- Additional Comments from Eric Carlson <eric.carlson at apple.com>

> +2010-08-19  Jer Noble  <jer.noble at apple.com>
> +
> +	   Reviewed by NOBODY (OOPS!).
> +

...

> +	   * fullscreen/full-screen-twice.html: Added.
> +
> +2010-08-09  Jer Noble  <jer.noble at apple.com>
> +
These two entries should be combined.

> +	   Add JavaScript API to allow a page to go fullscreen.
> +	   rdar://problem/6867795
> +	   https://bugs.webkit.org/show_bug.cgi?id=43099
> +
> +	   New tests for the new full screen API:
> +
> +	   full-screen-api.html: tests for the presence of the new full screen
apis.
> +	   full-screen-request.html: tests that clients can request the browser
enter full screen mode successfully.
> +	   full-screen-css.html: tests that the new full screen pseudo classes
are applied once full screen mode is entered.
> +	   full-screen-remove.html: tests that the document's current full
screen element is changed when that element is removed from the DOM tree.
> +	   full-screen-remove-ancestor.html: tests that the document's current
full screen element is changed an ancestor of that element is removed from the
DOM tree.
> +	   full-screen-twice.html: tests that entering full screen mode on two
different elements in a row does not fail.

Please wrap the long lines so they are readable without horizontal scrolling in
a reasonable size window.

> ===================================================================
> --- LayoutTests/fullscreen/full-screen-api.html	(revision 0)
> +++ LayoutTests/fullscreen/full-screen-api.html	(revision 0)
> @@ -0,0 +1,13 @@
> +<body>
> +    <script src="full-screen-test.js"></script>
> +<span></span>
> +<script>
> +    span = document.getElementsByTagName('span')[0];
> +	testExpected("document.webkitFullScreen", false);
> +	testExpected("document.webkitCancelFullScreen", undefined, "!=");
> +	testExpected("document.webkitCurrentFullScreenElement", null);
> +	testExpected("document.onwebkitfullscreenchange", null)
> +	testExpected("span.webkitRequestFullScreen", undefined, "!=");
> +	testExpected("span.onwebkitfullscreenchange", null)	
> +	endTest();
> +</script>

Ack, tabs!

> Index: LayoutTests/fullscreen/full-screen-css.html
> +<script>
> +    // Bail out early if the full screen API is not enabled or is missing:
> +    if (Element.prototype.webkitRequestFullScreen == undefined)
> +	   endTest();

Shouldn't this be an explicit fail?

> Index: LayoutTests/fullscreen/full-screen-remove-ancestor.html
> +<script>
> +    // Bail out early if the full screen API is not enabled or is missing:
> +    if (Element.prototype.webkitRequestFullScreen == undefined) 
> +	   endTest();

Here too.

> Index: LayoutTests/fullscreen/full-screen-remove.html
> +<script>
> +    // Bail out early if the full screen API is not enabled or is missing:
> +    if (Element.prototype.webkitRequestFullScreen == undefined)
> +	   endTest();

And here.

> Index: LayoutTests/fullscreen/full-screen-request.html
> +<script>
> +    // Bail out early if the full screen API is not enabled or is missing:
> +    if (Element.prototype.webkitRequestFullScreen == undefined)
> +	   endTest();

And here.

> +	   case '<':   success = observed <  expected; break;
> +	   case '<=': success = observed <= expected; break;
> +	   case '>':   success = observed >  expected; break;
> +	   case '>=': success = observed >= expected; break;
> +	   case '!=':  success = observed != expected; break;
> +	   case '==': success = observed == expected; break;

Strange spacing here...

> +function waitForEventAndEnd(eventName, funcString)
> +function waitForEventAndFail(element, eventName)
> +function waitForEventAndTest(element, eventName, testFuncString, endit)

These aren't used, you should remove them.

> Index: LayoutTests/fullscreen/full-screen-twice.html
> +<script>
> +    // Bail out early if the full screen API is not enabled or is missing:
> +    if (Element.prototype.webkitRequestFullScreen == undefined) 
> +	   endTest();

Fail here.

> Index: LayoutTests/platform/android-v8/Skipped
> Index: LayoutTests/platform/android/Skipped
> Index: LayoutTests/platform/chromium-linux/Skipped
> Index: LayoutTests/platform/chromium-linux/Skipped
> Index: LayoutTests/platform/chromium-mac/Skipped
> Index: LayoutTests/platform/chromium-win-vista/Skipped
> Index: LayoutTests/platform/chromium-win-xp/Skipped
> Index: LayoutTests/platform/chromium-win/Skipped
> Index: LayoutTests/platform/chromium/Skipped
> Index: LayoutTests/platform/google-chrome-linux64/Skipped
> Index: LayoutTests/platform/gtk/Skipped
> Index: LayoutTests/platform/qt-linux/Skipped
> Index: LayoutTests/platform/qt-mac/Skipped
> Index: LayoutTests/platform/qt-win/Skipped
> Index: LayoutTests/platform/qt/Skipped
> Index: LayoutTests/platform/win-wk2/Skipped
> Index: LayoutTests/platform/win/Skipped

You should only need to add the test the top level Skipped file for each port
(eg.
LayoutTests/platform/chromium/Skipped skips all chromium). You should add a
comment 
to each Skipped file about why the tests are disabled.

r=me with these changes.