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

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Xiaomei Ji]

GDI used to do Kashida justification.
After switch to Skia, we need to use space padding to justify the text (assume Skia can not do Kashida justification -- stretching words).

related chromium bug 
http://code.google.com/p/chromium/issues/detail?id=121265

[Xiaomei Ji]

Created attachment 135716 [details]
patch w/ layout test

the patch mostly matches justify in WebKit's UniscribeController::shapeAndPlaceItem.

But there are 2 things that I do not understand:
1. justify() is called per run, I think. Then, inside justify(), can we work justification on m_runs?, they should not be the same unless m_runs.size() always equals to 1. But the rendered results looks ok on the page I tested. I need to look into further.
2. in UniscribeController::shapeAndPlaceItem, the mapping of spaceCharacter is assigned as:
spaceCharacters[clusters[k]] = m_currentCharacter + k + item.iCharPos;
I do not understand why it is not "spaceCharacters[clusters[k]] = k + item.iCharPos;"

[Kent Tamura]

Bashi-san, can you do informal review for the patch?

[Xiaomei Ji]

> But there are 2 things that I do not understand:
> 1. justify() is called per run, I think. Then, inside justify(), can we work justification on m_runs?, they should not be the same unless m_runs.size() always equals to 1. But the rendered results looks ok on the page I tested. I need to look into further.

UniscribeHelper::m_runs is the SCRIPT_ITEM arrays generated by ScriptItemize against TextRun, so the logical should be fine.

[WebKit Review Bot]

Comment on attachment 135716 [details]
patch w/ layout test

Attachment 135716 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12335046

New failing tests:
platform/chromium-win/fast/text/arabic_justify.html

[WebKit Review Bot]

Created attachment 135731 [details]
Archive of layout-test-results from ec2-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Xiaomei Ji]

Comment on attachment 135716 [details]
patch w/ layout test

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.

[Xiaomei Ji]

(In reply to comment #5)
> Created an attachment (id=135731) [details]
> Archive of layout-test-results from ec2-cr-linux-02
> 
> The attached test failures were seen while running run-webkit-tests on the chromium-ews.
> Bot: ec2-cr-linux-02  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

hm..  the test is under platform/chromium-win, why it is run in cr-linux?

[Kenichi Ishibashi]

Comment on attachment 135716 [details]
patch w/ layout test

View in context: https://bugs.webkit.org/attachment.cgi?id=135716&action=review

Did you run Tools/Scripts/new-run-webkit-tests --chromium? There were no failure?

>> 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.

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

run -> runIndex?

> Source/WebCore/platform/graphics/chromium/UniscribeHelper.cpp:216
> +            padPerSpace = additionalSpaceInRun / numSpaces;

You can use:
int padPerSpace = numSpaces ? additionalSpaceInRun / numSpaces : 0;

> 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.

> Source/WebCore/platform/graphics/chromium/UniscribeHelper.cpp:223
> +                if (characterIndex != -1) {

if (spaceCharacters[i] != -1) and removing characterIndex is enough.

> Source/WebCore/platform/graphics/chromium/UniscribeHelper.cpp:230
> +                            additionalSpaceInRun = 0;

If |hasExtraSpacing| isn't needed, you can put return here.

> LayoutTests/platform/chromium-win/fast/text/arabic_justify.html:1
> +<!DOCTYPE html>

Please remove the character(s) before '<'.

> 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).

> LayoutTests/platform/chromium-win/fast/text/arabic_justify.html:6
> +<body>

Please add a description of how below text should be rendered.

[Xiaomei Ji]

Comment on attachment 135716 [details]
patch w/ layout test

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.

[Xiaomei Ji]

Created attachment 135775 [details]
patch w/ layout test

[Xiaomei Ji]

Comment on attachment 135775 [details]
patch w/ layout test

View in context: https://bugs.webkit.org/attachment.cgi?id=135775&action=review

> LayoutTests/platform/chromium-win/fast/text/international/arabic-justify.html:1
> +<!DOCTYPE html>

it is BOM, should be ok.

[Kenichi Ishibashi]

Comment on attachment 135716 [details]
patch w/ layout test

View in context: https://bugs.webkit.org/attachment.cgi?id=135716&action=review

>>> 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.

Ok. Let me make sure if I understood right. Suppose there are two rtl runs like:

ab|cde

I think values of related variables are:
m_runs[0].iCharPos = 0, m_shapes[0].charLength() = 2
m_runs[1].iCharPos = 2, m_shapes[1].charLength() = 3
m_screenOrder = [1,0]

In this case, the first loop will access the first three characters of m_input. Is this intentional? or is my understanding wrong?
I glanced UniscribeHelper::draw() and UniscribeHelper::xToCharacter() and could see m_runs and m_shapes are accessed via the same index parameter.

[WebKit Review Bot]

Comment on attachment 135775 [details]
patch w/ layout test

Attachment 135775 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12341238

New failing tests:
platform/chromium-win/fast/text/international/arabic-justify.html

[WebKit Review Bot]

Created attachment 135782 [details]
Archive of layout-test-results from ec2-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Xiaomei Ji]

