Mail message subjects don't get BiDi applied

RESOLVED FIXED

Status

()

defect
RESOLVED FIXED
18 years ago
11 years ago

People

(Reporter: ilya.konstantinov+future, Assigned: mkaply)

Tracking

Trunk
x86
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 3 obsolete attachments)

Reporter

Description

18 years ago
While UTF-8 and WINDOWS-1255 messages are displayed correctly (with BiDi), their
Hebrew subjects are displayed backwards.
Can you attach a screen shot? The subject of messages is displayed in several
different places, and I'm not quite sure what we're talking about.
Reporter

Comment 2

18 years ago
Posted image screenshot

Comment 3

18 years ago
Marking NEW.
Status: UNCONFIRMED → NEW
Ever confirmed: true
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: momoi → zach
Posted patch suggested patch (obsolete) — Splinter Review
Fixes the bug by adding |mDocument->SetBidiEnabled()| to codepaths in
nsGenericDOMDataNode.cpp where it wasn't being called.
Assignee

Comment 6

18 years ago
Comment on attachment 59411 [details] [diff] [review]
suggested patch

r=mkaply
Attachment #59411 - Flags: review+
We *really* don't want to increase the size of every data node in mozilla by 32
(or 64) bits just to have this state in the data nodes. We need an other
solution. As far as I can see, we can either jam this state into a low-bit in
one of the pointers in nsGenericDOMDataNode, or we can move this state into the
nsTextFragment , there we can use a bit from the length member at the cost of
limiting the length to 29 bits, in stead of 30, which seams reasonable to me,
2^29 is still 536,870,912 characters. Sounds like that's way more than we need
to support. If adding this to nsTextFragment makes sense from a BIDI point of
view, do that, if not, squeeze the bit into one of the pointers in
nsGeneridDOMDataNode.
Attachment #59411 - Attachment is obsolete: true
Attachment #60902 - Attachment is obsolete: true
Comment on attachment 61205 [details] [diff] [review]
Attachment 60902 [details] [diff] merged to tip

[...]
>+
>+#ifdef IBMBIDI
>+void nsGenericDOMDataNode::SetBidiStatus()
>+{
>+  PRBool isBidiDocument = PR_FALSE;
>+  if (nsnull != mDocument) {
>+    mDocument->GetBidiEnabled(&isBidiDocument);
>+    if (isBidiDocument) {
>+      // OK, we already know it's Bidi, so we won't test again
>+      return;
>+    }
>+  }
>+  mText.SetBidiFlag();
>+  if (nsnull != mDocument && mText.IsBidi()) {
>+    mDocument->SetBidiEnabled(PR_TRUE);
>+  }
>+}

Loose the "!= nsnull" in the if checks, simply if (mDocument) is enough. I'm
getting rid of all "!= nsnull"'s in this file...

>Index: mozilla/content/shared/public/nsTextFragment.h
[...]
>   struct FragmentBits {
>     PRBool mInHeap : 1;
>     PRBool mIs2b : 1;
>+#ifdef IBMBIDI
>+    PRBool mIsBidi : 1;
>+    PRUint32 mLength : 29;
>+#else
>     PRUint32 mLength : 30;
>+#endif

Loose the #ifdef here, leave the 1-bit mIsBidi property there even if BIDI is
not enabled, I'd prefere leaving mLength the same regardless of weither or not
BIDI is enabled.

>Index: mozilla/content/shared/src/nsTextFragment.cpp
[...]
>+#ifdef IBMBIDI
>+// To save time we only do this when we really want to know, not during
>+// every allocation
>+void
>+nsTextFragment::SetBidiFlag()
>+{
>+  mState.mIsBidi = PR_FALSE;
>+
>+  if (mState.mIs2b) {

Why clear the mIsBidi flag here? Why not leave the flag untouched above and
make the if check check that the flag is not already set, that way calling this
method more than once on the same fragment wouldn't causes mozilla to loop over
the text twice. The mIsBidi flag is cleard every time the text changes in a
fragment anyway.

Other than that, sr=jst
Attachment #61205 - Flags: superreview+
Fix checked in. Notice that this only fixes the subject in the messageinfo pane,
not in the headers pane (I hope I'm using the right terms). The headers pane is
an outliner, so it's an instance of bug 81032
Status: NEW → RESOLVED
Last Resolved: 18 years ago
Resolution: --- → FIXED

Updated

18 years ago
Blocks: 115713

Updated

11 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.