Bug 84327 - [Qt] Make DRT not dump pixel results if test is calling layoutTestController.dumpAsText()
: [Qt] Make DRT not dump pixel results if test is calling layoutTestController....
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: Tools / Tests
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To: Nobody
:
Depends on:
Blocks: 84246
  Show dependency treegraph
 
Reported: 2012-04-19 04:11 PDT by Milian Wolff
Modified: 2012-04-23 00:10 PDT (History)
3 users (show)

See Also:


Attachments
don't dump pixels if shouldDumpAsText is true (1.67 KB, patch)
2012-04-19 04:19 PDT, Milian Wolff
ossy: review-
Details | Formatted Diff | Diff
don't dump pixels if shouldDumpAsText is true (1.62 KB, patch)
2012-04-19 05:44 PDT, Milian Wolff
no flags Details | Formatted Diff | Diff
don't dump pixels if shouldDumpAsText is true (1.66 KB, patch)
2012-04-19 06:41 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-19 04:11:51 PDT
The DRT-Qt port creates pixel dumps even for test cases where this is not expected. I.e.:

./Tools/Scripts/run-webkit-tests -p LayoutTests/printing/page-break-orphans.html

-> creates a PNG of the page and complains about missing baseline

According to Dirk Pranke (https://lists.webkit.org/pipermail/webkit-dev/2012-April/020349.html) pixel dumps should not be created when layoutTestController.dumpAsText() was called. The attached patch does that and fixes the problem mentioned above.
Comment 1 Milian Wolff 2012-04-19 04:19:56 PDT
Created attachment 137875 [details]
don't dump pixels if shouldDumpAsText is true

patch that checks m_controller->shouldDumpAsText() and prevents the pixel-dump when that is true
Comment 2 Csaba Osztrogonác 2012-04-19 04:30:06 PDT
Comment on attachment 137875 [details]
don't dump pixels if shouldDumpAsText is true

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

Only small nitpicks, otherwise LGTM. Please update the patch and upload it with r?

> Tools/ChangeLog:5
> +        Qt: don't dump pixels if test should be dumped as text
> +
> +        [Qt] don't dump pixels if test should be dumped as text

Could you add the title (the new one, please) only once to the changelog?

> Tools/DumpRenderTree/qt/DumpRenderTreeQt.cpp:951
>      // FIXME: All other ports don't dump pixels, if generatePixelResults is false.

Please remove this FIXME, we don't need anymore after your fix.
Comment 3 Milian Wolff 2012-04-19 05:44:41 PDT
Created attachment 137886 [details]
don't dump pixels if shouldDumpAsText is true

fix issues raised by Ossy
Comment 4 Csaba Osztrogonác 2012-04-19 05:51:24 PDT
Comment on attachment 137886 [details]
don't dump pixels if shouldDumpAsText is true

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

Something is wrong with Tools/DumpRenderTree/qt/DumpRenderTreeQt.cpp, because it can't be applied.

> Tools/ChangeLog:5
> +        [Qt] Make DRT not dump pixel results if test is calling layoutTestController.dumpAsText()
> +
> +        Reviewed by NOBODY (OOPS!).

[Qt] Make DRT not dump pixel results if test is calling layoutTestController.dumpAsText()
https://bugs.webkit.org/show_bug.cgi?id=84327

Reviewed by NOBODY (OOPS!).
Comment 5 Milian Wolff 2012-04-19 06:41:42 PDT
Created attachment 137893 [details]
don't dump pixels if shouldDumpAsText is true

- create patch that directly applies against master
- add bug url to changelog entry
Comment 6 Csaba Osztrogonác 2012-04-22 23:30:57 PDT
Comment on attachment 137893 [details]
don't dump pixels if shouldDumpAsText is true

LGTM, r=me.
Comment 7 WebKit Review Bot 2012-04-23 00:10:44 PDT
Comment on attachment 137893 [details]
don't dump pixels if shouldDumpAsText is true

Clearing flags on attachment: 137893

Committed r114875: <http://trac.webkit.org/changeset/114875>
Comment 8 WebKit Review Bot 2012-04-23 00:10:49 PDT
All reviewed patches have been landed.  Closing bug.