Bug 152772 – [SVG -> OTF Converter] Parsing failures cause use of incomplete fonts

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Myles C. Maxfield]

[SVG -> OTF Converter] Parsing failures cause use of incomplete fonts

[Myles C. Maxfield]

Created attachment 268353 [details]
Patch

[Myles C. Maxfield]

Created attachment 268354 [details]
Patch

[Myles C. Maxfield]

<rdar://problem/24043104>

[Said Abou-Hallawa]

Comment on attachment 268354 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=268354&action=review

> Source/WebCore/svg/SVGToOTFFontConversion.cpp:61
> +    bool error() const

Why is this function needed? Can't we check m_error in functions like SVGToOTFFontConverter::convertSVGToOTFFont() and return immediately if its value is true?

> Source/WebCore/svg/SVGToOTFFontConversion.cpp:1583
>      SVGToOTFFontConverter converter(element);

Since SVGToOTFFontConverter::processGlyphElement() sets m_error to true and since it's called from the constructor of SVGToOTFFontConverter, should not we check converter.m_error before calling SVGToOTFFontConverter::convertSVGToOTFFont()? If m_error is true we are going to return Nullopt anyway.

> Source/WebCore/svg/SVGToOTFFontConversion.cpp:1586
> +        return Nullopt;

Can't this if-statment be written like this?

if (!converter.convertSVGToOTFFont())
     return Nullopt;

[Myles C. Maxfield]

Comment on attachment 268354 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=268354&action=review

>> Source/WebCore/svg/SVGToOTFFontConversion.cpp:61
>> +    bool error() const
> 
> Why is this function needed? Can't we check m_error in functions like SVGToOTFFontConverter::convertSVGToOTFFont() and return immediately if its value is true?

I anticipate there may be new sources of errors in the future which should result in conversion failure.

>> Source/WebCore/svg/SVGToOTFFontConversion.cpp:1583
>>      SVGToOTFFontConverter converter(element);
> 
> Since SVGToOTFFontConverter::processGlyphElement() sets m_error to true and since it's called from the constructor of SVGToOTFFontConverter, should not we check converter.m_error before calling SVGToOTFFontConverter::convertSVGToOTFFont()? If m_error is true we are going to return Nullopt anyway.

Yeah, this is a good idea.

>> Source/WebCore/svg/SVGToOTFFontConversion.cpp:1586
>> +        return Nullopt;
> 
> Can't this if-statment be written like this?
> 
> if (!converter.convertSVGToOTFFont())
>      return Nullopt;

👍

[Myles C. Maxfield]

Committed r194839: <http://trac.webkit.org/changeset/194839>