Closed Bug 1299848 Opened 3 years ago Closed 2 years ago

Add the ability to test printed output

Categories

(Core :: Printing: Output, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: jrmuizel, Assigned: tschneider)

References

(Depends on 1 open bug, Blocks 4 open bugs)

Details

Attachments

(3 files, 38 obsolete files)

1.00 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
14.91 KB, patch
Details | Diff | Splinter Review
33.53 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
We should get the ability to rasterize print output and compare it to another rasterization to make sure that it's sane.

- We can use CGContextDrawPDFPage on OS X
- PlayEnhMetaFile on Windows
- On linux we can probably use ghostscript.
Blocks: 1246805
Priority: -- → P3
Blocks: 1309272
Attached patch Reftest examples (obsolete) — Splinter Review
Assignee: nobody → tschneider
Uses PDF.js to compare print output with reference HTML. Documentation is following soon.
Isn't there already a copy of pdf.js in the tree? It would be nice to not duplicate it.
(In reply to Jeff Muizelaar [:jrmuizel] from comment #5)
> Isn't there already a copy of pdf.js in the tree? It would be nice to not
> duplicate it.

Also, it seems like it might make more sense to use the system pdf renderer instead of pdf.js so that we're not relying on our own rendering to check our own rendering.
PDF.js is not used for rendering here. I use the systems one by silently printing to a PDF. Then using PDF.js to parse the output file and e.g. compare its text content or number of pages with a reference file.
Re the duplicate copy: I used a slightly modified version of PDF.js here, making it possible to load it as a module. But I might update the one we already have in the tree and share it.
Attachment #8837789 - Attachment is obsolete: true
Attachment #8842512 - Attachment is obsolete: true
Attached patch Reftest examples (obsolete) — Splinter Review
Attachment #8837790 - Attachment is obsolete: true
Previous patches introduce a new reftest type 'print' that adds the ability to test printed output.

A print reftest passes if a PDF generated as a result of printing the test page equals
the reference page. Current assertions applied to test for equality are:

1. The number of total pages in the generated PDF must be the SAME as the number of
   elements matching "body > .page" in the reference page.

2. All text content of the reference page must match (non-rasterized) text content of
   the generated PDF.

...

More assertions to follow. All suggestions are welcome.
Updated patch to load and use the pdf.js bundles that already ship with FF.
Attachment #8847758 - Attachment is obsolete: true
Attached patch Reftest examples (obsolete) — Splinter Review
Attachment #8847759 - Attachment is obsolete: true
Attached patch PDF.js patches (obsolete) — Splinter Review
Patch for PDF.js bundles shipped with FF. These fixes will be upstreamed. Just adding them here if needed to test this bug.
Attached patch Reftest examples (obsolete) — Splinter Review
Actually including the files this time :).
Attachment #8863498 - Attachment is obsolete: true
Upstream PR for PDF.js patches: https://github.com/mozilla/pdf.js/pull/8361
Blocks: 428037
Depends on: 1362108
Added support to test print-selection and print-range.
Attachment #8863497 - Attachment is obsolete: true
Attachment #8866632 - Attachment is obsolete: true
Comment on attachment 8863501 [details] [diff] [review]
PDF.js patches

PDF.js fixes already landed in m-c.
Attachment #8863501 - Attachment is obsolete: true
Attached patch Printing reftests (obsolete) — Splinter Review
Attachment #8863502 - Attachment is obsolete: true
Do you have plans to support backends other than PDF? Hooking up EMF seems pretty important.
Flags: needinfo?(tschneider)
No concrete plans yet (tho I already looked into supporting PostScript), put definitely doable and will keep it in mind.
Flags: needinfo?(tschneider)
Attachment #8866633 - Flags: review?(dbaron)
So it seems like print tests could still use all of '==', '!=', and (for crashtests) 'load' tests.  So I'm not keen on the idea of 'print' being an alternative to those rather than a modifier of them.

I'd also, when possible, prefer things to be in the test files rather than the manifests.  But maybe this has to be in the manifest?

The documentation needs to *clearly* distinguish between the existing reftest-print mechanism that tests pagination and these.  I think it should also probably distinguish in naming.  I'm inclined to think that we should also rename *both* of them to have clearly disambiguated names (e.g., rename the existing one to paginated and this new one to pdf-printing, or something like that?).

Thoughts?
Flags: needinfo?(tschneider)
It's more like a 'script' than any of the others. Yet it takes reference html, like '==' and '!='. I choose it to be an alternative to clearly distinguish it from the others, since its not really comparing the two html's rather then generating an IR format (for now PDF, but could also be PS or EMF in the future). It also requires slightly different code paths in reftest.jsm which led to my decision here.

I agree on the point that the documentation should distinguish it more from the existing solution.
Flags: needinfo?(tschneider)
The normal harness is converting the two HTML files to 800x1000 images and then comparing the results.  Your new thing is still converting to another format and then comparing -- it's just a different result format that you're comparing for equality, right?
Comment on attachment 8866633 [details] [diff] [review]
Add support for (real) print reftests

>+            2. All text content of the reference page must match
>+               (non-rasterized) text content of the generated PDF.

