Add the ability to write comments to PNGCodec::Encode.

Add the ability to write comments to PNGCodec::Encode.

I'm going to use this to write layout test checksums to the
expected png files.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=79592

[brettw, 2011-03-25 00:06:09]

LGTM

http://codereview.chromium.org/6696085/diff/3001/ui/gfx/codec/png_codec.cc
File ui/gfx/codec/png_codec.cc (right):

http://codereview.chromium.org/6696085/diff/3001/ui/gfx/codec/png_codec.cc#ne...
ui/gfx/codec/png_codec.cc:554: : comments_(comments), png_text_(new
png_text[comments.size()]) {
Style guide implies these should go on different lines (exception is when
absolutely everything fits on one line).

http://codereview.chromium.org/6696085/diff/3001/ui/gfx/codec/png_codec.cc#ne...
ui/gfx/codec/png_codec.cc:582: // A PNG comment's can only be 79 characters
long.
Maybe a DCHECK on the length here would be nice so people aren't shocked if
their comments get truncated.

http://codereview.chromium.org/6696085/diff/3001/ui/gfx/codec/png_codec.h
File ui/gfx/codec/png_codec.h (right):

http://codereview.chromium.org/6696085/diff/3001/ui/gfx/codec/png_codec.h#new...
ui/gfx/codec/png_codec.h:49: Comment(const std::string& k, const std::string& t)
: key(k), text(t) {}
Wouldn't this constructor normally go first?

Can you make this non-inline and also write a non-inline destructor so we don't
get this instantiated all over the place (channeling erg).

[tony, 2011-03-25 01:04:44]

Can you take another look?  I had to remove PNG_NO_WRITE_TEXT from pnguser.h to
enable this on Mac and Win (Linux is using the system libpng.so which has it
enable).

w.r.t binary size, toggling the flag grew the binary by 8808 bytes, which seems
OK.  I guess there's also a tiny bit larger surface for an exploit, but since
we're not calling this code in chrome, I don't think it matters.

http://codereview.chromium.org/6696085/diff/3001/ui/gfx/codec/png_codec.cc
File ui/gfx/codec/png_codec.cc (right):

http://codereview.chromium.org/6696085/diff/3001/ui/gfx/codec/png_codec.cc#ne...
ui/gfx/codec/png_codec.cc:554: : comments_(comments), png_text_(new
png_text[comments.size()]) {
On 2011/03/25 00:06:09, brettw wrote:
> Style guide implies these should go on different lines (exception is when
> absolutely everything fits on one line).

Done.

http://codereview.chromium.org/6696085/diff/3001/ui/gfx/codec/png_codec.cc#ne...
ui/gfx/codec/png_codec.cc:582: // A PNG comment's can only be 79 characters
long.
On 2011/03/25 00:06:09, brettw wrote:
> Maybe a DCHECK on the length here would be nice so people aren't shocked if
> their comments get truncated.

Done.

http://codereview.chromium.org/6696085/diff/3001/ui/gfx/codec/png_codec.h
File ui/gfx/codec/png_codec.h (right):

http://codereview.chromium.org/6696085/diff/3001/ui/gfx/codec/png_codec.h#new...
ui/gfx/codec/png_codec.h:49: Comment(const std::string& k, const std::string& t)
: key(k), text(t) {}
On 2011/03/25 00:06:09, brettw wrote:
> Wouldn't this constructor normally go first?
> 
> Can you make this non-inline and also write a non-inline destructor so we
don't
> get this instantiated all over the place (channeling erg).

Done.

[brettw, 2011-03-26 18:33:20]

LGTM.

Why don't you ping the binary size reduction task force to tell them about the
8K? It would be nice if that was in a list of "stuff we could potentially get
rid of if we really wanted to".

[darin (slow to review), 2011-07-09 06:24:45]

http://codereview.chromium.org/6696085/diff/1020/webkit/support/webkit_suppor...
File webkit/support/webkit_support_gfx.h (right):

http://codereview.chromium.org/6696085/diff/1020/webkit/support/webkit_suppor...
webkit/support/webkit_support_gfx.h:31: gfx::Size(width, height),
row_byte_width, false,
FYI: this had the side effect of making DumpRenderTree need to link to the Size
constructor and destructor, which means for the DLL build, DumpRenderTree now
needs to link to ui.dll (containing gfx::Size).  Instead, I plan on making these
non-inline.