[webkit-reviews] review granted: [Bug 136956] Tweak and tighten SVG font converter : [Attachment 238753] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Darin Adler <darin at apple.com>'s
request for review:
Bug 136956: Tweak and tighten SVG font converter
https://bugs.webkit.org/show_bug.cgi?id=136956

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

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


Myles said r=me but didn’t change the review state to +.

>> Source/WebCore/svg/SVGToOTFFontConversion.cpp:84
>> +		, charString(WTF::move(charString))
> 
> Will this actually work? It looks like charString is getting copied in to the
function anyway - this is only saving the copy from the function to the member
variable. Or are constructor initializer lists special? Making the charString
an rvalue reference might help with readability in either case.

Yes, it will work. The caller moves into the argument, the function moves into
the data member. You can’t see the move at the call site here, but it’s there.

Changing this to an rvalue reference would also be fine with me.

>> Source/WebCore/svg/SVGToOTFFontConversion.cpp:334
>> +	int value =
m_fontElement.fastGetAttribute(SVGNames::horiz_adv_xAttr).toInt(&ok);
> 
> Instead of getting the attribute, you can use
m_fontFaceElement->horizontalAdvanceX()

That would be a big change. For one, it returns a float, not an integer, for
another, it returns 0 if there is no font element, and doesn’t provide a
separate “is OK” flag. If you want to make that change, you’ll need to do it.
It’s not just refactoring.

>> Source/WebCore/svg/SVGToOTFFontConversion.cpp:871
>> +	m_glyphs.append(GlyphData(WTF::move(path), glyphElement,
horizontalAdvance, verticalAdvance, m_boundingBox, codepoint));
> 
> I think it would be beneficial to only have one argument and conditionally
use downcast<>(), rather than having two arguments that mean almost the same
thing but are subtly different.

Which two arguments are you talking about? Is this comment about the
processGlyphElement function below? I actually think the current design of that
function is great, but sure you could check at runtime instead of compile time
if you prefer.

>> Source/WebCore/svg/SVGToOTFFontConversion.cpp:913
>> +	    // Indicate error by leaving the glyphs vector empty.
> 
> A comment at all might not be necessary; it's pretty obvious what's going on.
Up to you.

OK, I’ll omit that comment.