Convert more fullscreen tests to reftests

Convert more fullscreen tests to reftests

This removes ~242 KB of *-expected.png and leaves only a few
platform-specific expectations left in
LayoutTests/platform/linux/virtual/android/fullscreen/

Some tests were simultaneously pixel tests and text tests, which is
not great for a reftest. To avoid having the (formated) log output in
*-expected.html, instead make those tests time out if any check fails.

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=180036

[philipj_slow, 2014-08-08 12:13:22]

PTAL. Some tests are maybe a little bit worse off because of this, let me know
if there's any manual button or description you want back.

[philipj_slow, 2014-08-08 12:54:49]

doctype matters for margins

[falken, 2014-08-11 02:34:05]

Thanks for doing this.

I rather like the descriptions and manual buttons. They help me debug. Does
keeping them complicate the expected result too much?

https://codereview.chromium.org/457663002/diff/20001/LayoutTests/fullscreen/f...
File LayoutTests/fullscreen/full-screen-iframe-zIndex-expected.html (right):

https://codereview.chromium.org/457663002/diff/20001/LayoutTests/fullscreen/f...
LayoutTests/fullscreen/full-screen-iframe-zIndex-expected.html:2: <button>Go
full screen</button>
Out of scope for this patch, but...

this test is pretty confusing. It says only a "white box" should be visible but
it shows this button which made me think at first the test doesn't even exercise
fullscreen. I see now though the iframe that goes fullscreen has this button
which isn't used in the test.

We should also add a z-index: max_int test which shows crbug.com/398599

https://codereview.chromium.org/457663002/diff/20001/LayoutTests/fullscreen/f...
File LayoutTests/fullscreen/full-screen-placeholder-expected.html (right):

https://codereview.chromium.org/457663002/diff/20001/LayoutTests/fullscreen/f...
LayoutTests/fullscreen/full-screen-placeholder-expected.html:11: clear:left;
nit: insert space after :

https://codereview.chromium.org/457663002/diff/20001/LayoutTests/fullscreen/f...
File LayoutTests/fullscreen/full-screen-placeholder.html (left):

https://codereview.chromium.org/457663002/diff/20001/LayoutTests/fullscreen/f...
LayoutTests/fullscreen/full-screen-placeholder.html:22: <div>This layout test
checks that the offset positions of the blue and green divs does not change when
the red div enters full-screen mode.  Press <button
onclick="document.getElementById('float').webkitRequestFullScreen()">go
full-screen</a> to begin.</div>
I don't feel too strongly about it but I find these manual buttons useful when
debugging. Does keeping it complicate the expected result?

https://codereview.chromium.org/457663002/diff/20001/LayoutTests/fullscreen/f...
File LayoutTests/fullscreen/full-screen-stacking-context-expected.html (right):

https://codereview.chromium.org/457663002/diff/20001/LayoutTests/fullscreen/f...
LayoutTests/fullscreen/full-screen-stacking-context-expected.html:4: var
runPixelTests = true;
I'm not positive, but it might be possible to remove these runPixelTests
variables now that everything is a reftest.

https://codereview.chromium.org/457663002/diff/20001/LayoutTests/fullscreen/f...
File LayoutTests/fullscreen/full-screen-zIndex-after.html (right):

https://codereview.chromium.org/457663002/diff/20001/LayoutTests/fullscreen/f...
LayoutTests/fullscreen/full-screen-zIndex-after.html:10: logResult(false,
"Element.prototype.webkitRequestFullScreen == undefined");
Let's change these double-quotes to single-quotes to make the file
self-consistent. I prefer single-quotes for JavaScript strings.

[philipj_slow, 2014-08-11 11:22:09]

https://codereview.chromium.org/457663002/diff/20001/LayoutTests/fullscreen/f...
File LayoutTests/fullscreen/full-screen-iframe-zIndex-expected.html (right):

