[webkit-reviews] review denied: [Bug 85089] [Qt] ASSERT in FontCustomPlatformDataQt.cpp with invalid font in data URI : [Attachment 139254] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Wed May 2 03:28:53 PDT 2012
Simon Hausmann <hausmann at webkit.org> has denied Keith Rosenblatt
<keith.rosenblatt at nokia.com>'s request for review:
Bug 85089: [Qt] ASSERT in FontCustomPlatformDataQt.cpp with invalid font in
data URI
https://bugs.webkit.org/show_bug.cgi?id=85089
Attachment 139254: Patch
https://bugs.webkit.org/attachment.cgi?id=139254&action=review
------- Additional Comments from Simon Hausmann <hausmann at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=139254&action=review
I think the patch is correct, but at the same time I think a layout test should
cover this. It might be that there is already one, in which case this should be
mentioned in the ChangeLog.
> Source/WebCore/ChangeLog:11
> + No new tests.
Well, shouldn't there be one then? :) It doesn't sound difficult to create a
layout test that triggers the assert in a debug build.
>> Source/WebCore/platform/graphics/qt/FontCustomPlatformDataQt.cpp:77
>> + delete data;
>
> Maybe we could avoid allocating and de-allocating memory for a
FontCustomPlatformData instance altogether in that case by creating a temporary
QRawFont instance on the stack before that and doing the load / check there.
> If it's valid, the implicit sharing makes copying it to the newly created
FontCustomPlatformData almost free anyway.
I agree with Pierre. Either a temporary QRawFont object or putting
FontCustomPlatformData into an OwnPtr that is leaked() on return.
More information about the webkit-reviews
mailing list