364786 - [Mac] improve the font switching

Status: VERIFIED FIXED

(Core :: Graphics, defect)

Description

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

now, we are only using the members of the gfxFontGroup for font fallback list of ATSUI. However, sometimes, gfxFontGroup doesn't have enough fonts for the rendering. We should need to improve the fallback mechanism.

See bug 364678 comment 25.

Comment 1

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I have a plan for this bug. I'll create the patch in Jan, 2007.

Comment 2

[philippe (part-time)]

Some sample sites that contain Japanese text and display incorrectly
OS 10.4.8 locale: English

* http://mozillazine.jp (previously mentioned)
style sheet specify roman (western) fonts

* http://www.google.com/search?q=Mozilla%20Japan&sourceid=mozilla2&ie=utf-8&oe=utf-8

* http://www.la-grange.net/
Karl Dubost (W3C Q&A, Tokyo based, Mac User)
--> captions under images, text at bottom left of pages
style sheet specifies only roman fonts, charset=UTF-8
body { font-family: 'optima', 'verdana', 'bitstream vera sans', sans-serif;}

* http://www.pliink.com/mt/marxy/archives/2006/12/kanji-causes-ma.html
(Tokyo based English language blog)
Main text in English, some Japanese text included.
stylesheet specifies only roman fonts
.blogbody {font-family:helvetica, arial, sans-serif;}

* http://www.weathermap.co.jp/monkeyweather/region/054.php3
--> form controls on the right hand side.
page does not specify any fonts, charset=shift_jis
Gecko's forms.css applies
font: -moz-field; font: -moz-fixed; font: -moz-list; font: -moz-button;

* ViewSource on any page that contains Japanese characters

Comment 3

[Wevah (Nate Weaver)]

Another example in the reverse direction is http://bugzilla.mozilla.gr.jp/ : the Roman text should be in a serif font (specified in the CSS) but it isn't.

Comment 4

[Wevah (Nate Weaver)]

Er, the Roman text in the body, specifically.

Comment 5

[philippe (part-time)]

Pages that are mostly English/roman text but contain parts in Arabic
e.g. http://justworldnews.org/archives/002409.html
Arabic font used is 'Al-Bayan bold', because it happens to be the first font on my Mac that contains Arabic glyphs.
(And see bug 334729 for more issues related to Arabic text).

Comment 6

[philippe (part-time)]

err.. I mean see bug 361986 for Arabic text

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch rv0.1(work in progress)

This patch may works fine on some cases. But ATSUMatchFontsToText doesn't work fine for CSS spec...

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Note that win/pango/atsui's font selectors has similar code, so we can merge the code in xp level. But we should do it after this bug.

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 260025 [details] [diff] [review]
Patch rv0.1(work in progress)

I misunderstand the ATSUI behaviors, I'll post new concept patch.

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch rv0.2 (work in progress)

this works fine on most cases, but there are still some issues.

Comment 11

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch rv0.3 (work in progress)

remains work: need to check the system locale in AppendCJKFonts.

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I hope that the patch rv0.3 is tested by many testers.

Comment 13

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch rv0.4 (work in progress)

fix a crash bug.

Comment 14

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch rv0.5 (work in progress)

fix a performance issue and some nits.

Comment 15

[philippe (part-time)]

With patch v0.3 - That fixes the issues I was seeing on with Japanese text :-) 
Chinese text (utf-8) seems OK, I think.

Comment 16

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch rv0.6

adding a system locale checking.

if there are no problems, this should be reviewed.

Comment 17

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch rv0.7 (work in progress)

This optimize the "normal" web pages, if all text can be rendered by specified fonts and pref fonts for the text language, it is pretty faster.

But I have a report that the patch doesn't work fine on 10.3(maybe)...

Comment 18

[philippe (part-time)]

