Closed Bug 364786 Opened 18 years ago Closed 17 years ago

[Mac] improve the font switching

Categories

(Core :: Graphics, defect)

Product:
Core
Component:
Graphics
Platform:
x86
macOS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla1.9alpha8

People

(Reporter: masayuki, Assigned: masayuki)

References

Details

(Keywords: intl)

Attachments

(8 files, 13 obsolete files)

screenshot, comment 18
114.25 KB, image/png
Details
test case for comment 25
3.61 KB, text/html
Details
screenshot for comment 25
165.79 KB, image/png
Details
minimal test case
2.10 KB, text/html
Details
another test
780 bytes, text/html
Details
screen shot
14.33 KB, image/png
Details
mac font fallback, v3
32.72 KB, patch
masayuki
: review+
roc
: superreview+
Details | Diff | Splinter Review
Additional patch rv1.0
10.00 KB, patch
vlad
: review+
Details | Diff | Splinter Review 
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.
I have a plan for this bug. I'll create the patch in Jan, 2007.
Assignee: nobody → masayuki
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
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.
Er, the Roman text in the body, specifically.
Depends on: 334729
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).
err.. I mean see bug 361986 for Arabic text
Attached patch Patch rv0.1(work in progress) (obsolete) — — Splinter Review 
This patch may works fine on some cases. But ATSUMatchFontsToText doesn't work fine for CSS spec...
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 on attachment 260025 [details] [diff] [review]
Patch rv0.1(work in progress)

I misunderstand the ATSUI behaviors, I'll post new concept patch.
Attachment #260025 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attached patch Patch rv0.2 (work in progress) (obsolete) — — Splinter Review 
this works fine on most cases, but there are still some issues.
Attached patch Patch rv0.3 (work in progress) (obsolete) — — Splinter Review 
remains work: need to check the system locale in AppendCJKFonts.
Attachment #260108 - Attachment is obsolete: true
I hope that the patch rv0.3 is tested by many testers.
Attached patch Patch rv0.4 (work in progress) (obsolete) — — Splinter Review 
fix a crash bug.
Attachment #260109 - Attachment is obsolete: true
Attached patch Patch rv0.5 (work in progress) (obsolete) — — Splinter Review 
fix a performance issue and some nits.
Attachment #260110 - Attachment is obsolete: true
With patch v0.3 - That fixes the issues I was seeing on with Japanese text :-) 
Chinese text (utf-8) seems OK, I think.
Attached patch Patch rv0.6 (obsolete) — — Splinter Review 
adding a system locale checking.

if there are no problems, this should be reviewed.
Attachment #260112 - Attachment is obsolete: true
Attached patch Patch rv0.7 (work in progress) (obsolete) — — Splinter Review 
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)...
Attachment #260113 - Attachment is obsolete: true
(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.
Attached image 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.
(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.
(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 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.
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?).
And some codes may be merged in XP level after this bug...
(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



All 6 list-items should be displayed using a sans-serif ('gothic') font. List-items 1, 3, 6, fail on 10.3.9.
screen shot on 10.3.9 English.
Attachment #260263 - Attachment description: test case → test case for comment 25
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.
Attached patch Patch rv0.8 (work in progress) (obsolete) — — Splinter Review 
I think that this patch should work fine on 10.3 too.
Attachment #260207 - Attachment is obsolete: true
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.
Patch rv0.8 works quite well on both 10.4 (ppc) and 10.3.9.
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.
(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.

(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.
(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.
Attached patch Patch rv1.0 (obsolete) — — Splinter Review 
cleaned up the previous patch. Let's go to review process.
Attachment #260350 - Attachment is obsolete: true
Attachment #260696 - Flags: review?(vladimir)
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.
Attachment #260696 - Flags: review?(vladimir) → review-
Attached patch Patch rv1.1 (obsolete) — — Splinter Review 
O.K. This doesn't use XP code.
Attachment #260696 - Attachment is obsolete: true
Attachment #260807 - Flags: review?(vladimir)
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 :-)
> 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.
Vlad:

Would you review the patch?
Target Milestone: --- → mozilla1.9alpha6
punting remaining a6 bugs to b1, all of these shipped in a5, so we're at least no worse off by doing so.
Target Milestone: mozilla1.9alpha6 → mozilla1.9beta1
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.
What reasons are there to not use Stuart's code, though? Cross-platform font selection consistency would be a good thing IMHO.
Attached file 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.
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.
Attachment #271025 - Flags: superreview?(roc)
Attachment #271025 - Flags: review?(masayuki)
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.
Attached patch mac font fallback (obsolete) — — Splinter Review 
Fixed; some dumb mistakes in earlier patch with setting the glyph-runs-out-of-order flag.
Attachment #271025 - Attachment is obsolete: true
Attachment #271056 - Flags: superreview?(roc)
Attachment #271056 - Flags: review?(masayuki)
Attachment #271025 - Flags: superreview?(roc)
Attachment #271025 - Flags: review?(masayuki)
Attached file another test — 
Another test, requiring multiple font fallbacks.

Two glyphs in this test -- &#65792; &#65806; -- 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.
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.)
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.)
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?
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.
+    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.
Attached image 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
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.
OK, r/sr from me if you address my comments (just explain why you put back the fallback object)
(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.
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...
Attached patch mac font fallback, v2 (obsolete) — — Splinter Review 
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.
Attachment #271056 - Attachment is obsolete: true
Attachment #271241 - Flags: superreview?(roc)
Attachment #271241 - Flags: review?(masayuki)
Attachment #271056 - Flags: superreview?(roc)
Attachment #271056 - Flags: review?(masayuki)
> +    // 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
             ^
fm.. the latest patch has comment 57's bug.
looks like the advances of the glyphs are correct in the broken segment, but the glyph ids are used for another font.
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.
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).
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).

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.
Attachment #271241 - Attachment is obsolete: true
Attachment #271362 - Flags: superreview?(roc)
Attachment #271362 - Flags: review?(masayuki)
Attachment #271241 - Flags: superreview?(roc)
Attachment #271241 - Flags: review?(masayuki)
(In reply to comment #69)
> Created an attachment (id=271362) [details]
> mac font fallback, v3

This one works fine so far. Cool.


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

Good! The patch works fine for me. Thank you.
Attachment #271362 - Flags: review?(masayuki) → review+
The latest patch doesn't apply anymore - something in gfx/thebes/src/gfxAtsuiFonts.cpp (NS_STATIC_CAST problem I think) has gone sour.
Attachment #271362 - Flags: superreview?(roc) → superreview+
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)
Sorry, for the delay.

Let's land this.
Attachment #260807 - Attachment is obsolete: true
Attachment #274360 - Flags: review?(vladimir)
Attachment #260807 - Flags: review?(vladimir)
Comment on attachment 274360 [details] [diff] [review]
Additional patch rv1.0

Whoops, I'd forgotten about this too.  Looks fine.
Attachment #274360 - Flags: review?(vladimir) → review+
(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)?

(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.
checked-in the additional patch, thanks!
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
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).
Status: RESOLVED → VERIFIED
No longer blocks: 387410
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: