nsOutlinerBodyFrame.cpp need to be bidi enable

RESOLVED FIXED

Status

()

Core
Layout: Text
RESOLVED FIXED
17 years ago
9 years ago

People

(Reporter: Frank Tang, Assigned: smontagu)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

17 years ago
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

17 years ago
Switching qa contact to mahar@eg.ibm.com.
QA Contact: andreasb → mahar
(Reporter)

Comment 2

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

Comment 3

17 years ago
move component
Component: Internationalization → BiDi Hebrew & Arabic
(Assignee)

Comment 4

17 years ago
Created attachment 43995 [details] [diff] [review]
Suggested patch

Comment 5

17 years ago
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?
(Assignee)

Comment 6

17 years ago
>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

17 years ago
Mass-move all BiDi Hebrew and Arabic qa to me, zach@zachlipton.com. 
Thank you Gilad for your service to this component, and best of luck to you 
in the future.

Sholom. 
QA Contact: mahar → zach
(Assignee)

Comment 8

16 years ago
Created attachment 61615 [details] [diff] [review]
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
(Assignee)

Updated

16 years ago
Attachment #43995 - Attachment is obsolete: true
(Reporter)

Updated

16 years ago
Blocks: 115713
(Assignee)

Comment 9

16 years ago
*** Bug 119894 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 10

16 years ago
*** Bug 125174 has been marked as a duplicate of this bug. ***
(Assignee)

Updated

16 years ago
OS: Windows NT → All
Hardware: PC → All
(Assignee)

Comment 11

16 years ago
*** Bug 125966 has been marked as a duplicate of this bug. ***
(Assignee)

Updated

16 years ago
Keywords: nsbeta1
(Reporter)

Comment 12

16 years ago
nsbeta1+, please measure the performance with ji
Keywords: nsbeta1 → nsbeta1+
(Assignee)

Comment 13

16 years ago
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.
(Assignee)

Comment 14

16 years ago
Created attachment 72717 [details] [diff] [review]
Testing patch

This is the patch referred to in comment 13
(Assignee)

Comment 15

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

Updated

16 years ago
Status: NEW → ASSIGNED

Comment 16

16 years ago
Comment on attachment 72717 [details] [diff] [review]
Testing patch

r=mkaply
Attachment #72717 - Flags: review+

Comment 17

16 years ago
sr=hyatt

Comment 18

16 years ago
Comment on attachment 72717 [details] [diff] [review]
Testing patch

a=asa (on behalf of drivers) for checkin to the 1.0 trunk
Attachment #72717 - Flags: approval+
(Assignee)

Comment 19

16 years ago
Patch checked in
Status: ASSIGNED → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED

Updated

9 years ago
Component: Layout: BiDi Hebrew & Arabic → Layout: Text
QA Contact: zach → layout.fonts-and-text
You need to log in before you can comment on or make changes to this bug.