624636 - Allow reftests to set a "displayport" and "view transform"

Status: RESOLVED FIXED

(Core :: Layout, defect)

Comment 1

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Ugh, I keep missing the Shift key.

Anyway, what I want is to enable the reftest harness to be able to check bug 593243, for example.  I'd like to add magical document properties like we use for printing to define the necessary parameters.  The chrome side of the reftest harness would also need to enable async scroll mode for tests using this feature and disable it when done.

Comment 2

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: part 1: Allow reftests to override viewport+displayport

Pretty straightforward.  r? to roc since he reviewed most of the platform code being used here.

Comment 3

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: part 2: Allow reftests to disable layers-sync-before-snapshot, in order to test "natural" widget repainting

Requesting feedback on this because I'm not sure it adheres to the "reftest philosophy".

I wanted to use the code in this bug to test bug 593243.  Here's the test I wrote:
https://bug593243.bugzilla.mozilla.org/attachment.cgi?id=502873 .

This test, however, didn't tickle the underlying platform bug when run in the reftest harness.  The platform bug is that repaint tasks are being blocked because layout's and view's ideas of what's visible can be different when displayport rendering is enabled.  The reftest harness itself has to force shadow-layer repaints before requesting snapshots.  In this case, the synchronization code was working perfectly well; it didn't invalidate any additional content.  However, just forcing *any* repaint was enough to prevent this test from detecting the platform bug.

This patch adds a reftest-nosync attribute that tells the harness *not* to sync shadow layers before requesting an update snapshot.  This forces the test itself to ensure the necessary paints have finished.

It's not really possible, currently, to implement reftest-nosync for in-process content in the same way as for out-of-process content, and indeed this patch just ignores reftest-nosync for in-process content.

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 502874 [details] [diff] [review]
part 1: Allow reftests to override viewport+displayport

Why bother with useDisplayport? Calling setupDisplayport unconditionally would work fine. r+ with that.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Hmm. Should the attribute in part 2 be called reftest-no-sync-layers?

Comment 6

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to comment #4)
> Comment on attachment 502874 [details] [diff] [review]
> part 1: Allow reftests to override viewport+displayport
> 
> Why bother with useDisplayport? Calling setupDisplayport unconditionally would
> work fine. r+ with that.

Sure, we could do that.

(In reply to comment #5)
> Hmm. Should the attribute in part 2 be called reftest-no-sync-layers?

Suits me.

Comment 7

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to comment #3)
> Created attachment 502883 [details] [diff] [review]
> part 2: Allow reftests to disable layers-sync-before-snapshot, in order to test
> "natural" widget repainting

David, do you want to do the feedback? here, or should I post this for review?

Comment 8

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 502883 [details] [diff] [review]
part 2: Allow reftests to disable layers-sync-before-snapshot, in order to test "natural" widget repainting

Sorry, need to stop filing these bugs in "Testing" because they don't have the flags we need.

Requesting approval because
 - these patches are the only means we have of testing bug 593243
 - they're test-only code and safe, and sadly even this code within the reftest harness still doesn't run on tinderbox

Comment 9

[cmtalbert]

(In reply to comment #8)
> Comment on attachment 502883 [details] [diff] [review]
> part 2: Allow reftests to disable layers-sync-before-snapshot, in order to test
> "natural" widget repainting
> 
> Sorry, need to stop filing these bugs in "Testing" because they don't have the
> flags we need.
> 
> Requesting approval because
>  - these patches are the only means we have of testing bug 593243
>  - they're test-only code and safe, and sadly even this code within the reftest
> harness still doesn't run on tinderbox
Hmmm, I don't have necessary bits to give you approval, but in general test only code doesn't need approval to land.  We have a blanket "a=test-only" approval, and that's what we generally use when we're updating the frameworks.  I'd go ahead and land this with that a=test-only flag.

Comment 10

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 502883 [details] [diff] [review]
part 2: Allow reftests to disable layers-sync-before-snapshot, in order to test "natural" widget repainting

a=beltzner (fwiw, test-only changes don't need explicit approval)

Comment 11

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

http://hg.mozilla.org/mozilla-central/rev/f96f8d9b0f63
http://hg.mozilla.org/mozilla-central/rev/20b7143e5a27

Comment 12

[Martijn Wargers (dead)]

Would it be good to documentate this is in http://mxr.mozilla.org/mozilla-central/source/layout/tools/reftest/README.txt ?

Comment 13

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Yes, it would.

Comment 14

[Eric Shepherd [:sheppy]]

If this is a change to a readme file, this is outside the purview of the docs team, and I will clear the doc needed flag. If this is content that should be on devmo, then that's a whole other ball of wax. Let me know.

Comment 15

[Eric Shepherd [:sheppy]]

Is someone going to update the readme? Or should the documentation in that readme be migrated to devmo?

Comment 16

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

dbaron, roc, should the reftest README be on devmo?

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Let's just document it in the README file.

Comment 18

[Eric Shepherd [:sheppy]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #17)
> Let's just document it in the README file.

Who will do that then? Should there be a new bug for that or will it be handled here?