82383 - code drop of ICU arabic shaping engine

Status: RESOLVED WONTFIX

(Core :: Internationalization, defect, P1)

Description

[Frank Tang]

I got this code drop from [e-mail]. Here is the bug to track it

Comment 1

[Frank Tang]

Attachment: diff submit by [e-mail]

Comment 2

[Frank Tang]

--- ..\nsBidiUtilsImp.h
-   NS_IMETHOD HandleNumbers(const nsString aSrc, nsString & aDst );
+
+   NS_IMETHOD HandleNumbers(const nsAutoString aSrc, nsAutoString & aDst );

Why you change that? It is a wrong chagne. nsAutoString should not appeared on 
XPCOM interface.

..\nsIUBidiUtils.h
+  /**
+   * New Arabic Shaping Engine (ICU)
+   */
+    NS_IMETHOD ShapeArabic(const PRUnichar *source, PRInt32 sourceLength,
+                             PRUnichar *dest, PRInt32 &destSize,
+                             PRUint32 options,
+                             UErrorCode *pErrorCode)=0;
The official way in XPCOM is to pass error is by using the nsresult return 
value. Why you use a UErrorCode *pErrorCode here ?

What happen to
NS_IMETHOD ArabicShaping(const PRUnichar* aString, PRUint32 aLen,
 PRUnichar* aBuf, PRUint32* aBufLen)=0;
NS_IMETHOD Conv_FE_06(const nsString aSrc, nsString & aDst) = 0;
NS_IMETHOD Conv_FE_06_WithReverse(const nsString aSrc, nsString & aDst) 
NS_IMETHOD Conv_06_FE_WithReverse(const nsString aSrc, nsString & aDst, PRUint32 
aDir) = 0;

???
We we need to ShapeArabic? Is that to replace these rountines? or just to 
replace ArabicShaping?

 ..\sent\nsBidiPresUtils.cpp
Why is the following lines here ?
+//ahmed
+//#include "ushaping.h"
+//end

Why you need to chane the following ?
-  mBidiEngine = do_GetService("@mozilla.org/intl/bidi;1");
-  if (mBidiEngine) {
+  nsCOMPtr<nsIBidi> bidiEngine = do_GetService("@mozilla.org/intl/bidi;1");
+  if (bidiEngine) {
+    mBidiEngine = bidiEngine;

, should not be in the begining of the line
-                                   PRBool           aIsBidiSystem)
+                                   PRBool           aIsBidiSystem
+                                   ,nsIRenderingContext*  aRenderingContext)

