This is a bug on implementing http://wiki.mozilla.org/Gecko2:NewTextAPI As this happens, it's worth coordinating on both ends of the pipe -- thebes and DOM. For example, the current setup for ASCII text on Linux has to do the following: 1) Rescan it for non-ASCII chars in case it has some (because text fragments store ISO-88859-1 and ASCII the same way). 2) Convert it to UTF16 when creating the thebes text run in PaintASCIIText. 3) Convert the UTF16 to UTF8 when passing the text to pango. It really feels like we should be able to avoid things like this if we sort of decide on how we're handling this end-to-end...

Note also http://wiki.mozilla.org/Mozilla2:GFXTextRun And ccing Stephen now that I know his bugmail. ;)

Perhaps we should change content to only store 8-bit text fragments if the text is genuinely ASCII? gfxTextRun is supposed to get an ASCII interface. We still need to compress whitespace in the common case, so the path would become just "textnode (ASCII) -> nsTextFrame (whitespace-compressed ASCII) -> gfxTextRun -> Pango" (because ASCII is UTF8). That's one copy, which isn't so bad, and if we want to be ultra-clever we can avoid the copy for whitespace:pre and when whitespace compression is a noop.

Yes, just storing 8-bit in the ASCII case is definitely one possibility. It'd make storage a little larger for the ISO-8859-1 case, but I think it's worth the memory usage, frankly, especially if it leads to simpler code and less rescanning of the data in nsTextTransformer and such.

Another alternative is to store as ISO-8859-1 but keep a flag to indicate whether it is ascii or not. That way we can convert to the appropriate type (utf16 or ascii) while we're compressing whitespace. Not sure if it's worth the extra codepaths though. If we want to just store m1b as ascii, we should still consider keeping the optimization to use shared static data for 1-char strings for all of ISO-8859-1. Specifically, this is very useful for   (unicode 160) since that appears on its own a lot.

oooh yeah  . Then I'd go with this isascii flag.

We can do the static storage for single-char   even if we'd only store ASCII as m1b. It'd just take another if-statement. But the   would of course make any string be stored as 2-byte, so it might affect storage space. There is data we can examine in bug 329974 we can examine to see how much storage size would be affected.

The work I have done on this can be found at http://blacksapphire.com/firefox-rtl/thebes/ It's a sstripped-down version of nsTextFrame that uses a persisstent gfxTextRun and breaks the text into lines efficiently - intended as a good sstarting point for a re-write. It's almosst ready to be checked in (once some reliability problems are addressed), but it is certainly not complete. I'm trying to find the time to work on it - if someone wants to take it up they're most welcome.

Please tell me, what is status of this bug? and what is a target milestone? I think that thebes needs to change the text selection code on nsTextFrame for suppressing the shaken characters. But I don't know this bug's status, therefore, I don't know whether I should work on that issue now.

*** Bug 351242 has been marked as a duplicate of this bug. ***

I'm working on this. I've nearly finished coding the new textframe with almost everything implemented, along with the gfxTextRun API that it's designed around, but I still have to implement the new gfxTextRun at least for Linux before this can land in any form. Also the new code depends on the inline layout changes in bug 343445.

This patch does not build. It's just a checkpoint so I don't lose my work if my office catches fire. nsTextFrameThebes.cpp does compile, though, and is fairly complete. The next step is to implement the new gfxTextRun interface so I can test things, and for that I'm waiting on bug 339513 whose fix rewrites gfxTextRunPango. A few other things need to be done as indicated in XXX and TODO comments, including implementing nsSmallcapsTextRunFactory and nsCaseTransformTextRunFactory. The design is mostly as we've planned. One decision that was not made earlier was how to handle the differences between DOM text and transformed text, in particular when characters are removed by whitespace compression and other discarding. Instead of retransforming text on every operation, as we used to do, I've created an abstraction called gfxSkipChars which records these deletions, and I store it in the gfxTextRun. I also have a gfxSkipCharsIterator which lets you traverse the original DOM text and the transformed text in tandem, translating offsets between them. nsTextFrame is responsible for performing all such mapping; gfxTextRun only deals with transformed text. The new textframe doesn't ever manipulate a transformed-text string directly; it always works from the DOM text using gfxSkipCharsIterator to account for deleted characters. I'm a bit worried about confusing DOM text offsets with transformed text offsets (and I worry about that in today's nsTextFrame too), but choosing signed integers for DOM offsets and unsigned for transformed offsets caught a lot of bugs at compile time. Overall I feel that the complexity of the new textframe code is reasonable, and a big improvement over our existing code. There's still a lot of work to do though.

Roc, note that I'm working on gfxPangoFonts.cpp for refactoring it in bug 339513. The patch is under review.

Yes, I mentioned that in my previous comment. I'm waiting for that to land before I try to mess with the Pango code :-)

