80164 - add mozilla/layout/base/src/nsCopySupport.cpp for bidi

Status: RESOLVED FIXED

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

Description

[Frank Tang]

Comment 1

[Frank Tang]

Attachment: from 80130

Comment 2

[Frank Tang]

need to wait till 80160 done

Comment 3

[Frank Tang]

simon- we already put a stub in the unicharutil for Conv_XXX stuff. It should be 
safe to turn thsi part of code on right now, right?
We need to address the real one when we fix the FULL_ARABIC_SHAPING issue

Comment 4

[Frank Tang]

mjudge- can you review this one?

Comment 5

[kinmoz]

Why don't we just remove the #if/#endif statements completely? Also are the new 
#if comments accurate? What is the "main tree" it's referring to?

Comment 6

[kinmoz]

Also in this part of the code you are enabling, it looks like the multiple if 
statements make it possible that buffer can be set to an empty newBuffer:


#if 0 // Until we finalize the conversion routine
              if (GET_BIDI_OPTION_DIRECTION(bidiOptions) == IBMBIDI_TEXTDIRECTIO
N_LTR) {
                bidiUtils->Conv_FE_06_WithReverse(buffer, newBuffer);
              }
              if (GET_BIDI_OPTION_DIRECTION(bidiOptions) == IBMBIDI_TEXTDIRECTIO
N_RTL) {
                bidiUtils->Conv_FE_06 (buffer, newBuffer);
              }
#endif
            }
            else { //nonbidisystem
              bidiUtils->HandleNumbers(buffer, newBuffer);//ahmed
            }
            buffer = newBuffer;


If we are guaranteed that bidiOptions will always contain LTR or RTL then maybe 
we should use an 'else' with a comment instead of the 2nd 'if'.

Comment 7

[Marek Z. Jeziorek]

TM to 0.9.2 per PDT triage (it's OK to check it in by Friday or after 0.9.1
branch is made).

Comment 8

[Simon Montagu :smontagu]

I agree with kin that the checkin should just remove the #if/#endif, and not 
replace #if 0 with #if 1

What do you mean "if we are guaranteed that bidiOptions will always contain LTR 
or RTL"? I think the opposite is true: the case where bidiOptions contains 
something other than LTR or RTL is where the current code won't initialize 
newBuffer so we need if ... else.

Comment 9

[Simon Montagu :smontagu]

Attachment: new patch addressing kin's comments

Comment 10

[Frank Tang]

mkaply- do you have time to drive and check in this code? I am too busy right 
now.

Comment 11

[Mike Kaply [:mkaply] (PTO until Sept 14)]

Well, I can be the reviewer on this and I shall.

r=mkaply

kin - can I get an sr= on this?

Thanks

Comment 12

[kinmoz]

To answer Simon's question above ... I meant that if there is a guarantee at
all times that either IBMBIDI_TEXTDIRECTION_LTR or IBMBIDI_TEXTDIRECTION_RTL is
set in the bidiOptions that we can use an "if/else" ... but from you comments, I
gather that bidiOptions can also *not* contain IBMBIDI_TEXTDIRECTION_LTR or
IBMBIDI_TEXTDIRECTION_RTL ... also, so from your new patch I see that we always
want to call bidiUtils->Conv_FE_06_WithReverse() for all cases *except* when
IBMBIDI_TEXTDIRECTION_RTL is set.

If that's correct, then sr=[e-mail]

Comment 13

[Frank Tang]

mkaply- can you check in ?

Comment 14

[Mike Kaply [:mkaply] (PTO until Sept 14)]

Fix checked in

Comment 15

[Andreas Becker]

Changing QA contact to default Bidi-Hebrew owner [e-mail].