To test scrolling with reftests, which will be possible with retained layer scrolling, we need to modify the reftest harness. My patch tests the size of the reftest window. If it's the desired size (800x1000), we pass DRAWWINDOW_DRAW_VIEW to enable viewport scrolling and clipping, otherwise we don't. This ensures that mobile devices can still run reftests adequately.

First we need to modify some existing tests that fail when scrollbars are visible.

I noticed a couple of reftest images don't match their descriptions. I don't think this needs review, I'll check it in with the previous patch.

This is the part that actually enables viewport scrolling in reftests. I think these patches should work on trunk, without the retained layers stuff, but I'll check with the try server.

One of the previous images was wrong and caused a test failure. This fixes it.

A test was added that needs to be fixed.

(In reply to comment #2) > Created an attachment (id=447664) [details] > Part 2: fix images > > I noticed a couple of reftest images don't match their descriptions. I don't > think this needs review, I'll check it in with the previous patch. What was the mismatch? They look to me like they match their descriptions.

Comment on attachment 447921 [details] [diff] [review] Part2 v2 Hmm, they do to me too. I thought the borders weren't the right thickness but I guess I was wrong.

Oh, the size. Yes, it looks like the transparent-in-blue is the wrong size, though the green looks right to me. It's possible that was intentional, though.

If it was intentional, I think it should have been named layout/reftests/image-rect/transparent-14x14-in-blue-32x32.png

Yeah, so changing that image is fine, assuming things still pass.

(In reply to comment #0) > My patch tests the size of the reftest window. If it's the desired size > (800x1000), we pass DRAWWINDOW_DRAW_VIEW to enable viewport scrolling and > clipping, otherwise we don't. This ensures that mobile devices can still run > reftests adequately. Why would mobile devices have the window a different size? It's an iframe rather than a toplevel window, so it really seems like it should always be the right size.

Comment on attachment 447667 [details] [diff] [review] Part 3: fix reftest harness > function InitCurrentCanvasWithSnapshot() > { >+ if (gURLs[0].type == TYPE_LOAD || gURLs[0].type == TYPE_SCRIPT) { >+ // We don't want to snapshot this kind of test >+ return; >+ } Was the problem here that we were doing snapshots for tests with reftest-wait? It wasn't affecting non-reftest-wait tests, right?

Comment on attachment 447922 [details] [diff] [review] Part 1 v2 I don't understand the change to the very first pair of tests (379349-*), but everything else looks fine. I'll trust you on the first one, but please double-check it yourself.

(In reply to comment #11) > (In reply to comment #0) > > My patch tests the size of the reftest window. If it's the desired size > > (800x1000), we pass DRAWWINDOW_DRAW_VIEW to enable viewport scrolling and > > clipping, otherwise we don't. This ensures that mobile devices can still run > > reftests adequately. > > Why would mobile devices have the window a different size? It's an iframe > rather than a toplevel window, so it really seems like it should always be the > right size. OK. Then I probably need to strip the DRAWWINDOW_USE_WIDGET_LAYERS flag instead, since the layer tree will be cropped to the visible area of the IFRAME. (In reply to comment #12) > (From update of attachment 447667 [details] [diff] [review]) > > function InitCurrentCanvasWithSnapshot() > > { > >+ if (gURLs[0].type == TYPE_LOAD || gURLs[0].type == TYPE_SCRIPT) { > >+ // We don't want to snapshot this kind of test > >+ return; > >+ } > > Was the problem here that we were doing snapshots for tests with reftest-wait? > It wasn't affecting non-reftest-wait tests, right? Right. The problem was that for a load-only reftest-wait test, we'd take a snapshot and set gCurrentCanvas to a non-null canvas. Then when the test finaly completed, DocumentLoaded wouldn't clear gCurrentCanvas under "if (gURLs[0].type == TYPE_LOAD)". Then we'd run a non-load-only, non-reftest-wait test, and in DocumentLoaded we'd get down to } else if (gCurrentCanvas == null) { InitCurrentCanvasWithSnapshot(); } but gCurrentCanvas would be non-null, so we wouldn't take a snapshot, we'd just use the old gCurrentCanvas, and fail. Never taking a snapshot during a load-only test seemed like the best way to fix this. (In reply to comment #13) > (From update of attachment 447922 [details] [diff] [review]) > I don't understand the change to the very first pair of tests (379349-*), but > everything else looks fine. I'll trust you on the first one, but please > double-check it yourself. We need to make the height of the tests match the reference exactly, otherwise they get a different number of pages which makes the scrollbar for the "print preview" display get a different height.

(the scrollbar thumb, that is)

Did a couple of extra innocuous things: -- removed test_scrolling.html since it's obsoleted by the new scrolling infrastructure -- broke image-rect/background-zoom into four parts, one per DIV, since the fourth DIV fails on Mac (on trunk as well as with my patches, for me, and it's because Mac doesn't support EXTEND_PAD, which is a known issue my patches must be making show up on tryserver)

Addressed comment. Now we always use DRAWWINDOW_DRAW_VIEW and pass DRAWWINDOW_USE_WIDGET_LAYERS if the window is the right size.

Do we check somewhere that the iframe we're using fits exactly in the window it's in, and that there isn't any margin/border/padding/etc. pushing it a little bit off?

No, but don't we control that in reftest.xul anyway?

Sure, we control it. But does it match your assumptions?

Comment on attachment 451462 [details] [diff] [review] Part 3 v2 r=dbaron if you add 4 reftests to layout/reftests/reftest-sanity/ checking that a red 1px * 1px div is != to a green 1px * 1px div when positioned at each of the 4 corners of the 800*1000 canvas that we expect.

It matches my assumptions that there aren't any margin/border/padding, yes. I'll add those tests.

These tests won't really test anything on trunk, but with retained layer scrolling these tests will actually test that scrolling works and the right layer contents are retained or updated.

Part 1 of this bug is causing layout/reftests/first-line/stress-8.html to fail reliably. The test image is showing the rendering of layout/reftests/first-line/stress-6.html instead of stress-8 for some reason. I see this reliably on Linux, as does tbox on all three platforms...

I pushed http://hg.mozilla.org/mozilla-central/rev/88fa0b783306 to fix that orange; roc, can you review it? The issue was that if a 'load' test used reftest-wait, we'd set up a gCurrentCanvas for it in the MozAfterPaint event that fired before the class was removed. Then when loading a following '==' test we'd hit this code in DocumentLoaded: if (gURICanvases[gCurrentURL]) { gCurrentCanvas = gURICanvases[gCurrentURL]; } else if (gCurrentCanvas == null) { InitCurrentCanvasWithSnapshot(); } which would do nothing, so we'd keep using the gCurrentCanvas we had instead of taking a snapshot of the test page. Then of course the '==' test would fail.

Sorry sorry sorry. I forgot that part 3 fixed a bug in the harness that caused failures with part 1. Your fix is fine, r=me.

Parts 1, 2 and 4 stuck: http://hg.mozilla.org/mozilla-central/rev/971eabebb8b5 http://hg.mozilla.org/mozilla-central/rev/910c3d31a381 http://hg.mozilla.org/mozilla-central/rev/1e2a5a66b9d7 I backed out part 3 because of failures on Windows with the new tests dbaron asked me to add. It seems on Windows the IFRAME isn't tall enough! reftest.xul contains <browser id="browser" flex="1" type="content-primary" style="width: 800px; height: 1000px" /> I guess that needs to be flex="0".

This finally passed reftests on Windows tryserver. The problem was that we need to use min/max width/height to force the iframe size to be correct along both axes.

Hmmm. Seems odd that the iframe was some different size.

With no flex, the height was OK but the width changed to the width of the window, which seemed to be the problem on Windows.

http://hg.mozilla.org/mozilla-central/rev/9bf8818e3c64 And everything's green!