The following patch does two major things: hoist the platform-independent code from gfxPangoTextRun to gfxTextRun (removing gfxPangoTextRun in the process), and implement gfxTextRun "properly" for Mac and Windows. These have to be done at the same time.

This will probably regress performance on Windows and Mac, because they were caching things already (so no new win there) and this code does a bit more work now (extracting all advances and storing them with the glyph indices). But hopefully it's only a small difference and I'm sure it's a step in the right direction... For the Windows draw path, I now just use the direct cairo_show_glyphs path always. If this hurts drawing performance, I'd be surprised.

The patch works fine on Windows excepted the spacing rendering problem. # the issue already happens on Linux after landing the new textrun.

Which problem is that? I'm not sure about the handling of missing glyphs on Mac and Windows. I should be calling SetMissingGlyph in some places, but I'm not.

(In reply to comment #3) > Which problem is that? I can reproduce that the text is overflowed from nsTextFrame if the text is justified.

Updated with missing-glyph detection for Windows. The existing Mac code doesn't have any explicit missing-glyph detection and I'm not sure what to do there so I just left it alone. I found the bug with justification being off. It's a generic spacing bug that mistakenly added the spacing for the last character in a glyph-run twice. It's fixed in this patch by removing code at the end of gfxFont::Draw. I noticed that Windows (Uniscribe) is making item boundaries at punctuation even in simple testcases, which is a little weird, although it shouldn't be a problem as long as we process items efficiently. I modified gfxTextRun::AddGlyphRun to merge glyphruns with the same font so that those extra items don't affect the generated textrun.

Comment on attachment 255622 [details] [diff] [review] updated patch Vlad should look at the ATSUI code.

this makes the windows code use its fast paths more often and sets IS_ASCII/IS_8BIT appropriately. I'm seeing speed results between -2% and +5% on Tp on my machine which may just be noise so I don't think we'll see any real performance hit (at least on Windows).

+ if (IsAscii(aString, aLength)) + params.mFlags |= gfxTextRunFactory::TEXT_IS_ASCII; + else + params.mFlags |= gfxTextRunFactory::TEXT_IS_8BIT; 8BIT and ASCII aren't mutually exclusive. This should always set 8BIT and the assertion in gfxWindowsFontGroup::MakeTextRun(PRUint8*) could just be removed. Otherwise, looks good.

Running Camino with this patch applied I crashed when viewing an email message in Gmail STR 1. enter Gmail 2. view individual message 3. crash while loading from Apple crash.log 0 org.mozilla.camino 0x007f92a0 gfxTextRun::ComputeClusterAdvance(unsigned) + 76 1 org.mozilla.camino 0x007f93c0 gfxTextRun::ComputeLigatureData(unsigned, gfxTextRun::PropertyProvider*) + 236 2 org.mozilla.camino 0x007f9644 gfxTextRun::ShrinkToLigatureBoundaries(unsigned*, unsigned*) + 104 3 org.mozilla.camino 0x007f99d0 gfxTextRun::GetAdvanceWidth(unsigned, unsigned, gfxTextRun::PropertyProvider*) + 440 4 org.mozilla.camino 0x007f3880 nsThebesFontMetrics::GetWidth(unsigned short const*, unsigned, int&, int*, nsThebesRenderingContext*) + 284 5 org.mozilla.camino 0x0086862c nsRenderingContextImpl::GetPosition(unsigned short const*, unsigned, nsPoint) + 928 6 org.mozilla.camino 0x008686f8 nsRenderingContextImpl::GetPosition(unsigned short const*, unsigned, nsPoint) + 1132 7 org.mozilla.camino 0x003bfe48 nsTextFrame::MeasureText(nsPresContext*, nsHTMLReflowState const&, nsTextTransformer&, nsTextStyle&, nsTextFrame::TextReflowData&) + 1596 8 org.mozilla.camino 0x003c0784 nsTextFrame::MeasureText(nsPresContext*, nsHTMLReflowState const&, nsTextTransformer&, nsTextStyle&, nsTextFrame::TextReflowData&) + 3960 9 org.mozilla.camino 0x00396abc nsLineLayout::ReflowFrame(nsIFrame*, unsigned&, nsHTMLReflowMetrics*, int&) + 1096 10 org.mozilla.camino 0x00361808 nsBlockFrame::ReflowInlineFrame(nsBlockReflowState&, nsLineLayout&, nsLineList_iterator, nsIFrame*, LineReflowStatus*) + 132 11 org.mozilla.camino 0x00363400 nsBlockFrame::DoReflowInlineFrames(nsBlockReflowState&, nsLineLayout&, nsLineList_iterator, int*, LineReflowStatus*, int) + 480

OS X 10.4.8 PPC Camino build, checkout time 11:30 am Japan Standard Time

Do you get any assertions displayed on the console if you run it from the command line? (I assume this is a debug build.) Anything funny about the email message?

(In reply to comment #11) > Do you get any assertions displayed on the console if you run it from the > command line? (I assume this is a debug build.) It was an opt build unfortunately. I'll patch my debug build tonight. > Anything funny about the email > message? Nope, plain text messages typical from mailing lists. A couple of rules in userContent.css to bump up the font-size in Gmail.

Updates to Mac code. Remove the gfxAtsuiFonts.h/.cpp part from the latest patch Stuart posted, replace them with this, and apply to your tree. I found a crash viewing my own GMail. It was triggered by ATSUI's behaviour encountering DEVANAGARI VOWEL SIGN I; ATSUI was reversing the direction of the vowel sign + consonant pair, because the vowel precedes the consonant visually even though it follows the consonant in the text. I can handle this (and other swapped pairs) by forcing them to cluster. I don't see any email with the Devanagari in it, but Google was definitely sending me the text. With this update I don't crash even when viewing messages in my highly multicultural spam folder.

Comment on attachment 256418 [details] [diff] [review] fix? Still need you to take a look at the Mac code, vlad...

(In reply to comment #13) > Created an attachment (id=256418) [details] > fix? > Yep, no more crashes in Gmail. (and whoot ! the headlines of articles on ElPais.com now fix in the box.)

Trying to access http://news.bbc.co.uk/hi/arabic/news/ crashes, same stack as comment #10 I get these: WARNING: recurring into frame construction: 'mPresContext->mLayoutPhaseCount[eLayoutPhase_FrameC] == 0', file /Users/phiw/cocoafox/mozilla/layout/base/nsPresContext.h, line 924 ###!!! ASSERTION: Continuation at the start of a run??: 'index > 0', file /Users/phiw/cocoafox/mozilla/gfx/thebes/src/gfxAtsuiFonts.cpp, line 584 ###!!! ASSERTION: First character must be the start of a cluster and can't be a ligature continuation!: 'aCharIndex > 0 || (aGlyph.IsClusterStart() && !aGlyph.IsLigatureContinuation())', file ../../../dist/include/thebes/gfxFont.h, line 863 ###!!! ASSERTION: Ligature at the start of the run??: 'ligStart > 0', file /Users/phiw/cocoafox/mozilla/gfx/thebes/src/gfxFont.cpp, line 575 ###!!! ASSERTION: Unknown character type!: 'glyphData->IsComplexCluster()', file /Users/phiw/cocoafox/mozilla/gfx/thebes/src/gfxFont.cpp, line 553

This should fix that crash. I regressed the detection of the ATSUI bug ... the code to check whether the string character is a space was off by one.

(In reply to comment #17) > Created an attachment (id=256555) [details] > fix Ok, Arabic now works, but some other pages/languages on http://www.bbc.co.uk/worldservice/languages/ still go boom, particularly Hindi and Tamil (I gave up after those 2 ...). No problems with CJK pages, Cyrilic (tested the BBC), any Roman language. Multiple ###!!! ASSERTION: Lost some glyphs: 'glyph->originalOffset >= 2*index', file /Users/phiw/cocoafox/mozilla/gfx/thebes/src/gfxAtsuiFonts.cpp, line 590

Comment on attachment 256555 [details] [diff] [review] fix This looks fine, as far as I was able to understand it.. go forth!

I made additional changes to fix Mac crashes and to fix reftest failures. The Mac crashes were caused by glyph reordering like in comment #13 but with more general situations where K glyphs were moved to the left of J glyphs, for values of K or J greater than one. So I generalized the checks to work for K up to 10 and any J. The reftest failures were caused by the fact that gfxTextRun::Draw and DrawToPath round to the nearest pixel. Because ATSUI uses subpixel glyph placement, this was causing the markup "abc<!-->def" (two Draw calls) to render differently to "abcdef". I fixed this by removing the rounding in Draw and DrawToPath and having gfxAtsuiFontGroup round each glyph advance up to the nearest appunit so that aligning frames to appunits doesn't cause problems. I just checked this all in.

roc, as mentioned in bug 367884, Khmer unicode rendering has not been working on trunk builds for a very long time... High hopes every time I see you applying new text rendering related patches but always the same result. I confirm builds with this patch applied does not fix the problem either. Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a3pre) Gecko/20070305 Minefield/3.0a3pre

This regressed Tp on OSX by about 8%. Is that expected?

I'm working on it, see bug 372631. I think I'm close to a fix.

Could this have caused bug 404789?