Bug 83523 – REGRESSION(r102190) [chromium] text rendering (font and font size) in some Arabic/Persian page is wrong when using certain fonts

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Xiaomei Ji]

Created attachment 136334 [details]
test case

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

Looks like reverse r102190 fixes the problem.

As to the issue 73806 (https://bugs.webkit.org/show_bug.cgi?id=73806) which r102190 fixes, should the correct fix be the following?

Index: UniscribeHelper.cpp
===================================================================
--- UniscribeHelper.cpp (revision 108619)
+++ UniscribeHelper.cpp (working copy)
@@ -902,6 +921,7 @@
                 // the difference in the width of the glyph.
                 shaping.m_advance[glyphIndex] -= diff;
                 shaping.m_abc.abcB -= diff;
+                shaping.m_glyphs[glyphIndex] = shaping.m_spaceGlyph;
                 continue;
             }

[Xiaomei Ji]

Created attachment 136354 [details]
WIP

The source code change is completed.
But I have a question regarding to layout test.

The problem only reproduce when the 1st choice font does not render 0x000A. I used a webfont in the layout test, and the test file rendered correctly with the CL.
But I am not able to run the same test as layout test. It always timed out even I increase --time-out-ms dramatically. I guess our DRT does not support webfont loading?

Then, what would be the best way to write a test case?
I do not think I can submit the webfont due to licence issue.
Do I have to create my own Arabic font that can not render 0x000A?

[Kent Tamura]

(In reply to comment #1)
> But I am not able to run the same test as layout test. It always timed out even I increase --time-out-ms dramatically. I guess our DRT does not support webfont loading?

DRT doesn't allow to access to external web sites.  We need a local copy of a web font.

[Kenichi Ishibashi]

Comment on attachment 136354 [details]
WIP

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

> LayoutTests/ChangeLog:9
> +        * fast/text/international/no-break-space-in-arabic.html: Added.

Could you attach an expected result? Otherwise, we can't examine the patch is reasonable.

> LayoutTests/fast/text/international/no-break-space-in-arabic.html:6
> +/* font face*/

nit: This comment says nothing. Please remove it.

> LayoutTests/fast/text/international/no-break-space-in-arabic.html:8
> +font-family:"Mitra";

nit: inappropriate indentation.

> LayoutTests/fast/text/international/no-break-space-in-arabic.html:9
> +src: url("http://arashm.net/blog/wp-content/themes/arash/fonts/BMitra.eot");

As kent-san said, we can't access external site in layout tests. Does this regression occur only when using these fonts?
(FYI chromium doesn't support eot file)

[Kenichi Ishibashi]

(In reply to comment #1)
> The problem only reproduce when the 1st choice font does not render 0x000A. I used a webfont in the layout test, and the test file rendered correctly with the CL.

Ah, I didn't read this comment before writing the previous comment. That sounds adding a test is difficult. Of course you can create your font for the test, but I'm not sure it's worth to do.

[WebKit Review Bot]

Comment on attachment 136354 [details]
WIP

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

New failing tests:
fast/text/international/no-break-space-in-arabic.html

[WebKit Review Bot]

Created attachment 136406 [details]
Archive of layout-test-results from ec2-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01  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 136555 [details]
patch

tkent, is it acceptable without test?
Again, I have no idea why cr-linux failed.

[Xiaomei Ji]

Created attachment 136587 [details]
patch w/ layout test

this is the patch with layout test. 
I am changing DejaVu font in Linux (remove 0xA0 support there) for the test purpose.
But I am not very clear on DejaVu's license (whether it is compatible with LGPL or BSD).
(dejavu-fonts.org/wiki/index.php?title=License).

[Xiaomei Ji]

I am thinking to check-in the code first, with a follow-up patch on checking in the layout test (need to create an Arabic font without glyph of 0xA0, and need to learn how to create it from scratch).

[Kenichi Ishibashi]

(In reply to comment #9)
> I am thinking to check-in the code first, with a follow-up patch on checking in the layout test (need to create an Arabic font without glyph of 0xA0, and need to learn how to create it from scratch).

That sounds good to me.

[Kent Tamura]

Comment on attachment 136555 [details]
patch

rubber-stamped

[WebKit Review Bot]

Comment on attachment 136555 [details]
patch

Clearing flags on attachment: 136555

Committed r114187: <http://trac.webkit.org/changeset/114187>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.