Bug 25464 – Improve font fallback for 'common' characters in Chromium Win port

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

[Jungshik Shin]

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.

[Jungshik Shin]

Created attachment 29946 [details]
patch

[Jungshik Shin]

Brett, can you help Dimitri review the patch? Thanks.

[Dimitri Glazkov (Google)]

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.

[Jungshik Shin]

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.  

[Dimitri Glazkov (Google)]

Ah, I didn't see anonymous namespace in the context of the patch. LGTM then.

[Jungshik Shin]

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.

[Jungshik Shin]

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+.

[Eric Seidel (no email)]

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