Closed
Bug 1299848
Opened 8 years ago
Closed 7 years ago
Add the ability to test printed output
Categories
(Core :: Printing: Output, defect, P2)
Core
Printing: Output
Tracking
()
RESOLVED
FIXED
mozilla57
Tracking | Status | |
---|---|---|
firefox57 | --- | fixed |
People
(Reporter: jrmuizel, Assigned: tschneider)
References
(Blocks 3 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.
Updated•8 years ago
|
Priority: -- → P3
Comment 1•8 years ago
|
||
We can use class="reftest-print" [1] to constructed paginated layout reftest. [1] https://dxr.mozilla.org/mozilla-central/rev/7be6b348c431d69f96f0765af3a0c0a0fe56d4bf/layout/tools/reftest/README.txt#560
Assignee | ||
Comment 2•7 years ago
|
||
Assignee | ||
Comment 3•7 years ago
|
||
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → tschneider
Assignee | ||
Comment 4•7 years ago
|
||
Uses PDF.js to compare print output with reference HTML. Documentation is following soon.
Reporter | ||
Comment 5•7 years ago
|
||
Isn't there already a copy of pdf.js in the tree? It would be nice to not duplicate it.
Reporter | ||
Comment 6•7 years ago
|
||
(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.
Assignee | ||
Comment 7•7 years ago
|
||
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.
Assignee | ||
Comment 8•7 years ago
|
||
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.
Assignee | ||
Comment 9•7 years ago
|
||
Attachment #8837789 -
Attachment is obsolete: true
Attachment #8842512 -
Attachment is obsolete: true
Assignee | ||
Comment 10•7 years ago
|
||
Attachment #8837790 -
Attachment is obsolete: true
Assignee | ||
Comment 11•7 years ago
|
||
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.
Assignee | ||
Comment 12•7 years ago
|
||
Updated patch to load and use the pdf.js bundles that already ship with FF.
Attachment #8847758 -
Attachment is obsolete: true
Assignee | ||
Comment 13•7 years ago
|
||
Attachment #8847759 -
Attachment is obsolete: true
Assignee | ||
Comment 14•7 years ago
|
||
Patch for PDF.js bundles shipped with FF. These fixes will be upstreamed. Just adding them here if needed to test this bug.
Assignee | ||
Comment 15•7 years ago
|
||
Actually including the files this time :).
Attachment #8863498 -
Attachment is obsolete: true
Assignee | ||
Comment 16•7 years ago
|
||
Upstream PR for PDF.js patches: https://github.com/mozilla/pdf.js/pull/8361
Assignee | ||
Comment 17•7 years ago
|
||
Added support to test print-selection and print-range.
Attachment #8863497 -
Attachment is obsolete: true
Assignee | ||
Comment 18•7 years ago
|
||
Attachment #8866632 -
Attachment is obsolete: true
Assignee | ||
Comment 19•7 years ago
|
||
Comment on attachment 8863501 [details] [diff] [review] PDF.js patches PDF.js fixes already landed in m-c.
Attachment #8863501 -
Attachment is obsolete: true
Assignee | ||
Comment 20•7 years ago
|
||
Attachment #8863502 -
Attachment is obsolete: true
Reporter | ||
Comment 21•7 years ago
|
||
Do you have plans to support backends other than PDF? Hooking up EMF seems pretty important.
Flags: needinfo?(tschneider)
Assignee | ||
Comment 22•7 years ago
|
||
No concrete plans yet (tho I already looked into supporting PostScript), put definitely doable and will keep it in mind.
Flags: needinfo?(tschneider)
Assignee | ||
Updated•7 years ago
|
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)
Assignee | ||
Comment 24•7 years ago
|
||
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.
Assignee | ||
Comment 28•7 years ago
|
||
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.)
Assignee | ||
Comment 30•7 years ago
|
||
Yeah thats a good idea, will include that.
Assignee | ||
Comment 31•7 years ago
|
||
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
Assignee | ||
Comment 32•7 years ago
|
||
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].
Assignee | ||
Comment 33•7 years ago
|
||
Attachment #8871447 -
Attachment is obsolete: true
Assignee | ||
Comment 34•7 years ago
|
||
Attachment #8871525 -
Attachment is obsolete: true
Comment 35•7 years ago
|
||
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; }
Assignee | ||
Comment 36•7 years ago
|
||
Farmer, it was nice meeting you at the all hands and thanks for the patch. I'm going to update the patches accordingly.
Assignee | ||
Comment 38•7 years ago
|
||
Updated reftests.
Attachment #8866634 -
Attachment is obsolete: true
Updated•7 years ago
|
Priority: P3 → P2
Updated•7 years ago
|
Blocks: pdf-printing
Assignee | ||
Comment 41•7 years ago
|
||
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)
Assignee | ||
Updated•7 years ago
|
Attachment #8883708 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8883701 -
Flags: review?(dholbert)
Comment 43•7 years ago
|
||
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 44•7 years ago
|
||
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-
Comment 45•7 years ago
|
||
(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 46•7 years ago
|
||
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.
Comment 47•7 years ago
|
||
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 ?
Assignee | ||
Comment 48•7 years ago
|
||
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.
Assignee | ||
Comment 49•7 years ago
|
||
Addressed first batch of review comments
Attachment #8883701 -
Attachment is obsolete: true
Assignee | ||
Comment 50•7 years ago
|
||
Add comment to make clear that fail is used in manifest for testing only the harness itself.
Attachment #8883702 -
Attachment is obsolete: true
Assignee | ||
Comment 51•7 years ago
|
||
> ...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.
Assignee | ||
Comment 52•7 years ago
|
||
> (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: ... }.
Comment 53•7 years ago
|
||
(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?)
Assignee | ||
Comment 54•7 years ago
|
||
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
Assignee | ||
Comment 55•7 years ago
|
||
Depends on https://github.com/mozilla/pdf.js/pull/8761.
Assignee | ||
Comment 56•7 years ago
|
||
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
Assignee | ||
Updated•7 years ago
|
Attachment #8894709 -
Flags: review?(dholbert)
Comment 57•7 years ago
|
||
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?
Assignee | ||
Comment 58•7 years ago
|
||
Assignee | ||
Comment 59•7 years ago
|
||
Attachment #8894709 -
Attachment is obsolete: true
Attachment #8894709 -
Flags: review?(dholbert)
Assignee | ||
Comment 60•7 years ago
|
||
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)
Assignee | ||
Comment 61•7 years ago
|
||
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 62•7 years ago
|
||
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 63•7 years ago
|
||
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 64•7 years ago
|
||
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 65•7 years ago
|
||
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)
Comment 66•7 years ago
|
||
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
Comment 67•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/6e7dd648c6b7
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Updated•7 years ago
|
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 68•7 years ago
|
||
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.
Assignee | ||
Comment 69•7 years ago
|
||
> 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.
Assignee | ||
Comment 70•7 years ago
|
||
Moving all current tests in layout/reftests/printing to ayout/reftests/pagination.
Assignee | ||
Comment 71•7 years ago
|
||
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)
Assignee | ||
Updated•7 years ago
|
Attachment #8899182 -
Flags: review?(dholbert)
Assignee | ||
Updated•7 years ago
|
Attachment #8899183 -
Flags: review?(dholbert)
Assignee | ||
Comment 72•7 years ago
|
||
Attachment #8899183 -
Attachment is obsolete: true
Attachment #8899183 -
Flags: review?(dholbert)
Attachment #8899187 -
Flags: review?(dholbert)
Assignee | ||
Comment 73•7 years ago
|
||
Sorry, uploaded wrong patch earlier.
Attachment #8899187 -
Attachment is obsolete: true
Attachment #8899187 -
Flags: review?(dholbert)
Attachment #8899190 -
Flags: review?(dholbert)
Assignee | ||
Comment 74•7 years ago
|
||
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 75•7 years ago
|
||
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+
Assignee | ||
Comment 76•7 years ago
|
||
Removing layout/reftests/printing/blank.html. Same test already exists in layout/reftests/pagination.
Attachment #8899182 -
Attachment is obsolete: true
Comment 77•7 years ago
|
||
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-
Assignee | ||
Comment 78•7 years ago
|
||
Fixed typo.
Attachment #8899190 -
Attachment is obsolete: true
Attachment #8899627 -
Flags: review?(dholbert)
Comment 79•7 years ago
|
||
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-
Assignee | ||
Comment 80•7 years ago
|
||
> 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.
Assignee | ||
Comment 81•7 years ago
|
||
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.
Assignee | ||
Comment 82•7 years ago
|
||
Addressed latest round of review comments.
Attachment #8899627 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8899684 -
Flags: review?(dholbert)
Comment 83•7 years ago
|
||
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+
Assignee | ||
Comment 84•7 years ago
|
||
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)
Assignee | ||
Comment 85•7 years ago
|
||
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 86•7 years ago
|
||
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+
Assignee | ||
Comment 87•7 years ago
|
||
Added comment as suggested in review comment.
Attachment #8900077 -
Attachment is obsolete: true
Comment 88•7 years ago
|
||
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
Updated•7 years ago
|
Flags: needinfo?(tschneider)
Comment 89•7 years ago
|
||
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.)
Assignee | ||
Comment 90•7 years ago
|
||
Agreed on dholbert's suggestion and changed the patch accordingly.
Attachment #8900447 -
Attachment is obsolete: true
Comment 91•7 years ago
|
||
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-
Comment 92•7 years ago
|
||
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.
Assignee | ||
Comment 93•7 years ago
|
||
> 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.
Comment 94•7 years ago
|
||
(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.
Assignee | ||
Comment 95•7 years ago
|
||
Indeed the previous patch didn't revert the result filtering in comparePdfs directly. Fixed in this revision.
Attachment #8902001 -
Attachment is obsolete: true
Assignee | ||
Comment 96•7 years ago
|
||
Fix EXPECTED_FAIL test and typo.
Attachment #8902481 -
Attachment is obsolete: true
Assignee | ||
Comment 97•7 years ago
|
||
Try run for second patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=55d209b054998e1ab2b5f09a54fdaa46005939ea
Comment 98•7 years ago
|
||
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
Comment 99•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/1271f11e57e7
Status: REOPENED → RESOLVED
Closed: 7 years ago → 7 years ago
Resolution: --- → FIXED
Assignee | ||
Updated•7 years ago
|
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 100•7 years ago
|
||
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)
Assignee | ||
Comment 101•7 years ago
|
||
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)
Assignee | ||
Comment 102•7 years ago
|
||
Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2d51b2bfb377c0e60cb629dc713c3b537bbefac2
Comment 103•7 years ago
|
||
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+
Assignee | ||
Comment 104•7 years ago
|
||
Dropped try syntax from patch.
Attachment #8904867 -
Attachment is obsolete: true
Attachment #8905121 -
Flags: review?(dholbert)
Comment 105•7 years ago
|
||
Pushed by tschneider@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/74069a258786 Add support for (real) print reftests. r=dholbert
Comment 106•7 years ago
|
||
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+
Comment 107•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/74069a258786
Status: REOPENED → RESOLVED
Closed: 7 years ago → 7 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•