[webkit-reviews] review granted: [Bug 137097] [SVG -> OTF Converter] Support non-BMP codepoints : [Attachment 238834] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Myles C. Maxfield
<mmaxfield at apple.com>'s request for review:
Bug 137097: [SVG -> OTF Converter] Support non-BMP codepoints
https://bugs.webkit.org/show_bug.cgi?id=137097

Attachment 238834: Patch
https://bugs.webkit.org/attachment.cgi?id=238834&action=review

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


Has this been rebased after my changes? I think I see things that I thought I
improved back in the old style.

I see a few problems but this seems OK to land.

> Source/WebCore/svg/SVGToOTFFontConversion.cpp:43
> +static inline void append32(Vector<char>& result, uint32_t value)

Comment in change log makes it sound like you got rid of this, but it’s still
here!

> Source/WebCore/svg/SVGToOTFFontConversion.cpp:62
> +    typedef String Codepoints;

I don’t think this typedef is a good abstraction. Should just use String
directly.

It also seems a bis strange to use a string for this.

> Source/WebCore/svg/SVGToOTFFontConversion.cpp:97
> +    inline void append32(uint32_t value)

The keyword inline has no effect here in a class definition. Any functions
defined in the class definition are implicitly treated as inline. I suggest
either moving thereout of the class definition, or removing the redundant
inline keywords.

> Source/WebCore/svg/SVGToOTFFontConversion.cpp:108
> +    inline void append4ByteCode(const char code[4])

Seems to me this could just be an overload of append32. I don’t think 4ByteCode
adds any clarity. The only reason we include the size is to avoid accidentally
writing the wrong amount of data. We should use the same terminology for
appending 4 bytes in both cases, not arbitrarily call one 32 and the other
4Byte.

> Source/WebCore/svg/SVGToOTFFontConversion.cpp:645
> +	       U16_APPEND(buffer, length, 2, codepoint, error);

Seems to me that U16_APPEND is overkill here; all the buffer length checking is
pointless. But maybe it compiles to something efficient enough.

> Source/WebCore/svg/SVGToOTFFontConversion.cpp:647
> +		   for (auto index :
m_codepointsToIndicesMap.get(String(buffer, length)))

This is absurdly expensive, allocating and destroying a String for each code
point.

> Source/WebCore/svg/SVGToOTFFontConversion.cpp:939
> +bool SVGToOTFFontConverter::compareCodepointsLexicographically(const
GlyphData& data1, const GlyphData& data2)

Why is this a member function?

> Source/WebCore/svg/SVGToOTFFontConversion.cpp:943
> +    unsigned i1 = 0, i2 = 0;
> +    UChar32 codepoint1, codepoint2;
> +    unsigned length1 = data1.codepoints.length(), length2 =
data2.codepoints.length();

We normally avoid doing these definitions two on a line like this in WebKit
coding style.

I don’t understand why codepoint1 and codepoint2 are defined outside the loop.

> Source/WebCore/svg/SVGToOTFFontConversion.cpp:944
> +    for ( ; i1 < length1 && i2 < length2; ) {

We should put the initialization in here or just use while instead of for.

> Source/WebCore/svg/SVGToOTFFontConversion.cpp:962
> +	   &&
equalIgnoringCase(data1.glyphElement->fastGetAttribute(SVGNames::arabic_formAtt
r), String("isolated", String::ConstructFromLiteral)))

There is no need to construct a String just to call equalIgnoringCase. Or if
there is, it's just a missing overload that should be added. Please remove the
"String/ConstructFromLiteral" dance.