[Webkit-unassigned] [Bug 60569] Add flags to Chromium ImageDiff to write image comparison metrics out to files.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=60569


Tony Chang <tony at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #92996|review?, commit-queue?      |review-
               Flag|                            |




--- Comment #5 from Tony Chang <tony at chromium.org>  2011-05-10 15:42:01 PST ---
(From update of attachment 92996)
View in context: https://bugs.webkit.org/attachment.cgi?id=92996&action=review

In general, this looks fine, mostly just style nits.

Out of curiosity, what is this for?

> Tools/DumpRenderTree/chromium/ImageDiff.cpp:199
> +#define MAX2(a, b) ((a) < (b) ? (b) : (a))
> +#define MIN2(a, b) ((a) < (b) ? (a) : (b))

Do we really need to use macros here?  What's wrong with std::min and std::max?

> Tools/DumpRenderTree/chromium/ImageDiff.cpp:203
> +#define MAX3(a, b, c) ((a) < (b) ? MAX2((b), (c)) : MAX2((a), (c)))
> +#define RED(a) ((a << 24) >> 24)
> +#define GREEN(a) ((a << 16) >> 24)
> +#define BLUE(a) ((a << 8) >> 24)

I would also just make these functions.  If you want you can even mark them as inline.

> Tools/DumpRenderTree/chromium/ImageDiff.cpp:214
> +    for (int y = 0; y < h; y++) {
> +        for (int x = 0; x < w; x++) {

Nit: WebKit style prefers ++y and ++x.

> Tools/DumpRenderTree/chromium/ImageDiff.cpp:433
> +    fprintf(stderr, "ImageDiff::diffImages\n");

Did you mean to keep this line here?

> Tools/DumpRenderTree/chromium/ImageDiff.cpp:442
> +        int fileTypeDotOffset = strrchr(outFile, '.') - outFile;
> +        strncpy(metadataOutFilename, outFile, fileTypeDotOffset);
> +        strcpy(metadataOutFilename + fileTypeDotOffset, "-meta.json");

Can we just use std::string here?  Something like:
std::string outFileString(outFile);
outFileString = outFileString.substring(0, outFileString.find('.') - 1) + "-meta.json";

> Tools/DumpRenderTree/chromium/ImageDiff.cpp:443
> +        fprintf(stderr, "ImageDiff: metadata in %s\n", metadataOutFilename);

Do we need this logging statement?

> Tools/Scripts/webkitpy/layout_tests/port/chromium.py:156
> -                   actual_filename, diff_filename]
> +                   actual_filename, diff_filename,
> +                   '--writePercent', '--weighted']

Can we add a new test case rather than replacing an existing test case?

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.