Closed Bug 82383 Opened 19 years ago Closed 11 years ago

code drop of ICU arabic shaping engine

Categories

(Core :: Internationalization, defect, P1, minor)

defect

Tracking

()

RESOLVED WONTFIX

People

(Reporter: ftang, Assigned: mkaply)

References

Details

Attachments

(2 files)

I got this code drop from mahar@eg.ibm.com. Here is the bug to track it
--- ..\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);
 }


Changing QA contact to mahar@eg.ibm.com.
QA Contact: andreasb → mahar
Blocks: 82089
changed severity and priority
Severity: normal → blocker
Priority: -- → P1
Blocks: 82093
Blocks: 81194
Blocks: 82096
Blocks: 82097
Blocks: 75047
Blocks: 75009
fyi

I had plans to make an experimiental arabic 0.9.2 build but applying this patch
resulted in conflicts.
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?
hello!
we want the arabic support under linux
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
What is the status of this now that bug 92797 has been fixed? Is this WONTFIX?
Future?
Michael/Ftang - I have the same questions as Simon. Pls advise.
No longer blocks: 75009
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?
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.
Severity: blocker → minor
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?
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.
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)
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.
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.
Depends on: 120818
Target Milestone: --- → mozilla0.9.9
Target Milestone: mozilla0.9.9 → ---
Kindly, be informed that mahar@eg.ibm.com is no longer in Arabic bugs, so I 
have removed her from the CC list and added myself instead, ahtaha@eg.ibm.com. 
This comment is to inform you of this new status. Thank you.
Kindly be informed that Ahmad A. Abu-Taha (ahtaha@eg.ibm.com) from IBM Egypt is 
replacing Maha Abou El-Rous (mahar@eg.ibm.com) in monitoring and receiving 
notifications of Mozilla bugs regarding Arabic.
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.