19 years ago
18 years ago


(Reporter: mkaply, Assigned: mkaply)



Firefox Tracking Flags

(Not tracked)



(36 attachments)

12.97 KB, patch
Details | Diff | Splinter Review
5.12 KB, text/plain
4.12 KB, text/plain
2.05 KB, text/plain
1.83 KB, text/plain
2.48 KB, text/plain
2.08 KB, text/plain
16.59 KB, text/plain
32.94 KB, text/plain
6.01 KB, text/plain
17.22 KB, text/plain
82.85 KB, text/plain
78.31 KB, text/plain
8.16 KB, text/plain
14.41 KB, text/plain
3.45 KB, text/plain
11.24 KB, text/plain
8.87 KB, text/plain
10.35 KB, text/plain
4.98 KB, text/plain
23.60 KB, patch
Details | Diff | Splinter Review
6.43 KB, text/plain
18.37 KB, patch
Details | Diff | Splinter Review
1.28 KB, patch
Details | Diff | Splinter Review
50.83 KB, patch
Details | Diff | Splinter Review
269.03 KB, patch
Details | Diff | Splinter Review
2.11 KB, patch
Details | Diff | Splinter Review
2.13 KB, patch
Details | Diff | Splinter Review
4.10 KB, patch
Details | Diff | Splinter Review
9.20 KB, patch
Details | Diff | Splinter Review
17.60 KB, patch
Details | Diff | Splinter Review
1.87 KB, patch
Details | Diff | Splinter Review
1.69 KB, patch
Details | Diff | Splinter Review
4.17 KB, patch
Details | Diff | Splinter Review
1.14 KB, patch
Details | Diff | Splinter Review
6.50 KB, text/plain


19 years ago
I am using this bug to attach all the diffs and files for Bidi

Comment 1

19 years ago
Changing QA contact to me temporarily to avoid tons of spam to
QA Contact: teruko → mkaply

Comment 3

19 years ago

Comment 4

19 years ago

Comment 29

19 years ago

Comment 30

19 years ago

Comment 31

19 years ago

Comment 32

19 years ago

Comment 33

19 years ago

Comment 34

19 years ago

Comment 35

19 years ago

Comment 38

19 years ago
for a change of this magnitude, we'll need to divide the reviewing task.
Furthermore, I think after familiarizing themselves with the changes, the
reviewers need to get together and talk about any architectual or cross-module

rather than a long discussion in a bug report, I'll send out a note to everyone
I think needs to be involved, and we'll coordinate.

raw diffs of any size are very difficult to deal with in a vaccuum.  Michael,
can you provide some sort of summary document that discusses what you changed in
each module, and why?  It doesn't need to be exhaustive.  It just needs to act
as a guide to the reviewers.  Or, if there are already extensive comments in the
code, that should be good enough.  Just point them out so we don't have to go
digging for them.  This will speed up the process immensely.

Comment 39

19 years ago
Summary of Bidi Support in Mozilla:

Add a Reordering engine, providing a number of APIs for the full or partial 
implementation of the Unicode Bidi Algorithm.

Add a Shaping engine, performing literal and numeral shaping.

Rendering layer: build a document presentation model consisting of strictly 
uni-directional elements; format each element before rendering (including 
reordering, shaping, and symmetric swapping).

Rendering context: provide some APIs for the rendering layer (such as to query 
Bidi capabilities of the system); deal with platform specific issues (rendering 
with different fonts).

UI / Composer: enhance the interface for entering Bidi data.

UI / Bidi Options: add new items to the User Preferences and View Menu.
I haven't had a chance to look at most of the layout diffs, but here
are some comments after a quick look at *some* of the other patches
and the beginning of the layout patch.  I don't claim to be
knowledgable in all these areas, so don't take them too seriously.

dom/src/coreDOM/nsJSDocument.cpp - This file is auto-generated
so it shouldn't be modified by hand.  It is generated from
dom/public/idl/coreDOM/Document.idl.  Why do you want these changes
anyway?  Shouldn't the .dir property be associated with the root
element (or other elements) of the document instead?

xpfe/browser/src/nsBrowserInstance.cpp - This file is deprecated --
see bug 46200.  Why is there an |#ifdef OLDIBMBIDI|?   The corresponding
header file or IDL file changes seem to be missing.  The functions written
here in C++ look like they could and should be written in JS instead.

gfx/public/nsIDeviceContext.h - an interface shouldn't have member
variables.  There should be setter/getter functions, with the instance
variables in the implementation.

gfx/public/nsIDeviceContext.h and nsIRenderingContext.h - Is there
documentation on what these additions do?

view/src/nsScrollingView.cpp - Should the view system really have to
know about frame and style stuff?  Some of these changes might be better
placed in layout/html/base/src/nsScroll*?  But it does seem like the view
system is biased towards scrollbars on the right and bottom.

docshell/base/nsDocShell.cpp - Why transfer the bidi state differently
from all the other state (i.e., set it on the new viewer so late)?

