Closed Bug 36146 Opened 24 years ago Closed 22 years ago

GetWidth optimizations need to be implemented on Unix (text measurement performance)

Categories

(Core :: Layout, enhancement, P3)

Product:
Core
Component:
Layout
Platform:
x86
Linux
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla1.1beta

People

(Reporter: akkzilla, Assigned: roland.mainz)

References

Details

(Keywords: perf, platform-parity, Whiteboard: nsbeta3-)

Attachments

(1 file, 9 obsolete files)

New patch for 2002-08-04-08-trunk
81.72 KB, patch
smontagu
: review+
darin.moz
: superreview+
Details | Diff | Splinter Review 
See bug 36145 for the Mac version of this bug (which came from a thread on
mozilla-xpcom).  Erik, please close this if it's not relevant to Unix for some
reason.

troy implemented a version of nsRenderingContext::GetWidth for Windows that
measures text in larger chunks, for performance reasons. This needs to be
implemented on Unix too.
*** Bug 35980 has been marked as a duplicate of this bug. ***
Status: NEW → ASSIGNED
Target Milestone: --- → M17
I ported windows code to gtk. Seems that performance doesnt get much
beter, how big gain this is on windows?

I do some more testing with performance and attach patch later.
Here is patch. It currently brakes gfx/ps. I calculated average font
size from my name, looks like average gets bit too wide now.  Also
tryed "iW", but thats too wide.
Depends on: 50998
nominating for beta3

erik - have you had a chance to look at this patch?  we need a safe way to stub
out these functions on other toolkits (including ps).
Keywords: nsbeta3
Target Milestone: M17 → M18
Tomi, in your tree with this patch, do you render 
http://home.netscape.com/ja
http://home.netscape.com/ko
correctly ?


Ftang, very good guestion... there is lots of strange figures :)

I guess it's ok, and nightly build looks quite same as with my patches,
but hard to say it that right.
2nd question. Have we proof implement this will make it FASTER on Linux ? I know 
it is faster in window because troy quantify it. But I am not sure this will be 
FASTER on linux.

erik- what is your opinion ?
We ought to measure the speed change (if any). My guess is that it won't make
that much difference on Unix since the measurement routines are in Xlib anyway,
and don't need to call the X server, while on Windows, the measurement routines
have to go into a different CPU state(?) or something since it's in GDI
(according to Troy). The speed-up is pretty big on Windows, but somehow I don't
think it'll be as big on Unix. It would be good to measure the difference.
erik- how about the correctness of the patch ? 
I don't have time to look into unimportant things right now. I have high
priority bugs to work on.
nsbeta3- this bug. Tomi, can we wait. This is risky changes. It will be nice if 
we can check in after we sure this will help performance.
Whiteboard: nsbeta3-
bstell please measure the performance before we take the patch.
Assignee: erik → bstell
Status: ASSIGNED → NEW
Status: NEW → ASSIGNED
*** Bug 59436 has been marked as a duplicate of this bug. ***
I have had a chance to review code which in general looks good. However 
since the time the patch was submitted the windows version has had some 
patches which are applicable here.

Tomi, how would you like to proceed? 

If you would like to update your patch I can give you a summary of the changes 
I've found. I you would prefer it I can make an updated patch.
Target Milestone: M18 → Future
reconsider performance work for m9.4
Target Milestone: Future → ---
we discuss this again. we still do not believe this patch will speed up the
performacne on Linux. move to future
Target Milestone: --- → Future
--> ftang
Assignee: bstell → ftang
Status: ASSIGNED → NEW
bulk move NEW FUTURE bug to ASSIGN
Status: NEW → ASSIGNED
Attachment #12485 - Flags: needs-work+
Summary: GetWidth optimizations need to be implemented on Unix → GetWidth optimizations need to be implemented on Unix (text measurement performance)
Keywords: mozilla1.1
I would like to point out that on pages with large tables we are
spending something like 25% of our time in text-measurement.

So _any_ improvement in text measurement, even only a few percent,
would be a nice win on those pages....  So far I see no hard numbers
in this bug one way or the other, just a lot of handwaving saying
that people think this will maybe possibly not help performance as
much as it did on Windows.  What would it take to get the patch to
a point where a performance estimate could be done?  It doesn't have
to be perfect, just has to work.
I did some profiling and lot of time is spend measuring text in linux too. I made
dirty hack from windows code to gtk and get it work somehow (it compiles and
runs). I didnt yet profile this and compare it to old method.

There is couple of places still to be fixed.
Attached patch patch #2 for GetTextDimensions() (obsolete) — — Splinter Review 
To get this thing compile you need to run configure:
configure  --disable-xprint --disable-postscript
becouse it breaks xprint and postscript gfx.
Any plans to implement this on gtk2 as well?
Attachment #85987 - Flags: needs-work+
Attachment #12485 - Attachment is obsolete: true
Here is some measurements with long html page. (one 40000 line paragraph).

With old method it takes about 30sec to load that page from local disk, with
patch it takes about 24sec.

Numbers with eazel profiler, #1 is with patch, #2 is old

Function Name                calls   func%   func(ms)  f+c%     f+c(ms) 
#1 nsTextFrame::MeasureText()   43905   7.303   1836      29.132   7324  
#2 nsTextFrame::MeasureText()   38796   7.150   2231      44.179  13787


f+c is dropped to almost half.
Comment on attachment 85987 [details] [diff] [review]
patch #2 for GetTextDimensions()

>Index: gfx/public/nsIFontMetrics.h
>===================================================================
>RCS file: /cvsroot/mozilla/gfx/public/nsIFontMetrics.h,v
>retrieving revision 1.21
>diff -u -r1.21 nsIFontMetrics.h
>--- gfx/public/nsIFontMetrics.h	24 Apr 2002 03:56:07 -0000	1.21
>+++ gfx/public/nsIFontMetrics.h	2 Jun 2002 22:15:04 -0000
>@@ -234,7 +234,7 @@
>    */
>   NS_IMETHOD  GetFontHandle(nsFontHandle &aHandle) = 0;
> 
>-#if defined(_WIN32) || defined(XP_OS2)
>+#if defined(_WIN32) || defined(XP_OS2) || defined(XP_UNIX)

Change that XP_UNIX to MOZ_WIDGET_GTK.	This is still unimplemented for other
unix toolkits.	The same comment applies to other places where you used
XP_UNIX.

>Index: gfx/src/gtk/nsFontMetricsGTK.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/gfx/src/gtk/nsFontMetricsGTK.cpp,v
>retrieving revision 1.209
>diff -u -r1.209 nsFontMetricsGTK.cpp
>--- gfx/src/gtk/nsFontMetricsGTK.cpp	24 May 2002 00:00:28 -0000	1.209
>+++ gfx/src/gtk/nsFontMetricsGTK.cpp	2 Jun 2002 22:15:04 -0000
>@@ -1403,6 +1404,11 @@
>     PRUnichar space = (PRUnichar)' ';
>     mSpaceWidth = NSToCoordRound(ft->GetWidth(&space, 1) * f);
> 
>+/* TOMI
>+    gint sampleWidth = gdk_text_width(mFontHandle, "Tomi Leppikangas", 16);
>+    mAveCharWidth = PR_MAX(1, NSToCoordRound(float(sampleWidth)/16 * f));
>+*/
>+
>     unsigned long pr = 0;
>     if (ft->getXHeight(pr)) {
>       mXHeight = nscoord(pr * f);
>@@ -1471,17 +1477,21 @@
> 
>   mMaxAdvance = nscoord(fontInfo->max_bounds.width * f);
> 
>-  gint rawWidth;
>+  gint rawWidth, rawAwerage;
>   if ((fontInfo->min_byte1 == 0) && (fontInfo->max_byte1 == 0)) {
>     rawWidth = xFont->TextWidth8(" ", 1);
>+    rawAwerage = xFont->TextWidth8("Tomi Leppikangas", 16);
>   }
>   else {
>     XChar2b _16bit_space;
>     _16bit_space.byte1 = 0;
>     _16bit_space.byte2 = ' ';
>-    rawWidth = xFont->TextWidth16(&_16bit_space, sizeof(_16bit_space)/2);
>+     rawWidth = xFont->TextWidth16(&_16bit_space, sizeof(_16bit_space)/2);
>+     /* have to be 16bit? */
>+     rawAwerage = xFont->TextWidth8("Tomi Leppikangas", 16);
>   }
>   mSpaceWidth = NSToCoordRound(rawWidth * f);
>+  mAveCharWidth = PR_MAX(1, NSToCoordRound(float(rawAwerage)/16 * f));
> 
>   unsigned long pr = 0;
>   if (xFont->GetXFontProperty(XA_X_HEIGHT, &pr) &&

The documentation for the API's that are used for this on Win32 say that the
average character width is generally taken as the width of the letter 'x'.
We might get better results if we used that.  Also, did you mean "rawAverage",
not "rawAwerage"?

>@@ -1675,6 +1691,109 @@
>   return NS_OK;
> }
> 
>+/* TOMI */
>+nsFontGTK*
>+nsFontMetricsGTK::LocateFont(PRUint32 aChar, PRInt32 & aCount)

You don't need to decorate the function implementation with your name.	Really.
:)