https://codereview.chromium.org/457663002/diff/20001/LayoutTests/fullscreen/f...
LayoutTests/fullscreen/full-screen-iframe-zIndex-expected.html:2: <button>Go
full screen</button>
On 2014/08/11 02:34:04, falken wrote:
> Out of scope for this patch, but...
> 
> this test is pretty confusing. It says only a "white box" should be visible
but
> it shows this button which made me think at first the test doesn't even
exercise
> fullscreen. I see now though the iframe that goes fullscreen has this button
> which isn't used in the test.

I've updated the test to use empty.html instead and tweaked the description.

> We should also add a z-index: max_int test which shows crbug.com/398599

Is the attached zindex-defeats-fullscreen.html still good? Wanna create a
review?

https://codereview.chromium.org/457663002/diff/20001/LayoutTests/fullscreen/f...
File LayoutTests/fullscreen/full-screen-placeholder-expected.html (right):

https://codereview.chromium.org/457663002/diff/20001/LayoutTests/fullscreen/f...
LayoutTests/fullscreen/full-screen-placeholder-expected.html:11: clear:left;
On 2014/08/11 02:34:04, falken wrote:
> nit: insert space after :

Done.

https://codereview.chromium.org/457663002/diff/20001/LayoutTests/fullscreen/f...
File LayoutTests/fullscreen/full-screen-placeholder.html (left):

https://codereview.chromium.org/457663002/diff/20001/LayoutTests/fullscreen/f...
LayoutTests/fullscreen/full-screen-placeholder.html:22: <div>This layout test
checks that the offset positions of the blue and green divs does not change when
the red div enters full-screen mode.  Press <button
onclick="document.getElementById('float').webkitRequestFullScreen()">go
full-screen</a> to begin.</div>
On 2014/08/11 02:34:04, falken wrote:
> I don't feel too strongly about it but I find these manual buttons useful when
> debugging. Does keeping it complicate the expected result?

Having dummy buttons in the expected results felt a bit odd, but I've added them
back. I also introduced a few goFullScreen functions to make sure that the
manual button and the automated test actually do the same thing.

https://codereview.chromium.org/457663002/diff/20001/LayoutTests/fullscreen/f...
File LayoutTests/fullscreen/full-screen-stacking-context-expected.html (right):

https://codereview.chromium.org/457663002/diff/20001/LayoutTests/fullscreen/f...
LayoutTests/fullscreen/full-screen-stacking-context-expected.html:4: var
runPixelTests = true;
On 2014/08/11 02:34:05, falken wrote:
> I'm not positive, but it might be possible to remove these runPixelTests
> variables now that everything is a reftest.

Unfortunately it isn't, calling testRunner.dumpAsText() makes the test fail,
expecting "pass" but getting "image". Splitting runWithKeyDown into a separate
script would help, though, if you think it's worth it.

https://codereview.chromium.org/457663002/diff/20001/LayoutTests/fullscreen/f...
File LayoutTests/fullscreen/full-screen-zIndex-after.html (right):

https://codereview.chromium.org/457663002/diff/20001/LayoutTests/fullscreen/f...
LayoutTests/fullscreen/full-screen-zIndex-after.html:10: logResult(false,
"Element.prototype.webkitRequestFullScreen == undefined");
On 2014/08/11 02:34:05, falken wrote:
> Let's change these double-quotes to single-quotes to make the file
> self-consistent. I prefer single-quotes for JavaScript strings.

Done.

[falken, 2014-08-12 00:18:00]

LGTM

https://codereview.chromium.org/457663002/diff/20001/LayoutTests/fullscreen/f...
File LayoutTests/fullscreen/full-screen-iframe-zIndex-expected.html (right):

https://codereview.chromium.org/457663002/diff/20001/LayoutTests/fullscreen/f...
LayoutTests/fullscreen/full-screen-iframe-zIndex-expected.html:2: <button>Go
full screen</button>
On 2014/08/11 11:22:08, philipj wrote:
> On 2014/08/11 02:34:04, falken wrote:
> > Out of scope for this patch, but...
> > 
> > this test is pretty confusing. It says only a "white box" should be visible
> but
> > it shows this button which made me think at first the test doesn't even
> exercise
> > fullscreen. I see now though the iframe that goes fullscreen has this button
> > which isn't used in the test.
> 
> I've updated the test to use empty.html instead and tweaked the description.

