81032 - nsOutlinerBodyFrame.cpp need to be bidi enable

Status: RESOLVED FIXED

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

Description

[Frank Tang]

Hyatt told me that we also need to bidi enable the new outliner widget
I believe the code is in ayout/xul/base/src/outliner/src/nsOutlinerBodyFrame.cpp 
especially 2089     aRenderingContext.DrawString(realText, textRect.x, 
textRect.y);

Comment 1

[Andreas Becker]

Switching qa contact to [e-mail].

Comment 2

[Frank Tang]

simon: I believe we use nsOutlinerBodyFrame a lot. (for example, the URL auto
complete), can you fix this one?

Comment 3

[Frank Tang]

move component

Comment 4

[Simon Montagu :smontagu]

Attachment: Suggested patch

Comment 5

[David Hyatt]

I'm concerned about the need to iterate over the characters every time to
determine whether or not bidi is enabled.  It also seems that once you've
determined whether or not bidi is enabled for a given outliner, you don't need
to keep checking the characters of the cell text.

Perhaps the question of whether or not Bidi is enabled should be answered by the
view attached to the outliner?

Comment 6

[Simon Montagu :smontagu]

>Perhaps the question of whether or not Bidi is enabled should be answered by the
>view attached to the outliner?

That sounds reasonable, but I can't work out how and where to implement it. Can
you give me some pointers?

Comment 7

[Zach Lipton [:zach]]

Mass-move all BiDi Hebrew and Arabic qa to me, [e-mail]. 
Thank you Gilad for your service to this component, and best of luck to you 
in the future.

Sholom. 

Comment 8

[Simon Montagu :smontagu]

Attachment: Half of new solution

Divide and rule time... This is a patch to make nsOutlinerBodyFrame.cpp display
Bidi text correctly, assuming that the BidiEnabled flag has been set in the
PresContext's mDocument.

What is still lacking is a technique to set that flag, similar to what happens
now in nsGenericDOMDataNode.cpp

Comment 9

[Simon Montagu :smontagu]

*** Bug 119894 has been marked as a duplicate of this bug. ***

Comment 10

[Simon Montagu :smontagu]

*** Bug 125174 has been marked as a duplicate of this bug. ***

Comment 11

[Simon Montagu :smontagu]

*** Bug 125966 has been marked as a duplicate of this bug. ***

Comment 12

[Frank Tang]

nsbeta1+, please measure the performance with ji

Comment 13

[Simon Montagu :smontagu]

Maybe it would be no big deal to go through the Bidi path in all cases, since
the Bidi rendering is well optimized for cases which need no special treatment.
I just tested a patch that does that, displaying a full bookmark sidebar with
MOZ_TIMELINE.

The text drawing routine in nsOutlineBodyFrame::PaintText was called 131 times.
With no bidi, this took 0.189 seconds on average, and with bidi 0.194 seconds.
This translates to an increase of less than 0.1% of the whole startup.

Comment 14

[Simon Montagu :smontagu]

Attachment: Testing patch

This is the patch referred to in comment 13

Comment 15

[Simon Montagu :smontagu]

mkaply: if you agree that attachment 72717 [details] [diff] [review] is the least bad way to do this, can
you r= ?

Comment 16

[Mike Kaply [:mkaply]]

Comment on attachment 72717 [details] [diff] [review]
Testing patch

r=mkaply

Comment 17

[David Hyatt]

sr=hyatt

Comment 18

[Asa Dotzler [:asa]]

Comment on attachment 72717 [details] [diff] [review]
Testing patch

a=asa (on behalf of drivers) for checkin to the 1.0 trunk

Comment 19

[Simon Montagu :smontagu]

Patch checked in