[Webkit-unassigned] [Bug 53281] clean up CG allocations made while determining image size

[bugzilla-daemon at webkit.org]

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


Darin Adler <darin at apple.com> changed:

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




--- Comment #9 from Darin Adler <darin at apple.com>  2011-02-01 09:52:18 PST ---
(From update of attachment 80705)
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.

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