This does not fully build, but the layout parts do. This is a major revision over the previous checkpoint. Main changes: -- made gfxTextRuns span text nodes with the same font (including text nodes that are in different inline elements but contiguous in the flow). The main motivation is that this enables kerning, ligatures and shaping across text nodes (if the platform textrun supports these things). This adds significant complexity which might hurt performance. But it could also help performance by building bigger textruns (e.g. a paragraph full of text and links will likely be turned into a single gfxTextRun). -- added support for first-letter and first-line (which don't fit into the persistent textrun model very well) -- implemented smallcaps, text-transform and most of the other TODO items. Next step is to implement the new gfxTextRun on Pango.

This builds. It does not run. Well, I haven't tried it, but I'd bet my house it doesn't work :-) I've finished coding up gfxPangoTextRun. There were only very minor changes to the textrun API. I added DrawToPath, and realized that it's impossible to handle partial ligatures in that function, so we need to expose ligature boundaries in the character flags to let callers decide what to do. It turned out to be very troublesome to handle partial ligatures everywhere else (drawing/measuring substrings that begin or end in the middle of a ligature) but I've bitten that bullet. The new code integrates with Masayuki's font selection code fairly cleanly. I've reimplemented the nsThebesFontMetrics text APIs on top of gfxTextRun. Things are now fairly complete.

This updates to (nearly) trunk, which was a big update because I had landed the inline reflow changes that this depends on, so they're no longer needed in this patch. I've fixed a number of bugs; it can actually render some websites now without crashing. Something I've noticed that we might want to think about --- there are a lot of textframes that are completely empty, i.e., have only skipped whitespace in them. We might want to completely avoid creating textruns for such frames. Currently they're not too expensive because they usually end up just being a single space in a larger textrun that covers nearby nodes with real text, but it might still be a win to avoid this. This patch disables vlad's textrun cache. I'm thinking of updating the patch so we can use the new textrun with the old textframe and the textrun cache; this should work OK, and be pretty stable, and address some of the Linux text performance "problem" (understatement ahoy!). I will try updating the Windows and Mac textrun implementations to the new API for use with the old textframe. (Shouldn't be too hard, just stub out the methods that the Thebes text wrappers don't need, which is all but two methods!) We should be able to get this working soon, in which case we can thinking about landing this for 1.9a1 with the new textframe disabled for now.

> Something I've noticed that we might want to think about --- there are a lot of > textframes that are completely empty, i.e., have only skipped whitespace https://bugzilla.mozilla.org/show_bug.cgi?id=68489

That's a little different. I'm talking about text frames where if they had nonwhitespace text, it would be rendered.

This has more bugfixes, mostly to gfxPangoTextRun for spacing, RTL and Unicode processing. I've temporarily configured new-textframe off in this patch, and I've reintegrated the textrun cache for old-textframe users. Things seem to work well with the new-textrun, old-textframe combination although I haven't done any performance measurements. I've also added a stub new-textrun implementation to gfxWindowsFonts that has not been tested, or even compiled, but hopefully won't be difficult to get working (as long as you stick to the old-textframe, because it doesn't implement any of the cool new APIs that new-textframe needs). So this is basically what I'd like to land soonish, if someone can get the Windows stub working. Almost exactly the same stub code should also get Mac working.

Something I did for nsTextFragment, which may be doable for textruns too is that I keep a static copy of a single-space buffer and use that everywhere where we want a node (or run) containing just a single space.

Okay, this is another major update. It builds but almost certainly doesn't work at all because I haven't tested it, I just want to get it out there. In this patch I've rolled in a new linebreaker interface in content/base/public/nsLineBreaker.h. Right now it mostly behaves the same as the current linebreaker, except it will be much more consistent and not vary its results depending on how it's called. However, it will support much more interesting contextual linebreaking, including Thai*, if/when we get around to implementing that. I've reorganized things so that line break opportunities are stored persistently in the textruns instead of being provided dynamically from the text frame. Line-break opportunities are computed paragraph-at-a-time via the new linebreaker interface. Also, CSS "white-space" linebreaking control is applied at that time instead of during nsTextFrame::Reflow, avoiding the nearest-common-ancestor stuff that we currently do. * There is one more thing that has to be added to get Thai or other weird contextual linebreaking to work properly, and that is to trigger reflows when a change to text in one frame changes the break opportunities in previous text, possibly on previous lines. I already have code to detect this, I just need to fill in the TODO space with code to actually ensure necessary reflows happen. I'll probably defer this for now.

Okay! Another major update. -- Updated to the reflow branch. -- Pushed *all* whitespace compression, including compression of leading whitespace for a text node (combining into previous whitespace), to textrun construction time. This simplifies things because intra-node and before-node compression are no longer handled separately. Removes TEXT_SKIP_LEADING_WS. -- Removed TEXT_TRIMMED_TRAILING_WS and just use TEXT_END_OF_LINE. -- Rewrote text-transform-capitalize so that we no longer have to track "start of word" state through nsLineLayout. The capitalization textrun can get that directly from the linebreaker. (Although this is unfortunately complicated because we have to be able to reconstruct textruns if the linebreak opportunities change.) -- Removed GetMaxUnbreakableWidth. It was broken because we need to treat whitespace runs differently from regular character words. Fixing that would have complicated the interface too much. It makes more sense to just have nsTextFrame::AddInlineMinWidth do everything using gfxTextRun::GetAdvanceWidth, which should be fast anyway since everything's precomputed. -- Tested it a bit and got it to the point of rendering at least one testcase correctly. -- Included all the files this time.

I guess I lied in the last comment

