| Summary: | [chromium] wrong justification for arabic/persian page in cr-win | ||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Xiaomei Ji <xji> | ||||||||||||||||||||||||||||
| Component: | Platform | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||||
| Severity: | Normal | CC: | bashi, behdad, cc-bugs, dglazkov, jamesr, jshin, simonjam, tkent, webkit.review.bot | ||||||||||||||||||||||||||||
| Priority: | P2 | ||||||||||||||||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||||||||||||||||
| OS: | Unspecified | ||||||||||||||||||||||||||||||
| Bug Depends on: | 83727 | ||||||||||||||||||||||||||||||
| Bug Blocks: | |||||||||||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||||||||||
|
Description
Xiaomei Ji
2012-04-04 15:26:37 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;"
Bashi-san, can you do informal review for the patch? > 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 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 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 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. (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 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 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. Created attachment 135775 [details]
patch w/ layout test
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 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 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 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 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. 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 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. (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 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 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 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
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 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 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 on attachment 136121 [details]
patch w/ layout test
Rubber stamped.
Created attachment 136360 [details]
patch w/ layout test
tkent, PTAL.
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 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 Created attachment 136483 [details]
patch w/ layout test
changed chromium test_expectation accordingly.
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. 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 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 Created attachment 136736 [details]
patch w/ layout test
fix ChangeLog
Comment on attachment 136736 [details] patch w/ layout test Clearing flags on attachment: 136736 Committed r113912: <http://trac.webkit.org/changeset/113912> All reviewed patches have been landed. Closing bug. 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. 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!
(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? > 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.
(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. > 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.
(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 on attachment 136834 [details]
patch w/ layout test
rubber-stamped
Comment on attachment 136834 [details] patch w/ layout test Clearing flags on attachment: 136834 Committed r114267: <http://trac.webkit.org/changeset/114267> All reviewed patches have been landed. Closing bug. Created attachment 137373 [details]
rebase
|