Bug 84246 - [Qt] add LayoutTestController::setPrinting support to Qt unit tests
Summary: [Qt] add LayoutTestController::setPrinting support to Qt unit tests
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P3 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 84327
Blocks:
  Show dependency treegraph
 
Reported: 2012-04-18 08:27 PDT by Milian Wolff
Modified: 2012-04-25 08:21 PDT (History)
6 users (show)

See Also:


Attachments
patch that makes the Qt port properly handle setPrinting when creating pixel tests (89.94 KB, patch)
2012-04-18 08:27 PDT, Milian Wolff
no flags Details | Formatted Diff | Diff
patch that makes the Qt port properly handle setPrinting when creating pixel tests (93.09 KB, patch)
2012-04-19 04:20 PDT, Milian Wolff
no flags Details | Formatted Diff | Diff
patch that makes the Qt port properly handle setPrinting when creating pixel tests (89.36 KB, patch)
2012-04-19 05:43 PDT, Milian Wolff
no flags Details | Formatted Diff | Diff
patch that makes the Qt port properly handle setPrinting when creating pixel tests (89.52 KB, patch)
2012-04-19 06:44 PDT, Milian Wolff
hausmann: review-
hausmann: commit-queue-
Details | Formatted Diff | Diff
patch that makes the Qt port properly handle setPrinting when creating pixel tests (88.51 KB, patch)
2012-04-19 07:55 PDT, Milian Wolff
hausmann: review+
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
patch that makes the Qt port properly handle setPrinting when creating pixel tests (88.54 KB, patch)
2012-04-25 05:58 PDT, Milian Wolff
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Milian Wolff 2012-04-18 08:27:21 PDT
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
Comment 1 Milian Wolff 2012-04-18 08:32:01 PDT
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).
Comment 2 Milian Wolff 2012-04-19 04:20:13 PDT
Created attachment 137876 [details]
patch that makes the Qt port properly handle setPrinting when creating pixel tests

fix style issues, add changelog changes
Comment 3 Milian Wolff 2012-04-19 04:21:02 PDT
Bug 84327 contains the fix for the known issue 2)
Comment 4 Simon Hausmann 2012-04-19 04:32:59 PDT
(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)
Comment 5 Csaba Osztrogonác 2012-04-19 04:34:50 PDT
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>
Comment 6 Milian Wolff 2012-04-19 05:43:28 PDT
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
Comment 7 WebKit Review Bot 2012-04-19 05:47:14 PDT
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.
Comment 8 Csaba Osztrogonác 2012-04-19 05:49:35 PDT
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.
Comment 9 Milian Wolff 2012-04-19 06:44:50 PDT
Created attachment 137894 [details]
patch that makes the Qt port properly handle setPrinting when creating pixel tests

add bug url to changelog entries
Comment 10 Simon Hausmann 2012-04-19 06:57:25 PDT
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.
Comment 11 Milian Wolff 2012-04-19 07:55:14 PDT
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.
Comment 12 WebKit Review Bot 2012-04-23 00:13:47 PDT
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
Comment 13 Simon Hausmann 2012-04-23 00:45:17 PDT
(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"
Comment 14 Milian Wolff 2012-04-25 05:58:50 PDT
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...
Comment 15 WebKit Review Bot 2012-04-25 08:21:29 PDT
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>
Comment 16 WebKit Review Bot 2012-04-25 08:21:39 PDT
All reviewed patches have been landed.  Closing bug.