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 Rendering | Assignee: | Jungshik Shin <jshin> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
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 | ||||||||||||
Attachments: |
|
Description
Jungshik Shin
2010-04-27 15:17:19 PDT
Oops. it's http://crbug.com/36752 Created attachment 54471 [details]
patch
Created attachment 54472 [details]
patch (same as before with copyright year updated)
Looks sane to me. CCing txt and windows folks though. 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 on attachment 54472 [details]
patch (same as before with copyright year updated)
WebCore/platform/graphics/chromium/FontUtilsChromiumWin.cpp:51
+ fontName);
Nit: you don't need to fold "fontName);".
WebCore/platform/graphics/chromium/FontUtilsChromiumWin.cpp:112
+ struct FontMap2 {
FontMap2 is not a meaningful name.
The name should contain "families" or "family list".
WebCore/platform/graphics/chromium/FontUtilsChromiumWin.cpp:150
+ 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?
e.g.
const UChar* malayalamFamilies[] = {L"AnjaliOldLipi", L"Kartika", L"Rachana", 0}
const static FontMap2 fontMap2[] = {
{USCRIPT_MALAYALAM, malayalamFamilies},
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.
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 on attachment 58201 [details]
patch updated
WebCore/ChangeLog:5
+ Make it possible to specify a list of fonts for per-script
Please add the bug URL in the ChangeLog entry.
WebCore/platform/graphics/chromium/FontUtilsChromiumWin.cpp:168
+ while (*familyPtr != 0) {
Remove "!=0".
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 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 on attachment 58225 [details]
updated patch for commit (with style issues addressed)
re-set r+.
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. Sorry for the mess and thank you for r+ again. Next time in the same situation, I'll just commit myself. 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> All reviewed patches have been landed. Closing bug. |