(In reply to comment #17)
 
> But I have a report that the patch doesn't work fine on 10.3(maybe)...
> 

with patch rv 0.7
There are indeed still problems on 10.3. For Japanese text, it is 'better' than before, but not quite correct. I'll attach a screenshot next.
---
Another problem I noticed is the display of the font-names in the Preferences (>Contents>Advanced) on my system with locale set English (10.4.9, ppc)
For Japanese, Chinese the prefs show #GungSeo for both serif and sans-serif.
It displays correctly on system with locale set to Japanese.

As a test, I changed the names for Japanese fonts in 
/cvsroot/mozilla/modules/libpref/src/init/all.js
 to use the roman name (Hiragino Kaku Gothic Pro, and Hiragino Mincho Pro). That works on an English system, but not on the Japanese system here.
---
On 10.4.9, there seems to be a small delay before starting to display a page when mixed fonts (e.g. Roman + Japanese) are used.

Comment 19

[philippe (part-time)]

Attachment: screenshot, comment 18

http://mozillazine.jp seen on OS 10.3.9, with patch rv 0.7.
The site displays correctly on 10.4.9 ppc with the same patch.

Comment 20

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #18)
> Another problem I noticed is the display of the font-names in the Preferences
> (>Contents>Advanced) on my system with locale set English (10.4.9, ppc)
> For Japanese, Chinese the prefs show #GungSeo for both serif and sans-serif.
> It displays correctly on system with locale set to Japanese.

Yeah, I think it. But it happens on Windows too. We need to use the font name resolver from prefs dialog too.

Comment 21

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #19)
> Created an attachment (id=260214) [details]
> screenshot, comment 18
> 
> http://mozillazine.jp seen on OS 10.3.9, with patch rv 0.7.
> The site displays correctly on 10.4.9 ppc with the same patch.

Thanks.

I think that there are one or two bugs in 10.3:

1. ATSUMatchFontsToText ignores the order of the font fallback list. If this is correct, we need another way for checking whether the font has the glyph.

2. The font family names are different between 10.3 and 10.4. If this is correct, I cannot fix it in this bug. We need to fix the bug of comment 20.

Comment 22

[timeless]

Comment on attachment 260110 [details] [diff] [review]
Patch rv0.4 (work in progress)

>Index: gfx/thebes/public/gfxAtsuiFonts.h
>@@ -111,21 +113,38 @@ public:
>+    gfxAtsuiFont* GetCachedFont(ATSUFontID aID) const {
>+        nsAutoString name(GetUniqueNameFor(aID));
>+        nsRefPtr<gfxAtsuiFont> font;
>+        if (mFontCache.Get(name, &font))
>+            return font;
afaiu this returns a dead object.

>Index: gfx/thebes/src/gfxAtsuiFonts.cpp
>@@ -268,16 +275,43 @@ gfxAtsuiFont::~gfxAtsuiFont()
>+CreateFontFallbacksFromFontList(nsTArray< nsRefPtr<gfxFont> > *aFonts, ATSUFontFallbacks *aFallbacks)

>+    if (aFonts->Length() > NUM_STATIC_FIDS)
>+        fids = (ATSUFontID *) PR_Malloc(sizeof(ATSUFontID) * aFonts->Length());
this can fail

>+    for (unsigned int i = 0; i < aFonts->Length(); i++) {
this will crash:
>+        fids[i] = atsuiFont->GetATSUFontID();

and yes i know you're just moving the code.

>@@ -289,38 +323,19 @@ gfxAtsuiFontGroup::gfxAtsuiFontGroup(con
>+    mFontCache.Init(15);

most init functions I've met can fail. What happens when this one does? (remember, you're in a constructor.)

>@@ -446,35 +461,44 @@ gfxAtsuiFontGroup::MakeTextRun(const PRU
>+gfxAtsuiFontGroup::FindFontFor(ATSUFontID aID)
>+    font = new gfxAtsuiFont(aID, GetStyle());
when this fails, you put null somewhere, is that really a good idea?
>+    PutCachedFont(font);

>@@ -747,16 +771,302 @@ PostLayoutOperationCallback(ATSULayoutOp
>+class AtsuiFontSelector
>+private:
>+    const PRUnichar *mString;
>+    UniCharCount     mLength;
>+    UniCharCount     mHeaderChars;
>+
>+    nsTArray< nsRefPtr<gfxAtsuiFont> > mFonts;
>+    PRUint32                           mFontIndex;
>+    gfxAtsuiFontGroup                 *mGroup;
>+    gfxTextRun                        *mTextRun;
>+    ATSUTextLayout                    *mLayout;
>+    ATSUStyle                         *mMainStyle;
>+    nsTArray<ATSUStyle>               *mStylesToDispose;
>+    ATSUFontFallbacks                 *mFallbacks;
>+
>+    PRPackedBool mTriedPrefFonts;
>+
>+    void CreateFontList() {
>+        mFonts.Clear();
>+        for (PRUint32 i = 0; i < mGroup->FontListLength(); ++i)
>+            mFonts.AppendElement(mGroup->GetFontAt(i));
AppendElement functions can generally fail, what happens when this one does?
>+        mFontIndex = mFonts.Length();

>+    void InitSegments() {
>+        nsRefPtr<gfxAtsuiFont> mainFont = mFonts[0];
how sure are you that the array isn't empty? :)
>+    void AppendMissingSegment(UniCharArrayOffset aOffset, UniCharCount aLength) {
>+                memset(gCallbackClosure->mUnmatchedChars.get(), PR_FALSE, mLength);
I kinda expect 0 instead of PR_FALSE

>+            memset(gCallbackClosure->mUnmatchedChars.get() + aOffset - mHeaderChars, PR_TRUE, aLength);

>+    gfxAtsuiFont *GetNextFont(UniCharArrayOffset aOffset, UniCharCount aLength) {
>+TRY_AGAIN_HOPE_FOR_THE_BEST_2:

>+    void AppendPrefFonts(const char *aLangGroup) {
     ^
indentation issues...
     v
>+   void AppendCJKPrefFonts() {
>+       nsCOMPtr<nsIPrefService> prefs =
>+       prefs->GetBranch(0, getter_AddRefs(prefBranch));
the first param to GetBranch isn't a number, please use the more appropriate named creature unless style for the file really doesn't like it.

>+       // Add by the order of accept languages.
please find a native speaker to fix this comment.

>@@ -823,90 +1133,42 @@ gfxAtsuiFontGroup::InitTextRun(gfxTextRu
>     ATSUSetLayoutControls(layout,
>                           NS_ARRAY_LENGTH(layoutTags),
>                           layoutTags,
>                           layoutArgSizes,
>                           layoutArgs);
> 
>     /* Now go through and update the styles for the text, based on font matching. */
> 

>     // Trigger layout so that our callback fires. We don't actually care about
>     // the result of this call.
>     ATSTrapezoid trap;
calling anything that isn't a trap a trap is scaring me, but maybe it's common for people who aren't used to systems that trap and only think about pretty shapes.

--
yes, i know there are 4 attachments past this one. but I'm going to sleep.

Comment 23

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

timeless:

This patch has many OOM crash bugs, but the same bugs are in Win/Linux code too. Now, the text rendering codes are not thinking the OOM, I think that it should be fixed later (after we finish the almost works for text rendering?).

Comment 24

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

And some codes may be merged in XP level after this bug...

Comment 25

[philippe (part-time)]

(In reply to comment #21)

> I think that there are one or two bugs in 10.3:
> 
> 1. ATSUMatchFontsToText ignores the order of the font fallback list. If this is
> correct, we need another way for checking whether the font has the glyph.
> 
> 2. The font family names are different between 10.3 and 10.4. If this is
> correct, I cannot fix it in this bug. We need to fix the bug of comment 20.
> 

Not sure about the first one. About the second point: On 10.3.9, Eng locale, Gecko 1.9 only recognises the font-name if the 'W3' is included:
'Hiragino Kaku Gothic Pro' doesn't work
'Hiragino Kaku Gothic Pro W3' works
(or using the Japanese name: ヒラギノ角ゴ Pro W3)
The postscript name (HiraKakuPro-W3) is also recognised.
I'll attach a test case next

Comment 26

[philippe (part-time)]

Attachment: test case for comment 25

All 6 list-items should be displayed using a sans-serif ('gothic') font. List-items 1, 3, 6, fail on 10.3.9.

Comment 27

[philippe (part-time)]

Attachment: screenshot for comment 25

screen shot on 10.3.9 English.

Comment 28

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Thank you, philippe.

I have a feedback in bugzilla-jp and I looked your testcases and the screenshot. By these points, I believe that the bug is in the font name resolver.

I'm using |[NSFont fontWithName:familyName size:10.0]| for the resolving font family name to an actual font. This works as such behavior in 10.4. But in 10.3, it does not work so. Therefore, I need to change the current font name resolver for 10.3, first. And after it, I can go back to this bug.

Comment 29

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Note:

we may be able to use |NSFontManager fontWithFamily| instead of it.
http://developer.apple.com/documentation/Cocoa/Reference/ApplicationKit/Classes/NSFontManager_Class/Reference/Reference.html#//apple_ref/occ/instm/NSFontManager/fontWithFamily:traits:weight:size:

Comment 30

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch rv0.8 (work in progress)

I think that this patch should work fine on 10.3 too.

Comment 31

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Note that bug 375757 will change the base of this patch. So, it it will be checked-in, the patch cannot apply to trunk.

This patch kicks out the some codes to XP level, but some methods of gfxAtsuiFontSwitch cannot be moved to XP level, they will be moved after bug 366664.

And if the patch will be granted in the review, I will change the FontSelector in gfxPangoFonts.cpp and UniscribeItem in gfxWindowsFonts.cpp. Because they can use gfxFontSwitch and remove some methods from them.

I'm still thinking the better name for gfxFontSwitch. If you have some ideas, please tell me. And FontSelector of gfxPangoFont should be renamed to gfxPangoFontSwitch. But UniscribeItem should not be renamed. Because current name is better than gfxWindowsFontSwitch for the structure of the code.

Comment 32

[philippe (part-time)]

Patch rv0.8 works quite well on both 10.4 (ppc) and 10.3.9.

Comment 33

[Stuart Parmenter]

I really don't think we want to try and merge all the font selection loops.  I'm about to make big changes to the windows ones for things that don't effect linux or mac.  I don't see the need for any goto labels in the mac code (i.e. no reason to copy TRY_AGAIN_HOPE_FOR_THE_BEST labels).  Just make it a simple loop.

Comment 34

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #33)
> I'm about to make big changes to the windows ones for things that don't effect
> linux or mac.

I want to know the new code for Win... I hope that the three platforms should have same structure in XP level.

Comment 35

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #33)
> I really don't think we want to try and merge all the font selection loops. 

Why? I think that if there are missing glyphs only the specified fonts (they are in gfxFontGroup), we should use pref fonts of the language of the characters of the missing glyphs. The pref fonts should be better choice than system selected fonts.

Comment 36

[Stuart Parmenter]

(In reply to comment #35)
> (In reply to comment #33)
> > I really don't think we want to try and merge all the font selection loops. 
> 
> Why? I think that if there are missing glyphs only the specified fonts (they
> are in gfxFontGroup), we should use pref fonts of the language of the
> characters of the missing glyphs. The pref fonts should be better choice than
> system selected fonts.
> 

We should, and do use pref fonts.  I'm just saying that we don't have to merge all of that code or make all of the font selection loops look or be the same.  Maybe sometime down the road when we have a bigger view of how everything works we could merge it but I just don't see the value in trying to make all platforms work the same way when they just don't work the same way.  Yes, small parts are similar and in some cases we may need to have duplicated code but that is ok.

Comment 37

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch rv1.0

cleaned up the previous patch. Let's go to review process.

Comment 38

[Stuart Parmenter]

Comment on attachment 260696 [details] [diff] [review]
Patch rv1.0

I don't understand why you need yet another class in the font code.  As I've said before, the amount of code you want to share is minimal and it isn't worth trying to set it up yet.

I would suggest just putting the code you need to in the atsui code and not trying to create new cross-platform classes.

Comment 39

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch rv1.1

O.K. This doesn't use XP code.

Comment 40

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Sorry, I forgot to comment for the changes:

Maybe, the patch doesn't make damage for tp value if the font group has enough fonts for the run rendering.

The font group cache the font fallback list with |kATSULastResortOnlyFallback|. Therefore we can check whether the specified fonts are enough for the run. If it is enough, |AtsuiTextRunInitializer| sets the sub style for the runs. But if there are some missing glyphs, |AtsuiTextRunInitializer| creates the new font fallback list that is the fonts of the font group and the font prefs of the missing character languages. And the new font fallback list is with |kATSUSequentialFallbacksPreferred|. Note that the new list is created in this time. So, if the font group has enough fonts, it is not created.

For performance, |AtsuiTextRunInitializer| does not call the |gfxFontGroup::ForEachFont| in |AppendPrefFonts|. It is saved in |mAppendedLangFlags|. The each bits of it mean the appended languages.

And I added the |font.name-list| prefs with same values of |font.name|. Because if the user sets a font for a language but the font doesn't have the glyphs for the language, we need the preferred fonts for the language ourselves.

The new font fallback list has |kATSUSequentialFallbacksPreferred|, therefore, even if the new font fallback list doesn't have enough fonts, we don't need more process :-)

Comment 41

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

> For performance, |AtsuiTextRunInitializer| does not call the
> |gfxFontGroup::ForEachFont| in |AppendPrefFonts|.

For performance, |AtsuiTextRunInitializer| does not call the
|gfxFontGroup::ForEachFont| in |AppendPrefFonts| if the fonts for the language are already appended.

Comment 42

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Vlad:

Would you review the patch?

Comment 44

[Mike Connor [:mconnor]]

punting remaining a6 bugs to b1, all of these shipped in a5, so we're at least no worse off by doing so.

Comment 45

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

I'm reworking Masayuki's patch a bit, so that I can understand this code better.  Are there any good test cases for this bug?  I see the links mentioned in comment #2, but the "wrong" fallback just has me using a different font for the CJK glyphs (a serif/cursive one rather than the one specified in the fallbacks).  Am I supposed to change the fallback font to make sure that the change is picked up?

Stuart thinks we should switch to windows-style glyph lookups, but I think we may be able to avoid it.

Comment 46

[Robert O'Callahan (:roc) (email my personal email if necessary)]

What reasons are there to not use Stuart's code, though? Cross-platform font selection consistency would be a good thing IMHO.

Comment 47

[philippe (part-time)]

Attachment: minimal test case

Mixed Roman and Japanese text.
Vlad, here is a minimal test case. The second paragraph is how it _should_ display (forced substitution by adding a Japanese font in the stylesheet).

Fx 2.0.0.x or Webkit display the first paragraph like the second one.

Comment 48

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: mac font fallback, slightly different approach

No reason to not use Stuart's code, other than that it's more work at the moment (it would have to be extracted out of the win32 code), and I wanted to understand this code better.  I can still do the switch to the bitset-style, but ATSUI's font matching seems to follow CSS semantics, so that can be an optimization as opposed to a bugfix.

Here's a reworked version of Masayuki's patch; it doesn't use a separate class, so it doesn't have to pass a bunch of data around.  There are still some optimizations that can be done -- for example, the list of pref fonts is parsed from prefs and converted to gfxFonts each time, when instead we could cache the array of gfxFonts we'd get for each lang (flushing if the prefs change).

roc, I didn't know where to put the GLYPH_RUNS_OUT_OF_ORDER flag -- not sure if where it's at now is correct.  I had to add support for this so that I could go back and fill in the holes in the glyph runs using fallback fonts that weren't found in the first pass using the fonts specified in the CSS.

Also, this patch is missing the addition of the font-list prefs; we should take those as well, I think.

Comment 49

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Whoops, found some more bugs -- need to extend the looping a bit (not handling
the case where there are multiple scripts within a single fallback range
correctly).  I'll attach a testcase for this as well in a bit.

Comment 50

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: mac font fallback

Fixed; some dumb mistakes in earlier patch with setting the glyph-runs-out-of-order flag.

Comment 51

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: another test

Another test, requiring multiple font fallbacks.

Two glyphs in this test -- 𐄀 𐄎 -- seem to render fine in Fx2 (at least, I think; I'm not sure if they're rendering correctly, but something shows up -- they're from the Aegean Numbers set), but fail in both Minefield and Safari.

Comment 52

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Ah, I see.  All of them are doing the right thing, but Fx2 is displaying the missing-glyph slugs using a graphical representation of the unicode range for which codepoints are missing (in this case, the AEGEAN WEIGHT BASE UNIT character surrounded by a box).  Not sure how we can get that, but it's not essential.  (Safari just displays little boxes, no codepoints in them.)

Comment 53

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Branch builds use LastResort.dfont in most (but not all) cases where there's no font on the system that has the given glyph, whereas trunk seems not to do it at all and instead uses the home-rolled boxes.  (This is the first case I've ever seen where Safari does not use LastResort glyphs.)

Comment 54

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

So, I've tried, unsuccessfully, to find information about the Last Resort font and how to select it -- I can't see where/how the branch gfx code gets it, either.  I'd like to be able to use it... any idea how to get it/select it?

Comment 55

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Oh wait, is it just a font called "LastResort"?  (I see it in the fonts folder)... it wasn't showing up in Font Book, which is why I thought there was something special needed.

Comment 56

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+    ATSUFontID static_fids[NUM_STATIC_FIDS];
+    ATSUFontID *fids;
+    if (aFonts->Length() > NUM_STATIC_FIDS) {
+        fids = (ATSUFontID *) PR_Malloc(sizeof(ATSUFontID) * aFonts->Length());

Use nsAutoTArray and AppendElements.

+    // put back the fallback object, just in case

Why are you doing this?

Otherwise it looks OK.

> No reason to not use Stuart's code, other than that it's more work at the
> moment (it would have to be extracted out of the win32 code), and I wanted to
> understand this code better.  I can still do the switch to the bitset-style,
> but ATSUI's font matching seems to follow CSS semantics, so that can be an
> optimization as opposed to a bugfix.

If we used Stuart's code we wouldn't need ATSUI font matching at all and most of this could go away, right?

I must be missing something, because if most of this code is just going to go away, I don't see the value of working on it.

Comment 57

[philippe (part-time)]

Attachment: screen shot

It could be an issue on my side, but this is how the simple text case looks like here [1]. Note the odd text after the last 2 Japanese characters on the first line.
I see similar problems on http://mozillazine.jp/, it seems to affect Roman text coming after a run of Japanese text.

Also note the odd character in the title of the second tab, where the ellipsis (…) should be. This happens when the title contains a mix of Roman and Japanese characters (and sometimes goes away after switching tabs, or the affected tab has focus).

[1] 10.4.10 PPC, Minefield build with gcc 4.01, OS 10.4u SDK

Comment 58

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

philippe: Did you try with the first or second version of the patch?  The first one I posted would've done that -- I had some stupid bugs in the code that kept unsorted glyph runs, and so you'd get glyphs rendered using the wrong font.

roc: if we were to use Stuart's code it would all go away; the reason to do it this way is to just do it as an incremental improvement over what's there.  Using stuart's code is a bigger project; I'd like to do that at some point for both OSX and Linux, but since this was a relatively easy fix I think we should take this for now.

Comment 59

[Robert O'Callahan (:roc) (email my personal email if necessary)]

OK, r/sr from me if you address my comments (just explain why you put back the fallback object)

Comment 60

[philippe (part-time)]

(In reply to comment #58)
> philippe: Did you try with the first or second version of the patch?  The first
> one I posted would've done that -- I had some stupid bugs in the code that kept
> unsorted glyph runs, and so you'd get glyphs rendered using the wrong font.

Second patch: the one with '364786-mac-fallback' on the very fist line.

Comment 61

[philippe (part-time)]

Vlad, are you sure the second patch is correct? I saved both your patches locally and ran a diff: the only difference appears to be the time stamp

+++ b/gfx/thebes/public/gfxFont.h	Thu Jul 05 12:28:20 2007 +0200
---
+++ b/gfx/thebes/public/gfxFont.h	Thu Jul 05 12:51:27 2007 +0200

etc, etc

-------
Except for those odd glyph problems, all pages I checked displayed nicely.
One thing I noticed though, on http://mozillazine.jp/, the stylesheet specifies (roman) serif fonts for the body text - Minefield used 'Hiragino kaku gothic' (=sans-serif), whereas WebKit and Safari use 'Hiragino Mincho Pro' (=serif). Firefox 2.0.0.4 does the same as Minefield, so strictly speaking this not a regression there. Still, for the 3 people on the internet who actually care about those things...

Comment 62

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: mac font fallback, v2

Whoops -- somehow I managed to overwrite the previous patch!  Luckily the files were still opened in emacs buffers...

This is the real patch with the fixes, also with roc's comments incorporated.  roc, I put back the original fallback object because I'm disposing of any of the ones created while doing the fallback searching.  I added a check here to "put back" the original one only if we did actually create some other fallback objects, so nothing happens in the no-fallback case.

I'll make up a separate patch here that has the other two bits from masayuki's patch -- the font-list pref additions and the change to the QuartzFontCache; those are somewhat separate from this.

Comment 63

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

> +    // put back the fallback object, because we'll be disposing
> +    // of any of the ones we created while doing fallback
> +    if (fallbacksToDipose.Length() > 0) {

fallbacksToDispose
             ^

Comment 64

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

fm.. the latest patch has comment 57's bug.

Comment 65

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

looks like the advances of the glyphs are correct in the broken segment, but the glyph ids are used for another font.

Comment 66

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

That's what I get for adding a last-minute change without compiling it.  Whoops.

Though the wrong font/right glyph ids bug was the problem that the non-v2 patch had; basically the glyph runs out of order thing wasn't working due to some logic mistakes (the flag wasn't being set when it should have been, and thus they were never being sorted).  Are you sure you're seeing it with the new patch with the typo fixed?  With the testcase that I attached?  If so, there's probably some other logic issue in the glyph run ordering bit.  You may want to put some printfs in ::InitTextRun right after SortGlyphRuns to dump the glyph runs in order and their offsets; make sure they're strictly increasing.

Comment 67

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

the first paragraph of first testcase and the both paragraphs of seconde testcase are broken with attachment 271241 [details] [diff] [review] (with fix a typo).

Comment 68

[philippe (part-time)]

with the new (and patched) patch
Vlad's test case displays correctly here, but my minimal testcase fails (1st paragraph, all glyphs following the last run of Japanese text are broken).

Comment 69

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: mac font fallback, v3

Last one, I promise!  I was being too clever with the glyph run out of order business; got rid of all of that, and it's up to the caller to decide whether the glyph runs need to be sorted.  The problem was that we were adding all the current-font runs before getting to any fallback, and AddGlyphRun was helpfully coalescing those until we added the first out-of-order one.  So we ended up with bogus glyph runs at the end, where any runs of the same font as the first at the end were missing.  This is where the wrong glyphs came from at the end of the lines.

I'm pretty sure I know how to fix the serif/sans-serif business, but that's for another patch -- we're ignoring the presence of a generic font name in the list that comes from CSS and instead are always using the default generic style for that language (from the font.default.x-langgroup pref) to do the lookup in the font.name.x-langgroup.(serif|sans-serif|etc) pref.

Comment 70

[philippe (part-time)]

(In reply to comment #69)
> Created an attachment (id=271362) [details]
> mac font fallback, v3

This one works fine so far. Cool.

Comment 71

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 271362 [details] [diff] [review]
mac font fallback, v3

Good! The patch works fine for me. Thank you.

Comment 72

[philippe (part-time)]

The latest patch doesn't apply anymore - something in gfx/thebes/src/gfxAtsuiFonts.cpp (NS_STATIC_CAST problem I think) has gone sour.

Comment 73

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Checked in my patch; still need to do the secondary bits from masayuki's patch (the prefs additions and the fix to the quartz font cache)

Comment 74

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Additional patch rv1.0

Sorry, for the delay.

Let's land this.

Comment 75

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 274360 [details] [diff] [review]
Additional patch rv1.0

Whoops, I'd forgotten about this too.  Looks fine.

Comment 76

[philippe (part-time)]

(In reply to comment #74)
> Created an attachment (id=274360) [details]
> Additional patch rv1.0
> 

That patch still brings up the same issue as I mentioned in comment 18 above: on a system with a en-us locale the font-listing in the Preference window for Japanese (and other CJK) is still showing incorrect fonts. Because the font-names in all.js are in Japanese. It does work fine for a 'Japanese' user.

We still need a follow-up patch/bug here, I suppose (per comment 20)?

Comment 77

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #76)
> We still need a follow-up patch/bug here, I suppose (per comment 20)?

Yes. We need a new bug for it. I think that we need font name resolver which can be called from XUL layer.

Comment 78

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

checked-in the additional patch, thanks!

Comment 79

[philippe (part-time)]

Verified with the 2007080316 Minefield build. Thanks !

I filed:
bug 390901 – CJK - font-name is not recognised correctly in preferences.
for the preferences issues (affects all users of Gecko).

bug 390900 – Font-substitution with mixed languages should respect the generic font-family in the author stylesheet.
for the substitution of fonts to serif/sans-serif (comment 61 and comment 69 above).