Comment on attachment 135716 [details]
patch w/ layout test

View in context: https://bugs.webkit.org/attachment.cgi?id=135716&action=review

>>>> 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.
> 
> Ok. Let me make sure if I understood right. Suppose there are two rtl runs like:
> 
> ab|cde
> 
> I think values of related variables are:
> m_runs[0].iCharPos = 0, m_shapes[0].charLength() = 2
> m_runs[1].iCharPos = 2, m_shapes[1].charLength() = 3
> m_screenOrder = [1,0]
> 
> In this case, the first loop will access the first three characters of m_input. Is this intentional? or is my understanding wrong?
> I glanced UniscribeHelper::draw() and UniscribeHelper::xToCharacter() and could see m_runs and m_shapes are accessed via the same index parameter.

Yes, you are right! Changed.

[Xiaomei Ji]

Created attachment 135946 [details]
patch w/ layout test

The patch only touches UniscirbeHelper.cpp, I have no idea why it causes failure in cr-linux. Is it possible that cr-linux failure is bogus?

[Xiaomei Ji]

Comment on attachment 135946 [details]
patch w/ layout test

View in context: https://bugs.webkit.org/attachment.cgi?id=135946&action=review

> Source/WebCore/platform/graphics/chromium/UniscribeHelper.cpp:198
> +        Shaping& shaping = m_shapes[run];

I also fixed a bug here. since spacing and advancing are done visually from left to right. We should using visual order here instead of treating 'run' as logical index and doing m_shapes[m_screenOrder[run]]. It might be better to change 'run' to 'visualIndex', and I will see whether I can have a simple case reduction.

[Kent Tamura]

