[Qt] DumpRenderTree lacks pixel tests support

[Ariya Hidayat]

DumpRenderTree of the Qt port currently does not support pixel tests.

I will post a series of patches that implement this.

[Ariya Hidayat]

Created attachment 33737 [details]
Implement ImageDiff

[Ariya Hidayat]

Created attachment 33738 [details]
Add pixel tests to DumpRenderTree

[Antonio Gomes]

<ariya> tonikitoo: relay to kenneth, give a try, check the coding style etc

[Simon Hausmann]

Comment on attachment 33737 [details]
Implement ImageDiff

r=me. Cool stuff :)

[Simon Hausmann]

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.

[Ariya Hidayat]

Landed in 
http://trac.webkit.org/changeset/46579
http://trac.webkit.org/changeset/46577

[Csaba Osztrogonác]

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.

[Csaba Osztrogonác]

Created attachment 48028 [details]
proposed fix

[Ariya Hidayat]

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?

[Csaba Osztrogonác]

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.

[Csaba Osztrogonác]

Created attachment 48035 [details]
proposed fix

[Csaba Osztrogonác]

Created attachment 48055 [details]
proposed fix

[Ariya Hidayat]

Comment on attachment 48055 [details]
proposed fix

LGTM.

[Csaba Osztrogonác]

Fix landed in http://trac.webkit.org/changeset/54289 .