138097 - Do Arabic shaping in nsTextTransformer.cpp

Status: RESOLVED FIXED

(Core :: Layout: Text and Fonts, defect)

Description

[Simon Montagu :smontagu]

This is the main part of bug 99823. If we do Arabic shaping in
nsTextTransformer::GetNextWord (called from nsTextFrame::PrepareUnicodeText)
instead of in nsTextFrame::PaintUnicodeText, the text will be shaped whenever we
measure it and layout and selection will become much more accurate.

Comment 1

[Simon Montagu :smontagu]

Marking dependency

Comment 2

[Simon Montagu :smontagu]

Attachment: Patch v.1

Comment 3

[Simon Montagu :smontagu]

Attachment: Patch v.2

This patch is cleaner and works better. After some cross-platform testing I
will ask for review

Comment 4

[Simon Montagu :smontagu]

Attachment: Patch v.3

Some small improvements here, chiefly creating a new |ReorderUnicodeText|
instead of modifying the old |FormatUnicodeText|, which means that XUL doesn't
get horked.
(I will have to file another dependent to bug 99823 for cleaning up after all
the other dependencies).

I have tested this on a lot of Arabic & Persian pages on both Win2K (with
Arabic support) and Linux RH7.2 (without) and it looks ready for review.

Comment 5

[Simon Montagu :smontagu]

ftang, can you r= ?

Comment 6

[Frank Tang]

Comment on attachment 83443 [details] [diff] [review]
Patch v.3

r=ftang
the SetIsBidiSystem and GetIsBidiSystem sandwich code is a little bit hacky but
acceptable. We need to document that clearly. please file a different bug to
rename that two function to something like SetIsBidiSystemState and remember to
fix that later. 

Please run page load performance in your optimize build and make sure the new
code added to text transformer won't introduce performance slow down.

Comment 7

[Simon Montagu :smontagu]

I already ran the pageloader tests and didn't see any regression in performance.

Comment 8

[Chris Waterson]

Comment on attachment 83443 [details] [diff] [review]
Patch v.3

>Index: layout/base/src/nsBidiPresUtils.cpp
>===================================================================
>+nsresult
>+nsBidiPresUtils::ReorderUnicodeText(PRUnichar*       aText,
>+                                    PRInt32&         aTextLength,
>+                                    nsCharType       aCharType,
>+                                    PRBool           aIsOddLevel,
>+                                    PRBool           aIsBidiSystem)
>+{
>+  nsresult rv = NS_OK;
>+  PRBool doReverse = PR_FALSE;
>+

Probably ought to assert that aIsOddLevel == 0 or == 1, since you'll be bit
twiddling with it, below.

>+  if (aIsBidiSystem) {
>+    if ( (CHARTYPE_IS_RTL(aCharType)) ^ (aIsOddLevel) )
>+      doReverse = PR_TRUE;
>+  }
>+  else {
>+    if (aIsOddLevel)
>+      doReverse = PR_TRUE;
>+  }
>+
>+  if (doReverse) {
>+    PRInt32    newLen;
>+
>+    if (mBuffer.Length() < aTextLength) {
>+      mBuffer.SetLength(aTextLength);
>+    }
>+    PRUnichar* buffer = (PRUnichar*)mBuffer.get();
>+

What is the lifetime of this object? (Does mBuffer ever shrink?) It looks like
the buffer will hang around, being as large as the largest text that ever had
to get wrapped (which could be large, given the way that we compute preferred
size in table cells by doing an unconstrained reflow).

>+    if (doReverse) {
>+      rv = mBidiEngine->WriteReverse(aText, aTextLength, buffer,
>+                                     NSBIDI_DO_MIRRORING, &newLen);
>+      if (NS_SUCCEEDED(rv) ) {
>+        aTextLength = newLen;
>+        memcpy(aText, buffer, aTextLength * sizeof(PRUnichar) );
>+      }
>+    }
>+  }
>+  return rv;
>+}
>+