Why you need to change the following?
   if (!mUnicodeUtils) {
-    mUnicodeUtils = do_GetService("@mozilla.org/intl/unicharbidiutil;1");
-    if (!mUnicodeUtils) {
+    nsCOMPtr<nsIUBidiUtils> bidiUtils = 
do_GetService("@mozilla.org/intl/unicharbidiutil;1");
+    if (!bidiUtils) {
       return NS_ERROR_FAILURE;
     }
+    mUnicodeUtils = bidiUtils;

Why you need to change the following?
   if (aIsBidiSystem) {
-    if (CHARTYPE_IS_RTL(aCharType) ^ aIsOddLevel) {
+    if ( (CHARTYPE_IS_RTL(aCharType)) ^ (aIsOddLevel) )
       doReverse = PR_TRUE;
-    }
   }

Why you malloc here ?

+    UErrorCode *pErrorCode;
+    pErrorCode = (UErrorCode *)malloc(1*sizeof(UErrorCode));
+    *pErrorCode = U_ERROR_INFO_START;
    
rv=mUnicodeUtils->ShapeArabic(aText,aTextLength,buffer,newLen,options,pErrorCode
);
should it be 
UErrorCode err;
err = U_ERROR_INFO_START;
rv=mUnicodeUtils->ShapeArabic(aText,aTextLength,buffer,newLen,options,&err);
again, the official way in XPCOM to pass error value is use the return value. 
you should not introduce a seperate output parameter here. 
Why you change the following
- nsresult nsBidiPresUtils::GetBidiEngine(nsIBidi** aBidiEngine)
- {
-   nsresult rv = NS_ERROR_FAILURE;
-   if (mBidiEngine) {
-     *aBidiEngine = mBidiEngine;
-     NS_ADDREF(*aBidiEngine);
-     rv = NS_OK;
-   }
-   return rv; 
- }
+nsresult nsBidiPresUtils::GetBidiEngine(nsIBidi** aBidiEngine)
+{
+  nsresult rv = NS_ERROR_FAILURE;
+  if (mBidiEngine) {
+    *aBidiEngine = mBidiEngine;
+    rv = NS_OK;
+  }
+  return rv; 
+}
..\nsBidiUtilsImp.cpp  
Why you change the following?
 nsBidiUtilsImp::nsBidiUtilsImp()
 {
   NS_INIT_REFCNT();
+  PR_AtomicIncrement(&g_InstanceCount);
 }
 
 nsBidiUtilsImp::~nsBidiUtilsImp()
 {
+  PR_AtomicDecrement(&g_InstanceCount);
 }

Comment 3

[Andreas Becker]

Changing QA contact to [e-mail].

Comment 4

[Maha Abou El-Rous]

changed severity and priority

Comment 5

[Dawn Endico]

fyi

I had plans to make an experimiental arabic 0.9.2 build but applying this patch
resulted in conflicts.

Comment 6

[Frank Tang]

I have create a new patch and send to IBM. That patch will keep ICU code as it 
as possible we can and use nsAWritableStringGenerate to integrate. mkaply, do 
you still have that patch?

Comment 7

[Mike Kaply [:mkaply]]

Attachment: Diff that Frank gave me that involves the Generator

Comment 8

[adani]

hello!
we want the arabic support under linux

Comment 9

[Zain]

has the attachment (id=42116) been added to the source tree??
when can we expect the arabic shaping in the linux version??

Please answer me,

Zain

Comment 10

[Simon Montagu :smontagu]

What is the status of this now that bug 92797 has been fixed? Is this WONTFIX?
Future?

Comment 11

[Jaime Rodriguez, Jr.]

Michael/Ftang - I have the same questions as Simon. Pls advise.

Comment 12

[Mohammed Elzubeir]

Is this bug being deliberately postponed? Approaching 1.0 milestone and it seems
like no one has any intentions of fixing it. What is going on?

Comment 13

[Simon Montagu :smontagu]

I think there is a misunderstanding here. We have had Arabic shaping for over 3
months (from bug 92797). The solution in this bug is an alternative approach
which was being considered for a while. It certainly isn't blocking anything
that I'm aware of.

Comment 14

[Mohammed Elzubeir]

I am confused as to how the verification process works. Who is responsible for
verifying Arabic shaping and support? I am running 0.9.6 and I find the current
Arabic shaping severely broken. How do you perform your tests?

Comment 15

[Roozbeh Pournader]

Mohammed, would you please tell us what do you exactly mean by broken? Arabic
support in Mozilla is still not complete, and has many bugs. It would be great
if you consider running your own tests and file bugs in bugzilla.

Comment 16

[Mohammed Elzubeir]

I. Alignment - on most lines, what is in the rightmost column is not seen
(inconsistent, sometimes one character, sometimes more). The same goes for the
left side.

www.aljazeera.net -- 1. top header is all in '?????'.
                              2. picture captions are all '??????'.

www.ayna.com     --  ISO-8859-6, CP-1256, UTF-8 -- no matter what choice, it's
all '??????'
www.linux4arab.com  -- same as above

(this is performed on a FreeBSD 4.4-STABLE Mozilla 0.9.7 milestone, using the MS
Tahoma TrueType font)

Comment 17

[Mohammed Elzubeir]

I would also like to note this above comment has been tested on Linux and
Windows, with Mozilla 0.9.7, and the same problems occur.

Comment 18

[Simon Montagu :smontagu]

The problems listed in comment 16 are not directly related to this specific bug.

The alignment problem is bug 74929.

The various problems with ??? are also separate issues. Some of them may have
been fixed in recent builds -- I used to see ??? often instead of Arabic
characters on Linux but now I never do.

What I intend to do after fixing bug 120818 is test the performance of the ICU
shaping engine compared to the current Mozilla implementation. If it is
significantly better, or simply more correct, I will check it in.

I will be very grateful if anyone can point to specific bugs in the current
Mozilla shaping of Arabic, as opposed to layout and character display issues. I
know enough Arabic for basic testing, but I am sure there are details that I miss.

Comment 19

[Ahmad A. Abu-Taha]

Kindly, be informed that [e-mail] is no longer in Arabic bugs, so I 
have removed her from the CC list and added myself instead, [e-mail]. 
This comment is to inform you of this new status. Thank you.

Comment 20

[Ahmad A. Abu-Taha]

Kindly be informed that Ahmad A. Abu-Taha ([e-mail]) from IBM Egypt is 
replacing Maha Abou El-Rous ([e-mail]) in monitoring and receiving 
notifications of Mozilla bugs regarding Arabic.