Bug 38224

Summary: [chromium] Enable rendering of Ethiopic, Lao, Tibetan and a few other scripts on Win XP
Product: WebKit Reporter: Jungshik Shin <jshin>
Component: Layout and RenderingAssignee: Jungshik Shin <jshin>
Severity: Normal CC: aroben, brettw, commit-queue, eric, levin, mal, mitz, tkent, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Windows XP   
Description Flags
patch (same as before with copyright year updated)
patch updated
updated patch for commit (with style issues addressed) none

Description Jungshik Shin 2010-04-27 15:17:19 PDT
Currently, the last resort fallback font selection in Chromium Win is 'hard-coded'. For some scripts (Ethiopic, Lao, Tibetan, etc), those fonts listed are only available on Vista.  We're requested to release Chrome/Chromium in Ethiopic UI on Windows XP because it's the platform dominant in Ethiopia. 

To solve the problem for upcoming Chrome 5 release, I'm going to add a simple short-term  "fix" of going through multiple fonts (still hard-coded but having a high chance of availability) and picking the first one present on users' machines. While doing so, I do the same for some other scripts. 

This is for http://crbug.com/3752
Comment 1 Jungshik Shin 2010-04-27 15:17:41 PDT
Oops. it's http://crbug.com/36752
Comment 2 Jungshik Shin 2010-04-27 16:50:17 PDT
Created attachment 54471 [details]
Comment 3 Jungshik Shin 2010-04-27 16:53:40 PDT
Created attachment 54472 [details]
patch (same as before with copyright year updated)
Comment 4 Eric Seidel (no email) 2010-05-02 18:37:10 PDT
Looks sane to me. CCing txt and windows folks though.
Comment 5 Jungshik Shin 2010-05-05 10:58:06 PDT
Brett, can you take a quick look? If his nod and Eric's previous comment are sufficient for r+, it'd be great to get r+ from one of wk reviewers.
Comment 6 Kent Tamura 2010-05-28 04:12:48 PDT
Comment on attachment 54472 [details]
patch (same as before with copyright year updated)

 +                               fontName);
Nit: you don't need to fold "fontName);".

 +      struct FontMap2 {
FontMap2 is not a meaningful name.
The name should contain "families" or "family list".

 +      DEFINE_STATIC_LOCAL(Vector<Vector<String>* >, familyLists, ());
FontMap2 has a comma-separated value, the code parses it, and stores a static storage.  It is inefficient.
Why doesn't FontMap2 have a pointer to a string list?

const UChar* malayalamFamilies[] = {L"AnjaliOldLipi", L"Kartika", L"Rachana", 0}
const static FontMap2 fontMap2[] = {
    {USCRIPT_MALAYALAM, malayalamFamilies},
Comment 7 Jungshik Shin 2010-06-08 17:33:30 PDT
Created attachment 58201 [details]
patch updated 

I considered what tkent suggested and decided against it, but I don't remember what it was and I can't come up with any reason not to. So, I'm doing it with this patch.
Comment 8 WebKit Review Bot 2010-06-08 17:34:49 PDT
Attachment 58201 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/platform/graphics/chromium/FontUtilsChromiumWin.cpp:168:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 1 in 2 files

If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 9 Kent Tamura 2010-06-08 18:25:08 PDT
Comment on attachment 58201 [details]
patch updated 

 +          Make it possible to specify a list of fonts for per-script
Please add the bug URL in the ChangeLog entry.

 +          while (*familyPtr != 0) {
Remove "!=0".
Comment 10 Jungshik Shin 2010-06-09 02:20:48 PDT
Created attachment 58225 [details]
updated patch for commit (with style issues addressed)

@tkent, thank you for the review. Your concern was addressed and I'm carrying over r+.
Comment 11 WebKit Commit Bot 2010-06-09 02:25:48 PDT
Comment on attachment 58225 [details]
updated patch for commit (with style issues addressed)

Rejecting patch 58225 from review queue.

jshin@chromium.org does not have reviewer permissions according to http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/webkitpy/common/config/committers.py.

- If you do not have reviewer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have reviewer rights please correct the error in WebKitTools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed).  Due to bug 30084 the commit-queue will require a restart after your change.  Please contact eseidel@chromium.org to request a commit-queue restart.  After restart the commit-queue will correctly respect your reviewer rights.
Comment 12 Kent Tamura 2010-06-09 02:36:11 PDT
Comment on attachment 58225 [details]
updated patch for commit (with style issues addressed)

re-set r+.
Comment 13 Eric Seidel (no email) 2010-06-09 11:15:47 PDT
Comment on attachment 58201 [details]
patch updated 

Cleared Kent Tamura's review+ from obsolete attachment 58201 [details] so that this bug does not appear in http://webkit.org/pending-commit.
Comment 14 Jungshik Shin 2010-06-09 14:20:26 PDT
Sorry for the mess and thank you for r+ again.  Next time in the same situation, I'll just commit myself.
Comment 15 WebKit Commit Bot 2010-06-10 06:31:16 PDT
Comment on attachment 58225 [details]
updated patch for commit (with style issues addressed)

Clearing flags on attachment: 58225

Committed r60953: <http://trac.webkit.org/changeset/60953>
Comment 16 WebKit Commit Bot 2010-06-10 06:31:27 PDT
All reviewed patches have been landed.  Closing bug.