OK, apparently this doesn't mean what I think it means, but instead means something that's very different from reftest.  (I thought you were just describing reftest equality but with strange wording, but based on IRC conversation it sounds like it means something entirely different, but I don't understand what.)

Please expand this to explain what your new test format does in a way that makes it clear to somebody who doesn't already know.

I'm also interested to know *why* you chose to make the tests work that way.
Attachment #8866633 - Flags: review?(dbaron) → review-
... and why you chose to put it in the reftest harness rather than a separate thing.
As discussed on IRC, current reftest-print should rather be called reftest-paged.
Comment on attachment 8871447 [details] [diff] [review]
Rename reftest-print to reftest-paged

It would be good to make reftest-print an error (at least temporarily) so that people adding new tests around the same time don't accidentally check in some "reftest-print" tests that don't get fixed up.

(And then also to land this a bit in advance of the other patch.)
Yeah thats a good idea, will include that.
This patch changes the current implementation to print out PDFs for both, the test and the reference html and compares the structure of the two PDFs using PDF.js. I think this approach comes closer to our definition of a reftest. I also added an exception when obsolete 'deftest-print' is, with a hint to use the new 'reftest-paged' instead.
Attachment #8866633 - Attachment is obsolete: true
Just noticed that I've overseen dbaron's last sentence in comment 29. This totally makes sense and means that the check for usage of obsolete 'deftest-print' should go into attachment 8871447 [details] [diff] [review].
Attachment #8871447 - Attachment is obsolete: true
Attachment #8871525 - Attachment is obsolete: true
I hit some UNEXPECTED-FAIL with your patch in Windows.
Thanks for debugging together during the work week.
I summarized our finding in following.
1. Set page margin to 0 to avoid printing head and foot.
2. Please handle expected fail case.
3. Please modify function doPrintMode() of reftest-content.js
like this:

function doPrintMode(contentRootElement) {
     // use getAttribute because className works differently in HTML and SVG
-    var classList = contentRootElement &&
-                    contentRootElement.hasAttribute('class') &&
-                    contentRootElement.getAttribute('class').split(/\s+/);
-    if (classList.indexOf("reftest-print")) {
-        SendException("reftest-print is obsolete, use reftest-paged instead");
-        return;
+    if (contentRootElement &&
+        contentRootElement.hasAttribute('class')) {
+      var classList = contentRootElement.getAttribute('class').split(/\s+/);
+      if (classList.indexOf("reftest-print")) {
+          SendException("reftest-print is obsolete, use reftest-paged instead");
+          return;
+      }
+      return classList.indexOf("reftest-paged") != -1;
     }
-    return classList.indexOf("reftest-paged") != -1;
+    return;
 }
Farmer, it was nice meeting you at the all hands and thanks for the patch. I'm going to update the patches accordingly.
Updated patch.
Attachment #8871528 - Attachment is obsolete: true
Attached patch Printing reftests (obsolete) — Splinter Review
Updated reftests.
Attachment #8866634 - Attachment is obsolete: true
Updated patch.
Attachment #8871527 - Attachment is obsolete: true
Priority: P3 → P2
Duplicate of this bug: 1242463
dbaron: We now changed the test harness to generate 2 PDF's from the test and reference HTML and compare them with each other. Even tho this is still not a visual comparison rather than a structural, I think this comes closer to the definition of a ref test. Could this be something you can live with being in the ref-test rather then creating something separate? We should get something landed soonish so we can start having useful tests written.
Flags: needinfo?(dbaron)
(In reply to Tobias Schneider [:tobytailor] from comment #41)
> dbaron: We now changed the test harness to generate 2 PDF's from the test
> and reference HTML and compare them with each other. Even tho this is still
> not a visual comparison rather than a structural, I think this comes closer
> to the definition of a ref test. Could this be something you can live with
> being in the ref-test rather then creating something separate? We should get
> something landed soonish so we can start having useful tests written.

That sounds reasonable.  (Assuming s/ rather than/, rather/.)
Flags: needinfo?(dbaron)
Attachment #8883708 - Attachment is obsolete: true
Depends on: 1382327
Attachment #8883701 - Flags: review?(dholbert)
No longer blocks: 1309272
Blocks: 1321689
Comment on attachment 8883701 [details] [diff] [review]
Add support for (real) print reftests

Review of attachment 8883701 [details] [diff] [review]:
-----------------------------------------------------------------

Both patches here are missing a commit message.

(Do you mean to have two separate patches, or did you want to fold them together? Either way seems fine to me.)

::: layout/reftests/printing/reftest.list
@@ +41,5 @@
> +
> +print test-text.html test-text-ref.html
> +fails print test-text-fail.html test-text-fail-ref.html
> +print test-number-of-pages.html test-number-of-pages-ref.html
> +fails print test-number-of-pages-fail.html test-number-of-pages-fail-ref.html

Note: these reftest.list changes belong in the other patch (where these HTML files are added).  But one note, while I'm looking at them:  I think here you're using "fails" to indicate that these two files are *expected not to match* -- is that right?

That's kind of abusing the "fails" annotation.  For normal reftests, we have "==" for tests that are expected to match, and "!= " for tests that are expected *not* to match.  And then *separately*, we have the "fails" annotation, which can be added as a modifier to either of those, to indicate that we don't live up to the test's expectations currently -- and it indicates that *our current rendering is known-incorrect*.  This makes a difference in the output, too -- both != and == tests are reported as TEST-EXPECTED-PASS when they pass, IIRC (whereas if there's a "fails" annotation, it gets marked as TEST-EXPECTED-FAIL, in the sense of this being a known bug/failure, and it increments a different count for test results).

So: your "fails" usage here is probably worth avoiding & probably going to cause some confusion...  Perhaps we need "print==" and "print!=", or something like that, to better distinguish between expected-mismatch vs. known-test-failure?  I would defer to dbaron on this, probably.  Ideally we'd still just use "==" and "!=" with "print" being encoded into the test itself, but I assume there's reasons that can't work?

::: layout/tools/reftest/README.txt
@@ +278,5 @@
>  
>               url_ref must be omitted. The test may be marked as fails or
>               random. (Used to test the JavaScript Engine.)
> +      print  The test passes if the printouts (as PDF) of the two renderings
> +             are the SAME.

This isn't really true, is it?  We're not comparing the pixels of the two renderings.  Please mention what we *do* compare here.

(Questions that come to mind: what size paper do we use for the printouts? What margins? What orientation? I don't actually see that encoded in reftest-content.js at all

It would also be good to have a parenthetical here mentioning "reftest-paged" and briefly explaining the difference (and ideally also explaining why this verison needs to be its own manifest-keyword, rather than a <html class> like for "print".)

(Perhaps you're leaving this intentionally vague because there are more comparisons that are planned but not-yet-implemented -- e.g. comparing the actual pixels of the rendering.  If so: you should still mention what we *do* compare, and maybe include hypothetical future plans in an afterthought or an XXX comment or something.)

::: layout/tools/reftest/reftest-content.js
@@ +21,5 @@
>  // "<!--CLEAR-->"
>  const BLANK_URL_FOR_CLEARING = "data:text/html;charset=UTF-8,%3C%21%2D%2DCLEAR%2D%2D%3E";
>  
>  CU.import("resource://gre/modules/Timer.jsm");
> +//CU.import("resource://gre/modules/AsyncSpellCheckTestHelper.jsm");

This commented-out import probably wants to get removed.

@@ +59,5 @@
>  
>  const TYPE_LOAD = 'load';  // test without a reference (just test that it does
>                             // not assert, crash, hang, or leak)
>  const TYPE_SCRIPT = 'script'; // test contains individual test results
> +const TYPE_PRINT = 'print';

This probably needs a brief comment like the lines above it.

@@ +254,5 @@
> +    ps.printToFile = true;
> +    ps.toFileName = file.path;
> +    ps.printFrameType = CI.nsIPrintSettings.kFramesAsIs;
> +    ps.outputFormat = CI.nsIPrintSettings.kOutputFormatPDF;
> +    

whitespace on blank line

::: layout/tools/reftest/reftest.jsm
@@ +422,5 @@
>  #endif
>  
> +    try {
> +        prefs.setBoolPref("print.print_via_parent", false);
> +    } catch (e) {}

Could you include a comment here hinting at why we need to toggle this pref?  (and indicating that it's specifically for "print" reftests -- that might seem obvious, but for all I know reading this file, maybe it's related to reftest-paged or something.)

@@ +1687,5 @@
> +          
> +            gTestPrintOutput = scriptResults;
> +          
> +            // Proceed to load the second document.
> +          

fix whitespace on last 3 blank lines here (perhaps by getting rid of some of those blank lines?)

@@ +2357,5 @@
> +                            resolve({
> +                                passed: passed,
> +                                description: passed ?
> +                                    "Page " + pages[0].pageNumber + " contains same text" :
> +                                    "Expected page " + pages[0].pageNumber + " to contain text '" + text1 + "'"

Shouldn't this error output use text2, rather than text1?  (Isn't text2 the "reference" version, i.e. the thing we're expecting the other thing to match?)
Comment on attachment 8883701 [details] [diff] [review]
Add support for (real) print reftests

Review of attachment 8883701 [details] [diff] [review]:
-----------------------------------------------------------------

One more note on the first part for the time being:

::: layout/tools/reftest/reftest-content.js
@@ +261,5 @@
> +    } else if (printRange) {
> +        ps.printRange = CI.nsIPrintSettings.kRangeSpecifiedPageRange;
> +        var range = printRange.split('-');
> +        ps.startPageRange = +range[0] || 1;
> +        ps.endPageRange = +range[1] || 1;

What will happen here if someone includes invalid characters (e.g. letters, or a comma, or a period for a decimal number) in their print range, so that we can't get parse out valid numbers here? Do we end up printing nothing, or the whole thing, or falling over in some way?  Similarly, what if someone includes 2 hyphens (e.g. "-2-3"), or just a blank space (reftest-print-range=" ")

Perhaps we should validate the input, and throw an exception or force the test to fail, to indicate that there was a problem?  (My main worry is that someone might *think* they're including a test with their patch, but their test actually doesn't test anything because they've got a typo in their range syntax. And reviewers are unlikely to notice this for the forseeable future because reviewers won't necessarily be familiar with this syntax.)


Also: Please make sure that the "reftest-print-selection" and "reftest-print-range" attributes are documented somewhere (I don't think there's any documentation for them in this patch right now.)
Attachment #8883701 - Flags: review?(dholbert) → review-
(In reply to Daniel Holbert [:dholbert] from comment #43)
> That's kind of abusing the "fails" annotation.  For normal reftests, we have
> "==" for tests that are expected to match, and "!= " for tests that are
> expected *not* to match.  And then *separately*, we have the "fails"
> annotation, which can be added as a modifier to either of those

After some further thought: maybe you're not expecting "!=" style tests to be very common in actual tests, and you're only using "fails" here to test the harness itself?  If so, that makes a bit more sense to me.  It would be good to explicitly call that out with a code-comment, though, alongside these tests in the reftest.list manifest -- maybe something like  the following:
"The 'print' tests below that are annotated as 'fails' are really just testing the integrity of the reftest harness.  Their testcases & reference case are known to differ, and we're testing that the harness correctly detects them as differing & correctly handles the 'fails' annotation."


(It'd still be nice to still have the ability to include "!=" tests down the road, but maybe that doesn't have to be a dealbreaker at this stage.)
Comment on attachment 8883702 [details] [diff] [review]
Printing reftests

Review of attachment 8883702 [details] [diff] [review]:
-----------------------------------------------------------------

Some nits on the reftest files patch:

::: layout/reftests/printing/test-number-of-pages-fail.html
@@ +4,5 @@
> +  <meta charset="utf-8">
> +  <style>
> +    @page {
> +      margin: 0;
> +    }

If it's expected that all print reftests should include this, perhaps you should pull it out into a helper .css file?

(And as I recall, you mentioned to me that this "margin:0" is *required* for tests to have any chance of matching -- otherwise we print the header/footer text, which will trivially differ due to having different filename and/or timestamp.  If that's the case, then you should *definitely* pull this out into a helper .css file, so that you can include an explanatory comment inside that helper file with a mention of the fact that "print" reftests *need* to have this.  Also, that way, new tests can pull in that CSS as a one-liner, rather than always needing an identical 5-line <style> block.)

::: layout/reftests/printing/test-number-of-pages.html
@@ +5,5 @@
> +  <style>
> +    @page {
> +      margin: 0;
> +    }
> +    

whitespace on blank line.

Also: once that whitespace is removed, this testcase and its reference case (test-number-of-pages-ref.html) are identical -- that's probably a mistake?

If you want to assert that this file's markup renders consistently, you could do so more transparently by just including the same filename twice on the same line in your manifest (e.g. "print test-number-of-pages.html test-number-of-pages.html" or whatever), rather than using a duplicate copy of the file as its reference.

Or if you really want to exercise the harness by testing it on two files that happen to have the same contents, you should make it clearer that that's what you're doing in the manifest and/or the reference case.  Otherwise, the default assumption is that testcase and reference case have different contents, and that we're likely to render the correct thing for the reference case even if we mess up the testcase. (And that assumption is clearly invalid here, where the two files have the same contents.)

::: layout/reftests/printing/test-print-range-ref.html
@@ +5,5 @@
> +  <style>
> +    @page {
> +      margin: 0;
> +    }
> +    

whitespace on blank line

::: layout/reftests/printing/test-print-range.html
@@ +5,5 @@
> +  <style>
> +    @page {
> +      margin: 0;
> +    }
> +    

whitespace on blank line

::: layout/reftests/printing/test-print-selection.html
@@ +11,5 @@
> +    function selectNodeById(id) {
> +      var node = document.getElementById(id);
> +      var rng = document.createRange();
> +      rng.selectNode(node);
> +      window.getSelection().addRange(rng);

Since you've got script here -- that leads me to wonder: does "reftest-wait"[1] work with these print tests?  (to let us run arbitrary amounts of async javascript, mess with the DOM, etc, before the print operation + comparison is actually performed)

* If it does work, please include a test that depends on it working (e.g. with some DOM mutation happening inside of a 0-duration setTimeout -- or perhaps two chained setTimeouts for good measure -- with the print being triggered by "reftest-wait" class removal.)
* If it doesn't work: we should document that it doesn't work (and maybe file a bug on making it work, since it's handy for testing bugs that only happen after dynamic changes to the page... maybe that's less important for static print content though?)

[1] https://dxr.mozilla.org/mozilla-central/rev/7d2e89fb92331d7e4296391213c1e63db628e046/layout/tools/reftest/README.txt#419-438

::: layout/reftests/printing/test-text-fail.html
@@ +8,5 @@
> +    }
> +  </style>
> +</head>
> +<body>
> +  <p>This text should appear on page 1</p>

This file is identical to "test-text.html", which raises the question of why we have two copies of the same testcase.

Either eliminate one copy, or (preferred) add a <title> element to each file to explain what it's testing.

::: layout/reftests/printing/test-text.html
@@ +8,5 @@
> +    }
> +  </style>
> +</head>
> +<body>
> +  <p>This text should appear on page 1</p>

Here, too, the testcase and its reference case are identical, so it's not clear that you're usefully testing anything.
RE "test-text-fail.html" vs "test-text.html" -- we sometimes have
 == foo.html foo-ref.html
 != foo.html foo-notref.html

You could do that here with test-text.html:
 print test-text.html test-text-ref.html
 fails print test-text.html test-text-notref.html

...or given that the reference case is identical, you maybe really want:
 print test-text.html test-text.html
 fails print test-text.html test-text-notref.html
?
Daniel, thanks for the fast and comprehensive review! 

> After some further thought: maybe you're not expecting "!=" style tests to
> be very common in actual tests, and you're only using "fails" here to test
> the harness itself?  If so, that makes a bit more sense to me.  It would be
> good to explicitly call that out with a code-comment, though, alongside
> these tests in the reftest.list manifest -- maybe something like  the
> following:
> "The 'print' tests below that are annotated as 'fails' are really just
> testing the integrity of the reftest harness.  Their testcases & reference
> case are known to differ, and we're testing that the harness correctly
> detects them as differing & correctly handles the 'fails' annotation."

Yes this is exactly what these fail notated tests are for. I will add a comment as suggested.
Addressed first batch of review comments
Attachment #8883701 - Attachment is obsolete: true
Attached patch Printing reftests (obsolete) — Splinter Review
Add comment to make clear that fail is used in manifest for testing only the harness itself.
Attachment #8883702 - Attachment is obsolete: true
> ...or given that the reference case is identical, you maybe really want:
>  print test-text.html test-text.html
>  fails print test-text.html test-text-notref.html
> ?

Like that. Will change it tho this scheme.
> (Questions that come to mind: what size paper do we use for the printouts?
> What margins? What orientation? I don't actually see that encoded in
> reftest-content.js at all

Was hoping you can set paper size and orientation via @page { size: ... } in the test itself but just realized FF doesn't have support for that jet. Anyhow, we are currently not testing if paper size or orientation is correct, but this would make a great follow-up candidate.

As for margin, this you can already define individually for each test via @page { margin: ... }.
(In reply to Tobias Schneider [:tobytailor] from comment #52)
> > (Questions that come to mind: what size paper do we use for the printouts?
> > What margins? What orientation? I don't actually see that encoded in
> > reftest-content.js at all
> 
> Was hoping you can set paper size and orientation via @page { size: ... } in
> the test itself but just realized FF doesn't have support for that jet.

Cool - please make sure the documentation mentions what size/orientation we *do* use (by default at least, if tests get the ability to customize this in the future).

(Is it always 8.5x11 portrait? Or the system's default paper size/orientation?  Or something else?)
Second batch of addressing review comments. Whats remaining is documentation and support for reftest-wait (this change includes a test for it which currently fails). Default paper size is set to 5x3, same as for reftest-paged.
Attachment #8889256 - Attachment is obsolete: true
Attachment #8889258 - Attachment is obsolete: true
Addressed all code related review comments. reftest-await works (with test). Current PDF.js build in FF has an issue when loading in jsm module. See previous comment with link to GitHub PR.
Attachment #8893622 - Attachment is obsolete: true
Attachment #8894709 - Flags: review?(dholbert)
Comment on attachment 8894709 [details] [diff] [review]
Add support for (real) print reftests

Review of attachment 8894709 [details] [diff] [review]:
-----------------------------------------------------------------

Looks like this patch needs a commit message (and author information) -- right now it's just a "hg diff" result.

::: layout/reftests/printing/test-async-paged.html
@@ +1,1 @@
> +<!DOCTYPE html><html class="reftest-wait reftest-paged"><style>html{font-size:12pt}</style>

It looks like this test file is simply being renamed from "test-reftest-print" to "test-reftest-paged" (with the <html class> being updated as well) -- is that correct?

If so, please represent this as a "hg mv" (aka "hg rename") operation, to reflect reality & so that blame is preserved & so the patch is smaller.

This *might* mean that you need [or want] to create your own new version of "test-async-print.html" in a separate commit (so that the rename + new-file-creation happen separately).

::: layout/reftests/printing/test-async-print.html
@@ +3,5 @@
>  <head>
> +  <meta charset="utf-8">
> +  <link href="print.css" rel="stylesheet">
> +  <script>
> +    window.onload = function () { 

Fix EOL whitespace

@@ +8,5 @@
> +        setTimeout(function() {
> +            document.getElementById("page1").innerText = 'This text should appear on page 1';
> +            document.documentElement.className = "";
> +        },
> +        1000);

Why do we have a 1-second timeout here?  I think you're copying this from the old version of test-async-print.html, but I have the same question about that existing test, actually.

 - Please reduce this (to 0 if possible -- or even better, listen for the MozReftestInvalidate event instead of an arbitrary time value).
 - Please include a comment to explain why we are using setTimeout in the first place.

1 second may not seem like much, but when viewed in the grand scheme of *all reftest runs from now until forever*, a mandatory 1-second "sit and do nothing" instruction is a pretty significant waste of time!  (when the test might otherwise only take a few milliseconds)

::: layout/tools/reftest/README.txt
@@ +278,5 @@
>  
>               url_ref must be omitted. The test may be marked as fails or
>               random. (Used to test the JavaScript Engine.)
> +      print  The test passes if the printouts (as PDF) of the two renderings
> +             are the SAME.

This README text seems the same as it was in my last round of review (comment 43)  My comments there (and in comment 53) still apply.

::: layout/tools/reftest/reftest.jsm
@@ +423,5 @@
>  
> +    try {
> +        prefs.setBoolPref("print.print_via_pdf_encoder", "skia-pdf");
> +        // We have to disable printing via parent or else silent prints are being ignored
> +        // and a dialog is being displayed.

s/are being/would be/
s/is being/would be/

@@ +424,5 @@
> +    try {
> +        prefs.setBoolPref("print.print_via_pdf_encoder", "skia-pdf");
> +        // We have to disable printing via parent or else silent prints are being ignored
> +        // and a dialog is being displayed.
> +        // See http://searchfox.org/mozilla-central/source/layout/printing/nsPrintEngine.cpp#617

This URL will become bogus as soon as any lines are added/removed before this point in nsPrintEngine.cpp.  Please use the "permalink" feature of searchfox/DXR so that the line doesn't migrate.

@@ +425,5 @@
> +        prefs.setBoolPref("print.print_via_pdf_encoder", "skia-pdf");
> +        // We have to disable printing via parent or else silent prints are being ignored
> +        // and a dialog is being displayed.
> +        // See http://searchfox.org/mozilla-central/source/layout/printing/nsPrintEngine.cpp#617
> +        prefs.setBoolPref("print.print_via_parent", false);   

EOL whitespace

::: testing/web-platform/tests/intersection-observer/timestamp.html
@@ +25,4 @@
>  var topWindowTimeBeforeCreatingIframe;
>  var topWindowTimeBeforeNotification;
>  var iframeWindowTimeBeforeNotification;
> +var timeSkew = ;

This intersection-observer change seems unrelated to this patch. Accidentally folded in?
Attachment #8894709 - Attachment is obsolete: true
Attachment #8894709 - Flags: review?(dholbert)
Comment on attachment 8895148 [details] [diff] [review]
Add support for (real) print reftests

Addressed latest review comments. Extracted renaming of test-async-print into separate patch. Added documentation.
Attachment #8895148 - Flags: review?(dholbert)
Turned out checking for MOZ_ENABLE_SKIA_PDF. Created a separate patch for this to make review easier. Should probably merged with the main patch before landing.
Attachment #8895573 - Flags: review?(dholbert)
Comment on attachment 8895147 [details] [diff] [review]
Rename test-async-print.html to test-async-paged.html

Review of attachment 8895147 [details] [diff] [review]:
-----------------------------------------------------------------

r=me on this renaming patch.
Attachment #8895147 - Flags: review+
Comment on attachment 8895573 [details] [diff] [review]
Skip tests if compiled without Skia PDF enabled

Review of attachment 8895573 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/reftests/printing/reftest.list
@@ +48,5 @@
> +skip-if(!skiaPdf) print test-number-of-pages.html test-number-of-pages.html
> +skip-if(!skiaPdf) fails print test-number-of-pages.html test-number-of-pages-noref.html
> +skip-if(!skiaPdf) print test-print-selection.html test-print-text-ref.html
> +skip-if(!skiaPdf) print test-print-range.html test-print-range-ref.html
> +skip-if(!skiaPdf) print test-async-print.html test-print-text-ref.html

If you like, you could also put the "print" tests in a dedicated directory, and put skip-if(!skiaPdf) before the "include" for that directory.

Like we do here, for example:
https://dxr.mozilla.org/mozilla-central/rev/4c5fbf49376351679dcc49f4cff26c3c2e055ccc/layout/reftests/reftest.list#410

Unless this "skip-if(!skiaPdf)" annotations are going away soon, I would probably recommend that, so that the annotations are actually readable... I'm envisioning
  skip-if(!skiaPdf) fails-if(something && somethingElse) print foo.html foo-ref.html
and it would be great to be able to remove some boilerplate there so that the actually-relevant parts of that line are easier to see.

Maybe you want to rename the existing layout/reftests/printing directory to "paged" (since I'm pretty sure they all use reftest-paged), and create a new directory called layout/reftests/print for your new "print" reftests?
Comment on attachment 8895148 [details] [diff] [review]
Add support for (real) print reftests

Review of attachment 8895148 [details] [diff] [review]:
-----------------------------------------------------------------

Partial review (I haven't thoroughly looked through the reftest-content.js and reftest.jsm changes, but I reviewed the rest):

::: layout/reftests/printing/print.css
@@ +1,4 @@
> +@page {
> +    margin: 0;
> +}
> +    

Fix EOL whitespace

::: layout/reftests/printing/reftest.list
@@ +45,5 @@
> +# detects them as differing & correctly handles the 'fails' annotation.
> +print test-text.html test-text.html
> +fails print test-text.html test-text-noref.html
> +print test-number-of-pages.html test-number-of-pages.html
> +fails print test-number-of-pages.html test-number-of-pages-noref.html

Could you also add a note to the comment above this section, to mention the fact that some of these tests are used as their own reference? (& hint at why)

I think you're just testing that we reliably produce the same output for a given file, or something...?  Anyway, it's worth mentioning, because it means these tests quite aren't checking what they might superficially seem to be checking (e.g. test-number-of-pages.html doesn't actually test how many pages we produce -- as far as I can tell, it'd still match our pass/fail expectations even if we printed all content on a single page, or on any arbitrary number of pages).

::: layout/reftests/printing/test-print-range.html
@@ +1,2 @@
> +<!DOCTYPE html>
> +<html reftest-print-range="2-3">

Could you include another version of this print-range testcase, with a range of exactly 1 page? (to test that edge case in the harness, and to demonstrate how that is done for the benefit of people writing reftest-print-range tests in the future)

(From looking at reftest-content.js, I think you have to do it as reftest-print-range="2-2" to get just page 2, right?)

::: layout/tools/reftest/README.txt
@@ +284,5 @@
> +              - The number of pages generated for both printouts must match.
> +              - The text content of both printouts must match (rasterized text
> +                does not match real text).
> +
> +             You can specify the a print range by setting the reftest-print-range

s/the a/a/

@@ +287,5 @@
> +
> +             You can specify the a print range by setting the reftest-print-range
> +             attribute on the document element. Example:
> +
> +              <html reftest-print-range="2-3">

You should also include an example here of how to get a 1-page-long page range. (I think 2-2, right?)

@@ +293,5 @@
> +
> +             You can also print selected elements only with the
> +             reftest-print-selection attribute:
> +
> +              <html reftest-print-range="2-3">

copypaste typo here -- you meant to use "reftest-print-selection"

@@ +306,5 @@
> +              - Testing (fuzzy) position of elements
> +              - Testing specific print related CSS properties
> +              - ...
> +
> +             Main difference to == and != is that this is comparing the structure

This isn't quite grammatically valid.

Please reword to something like:
"The main difference between 'print' and '=='/'!=' reftests is that 'print' makes us compare the structure of print results [...]

@@ +309,5 @@
> +
> +             Main difference to == and != is that this is comparing the structure
> +             of print results (by parsing the output PDF) rather than taking screenshots
> +             and comparing pixel values. This allows us to test for common printing
> +             related issues like text being rasterized where it shouldn't.

Please add at the end here:
  "This difference in behavior is also why this is its own reftest operator, rather than a flavor of ==/!=. It would be somewhat misleading to list these print reftests as ==/!=, because they don't actually check for pixel matching."

(This answers dbaron's comment 26-27, I think, and it answers it more directly than your paragraph further down does.)

@@ +316,5 @@
> +             layout in pagination mode.
> +
> +             A new type was introduced (rather than as an attribute on the document
> +             element) to enforce both HTML files to be set in printing mode. This
> +             way the author of a test isn't responsible for making sure both are

This last paragraph ("A new type was introduced..." -- expressing the motivation) should just be removed, I think.

Its stated motivation (about not wanting test-authors to have to remember to do something twice) is not particularly convincing.  That is not a good enough motivation for adding complexity to the reftest harness/manifest, and plus, we already have that same "problem" (authors needing to do something twice) for "reftest-paged", and that's worked out fine. (It's easy enough to remember to add the class for both files, because the test trivially fails if you forget, I think. :))

I think the *real* motivation for creating a new reftest operator here is that you're doing something fundamentally different from  "=="/"!=".  So I think it makes more sense to explain *that* motivation, and to do it as an afterthought to your earlier paragraph that already compares to ==/!=.

::: layout/tools/reftest/reftest.jsm
@@ +2311,5 @@
>      gBrowserMessageManager.sendAsyncMessage("reftest:ResetRenderingState");
>  }
> +
> +function readPdf(path, callback) {
> +    logger.warning("debug: " + typeof PDFJS);

Is this warning useful?  Looks like it was maybe a temporary debugging thing you added & can now remove -- but if you want to keep it around, please make it a bit clearer.
Comment on attachment 8895148 [details] [diff] [review]
Add support for (real) print reftests

Review of attachment 8895148 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/tools/reftest/reftest-content.js
@@ +26,5 @@
> +CU.import('resource://gre/modules/XPCOMUtils.jsm');
> +
> +XPCOMUtils.defineLazyGetter(this, "ContentAreaUtils", function() {
> +    let ContentAreaUtils = {};
> +    Services.scriptloader.loadSubScript("chrome://global/content/contentAreaUtils.js", ContentAreaUtils);

This line is a bit too long.  (This file is mostly [though not entirely] nicely wrapped to 80 columns.)

It's still too long if you were to wrap after the comma, too -- so, perhaps shorten by pulling out the URI string into a local var?  Like so:
    let scriptURI = "chrome://global/content/contentAreaUtils.js";
    Services.scriptloader.loadSubScript(scriptURI, ContentAreaUtils);

THOUGH, maybe you don't even need this lazy getter at all -- see comment below about fileName.

@@ +220,4 @@
>     docShell.contentViewer.setPageMode(true, ps);
>  }
>  
> +function printToPdf(cb) {

Could you include a comment here to indicate what "cb" represents?

(It seems to be a callback of some sort, but it's not used for quite a while.)

@@ +221,5 @@
>  }
>  
> +function printToPdf(cb) {
> +    var currentDoc = content.document;
> +    var printSelection = false;

Let's call this "isPrintSelection", to make it more clearly a bool.

(Right now, this variable name sounds like it could be an object representing "the print selection", i.e. the selection to be printed.)

@@ +227,5 @@
> +
> +    if (currentDoc) {
> +        var contentRootElement = currentDoc.documentElement;
> +        printSelection = contentRootElement.hasAttribute("reftest-print-selection");
> +        printRange = contentRootElement.getAttribute("reftest-print-range") || '';

I'd prefer we avoid introducing two new mutually-exclusive attributes here -- instead, we just use reftest-print-range="selection", and check for that as an exact string match here (before we try to parse reftest-print-range with a "number-number" regex)?

Benefits of this representation:
 - It would match the actual print UI a bit more closely, where "Selection" is one of the options *inside of* a section called "range".
 - It would match the underlying implementation a bit more closely (where both categories are flavors of ps.printRange)
 - It simplifies some unnecessary over-expressiveness in the current implementation. (right now it's possible, though invalid, to specify both attributes simultaneously)
 - It would remove the need to have an attribute-whose-value-we-ignore ("reftest-print-selection"), which feels kind of like a wart in the current patch.

@@ +244,5 @@
> +    var ioService = CC[IO_SERVICE_CONTRACTID].getService(CI.nsIIOService);
> +    var currentURI = ioService.newURI(currentDoc.location.href, null, null);
> +    var fileName =
> +        ContentAreaUtils.getDefaultFileName(content.document.title, currentURI, null, null);
> +    fileName = fileName.trim() + ".pdf";

It looks like you're using the document's title as part of the PDF filename. Is that really necessary?  It's somewhat uncommon for reftests to have useful titles, so it's a bit odd to depend on them heavily here (and I expect that this will frequently be the empty string -- it looks like it will be for all of your included reftests, since none of them have <title> elements I think).

Do we even actually care what the filename is? If not, maybe just make fileName = "reftestPrint" or something like that, and disregard the document's title. (which means you can get rid of your ContentAreaUtils lazy-getter, I think)  That way someone who's really curious will be able to identify these files, while we're still leaving them as mostly an implementation detail.

(I assume file.createUnique handles adding some random junk to the filename to uniquify it, too.)

@@ +247,5 @@
> +        ContentAreaUtils.getDefaultFileName(content.document.title, currentURI, null, null);
> +    fileName = fileName.trim() + ".pdf";
> +    var file = Services.dirsvc.get("TmpD", CI.nsIFile);
> +    file.append(fileName);
> +    file.createUnique(file.NORMAL_FILE_TYPE, parseInt("666", 8));

Let's use the numeric value 0o644 as the second arg here. Because:

(1) type-wise, it seems silly/wasteful to spin our wheels on string-parsing every time we get here, just to come up with a numeric octal code.  I found some other usages of this createUnique API that use hardcoded numeric octal values (with 0o prefix) which are just as readable and don't require parsing - seems better to go with that.

(2) 666 is unnecessarily permissive  (It gives other users write access to this file, which seems like it opens up needless opportunity for mischief / mysterious-test-failure, particularly in a globally-accessable directory like /tmp.)  It looks like some other createUnique calls use "FileUtils.PERMS_FILE" as their permissions, which is a constant equal to 0o644.  That gives read access to everyone but write access only to the current user. That seems more appropriate to me - it's gracefully permissive, but only allows as much write access as we know is necessary.  (You could reference this value via its FileUtils constant, but it's probably not worth it because that requires an XPCOMUtils lazy getter; so, hardcoded numeric value 0o644 seems fine.)

@@ +1052,1 @@
>          // Script tests or load-only tests do not need any snapshotting

This comment needs an update.

Previously it was inside a check for TYPE_SCRIPT || TYPE_LOAD, now it's also || TYPE_PRINT. It should probably say something like:
        // Script, load-only, and PDF-print tests do not need any snapshotting.

@@ +1080,5 @@
>          function (m) { RecvLoadScriptTest(m.json.uri, m.json.timeout); }
>      );
>      addMessageListener(
> +        "reftest:LoadPrintTest",
> +        function (m) { RecvLoadPrintTest(m.json.uri, m.json.timeout, m.json.refPDF); }

Are you actually using m.json.refPDF here?  I think that arg is bogus & needs to be dropped. Because:

 (1) The function you're calling, RecvLoadPrintTest (defined a few lines down), only takes 2 args -- it's defined as "function RecvLoadPrintTest(uri, timeout)".  So it looks like it drops that 3rd arg on the floor right now.

 (2) The function that calls into this code, SendLoadPrintTest (over in reftest.jsm), only seems to send |uri| and |timeout| -- no |refPDF|.  So I think this "refPDF" is undefined here anyway.

::: layout/tools/reftest/reftest.jsm
@@ +32,4 @@
>            "@mozilla.org/file/directory_service;1";
>  const NS_OBSERVER_SERVICE_CONTRACTID =
>            "@mozilla.org/observer-service;1";
> +const PRINTSETTINGS_CONTRACTID = "@mozilla.org/gfx/printsettings-service;1";

This PRINTSETTINGS_CONTRACTID isn't used at all in this file -- it's only used in reftest-content.js.  Maybe define it over there?  (Unless there's a reason for it to be here.)

@@ +150,4 @@
>  const TYPE_LOAD = 'load';     // test without a reference (just test that it does
>                                // not assert, crash, hang, or leak)
>  const TYPE_SCRIPT = 'script'; // test contains individual test results
> +const TYPE_PRINT = 'print';

Please copy over your code-comment from reftest-content.js, to keep this chunk in sync between the two files.

@@ +421,5 @@
>      } catch(e) {}
>  #endif
>  
> +    try {
> +        prefs.setBoolPref("print.print_via_pdf_encoder", "skia-pdf");

Could you add a comment explaining why we need to hardcode the PDF encoder like this?  (What happens if we don't set this pref? Do we just generate less-predictable PDF output?)

@@ +426,5 @@
> +        // We have to disable printing via parent or else silent prints would be ignored
> +        // and a dialog would be displayed.
> +        // See http://searchfox.org/mozilla-central/rev/bd39b6170f04afeefc751a23bb04e18bbd10352b/layout/printing/nsPrintEngine.cpp#617
> +        prefs.setBoolPref("print.print_via_parent", false);
> +    } catch (e) {}

Can we really proceed usefully (with print reftests) if these setBoolPref calls fail?  I suspect we can't...

W should probably put something (either a throw/return of some sort, or just a "/* uh oh, print reftests may not work... */") in the catch {} block -- OR, put these pref assignments in https://dxr.mozilla.org/mozilla-central/source/testing/profiles/prefs_general.js , so that we don't have to worry about setting them here.

@@ +1686,5 @@
>      var output;
>      var extra;
>  
> +    switch (gURLs[0].type) {
> +    case TYPE_LOAD:

If you want to convert this to be a switch statement, could you do that in its own changeset, *and* create a "case" for TYPE_EQUAL/NOTEQUAL as well?  (I don't care whether it's before or after the main patch here -- whatever's easier.)

Right now in current mozilla-central, this is structured as:
  if (...TYPE_SCRIPT) {
    ... return;
  }
  if (...TYPE_LOAD) {
   ...return;
  }
  // Code to handle TYPE_REFTEST_EQUAL & TYPE_REFTEST_NOTEQUAL

Right now you're porting just those "if" checks, but not the code at the end.  For a legit 'switch' conversion, you should convert that code at the end, too, if I'm understanding correctly. Perhaps by folding it into a shared "case" chunk like so:
  case TYPE_REFTEST_EQUAL:
  case TYPE_REFTEST_NOTEQUAL: {
     // Code to handle TYPE_REFTEST_EQUAL & TYPE_REFTEST_NOTEQUAL
  };

That way the switch statement would actually be a clear breakdown of the possibilities.

@@ +1726,5 @@
> +            return;
> +        default:
> +            throw "Unexpected state.";
> +        }
> +        break;

This "break" (at the end of "case TYPE_PRINT") needs to be removed or replaced.

Right now, it's problematic for two reasons:
 (1) It's unreachable, since we return or throw before we reach it.
 (2) Even if it were reachable: it currently drops us down into the code for handling TYPE_REFTEST_EQUAL/NOTEQUAL (I think that's what the code after TYPE_SCRIPT is all about), which would be likely to produce unexpected behavior given that we're really TYPE_PRINT.

@@ +2337,5 @@
> +            readPdf(path, function(error, pdf) {
> +                if (error) {
> +                    reject(error);
> +                } else {
> +                    resolve(pdf);

Could you include a comment to explain this chunk (the start of the function up through this point)? The levels of bracing & nesting/abstraction/callback are a bit unintuitive, unless perhaps you've got recent Promise experience (which I do not).

@@ +2357,5 @@
> +            for (var i = 0; i < numberOfPages; i++) {
> +                var pageNum = i + 1;
> +                var promise1 = pdfs[0].getPage(pageNum);
> +                var promise2 = pdfs[1].getPage(pageNum);
> +                promises.push(new Promise(function(resolve, reject) {

Could you add some documentation for what's going on here?  My head spins a bit from the distinction between...
 - "promises" (an array which already has 1 entry when we get here)
 - promise1, promise2 (which sound like they would be array entries in promises, but they are not)
 - this new Promise that we're appending to the promises array here (which uses promise1 and promise2 somehow but is its own different promise)
 - textContent1 and textContent2 (defined below, also seem to be promises)
Pushed by tschneider@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6e7dd648c6b7
Rename test-async-print.html to test-async-paged.html. r=dholbert
https://hg.mozilla.org/mozilla-central/rev/6e7dd648c6b7
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment on attachment 8895148 [details] [diff] [review]
Add support for (real) print reftests

Review of attachment 8895148 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/tools/reftest/reftest.jsm
@@ +1697,5 @@
> +    case TYPE_PRINT:
> +        switch (gState) {
> +        case 1:
> +            // First document has been loaded.
> +            gTestPrintOutput = scriptResults;

This "scriptResults" arg should be renamed. The only reason it has "script" in its name is because, right now, it is *purely* used for TYPE_SCRIPT. But this patch is co-opting it for another type, so we should change its name.

Maybe call it "typeSpecificResults"?  (to indicate that its contents are TYPE_***-specific and that it could be possibly-empty)

@@ +1703,5 @@
> +            CleanUpCrashDumpFiles();
> +            StartCurrentURI(2);
> +            return;
> +        case 2:
> +            comparePdfs(gTestPrintOutput, scriptResults, function(error, results) {

"comparePdfs(gTestPrintOutput, scriptResults, ...)" is a bit confusing here (even if scriptResults gets renamed to something more generic).  Could you create a local variable alias for the reference PDF, to make it clearer what you're comparing here? Something like:
  let referencePrintOutput = scriptResults;
  comparePdfs(gTestPrintOutput, referencePrintOutput, ...)

@@ +1711,5 @@
> +                    ++gTestResults[output.n];
> +                    logger.testEnd(gURLs[0].identifier, output.s[0], output.s[1],
> +                                   error.message, null, extra);
> +                } else {
> +                    var outputPair = outputs[EXPECTED_PASS];

Two things:
 (1) Why are you hardcoding EXPECTED_PASS, rather than using gURLs[0].expected like we do for other TEST_*** types?  (How do "fails" expected-fail print reftests get their logging printed correctly, if we're hardcoding EXPECTED_PASS here? I admit I don't fully know how this is supposed to work, but I would expect they'd use outputs[EXPECTED_FAIL] or something, and I thought gURLs[0].expected is supposed to tell us that.)

 (2) Here and elsewhere in this patch, you should probably be preferring "let" rather than "var", for better scoping & to avoid subtle bugs & leakage (particularly in huge functions like this one where scoping is a bigger deal). I tend to think this whole patch should be using "let" (which I think you could do using a search-and-replace regex on the patch file itself, to fix it across the board, for all lines that start with "+" and then have space and then "var" and then space).  Though if you'd prefer, you could also file a followup to do a whole-file conversion (though ideally we should fix that followup soon to reduce the likelihood that this patch is introducing a subtle bug from variable scope leakage).

@@ +2367,5 @@
> +                            var text2;
> +                            var passed = texts[0].items.every(function(o, i) {
> +                                text1 = o.str;
> +                                text2 = texts[1].items[i].str;
> +                                return text1 === text2;

Can you add some documentation to make it clearer what the distinction is between "textContent1/2", "texts", and "text1/2"?  (Maybe also rename "texts" or "text1/2", because I would fully expect text1 to be an array-entry in "texts", but it is not. Though with good documentation, maybe the names are OK.)

Also: I think this comparison isn't quite complete.  You're iterating across all of texts[0].items and checking for a match in texts[1], I think -- but what if texts[1] has some trailing junk at the end?  I think this won't catch that, because we stop when we get to the end of texts[0].items.  So you probably need to compare texts[0].items.length against texts[1].items.length or something like that, right?

If possible, please add a reftest that would've caught this -- i.e. a reftest that is expected to fail (and is marked as "fails" in the manifest) but that would've incorrectly been reported as passing with the current patch because we don't check the extra text at the end of texts[1].

@@ +2373,5 @@
> +                            resolve({
> +                                passed: passed,
> +                                description: passed ?
> +                                    "Page " + pages[0].pageNumber + " contains same text" :
> +                                    "Expected page " + pages[0].pageNumber + " to contain text '" + text2 + "'"

Might be worth including
  "but found '" + text1 + "' instead."
at the end of this description.

Also: this error message (or some error message) will also need to handle the case where texts[0] and texts[1] have different length "items" arrays.
> Maybe you want to rename the existing layout/reftests/printing directory to
> "paged" (since I'm pretty sure they all use reftest-paged), and create a new
> directory called layout/reftests/print for your new "print" reftests?

We actually already have a layout/reftests/pagination that contains reftest-paged tests. I think it makes sense to move all existing tests in layout/reftests/printing there.
Moving all current tests in layout/reftests/printing to ayout/reftests/pagination.
Addressed latest review comments. Except changing var to let, which I will do in a follow-up patch as discussed on IRC.
Attachment #8895148 - Attachment is obsolete: true
Attachment #8895573 - Attachment is obsolete: true
Attachment #8895148 - Flags: review?(dholbert)
Attachment #8895573 - Flags: review?(dholbert)
Attachment #8899182 - Flags: review?(dholbert)
Attachment #8899183 - Flags: review?(dholbert)
Attachment #8899183 - Attachment is obsolete: true
Attachment #8899183 - Flags: review?(dholbert)
Attachment #8899187 - Flags: review?(dholbert)
Sorry, uploaded wrong patch earlier.
Attachment #8899187 - Attachment is obsolete: true
Attachment #8899187 - Flags: review?(dholbert)
Attachment #8899190 - Flags: review?(dholbert)
We have another regression in PDF.js this patch depends on. It was introduced in https://hg.mozilla.org/mozilla-central/rev/eaa5c44cfd97. Currently talking to the PDF.js team to help getting this fixed.
Comment on attachment 8899182 [details] [diff] [review]
Move all reftest-paged test to layout/reftests/pagination

Review of attachment 8899182 [details] [diff] [review]:
-----------------------------------------------------------------

r=me, though one request:

::: layout/reftests/printing/reftest.list
@@ +1,2 @@
>  # Sanity check
>  == blank.html blank.html

This patch is leaving "blank.html" behind in this directory (along with this line of reftest.list) -- but I think it should move as well.  Could you move it?

(blank.html is a "reftest-paged" test, not one of the new-style reftest-print tests.)

So I think this patch should leave layout/reftests/printing/reftest.list as a file with just a single blank line, and no other files in the same directory -- and then the next patch will repopulate it with list-entries.
Attachment #8899182 - Flags: review?(dholbert) → review+
Removing layout/reftests/printing/blank.html. Same test already exists in layout/reftests/pagination.
Attachment #8899182 - Attachment is obsolete: true
Comment on attachment 8899190 [details] [diff] [review]
Add support for (real) print reftests

Review of attachment 8899190 [details] [diff] [review]:
-----------------------------------------------------------------

I think this is r+-with-nits, but this probably merits one last look-over, so I'm marking as r- for now and I'll take a quick look at the final version once you've addressed this feedback.

::: layout/reftests/printing/reftest.list
@@ +5,5 @@
> +# testing the integrity of the reftest harness. Their testcases & reference
> +# case are known to differ, and we're testing that the harness correctly
> +# detects them as differing & correctly handles the 'fails' annotation.
> +# Some of these tests are used as their own reference to guarantee basic
> +# text and page number matching works as expected.

s/guarantee/verify/  (or "confirm" perhaps?)

("guarantee" sounds like a higher bar than we can really promise here.  Consider a hypothetical trivially-broken rendering engine that only prints blank pages -- such an engine would "pass" these file-is-its-own-reference tests.  So, we can't take those specific tests' passing as a "guarantee" that our matching logic is correct.  It's more of a verification pass -- so a less-absolute word like "verify" or "confirm" seems more appropriate.)

@@ +12,5 @@
> +print test-number-of-pages.html test-number-of-pages.html
> +fails print test-number-of-pages.html test-number-of-pages-noref.html
> +print test-print-selection.html test-text-ref.html
> +print test-print-range.html test-print-range-ref.html
> +fails print test-print-single-page.html test-print-single-page-ref.html

"test-print-single-page-ref.html" should really have a "noref" suffix (instead of a "ref" suffix), right?  Since the expectation is that it does *not* match?

::: layout/reftests/printing/test-unexpected-text.html
@@ +6,5 @@
> +</head>
> +<body>
> +  <p>
> +      <span>This text should appear on page 1</span>
> +      <b>But this text should not</b>

excessive indentation on these last 2 lines -- deindent by 2 spaces.

::: layout/reftests/reftest.list
@@ +296,4 @@
>  include position-dynamic-changes/reftest.list
>  
>  # printing
> +skip-if(!skiaPdf) include printing/reftest.list

Please include a code-comment just before this line, explaining why the "skip-if(!skiaPDF)" is here.

::: layout/tools/reftest/README.txt
@@ +278,5 @@
>  
>               url_ref must be omitted. The test may be marked as fails or
>               random. (Used to test the JavaScript Engine.)
> +      print  The test passes if the printouts (as PDF) of the two renderings
> +             are the SAME by applying the following tests:

s/tests/comparisons/

(to reduce ambiguity, since the files themselves are called "tests")

@@ +302,5 @@
> +              ...
> +
> +             Make sure to include code in your test that actually selects something.
> +
> +             Future additions to the set of tests might include:

As above:
 s/set of tests/set of comparisons/

(Otherwise this sounds like you're talking about "the set of reftest files".)

@@ +314,5 @@
> +             The main difference between 'print' and '=='/'!=' reftests is that
> +             'print' makes us compare the structure of print results (by parsing
> +             the output PDF) rather than taking screenshots and comparing pixel
> +             values. This allows us to test for common printing related issues
> +             like text being rasterized where it shouldn't. This difference in

s/where/when/

(The current language, "test for...text being rasterized where it shouldn't", sounds like we're inspecting the *physical location on the page where the text is rasterized*. But that's not what you meant to say.)

::: layout/tools/reftest/reftest.jsm
@@ +423,5 @@
>  
> +#ifdef MOZ_ENABLE_SKIA_PDF
> +    try {
> +        // We have to disable printing via parent or else silent prints would be ignored
> +        // and a dialog would be displayed.

The first line here (about "silent prints") is too long -- please rewrap.

Also - please elaborate slightly to hint a bit more clearly about what "silent print" means, and why we care about them being ignored.

e.g. maybe you mean to say something like: "...or else silent print operations (the type that we use here) would be treated as non-silent -- in other words, a print dialog would appear for each print operation, which would interrupt the test run"

@@ +1695,4 @@
>      var output;
>      var extra;
>  
> +     if (gURLs[0].type == TYPE_LOAD) {

Revert the change to this line. (The patch just adds an extra space of indentation here -- looks like that was just a mistake.)

@@ +2381,5 @@
> +                            var testText;
> +                            var refText;
> +                            var passed = refTextItems.every(function(o, i) {
> +                                testText = o.str;
> +                                refText = testTextItems[i].str;

At this point, we don't even know whether testTextItems[i] exists, do we? We haven't yet tested the length of testTextItems. So we don't know if it has an i'th entry.

(We have a do-lengths-match test further down, inside "if (passed)", but it seems like we should do that check up here before we've even bothered to test the text.)

@@ +2390,5 @@
> +                                if (testTextItems.length > refTextItems.length) {
> +                                    passed = false;
> +                                    description = "Page " + pages[0].pageNumber +
> +                                        " contains unexpected text like '" +
> +                                        testTextItems[refTextItems.length].str + "'";

Let's get rid of this special-case description -- it makes the incorrect assumption that, when there's a length mismatch, the unexpected text is always at the end.  But really the unexpected text might be at the beginning somewhere or in the middle (and in that case, this failure-description would produce confusing output for someone debugging the output.

Probably simplest & clearest to just share your one main failure description for all mismatch cases (printing out refText and testText in their entirety).
Attachment #8899190 - Flags: review?(dholbert) → review-
Fixed typo.
Attachment #8899190 - Attachment is obsolete: true
Attachment #8899627 - Flags: review?(dholbert)
Comment on attachment 8899627 [details] [diff] [review]
Add support for (real) print reftests

Review of attachment 8899627 [details] [diff] [review]:
-----------------------------------------------------------------

Ah, thanks for fixing that fail/fails typo.

This led me to look a bit closer at the testcase in question (test-unexpected-text.html), and I have a few notes on that.  Carrying forward r- (but probably r+ after a final look once you've addressed all this feedback)

::: layout/reftests/printing/reftest.list
@@ +14,5 @@
> +print test-print-selection.html test-text-ref.html
> +print test-print-range.html test-print-range-ref.html
> +fails print test-print-single-page.html test-print-single-page-ref.html
> +print test-async-print.html test-text-ref.html
> +fails print test-unexpected-text.html test-text-ref.html

Ideally, it'd be great if *all* the "expected-failure" print reftests could have a reference case with "noref" suffix instead of "ref" suffix, just to drive home the fact that these really are the equivalent of "!=" reftests, rather than real failures.

So: could you create a custom "test-unexpected-text-noref.html" non-reference file to use on this last line here?  See my notes below on the test itself for what I think it should contain.

::: layout/reftests/printing/test-unexpected-text.html
@@ +6,5 @@
> +</head>
> +<body>
> +  <p>
> +      <span>This text should appear on page 1</span>
> +      <b>But this text should not</b>

This is quite misleading/confusing. The text says it should not appear -- but it does appear! And really, it indeed *should* appear!

Perhaps replace the textual contents here with something like:
 <span>This text appears in both the testcase and non-reference case.</span>
 <b>But this text ONLY appears in the testcase.</b>

...and create a -noref file that's identical except without the <b>?
Attachment #8899627 - Flags: review?(dholbert) → review-
> At this point, we don't even know whether testTextItems[i] exists, do we? We
> haven't yet tested the length of testTextItems. So we don't know if it has
> an i'th entry.

Correct, we should return false if !testTextItems[i]. This covers the case where refTestItems.length > testTextItems.length, meaning every extra text found in refTestItems fail with "Expected page X to contain text Y.", but without the "... but found Z instead.".

> @@ +2390,5 @@
> > +                                if (testTextItems.length > refTextItems.length) {
> > +                                    passed = false;
> > +                                    description = "Page " + pages[0].pageNumber +
> > +                                        " contains unexpected text like '" +
> > +                                        testTextItems[refTextItems.length].str + "'";
> 
> Let's get rid of this special-case description -- it makes the incorrect
> assumption that, when there's a length mismatch, the unexpected text is
> always at the end.  But really the unexpected text might be at the beginning
> somewhere or in the middle (and in that case, this failure-description would
> produce confusing output for someone debugging the output.
> 
> Probably simplest & clearest to just share your one main failure description
> for all mismatch cases (printing out refText and testText in their entirety).

Well, this actually covers the second case, where testTextItems.length is > refTestItems.length, meaning every extra text found in testTestItems should fail "Page X contains unexpected text". Unexpected test at the beginning would be handled by the first case described above.

To sum this up we basically have (and should have) 3 cases to handle:

1. The reference contains text that can not be found in the test.
2. The reference contains text that is different to the one found in the test.
3. The test contains extra text that that can not be found in reference.
Currently we don't have a test covering the first case (The reference contains text that can not be found in the test). Will include one in the next patch revision.
Addressed latest round of review comments.
Attachment #8899627 - Attachment is obsolete: true
Attachment #8899684 - Flags: review?(dholbert)
Comment on attachment 8899684 [details] [diff] [review]
Add support for (real) print reftests

Review of attachment 8899684 [details] [diff] [review]:
-----------------------------------------------------------------

r=me, just one nit:

::: layout/tools/reftest/reftest.jsm
@@ +2404,5 @@
> +                            } else {
> +                                description = "Expected page " + pages[0].pageNumber +
> +                                    " to contain text '" + refText +
> +                                    (testText ? "' but found '" + testText +
> +                                        "' instead." : '.')

Nit: this description-string ends with a period (in both branches of its ternary condition), but the two previous "description = ..." assignments directly above it do not end with a period.

Please make that consistent (among all your |description| strings in these "passed:...description:..." structs), whichever way makes sense based on the surrounding logging-text when this message is displayed).
Attachment #8899684 - Flags: review?(dholbert) → review+
Small PDF.js compatibility fix. Also, as discussed on IRC, it now returns a single result object in case all tests passed to avoid "UNEXPECTED-TEST-PASS" fails.
Attachment #8899684 - Attachment is obsolete: true
Attachment #8900077 - Flags: review?(dholbert)
Depends on https://github.com/mozilla/pdf.js/pull/8812 and https://github.com/mozilla/pdf.js/pull/8813 being merged into m-c before landing this patches.
Comment on attachment 8900077 [details] [diff] [review]
Add support for (real) print reftests

Review of attachment 8900077 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with a comment added:

::: layout/tools/reftest/reftest.jsm
@@ +2421,5 @@
> +
> +        Promise.all(resultPromises).then(function (results) {
> +            var failed = results.filter(function (result) { return !result.passed });
> +            if (failed.length) {
> +                callback(null, failed);

Please include a comment here to explain why we do this slightly-awkward filtering dance (and return *only* a list of failures *or* a singleton success result).

(Superficially, it seems like we could just return the results list directly/unconditionally as your last patch did, and someone reading this code might be confused why we're not just doing that.)
Attachment #8900077 - Flags: review?(dholbert) → review+
Added comment as suggested in review comment.
Attachment #8900077 - Attachment is obsolete: true
Comment on attachment 8900447 [details] [diff] [review]
Add support for (real) print reftests. r=dholbert

Review of attachment 8900447 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/tools/reftest/reftest.jsm
@@ +2422,5 @@
> +        Promise.all(resultPromises).then(function (results) {
> +            // To avoid TEST-UNEXPECTED-PASS failures, we don't want to mix
> +            // passed and failed results. Instead we return a single passed
> +            // result in case ALL tests passed or ALL failed results if not.
> +            var failed = results.filter(function (result) { return !result.passed });

Sorry to nitpick, but this is still assuming a bit too much contextual knowledge.  (reader is left wondering: why would mixing passed and failed results cause TEST-UNEXPECTED-PASS failures?)

I was going to suggest that you reword this to be something like:
 // Note: you might think we could just directly call "callback(null, results)" here.  But, that'd be problematic for expected-fail print reftests testcases, because [...]

BUT, as I thought about it further, I wondered *why* it's actually problematic, and really it looks like it's entirely up to our callback (elsewhere in this patch) to decide how it wants to process the |results| that we pass it.  Moreover, our callback is in a position of knowing whether or not we're on an expected-failure testcase or not, so it seems like a much better place to have this hack (since it can apply it more tightly, and since it is also easier to see the connection to the logging that you want to avoid if the hack is up there & nearby the function which does that logging).

So: could you revert this to how it was before (just directly passing |results| through to the callback without filtering), and instead just add a special case in the callback which *only* gets triggered when we're on an expected-failure testcase? Something like in this pastebin (my suggested pseudocode-ish lines are the ones without the leading "+"): https://pastebin.mozilla.org/9030394
Depends on: 1393930
Flags: needinfo?(tschneider)
No longer depends on: 1393930
Also: logging-wise, if you think it really makes more sense to log a single "comparison passed" result most of the time [and maybe it does, to reduce repeated "same page count...same text...good" output], then that's probably fine (whether via post-processing on |results| in comparePdfs or in the callback).  The behavior just needs to be logically coherent and clearly documented.

(My concern with the most recent patch was that the logic felt like -- and was documented as -- a special-case hack, and that's what I want to avoid, when we have better options.  For example, we could instead just implement it as a later filter [in the callback] less-hackily, per comment 88, OR we just document comparePdf's behavior coherently such that this filtering is a natural part of how it works & what it passes to the callback.)
Agreed on dholbert's suggestion and changed the patch accordingly.
Attachment #8900447 - Attachment is obsolete: true
Comment on attachment 8902001 [details] [diff] [review]
Add support for (real) print reftests. r=dholbert

Review of attachment 8902001 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/tools/reftest/reftest.jsm
@@ +1728,5 @@
> +                    logger.testEnd(gURLs[0].identifier, output.s[0], output.s[1],
> +                                   error.message, null, extra);
> +                } else {
> +                    var outputPair = outputs[expected];
> +                    if (EXPECTED_FAIL) {

I had "if (EXPECTED_FAIL)" in my pastebin pseudocode, but I don't think it's an actual thing you can filter on here.

I think you need to check "expected == EXPECTED_FAIL", or something like that?

@@ +2434,5 @@
> +        }
> +
> +        Promise.all(resultPromises).then(function (results) {
> +            var failed = results.filter(function (result) { return !result.passed });
> +            if (failed.length) {

You're filtering inside the callback now, so I don't think you want this results.filter and length-check here (before you call the callback) anymore...
Attachment #8902001 - Flags: review-
Oh, I guess I really had $IS_EXPECTED_FAILURE in my pseudocode. Anyway, "if (EXPECTED_FAIL)" is just checking a global-constant value's truthiness and seems definitely wrong.
> I had "if (EXPECTED_FAIL)" in my pastebin pseudocode, but I don't think it's
> an actual thing you can filter on here.

Right, this should be expected === EXPECTED_FAIL.

> You're filtering inside the callback now, so I don't think you want this
> results.filter and length-check here (before you call the callback)
> anymore...

We dont do any filtering in comparePDF anymore. So we should still filter here to get failures only and fall through if there are no to return the passing once and trigger a UNEXPECTED_TEST_PASS if needed.
(In reply to Tobias Schneider [:tobytailor] from comment #93)
> We dont do any filtering in comparePDF anymore

Are you sure? The code that I quoted there (with the seemingly-redundant filtering) *is* in comparePdfs().  That's why I brought it up -- because I thought that was going away.
Indeed the previous patch didn't revert the result filtering in comparePdfs directly. Fixed in this revision.
Attachment #8902001 - Attachment is obsolete: true
Fix EXPECTED_FAIL test and typo.
Attachment #8902481 - Attachment is obsolete: true
Pushed by tschneider@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1271f11e57e7
Move all reftest-paged tests to layout/reftests/pagination. r=dholbert
https://hg.mozilla.org/mozilla-central/rev/1271f11e57e7
Status: REOPENED → RESOLVED
Closed: 2 years ago2 years ago
Resolution: --- → FIXED
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Depends on: 1396432
Daniel, as discussed on IRC, I changed the patch to only enable printing tests on Mac OS X for now, since this is the only platform that prints via Skia PDF for now. It now also makes sure to flush all writes to the output PDF before reading via PDF.js. Furthermore I decided to not include removing the include of AsyncSpellCheckTestHelper, since I'm not a 100% sure about it's side effects and and not related to testing printing at all. If it's really dead code, we should remove it in a separate cleanup patch
Attachment #8902489 - Attachment is obsolete: true
Attachment #8904800 - Flags: review?(dholbert)
Printing tests should be skipped if styloVsGecko (instead of stylo) is set, since it forces all reftest to run in pixel comparison (==) mode.
Attachment #8904800 - Attachment is obsolete: true
Attachment #8904800 - Flags: review?(dholbert)
Attachment #8904867 - Flags: review?(dholbert)
Comment on attachment 8904867 [details] [diff] [review]
Add support for (real) print reftests

Review of attachment 8904867 [details] [diff] [review]:
-----------------------------------------------------------------

Commit message nit:
> Bug 1299848 - Add support for (real) print reftests. r=dholbert. try: -b do -p all -u all -t none

Remember to drop the try syntax before landing.

r=me
Attachment #8904867 - Flags: review?(dholbert) → review+
Dropped try syntax from patch.
Attachment #8904867 - Attachment is obsolete: true
Attachment #8905121 - Flags: review?(dholbert)
Pushed by tschneider@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/74069a258786
Add support for (real) print reftests. r=dholbert
Comment on attachment 8905121 [details] [diff] [review]
Bug 1299848 - Add support for (real) print reftests. r=dholbert

I think you spawned this review request by accident? This looks the same as the previous r+'d verison except for the try-syntax commit message tweak. Also, it looks like this landed. (woohoo!)

Anyway, r+. :)
Attachment #8905121 - Flags: review?(dholbert) → review+
https://hg.mozilla.org/mozilla-central/rev/74069a258786
Status: REOPENED → RESOLVED
Closed: 2 years ago2 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.