[webkit-reviews] review denied: [Bug 53281] clean up CG allocations made while determining image size : [Attachment 80705] track extra CG allocations as part of an image's decoded size

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Ian Henderson <ianh at apple.com>'s
request for review:
Bug 53281: clean up CG allocations made while determining image size
https://bugs.webkit.org/show_bug.cgi?id=53281

Attachment 80705: track extra CG allocations as part of an image's decoded size
https://bugs.webkit.org/attachment.cgi?id=80705&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=80705&action=review

Looks good, on the right track.

But there’s enough here that needs some additional thought and revision that I
am going to say review-.

> Source/WebCore/ChangeLog:5
> +	   Clean up CG allocations made while determining image size

I don’t understand the title of this bug. This patch doesn’t seem to “clean up
allocations”.

> Source/WebCore/platform/graphics/BitmapImage.cpp:88
> -    destroyMetadataAndNotify(framesCleared);
> +    destroyMetadataAndNotify(framesCleared, true);

We frown on this type of boolean argument in WebKit code. Seeing “true” here
doesn’t tell you what’s going on. Generally we either avoid these kinds of
arguments altogether or use enums so you can read the constant at the call
site.

> Source/WebCore/platform/graphics/BitmapImage.cpp:103
> +void BitmapImage::destroyMetadataAndNotify(int framesCleared, bool
propertiesDataCleared)

The meaning of the boolean “properties data cleared” is not clear to me. Who
cleared the properties data? What exactly does that entail. I guess the
existing “framesCleared” argument is already confusing in a similar way.

> Source/WebCore/platform/graphics/BitmapImage.cpp:151
> +void BitmapImage::sourceDecodedProperties() const

I don’t understand this function name. It seems to tell the image to “source”
the “decoded properties”, but I’m not sure what either of those terms means.

Maybe it’s a function for the “source” to call that says that it has “decoded
properties”. Probably best to name it “sourceDidDecodeProperties” or
“didDecodeProperties” in that case.

> Source/WebCore/platform/graphics/BitmapImage.cpp:159
> +    int deltaBytes = updatedSize;
> +    deltaBytes -= m_decodedPropertiesSize;

It would be a little more conventional to just write:

    int deltaBytes = updatedSize - m_decodedPropertiesSize;

Subtracting two size_t and putting the result into an int could result in
overflow. What guarantees the difference in size will fit in an int?

> Source/WebCore/platform/graphics/cg/ImageSourceCG.cpp:227
> +    // Measured by tracing malloc/calloc calls.

This definitely can’t be correct for both 32-bit and 64-bit systems, nor for
multiple versions of CG. So the comment should explain that it’s OK to have
this value that’s likely to be inaccurate. It’s good that your comment explains
you got the number by tracing on one particular system and version, but it also
needs to explain that it’s OK for to have an estimate here that might be wrong.