Comment on attachment 248609 [details] [diff] [review] checkpoint #7.1: fix missing files Okay, I would like to move toward landing this on trunk, with new-text-frame disabled as it is in this patch. This means we'll be using the new gfxPangoTextRun on Linux, but only a small part of its functionality (GetAdvanceWidth and Draw, basically). Most of the rest of this patch will not be executed. The only parts of the patch that will affect builds immediately and significantly are: -- gfxPangoTextRun changes -- new-textrun wrappers around old-textrun implementations on Windows and Mac (these need to be tested, help!) -- nsThebesFontMetrics wrapper changeover to the new-textrun interface -- gfxTextRunCache changes to the new interface -- Some refactoring of nsBlockFrame code to nsLayoutUtils -- Very minor change to track current line box in nsLineLayout -- Minor change to expose gCaseConv from nsTextTransformer -- extra nsTArray method (already submitted under separate bug) So I'm looking for dbaron-review on the layout parts of that, pav-review on the gfx parts, and no-review on the rest. Someone should build and test the Mac and Windows bits too.

Comment on attachment 248609 [details] [diff] [review] checkpoint #7.1: fix missing files Is there value in having the configure.in changes? It seems easier to flip a definition in layout/generic/Makefile.in than to flip one in configure.in and have to rebuild the world. I skipped everything in gfx/. You introduced a |class nsIAtom;| in nsILineBreaker.h for no apparent reason. It seems like the nsILineBreaker change should be called GetCJKBreaks, not SetCJKBreaks. And is it really specific to CJK? It doesn't look it (there's Thai stuff). Why not just GetBreaks? And you should document that the aLength applies to *both* the first and third parameters. Your declarations/definitions of SetCJKBreaks disagree about the name of the third parameter. In SetCJKBreaks, the NS_ASSERTION that aLength > 0 doesn't seem like something that could be a problem, although maybe there's a good reason to want it for the future. In SetCJKBreaks, are the repeated calls to TrbWordBreakPos needed, or could you call it only once per break within a run of Thai? nsLayoutUtils: FindImmediateChildOf is a bad name. If you're moving it to nsLayoutUtils, could you rename it to FindChildWithDescendant or FindChildContainingDescendant? ("Immediate child" seems to be the terminology of people who use "child" to mean "descendant" and then realize they need something to mean "child".) And please fix the comment too. I'd prefer to avoid adding line_at if you can pass line iterators through to where you need them. (I've long been planning to test the performance characteristics of using XORed pointers to save a word in the block frame, to trade with an increase in the size of the iterators.) If it's easier, feel free to land as as and do a followup patch. Or, if it's particularly hard, don't bother and just leave it. (This doesn't seem too hard, except that what you'd want carried in the line layout is line_iterator | NONE rather than just line_iterator.) The comment change in nsLineLayout::ReflowFrame should probably be reverted. These changes look like they'll make it even more important that we fix 320502. I skipped the nsTArray.h changes. Do you have changes to the current nsTextFrame? It looks like you've factored some pieces out into nsTextFrameUtils. If not, why is that a separate file? Or should it be called nsTextUtils? Your comment near the beginning of nsLineBreaker.h about CSS semantics should probably say that you're talking about stuff that's undefined by the CSS spec, but you're describing how we implement the 'white-space' property. The modelines at the beginning of nsTextFrameUtils.h and .cpp, gfxSkipChars.h and .cpp, and nsTextRunTransformations .h and .cpp are incorrect (they specify 4-space indent, but the files have 2-space indent; the 20-space tabs also seem quite odd and perhaps not totally irrelevant if an emacs user types <TAB>.). Other than that, I haven't looked at the new files -- and since you don't seem to want review on them at this time, I'll give you r+sr=dbaron on the non-gfx changes other than new files, which I think is what you were asking. That said, I should probably try to understand the stuff in the new files at some point.

I tried building this on Windows, but the patch is missing gfxRect.cpp. There were a few trivial merge conflicts to resolve, too.

