Bug 57871

Created attachment 88291 [details] Patch

Created attachment 88451 [details] Patch

Not sure who a good reviewer for this would be. Just guessing based on what appears to be authors of the pixel dumping code.

Comment on attachment 88451 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=88451&action=review > Tools/DumpRenderTree/PixelDumpSupport.cpp:40 > +/* > + * makeCrcTable and computeCrc are conversions of the JS methods in pnglib.js. > + * > + * A handy class to calculate color values. > + * > + * @version 1.0 > + * @author Robert Eisele <robert@xarg.org> > + * @copyright Copyright (c) 2010, Robert Eisele > + * @link > + * http://www.xarg.org/2010/03/generate-client-side-png-files-using-javascript/ > + * @license http://www.opensource.org/licenses/bsd-license.php BSD License > + * > + */ Just put them in their own file, then we don't have to have this strangeness here.

(In reply to comment #4) > Just put them in their own file, then we don't have to have this strangeness here. I'm happy to move these two functions to their own file, but just to be clear, the strangeness we're trying to avoid is having 2 licenses in a single file? Even when in it's own file, I think I would still need both licenses since I authored a derivative work of the original code.

Created attachment 88488 [details] Patch

(In reply to comment #6) > Created an attachment (id=88488) [details] > Patch Here's a version with a separate .h/.cpp for the ported methods. Also, this doesn't work with QT yet because it doesn't use PixelDumpSupport.*. I'll fix that in a follow up change.

adding more reviewers

Comment on attachment 88488 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=88488&action=review > Tools/ChangeLog:8 > + We insert the bytes for the comment in printPNG rather than at encode indent problem? > Tools/DumpRenderTree/PNGUtil.h:32 > +#ifndef PNGUtil_h > +#define PNGUtil_h WebKit generally frowns upon things with "util" in their name. Why not CyclicRedundancyCheck.h ? > Tools/DumpRenderTree/PixelDumpSupport.cpp:88 > + bytesToAdd.resize(prefixLength + checksumLength + crcLength); > + memcpy(bytesToAdd.data(), textCommentPrefix, prefixLength); > + memcpy(bytesToAdd.data() + prefixLength, checksum, checksumLength); Why not just use append ? > Tools/DumpRenderTree/PixelDumpSupport.cpp:110 > + const unsigned char* idatPosition = std::search(data, data + dataLength, idat, idat + 4); Does this always work? For example, is there some reason IDAT can't appear in an ICC profile?

Comment on attachment 88488 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=88488&action=review did you forget to add to gypi build? In general this looks good, but I think we should go another round.... > Tools/ChangeLog:6 > + update DRT to embed checksums in png files > + https://bugs.webkit.org/show_bug.cgi?id=57871 tabs? spaceing? > Tools/DumpRenderTree/PNGUtil.cpp:40 > + * @license http://www.opensource.org/licenses/bsd-license.php BSD License Why not just inline the license like we do normally? This license block feels very non-standard. > Tools/DumpRenderTree/PNGUtil.cpp:52 > + if (c & 1) Is this how we normally check for odd? > Tools/DumpRenderTree/PNGUtil.cpp:53 > + c = -306674912 ^ ((c >> 1) & 0x7fffffff); This seems needlessly mysterious. > Tools/DumpRenderTree/PNGUtil.cpp:61 > +unsigned long computeCrc(unsigned char* buffer, size_t size) I would just pass in a vector. > Tools/DumpRenderTree/PixelDumpSupport.cpp:82 > + static const size_t prefixLength = 17; sizeof(textCommentPrefix)? > Tools/DumpRenderTree/PixelDumpSupport.cpp:91 > + dataToCrc.append(textCommentPrefix + 4, prefixLength - 4); // Don't include the length in the crc. You have crcLength in a local, why not use it? > Tools/DumpRenderTree/PixelDumpSupport.cpp:98 > + bytesToAdd[prefixLength + checksumLength] = ((crc32 >> 24) & 0xff); > + bytesToAdd[prefixLength + checksumLength + 1] = ((crc32 >> 16) & 0xff); > + bytesToAdd[prefixLength + checksumLength + 2] = ((crc32 >> 8) & 0xff); > + bytesToAdd[prefixLength + checksumLength + 3] = (crc32 & 0xff); I'm not sure why we're operating in bytes here. > Tools/DumpRenderTree/PixelDumpSupport.cpp:116 > + fwrite(bytesToAdd.data(), 1, bytesToAdd.size(), stdout); I'm confused why we use bytesToAdd and write in chunks of chars instead of operating with ints since it seems we are using ints?

Comment on attachment 88488 [details] Patch It looks like whoever wrote the JS one copied it directly from teh spec: http://www.libpng.org/pub/png/spec/1.2/PNG-CRCAppendix.html I love how we've round-tripped the language a couple times now. :)

Created attachment 88673 [details] Patch

Comment on attachment 88488 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=88488&action=review >> Tools/ChangeLog:8 >> + We insert the bytes for the comment in printPNG rather than at encode > > indent problem? Fixed >> Tools/DumpRenderTree/PNGUtil.cpp:40 >> + * @license http://www.opensource.org/licenses/bsd-license.php BSD License > > Why not just inline the license like we do normally? This license block feels very non-standard. Don't I have to copy the license/copyright verbatim as part of the BSD License? >> Tools/DumpRenderTree/PNGUtil.cpp:53 >> + c = -306674912 ^ ((c >> 1) & 0x7fffffff); > > This seems needlessly mysterious. I'm just copying the code from the source. If we want to improve on it, maybe in a follow up, but I don't think that is necessary here. >> Tools/DumpRenderTree/PNGUtil.cpp:61 >> +unsigned long computeCrc(unsigned char* buffer, size_t size) > > I would just pass in a vector. Done. >> Tools/DumpRenderTree/PNGUtil.h:32 >> +#define PNGUtil_h > > WebKit generally frowns upon things with "util" in their name. Why not CyclicRedundancyCheck.h ? Renamed. >> Tools/DumpRenderTree/PixelDumpSupport.cpp:82 >> + static const size_t prefixLength = 17; > > sizeof(textCommentPrefix)? Won't that give me the size of a pointer? >> Tools/DumpRenderTree/PixelDumpSupport.cpp:88 >> + memcpy(bytesToAdd.data() + prefixLength, checksum, checksumLength); > > Why not just use append ? Done. >> Tools/DumpRenderTree/PixelDumpSupport.cpp:91 >> + dataToCrc.append(textCommentPrefix + 4, prefixLength - 4); // Don't include the length in the crc. > > You have crcLength in a local, why not use it? Sorry, updated the comment to mean the chunk length, not the crc length. >> Tools/DumpRenderTree/PixelDumpSupport.cpp:98 >> + bytesToAdd[prefixLength + checksumLength + 3] = (crc32 & 0xff); > > I'm not sure why we're operating in bytes here. I think endianness matters. >> Tools/DumpRenderTree/PixelDumpSupport.cpp:110 >> + const unsigned char* idatPosition = std::search(data, data + dataLength, idat, idat + 4); > > Does this always work? For example, is there some reason IDAT can't appear in an ICC profile? IDAT is the section of the png that contains the actual pixels. It is a required part of all pngs. >> Tools/DumpRenderTree/PixelDumpSupport.cpp:116 >> + fwrite(bytesToAdd.data(), 1, bytesToAdd.size(), stdout); > > I'm confused why we use bytesToAdd and write in chunks of chars instead of operating with ints since it seems we are using ints? I don't understand this question. bytesToAdd has both text (e.g., the checksum) and ints.

(In reply to comment #13) > (From update of attachment 88488 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=88488&action=review > >> Tools/DumpRenderTree/PixelDumpSupport.cpp:110 > >> + const unsigned char* idatPosition = std::search(data, data + dataLength, idat, idat + 4); > > > > Does this always work? For example, is there some reason IDAT can't appear in an ICC profile? > > IDAT is the section of the png that contains the actual pixels. It is a required part of all pngs. Oh, I meant that those characters might randomly appear in an ICC profile and you might corrupt the PNG.

Comment on attachment 88673 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=88673&action=review I still love the change. I still think this direct byte-stream, c-style code is kinda ugly. But I'm not sure I have a lot of better suggestions. > Tools/DumpRenderTree/CyclicRedundancyCheck.cpp:40 > + * @license http://www.opensource.org/licenses/bsd-license.php BSD License Redistributions of source code must retain the above copyright notice, this list of conditions and the following disclaimer. I don't think that means the @link stuff. In fact, I suspect that we're more likely violating this by putting in our own Google version of the license. I would jsut delete all this and put Copyright 2010 Robert Eisele <robert@xarg.org> at the top of one of our normal BSD licenses. The version stuff and the link isn't very useful. I'm also not sure ohw much sense it makes for him to be able to copyright this since clearly he copied it directly from the spec I linked earlier. > Tools/DumpRenderTree/PixelDumpSupport.cpp:82 > + static const size_t prefixLength = 17; I was thinking of: const char[] foo = "foo"; sizeof(foo) which does work as expected. > Tools/DumpRenderTree/PixelDumpSupport.cpp:90 > + FILE* f = fopen("/tmp/logzzz", "w"); > + fprintf(f, "%d\n", (int)bytesToAdd.size()); > + fclose(f); Um.. > Tools/DumpRenderTree/PixelDumpSupport.cpp:101 > + bytesToAdd[prefixLength + checksumLength] = ((crc32 >> 24) & 0xff); > + bytesToAdd[prefixLength + checksumLength + 1] = ((crc32 >> 16) & 0xff); > + bytesToAdd[prefixLength + checksumLength + 2] = ((crc32 >> 8) & 0xff); > + bytesToAdd[prefixLength + checksumLength + 3] = (crc32 & 0xff); This is fine, but I would hav emade this a helper function. appendInt(vector, int) or similar. > Tools/DumpRenderTree/PixelDumpSupport.cpp:113 > + const unsigned char* idatPosition = std::search(data, data + dataLength, idat, idat + 4); magic 4? > Tools/DumpRenderTree/PixelDumpSupport.cpp:115 > + ASSERT(idatPosition - data > 4); magic 4? > Tools/DumpRenderTree/PixelDumpSupport.cpp:117 > + size_t insertOffset = (idatPosition - data) - 4; What's the magic 4?

(In reply to comment #14) > (In reply to comment #13) > > (From update of attachment 88488 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=88488&action=review > > >> Tools/DumpRenderTree/PixelDumpSupport.cpp:110 > > >> + const unsigned char* idatPosition = std::search(data, data + dataLength, idat, idat + 4); > > > > > > Does this always work? For example, is there some reason IDAT can't appear in an ICC profile? > > > > IDAT is the section of the png that contains the actual pixels. It is a required part of all pngs. > > Oh, I meant that those characters might randomly appear in an ICC profile and you might corrupt the PNG. Ah, right. Yes, you're right. I think it'll be safer to inject the bytes right after the IHDR chunk. Let me try that.

Created attachment 88696 [details] Patch

(In reply to comment #15) > (From update of attachment 88673 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=88673&action=review > > I still love the change. I still think this direct byte-stream, c-style code is kinda ugly. But I'm not sure I have a lot of better suggestions. Lots of iterations does not bother me. > > Tools/DumpRenderTree/CyclicRedundancyCheck.cpp:40 > > + * @license http://www.opensource.org/licenses/bsd-license.php BSD License > > I would jsut delete all this and put Copyright 2010 Robert Eisele <robert@xarg.org> at the top of one of our normal BSD licenses. The version stuff and the link isn't very useful. I'm also not sure ohw much sense it makes for him to be able to copyright this since clearly he copied it directly from the spec I linked earlier. Done. > > Tools/DumpRenderTree/PixelDumpSupport.cpp:82 > > + static const size_t prefixLength = 17; > > I was thinking of: const char[] foo = "foo"; sizeof(foo) which does work as expected. Done (but had to subtract 1 for the null). > > Tools/DumpRenderTree/PixelDumpSupport.cpp:90 > > + FILE* f = fopen("/tmp/logzzz", "w"); > > + fprintf(f, "%d\n", (int)bytesToAdd.size()); > > + fclose(f); > > Um.. Oops! Deleted. > > Tools/DumpRenderTree/PixelDumpSupport.cpp:101 > > + bytesToAdd[prefixLength + checksumLength] = ((crc32 >> 24) & 0xff); > > + bytesToAdd[prefixLength + checksumLength + 1] = ((crc32 >> 16) & 0xff); > > + bytesToAdd[prefixLength + checksumLength + 2] = ((crc32 >> 8) & 0xff); > > + bytesToAdd[prefixLength + checksumLength + 3] = (crc32 & 0xff); > > This is fine, but I would hav emade this a helper function. appendInt(vector, int) or similar. Done. > > Tools/DumpRenderTree/PixelDumpSupport.cpp:113 > > + const unsigned char* idatPosition = std::search(data, data + dataLength, idat, idat + 4); Removed since we insert at a different point.

Created attachment 88698 [details] update changelog

Comment on attachment 88698 [details] update changelog View in context: https://bugs.webkit.org/attachment.cgi?id=88698&action=review Looks great. I'm glad you don't mind iterating. :) This looks much better than the first version to me. > Tools/DumpRenderTree/PixelDumpSupport.cpp:91 > + static const size_t prefixLength = sizeof(textCommentPrefix) - 1; I would comment that the -1 is for the null.

Committed r83219: <http://trac.webkit.org/changeset/83219>