>Index: gfx/src/gtk/nsFontMetricsGTK.h
>===================================================================
>RCS file: /cvsroot/mozilla/gfx/src/gtk/nsFontMetricsGTK.h,v
>retrieving revision 1.48
>diff -u -r1.48 nsFontMetricsGTK.h
>--- gfx/src/gtk/nsFontMetricsGTK.h	23 Mar 2002 21:30:10 -0000	1.48
>+++ gfx/src/gtk/nsFontMetricsGTK.h	2 Jun 2002 22:15:04 -0000
>@@ -264,6 +264,18 @@
>   PRBool                 mAlreadyCalledLoadFont;
> };
> 
>+struct nsFontSwitch {
>+  // Simple wrapper on top of nsFontWin for the moment
>+  // Could hold other attributes of the font
>+  nsFontGTK* mFontGTK;
>+};

Comment needs updated.	This is not nsFontWin.

>@@ -294,12 +306,18 @@
>   NS_IMETHOD  GetMaxAscent(nscoord &aAscent);
>   NS_IMETHOD  GetMaxDescent(nscoord &aDescent);
>   NS_IMETHOD  GetMaxAdvance(nscoord &aAdvance);
>+  NS_IMETHOD  GetAveCharWidth(nscoord &aAdvance);

Parameter name should agree with the interface, aAveCharWidth.

>   NS_IMETHOD  GetFont(const nsFont *&aFont);
>   NS_IMETHOD  GetLangGroup(nsIAtom** aLangGroup);
>   NS_IMETHOD  GetFontHandle(nsFontHandle &aHandle);
>   
>   NS_IMETHOD  GetSpaceWidth(nscoord &aSpaceWidth);
> 
>+  virtual nsresult
>+  ResolveForwards(const PRUnichar*     aString,
>+                  PRUint32             aLength,
>+                  nsFontSwitchCallback aFunc, 
>+                  void*                aData);

You can just use NS_IMETHOD for a virtual function returning nsresult.

>Index: gfx/src/gtk/nsRenderingContextGTK.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/gfx/src/gtk/nsRenderingContextGTK.cpp,v
>retrieving revision 1.159
>diff -u -r1.159 nsRenderingContextGTK.cpp
>--- gfx/src/gtk/nsRenderingContextGTK.cpp	24 May 2002 20:06:25 -0000	1.159
>+++ gfx/src/gtk/nsRenderingContextGTK.cpp	2 Jun 2002 22:15:08 -0000
>+
>+      // Measure the text
>+      nscoord twWidth = 0;
>+      if ((1 == numChars) && (aString[start] == ' ')) {
>+        mFontMetrics->GetSpaceWidth(twWidth);
>+      } 
>+      else if (numChars > 0) {
>+        GetWidth( &aString[start], numChars, twWidth);
>+/*
>+        int size;
>+        size =  gdk_text_width (mCurrentFont, &aString[start], numChars);
>+        twWidth = NSToCoordRound(float(size) * mP2T);
>+*/
>+/*
>+        SIZE  size;
>+        ::GetTextExtentPoint32(mDC, &aString[start], numChars, &size);
>+        twWidth = NSToCoordRound(float(size.cx) * mP2T);
>+*/
>+      }

What's going on with the commented-out code?
bstell, think you could review this?
There is still missing part of truetype font calculation and needs
some fix for xprint/postscript bustage.

I think xprint/postscript needs some dummy functions for ResolveForward() and
GetTextDimensions(), how do we prevent calling them from nsTextFrame::MeasureText
when gfx == (xprint || postscript) ?

I think i can fix things that bryer mentioned and add truetype stuff, but i
have no idea how to detect which gfx is in use when measuring text.
The use of the string "Tomi Leppikangas" seem like a sub-optimal choice for
calculating the average width of a character.

>   if ((fontInfo->min_byte1 == 0) && (fontInfo->max_byte1 == 0)) {
...
>+    rawAwerage = xFont->TextWidth8("Tomi Leppikangas", 16);
>+     rawWidth = xFont->TextWidth16(&_16bit_space, sizeof(_16bit_space)/2);
>+     /* have to be 16bit? */
>+     rawAwerage = xFont->TextWidth8("Tomi Leppikangas", 16);

This test is only there because it is handling the case of a 16 bit indexed
font vs an 8 bit indexed font. So, yes, it has to be 16 bit. Perhaps you should
use the name rawAveWidth instead of rawAverage. Or maybe XAveCharWidth.

>+struct nsFontSwitch {
>+  // Simple wrapper on top of nsFontWin for the moment
>+  // Could hold other attributes of the font
>+  nsFontGTK* mFontGTK;
>+};

What is this?

> There is still missing part of truetype font calculation and needs
> some fix for xprint/postscript bustage.

Could you be more specific about this?

> i have no idea how to detect which gfx is in use when measuring text.

You should not know. You should be calling a method of the nsFontGTK object 
Calling a specific text function like gdk_text_width on a font in 
nsRenderingContextGTK must not be done as this completely breaks the
virtual classes that are there to allow us to have multiple font types. 


> > i have no idea how to detect which gfx is in use when measuring text.
> 
> You should not know. You should be calling a method of the nsFontGTK object 

Please ignore this comment, I mistook this for gtk.

Here is better patch, still needs to fix xprint and postscript bustage.
I guess xprint and postscript needs dummy GetTextDimensions() and
some guardian in nsTextFrame::MeasureText() not to call those.
Attachment #85987 - Attachment is obsolete: true
Attachment #86234 - Flags: needs-work+
Overall this is impressive code.

-    XChar2b _16bit_space;
+    XChar2b _16bit_space, _16bit_x;
     _16bit_space.byte1 = 0;
     _16bit_space.byte2 = ' ';
+    _16bit_space.byte1 = 0;
+    _16bit_space.byte2 = 'x';
     rawWidth = xFont->TextWidth16(&_16bit_space, sizeof(_16bit_space)/2);
+    rawAverage = xFont->TextWidth16(&_16bit_x, sizeof( _16bit_x)/2);

Is this right? How was this been tested?

+    //    if (font->HasGlyph(aChar))

Nit: did you mean to have this comment? 

+    if(CCMAP_HAS_CHAR(font->mCCMap, aChar))

Very minor nit: a space between the 'if' and '('

Am I correct in assumming that the breaks passed to ResolveForwards handle word 
breaks, line breaks, and would be suitable for ligatures, diacritics and such?

To me it feels like ResolveForwards belongs in nsRenderingContextGTK.cpp as much 
of the code that deals with the fact that the "font" is actually a list of fonts
is in nsRenderingContextGTK.cpp. My understanding is that nsFontMetricsGTK.cpp 
has the X font specific code.

+#ifdef DEBUG_rbs

rbs?

Nit: I'm curious that sometimes I see

  +  if (aFontID) *aFontID = 0;

and sometimes I see

  +  if (!count)
  +    return NS_OK;


Tomi Leppikangas wrote:
> Here is better patch, still needs to fix xprint and postscript bustage.
> I guess xprint and postscript needs dummy GetTextDimensions() and
> some guardian in nsTextFrame::MeasureText() not to call those.

I am not sure about PostScript - but Xprint works like the Xlib gfx code
(gfx/src/xlib/) which is nearly the same code as in gfx/src/gtk/
Just adding dummy code may break both Xprint and Xlib gfx...
Tomi Leppikangas:
Can you file a new patch with the issues listed in bstell's comment #31 fixed,
please ? Once the patch gets reviewed I'll port it over to Xlib gfx (Xprint uses
Xlib gfx, therefore we do not need a seperate port) and file a combined patch
for both modules...
This patch fixes bstell's comments. 99% of code is copied from windows,
so linebreaks and such should work just like they do in windows.

