288634 - non-ASCII font support in Gfx:Pango and port a couple of other changes in Gfx:Xft to Pango

Status: RESOLVED FIXED

(Core Graveyard :: GFX: Gtk, defect)

Description

[Jungshik Shin]

Since the fork of Gfx:Pango, there have been a few changes made in Gfx:Xft that
need to ported to Pango Gfx. I'm building a pango build with my patch. After
testing it, I'll upload a patch here.

Comment 1

[Jungshik Shin]

Attachment: patch

I got rid of some duplicated code (nsFontConfigUtils.* are compiled in without
this patch so that it's worth to use functions there instead of duping them in
nsFontMetricsPango.cpp).
As for non-ASCII font name support, somehow it doesn't work. Pango may have to
be updated as well? 
I haven't yet ported 'font-list' support.

Comment 2

[Jungshik Shin]

Comment on attachment 179348 [details] [diff] [review]
patch

>-    if (!IsASCIIFontName(aFamily))
>-        return PR_TRUE;
>+    NS_ConvertUTF16toUTF8 name(aFamily);
> 
>-    nsCAutoString name;
>+    // The newest fontconfig does the full Unicode case folding so that 
>+    // we're being lazy here by calling |ToLowerCase| after converting
>+    // to UTF-8  assuming that in virtually all cases, we just have to
>+    // fold [A-Z].  (bug 223653). 
>     name.AssignWithConversion(aFamily.get());

I forgot to remove the above line (|name.AssignWithConversion...|, which is why
fonts with the native names didn't work. With the above line removed, the test
case in bug 223653 gets rendered as it should be.

Asking for r/sr with the above line removed.

Comment 3

[Jungshik Shin]

Comment on attachment 179348 [details] [diff] [review]
patch

trying bryner

Comment 4

[Jungshik Shin]

Attachment: patch updated to the trunk

updated to the trunk. there's a little conflict with the patch for bug 300491
(not in a line changed by this patch but in a line included in the patch as a
'context': pango_xft_get_context -> gdk_pango_context_get). I'll take care of
it if this goes after the patch for bug 300491.

Comment 5

[Christopher Aillon (sabbatical, not receiving bugmail)]

ack, looks like the other patch landed, and we want to not share the "xft" name
if possible.  jshin, can you get an updated patch with the xft-isms removed?

Comment 6

[Jungshik Shin]

Attachment: patch without 'xft'

I replaced 'Xft' with 'Gtk'.

Comment 7

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 190828 [details] [diff] [review]
patch without 'xft'

+    NS_ConvertUTF16toUTF8 name(aFamily);

-    nsCAutoString name;
+    // The newest fontconfig does the full Unicode case folding so that 
+    // we're being lazy here by calling |ToLowerCase| after converting
+    // to UTF-8  assuming that in virtually all cases, we just have to
+    // fold [A-Z].  (bug 223653). 
     name.AssignWithConversion(aFamily.get());

do you really want that AssignWithConversion here?
(EnumFontCallback)

Comment 8

[Jungshik Shin]

(In reply to comment #7)
> (From update of attachment 190828 [details] [diff] [review] [edit])
> +    NS_ConvertUTF16toUTF8 name(aFamily);
> 
> -    nsCAutoString name;

>      name.AssignWithConversion(aFamily.get());
> 
> do you really want that AssignWithConversion here?

That line should have been removed. Thanks. I fixed that mistake as I wrote in
comment #2, but I resurrected it by applying attachment 179348 [details] [diff] [review] to the fresh cvs
copy. 

Comment 9

[Jungshik Shin]

ping caillon, can you review it? I've already fixed, in my local tree, what cbie
pointed out.

Comment 10

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 190828 [details] [diff] [review]
patch without 'xft'

r=caillon, sorry for the delay

Comment 11

[Jungshik Shin]

Comment on attachment 190828 [details] [diff] [review]
patch without 'xft'

asking for a 1.8b5

this is a patch for non-default build (pango build), but RedHat ships a
pango-enabled build in FC/REL so that we'd better fix it before 1.5. It's
rather safe (porting what's been available in our default build for a while to
Pango build)

Comment 12

[Asa Dotzler [:asa]]

Comment on attachment 190828 [details] [diff] [review]
patch without 'xft'

what's the risk to non-pango builds here?

Comment 13

[Jungshik Shin]

fixed on both 1.8 branch (today) and trunk (Sept. 4th)