This warning appears too frequently: WARNING: Font mismatch inside cluster: file /Users/jruderman/trunk/mozilla/gfx/thebes/src/gfxAtsuiFonts.cpp, line 922

I commented this assertion out locally.

Testcase contains two entities: ⊣‌ Output with various printf's added within ATSUI font matching code: 0x298cbe0(serif) TEXTRUN "⊣‌" ENDTEXTRUN [22a3 200c ] ==== Starting font matching [string length: 3 (⊣‌.)] (loop start, missing ranges: 0) ... glyphs not found: 1 1 total length: 2 changedOffset: 1 changedLength: 1 ... glyphs found in first: 2 1 WARNING: Font mismatch inside cluster: file /builds/trunk/mozilla/gfx/thebes/src/gfxAtsuiFonts.cpp, line 1221 (loop start, missing ranges: 1) ... missing range: 1 1 ..AppendPrefFonts: lang group - x-unicode ..AppendFontToList: Times-Roman ..AppendFontToList: Times-Roman ... fallback list created, 2 elements ... glyphs matched in fallback: 1 1 (Code2000) ... substitute font used total length: 1 changedOffset: 1 changedLength: 1 ==== End font matching

The testcase is a bit unrealistic, the ‌ entity is used to prevent joining between two adjacent characters. We should probably skip font matching for it, although we probably need to confirm that ATSUI handles this correctly.

I think we should fix this so that we can support characters in a cluster with different fonts. Otherwise we run a big risk of displaying something rather different from what ATSUI asked for, and that could mangle text down the road. Also, fundamentally cluster boundaries need not and should not affect glyph measurement and rendering. I have a patch that reorganizes textrun internals a bit to fix this.

This patch does the following: -- Removes support for ABSOLUTE_SPACING which is no longer needed now that no callers to nsIReneringContext methods use aSpacing parameters. I'm removing it now because otherwise it would have to be updated to work with the rest of this patch. -- Flip IsLigatureContinuation to IsLigatureGroupStart to be more consistent with methods like IsClusterStart. -- Important change #1: replace the fixed set of character tag values with four orthogonal bits representing start of continuation, start of ligature-group, missing, and low surrogate. -- Important change #2: allow any character to have associated glyphs. In particular non-cluster-start characters can have associated glyphs. This is how we can support glyphs in the same cluster having different fonts. -- Store the glyph count in the CompressedGlyph data instead of using a bit in the DetailedGlyphs. This simplifies code a bit. -- Make gfxAtsuiFonts construct smaller ligature-groups --- characters in the same cluster no longer have to be in the same ligature-group, they can each have their own glyphs, with their own fonts. -- Add a check to SetGlyphsForCharacterGroup to detect remaining cases where glyphs in the same ligature-group are associated with different fonts. I don't know of any cases where this will happen, but if they do happen, we drop some of the glyphs on the floor. The only case I can think of where it might happen is where we construct a ligature group because we have out-of-order glyphs, i.e. DEVANAGARI VOWEL I after a consonant, where the vowel is rendered before the consonant; we handle this by gluing the glyphs together and calling it a cluster --- and ATSUI decides to put the vowel and the consonant in different fonts. Handling this "properly" would be pretty hard, we'd have to play games with glyph advances and offsets. -- Remove gfxAtsuiFonts' now-unnecessary check that glyph runs don't break up clusters (what this bug is about). -- Make gfxFont methods handle glyphs attached to any kind of character. -- Factor out code to sum up advances for all glyphs in a range of characters -- Update gfxPangoFonts and gfxWindowsFonts without changing behaviour -- Simplify MergeCharactersInTextRun and make it work with the new invariants

Comment on attachment 280719 [details] [diff] [review] fix need simon-review on the layout bits

This patch fixes a crash in bug 394246.

Applying the patch in this bug makes this testcase assert: ###!!! ASSERTION: Glyph runs out of order (and run not forced): 'lastGlyphRun->mCharacterOffset <= aUTF16Offset', file /Users/jruderman/trunk/mozilla/gfx/thebes/src/gfxFont.cpp, line 1370

Updated patch to fix the issue Jesse found (need to call ResetGlyphRuns before copying glyphs into the textrun).

I need reviews here, guys...

We should consider this a blocker because without it, Mac text is going to display incorrectly in some situations, and we'll even crash (see bug 394246).

Comment on attachment 281148 [details] [diff] [review] updated patch a=endgame drivers for post-M9 landing (can only be landed after M9 freeze passes)

This is ready to check in

