Bug 4844 - Render "simple" Hebrew using the CG codepath
Summary: Render "simple" Hebrew using the CG codepath
Status: VERIFIED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 420+
Hardware: Mac OS X 10.4
: P3 Normal
Assignee: mitz
URL:
Keywords:
Depends on: 4940 5166
Blocks:
  Show dependency treegraph
 
Reported: 2005-09-04 10:23 PDT by mitz
Modified: 2005-12-27 22:32 PST (History)
0 users

See Also:


Attachments
Exclude Hebrew letters and maqaf from ATSUI rendering (1.39 KB, patch)
2005-09-04 22:55 PDT, mitz
no flags Details | Formatted Diff | Diff
patch expected layout test results (17.20 KB, patch)
2005-09-05 12:44 PDT, mitz
no flags Details | Formatted Diff | Diff
updated expected pixel test results (105.69 KB, application/octet-stream)
2005-09-05 13:09 PDT, mitz
no flags Details
Exclude Hebrew letters and maqaf from ATSUI rendering (1.96 KB, patch)
2005-12-23 02:08 PST, mitz
mjs: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description mitz 2005-09-04 10:23:06 PDT
The CG codepath can handle Hebrew runs that don't contain niqqud or other "diacritics".

It seems that Hebrew was switched over to ATSUI over rdar://problem/3456887 , which was actually 
bug 3618 in the CG codepath, which is now fixed. While the CG codepath (still) can't handle combined 
characters and Hebrew presentation forms, it works fine for "simple" Hebrew. An overwhelming 
majority of Hebrew websites use nothing but "simple" Hebrew.

Advantages of using CG for "simple" Hebrew:
* improved performance
* support for layout features that are missing from the ATSUI codepath (bug 3922).

Disadvantage:
* if there's sporadic use of niqqud on a page, then adjacent text runs may end up using different 
codepaths. If, on top of that, the page uses text-align:justify (or word- or letter-spacing), the 
appearance will be weird. Considering that anything with text-align:justify already looks weird, I don't 
think this is a major issue.

The only code that needs to change is the range checking in shouldUseATSU().
Comment 1 mitz 2005-09-04 22:55:19 PDT
Created attachment 3768 [details]
Exclude Hebrew letters and maqaf from ATSUI rendering
Comment 2 mitz 2005-09-04 23:21:25 PDT
Comment on attachment 3768 [details]
Exclude Hebrew letters and maqaf from ATSUI rendering

This patch causes 4 bidi layout tests to fail with benign horizontal shifting
of text.
Comment 3 Darin Adler 2005-09-05 08:15:16 PDT
Comment on attachment 3768 [details]
Exclude Hebrew letters and maqaf from ATSUI rendering

Seems fine. r=me

Can we add some more Hebrew test cases to layout tests?
Comment 4 mitz 2005-09-05 12:44:43 PDT
Created attachment 3772 [details]
patch expected layout test results
Comment 5 mitz 2005-09-05 13:09:09 PDT
Created attachment 3773 [details]
updated expected pixel test results
Comment 6 mitz 2005-09-11 09:38:13 PDT
Comment on attachment 3768 [details]
Exclude Hebrew letters and maqaf from ATSUI rendering

Patch causes a problem where shouldUseATSU returns YES for a run (when passed
from=0, to=length) but NO for some of its subruns (typically words), because CG
and ATSU don't give the same width even for CG-safe runs.
Comment 7 mitz 2005-09-11 09:55:55 PDT
Of the Hebrew fonts I have, I'm seeing differences between CG and ATSU in width of Hebrew text only in 
Lucida Grande.
Comment 8 mitz 2005-09-24 06:34:27 PDT
Comment on attachment 3768 [details]
Exclude Hebrew letters and maqaf from ATSUI rendering

Now that ATSU does the rounding hack it's safe to make this change. I'm not
sure the updated test results are still valid, though.
Comment 9 Darin Adler 2005-09-24 06:56:03 PDT
Comment on attachment 3768 [details]
Exclude Hebrew letters and maqaf from ATSUI rendering

r=me -- we're assuming this won't affect any layout tests now
Comment 10 mitz 2005-09-25 06:36:16 PDT
(In reply to comment #9)

> r=me -- we're assuming this won't affect any layout tests now

Unfortunately, it will, for two reasons:
1. There can still be a <1px difference in the position of individual glyphs between CG and ATSU (since the 
CG code path applies the rounding in logical order and ATSU in left-to-right order), even though the total 
width remains the same.
2. The fix for bug 4940 didn't address fallback fonts that don't have the same rendering mode as the 
original font. This happens in tests that use Times (since the fallback is Lucida Grande).
Comment 11 Darin Adler 2005-09-25 22:05:37 PDT
Comment on attachment 3768 [details]
Exclude Hebrew letters and maqaf from ATSUI rendering

OK. Issue (1) is not a concern, but issue (2) is. I think we should put this on
hold again until that's dealt with.
Comment 12 mitz 2005-09-25 22:25:54 PDT
Comment on attachment 3768 [details]
Exclude Hebrew letters and maqaf from ATSUI rendering

> OK. Issue (1) is not a concern, but issue (2) is. I think we should put this on
> hold again until that's dealt with.

I think the exact opposite in this case. The first time I pulled the patch was
because it made things worse for lines mixing "simple" Hebrew with non-"simple"
Hebrew (say, one word with niqqud), since they went from all-ATSU lines to
mixed ATSU/CG, and there was a problem with mixed ATSU/CG.

Issue (2) currently affects (certain) mixed Latin/Hebrew lines, since they
currently mix ATSU with CG, and there is (still) a problem with mixed ATSU/CG,
but this time, changing Hebrew to CG will actually make things better for these
lines.
Comment 13 Darin Adler 2005-09-26 00:35:05 PDT
Comment on attachment 3768 [details]
Exclude Hebrew letters and maqaf from ATSUI rendering

OK.
Comment 14 mitz 2005-09-26 08:06:50 PDT
Comment on attachment 3768 [details]
Exclude Hebrew letters and maqaf from ATSUI rendering

I changed my mind (again). The phenomenon described in comment #6 does remain a
problem in too many cases, due to issue (2). Need to figure out a solution for
that. Sorry about the mess.
Comment 15 mitz 2005-12-23 02:08:04 PST
Created attachment 5240 [details]
Exclude Hebrew letters and maqaf from ATSUI rendering

At last, making this change really affects only rendering. Pixel results are
expected to change for the following tests:
fast/text/international/bidi-AN-after-L
fast/text/international/bidi-L2-run-reordering
fast/text/international/bidi-LDB-2-CSS
fast/text/international/bidi-LDB-2-HTML 
fast/text/international/bidi-LDB-2-formatting-characters
fast/text/international/bidi-explicit-embedding
fast/text/international/bidi-fallback-font-weight
fast/text/international/bidi-ignored-for-first-child-inline
fast/text/international/bidi-innertext
fast/text/international/bidi-layout-across-linebreakfast/text/international/bidi-override
Comment 16 Maciej Stachowiak 2005-12-25 03:15:01 PST
Comment on attachment 5240 [details]
Exclude Hebrew letters and maqaf from ATSUI rendering

Patch looks simple enough. I will take it on faith that it is conceptually
right as well.