Bug 27813

Created attachment 33737 [details] Implement ImageDiff

Created attachment 33738 [details] Add pixel tests to DumpRenderTree

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

Comment on attachment 33737 [details] Implement ImageDiff r=me. Cool stuff :)

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.

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

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.

Created attachment 48028 [details] proposed fix

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?

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.

Created attachment 48035 [details] proposed fix

Created attachment 48055 [details] proposed fix

Comment on attachment 48055 [details] proposed fix LGTM.

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