(In reply to comment #16)
> The patch only touches UniscirbeHelper.cpp, I have no idea why it causes failure in cr-linux. Is it possible that cr-linux failure is bogus?

We run all of tests including platform/<others> unless tests are skipped.

We had better add
  WONTFIX LINUX MAC : platform/chromium-win = PASS FAIL
to the "WONTFIX TESTS" section of test_expectations.txt.

[Kenichi Ishibashi]

Comment on attachment 135946 [details]
patch w/ layout test

View in context: https://bugs.webkit.org/attachment.cgi?id=135946&action=review

LGTM. Thank you for working on this!
Kent-san, could you do formal-review?

>> Source/WebCore/platform/graphics/chromium/UniscribeHelper.cpp:198
>> +        Shaping& shaping = m_shapes[run];
> 
> I also fixed a bug here. since spacing and advancing are done visually from left to right. We should using visual order here instead of treating 'run' as logical index and doing m_shapes[m_screenOrder[run]]. It might be better to change 'run' to 'visualIndex', and I will see whether I can have a simple case reduction.

Ok. (I'm not sure how justification should work for RTL text, though)

> Source/WebCore/platform/graphics/chromium/UniscribeHelper.cpp:207
> +                spaceCharacters[shaping.m_logs[i]] =  characterIndex;

nit: two spaces after '='.

[WebKit Review Bot]

Comment on attachment 135946 [details]
patch w/ layout test

Attachment 135946 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12337606

New failing tests:
platform/chromium-win/fast/text/international/arabic-justify.html

[WebKit Review Bot]

Created attachment 135967 [details]
Archive of layout-test-results from ec2-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Xiaomei Ji]

Created attachment 136121 [details]
patch w/ layout test

sorry that I found another 2 bugs in the CL.
1. the numSpaces should be counted within one TextRun, across m_runs so that padding are evenly distributed inside TextRun.
2. sine shaping.m_justify is used not only in draw() but also in advanceForItem(), I am keeping m_justify here with the value adjusted to account for extra spacing.

related test is added.

[Xiaomei Ji]

Comment on attachment 136121 [details]
patch w/ layout test

View in context: https://bugs.webkit.org/attachment.cgi?id=136121&action=review

> Source/WebCore/platform/graphics/chromium/UniscribeHelper.cpp:219
> +        shaping.m_justify.resize(shaping.glyphLength());

Ah, for performance improvement, I could do a memcpy on shaping.m_justify and only re-assign m_justify for space character in the following loop and the loop could early exit when additionalSpace ==0.

[Kenichi Ishibashi]

Comment on attachment 136121 [details]
patch w/ layout test

View in context: https://bugs.webkit.org/attachment.cgi?id=136121&action=review

LGTM if all layout tests are passed.

>> Source/WebCore/platform/graphics/chromium/UniscribeHelper.cpp:219
>> +        shaping.m_justify.resize(shaping.glyphLength());
> 
> Ah, for performance improvement, I could do a memcpy on shaping.m_justify and only re-assign m_justify for space character in the following loop and the loop could early exit when additionalSpace ==0.

To do it, I think you can "shaping.m_justify = shaping.m_advance" here.

[Kent Tamura]

Comment on attachment 136121 [details]
patch w/ layout test

Rubber stamped.

[Xiaomei Ji]

Created attachment 136360 [details]
patch w/ layout test

tkent, PTAL.

[Xiaomei Ji]

Comment on attachment 136360 [details]
patch w/ layout test

View in context: https://bugs.webkit.org/attachment.cgi?id=136360&action=review

> LayoutTests/platform/chromium/test_expectations.txt:3982
> +BUGWK83227 : fast/text/international/arabic-justify.html = PASS MISSING

I will rebase for cr-linux. I think cr-mac should pass. Then, is this the right format?

[Kent Tamura]

Comment on attachment 136360 [details]
patch w/ layout test

View in context: https://bugs.webkit.org/attachment.cgi?id=136360&action=review

>> LayoutTests/platform/chromium/test_expectations.txt:3982
>> +BUGWK83227 : fast/text/international/arabic-justify.html = PASS MISSING
> 
> I will rebase for cr-linux. I think cr-mac should pass. Then, is this the right format?

I don't think so.
You don't need to add PASS platforms, and chromium-linux looks chromium-win results as fallback. so,
BUGWK83227 LINUX : fast/text/international/arabic-justify.html = FAIL

[Xiaomei Ji]

Created attachment 136483 [details]
patch w/ layout test

changed chromium test_expectation accordingly.

[Kent Tamura]

Comment on attachment 136483 [details]
patch w/ layout test

View in context: https://bugs.webkit.org/attachment.cgi?id=136483&action=review

> LayoutTests/platform/chromium/test_expectations.txt:3982
>  BUGWK83185 MAC DEBUG : fast/writing-mode/relative-positioning-percentages.html = CRASH
> +
> +BUGWK83227 LINUX : fast/text/international/arabic-justify.html = FAIL

I don't recommend appending an entry to the end of the file.  It easily makes patch conflict.

[Xiaomei Ji]

Created attachment 136727 [details]
patch w/ layout test

per tkent's suggestion, move the change in chromium's test_expectation.txt out of end of file to avoid conflict.

[WebKit Review Bot]

Comment on attachment 136727 [details]
patch w/ layout test

Rejecting attachment 136727 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1

[e-mail] found in /mnt/git/webkit-commit-queue/LayoutTests/ChangeLog does not appear to be a valid reviewer according to committers.py.
ERROR: /mnt/git/webkit-commit-queue/LayoutTests/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://queues.webkit.org/results/12383955

[Xiaomei Ji]

Created attachment 136736 [details]
patch w/ layout test

fix ChangeLog

[WebKit Review Bot]

Comment on attachment 136736 [details]
patch w/ layout test

Clearing flags on attachment: 136736

Committed r113912: <http://trac.webkit.org/changeset/113912>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[James Simonsen]

This has been rolled out. It broke atsui-spacing-features.html on Chromium-win. The last latin word is no longer justified. It should match the green box at the bottom.

There are also changes to justify-ideograph-leading-expansion.html, but I'm not sure if they're good or bad. You should verify those against the Mac port, which generally does a better job on these tests than we do.

[Xiaomei Ji]

Created attachment 136834 [details]
patch w/ layout test

I still use Window's ScriptJustify but disabled kashida justification when using skia to draw.

tkent, bashi,
PTAL again, Thanks!

[Kenichi Ishibashi]

(In reply to comment #37)
> Created an attachment (id=136834) [details]
> patch w/ layout test
> 
> I still use Window's ScriptJustify but disabled kashida justification when using skia to draw.
> 
> tkent, bashi,
> PTAL again, Thanks!

According to the blog post mentioned in the comment, the patch looks good. However, I'm not sure how this work and worry about regressions. Did you confirm that the patch doesn't cause any regression by running new-run-layout-tests?

[Xiaomei Ji]

> According to the blog post mentioned in the comment, the patch looks good. However, I'm not sure how this work and worry about regressions. Did you confirm that the patch doesn't cause any regression by running new-run-layout-tests?

I tried ran run_webkit_tests.bat locally which hangs if I did not run test manually one-by-one.
I tried to submit the CL to chromium's win_layout try bot, but that try bot is unmaintained and it is always failed in update.

Also, it is very weird that I am not able to generate png for the previous failed test -- atsui-spacing-features.html. I use the same command, and it could generate the png result for other tests I tried. But "./run_webkit_tests.bat --debug --pixel-tests fast/text/atsui-spacing-features.html" reported "1 tests ran as expected" locally (without comparing pixel result).

I will try new-run-layout-tests in windows.

[Xiaomei Ji]

(In reply to comment #39)
> > According to the blog post mentioned in the comment, the patch looks good. However, I'm not sure how this work and worry about regressions. Did you confirm that the patch doesn't cause any regression by running new-run-layout-tests?
> 
> I tried ran run_webkit_tests.bat locally which hangs if I did not run test manually one-by-one.
> I tried to submit the CL to chromium's win_layout try bot, but that try bot is unmaintained and it is always failed in update.
> 
> Also, it is very weird that I am not able to generate png for the previous failed test -- atsui-spacing-features.html. I use the same command, and it could generate the png result for other tests I tried. But "./run_webkit_tests.bat --debug --pixel-tests fast/text/atsui-spacing-features.html" reported "1 tests ran as expected" locally (without comparing pixel result).
> 
> I will try new-run-layout-tests in windows.

Theoretically, the patch should work. For non-arabic page, it should do things as before. As to arabic page that triggers kashida justification, we turned it off. The previous patch has problem of not exactly justifying text, because I use paddingPerSpace = additionalSpace / numOfSpaces, if additionalSpace % numOfSpaces != 0, it is not fully justified. It should be able to further tune. But I think using Window's native should works better and consistent with previous behavior.

I tried run run_webkit_tests.bat (http://www.chromium.org/developers/testing/webkit-layout-tests). Seems that it hangs on those failed test in Windows (specified in test_expectations.txt). So, I only ran those tests under fast/text that having 'justify' style specified (exclude the 6 failed tests that cause run_webkit_tests hang). They pass.

Tried following http://dev.chromium.org/developers/contributing-to-webkit in Windows, but I got a lot of missing file warning when running "update-webkit --chromium'. And I am giving it up.

[Kenichi Ishibashi]

> Tried following http://dev.chromium.org/developers/contributing-to-webkit in Windows, but I got a lot of missing file warning when running "update-webkit --chromium'. And I am giving it up.

I think you need not to run update-webkit. Building pull_in_DumpRenderTree target (in src/webkit/webkit.sln) creates DRT. The code itself looks good to me.

[Xiaomei Ji]

(In reply to comment #41)
> > Tried following http://dev.chromium.org/developers/contributing-to-webkit in Windows, but I got a lot of missing file warning when running "update-webkit --chromium'. And I am giving it up.
> 
> I think you need not to run update-webkit. Building pull_in_DumpRenderTree target (in src/webkit/webkit.sln) creates DRT. The code itself looks good to me.

fast/text pass with the CL (run with run-webkit-tests).

[Kent Tamura]

Comment on attachment 136834 [details]
patch w/ layout test

rubber-stamped

[WebKit Review Bot]

Comment on attachment 136834 [details]
patch w/ layout test

Clearing flags on attachment: 136834

Committed r114267: <http://trac.webkit.org/changeset/114267>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Xiaomei Ji]

Created attachment 137373 [details]
rebase