Thanks!

> 
> > We should also add a z-index: max_int test which shows crbug.com/398599
> 
> Is the attached zindex-defeats-fullscreen.html still good? Wanna create a
> review?

Good idea, I'll do that.

https://codereview.chromium.org/457663002/diff/20001/LayoutTests/fullscreen/f...
File LayoutTests/fullscreen/full-screen-placeholder.html (left):

https://codereview.chromium.org/457663002/diff/20001/LayoutTests/fullscreen/f...
LayoutTests/fullscreen/full-screen-placeholder.html:22: <div>This layout test
checks that the offset positions of the blue and green divs does not change when
the red div enters full-screen mode.  Press <button
onclick="document.getElementById('float').webkitRequestFullScreen()">go
full-screen</a> to begin.</div>
On 2014/08/11 11:22:08, philipj wrote:
> On 2014/08/11 02:34:04, falken wrote:
> > I don't feel too strongly about it but I find these manual buttons useful
when
> > debugging. Does keeping it complicate the expected result?
> 
> Having dummy buttons in the expected results felt a bit odd, but I've added
them
> back. I also introduced a few goFullScreen functions to make sure that the
> manual button and the automated test actually do the same thing.

I see, thanks for bringing them back. goFullScreen is a nice improvement.

https://codereview.chromium.org/457663002/diff/20001/LayoutTests/fullscreen/f...
File LayoutTests/fullscreen/full-screen-stacking-context-expected.html (right):

https://codereview.chromium.org/457663002/diff/20001/LayoutTests/fullscreen/f...
LayoutTests/fullscreen/full-screen-stacking-context-expected.html:4: var
runPixelTests = true;
On 2014/08/11 11:22:08, philipj wrote:
> On 2014/08/11 02:34:05, falken wrote:
> > I'm not positive, but it might be possible to remove these runPixelTests
> > variables now that everything is a reftest.
> 
> Unfortunately it isn't, calling testRunner.dumpAsText() makes the test fail,
> expecting "pass" but getting "image". Splitting runWithKeyDown into a separate
> script would help, though, if you think it's worth it.

Ah, this is because not all users of full-screen-tests.js are ref tests? I was
thinking it may be possible to delete the lines:
    if (window.runPixelTests)
        testRunner.dumpAsTextWithPixelResults();
    else
        testRunner.dumpAsText();

[philipj_slow, 2014-08-12 06:41:20]

https://codereview.chromium.org/457663002/diff/20001/LayoutTests/fullscreen/f...
File LayoutTests/fullscreen/full-screen-stacking-context-expected.html (right):

https://codereview.chromium.org/457663002/diff/20001/LayoutTests/fullscreen/f...
LayoutTests/fullscreen/full-screen-stacking-context-expected.html:4: var
runPixelTests = true;
On 2014/08/12 00:18:00, falken wrote:
> On 2014/08/11 11:22:08, philipj wrote:
> > On 2014/08/11 02:34:05, falken wrote:
> > > I'm not positive, but it might be possible to remove these runPixelTests
> > > variables now that everything is a reftest.
> > 
> > Unfortunately it isn't, calling testRunner.dumpAsText() makes the test fail,
> > expecting "pass" but getting "image". Splitting runWithKeyDown into a
separate
> > script would help, though, if you think it's worth it.
> 
> Ah, this is because not all users of full-screen-tests.js are ref tests? I was
> thinking it may be possible to delete the lines:
>     if (window.runPixelTests)
>         testRunner.dumpAsTextWithPixelResults();
>     else
>         testRunner.dumpAsText();

No, that can't be removed just yet. However, after trust-event.js has landed
maybe I can convert some reftests to use that instead, getting rid of
runPixelTests.

[philipj_slow, 2014-08-12 06:41:26]

The CQ bit was checked by philipj@opera.com

[commit-bot: I haz the power, 2014-08-12 06:42:28]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/philipj@opera.com/457663002/40001

[commit-bot: I haz the power, 2014-08-12 06:46:37]

Change committed as 180036