(In reply to comment #25) > Is there value in having the configure.in changes? It seems easier to flip a > definition in layout/generic/Makefile.in than to flip one in configure.in and > have to rebuild the world. Good point. I thought I might want to conditionalize stuff in gfx but in the end I haven't. > You introduced a |class nsIAtom;| in nsILineBreaker.h for no apparent reason. I'll remove it. > It seems like the nsILineBreaker change should be called GetCJKBreaks, not > SetCJKBreaks. And is it really specific to CJK? It doesn't look it (there's > Thai stuff). Why not just GetBreaks? I'll call it GetJISx4501Breaks. The Thai stuff isn't even JISx4501, but there is no name for the mess we're doing... > And you should document that the > aLength applies to *both* the first and third parameters. Sure > In SetCJKBreaks, the NS_ASSERTION that aLength > 0 doesn't seem like something > that could be a problem, although maybe there's a good reason to want it for > the future. No, I'll remove it. In the future I want this entire interface and component to go away and be replaced by an implementation of UAX#14 plus proper Thai plus Web-specific modifications, living in content/base/src/nsLineBreaker.cpp. > In SetCJKBreaks, are the repeated calls to TrbWordBreakPos needed, or could > you call it only once per break within a run of Thai? I don't know and frankly I don't care :-). > FindImmediateChildOf is a bad name. If you're moving it to nsLayoutUtils, > could you rename it to FindChildWithDescendant or > FindChildContainingDescendant? ("Immediate child" seems to be the terminology > of people who use "child" to mean "descendant" and then realize they need > something to mean "child".) And please fix the comment too. Sure, FindChildContainingDescendant. > I'd prefer to avoid adding line_at if you can pass line iterators through to > where you need them. (I've long been planning to test the performance > characteristics of using XORed pointers to save a word in the block frame, to > trade with an increase in the size of the iterators.) If it's easier, feel > free to land as as and do a followup patch. Or, if it's particularly hard, > don't bother and just leave it. (This doesn't seem too hard, except that what > you'd want carried in the line layout is line_iterator | NONE rather than just > line_iterator.) It's doable. I'll do it. > The comment change in nsLineLayout::ReflowFrame should probably be reverted. OK > These changes look like they'll make it even more important that we fix > 320502. Indeed. > Do you have changes to the current nsTextFrame? No. That's a good thing. > It looks like you've factored > some pieces out into nsTextFrameUtils. If not, why is that a separate file? > Or should it be called nsTextUtils? I wanted to reduce the size of nsTextFrameThebes.cpp by splitting out stuff that could be easily separated with a clean interface, that's all. > Your comment near the beginning of nsLineBreaker.h about CSS semantics should > probably say that you're talking about stuff that's undefined by the CSS spec, > but you're describing how we implement the 'white-space' property. OK. > The modelines at the beginning of nsTextFrameUtils.h and .cpp, gfxSkipChars.h > and .cpp, and nsTextRunTransformations .h and .cpp are incorrect (they specify > 4-space indent, but the files have 2-space indent; the 20-space tabs also seem > quite odd and perhaps not totally irrelevant if an emacs user types <TAB>.). OK. > Other than that, I haven't looked at the new files -- and since you don't seem > to want review on them at this time, I'll give you r+sr=dbaron on the non-gfx > changes other than new files, which I think is what you were asking. Right. > That said, I should probably try to understand the stuff in the new files at > some point. When I'm ready to commit I'll write a treatise explaining how it all works. Hmm, wiki, blog, code comments, or separate doc file in CVS?

i'll get through this today or tomorrow

> The modelines at the beginning of nsTextFrameUtils.h > and .cpp, gfxSkipChars.h and .cpp, and > nsTextRunTransformations .h and .cpp are incorrect > (they specify 4-space indent, but the files have > 2-space indent; the 20-space tabs also seem quite odd > and perhaps not totally irrelevant if an emacs user > types <TAB>.). These modelines are actually copied from others in gfx. So what I'll do is give nsTextRunTransformations.* and nsTextFrameUtils.* 2-space-indent modelines like other files in layout, and leave the gfxSkipChars.* modelines alone but reindent those files to 4 spaces.

Really fix all missing files, I mean it, honest! Also incorporates changes requested by dbaron. Also an attempt at copying the Windows wrapper code for use on Mac (gfxAtsuiFonts). Still no idea whether these wrappers work.

(In reply to comment #27) > I'll call it GetJISx4501Breaks. The Thai stuff isn't even JISx4501, but there > is no name for the mess we're doing... At least in new code and comments, please refer to it consistently as JISx4051, not 4501.

Fortunately, I did :-)

Comment on attachment 248692 [details] [diff] [review] checkpoint #8 >+ DetailedGlyph* details = mDetailedGlyphs[...] Cause crash in most cases in gfxPangoFonts.cpp, because mDetailedGlyphs array even not initialized.

Which one are you talking about? That code occurs in many places. What's the testcase? In general we should initialize mDetailedGlyphs in SetGlyphs here if it's required: + if (!mDetailedGlyphs) { + mDetailedGlyphs = new nsAutoArrayPtr<DetailedGlyph>[mCharacterCount];

Yes it should, but it never comes in that SetGlyphs... + } else { + if (!mDetailedGlyphs) { You can try to load for example: www.linux.org.ru or http://www.oikotie.fi/kartta It should crash in gfxPangoTextRun::GetAdvanceWidth ~ gfxPangoFonts.cpp:1886 .... I have tried to set: - } else if (glyphData->IsComplexCluster()) { + } else if (glyphData->IsComplexCluster() && mDetailedGlyphs) { in all places, http://www.oikotie.fi/kartta - works ok. but after that it crashes somewhere here: www.linux.org.ru gfxPangoTextRun::GetAdvanceWidth ShrinkToLigatureBoundaries(&ligatureRunStart, &ligatureRunEnd); With message ...(this=0x0...)... Also I got some another crash on www.ft.com in nsTextFrameThebes.cpp need to build it again to reproduce with exact place of crash... My config options for linux: --enable-default-toolkit=cairo-gtk2 ac_add_options --disable-freetype2 MOZ_ENABLE_NEW_TEXT_FRAME = 1 in layout Makefile.in

updated patch with code that builds on windows and seems to work (I should do more testing) and is also merged to the trunk.

This makes Mac build with the textrun wrapper. Unfortunately I can't test it because my Mac builds don't currently work. I also tried to fix a Windows/Mac RTL problem that smontagu reported.

(In reply to comment #37) Hi roc Tab codes in your patch is converted to spaces. Could you prepare a patch without "-t" in diff command?

Patch without -t

Patch v10.1 works fine on Mac OS X. I tested it on a variety of sites including a couple rtl sites. I also poked around in chrome.

I don't see any more RTL issues on Windows in the latest patch.

OK, I think after stuart has re-merged and posted a new patch, we can land.

this is just gfxPangoFonts.cpp and gfxPangoFonts.h which are currently wrong in 10.1

Update to trunk, incorporate stuart's Pango merges, and fixes a gfxPangoTextRun bug that computes the wrong GetAdvanceWidth for a string with non-compressed glyphs (e.g. a glyph with an advance width that's not an integral number of pixels).

I cannot build on Win32... MOZ_ENABLE_NEW_TEXT_FRAME is not set, it cannot build nsEditor. d:/m/mozilla-c/mozilla/editor/libeditor/base/nsEditor.cpp(4306) : error C2039: 'BeginReflowBatching' is not a member of : 'nsDerivedSafe<T>' with [ T=nsIPresShell ] d:/m/mozilla-c/mozilla/editor/libeditor/base/nsEditor.cpp(4362) : error C2039: 'EndReflowBatching' is not a member of : 'nsDerivedSafe<T>' with [ T=nsIPresShell ] Otherwise, d:/m/mozilla-c/mozilla/layout/generic/nsTextFrameThebes.cpp(1660) : error C2440: 'const void *' cannot cast to 'void *'

Sorry, the changes to nsIPresShell and nsPresShell.cpp should not be part of this patch, they're part of another patch.

I checked this in (minus the presshell changes, of course). File any regressions as blockers of this bug. I've created a new bug to track new-textframe work --- it's bug 367177. I'll make it block this bug, so leave this bug open.

I backed this out because it caused major Tp regressions on all platforms and caused crashes on Windows. I'm going to investigate the problems and also reland parts of the patch that can be treated as independent pieces. Hopefully that will narrow down the problems.

The new files were never backed out actually. Also, I relanded much of the innocuous-looking code that could stand alone. Tinderboxes seem fine so far. This patch is all that's left --- basically, the textrun changes, plus the change to nsTextFragment.h. I suspect it's the latter that's caused the Windows crash, based on comments about problems from smontagu and CTho. I'll verify that that's the problem and reland a fixed patch for that (probably just need to change PRPackedBool to unsigned int, although I'm not sure why PRPackedBool (i.e. unsigned char) isn't working).

This patch is merged attachment 250580 [details] [diff] [review] of bug 357637. The patch fixes the hung up bug at failing the auto detect. # but not tested, because I cannot build.

Oops, this patch only has gfxPangoFonts.h and gfxPangoFonts.cpp.

There are memory leaking bugs.

Sorry for the spam.

fix the some bugs. But this patch still crashes...

fix the crash.

Umm... The checkpoint #12 breaks the text rendering with spacing. Because in |nsThebesFontMetrics::DrawString|, the code for rounding to dev pixel is dropped. See: http://www.d-toybox.com/studio/weblog/show.php?mode=single&id=2007010608 this page uses letter-spacing and justification.

Roc: It seems that the RTL text is not rendered correctly on Linux. And my patch crashes with RTL text...

+ gfxTextRun* tr = nsnull; + // Don't cache textruns that use spacing + if (!gDisableCache && !aEnableSpacing) { + // Evict first, to make sure that the textrun we return is live. + EvictUTF16(); + + gfxTextRun *tr; Spot the obvious bug here. This is what was causing the massive performance hit. (Shadowing of the outer tr by the inner tr was causing us to never notice that the textrun was in the cache, even after we just added it, so we'd always create a fresh textrun, and in cache-miss situations we'd actually create TWO textruns.)

I forgot to move the comment, sorry.

I want the crash and leak fixes but I don't want the fix for bug 357637; I'm guessing vlad will want that to land after we have the glyph selection tests running, which is waiting for this to land.

I don't see where the crash and leak fixes are in the patch. Can you separate them out into their own patch? I see that you added a g_object_unref(mPangoFont) to ~GlyphRun, but I think we never addrefed that mPangoFont ... did we?

Also, can you describe how to reproduce the crashes and leaks that you're fixing?

(In reply to comment #63) > I don't see where the crash and leak fixes are in the patch. Can you separate > them out into their own patch? The crash may be the bug of my patch. I can reproduce the crash: http://arabic.cnn.com/ And the without my patch, the page can be rendered without crash. But the RTL texts are not correct. 1) The all text are in wrong position. 2)It looks like that the orders of the all words are reversed by your patch. The pango is sorted the glyphs in visual order (i.e., LTR), but your patch may reverse the orders. > I see that you added a g_object_unref(mPangoFont) to ~GlyphRun, but I think we > never addrefed that mPangoFont ... did we? I guess that the leaking is on |pango_context_load_font| of |GetPangoFont|. Because it is not unrefed in anywhere. And I think that we should add ref to PangoFont* at caching in the class. The leaking is not confirmed on testing, but the unrefed PangoFont* doesn't cause crash. And the font is added the ref in |pango_fc_font_map_load_font|. But I cannot found the unrefed point...

The left is Linux with checkpoint #12, the right is Windows (2007011504).

And Hebrew too..

The mPangoFont is added in attachment 251820 [details] [diff] [review]. But that is needed for HasGlyph.

Roc: we don't need |pango_reorder_items| in |gfxPangoTextRun::CreateGlyphRunsItemizing|. I removed this, it looks like the correct word order. But there is still the wrong offset problem.

> NS_ASSERTION(isRTL == item->analysis.level % 2, "RTL assumption mismatch"); This assertion is raised in the CNN arabic. I can see this assertion. At the time, the |isRTL| of |CreateGlyphRunsItemizing| means LTR, but the item is RTL. I don't know why this happens.

fix the crash. And the RTL text wrong positioning. The cause of crash is strange. Because the RTL code of checkpoint #12 is not good, because it didn't assume that the segments have different direction. But it happens and it causes the crash of my patch. The wrong positioning of RTL text is fixed by this change too.

The diff for RTL problems, see: https://bugzilla.mozilla.org/attachment.cgi?oldid=251820&action=interdiff&newid=251949&headers=1

And I found serious bug in checkpoint #12. See this page: http://www.d-toybox.com/studio/lib/romanNumerals.html This page has the minor characters. By this page, we may lost some fonts until reboot fx... (some fonts are not rendered in other pages after show the page.)

The left side is the screenshot immediately after loading the page. The right side is after scrolling the page to bottom and back to top. In the right side, some ASCII text is not shown. This bug is happen in other pages too until rebooting the Fx.

fix the "lost font" bug. If the glyph id is missing glyph and it is processed in |cairo_show_glyphs|, the font doesn't work fine after it. We should not pass the unknown glyph to cairo. I don't know the reason. Note that we may boost up the speed of rendering the unknown glyph. For it, we need both FontSelector::AppendMissingSegment and gfxPangoTextRun::SetMissingGlyphs. And we set the glyph id, advanced width, x offset and y offset to glyph cache without pango_shape...

The diff between the previous patch and the latest patch is only for fix the "lost font" bug. https://bugzilla.mozilla.org/attachment.cgi?oldid=251949&action=interdiff&newid=252055&headers=1

There's a fundamental problem here, which is that the new textrun abstraction is designed to handle one direction only per textrun --- either LTR or RTL --- but current nsIRenderingContext APIs handle mixed-direction text to some extent. GetWidth handles mixed-direction text fully, more or less, and doesn't care about any direction being set, and DrawString doesn't do well but doesn't die. So when we wrap gfxTextRun in those nsIRenderingContext APIs, we really want to make sure the text is unidirectional and that the direction has been specified. In many places this requires additional information to be passed from callers, and when mixed-directional text may be present, we need to do bidi resolution on the string and draw (or measure) each single-direction segment separately. When we're measuring width, we don't really need to set the direction correctly as long as the string is in one direction, but it's helpful to do so because then the textrun cache will work better. I've worked up a patch to do this that can land on its own before we land the remaining gfx work here. It adds a method to nsIRenderingContext, which is unfortunate since we want that interface to go away, but this may be the best interim solution that allows us to make forward progress. One thing I've noticed is that the computation of inline min-width and pref-width for textframes can happen before reflow, thus before the splitting of text frames into single-direction runs, and therefore must handle mixed-direction text. This is very unfortunate and should be fixed by moving bidi resolution to frame construction time, something we want to do anyway. We can hack around this for now, at some performance cost that be incurred on bidi-enabled pages. As an alternative to all this we could alter the textrun API to allow mixed-direction strings in a textrun. It'll make the API and implementation more complicated, and we won't use the mixed-direction ability most of the time, but it would be be easier for non-textframe consumers. We could pass a flag to the textrun when we know we're sending a single-direction string, to allow implementations to be more efficient in that (very common) case. We could perhaps even implement a generic cross-platform wrapper text run that does bidi resolution and stitches together a series of platform-level single-direction text runs into one. I'll think about this approach a bit more.

(In reply to comment #77) > when mixed-directional text may be present, we need to do bidi resolution > on the string and draw (or measure) each single-direction segment separately. FWIW, we do something similar to that for XUL text in nsBidiPresUtils::RenderText

I checked the new spacing calculation code. The new text frame sets the spacing to |Spacing|. Therefore, we cannot fix the rounding bug in gfx. I think that we should fix it in layout code. Should I separate it to new bug?

What rounding bug is that? After more thought, I don't think we should make textruns multi-directional. It would complicate their API a lot (things like GetAdvanceWidth on a substring would no longer make sense when the substring can be visually discontiguous). Furthermore this code that is measuring and drawing multi-directional strings only ever cares about the entire string, not substrings, so the complication is wasted. So we should have a different API that allows measuring and drawing of multi-directional strings, the whole string only, similar to (or equal to) nsBidiPresUtils::RenderText which is used for XUL as Simon mentioned. And that's pretty much what my new patch does; adds two new nsLayoutUtils methods, GetStringWidth and DrawString, which wrap nsBidiPresUtils::RenderText and a new method nsBidiPresUtils::GetStringWidth. Those nsBidiPresUtils methods do bidi resolution and then measure or draw the whole thing, ensuring that only single-direction substrings are passed to nsIRenderingContext::GetWidth or DrawString. All code that calls string methods on nsIRenderingContext is audited and if the strings are guaranteed unidirectional, we set the direction via nsIRenderingContext::SetTextRunRTL (new API). Otherwise we call the new nsLayoutUtils methods to handle possible multidirectional text.

eventually we should combine the multidirectional measure and draw APIs into a single object which contains a collection of gfxTextRuns, so we don't need to do bidi resolution and glyph conversion twice.

(In reply to comment #80) > What rounding bug is that? see comment 57. your patch breaks the spacing rendering. there are two bugs. 1. at selecting the spacing text, the characters are shaken. 2. the text is overflowed from nsTextFrame. I have fixed it in bug 327184 comment 37.

And new text frame doesn't care the "extra jot" for justification. This should be a cause of failing to align the text to end edge.

(In reply to comment #57) > See: http://www.d-toybox.com/studio/weblog/show.php?mode=single&id=2007010608 > this page uses letter-spacing and justification. This doesn't work correctly for me on trunk or in FF2 (Mac), the right edge is not straight in either case. I think we should work on this later.

Ok. This is checkpoint #12 updated with fixes for most of the issues --- the issues that I could reproduce. In particular -- Don't render missing glyphs (I slightly adapted Masayuki's code, thanks!!) -- Refcount PangoFonts used in GlyphRuns -- Fix shadowing of "tr" so that textrun cache works properly -- Big one: modify users of nsIRenderingContext string methods to call SetTextRunRTL (when operating on a string known to be single-direction) or call new nsLayoutUtils::GetStringWidth/DrawString methods (when operating on a string which may be mixed-direction). nsLayoutUtils::GetStringWidth is #ifdefed to just call nsIRenderingContext::GetWidth for non-X11 platforms, for now, because we assume those platforms' GetWidth implementations can handle mixed-direction strings. This is to prevent a performance regression on those platforms when textframes start doing bidi resolution for AddInlineMin/PrefWidth. -- Make gfxPangoTextRun insert a LRO (left-to-right override) or RLO (right-to-left override) character at the start of the string, when necessary, so when itemizing we force Pango to use our direction. Potentially fixes some issues with neutral characters, maybe -- Fixed a bug in FontSelector segmentation code (new code is + if (!IS_MISSING_GLYPH(info->glyph)) { + missingLength = aOffset + clusters[j] - offset; ) With this patch we render haaretz.co.il and arabic.cnn.com almost identically to trunk, as far as I can tell. I need someone to look at the layout code and check that what I'm doing there makes sense. David, would you mind terribly? :-)

Comment on attachment 252279 [details] [diff] [review] checkpoint #13 oops. Again, just requesting a look at the layout changes related to nsIRenderingContext string method calls.

Actually smontagu could look at the layout changes as well...

(In reply to comment #84) > (In reply to comment #57) > > See: http://www.d-toybox.com/studio/weblog/show.php?mode=single&id=2007010608 > > this page uses letter-spacing and justification. > > This doesn't work correctly for me on trunk or in FF2 (Mac), the right edge is > not straight in either case. I think we should work on this later. It doesn't work only on Mac. Because we wait this bug, therefore, we don't implemented on Mac. On Win and Linux, it works fine.

(In reply to comment #88) > (In reply to comment #84) > > (In reply to comment #57) > > > See: http://www.d-toybox.com/studio/weblog/show.php?mode=single&id=2007010608 > > > this page uses letter-spacing and justification. > > > > This doesn't work correctly for me on trunk or in FF2 (Mac), the right edge is > > not straight in either case. I think we should work on this later. > > It doesn't work only on Mac. Because we wait this bug, therefore, we don't > implemented on Mac. On Win and Linux, it works fine. > Oops, fx2 doesn't work fine???

Yeah, I get a ragged right margin on Fx2. In fact it looks a lot like the Linux situation with my patch :-).

Well, SuSE's Linux Fx2 build isn't properly justified either. My patch makes things a little worse because the text-decoration doesn't extend all the way to the end of the line --- I presume that's accumulated rounding error. But perhaps we can let that go for now...

Um.. It's strange. I don't have problem on Fx2 Win. But it is true which we need more work on trunk. Let's try to work on it in new bug.

roc, you don't merge the attachment 250580 [details] [diff] [review] (bug 357637). by this, your patch will cause a hang up at failing the auto detect. Should I fix again it after your check-in?

yes, I'd like to land that separately please.

o.k. good. I have another worry. the patch looks like *always* setting the TEXT_ABSOLUTE_SPACING for gfxTextRun. if we build it with the new text frame, isn't it a wrong? Or I misread the patch?

(In reply to comment #92) > Um.. It's strange. I don't have problem on Fx2 Win. But it is true which we > need more work on trunk. Let's try to work on it in new bug. > Windows has completely different internal kerning tables for (common) Japanese fonts than Mac OS X or or most Linux distros I've seen. Text-justification would work nicely on your page without the letter-spacing. --- @roc, I tried to apply checkpoint #13 here on OS X. That failed: $ patch -p0 < 333659-patch.patch ... patching file gfx/thebes/public/gfxFont.h Hunk #2 FAILED at 141. Hunk #3 FAILED at 315. ...

Crashes on this URL: "data:text/plain,a%00" linux, cairo-gtk2 #3 0xa5b95778 in gfxPangoTextRun::CompressedGlyph::SetComplex (this=0x37d, aTag=1) #4 0xa5b957b1 in gfxPangoTextRun::CompressedGlyph::SetLigatureContinuation (this=0x37d) #5 0xa5b90ccd in gfxPangoTextRun::SetGlyphs (this=0x84100c8, aUTF8=0x8405358 "a", aUTF8Length=2, aUTF16Offset=0xafb9ef98, aGlyphs=0x840c220, aOverrideSpaceWidth=0, aAbortOnMissingGlyph=1) #6 0xa5b93323 in gfxPangoTextRun::CreateGlyphRunsFast (this=0x84100c8, aUTF8=0x8405358 "a", aUTF8Length=2, aUTF16=0x0, aUTF16Length=0) #7 0xa5b933d6 in gfxPangoTextRun::Init (this=0x84100c8, aParams=0xafba03d4, aUTF8Text=0x8405358 "a", aUTF8Length=2, aUTF8HeaderLength=0, aUTF16Text=0x0, aUTF16Length=0) #8 0xa5b93a1a in gfxPangoTextRun (this=0x84100c8, aGroup=0x840a920, aString=0x8405358 "a", aLength=2, aParams=0xafba03d4)

(In reply to comment #85) > -- Make gfxPangoTextRun insert a LRO (left-to-right override) or RLO > (right-to-left override) character at the start of the string Cunning! The layout changes look good to me.

Comment on attachment 252279 [details] [diff] [review] checkpoint #13 Should the nsBulletFrame change use (level & 1)? There are random whitespace changes in nsTextFrame and nsTextBoxFrame (one each, trailing whitespace in the latter case). Could you wrap the code and (especially) comments in nsTextFrame::AddInlineMinWidth (and Pref) at less than 80 characters? And add an "XXX" before the comment about how things are broken? r=dbaron on the layout changes

> Should the nsBulletFrame change use (level & 1)? No, level is only 0 or 1 here. Checked in the layout stuff with the other changes. Found an issue with nulls (thanks romaxa) ... g_utf8_find_next_char works on null-terminated strings :-(. Fixed in gfxPangoTextRun. I'll try to land the gfx stuff tomorrow, if the results of this latest checkin are OK. Hard to tell right now since bonsai is recovering from problems. masayuki: the textrun cache always sets ABSOLUTE_SPACING, but nsTextFrameThebes doesn't; it uses relative spacing.

I filed the bug 367848 for spacing rendering. roc: thank you, I found the code for creating the textrun in nsTextFrameThebes.

I relanded the gfx code. Tinderboxes are green, there's a slight Tp regression on Mac, and a more significant one on bl-bldlnx01, a few % by the look of it. I'll have a look in the morning to see if there's any quick way we can get some back, but we might just have to stick it out for now.

cairo-gtk2, Linux, gfxPangoFonts.cpp Still exists problems with IsComplexCluster and NULL == mDetailedGlyphs... I think there are some problems with Complex definition... Also: Gecko Hungs on URL:"data:text/plain,%D0%9F" See Fuctions log in attachment

(In reply to comment #103) > Also: > Gecko Hungs on URL:"data:text/plain,%D0%9F" I cannot reproduce it with UTF-8 on FC6.

> I cannot reproduce it with UTF-8 on FC6. > It very strange... may be problem in Pango version... Debian unstable: libpango1.0-0: "1.14.8-4" I have both problem almost on every 1/2 page... google.com, ya.ru, .... :(

in my environments, the version is 1.14.8-1.fc6.

fmm. I found a bug which sometimes drops the first letter of segment. But I don't found the wrong code. However, I cannot reproduce it with my patch for bug 357637. I think the bug is in FontSelector::InitSegments.

romaxa: can you reproduce the hung up with attachment 252460 [details] [diff] [review]?

Also reproducible "data:text/plain,%D0%9F" MakeTextRun (.., aLength = 0, ...) { gfxPangoTextRun() - here initialization finished with mCharacterCount = 0 -> Hungs/Crashes.... } "data:text/plain,P" MakeTextRun (.., aLength = 1, ...) { gfxPangoTextRun() - here initialization finished with mCharacterCount = 1 -> All works fine... }

Ah, you are enabling the nsTextFrameThebes? I don't test it.

> Ah, you are enabling the nsTextFrameThebes? Hmm... Yes... But I should not do that?

I don't know the completeness of the new text frame. I guess that it's the bug of new text frame, not gfx...

Yes, there are (ThebesFrame) some problems with calculations of transformed length...

The new text frame is very far from complete. That is what I'll be working on next. Hopefully I'll get some help. I'll be writing more docs for it. Any regressions from the landing of the code in this bug --- with the new text frame disabled, i.e., bugs found on trunk --- should be filed as bugs blocking this bug. Any bugs with the new text frame enabled should be filed as bugs blocking bug 367177. Thanks!

Comment on attachment 252279 [details] [diff] [review] checkpoint #13 >+void >+gfxWrapperTextRun::Draw(gfxContext *aContext, gfxPoint aPt, >+ PRUint32 aStart, PRUint32 aLength, >+ const gfxRect* aDirtyRect, >+ PropertyProvider* aBreakProvider, >+ gfxFloat* aAdvanceWidth) >+{ >+ NS_ASSERTION(aStart == 0 && aLength == mLength, "Can't handle substrings"); >+ SetupSpacingFromProvider(aBreakProvider); >+ gfxPoint pt(aPt.x/mPixelsToAppUnits, aPt.y/mPixelsToAppUnits); >+ return mInner.Draw(mContext, pt); >+} gfxWindowsFonts.cpp(639) : error C2562: 'Draw' : 'void' function returning a value

What's up with the "for (;;) ;" stuff in the inline method definitions in gfxWindowsFonts.cpp that were added with attachment 252279 [details] [diff] [review]?

I have a question. Who is working for new textrun for Win and Mac?

> Who is working for new textrun for Win and Mac? roc, in bug 370588 (fixed last week).

I've just landed a major update to the new-textframe code. You should be able to build it and browse with it on all platforms. There are still some major bugs --- in particular caret movement is rather broken --- but it can really be tested now. If anyone tests, file bugs as blocking bug 367177. Turn it on by setting MOZ_NEW_TEXTFRAME to 1 in layout/generic/Makefile.in and rebuilding layout. I can run through the Tp2 test set. Right now turning on new textframe is a slight slowdown, but some analysis shows a) more than 30% of textruns created are for a single space --- easily optimized b) it doesn't use a textrun cache, but if it did, then (assuming space-only runs were already taken care of) we'd get a 30% cache hit rate. Using a cache is therefore probably a good idea. So I'm hopeful that we'll see a significant Tp win here after some work.

I've checked in more fixes. The new-textframe now passes reftests. There aren't many tests to specifically test textframe features, but it's still a good sanity check.

I suppose this is fixed now?

Yes.