[Webkit-unassigned] [Bug 83227] [chromium] wrong justification for arabic/persian page in cr-win

Thu Apr 5 00:53:49 PDT 2012

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

--- Comment #9 from Xiaomei Ji <xji at chromium.org>  2012-04-05 00:53:49 PST ---
(From update of attachment 135716)
View in context: https://bugs.webkit.org/attachment.cgi?id=135716&action=review

>>> Source/WebCore/platform/graphics/chromium/UniscribeHelper.cpp:198
>>> +        int additionalSpaceInRun = additionalSpace; 
>> 
>> this line should not be necessary. all the 'additionalSpaceInRun" access should be just "additionalSpace".
>> Semantically, it is the same as moving this line to outside of 'for' loop.
> 
> Please remove it.

done

>> Source/WebCore/platform/graphics/chromium/UniscribeHelper.cpp:207
>> +            int characterIndex = m_runs[run].iCharPos + i;
> 
> run -> runIndex?

I think it should be 'run' as m_runs and run are logical, while runIndex and shaping is visual.

>> Source/WebCore/platform/graphics/chromium/UniscribeHelper.cpp:216
>> +            padPerSpace = additionalSpaceInRun / numSpaces;
> 
> You can use:
> int padPerSpace = numSpaces ? additionalSpaceInRun / numSpaces : 0;

done.

>> Source/WebCore/platform/graphics/chromium/UniscribeHelper.cpp:219
>> +            bool hasExtraSpacing = letterSpacing() || wordSpacing() || additionalSpaceInRun;
> 
> Why hasExtraSpacing is needed? Looks like this loop does nothing when additionalSpaceInRun is 0.

you are right. changed to check 'if (additionalSpace)'

>> Source/WebCore/platform/graphics/chromium/UniscribeHelper.cpp:223
>> +                if (characterIndex != -1) {
> 
> if (spaceCharacters[i] != -1) and removing characterIndex is enough.

done.

>> Source/WebCore/platform/graphics/chromium/UniscribeHelper.cpp:230
>> +                            additionalSpaceInRun = 0;
> 
> If |hasExtraSpacing| isn't needed, you can put return here.

changed this for loop in this direction.

>> LayoutTests/platform/chromium-win/fast/text/arabic_justify.html:4
>> +<title>Issue 121265</title>
> 
> The title is not descriptive. Please consider more precise title (or just remove title tag).

removed.

>> LayoutTests/platform/chromium-win/fast/text/arabic_justify.html:6
>> +<body>
> 
> Please add a description of how below text should be rendered.

done.

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