>Index: layout/html/base/src/nsTextTransformer.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/layout/html/base/src/nsTextTransformer.cpp,v
>retrieving revision 1.66
>diff -u -r1.66 nsTextTransformer.cpp
>--- layout/html/base/src/nsTextTransformer.cpp	26 Jan 2002 00:04:35 -0000	1.66
>+++ layout/html/base/src/nsTextTransformer.cpp	14 May 2002 00:15:11 -0000
>@@ -1360,8 +1390,94 @@
>   return result;
> }
> 
>-//----------------------------------------------------------------------
>+#ifdef IBMBIDI
>+void
>+nsTextTransformer::DoArabicShaping(PRUnichar* aText, 
>+                                   PRInt32& aTextLength, 
>+                                   PRBool* aWasTransformed)
>+{
>+  if (aTextLength <= 0)
>+    return;
>+  
>+  PRInt32 newLen;
>+  PRBool isVisual;
>+  mPresContext->IsVisualMode(isVisual);
>+
>+  nsAutoString buf;
>+  buf.SetLength(aTextLength);
>+  PRUnichar* buffer = (PRUnichar*)buf.get();

Note that if SetLength fails (e.g., because the string is too long for
malloc() to work), you'll start slapping bits into stack memory.

Is there any chance that this could be re-written to use nsAString and
the like?

(I'll file a bug on strings so that a failed SetLength will abort the
program.)


>+#ifdef IBMBIDI
>+  // Returns PR_TRUE if the text in the transform bufer needs Arabic
>+  // shaping
>+  PRBool NeedsArabicShaping() const {
>+    return (mFlags & NS_TEXT_TRANSFORMER_DO_ARABIC_SHAPING) ? PR_TRUE : PR_FALSE;
>+  }

How about (mFlags & NS_FOO) != 0, instead. Easier to optimize.

>+  
>+  // Returns PR_TRUE if the text in the transform bufer needs numeric
>+  // shaping
>+  PRBool NeedsNumericShaping() const {
>+    return (mFlags & NS_TEXT_TRANSFORMER_DO_NUMERIC_SHAPING) ? PR_TRUE : PR_FALSE;
>+  }

Same.

sr=waterson if you think about and or deal with the above.

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 83443 [details] [diff] [review]
Patch v.3

>+    PRUnichar* buffer = (PRUnichar*)mBuffer.get();

You probably ought to use

  PRUnichar* buffer = NS_CONST_CAST(PRUnichar*, mBuffer.get());

here (and in similar places) to make it clearer what the
cast is for (and make it more visible, since it's somewhat
evil).

Comment 10

[Simon Montagu :smontagu]

Most of the issues raised in comment 8 and comment 9 are on code based on
existing code, which I would rather file new bugs on rather than making the new
code better, but inconsistent with the existing.

This applies to:

>Probably ought to assert that aIsOddLevel == 0 or == 1, since you'll be bit
>twiddling with it, below.

>What is the lifetime of this object? (Does mBuffer ever shrink?) It looks like
>the buffer will hang around, being as large as the largest text that ever had
>to get wrapped (which could be large, given the way that we compute preferred
>size in table cells by doing an unconstrained reflow).

>How about (mFlags & NS_FOO) != 0, instead. Easier to optimize.

and 

>You probably ought to use
>
>  PRUnichar* buffer = NS_CONST_CAST(PRUnichar*, mBuffer.get());
>
>here (and in similar places) to make it clearer what the
>cast is for (and make it more visible, since it's somewhat
>evil).

That leaves only this:

>Note that if SetLength fails (e.g., because the string is too long for
>malloc() to work), you'll start slapping bits into stack memory.
>
>Is there any chance that this could be re-written to use nsAString and
>the like?

We already have bug 125635 on converting all the interfaces in nsBidiUtils.cpp
to use nsAStrings. ArabicShaping isn't mentioned explicitly, and I will add a
note to the bug so I don't forget it.

I will prevent an overrun in this call to |ArabicShaping| 

>+  ArabicShaping(aText, aTextLength, buffer, (PRUint32 *)&newLen, !isVisual, 
>!isVisual);

by changing the length argument from |aTextLength| to |bufferLength()|, and then
take the sr= and run.
 

can I take the sr= and run?

Comment 11

[Simon Montagu :smontagu]

Attachment: Patch v.4

Changed as described in the previous comment, also made the change to
|NeedsArabicShaping| and parallel methods, added the assertion on aIsOddLevel,
and changed the casts to NS_CONST_CAST as dbaron suggested.

That leaves open the issue with mBuffer in nsBidiPresUtils, which I will file a
new bug on, and converting the methods in nsBidiUtils to use nsAString in bug
125635

Comment 12

[Simon Montagu :smontagu]

Checked in to trunk