Bug 25464 - Improve font fallback for 'common' characters in Chromium Win port
Summary: Improve font fallback for 'common' characters in Chromium Win port
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Windows XP
: P2 Normal
Assignee: Jungshik Shin
URL:
Keywords:
Depends on:
Blocks: 25816 113169
  Show dependency treegraph
 
Reported: 2009-04-28 17:45 PDT by Jungshik Shin
Modified: 2013-03-24 20:42 PDT (History)
2 users (show)

See Also:


Attachments
patch (11.31 KB, patch)
2009-05-01 13:39 PDT, Jungshik Shin
dglazkov: review+
Details | Formatted Diff | Diff
patch : same as before except for one indentation change (2->4) (11.31 KB, patch)
2009-05-05 12:53 PDT, Jungshik Shin
no flags Details | Formatted Diff | Diff
same as the previous except the test location (11.74 KB, patch)
2009-05-07 13:37 PDT, Jungshik Shin
jshin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jungshik Shin 2009-04-28 17:45:53 PDT
This is to fix Chromium bug ( http://crbug.com/3536 http://crbug.com/1716 and related issues not yet reported). 

I'll upload the patch (at http://codereview.chromium.org/99146) along with a layout test/result.
Comment 1 Jungshik Shin 2009-05-01 13:39:25 PDT
Created attachment 29946 [details]
patch
Comment 2 Jungshik Shin 2009-05-02 09:53:41 PDT
Brett, can you help Dimitri review the patch? Thanks.
Comment 3 Dimitri Glazkov (Google) 2009-05-05 11:05:32 PDT
Comment on attachment 29946 [details]
patch 

Looks good except for style comments:

> +UScriptCode getScriptBasedOnUnicodeBlock(int ucs4)

Add static modifier.

> +UScriptCode getScript(int ucs4)

Ditto.

> +    if (script <= USCRIPT_INHERITED || U_FAILURE(err))
> +      script = getScriptBasedOnUnicodeBlock(ucs4);

4 spaces.
Comment 4 Jungshik Shin 2009-05-05 12:17:03 PDT
Thank you for the review. 
(In reply to comment #3)
> (From update of attachment 29946 [details] [review])
> Looks good except for style comments:
> 
> > +UScriptCode getScriptBasedOnUnicodeBlock(int ucs4)
> 
> Add static modifier.
> 
> > +UScriptCode getScript(int ucs4)
> 
> Ditto.

They're currently inside an anonymous namespace. I'm aware that WebKit does not use anonymous namespace elsewhere. Do you think we have to follow the convention in this file? 

> > +    if (script <= USCRIPT_INHERITED || U_FAILURE(err))
> > +      script = getScriptBasedOnUnicodeBlock(ucs4);
> 
> 4 spaces.

I'll upload a check-in-ready patch once I hear back about the namespace issue above.  

Comment 5 Dimitri Glazkov (Google) 2009-05-05 12:20:09 PDT
Ah, I didn't see anonymous namespace in the context of the patch. LGTM then.
Comment 6 Jungshik Shin 2009-05-05 12:53:22 PDT
Created attachment 30031 [details]
patch : same as before except for one indentation change (2->4)

Thanks again for review. This is ready for check-in.
Comment 7 Jungshik Shin 2009-05-07 13:37:42 PDT
Created attachment 30115 [details]
same as the previous except the test location

I moved two tests I'm adding to fast/text/international from fast/text.
Transferring r+.
Comment 8 Eric Seidel (no email) 2009-05-15 00:50:00 PDT
Your patch was missing pixel results, but I added them before committing.

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	LayoutTests/ChangeLog
	A	LayoutTests/fast/text/international/danda-space.html
	A	LayoutTests/fast/text/international/thai-baht-space.html
	A	LayoutTests/platform/mac/fast/text/international/danda-space-expected.checksum
	A	LayoutTests/platform/mac/fast/text/international/danda-space-expected.png
	A	LayoutTests/platform/mac/fast/text/international/danda-space-expected.txt
	A	LayoutTests/platform/mac/fast/text/international/thai-baht-space-expected.checksum
	A	LayoutTests/platform/mac/fast/text/international/thai-baht-space-expected.png
	A	LayoutTests/platform/mac/fast/text/international/thai-baht-space-expected.txt
	M	WebCore/ChangeLog
	M	WebCore/platform/graphics/chromium/FontUtilsChromiumWin.cpp
Committed r43759