Bug 27813

Summary: [Qt] DumpRenderTree lacks pixel tests support
Product: WebKit Reporter: Ariya Hidayat <ariya.hidayat>
Component: Tools / TestsAssignee: Ariya Hidayat <ariya.hidayat>
Status: RESOLVED FIXED    
Severity: Normal CC: abecsi, hausmann, kenneth, ossy, tonikitoo
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Other   
OS: Linux   
Attachments:
Description Flags
Implement ImageDiff
none
Add pixel tests to DumpRenderTree
none
proposed fix
none
proposed fix
none
proposed fix ariya.hidayat: review+

Description Ariya Hidayat 2009-07-29 13:19:18 PDT
DumpRenderTree of the Qt port currently does not support pixel tests.

I will post a series of patches that implement this.
Comment 1 Ariya Hidayat 2009-07-29 13:23:40 PDT
Created attachment 33737 [details]
Implement ImageDiff
Comment 2 Ariya Hidayat 2009-07-29 13:27:26 PDT
Created attachment 33738 [details]
Add pixel tests to DumpRenderTree
Comment 3 Antonio Gomes 2009-07-29 13:46:25 PDT
<ariya> tonikitoo: relay to kenneth, give a try, check the coding style etc
Comment 4 Simon Hausmann 2009-07-30 05:16:11 PDT
Comment on attachment 33737 [details]
Implement ImageDiff

r=me. Cool stuff :)
Comment 5 Simon Hausmann 2009-07-30 05:18:15 PDT
Comment on attachment 33738 [details]
Add pixel tests to DumpRenderTree

r=me. One sidenote: Due to the WTF assertion macros not be consistenly exported in Mac framework builds in dbg vs. release it may be better to use Q_ASSERT instead of ASSERT() for now.
Comment 7 Csaba Osztrogonác 2010-02-03 07:32:33 PST
Now pixel tests don't work with QtWebKit because of defective DRT implementation.

bug #1
run-webkit-tests pass filename with expected PNG hash to DRT.
eg: test_path/test_name.html'3f973ccf15193923c3fa3fda3cccbb50

If the filename/URL doesn't start with "http:", "https:", "file:",
DumpRenderTree::processLine test file existance incorrectly, because
we should remove the hash code:
QFileInfo fi(line);
if (!fi.exists()) ...

Now DumpRenderTree::open() separates the real filename and hash,
we should move this mechanism to processLine to work correctly.


bug #2
DumpRenderTree::dump() dumps only the first 32768 bytes chuck of PNG file.
I have no idea what would you like to do with this code:
if (written == block)
    break;

I think it is absolutely unnecessary (and bad), so I propose to remove it.
Comment 8 Csaba Osztrogonác 2010-02-03 07:33:14 PST
Created attachment 48028 [details]
proposed fix
Comment 9 Ariya Hidayat 2010-02-03 07:55:20 PST
Comment on attachment 48028 [details]
proposed fix

> +        [Qt] DumpRenderTree lacks pixel tests support
> +        https://bugs.webkit.org/show_bug.cgi?id=27813

Maybe provide the description of your fix, rather then the bug?

> +    m_expectedHash = QString();
> +    if (m_dumpPixels) {
> +        // single quote marks the pixel dump hash
> +        int i = line.indexOf('\'');
> +        if (i > -1) {
> +            m_expectedHash = line.mid(i + 1, line.length());
> +            line.remove(i, line.length());
> +        }
> +    }

Looks good.

> -                if (written == block)
> -                    break;

This is because it is ported from other DRT. Maybe check why this exists elsewhere?
Comment 10 Csaba Osztrogonác 2010-02-03 08:18:51 PST
You ported this code from WebKitTools/DumpRenderTree/PixelDumpSupport.cpp:
const size_t bytesToWriteInOneChunk = 1 << 15;
    size_t dataRemainingToWrite = dataLength;
    while (dataRemainingToWrite) {
        size_t bytesToWriteInThisChunk = std::min(dataRemainingToWrite, bytesToWriteInOneChunk);
        size_t bytesWritten = fwrite(data, 1, bytesToWriteInThisChunk, stdout);
        if (bytesWritten != bytesToWriteInThisChunk)
            break;
        dataRemainingToWrite -= bytesWritten;
        data += bytesWritten;
}

--

s/==/!=/ can solve the problem, but I think the original method and variable names are more understandable, so I suggest we should only port it without refactoring.
Comment 11 Csaba Osztrogonác 2010-02-03 08:19:30 PST
Created attachment 48035 [details]
proposed fix
Comment 12 Csaba Osztrogonác 2010-02-03 11:17:36 PST
Created attachment 48055 [details]
proposed fix
Comment 13 Ariya Hidayat 2010-02-03 11:33:12 PST
Comment on attachment 48055 [details]
proposed fix

LGTM.
Comment 14 Csaba Osztrogonác 2010-02-03 11:40:23 PST
Fix landed in http://trac.webkit.org/changeset/54289 .