[Webkit-unassigned] [Bug 77067] Parenthesis in RTL fonts are not mirrored with SVG fonts

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=77067





--- Comment #5 from Philip Rogers <pdr at google.com>  2012-01-26 19:26:09 PST ---
(In reply to comment #2)
> (From update of attachment 124053 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=124053&action=review
> 
> Nice first stab, needs more tests and some speedups:
> 
> > Source/WebCore/svg/SVGFontData.cpp:142
> > +    if (mirror)
> > +        remainingTextInRun = replaceMirroredCharacters(remainingTextInRun.characters(), remainingTextInRun.length());
> >      if (!currentCharacter && arabicForms.isEmpty())
> >          arabicForms = charactersWithArabicForm(remainingTextInRun, mirror);
> 
> Danger Will Robinson :-) This can potentially break the arabic-form support - which already handles mirroring. Please add another testcase (you can grep for arabic-form and duplicate an existing) to see how it plays together with your testcase text data.

Good eye :) I added the glyph-selection-arabic-forms.svg test and I think everything still works as it did (except now with correct mirroring!) I'm new to Arabic forms but if I understand them correctly, the original code did not handle mirroring for Arabic forms. A mirror parameter is passed to charactersWithArabicForm but only for determining whether the text should be processed ltr or rtl. I think it would be best to change that bool parameter from "mirror" to "rtl" in charactersWithArabicForm to prevent confusion--do you agree?

> 
> It's also a PITA that we're allocating a remainingTextInRun String object several times :( This needs to be optimized, applySVGGlyphSelection is hot.
> You shoulnd't need to build a whole new string, but instead iterate over the remainingTextInRun by looping over its const UChar* buffer, and call mirroredChar on the individual characters.

We definitely agree here but (per our IRC chat) I think it's best to leave this slightly slow function because the characters are immutable. If this ends up being a problem, a future optimization would be to do both mirroring and space normalization in a single pass.


Note: This patch will fail tests on most platforms at the moment. I need to add/update platform-specific test expectations when I get in the office tomorrow morning.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.