Bug 83227 - [chromium] wrong justification for arabic/persian page in cr-win
Summary: [chromium] wrong justification for arabic/persian page in cr-win
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 83727
Blocks:
  Show dependency treegraph
 
Reported: 2012-04-04 15:26 PDT by Xiaomei Ji
Modified: 2012-04-16 12:12 PDT (History)
9 users (show)

See Also:


Attachments
patch w/ layout test (12.41 KB, patch)
2012-04-04 16:33 PDT, Xiaomei Ji
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-02 (6.37 MB, application/zip)
2012-04-04 17:50 PDT, WebKit Review Bot
no flags Details
patch w/ layout test (28.22 KB, patch)
2012-04-05 01:05 PDT, Xiaomei Ji
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-03 (6.38 MB, application/zip)
2012-04-05 01:51 PDT, WebKit Review Bot
no flags Details
patch w/ layout test (28.17 KB, patch)
2012-04-05 17:39 PDT, Xiaomei Ji
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-03 (6.65 MB, application/zip)
2012-04-05 19:11 PDT, WebKit Review Bot
no flags Details
patch w/ layout test (19.81 KB, patch)
2012-04-06 19:29 PDT, Xiaomei Ji
tkent: review+
Details | Formatted Diff | Diff
patch w/ layout test (71.22 KB, patch)
2012-04-09 18:15 PDT, Xiaomei Ji
tkent: review-
Details | Formatted Diff | Diff
patch w/ layout test (71.22 KB, patch)
2012-04-10 10:26 PDT, Xiaomei Ji
tkent: review+
Details | Formatted Diff | Diff
patch w/ layout test (71.20 KB, patch)
2012-04-11 12:48 PDT, Xiaomei Ji
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
patch w/ layout test (71.18 KB, patch)
2012-04-11 13:19 PDT, Xiaomei Ji
no flags Details | Formatted Diff | Diff
patch w/ layout test (76.56 KB, patch)
2012-04-11 23:42 PDT, Xiaomei Ji
no flags Details | Formatted Diff | Diff
rebase (178.47 KB, patch)
2012-04-16 12:12 PDT, Xiaomei Ji
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Xiaomei Ji 2012-04-04 15:26:37 PDT
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
Comment 1 Xiaomei Ji 2012-04-04 16:33:30 PDT
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;"
Comment 2 Kent Tamura 2012-04-04 16:38:18 PDT
Bashi-san, can you do informal review for the patch?
Comment 3 Xiaomei Ji 2012-04-04 17:12:25 PDT
> 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.
Comment 4 WebKit Review Bot 2012-04-04 17:49:45 PDT
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
Comment 5 WebKit Review Bot 2012-04-04 17:50:03 PDT
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
Comment 6 Xiaomei Ji 2012-04-04 19:17:21 PDT
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.
Comment 7 Xiaomei Ji 2012-04-04 19:17:57 PDT
(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?
Comment 8 Kenichi Ishibashi 2012-04-04 22:45:02 PDT
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.
Comment 9 Xiaomei Ji 2012-04-05 00:53:49 PDT
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.
Comment 10 Xiaomei Ji 2012-04-05 01:05:53 PDT
Created attachment 135775 [details]
patch w/ layout test
Comment 11 Xiaomei Ji 2012-04-05 01:08:50 PDT
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.
Comment 12 Kenichi Ishibashi 2012-04-05 01:44:28 PDT
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.
Comment 13 WebKit Review Bot 2012-04-05 01:51:15 PDT
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
Comment 14 WebKit Review Bot 2012-04-05 01:51:21 PDT
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
Comment 15 Xiaomei Ji 2012-04-05 17:36:04 PDT
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.
Comment 16 Xiaomei Ji 2012-04-05 17:39:49 PDT
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?
Comment 17 Xiaomei Ji 2012-04-05 17:46:50 PDT
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.
Comment 18 Kent Tamura 2012-04-05 17:53:03 PDT
(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.
Comment 19 Kenichi Ishibashi 2012-04-05 17:55:02 PDT
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 '='.
Comment 20 WebKit Review Bot 2012-04-05 19:11:51 PDT
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
Comment 21 WebKit Review Bot 2012-04-05 19:11:57 PDT
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
Comment 22 Xiaomei Ji 2012-04-06 19:29:26 PDT
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.
Comment 23 Xiaomei Ji 2012-04-07 00:54:53 PDT
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.
Comment 24 Kenichi Ishibashi 2012-04-08 18:43:11 PDT
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.
Comment 25 Kent Tamura 2012-04-09 17:59:53 PDT
Comment on attachment 136121 [details]
patch w/ layout test

Rubber stamped.
Comment 26 Xiaomei Ji 2012-04-09 18:15:06 PDT
Created attachment 136360 [details]
patch w/ layout test

tkent, PTAL.
Comment 27 Xiaomei Ji 2012-04-09 18:16:12 PDT
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?
Comment 28 Kent Tamura 2012-04-09 18:28:42 PDT
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
Comment 29 Xiaomei Ji 2012-04-10 10:26:22 PDT
Created attachment 136483 [details]
patch w/ layout test

changed chromium test_expectation accordingly.
Comment 30 Kent Tamura 2012-04-10 16:26:20 PDT
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.
Comment 31 Xiaomei Ji 2012-04-11 12:48:32 PDT
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.
Comment 32 WebKit Review Bot 2012-04-11 12:55:25 PDT
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

tkent@chromium.org 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
Comment 33 Xiaomei Ji 2012-04-11 13:19:15 PDT
Created attachment 136736 [details]
patch w/ layout test

fix ChangeLog
Comment 34 WebKit Review Bot 2012-04-11 14:43:16 PDT
Comment on attachment 136736 [details]
patch w/ layout test

Clearing flags on attachment: 136736

Committed r113912: <http://trac.webkit.org/changeset/113912>
Comment 35 WebKit Review Bot 2012-04-11 14:43:48 PDT
All reviewed patches have been landed.  Closing bug.
Comment 36 James Simonsen 2012-04-11 16:23:16 PDT
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.
Comment 37 Xiaomei Ji 2012-04-11 23:42:19 PDT
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!
Comment 38 Kenichi Ishibashi 2012-04-12 00:22:44 PDT
(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?
Comment 39 Xiaomei Ji 2012-04-12 10:42:54 PDT
> 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.
Comment 40 Xiaomei Ji 2012-04-13 00:03:53 PDT
(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.
Comment 41 Kenichi Ishibashi 2012-04-13 03:57:03 PDT
> 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.
Comment 42 Xiaomei Ji 2012-04-13 17:16:25 PDT
(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).
Comment 43 Kent Tamura 2012-04-14 18:33:18 PDT
Comment on attachment 136834 [details]
patch w/ layout test

rubber-stamped
Comment 44 WebKit Review Bot 2012-04-16 10:04:02 PDT
Comment on attachment 136834 [details]
patch w/ layout test

Clearing flags on attachment: 136834

Committed r114267: <http://trac.webkit.org/changeset/114267>
Comment 45 WebKit Review Bot 2012-04-16 10:04:36 PDT
All reviewed patches have been landed.  Closing bug.
Comment 46 Xiaomei Ji 2012-04-16 12:12:12 PDT
Created attachment 137373 [details]
rebase