[webkit-reviews] review denied: [Bug 53455] Update ImageDecoder as suggested on webkit-dev : [Attachment 80691] Patch

[bugzilla-daemon at webkit.org]

David Levin <levin at chromium.org> has denied Adam Barth <abarth at webkit.org>'s
request for review:
Bug 53455: Update ImageDecoder as suggested on webkit-dev
https://bugs.webkit.org/show_bug.cgi?id=53455

Attachment 80691: Patch
https://bugs.webkit.org/attachment.cgi?id=80691&action=review

------- Additional Comments from David Levin <levin at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=80691&action=review

Compile error (where header needs to be contents) plus a few other things to
consider.

> Source/WebCore/platform/image-decoders/ImageDecoder.cpp:43
> +unsigned copyFromSharedBuffer(char* buffer, unsigned bufferLength, const
SharedBuffer& sharedBuffer, unsigned offset)

Why did this go from being static to being in an anonymous namespace? (I've
seen this a lot in Chromium code but WebKit seems to go with static functions,
so I was curious what caused you to choose one over the other.)

> Source/WebCore/platform/image-decoders/ImageDecoder.cpp:60
> +    return !strncmp(contents, "GIF8", 4);

How odd that some places use memcmp and others strncmp but you are simply
translating what was there already.

>> Source/WebCore/platform/image-decoders/ImageDecoder.cpp:95
>> +}
> 
> I think this is supposed to get a "// namespace" comment?

fwiw, that isn't a WebKit style (and several folks don't want it).

> Source/WebCore/platform/image-decoders/ImageDecoder.cpp:97
>  ImageDecoder* ImageDecoder::create(const SharedBuffer& data,
ImageSource::AlphaOption alphaOption, ImageSource::GammaAndColorProfileOption
gammaAndColorProfileOption)

Additional future clean up would make this a PassOwnPtr.

> Source/WebCore/platform/image-decoders/ImageDecoder.cpp:99
> +    static const unsigned lengthOfLongestSignature = 14; // To wit:
"RIFF????WEBPVP"

This is different from before in that it potentially allowed 4 byte items to go
thru previously but perhaps no image can be encoded in less than 14 bytes?

What about doing something like
  memset(contents, sizeof(contents) - length, 0xee);
instead of returning?