I was thinking about implementing guardian for that GetTextDimensions()
with new rendering hint:

aRenderingContext->GetHint(&hint);
if(hint == NS_RENDERING_HINT_FAST_MEASURE_IMPLEMENTED)
  do this
else 
  do that


How does that sound? Or is there way to do somekind of runtime typing, like
in java:
if (aRenderingContext inctanceof nsRenderingContextGTK) { }
Attachment #86234 - Attachment is obsolete: true
Here is final patch. This adds workaround for xpint/postscript bustage.

Workaround adds new hint to renderingcontex that is sen only if
GetTextDimensions is implemented.
Attachment #87138 - Attachment is obsolete: true
Tomi Leppikangas wrote:
> patch #4  added workaround for xprint/postscript bustage
>
> Here is final patch. This adds workaround for xpint/postscript bustage.

bstell:
Is this patch OK for you ? If yes then I'll port the patch to Xlib gfx...
Please file separate bug for xlib implementation. This patch has been lying
here for allmost 2 years, i don't want to rewrite it third time when
windows code changes.

Also i don't have cvs access, so someone else should check this in.
Tomi Leppikangas wrote:
> Please file separate bug for xlib implementation.

I am not sure whether this is wise. Having two bugs requires review, superreview
and approval twice for the same code. I tried that in the past and it was always
the root of adminstrative problems... ;-(

> This patch has been lying
> here for allmost 2 years, i don't want to rewrite it third time when
> windows code changes.

Uhm, I can file the Xlib gfx port within one day; I am currently building for
that purpose...
... I only wait for either bstell or shanjian to say "OK" to the current
patch...
In general, I like this patch and would like to see it happen. I would like to
spend more time next week to better examine the patch. Send me an email if I
forgot to do so.  I would also suggest to hear something from brian. He has
better understanding about X font than I do. 

bstell, mind take another look? 
Let Roland have this one
Assignee: ftang → Roland.Mainz
Status: ASSIGNED → NEW
Severity: normal → enhancement
Status: NEW → ASSIGNED
Target Milestone: Future → mozilla1.1beta
(isn't no time always a excuse? :-)
I checked the patch again just now, and I found many of the code are identical
to windows. So I am confident enough to give my OK now. 

Comment on attachment 89465 [details] [diff] [review]
patch #4  added workaround for xprint/postscript bustage

r=shanjian
Attachment #89465 - Flags: review+
Comment on attachment 89465 [details] [diff] [review]
patch #4  added workaround for xprint/postscript bustage

shanjian:
Thanks!
Xlib port coming in a few hours...
Attachment #89465 - Flags: needs-work+
Comment on attachment 89465 [details] [diff] [review]
patch #4  added workaround for xprint/postscript bustage

This patch does not work on Solaris/SPARC (tested with 2002-06-27-08-trunk);
the build crashes in TransformTextToUnicode() due a misaligned pointer... ;-(
-- snip --
WARNING: CSSLoaderImpl::LoadSheet: Load of URL
'chrome://communicator/skin/bookmarks/platformBookmarks.css' failed.  Error
code: 16389, file
../../../../../../../../../home/mozilla/src/2002-06-27-08-trunk/mozilla/content
/html/style/src/nsCSSLoader.cpp, line 1304
Reading libimgpng.so
t@1 (l@1) signal BUS (invalid address alignment) in TransformTextToUnicode at
line 4529 in file "nsTextFrame.cpp"
 4529	    *cp2-- = PRUnichar(*cp1--);
(dbx) print cp2 cp1
dbx: syntax error on "cp1"
Use the `help' command for more information.
(dbx) print cp2, cp1
cp2 = 0xffbe46b7
cp1 = 0xffbe46b7 "on"
(dbx) print aText, aNumChars
aText = 0xffbe46b7 "on"
aNumChars = 1
(dbx) where
current thread: t@1
=>[1] TransformTextToUnicode(aText = 0xffbe46b7 "on", aNumChars = 1), line 4529
in "nsTextFrame.cpp"
  [2] nsTextFrame::MeasureText(this = 0x7ef8f0, aPresContext = 0x6ccdd8,
aReflowState = STRUCT, aTx = CLASS, aLb = 0x738458, aTs = STRUCT, aTextData =
STRUCT), line 5036 in "nsTextFrame.cpp"
  [3] nsTextFrame::Reflow(this = 0x7ef8f0, aPresContext = 0x6ccdd8, aMetrics =
STRUCT, aReflowState = STRUCT, aStatus = 0), line 5341 in "nsTextFrame.cpp"
  [4] nsLineLayout::ReflowFrame(this = 0xffbe5448, aFrame = 0x7ef8f0,
aReflowStatus = 0, aMetrics = (nil), aPushedFrame = 0), line 1081 in
"nsLineLayout.cpp"
  [5] nsInlineFrame::ReflowInlineFrame(this = 0x7ef884, aPresContext =
0x6ccdd8, aReflowState = STRUCT, irs = STRUCT, aFrame = 0x7ef8f0, aStatus = 0),
line 713 in "nsInlineFrame.cpp"
  [6] nsInlineFrame::ReflowFrames(this = 0x7ef884, aPresContext = 0x6ccdd8,
aReflowState = STRUCT, irs = STRUCT, aMetrics = STRUCT, aStatus = 0), line 523
in "nsInlineFrame.cpp"
  [7] nsInlineFrame::Reflow(this = 0x7ef884, aPresContext = 0x6ccdd8, aMetrics
= STRUCT, aReflowState = STRUCT, aStatus = 0), line 433 in "nsInlineFrame.cpp"
  [8] nsLineLayout::ReflowFrame(this = 0xffbe5448, aFrame = 0x7ef884,
aReflowStatus = 0, aMetrics = (nil), aPushedFrame = 0), line 1081 in
"nsLineLayout.cpp"
  [9] nsBlockFrame::ReflowInlineFrame(this = 0x7ef738, aState = CLASS,
aLineLayout = CLASS, aLine = CLASS, aFrame = 0x7ef884, aLineReflowStatus =
0xffbe539f ""), line 3745 in "nsBlockFrame.cpp"
  [10] nsBlockFrame::DoReflowInlineFrames(this = 0x7ef738, aState = CLASS,
aLineLayout = CLASS, aLine = CLASS, aKeepReflowGoing = 0xffbe5d94,
aLineReflowStatus = 0xffbe5943 "^B", aUpdateMaximumWidth = 0, aDamageDirtyArea
= 0), line 3621 in "nsBlockFrame.cpp"
  [11] nsBlockFrame::DoReflowInlineFramesAuto(this = 0x7ef738, aState = CLASS,
aLine = CLASS, aKeepReflowGoing = 0xffbe5d94, aLineReflowStatus = 0xffbe5943
"^B", aUpdateMaximumWidth = 0, aDamageDirtyArea = 0), line 3510 in
"nsBlockFrame.cpp"
  [12] nsBlockFrame::ReflowInlineFrames(this = 0x7ef738, aState = CLASS, aLine
= CLASS, aKeepReflowGoing = 0xffbe5d94, aDamageDirtyArea = 0,
aUpdateMaximumWidth = 0), line 3454 in "nsBlockFrame.cpp"
  [13] nsBlockFrame::ReflowLine(this = 0x7ef738, aState = CLASS, aLine = CLASS,
aKeepReflowGoing = 0xffbe5d94, aDamageDirtyArea = 0), line 2609 in
"nsBlockFrame.cpp"
  [14] nsBlockFrame::ReflowDirtyLines(this = 0x7ef738, aState = CLASS), line
2253 in "nsBlockFrame.cpp"
  [15] nsBlockFrame::Reflow(this = 0x7ef738, aPresContext = 0x6ccdd8, aMetrics
= STRUCT, aReflowState = STRUCT, aStatus = 0), line 949 in "nsBlockFrame.cpp"
  [16] nsBlockReflowContext::DoReflowBlock(this = 0xffbe6ac0, aReflowState =
STRUCT, aReason = eReflowReason_Resize, aFrame = 0x7ef738, aSpace = STRUCT,
aApplyTopMargin = 0, aPrevBottomMargin = STRUCT, aIsAdjacentWithTop = 1,
aComputedOffsets = STRUCT, aFrameReflowStatus = 0), line 568 in
"nsBlockReflowContext.cpp"
  [17] nsBlockReflowContext::ReflowBlock(this = 0xffbe6ac0, aFrame = 0x7ef738,
aSpace = STRUCT, aApplyTopMargin = 0, aPrevBottomMargin = STRUCT,
aIsAdjacentWithTop = 1, aComputedOffsets = STRUCT, aFrameReflowStatus = 0),
line 341 in "nsBlockReflowContext.cpp"
  [18] nsBlockFrame::ReflowBlockFrame(this = 0x7ef618, aState = CLASS, aLine =
CLASS, aKeepReflowGoing = 0xffbe6fb4), line 3215 in "nsBlockFrame.cpp"
  [19] nsBlockFrame::ReflowLine(this = 0x7ef618, aState = CLASS, aLine = CLASS,
aKeepReflowGoing = 0xffbe6fb4, aDamageDirtyArea = 0), line 2471 in
"nsBlockFrame.cpp"
  [20] nsBlockFrame::ReflowDirtyLines(this = 0x7ef618, aState = CLASS), line
2253 in "nsBlockFrame.cpp"
  [21] nsBlockFrame::Reflow(this = 0x7ef618, aPresContext = 0x6ccdd8, aMetrics
= STRUCT, aReflowState = STRUCT, aStatus = 0), line 949 in "nsBlockFrame.cpp"
  [22] nsBlockReflowContext::DoReflowBlock(this = 0xffbe7ce0, aReflowState =
STRUCT, aReason = eReflowReason_Resize, aFrame = 0x7ef618, aSpace = STRUCT,
aApplyTopMargin = 1, aPrevBottomMargin = STRUCT, aIsAdjacentWithTop = 0,
aComputedOffsets = STRUCT, aFrameReflowStatus = 0), line 568 in
"nsBlockReflowContext.cpp"
  [23] nsBlockReflowContext::ReflowBlock(this = 0xffbe7ce0, aFrame = 0x7ef618,
aSpace = STRUCT, aApplyTopMargin = 1, aPrevBottomMargin = STRUCT,
aIsAdjacentWithTop = 0, aComputedOffsets = STRUCT, aFrameReflowStatus = 0),
line 341 in "nsBlockReflowContext.cpp"
  [24] nsBlockFrame::ReflowBlockFrame(this = 0x7ef27c, aState = CLASS, aLine =
CLASS, aKeepReflowGoing = 0xffbe81d4), line 3215 in "nsBlockFrame.cpp"
  [25] nsBlockFrame::ReflowLine(this = 0x7ef27c, aState = CLASS, aLine = CLASS,
aKeepReflowGoing = 0xffbe81d4, aDamageDirtyArea = 0), line 2471 in
"nsBlockFrame.cpp"
  [26] nsBlockFrame::ReflowDirtyLines(this = 0x7ef27c, aState = CLASS), line
2253 in "nsBlockFrame.cpp"
  [27] nsBlockFrame::Reflow(this = 0x7ef27c, aPresContext = 0x6ccdd8, aMetrics
= STRUCT, aReflowState = STRUCT, aStatus = 0), line 949 in "nsBlockFrame.cpp"
  [28] nsBlockReflowContext::DoReflowBlock(this = 0xffbe8f00, aReflowState =
STRUCT, aReason = eReflowReason_Resize, aFrame = 0x7ef27c, aSpace = STRUCT,
aApplyTopMargin = 1, aPrevBottomMargin = STRUCT, aIsAdjacentWithTop = 0,
aComputedOffsets = STRUCT, aFrameReflowStatus = 0), line 568 in
"nsBlockReflowContext.cpp"
  [29] nsBlockReflowContext::ReflowBlock(this = 0xffbe8f00, aFrame = 0x7ef27c,
aSpace = STRUCT, aApplyTopMargin = 1, aPrevBottomMargin = STRUCT,
aIsAdjacentWithTop = 0, aComputedOffsets = STRUCT, aFrameReflowStatus = 0),
line 341 in "nsBlockReflowContext.cpp"
  [30] nsBlockFrame::ReflowBlockFrame(this = 0x7ee740, aState = CLASS, aLine =
CLASS, aKeepReflowGoing = 0xffbe93f4), line 3215 in "nsBlockFrame.cpp"
  [31] nsBlockFrame::ReflowLine(this = 0x7ee740, aState = CLASS, aLine = CLASS,
aKeepReflowGoing = 0xffbe93f4, aDamageDirtyArea = 0), line 2471 in
"nsBlockFrame.cpp"
  [32] nsBlockFrame::ReflowDirtyLines(this = 0x7ee740, aState = CLASS), line
2253 in "nsBlockFrame.cpp"
  [33] nsBlockFrame::Reflow(this = 0x7ee740, aPresContext = 0x6ccdd8, aMetrics
= STRUCT, aReflowState = STRUCT, aStatus = 0), line 949 in "nsBlockFrame.cpp"
  [34] nsContainerFrame::ReflowChild(this = 0x7ee6e0, aKidFrame = 0x7ee740,
aPresContext = 0x6ccdd8, aDesiredSize = STRUCT, aReflowState = STRUCT, aX = 51,
aY = 51, aFlags = 0, aStatus = 0), line 801 in "nsContainerFrame.cpp"
  [35] nsTableCellFrame::Reflow(this = 0x7ee6e0, aPresContext = 0x6ccdd8,
aDesiredSize = STRUCT, aReflowState = STRUCT, aStatus = 0), line 944 in
"nsTableCellFrame.cpp"
  [36] nsContainerFrame::ReflowChild(this = 0x760e04, aKidFrame = 0x7ee6e0,
aPresContext = 0x6ccdd8, aDesiredSize = STRUCT, aReflowState = STRUCT, aX =
2295, aY = 0, aFlags = 0, aStatus = 0), line 801 in "nsContainerFrame.cpp"
  [37] nsTableRowFrame::ReflowChildren(this = 0x760e04, aPresContext =
0x6ccdd8, aDesiredSize = STRUCT, aReflowState = STRUCT, aTableFrame = CLASS,
aStatus = 0, aDirtyOnly = 0), line 1040 in "nsTableRowFrame.cpp"
  [38] nsTableRowFrame::Reflow(this = 0x760e04, aPresContext = 0x6ccdd8,
aDesiredSize = STRUCT, aReflowState = STRUCT, aStatus = 0), line 1448 in
"nsTableRowFrame.cpp"
  [39] nsContainerFrame::ReflowChild(this = 0x760d58, aKidFrame = 0x760e04,
aPresContext = 0x6ccdd8, aDesiredSize = STRUCT, aReflowState = STRUCT, aX = 0,
aY = 0, aFlags = 0, aStatus = 0), line 801 in "nsContainerFrame.cpp"
  [40] nsTableRowGroupFrame::ReflowChildren(this = 0x760d58, aPresContext =
0x6ccdd8, aDesiredSize = STRUCT, aReflowState = STRUCT, aStatus = 0,
aStartFrame = (nil), aDirtyOnly = 0, aFirstRowReflowed = (nil),
aPageBreakBeforeEnd = 0xffbea7d8), line 443 in "nsTableRowGroupFrame.cpp"
  [41] nsTableRowGroupFrame::Reflow(this = 0x760d58, aPresContext = 0x6ccdd8,
aDesiredSize = STRUCT, aReflowState = STRUCT, aStatus = 0), line 1213 in
"nsTableRowGroupFrame.cpp"
  [42] nsContainerFrame::ReflowChild(this = 0x760c78, aKidFrame = 0x760d58,
aPresContext = 0x6ccdd8, aDesiredSize = STRUCT, aReflowState = STRUCT, aX = 0,
aY = 0, aFlags = 0, aStatus = 0), line 801 in "nsContainerFrame.cpp"
  [43] nsTableFrame::ReflowChildren(this = 0x760c78, aPresContext = 0x6ccdd8,
aReflowState = STRUCT, aDoColGroups = 1, aDirtyOnly = 0, aStatus = 0,
aLastChildReflowed = (nil), aReflowedAtLeastOne = (nil)), line 3310 in
"nsTableFrame.cpp"
  [44] nsTableFrame::ReflowTable(this = 0x760c78, aPresContext = 0x6ccdd8,
aDesiredSize = STRUCT, aReflowState = STRUCT, aAvailHeight = 1073741824,
aReason = eReflowReason_Resize, aLastChildReflowed = (nil), aDoCollapse = 0,
aDidBalance = 1, aStatus = 0), line 2203 in "nsTableFrame.cpp"
  [45] nsTableFrame::Reflow(this = 0x760c78, aPresContext = 0x6ccdd8,
aDesiredSize = STRUCT, aReflowState = STRUCT, aStatus = 0), line 2070 in
"nsTableFrame.cpp"
  [46] nsContainerFrame::ReflowChild(this = 0x760aa8, aKidFrame = 0x760c78,
aPresContext = 0x6ccdd8, aDesiredSize = STRUCT, aReflowState = STRUCT, aX = 0,
aY = 0, aFlags = 3U, aStatus = 0), line 801 in "nsContainerFrame.cpp"
  [47] nsTableOuterFrame::OuterReflowChild(this = 0x760aa8, aPresContext =
0x6ccdd8, aChildFrame = 0x760c78, aOuterRS = STRUCT, aMetrics = STRUCT,
aAvailWidth = (nil), aDesiredSize = STRUCT, aMargin = STRUCT, aMarginNoAuto =
STRUCT, aPadding = STRUCT, aReflowReason = eReflowReason_Initial, aStatus = 0),
line 1025 in "nsTableOuterFrame.cpp"
  [48] nsTableOuterFrame::Reflow(this = 0x760aa8, aPresContext = 0x6ccdd8,
aDesiredSize = STRUCT, aOuterRS = STRUCT, aStatus = 0), line 1609 in
"nsTableOuterFrame.cpp"
  [49] nsBlockReflowContext::DoReflowBlock(this = 0xffbebb48, aReflowState =
STRUCT, aReason = eReflowReason_Initial, aFrame = 0x760aa8, aSpace = STRUCT,
aApplyTopMargin = 1, aPrevBottomMargin = STRUCT, aIsAdjacentWithTop = 0,
aComputedOffsets = STRUCT, aFrameReflowStatus = 0), line 568 in
"nsBlockReflowContext.cpp"
  [50] nsBlockReflowContext::ReflowBlock(this = 0xffbebb48, aFrame = 0x760aa8,
aSpace = STRUCT, aApplyTopMargin = 1, aPrevBottomMargin = STRUCT,
aIsAdjacentWithTop = 0, aComputedOffsets = STRUCT, aFrameReflowStatus = 0),
line 341 in "nsBlockReflowContext.cpp"
  [51] nsBlockFrame::ReflowBlockFrame(this = 0x733618, aState = CLASS, aLine =
CLASS, aKeepReflowGoing = 0xffbec03c), line 3215 in "nsBlockFrame.cpp"
  [52] nsBlockFrame::ReflowLine(this = 0x733618, aState = CLASS, aLine = CLASS,
aKeepReflowGoing = 0xffbec03c, aDamageDirtyArea = 1), line 2471 in
"nsBlockFrame.cpp"
  [53] nsBlockFrame::ReflowDirtyLines(this = 0x733618, aState = CLASS), line
2253 in "nsBlockFrame.cpp"
  [54] nsBlockFrame::Reflow(this = 0x733618, aPresContext = 0x6ccdd8, aMetrics
= STRUCT, aReflowState = STRUCT, aStatus = 0), line 949 in "nsBlockFrame.cpp"
  [55] nsBlockReflowContext::DoReflowBlock(this = 0xffbecd68, aReflowState =
STRUCT, aReason = eReflowReason_Incremental, aFrame = 0x733618, aSpace =
STRUCT, aApplyTopMargin = 1, aPrevBottomMargin = STRUCT, aIsAdjacentWithTop =
1, aComputedOffsets = STRUCT, aFrameReflowStatus = 0), line 568 in
"nsBlockReflowContext.cpp"
  [56] nsBlockReflowContext::ReflowBlock(this = 0xffbecd68, aFrame = 0x733618,
aSpace = STRUCT, aApplyTopMargin = 1, aPrevBottomMargin = STRUCT,
aIsAdjacentWithTop = 1, aComputedOffsets = STRUCT, aFrameReflowStatus = 0),
line 341 in "nsBlockReflowContext.cpp"
  [57] nsBlockFrame::ReflowBlockFrame(this = 0x7334a0, aState = CLASS, aLine =
CLASS, aKeepReflowGoing = 0xffbed25c), line 3215 in "nsBlockFrame.cpp"
  [58] nsBlockFrame::ReflowLine(this = 0x7334a0, aState = CLASS, aLine = CLASS,
aKeepReflowGoing = 0xffbed25c, aDamageDirtyArea = 1), line 2471 in
"nsBlockFrame.cpp"
  [59] nsBlockFrame::ReflowDirtyLines(this = 0x7334a0, aState = CLASS), line
2253 in "nsBlockFrame.cpp"
  [60] nsBlockFrame::Reflow(this = 0x7334a0, aPresContext = 0x6ccdd8, aMetrics
= STRUCT, aReflowState = STRUCT, aStatus = 0), line 949 in "nsBlockFrame.cpp"
  [61] nsContainerFrame::ReflowChild(this = 0x790bf0, aKidFrame = 0x7334a0,
aPresContext = 0x6ccdd8, aDesiredSize = STRUCT, aReflowState = STRUCT, aX = 0,
aY = 0, aFlags = 0, aStatus = 0), line 801 in "nsContainerFrame.cpp"
  [62] CanvasFrame::Reflow(this = 0x790bf0, aPresContext = 0x6ccdd8,
aDesiredSize = STRUCT, aReflowState = STRUCT, aStatus = 0), line 556 in
"nsHTMLFrame.cpp"
  [63] nsBoxToBlockAdaptor::Reflow(this = 0x733404, aState = CLASS,
aPresContext = 0x6ccdd8, aDesiredSize = STRUCT, aReflowState = STRUCT, aStatus
= 0, aX = 0, aY = 0, aWidth = 13617, aHeight = 7123, aMoveFrame = 1), line 880
in "nsBoxToBlockAdaptor.cpp"
  [64] nsBoxToBlockAdaptor::DoLayout(this = 0x733404, aState = CLASS), line 617
in "nsBoxToBlockAdaptor.cpp"
  [65] nsBox::Layout(this = 0x733404, aState = CLASS), line 1060 in "nsBox.cpp"
  [66] nsScrollBoxFrame::DoLayout(this = 0x791008, aState = CLASS), line 392 in
"nsScrollBoxFrame.cpp"
  [67] nsBox::Layout(this = 0x791040, aState = CLASS), line 1060 in "nsBox.cpp"
  [68] nsContainerBox::LayoutChildAt(aState = CLASS, aBox = 0x791040, aRect =
STRUCT), line 645 in "nsContainerBox.cpp"
  [69] nsGfxScrollFrameInner::LayoutBox(this = 0x71ecd8, aState = CLASS, aBox =
0x791040, aRect = STRUCT), line 1081 in "nsGfxScrollFrame.cpp"
  [70] nsGfxScrollFrameInner::Layout(this = 0x71ecd8, aState = CLASS), line
1233 in "nsGfxScrollFrame.cpp"
  [71] nsGfxScrollFrame::DoLayout(this = 0x790e08, aState = CLASS), line 1089
in "nsGfxScrollFrame.cpp"
  [72] nsBox::Layout(this = 0x790e40, aState = CLASS), line 1060 in "nsBox.cpp"
  [73] nsBoxFrame::Reflow(this = 0x790e08, aPresContext = 0x6ccdd8,
aDesiredSize = STRUCT, aReflowState = STRUCT, aStatus = 0), line 999 in
"nsBoxFrame.cpp"
  [74] nsGfxScrollFrame::Reflow(this = 0x790e08, aPresContext = 0x6ccdd8,
aDesiredSize = STRUCT, aReflowState = STRUCT, aStatus = 0), line 776 in
"nsGfxScrollFrame.cpp"
  [75] nsContainerFrame::ReflowChild(this = 0x790bb4, aKidFrame = 0x790e08,
aPresContext = 0x6ccdd8, aDesiredSize = STRUCT, aReflowState = STRUCT, aX = 0,
aY = 0, aFlags = 0, aStatus = 0), line 801 in "nsContainerFrame.cpp"
  [76] ViewportFrame::Reflow(this = 0x790bb4, aPresContext = 0x6ccdd8,
aDesiredSize = STRUCT, aReflowState = STRUCT, aStatus = 0), line 575 in
"nsViewportFrame.cpp"
  [77] IncrementalReflow::Dispatch(this = 0xffbeebf0, aPresContext = 0x6ccdd8,
aDesiredSize = STRUCT, aMaxSize = STRUCT, aRendContext = CLASS), line 936 in
"nsPresShell.cpp"
  [78] PresShell::ProcessReflowCommands(this = 0x75ae98, aInterruptible = 1),
line 6445 in "nsPresShell.cpp"
  [79] ReflowEvent::HandleEvent(this = 0x802628), line 6300 in
"nsPresShell.cpp"
  [80] HandlePLEvent(aEvent = 0x802628), line 6316 in "nsPresShell.cpp"
  [81] PL_HandleEvent(self = ???) (optimized), at 0xff074688 (line ~596) in
"plevent.c"
  [82] PL_ProcessPendingEvents(self = ???) (optimized), at 0xff0744c0 (line
~526) in "plevent.c"
dbx: internal warning: Typeid already exists with different
name:__1nKnsACString_ or symclass:struct, ignore current
stab:./libxpcom.so:../../../../../../../home/mozilla/src/2002-06-27-08-trunk/mo
zilla/xpcom/threads/nsEventQueue.cpp stab #349 __1nKnsACString_:U(0,314)
  [83] nsEventQueueImpl::ProcessPendingEvents(this = ???) (optimized), at
0xff076794 (line ~388) in "nsEventQueue.cpp"
  [84] event_processor_callback(data = ???, source = ???, condition = ???)
(optimized), at 0xfc8a4adc (line ~184) in "nsAppShell.cpp"
  [85] our_gdk_io_invoke(source = ???, condition = ???, data = ???)
(optimized), at 0xfc8a469c (line ~76) in "nsAppShell.cpp"
dbx: warning: can't find file
"/home/gisburn/package-builds/glib/glib-1.2.8/objdir/giounix.lo"
dbx: warning: see `help finding-files'
  [86] g_io_unix_dispatch(0x321d18, 0xffbef128, 0x2896d0, 0xff3e4270,
0xfe56ca24, 0xffbef090), at 0xfe9b2dc8
dbx: warning: can't find file
"/home/gisburn/package-builds/glib/glib-1.2.8/objdir/gmain.lo"
  [87] g_main_dispatch(0xffbef128, 0x1140f8, 0x1, 0x0, 0xfeb5155b, 0x378), at
0xfe9b6dc8
  [88] g_main_iterate(0x1, 0x1, 0x5, 0xff3e4270, 0xfc8978fb, 0x18), at
0xfe9b7bcc
  [89] g_main_run(0x31c708, 0x31c708, 0x1, 0xfc911e98, 0xfc911e9c, 0x155754),
at 0xfe9b7f64
dbx: warning: can't find file
"/home/gisburn/package-builds/gtk+/gtk+-1.2.8/objdir/gtk/gtkmain.lo"
  [90] gtk_main(0xc7838, 0xc7730, 0xffbef204, 0xffbef208, 0x0, 0xfc8a504c), at
0xfecd60a0
  [91] nsAppShell::Run(this = ???) (optimized), at 0xfc8a4fa0 (line ~332) in
"nsAppShell.cpp"
  [92] nsAppShellService::Run(this = ???) (optimized), at 0xfd62f0e8 (line
~457) in "nsAppShellService.cpp"
  [93] main1(argc = ???, argv = ???, nativeApp = ???) (optimized), at 0x19d98
(line ~1456) in "nsAppRunner.cpp"
  [94] main(argc = ???, argv = ???) (optimized), at 0x1a780 (line ~1805) in
"nsAppRunner.cpp"
-- snip --

Disabling |NS_RENDERING_HINT_FAST_MEASURE| in |nsRenderingContext*::GetHints()|
fixes the issue...
patch #5, now covers GTK+/Xlib/Xprint and allows other Unix platforms like BeOS
to set |NS_RENDERING_HINT_FAST_MEASURE| if they implement it later without the
need to play with the #ifdef's in nsTextFrame.cpp&co.
Attachment #89465 - Attachment is obsolete: true
Comment on attachment 89662 [details] [diff] [review]
patch #5, now covers GTK+/Xlib/Xprint

Patch still suffers from the memory alignment issue present in the original
patch... I have to do some test builds to figure out a fix...
Attachment #89662 - Flags: needs-work+
Comment on attachment 89662 [details] [diff] [review]
patch #5, now covers GTK+/Xlib/Xprint

I did two x86 builds (Xlib + GTK+); the patch seems to work there (tested with
MathML+some i18n pages with both normal and Xprint Xserver).

I assume the patch only causes problems on hardware platforms which force
natural alignment of the data types.
More fun:
Trying to bypass the alignment issue in nsTextFrame.cpp does not help much since
it passes the misaligned pointers to other modules, too.
Working around the issue in comment #44 then ends few steps later in
nsRenderingContextGTK:
-- snip --
Finished reading in bookmarks.html  (175209 microseconds)
Reading libmork.so
Reading xomEuro.so.2
WEBSHELL+ = 4
Reading libimgpng.so
Opening file cookperm.txt failed
t@1 (l@1) signal BUS (invalid address alignment) in
nsRenderingContextGTK::GetTextDimensions at line 1957 in file
"nsRenderingContextGTK.cpp"
 1957         PRUnichar c = aString[i];
(dbx) print i, aString, aString[i], &aString[i] 
i = 0
aString = 0xffbe67bf
aString[i] = 111U
&aString[i] = 0xffbe67bf
(dbx) print (char *)&aString[i]
(char *) &aString[i] = 0xffbe67bf ""
(dbx) where
current thread: t@1
=>[1] nsRenderingContextGTK::GetTextDimensions(this = 0x7314e8, aString =
0xffbe67bf, aLength = 2U, aDimensions = STRUCT, aFontID = (nil)), line 1957 in
"nsRenderingContextGTK.cpp"
  [2] nsTextFrame::MeasureText(this = ???, aPresContext = ???, aReflowState =
STRUCT, aTx = CLASS, aLb = ???, aTs = STRUCT, aTextData = STRUCT) (optimized),
at 0xfb6200f8 (line ~5055) in "nsTextFrame.cpp"
  [3] nsTextFrame::Reflow(this = ???, aPresContext = ???, aMetrics = STRUCT,
aReflowState = STRUCT, aStatus = ???) (optimized), at 0xfb621630 (line ~5341) in
"nsTextFrame.cpp"
  [4] nsLineLayout::ReflowFrame(this = ???, aFrame = ???, aReflowStatus = ???,
aMetrics = ???, aPushedFrame = ???) (optimized), at 0xfb5b40a8 (line ~1081) in
"nsLineLayout.cpp"
  [5] nsInlineFrame::ReflowInlineFrame(this = ???, aPresContext = ???,
aReflowState = STRUCT, irs = STRUCT, aFrame = ???, aStatus = ???) (optimized),
at 0xfb5ab9c0 (line ~713) in "nsInlineFrame.cpp"
  [6] nsInlineFrame::ReflowFrames(this = ???, aPresContext = ???, aReflowState =
STRUCT, irs = STRUCT, aMetrics = STRUCT, aStatus = ???) (optimized), at
0xfb5ab4c4 (line ~523) in "nsInlineFrame.cpp"
  [7] nsInlineFrame::Reflow(this = ???, aPresContext = ???, aMetrics = STRUCT,
aReflowState = STRUCT, aStatus = ???) (optimized), at 0xfb5ab23c (line ~433) in
"nsInlineFrame.cpp"
  [8] nsLineLayout::ReflowFrame(this = ???, aFrame = ???, aReflowStatus = ???,
aMetrics = ???, aPushedFrame = ???) (optimized), at 0xfb5b40a8 (line ~1081) in
"nsLineLayout.cpp"
  [9] nsBlockFrame::ReflowInlineFrame(this = ???, aState = CLASS, aLineLayout =
CLASS, aLine = CLASS, aFrame = ???, aLineReflowStatus = ???) (optimized), at
0xfb54ae74 (line ~3745) in "nsBlockFrame.cpp"
  [10] nsBlockFrame::DoReflowInlineFrames(this = ???, aState = CLASS,
aLineLayout = CLASS, aLine = CLASS, aKeepReflowGoing = ???, aLineReflowStatus =
???, aUpdateMaximumWidth = ???, aDamageDirtyArea = ???) (optimized), at
0xfb54aa78 (line ~3621) in "nsBlockFrame.cpp"
etc. etc.
-- snip --
It looks like you should fix TransformTextToUnicode to handle alignment.
Sorry it has taken me a while to get back to this. The attachment does not 
appear to be dramatically different and I only saw a few items in the previous 
patch and the serious item appear to have been addressed. I'm glad to see a
bit of effort for the Postscript code (which hopefully should go thru a major
upgrade in the next few months). Shanjian seems to be reviewing the work more
promptly than I am able to (Thanks Shanjian!).

bstell wrote:
> I'm glad to see a bit of effort for the Postscript code (which hopefully 
> should go thru a major upgrade in the next few months).

Why should anyone work on the PostScript module ? It's dead code and will be
fully replaced by the Xprint module (that's why we have already the
--disable-postscript configure switch).
Roland: this is off topic. If you feel the need to discuss printing technologies 
kindly open a bug for that topic.
I'll offload the "platform-requires-natural-alignment"-issue to another bug.
The fast measuring codepath is now only enabled on x86-platforms (AFAIK the
majority of the Linux users :) - I'll file a new bug to get |nsTextFrame|
adjusted in a later step for the other platforms.
Attachment #89662 - Attachment is obsolete: true
Comment on attachment 93419 [details] [diff] [review]
New patch for 2002-07-27-08-trunk

*Ave* => *Avg* unless you can't in which case i'd like to complain to whomever
r/sr'd that.

+  //This if block is mean to speedup the process in normal situation, when

mean=>meant. or use 'designed' instead

+	   if ((1 == numChars) && (aString[start] == ' ')) {
you don't have to worry about alternative space characters?

and this code doesn't have to worry about bidi?

+  // Our current state relatively to the _full_ string...
'relative'
+  // This allows emulating the previous code...
'emulation of' or 'us to emulate'

+      // Find the nearest place to break that is less than or equal to
+      // the estimated break offset
i don't need a mathematical wording of what's written in plain c++, how about
using 'preceding or up to the estimated break offset' or something like that
instead.

+      // We found a place to break that is before the estimated break
+      // offset. Where we break depends on whether the text crosses a
+      // segment boundary
this comment is much better :)

+      // If we computed the break index and we're not in the middle
+      // of a segment then this is a spot that we can back up to if
+      // we need to so remember this state
s/to so/to, so/

These long comments look like sentences but lack periods (.) please add them as
appropriate

+	 // There's no place to back up to so even though the text doesn't fit
s/to so/to, so/
(i wonder why this is a common nit)

+	 // until we at least measure the first word entirely
'entire first word'

+	   // from now on, the estimated number of characters is what we want
to measure
'we want to measure the estimated number of characters'

+  NS_ASSERTION(allDone || start == i, "internal error");
+  NS_ASSERTION(allDone || data->mNumCharsFit != numCharsFit, "internal
error");
so if allDone, assert "internal error" twice?

+  // return earlier, it would mean that the unique word needs several fonts
+  // and we will still have to loop over the fonts to return the final height
case/tense mix error
"if we didn't" (hypothetical past?) "needs" (ongoing present) "still have to"
(subjunctive present ongoing or something -- excuse me for thinking in spanish)

i think my comments apply to at least the two toolkits patched here if not the
ones form which the patch originated. (this thought indicates i'm reviewing the
second toolkit)

+	   // Remember we're in the middle of a segment and not in between
+	   // two segments
i think you can drop 'in'

+    // see we have not reached the last word yet
this is currently a statement indicating to a confounded reader, do you want to
make this a question?

        Attachment #93419 -
        Attachment is obsolete: true

    
    

    
  
timeless@mac.com wrote:
> *Ave* => *Avg* unless you can't in which case i'd like to complain to whomever
> r/sr'd that.

Gnnnn. This isn't easy. The whole damn Win32 code is full of that... and all
platforms require to be in sync AFAIK.
Not fixed... ;-(

> +  //This if block is mean to speedup the process in normal situation, when
> 
> mean=>meant. or use 'designed' instead

Fixed.

> +          if ((1 == numChars) && (aString[start] == ' ')) {
> you don't have to worry about alternative space characters?
> and this code doesn't have to worry about bidi?

Good question. Win32 uses the same code and it causes no problem there - but
that does not mean that this may be wrong if we hit alternal space chars.
I'll try to catch smontagu&co. and file a seperate bug on demand, but currently
it doesn't seem to cause problems...

> +  // Our current state relatively to the _full_ string...
> 'relative'
> +  // This allows emulating the previous code...
> 'emulation of' or 'us to emulate'

Fixed.

> +        // There's no place to back up to so even though the text doesn't fit
> s/to so/to, so/
> (i wonder why this is a common nit)

Fixed.

> +        // until we at least measure the first word entirely
> 'entire first word'

Fixed.

> +          // from now on, the estimated number of characters is what we want
> to measure
> 'we want to measure the estimated number of characters'

Fixed.

> +  NS_ASSERTION(allDone || start == i, "internal error");
> +  NS_ASSERTION(allDone || data->mNumCharsFit != numCharsFit, "internal
> error");
> so if allDone, assert "internal error" twice?

Mhhh, that's in |DEBUG_rbs| ...
... but: fixed.

> +  // return earlier, it would mean that the unique word needs several fonts
> +  // and we will still have to loop over the fonts to return the final height
> case/tense mix error
> "if we didn't" (hypothetical past?) "needs" (ongoing present) "still have to"
> (subjunctive present ongoing or something -- excuse me for thinking in
spanish)

Ugh. Do you have any suggestion for a replacement or do you want that I get my
english books from by school time to get that sentence correct ? :)

> i think my comments apply to at least the two toolkits patched here if not the
> ones form which the patch originated. (this thought indicates i'm reviewing
the
> second toolkit)

The new patch fixes GTK+/Xlib and the source of the evil comments
(gfx/src/windows) ...

> +          // Remember we're in the middle of a segment and not in between
> +          // two segments
> i think you can drop 'in'

Fixed.

> +    // see we have not reached the last word yet
> this is currently a statement indicating to a confounded reader, do you want
to
> make this a question?

Fixed.

    
    

    
  
timeless wrote in comment 54:
> +  NS_ASSERTION(allDone || start == i, "internal error");
> +  NS_ASSERTION(allDone || data->mNumCharsFit != numCharsFit, "internal
> error");
> so if allDone, assert "internal error" twice?

No, I think you have assertions backwards.  If |allDone| is true, then the
assertions will never fire.

    
    

    
  
Requesting r=/sr=/a= etc. etc.
Keywords: patch, 
          
            review

    
    

    
  
You might want to replace CCMAP_HAS_CHAR with CCMAP_HAS_CHAR_EXT. The latter one
can handle surrogate pair (ie. characters outside BMP). (katakai is working on
that right now, we will need this in near future.)  After that, mark r=shanjian

    
    

    
  
Shanjian Li wrote:
> You might want to replace CCMAP_HAS_CHAR with CCMAP_HAS_CHAR_EXT. The latter 
> one can handle surrogate pair (ie. characters outside BMP). (katakai is 
> working on that right now, we will need this in near future.)  After that, 
> mark r=shanjian

Can we forward this change to the follow-up (see comment #53 & co.) for this
bug, please ? The current patch is already ~~77KB and it'd like to check it in
ASAP to avoid that it gets rotten again...

    
    

    
  
What, you delayed the gtk version of the patch for over a month because there
wasn't an xlib version, and now it has to be in immediately, without even fixing
reviewers comments?  Sounds fishy to me.

    
    

    
  
David Baron wrote:
> What, you delayed the gtk version of the patch for over a month because there
> wasn't an xlib version, and now it has to be in immediately, without even 
> fixing reviewers comments?  Sounds fishy to me

I did not delay it due the Xlib port. I delayed it due the problem that the code
does not run on platforms which want native alignments of datatypes (for
example: SPARC) - which requires rewrite of the "some" code in nsTextFrame. I
tried various versions and finally came to the conclusion that it is better to
push the current patch "in" (since I fear that the major upcoming patches for
the gfx/src/gtk code will kill this patch) and then focus on the followups
(rewrite nsTextFrame's measuring code, fix shanijian's suggestion, investigate
possible solutions for bug 50998 etc. etc.).

    
    

    
  
I have already suggested to Katakai to replace CCMAP_HAS_CHAR_EXT with 
CCMAP_HAS_CHAR in patch for bug 127713 so I don't think this particular issue 
should stop this patch.



    
    

    
  
Comment on attachment 93563 [details] [diff] [review]
New patch for 2002-07-27-08-trunk per dbaron's previous comment (|DEBUG_rbs| restored to original functionality)

sr=rbs

some typos:

-	   // from now on, the estimated number of characters is what we want
to measure
+	   // from now on, we want to measure the estimated number of
characters

The new comment doesn't reflect the true story. The idea at that point is that
you don't care anymore what is the _real_  estimated number of characters that
fits. Rather, you have no where to break and has to measure one word fully, but
the real estimate is less than that one word. However, since the other bits of
code rely on what is in "data->mEstimatedNumChars", you want to override
"data->mEstimatedNumChars" and pass in what _has_ to be measured so that it is
transparent to the other bits that depend on it.

+    // 							 if the
whitespace
+    // happens to come from a font with a bigger ascent and/or descent than
all
+    // current fonts on the line, this can cause the next lines to be shifted
+    // down the window is slowly resized to fit that whitespace.
	   ^^^when

Long-standing typo.


[Hope that at some stage the other codes (GetWidth, GetTextDimensions,
DrawString) could be migrated to the callback of ResolveForwards because it can
help to hide the particularities of fonts (XFont, FreeType, etc), and brings
the testing for surrogate chars in one location, making maintenance and
extensions easier.]

        Attachment #93563 -
        Flags: superreview+

        Attachment #93563 -
        Flags: review+

    
    

    
  
Brian Stell wrote:
> I have already suggested to Katakai to replace CCMAP_HAS_CHAR_EXT with 
> CCMAP_HAS_CHAR in patch for bug 127713 so I don't think this particular issue 
> should stop this patch.

I've talked to shanjian in the meantime, his email convinced me that a change
from |CCMAP_HAS_CHAR| to |CCMAP_HAS_CHAR_EXT| is more or less safe.
I'll file a new patch when my build is "done" ...



    
    

    
  
My concern is not that it is safe but that it is unnessary. If we do not have
a clear need for both CCMAP_HAS_CHAR and CCMAP_HAS_CHAR_EXT then lets 
eliminate the _EXT version. If it is not needed them it is an unnecessary
place where developers in the future could be confused (and this whole area
is fairly complex already).

    
    

    
  


  
New patch to match rbs's superreview comments.

I've not changed s/CCMAP_HAS_CHAR/CCMAP_HAS_CHAR_EXT/ for now (but I am willing
to file a sepeate add-on-patch on demand to change that... :)
Additionally I added tests for two new env vars to turn the new code "on"
(="MOZILLA_GFX_ENABLE_FAST_MEASURE") or
"off"(="MOZILLA_GFX_DISABLE_FAST_MEASURE") to make regression testing easier
for non-developers (e.g. on customer side). We have (at least) two follow-up
bugs to this one and one of them can remove that extra code when we are sure
that we don't need it anymore.

        Attachment #93563 -
        Attachment is obsolete: true

    
    

    
  
Comment on attachment 94534 [details] [diff] [review]
New patch for 2002-08-04-08-trunk

r=smontagu for the env var additions (on the understanding that there will be a
new bug filed to clean them up)

        Attachment #94534 -
        Flags: review+

    
    

    
  
Comment on attachment 94534 [details] [diff] [review]
New patch for 2002-08-04-08-trunk

sr=darin for the additional PR_GetEnv checks.  assuming the only change was to
the getenv_done blocks.

        Attachment #94534 -
        Flags: superreview+

    
    

    
  
Patch checked-in
(http://bonsai.mozilla.org/cvsquery.cgi?module=MozillaTinderboxAll&branch=HEAD&cvsroot=/cvsroot&date=explicit&mindate=1028851800&maxdate=1028852340&who=smontagu%25netscape.com),
marking bug as FIXED.
Status: ASSIGNED → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED

    
    

    
  
Follow-up bugs:
- bug 161826 ("nsTextFrame::MeasureText()'s fast text measuring codepath crashes
on RISC machines")
- bug 161825 ("GetWidth optimizations need to be implemented on BeOS (text
measurement performance)")
- bug 161827 ("[ps] GetWidth optimizations need to be implemented for the
PostScript module")
Blocks: 161825, 161826, 161827

    
    

    
  
It appears that this commit have added two "might be used uninitialized"
warnings (see also bug 59652) to brad TBox:

+layout/html/base/src/nsTextFrame.cpp:4638
+ `PRUnichar * bp2' might be used uninitialized in this function
+ `char * bp1' might be used uninitialized in this function

    
    

    
  
side-notice, as this patch was about text-rendering performance. Don't wish to
open bug for "alien" platforms :)

At least Gtk code seems non-optimal when rendering strings with "align=justify"
- it iterates position for every char and calls DrawString aLength times.
In BeOS i used word-by-word method, which seems much more (depending on average
word length in justified text) effective.

    
  
No longer blocks: 161825

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: