Bug 42374

Summary: Add possibility to dumpAsText and generate a pixel test result
Product: WebKit Reporter: Nikolas Zimmermann <zimmermann>
Component: Tools / TestsAssignee: Nikolas Zimmermann <zimmermann>
Status: RESOLVED FIXED    
Severity: Normal CC: dglazkov, mjs, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
See Also: https://bugs.webkit.org/show_bug.cgi?id=258298
Bug Depends on:    
Bug Blocks: 42376    
Attachments:
Description Flags
Patch
none
Updated patch
none
Updated patch v2 krit: review+

Nikolas Zimmermann
Reported 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.
Attachments
Patch (13.55 KB, patch)
2010-07-15 07:52 PDT, Nikolas Zimmermann 		no flags
DetailsFormatted DiffDiff
Updated patch (11.91 KB, patch)
2010-07-16 01:05 PDT, Nikolas Zimmermann 		no flags
DetailsFormatted DiffDiff
Updated patch v2 (11.90 KB, patch)
2010-07-16 01:35 PDT, Nikolas Zimmermann 		krit: review+
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Nikolas Zimmermann
Comment 1 2010-07-15 07:52:22 PDT
Created attachment 61661 [details] Patch Maciej, do you think that would be acceptable?
WebKit Review Bot
Comment 2 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.
Nikolas Zimmermann
Comment 3 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).
Nikolas Zimmermann
Comment 4 2010-07-16 01:05:18 PDT
Created attachment 61774 [details] Updated patch Updated patch after IRC discussion with Maciej.
WebKit Review Bot
Comment 5 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.
Nikolas Zimmermann
Comment 6 2010-07-16 01:09:21 PDT
Oops, fixed style issue (not uploading a new patch to let EWS run)
WebKit Review Bot
Comment 7 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
Nikolas Zimmermann
Comment 8 2010-07-16 01:35:33 PDT
Created attachment 61778 [details] Updated patch v2 Fix style issue and chromium build.
Nikolas Zimmermann
Comment 9 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!
Nikolas Zimmermann
Comment 10 2010-07-16 02:31:26 PDT
Oops, ignore that commented on wrong br.
Dirk Schulze
Comment 11 2010-07-16 02:43:54 PDT
Comment on attachment 61778 [details] Updated patch v2 LGTM r=me
Nikolas Zimmermann
Comment 12 2010-07-16 02:49:04 PDT
Discussed the concept with Maciej, he was fine with it. Landed in r63536.
Note You need to log in before you can comment on or make changes to this bug.