Closed Bug 568392 Opened 10 years ago Closed 10 years ago

Enable testing of viewport scrolling in reftests

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: roc, Assigned: roc)

References

Details

Attachments

(4 files, 5 obsolete files)

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.
Attached patch Part 1: fix tests (obsolete) — Splinter Review
First we need to modify some existing tests that fail when scrollbars are visible.
Attachment #447663 - Flags: review?(dbaron)
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.
Attached patch Part 3: fix reftest harness (obsolete) — Splinter Review
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.
Attachment #447667 - Flags: review?(dbaron)
Depends on: 564991
No longer depends on: 564993
Attached patch Part2 v2 (obsolete) — Splinter Review
One of the previous images was wrong and caused a test failure. This fixes it.
Attachment #447664 - Attachment is obsolete: true
Attached patch Part 1 v2 (obsolete) — Splinter Review
A test was added that needs to be fixed.
Attachment #447663 - Attachment is obsolete: true
Attachment #447922 - Flags: review?(dbaron)
Attachment #447663 - Flags: review?(dbaron)
(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.
Attachment #447921 - Attachment is obsolete: true
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.
Attachment #447922 - Flags: review?(dbaron) → review+
(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)
Attached patch Part 1 v3Splinter Review
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)
Attachment #447664 - Attachment is obsolete: false
Whiteboard: [needs review]
Attached patch Part 3 v2 (obsolete) — Splinter Review
Addressed comment. Now we always use DRAWWINDOW_DRAW_VIEW and pass DRAWWINDOW_USE_WIDGET_LAYERS if the window is the right size.
Attachment #451462 - Flags: review?(dbaron)
Attachment #447667 - Attachment is obsolete: true
Attachment #447667 - Flags: review?(dbaron)
Blocks: 564991
No longer depends on: 564991
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.
Attachment #451462 - Flags: review?(dbaron) → review+
It matches my assumptions that there aren't any margin/border/padding, yes.

I'll add those tests.
Whiteboard: [needs review] → [needs landing]
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".
Whiteboard: [needs landing]
Attached patch Part 3 v3Splinter Review
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.
Attachment #447922 - Attachment is obsolete: true
Attachment #451462 - Attachment is obsolete: true
Whiteboard: [needs landing]
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!
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [needs landing]
You need to log in before you can comment on or make changes to this bug.