layout/base/public/* - Many of the additions to interfaces don't have
any comments.  It's hard to understand the code without knowing what
these functions are supposed to do.  One shouldn't need to go through
the implementation to figure out the purpose of the function.  This is
especially important for BiDi, because many developers aren't very
familiar with it.

layout/base/public/nsICaret.h - Again, you shouldn't have member variables
on interfaces; they should be in the implementations.

layout/base/public/nsIDocument.h - Is this attached to nsIDocument rather
than just nsIPresContext because of printing, i.e., because you want this
state to persist across multiple views (screen, print) of the same
document?  If so, perhaps that should be documented?

layout/base/public/nsIPresShell.h - There's an extraneous |#endif|
floating around.

layout/base/public/nsTextFragment.h - The purpose of the change here
should be documented - it's not obvious.
Cc'ing alecf, who along with a bunch of us is on a mission to expunge
nsBrowserInstance.cpp/.h from the face of Mozilla.


Comment 42

19 years ago
regarding nsBrowserInstance - the file is not deprecated YET, but no new
functions should be added. Please do NOT touch this file.

 It looks like the functions you need to add are
basically giving JavaScript access to non-scriptable methods of your bidi stuff
from nsIMarkupDocumentViewer:
+  [noscript] void setBidi(in voidPtr aBidi);
+  [noscript] void getBidi(in voidPtr aBidi);
+  [noscript] void setBidiClipboardMode(in voidPtr aBidi

You need to make these scriptable, and access them directly in the places that
you are calling into the nsBrowserInstance/appcore.

Comment 43

19 years ago
Regarding David Baron's comment:

"docshell/base/nsDocShell.cpp - Why transfer the bidi state differently
from all the other state (i.e., set it on the new viewer so late)?" -

we wait untill the new viewer's pres context is initialised.

Comment 44

19 years ago
Regarding David's comment:

"dom/src/coreDOM/nsJSDocument.cpp - ... Why do you want these changes
anyway?  Shouldn't the .dir property be associated with the root
element (or other elements) of the document instead?"

What do you mean by "associating with the root element"?

In the existing bidi code, the .dir property will finally be accociated with 
the root element during a style change reflow.


Comment 45

19 years ago
lkemmel: more importantly, nsJSDocument.cpp is a generated file. You cannot edit 
it `by hand'

Comment 46

19 years ago
waterson: yes, I see. Besides dom/public/idl/coreDOM/Document.idl, may I edit

Comment 47

19 years ago
lkemmel: yes, it is possible to edit that tool, but I wonder if there's a better 
way to accomplish what you're trying to do. Why not just edit Document.idl and 
add a `direction' attribute to the NSDocument interface, then implement that 
attribute in nsGenericDocument.cpp, as with all other properties?
>layout/base/public/nsICaret.h - Again, you shouldn't have member variables
>on interfaces; they should be in the implementations.

These variables are obsolete anyway -- I have removed them.

Comment 49

19 years ago
waterson: I didn't want to add new things to nsDocument, when we're able to use
an existing mechanism... anyway, I'll be very glad to proceed as you suggested.
btw, couldn't find nsGenericDocument.cpp. didn't you mean
layout/base/src/nsDocument.cpp? Thanks.

Comment 50

19 years ago
I have posted a summary of the review meeting on netscape.public.mozilla.layout.
 Please add your comments there.

I would suggest that we do *not* continue to address code details in this bug
report, because it will soon become unreadable.  It's already starting to read
like an IRC session.


19 years ago
Blocks: 26371

Comment 51

19 years ago
RCS file: /cvsroot/mozilla/xpfe/components/prefwindow/resources/content/,v
retrieving revision 1.37
diff -u -r1.37
--- 2000/08/25 03:18:13     1.37
+++ 2000/12/13 22:45:21
@@ -63,6 +63,7 @@
                pref-languages-add.xul \
                pref-languages.js \
                pref-navigator.xul \
+               pref-BidiOptions.js \
                pref-navigator.js \
                pref-offline.xul \
                pref-policies.xul \
In think this should be pref-BidiOptions.xul, right ?

Comment 52

19 years ago
these patches are now almost 2 months old. Are we going to see new patches that
reflect the comments here.

Comment 53

19 years ago
I have just received new files. I will merge and diff them early next week.

Comment 54

19 years ago
As discussed with Simon, it would be nice to attach the new patches to a new
bug. Otherwise, this bug would become rather unwieldy. Thanks!

Comment 55

18 years ago
sr=erik for the following files:

M intl/uconv/src/
M intl/uconv/src/
M intl/uconv/src/
M modules/libpref/src/init/all.js

Comment 56

18 years ago
Does bug #68433 deal with this code too ?
It deals with Arabic and UTF-8 code, a minor bug that could be solved easily I
guess. :)

Comment 57

18 years ago
Bug 68433 is not related to this bug. Please do not insert any more comments
about that in this bug report. Thanks!


18 years ago
No longer blocks: 64490


18 years ago
No longer blocks: 26371

Comment 58

18 years ago
I'm marking this bug fixed.

Ths IBMBIDI code is in.

We will use:

To track actually turning on the #ifdef in the default build.

Note I have built with IBMBIDI turned on on Windows today and the Bidi code 
compiled and worked.
Last Resolved: 18 years ago
Resolution: --- → FIXED

Comment 59

18 years ago
Sorry for the spam.

This bug verified as fix.
You need to log in before you can comment on or make changes to this bug.