Summary: | [SVG -> OTF Converter] Support non-BMP codepoints | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Myles C. Maxfield <mmaxfield> | ||||||||||
Component: | Layout and Rendering | Assignee: | Myles C. Maxfield <mmaxfield> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | commit-queue, darin, dino, d-r, fmalita, gyuyoung.kim, jonlee, pdr, schenney, sergio, simon.fraser, thorton | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Attachments: |
|
Description
Myles C. Maxfield
2014-09-24 17:29:09 PDT
Created attachment 238798 [details]
Patch
Comment on attachment 238798 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=238798&action=review > Source/WebCore/ChangeLog:1 > +2014-09-27 Myles C. Maxfield <litherum@gmail.com> Fix email address Created attachment 238820 [details]
Patch
Created attachment 238825 [details]
Patch
Comment on attachment 238825 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=238825&action=review > Source/WebCore/svg/SVGToOTFFontConversion.cpp:949 > + codepoint2 = data1.codepoints[i2++]; data2.codepoints Comment on attachment 238825 [details]
Patch
Myles, are you going to review my tweaks to this file before landing all these additional changes? Or are you not reviewing because you are not a reviewer?
Code point is two words, not one.
Created attachment 238834 [details]
Patch
Darin: Oh, whoops - I didn't realize there was more to review! Comment on attachment 238834 [details] Patch 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_formAttr), 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. Comment on attachment 238834 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=238834&action=review >> 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! This is for CFFBuilder. I've updated the ChangeLog. Done. >> 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. Done. Do you think I should use Vector<UChar32> instead? I'll keep String for now but I can always change it before it gets turned on. >> 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. Ah, good to know! I'll remove the keyword. Done. >> 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. Turns out that this leads to ambiguous calls such as "append32(0)". I'll rename this to append32BitCode(). Done. >> 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. Given that length will always be 0 and capacity will always be 2, the compiler should be able to optimize out the check. >> 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. I'll make a bug about this - I'm thinking about making another data structure that remembers code points as UChar32 instead of Strings. https://bugs.webkit.org/show_bug.cgi?id=137395 Done. >> Source/WebCore/svg/SVGToOTFFontConversion.cpp:939 >> +bool SVGToOTFFontConverter::compareCodepointsLexicographically(const GlyphData& data1, const GlyphData& data2) > > Why is this a member function? GlyphData is a private member of SVGToOTFFontConverter >> Source/WebCore/svg/SVGToOTFFontConversion.cpp:943 >> + 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. Done. Done. >> 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. Done. >> Source/WebCore/svg/SVGToOTFFontConversion.cpp:962 >> + && equalIgnoringCase(data1.glyphElement->fastGetAttribute(SVGNames::arabic_formAttr), 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. Done. |