[webkit-reviews] review denied: [Bug 21445] on windows, we try to remove m_smallCapsFontData from the font cache, but we never added it there : [Attachment 25902] updated changelog

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Tony Chang (Google)
<tony at chromium.org>'s request for review:
Bug 21445: on windows, we try to remove m_smallCapsFontData from the font
cache, but we never added it there
https://bugs.webkit.org/show_bug.cgi?id=21445

Attachment 25902: updated changelog
https://bugs.webkit.org/attachment.cgi?id=25902&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
It's weak when the code that allocates m_smallCapsFontData and the code that
destroys it are so far apart. This makes it hard to be sure we're using it
right. I think that's an argument for moving the FontCache::releaseFontData
call from SimpleFontData::~SimpleFontData into the
SimpleFontData::platformDestroy functions for the Mac platform. I think if we
did just that it would fix this bug.

> +    } else if (m_smallCapsFontData)
> +	   delete m_smallCapsFontData;

This patch is definitely wrong for some platforms other than Windows. If
isCustomFont() is false, then both SimpleFontData::~SimpleFontData and
SimpleFontData::platformDestroy will call delete on m_smallCapsFontData. Double
deletes are not good.

By the way, there's no need for an if here, because C++ already does nothing if
it's passed 0. Specifying the if just adds an extra branch, and would only be
justified in unusual circumstances.