[webkit-reviews] review denied: [Bug 77067] Parenthesis in RTL fonts are not mirrored with SVG fonts : [Attachment 124330] Update per reviewer comments

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri Jan 27 10:42:14 PST 2012


Darin Adler <darin at apple.com> has denied Philip Rogers <pdr at google.com>'s
request for review:
Bug 77067: Parenthesis in RTL fonts are not mirrored with SVG fonts
https://bugs.webkit.org/show_bug.cgi?id=77067

Attachment 124330: Update per reviewer comments
https://bugs.webkit.org/attachment.cgi?id=124330&action=review

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


> Source/WebCore/svg/SVGFontData.cpp:279
> +    StringBuilder mirroredCharacters;
> +    mirroredCharacters.reserveCapacity(length);
> +
> +    for (unsigned i = 0; i < length; ++i)
> +	   mirroredCharacters.append((UChar) mirroredChar(characters[i]));
> +
> +    return mirroredCharacters.toString();

This loop is incorrect. It walks through UTF-16 character codes rather than
walking through Unicode characters so won’t correctly mirror non-BMP
characters. The need to cast mirroredChar’s result to (UChar) emphasizes the
mistake.

It’s straightforward to iterate the characters instead and handle surrogates
correctly. There are examples of the use of U16_NEXT to read a character at a
time in functions such as SegmentedFontData::containsCharacters and we can
correctly write the characters in UTF-16 by using U16_LENGTH, U16_LEAD, and
U16_TRAIL.

> Source/WebCore/svg/SVGFontData.h:59
> +    String replaceMirroredCharacters(const UChar* characters, unsigned
length) const;

Naming this function that returns a newly created string “replace” is not so
great. I would name it something like createStringWithMirroredCharacters.


More information about the webkit-reviews mailing list