Checking in gfx/src/thebes/nsThebesFontMetrics.cpp; /cvsroot/mozilla/gfx/src/thebes/nsThebesFontMetrics.cpp,v <-- nsThebesFontMetrics.cpp new revision: 1.40; previous revision: 1.39 done Checking in gfx/src/thebes/nsThebesFontMetrics.h; /cvsroot/mozilla/gfx/src/thebes/nsThebesFontMetrics.h,v <-- nsThebesFontMetrics.h new revision: 1.18; previous revision: 1.17 done Checking in gfx/thebes/public/gfxFont.h; /cvsroot/mozilla/gfx/thebes/public/gfxFont.h,v <-- gfxFont.h new revision: 1.90; previous revision: 1.89 done Checking in gfx/thebes/src/gfxAtsuiFonts.cpp; /cvsroot/mozilla/gfx/thebes/src/gfxAtsuiFonts.cpp,v <-- gfxAtsuiFonts.cpp new revision: 1.73; previous revision: 1.72 done Checking in gfx/thebes/src/gfxFont.cpp; /cvsroot/mozilla/gfx/thebes/src/gfxFont.cpp,v <-- gfxFont.cpp new revision: 1.71; previous revision: 1.70 done Checking in gfx/thebes/src/gfxPangoFonts.cpp; /cvsroot/mozilla/gfx/thebes/src/gfxPangoFonts.cpp,v <-- gfxPangoFonts.cpp new revision: 1.112; previous revision: 1.111 done Checking in gfx/thebes/src/gfxWindowsFonts.cpp; /cvsroot/mozilla/gfx/thebes/src/gfxWindowsFonts.cpp,v <-- gfxWindowsFonts.cpp new revision: 1.157; previous revision: 1.156 done Checking in layout/generic/nsTextFrameThebes.cpp; /cvsroot/mozilla/layout/generic/nsTextFrameThebes.cpp,v <-- nsTextFrameThebes.cpp new revision: 3.120; previous revision: 3.119 done Checking in layout/generic/nsTextRunTransformations.cpp; /cvsroot/mozilla/layout/generic/nsTextRunTransformations.cpp,v <-- nsTextRunTransformations.cpp new revision: 3.15; previous revision: 3.14 done

I backed this out as the possible cause of bug 402990.

This also breaks the OS/2 build, so if you check it in again, would you be so kind to also apply this patch? It's just copied from the change to the PangoFonts fast path.

The breakage was very simple. FetchGlyphExtents was mis-merged and I was using "i" where I should have been using "j" ... and there was a shadowing definition of "j" which really should have been a new variable, so I've called it "k". I've also included the OS/2 fix. Reftests pass. I think this can reland.

Checking in gfx/src/thebes/nsThebesFontMetrics.cpp; /cvsroot/mozilla/gfx/src/thebes/nsThebesFontMetrics.cpp,v <-- nsThebesFontMetrics.cpp new revision: 1.42; previous revision: 1.41 done Checking in gfx/src/thebes/nsThebesFontMetrics.h; /cvsroot/mozilla/gfx/src/thebes/nsThebesFontMetrics.h,v <-- nsThebesFontMetrics.h new revision: 1.20; previous revision: 1.19 done Checking in gfx/thebes/public/gfxFont.h; /cvsroot/mozilla/gfx/thebes/public/gfxFont.h,v <-- gfxFont.h new revision: 1.92; previous revision: 1.91 done Checking in gfx/thebes/src/gfxAtsuiFonts.cpp; /cvsroot/mozilla/gfx/thebes/src/gfxAtsuiFonts.cpp,v <-- gfxAtsuiFonts.cpp new revision: 1.75; previous revision: 1.74 done Checking in gfx/thebes/src/gfxFont.cpp; /cvsroot/mozilla/gfx/thebes/src/gfxFont.cpp,v <-- gfxFont.cpp new revision: 1.73; previous revision: 1.72 done Checking in gfx/thebes/src/gfxOS2Fonts.cpp; /cvsroot/mozilla/gfx/thebes/src/gfxOS2Fonts.cpp,v <-- gfxOS2Fonts.cpp new revision: 1.18; previous revision: 1.17 done Checking in gfx/thebes/src/gfxPangoFonts.cpp; /cvsroot/mozilla/gfx/thebes/src/gfxPangoFonts.cpp,v <-- gfxPangoFonts.cpp new revision: 1.114; previous revision: 1.113 done Checking in gfx/thebes/src/gfxWindowsFonts.cpp; /cvsroot/mozilla/gfx/thebes/src/gfxWindowsFonts.cpp,v <-- gfxWindowsFonts.cpp new revision: 1.159; previous revision: 1.158 done Checking in layout/generic/nsTextFrameThebes.cpp; /cvsroot/mozilla/layout/generic/nsTextFrameThebes.cpp,v <-- nsTextFrameThebes.cpp new revision: 3.124; previous revision: 3.123 done Checking in layout/generic/nsTextRunTransformations.cpp; /cvsroot/mozilla/layout/generic/nsTextRunTransformations.cpp,v <-- nsTextRunTransformations.cpp new revision: 3.17; previous revision: 3.16 done

I checked in both testcases as crashtests.

verified fixed using Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9pre) Gecko/2008050621 Firefox/3.0pre and the testcases -> no assertion/warning --> Verified fixed