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

RESOLVED FIXED

Status

Core Graveyard
GFX: Gtk
RESOLVED FIXED
13 years ago
9 years ago

People

(Reporter: Jungshik Shin, Assigned: Jungshik Shin)

Tracking

({fixed1.8, intl})

Trunk
x86
Linux
fixed1.8, intl

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

11.77 KB, patch
Jungshik Shin
: superreview+
Details | Diff | Splinter Review
12.90 KB, patch
Christopher Aillon (sabbatical, not receiving bugmail)
: review+
Jungshik Shin
: superreview+
Details | Diff | Splinter Review
(Assignee)

Description

13 years ago
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.
(Assignee)

Comment 1

13 years ago
Created attachment 179348 [details] [diff] [review]
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.
(Assignee)

Comment 2

13 years ago
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.
Attachment #179348 - Flags: superreview?(dbaron)
Attachment #179348 - Flags: review?(blizzard)
Attachment #179348 - Flags: superreview?(dbaron) → superreview+
(Assignee)

Comment 3

12 years ago
Comment on attachment 179348 [details] [diff] [review]
patch

trying bryner
Attachment #179348 - Flags: review?(blizzard) → review?(bryner)
(Assignee)

Comment 4

12 years ago
Created attachment 190231 [details] [diff] [review]
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.
Attachment #179348 - Attachment is obsolete: true
Attachment #190231 - Flags: superreview+
Attachment #190231 - Flags: review?(caillon)
(Assignee)

Updated

12 years ago
Attachment #179348 - Flags: review?(bryner)
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?
Attachment #190231 - Flags: review?(caillon)
(Assignee)

Comment 6

12 years ago
Created attachment 190828 [details] [diff] [review]
patch without 'xft'

I replaced 'Xft' with 'Gtk'.
Attachment #190828 - Flags: superreview+
Attachment #190828 - Flags: review?(caillon)
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)
(Assignee)

Comment 8

12 years ago
(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. 


Status: NEW → ASSIGNED
(Assignee)

Comment 9

12 years ago
ping caillon, can you review it? I've already fixed, in my local tree, what cbie
pointed out.
Comment on attachment 190828 [details] [diff] [review]
patch without 'xft'

r=caillon, sorry for the delay
Attachment #190828 - Flags: review?(caillon) → review+
(Assignee)

Comment 11

12 years ago
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)
Attachment #190828 - Flags: approval1.8b5?

Updated

12 years ago
Attachment #190828 - Flags: approval1.8b5? → approval1.8b5+

Comment 12

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

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

Comment 13

12 years ago
fixed on both 1.8 branch (today) and trunk (Sept. 4th)
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED

Updated

12 years ago
Keywords: fixed1.8
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.