Bug 58173

Summary: [Qt] embed checksums in PNGs written by Qt-DRT
Product: WebKit Reporter: Tony Chang <tony>
Component: New BugsAssignee: Tony Chang <tony>
Status: RESOLVED FIXED    
Severity: Normal CC: ossy
Priority: P2 Keywords: Qt, QtTriaged
Version: 528+ (Nightly build)   
Hardware: Other   
OS: OS X 10.5   
Bug Depends on: 58243    
Bug Blocks: 56286    
Attachments:
Description Flags
Patch
none
fix license
none
crash log
none
Patch kling: review+

Description Tony Chang 2011-04-08 16:19:02 PDT
[qt] embed checksums in PNGs written by DRT QT
Comment 1 Tony Chang 2011-04-08 16:36:03 PDT
Created attachment 88894 [details]
Patch
Comment 2 Tony Chang 2011-04-08 16:41:35 PDT
Created attachment 88895 [details]
fix license
Comment 3 Eric Seidel (no email) 2011-04-10 15:57:55 PDT
Comment on attachment 88895 [details]
fix license

View in context: https://bugs.webkit.org/attachment.cgi?id=88895&action=review

> Tools/DumpRenderTree/CyclicRedundancyCheck.cpp:36
> +static void makeCrcTable(unsigned int crcTable[256])

Why not just "unsigned"?  I don't think the "int" adds anything here.

> Tools/DumpRenderTree/qt/DumpRenderTree.pro:32
> +    ../CyclicRedundancyCheck.h \

Does the .pro normally use ../ relative paths?
Comment 4 Csaba Osztrogonác 2011-04-11 04:57:58 PDT
Comment on attachment 88895 [details]
fix license

View in context: https://bugs.webkit.org/attachment.cgi?id=88895&action=review

Unfortunately DRT crashes sometimes with this patch.
r- now due to crashes. I will check it, and try to help fixing it.

>> Tools/DumpRenderTree/qt/DumpRenderTree.pro:32
>> +    ../CyclicRedundancyCheck.h \
> 
> Does the .pro normally use ../ relative paths?

It works, but we usually add the directory to INCLUDEPATH instead of using relative header paths.
Comment 5 Csaba Osztrogonác 2011-04-11 04:58:41 PDT
*** Bug 58021 has been marked as a duplicate of this bug. ***
Comment 6 Csaba Osztrogonác 2011-04-11 05:49:15 PDT
Created attachment 88995 [details]
crash log

I got this crash log with the following command:
$ WebKitBuild/Release/bin/DumpRenderTree LayoutTests/svg/W3C-SVG-1.1-SE/paths-dom-02-f.svg

I can't reproduce this crash in debug mode and/or without your patch.

If we don't pass --pixel-tests, your code doesn't run.
I don't understand what and why cause this strange crash.
Comment 7 Tony Chang 2011-04-11 09:54:51 PDT
Splitting the 64bit fix into a separate bug while I look into the 64bit test crash.
Comment 8 Tony Chang 2011-04-11 10:48:30 PDT
(In reply to comment #6)
> Created an attachment (id=88995) [details]
> crash log
> 
> I got this crash log with the following command:
> $ WebKitBuild/Release/bin/DumpRenderTree LayoutTests/svg/W3C-SVG-1.1-SE/paths-dom-02-f.svg
> 
> I can't reproduce this crash in debug mode and/or without your patch.
> 
> If we don't pass --pixel-tests, your code doesn't run.
> I don't understand what and why cause this strange crash.

I suspect this crash has to do with including FastMalloc.cpp.  When writing this patch, I wasn't sure if it was OK to use wtf types (Vector.h) in QT's DRT.  Up until now, DRT hasn't had any dependencies on wtf except for Assertions.h.

If I can't use wtf, I could either switch to std::vector or duplicate the code (might be simpler since other code probably wants to use WTF::Vector).
Comment 9 Tony Chang 2011-04-12 11:37:12 PDT
Created attachment 89233 [details]
Patch
Comment 10 Tony Chang 2011-04-12 11:38:04 PDT
(In reply to comment #9)
> Created an attachment (id=89233) [details]
> Patch

QT++ for exposing comment writing/reading in the QImage API.
Comment 11 Andreas Kling 2011-04-12 12:01:46 PDT
Comment on attachment 89233 [details]
Patch

Cool! r=me
Comment 12 Tony Chang 2011-04-12 12:10:53 PDT
Committed r83619: <http://trac.webkit.org/changeset/83619>