Summary: | [GTK] Implement ImageDiff and add it to the build system | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Martin Robinson <mrobinson> | ||||
Component: | WebKitGTK | Assignee: | Nobody <webkit-unassigned> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | webkit.review.bot | ||||
Priority: | P2 | ||||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | PC | ||||||
OS: | Linux | ||||||
Bug Depends on: | |||||||
Bug Blocks: | 31518 | ||||||
Attachments: |
|
Description
Martin Robinson
2010-07-07 11:13:12 PDT
Created attachment 60764 [details]
ImageDiff implementation
Attachment 60764 [details] did not pass style-queue:
Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebKitTools/DumpRenderTree/gtk/ImageDiff.cpp:30: Found other header before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4]
Total errors found: 1 in 3 files
If any of these errors are false positives, please file a bug against check-webkit-style.
This is a stand-alone utility, therefore I think it shouldn't need config.h. Once it's in the tree I'll post a patch for this issue. Comment on attachment 60764 [details]
ImageDiff implementation
102 void* diffBuffer = malloc(width * height);
[...]
110 unsigned char* diff = static_cast<unsigned char*>(diffBuffer);
[...]
143 *differenceImage = differenceImageFromDifferenceBuffer(static_cast<unsigned char*>(diffBuffer), width, height);
Given that you never use the diffBuffer as a void*, I would prefer having the cast in the malloc call, it makes it a bit easier on the reader of the code.
I find the mixed usage of g_print, fputs and fprintf unnecessary, though I do not oppose to it. I feel like you cal also make the ifs that are under the else ifs here be just one more condition of the else if:
206 if (imageSize > 0 && !actualImage) {
207 if (!(actualImage = readPixbufFromStdin(imageSize))) {
208 fputs("Error, could not read actual image.\n", stdout);
209 return 1;
210 }
211 } else if (imageSize > 0 && !baselineImage) {
212 if (!(baselineImage = readPixbufFromStdin(imageSize))) {
213 fputs("Error, could not read baseline image.\n", stdout);
214 return 1;
215 }
It will make it a bit easier to read, IMO. Other than the above comments, I think the code looks right, let's start getting some experience with how well pixel tests will work for us, I guess =)
(In reply to comment #4) Thanks for the review! > Given that you never use the diffBuffer as a void*, I would prefer having > the cast in the malloc call, it makes it a bit easier on the reader of the > code. Okay, will fix. > I find the mixed usage of g_print, fputs and fprintf unnecessary, > though I do not oppose to it. I feel like you cal also make the ifs > that are under the else ifs here be just one more condition of the else if: Okay, I'll switch it to all (f)printf. > > 206 if (imageSize > 0 && !actualImage) { > 207 if (!(actualImage = readPixbufFromStdin(imageSize))) { > 208 fputs("Error, could not read actual image.\n", stdout); > 209 return 1; > 210 } > 211 } else if (imageSize > 0 && !baselineImage) { > 212 if (!(baselineImage = readPixbufFromStdin(imageSize))) { > 213 fputs("Error, could not read baseline image.\n", stdout); > 214 return 1; > 215 } I think this has to stay. I want to actually bail out if the image fails to load, but I don't want to bail out if the first condition fails. Committed as http://trac.webkit.org/changeset/62817 . Closing bug. |