Bug 42374 - Add possibility to dumpAsText and generate a pixel test result
Summary: Add possibility to dumpAsText and generate a pixel test result
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nikolas Zimmermann
URL:
Keywords:
Depends on:
Blocks: 42376
  Show dependency treegraph
 
Reported: 2010-07-15 07:47 PDT by Nikolas Zimmermann
Modified: 2010-07-16 02:49 PDT (History)
3 users (show)

See Also:


Attachments
Patch (13.55 KB, patch)
2010-07-15 07:52 PDT, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Updated patch (11.91 KB, patch)
2010-07-16 01:05 PDT, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Updated patch v2 (11.90 KB, patch)
2010-07-16 01:35 PDT, Nikolas Zimmermann
krit: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nikolas Zimmermann 2010-07-15 07:47:51 PDT
When using layoutTestController.setDumpAsText() most DRT implementations will not generate a pixel test result.
This breaks svg/dynamic-updates test, which don't want to dump render tree information, but pure text, and a pixel test result. Without pixel test results these tests are useless.
It's unfortunate, this hasn't been noticed for a long time - DRT has changed at some point to not generate pixel results when dumpAsText=true.

I propose introducing setDumpAsTextAndPixels logic - already tested, works fine and gives me what I want.
Comment 1 Nikolas Zimmermann 2010-07-15 07:52:22 PDT
Created attachment 61661 [details]
Patch

Maciej, do you think that would be acceptable?
Comment 2 WebKit Review Bot 2010-07-15 07:55:03 PDT
Attachment 61661 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKitTools/DumpRenderTree/wx/DumpRenderTreeWx.cpp:206:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
Total errors found: 1 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Nikolas Zimmermann 2010-07-15 07:56:29 PDT
The style failure is in existing code, I'll fix it before landing (if this patch gets a positive review).
Comment 4 Nikolas Zimmermann 2010-07-16 01:05:18 PDT
Created attachment 61774 [details]
Updated patch

Updated patch after IRC discussion with Maciej.
Comment 5 WebKit Review Bot 2010-07-16 01:07:13 PDT
Attachment 61774 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKitTools/DumpRenderTree/wx/DumpRenderTreeWx.cpp:206:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
Total errors found: 1 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 6 Nikolas Zimmermann 2010-07-16 01:09:21 PDT
Oops, fixed style issue (not uploading a new patch to let EWS run)
Comment 7 WebKit Review Bot 2010-07-16 01:32:56 PDT
Attachment 61774 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3413423
Comment 8 Nikolas Zimmermann 2010-07-16 01:35:33 PDT
Created attachment 61778 [details]
Updated patch v2

Fix style issue and chromium build.
Comment 9 Nikolas Zimmermann 2010-07-16 02:31:09 PDT
Issue 1:
float* totals is passed from fastSetInteriorPixels, and the new helper function setDestinationPixels is called multiple times. That means you're doing the divisior division multiple times, accumulated!

for (int i = 0; i < numTotals; ++i)
    totals[i] = totals[i] / divisor + bias;

Issue 2:
for (int i = 0; i < 3; ++i)
    image->set(pixel++, clampRGBAValue(totals[0], maxAlpha));

You're always clamping totals[0], not totals[i].

Very evil!
Comment 10 Nikolas Zimmermann 2010-07-16 02:31:26 PDT
Oops, ignore that commented on wrong br.
Comment 11 Dirk Schulze 2010-07-16 02:43:54 PDT
Comment on attachment 61778 [details]
Updated patch v2

LGTM r=me
Comment 12 Nikolas Zimmermann 2010-07-16 02:49:04 PDT
Discussed the concept with Maciej, he was fine with it.
Landed in r63536.