Bug 84246 – [Qt] add LayoutTestController::setPrinting support to Qt unit tests

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Milian Wolff]

Created attachment 137699 [details]
patch that makes the Qt port properly handle setPrinting when creating pixel tests

The Qt port currently has no support for unit tests that rely on LayoutTestController::setPrinting. This bug report should track the effort to add the required bits and pieces.

The attached patch adds handling of LayoutTestController::isPrinting to DumpRenderTreeQt::dump such that it creates PNG files analogous to the other ports. Furthermore a few tests are removed from the Skipped list and their expected baselines are added.

Known Issues:

1) the setPrinting.html test is still Skipped, but when you run it manually with the patch above applied, you get strange results: 
http://imagebin.org/index.php?mode=image&id=207871
- the box is one pixel too high (compare to another platform's expected PNG), notable by the lack of the white 1px line between the blue and green one
- the right border of the box is not painted at all

2) PixelTests need to be run by hand, i.e.:
./Tools/Scripts/run-webkit-tests --print 'everything' -v -p LayoutTests/printing/setPrinting.html

If you'd run it on the whole printing folder, it would also create png-images of test cases that do not expect any image (like e.g. printing/page-break-widows.html). I'll look into this in another bug report.

3) I was unsure on where to add the images to, by default they where created in platform/qt-linux but I hope that they should be the same on all platforms, thanks to the test fonts? Since I was not sure though, I left them in qt-linux for now. If I should move them to qt instead, please tell me.

Cheers

[Milian Wolff]

ah I forgot to mention: If I'd create a PDF-version of the test, instead of calling my DumpRenderTreeSupportQt::paintPagesWithBoundaries(mainFrame, image); in DumpRenderTree::dump, i.e. something like this:

    QPrinter printer;
    printer.setOutputFormat(QPrinter::PdfFormat);
    printer.setOutputFileName("test-dump-print.pdf");
    printer.setResolution(96);
    printer.setPaperSize(QSizeF(800, 600), QPrinter::DevicePixel);
    printer.setPageMargins(qreal(0), qreal(0), qreal(0), qreal(0), QPrinter::DevicePixel);
    mainFrame->print(&printer);

Then the created PDF looks "good", esp. for the setPrinting.html test (see known issue #1 above).

[Milian Wolff]

Created attachment 137876 [details]
patch that makes the Qt port properly handle setPrinting when creating pixel tests

fix style issues, add changelog changes

[Milian Wolff]

Bug 84327 contains the fix for the known issue 2)

[Simon Hausmann]

(In reply to comment #2)
> Created an attachment (id=137876) [details]
> patch that makes the Qt port properly handle setPrinting when creating pixel tests

You may want to mark the patch up for review by setting "r?". Marked up for review means that you want reviewers to look at this patch (as opposed to other attachments on the bug)

[Csaba Osztrogonác]

Please add layout test results into platform/qt directory instead of platform/qt-linux. But unfortunately NRWT handles Qt platform incorrectly, and you can't overload the target platform with it. You can generate results with ORWT simple: Tools/Scripts/old-run-webkit-tests --platform qt -p --reset-results --add-platform-exceptions <TESTS>

[Milian Wolff]

Created attachment 137884 [details]
patch that makes the Qt port properly handle setPrinting when creating pixel tests

- move new files to platform/qt/
- adapt changelogs with new bug title

[WebKit Review Bot]

Attachment 137884 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1
Tools/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
LayoutTests/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Source/WebKit/qt/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Total errors found: 3 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Csaba Osztrogonác]

Comment on attachment 137884 [details]
patch that makes the Qt port properly handle setPrinting when creating pixel tests

View in context: https://bugs.webkit.org/attachment.cgi?id=137884&action=review

> LayoutTests/ChangeLog:3
> +        [Qt] add LayoutTestController::setPrinting support to Qt unit tests

Add "https://bugs.webkit.org/show_bug.cgi?id=84246" under the title and stle queue will be happy.

[Milian Wolff]

Created attachment 137894 [details]
patch that makes the Qt port properly handle setPrinting when creating pixel tests

add bug url to changelog entries

[Simon Hausmann]

Comment on attachment 137894 [details]
patch that makes the Qt port properly handle setPrinting when creating pixel tests

View in context: https://bugs.webkit.org/attachment.cgi?id=137894&action=review

Looks good in general, but I think the function should be moved. More detailed comment below.

> Source/WebKit/qt/Api/qwebframe.h:229
> +    void paintPagesWithBoundaries(QImage &);

This function is only relevant for DRT, so I think it should be moved to DumpRenderTreeSupport (I'm sure we can get the WebCore::Frame pointer there, too :). Also I think it should return a QImage instead of taking a reference.

[Milian Wolff]

Created attachment 137902 [details]
patch that makes the Qt port properly handle setPrinting when creating pixel tests

- get rid of QWebFrame::paintPagesWithBoundaries, push implementation to DRTSupportQt
- return QImage instead of taking QImage& as argument in DRTSupportQt::paintPagesWithBoundaries

Thanks Simon, that really was badly done before.

[WebKit Review Bot]

Comment on attachment 137902 [details]
patch that makes the Qt port properly handle setPrinting when creating pixel tests

Rejecting attachment 137902 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
webkit-commit-queue/Source/WebKit/chromium/webkit --revision 133348 --non-interactive --force --accept theirs-conflict --ignore-externals' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
48>At revision 133348.

________ running '/usr/bin/python tools/clang/scripts/update.py --mac-only' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'

________ running '/usr/bin/python gyp_webkit' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
Updating webkit projects from gyp files...

Full output: http://queues.webkit.org/results/12511121

[Simon Hausmann]

(In reply to comment #12)
> (From update of attachment 137902 [details])
> Rejecting attachment 137902 [details] from commit-queue.
> 
> Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2
> 
> Last 500 characters of output:
> webkit-commit-queue/Source/WebKit/chromium/webkit --revision 133348 --non-interactive --force --accept theirs-conflict --ignore-externals' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
> 48>At revision 133348.
> 
> ________ running '/usr/bin/python tools/clang/scripts/update.py --mac-only' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
> 
> ________ running '/usr/bin/python gyp_webkit' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
> Updating webkit projects from gyp files...
> 
> Full output: http://queues.webkit.org/results/12511121

Looks like the patch did not apply:

"patching file Tools/DumpRenderTree/qt/DumpRenderTreeQt.cpp
Hunk #1 FAILED at 950.
1 out of 1 hunk FAILED -- saving rejects to file Tools/DumpRenderTree/qt/DumpRenderTreeQt.cpp.rej"

[Milian Wolff]

Created attachment 138796 [details]
patch that makes the Qt port properly handle setPrinting when creating pixel tests

rebase on current master, hope it applies properly now...

[WebKit Review Bot]

Comment on attachment 138796 [details]
patch that makes the Qt port properly handle setPrinting when creating pixel tests

Clearing flags on attachment: 138796

Committed r115210: <http://trac.webkit.org/changeset/115210>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.