62777 - Bidi support from IBM

Status: VERIFIED FIXED

(Core :: Internationalization, defect, P3)

Description

[Mike Kaply [:mkaply]]

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

Comment 1

[Mike Kaply [:mkaply]]

Changing QA contact to me temporarily to avoid tons of spam to 
[e-mail].

Comment 2

[Mike Kaply [:mkaply]]

Attachment: Diffs for mozilla/intl directory

Comment 3

[Mike Kaply [:mkaply]]

Attachment: intl/uconv/ucvibm/864i.uf

Comment 4

[Mike Kaply [:mkaply]]

Attachment: intl/uconv/ucvibm/864i.ut

Comment 5

[Mike Kaply [:mkaply]]

Attachment: intl/uconv/ucvibm/nsCP864iToUnicode.cpp

Comment 6

[Mike Kaply [:mkaply]]

Attachment: intl/uconv/ucvibm/nsCP864iToUnicode.h

Comment 7

[Mike Kaply [:mkaply]]

Attachment: intl/uconv/ucvibm/nsUnicodeToCP864i.cpp

Comment 8

[Mike Kaply [:mkaply]]

Attachment: intl/uconv/ucvibm/nsUnicodeToCP864i.h

Comment 9

[Mike Kaply [:mkaply]]

Attachment: intl/unicharutil/public/nsBidiOptions.h

Comment 10

[Mike Kaply [:mkaply]]

Attachment: intl/unicharutil/public/nsIBidi.h

Comment 11

[Mike Kaply [:mkaply]]

Attachment: intl/unicharutil/public/nsIUBidiCategory.h

Comment 12

[Mike Kaply [:mkaply]]

Attachment: intl/unicharutil/public/nsIUBidiUtils.h

Comment 13

[Mike Kaply [:mkaply]]

Attachment: intl/unicharutil/src/bidicattable.h

Comment 14

[Mike Kaply [:mkaply]]

Attachment: intl/unicharutil/src/nsBidiImp.cpp

Comment 15

[Mike Kaply [:mkaply]]

Attachment: intl/unicharutil/src/nsBidiImp.h

Comment 16

[Mike Kaply [:mkaply]]

Attachment: intl/unicharutil/src/nsBidiUtilsImp.cpp

Comment 17

[Mike Kaply [:mkaply]]

Attachment: intl/unicharutil/src/nsBidiUtilsImp.h

Comment 18

[Mike Kaply [:mkaply]]

Attachment: intl/unicharutil/src/symmtable.h

Comment 19

[Mike Kaply [:mkaply]]

Attachment: intl/unicharutil/tools/BidiMirroring.txt

Comment 20

[Mike Kaply [:mkaply]]

Attachment: intl/unicharutil/tools/genbidicattable.pl

Comment 21

[Mike Kaply [:mkaply]]

Attachment: intl/unicharutil/tools/gensymmtable.pl

Comment 22

[Mike Kaply [:mkaply]]

Attachment: Diffs for mozilla/xpfe directory

Comment 23

[Mike Kaply [:mkaply]]

Attachment: xpfe/components/prefwindow/resources/content/pref-BidiOptions.xul

Comment 24

[Mike Kaply [:mkaply]]

Attachment: Diffs for mozilla/widget directory

Comment 25

[Mike Kaply [:mkaply]]

Attachment: widget/public/nsIBidiKeyboard.idl

Comment 26

[Mike Kaply [:mkaply]]

Attachment: mozilla/widget diffs - with new files included (not attached separately)

Comment 27

[Mike Kaply [:mkaply]]

Attachment: mozilla/layout diffs - with new files included

Comment 28

[Mike Kaply [:mkaply]]

Attachment: Build changes - configure.in and mozilla\config

Comment 29

[Mike Kaply [:mkaply]]

Attachment: mozilla/docshell changes

Comment 30

[Mike Kaply [:mkaply]]

Attachment: mozilla/dom changes

Comment 31

[Mike Kaply [:mkaply]]

Attachment: mozilla/editor changes

Comment 32

[Mike Kaply [:mkaply]]

Attachment: mozilla/gfx diffs

Comment 33

[Mike Kaply [:mkaply]]

Attachment: all.js changes

Comment 34

[Mike Kaply [:mkaply]]

Attachment: mozilla/rdf diffs

Comment 35

[Mike Kaply [:mkaply]]

Attachment: mozilla/view diffs

Comment 36

[Mike Kaply [:mkaply]]

Attachment: last one - mozilla/webshell diffs

Comment 37

[Mike Kaply [:mkaply]]

Attachment: List of all files changed/created for Bidi

Comment 38

[buster]

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

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

[Mike Kaply [:mkaply]]

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.

Comment 40

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 41

[Brendan Eich [:brendan]]

Cc'ing alecf, who along with a bunch of us is on a mission to expunge
nsBrowserInstance.cpp/.h from the face of Mozilla.

/be

Comment 42

[Alec Flett]

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

[Lina Kemmel]

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

[Lina Kemmel]

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.

Thanks

Comment 45

[Chris Waterson]

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

Comment 46

[Lina Kemmel]

waterson: yes, I see. Besides dom/public/idl/coreDOM/Document.idl, may I edit
dom/tools/JSStubGen.cpp?

Comment 47

[Chris Waterson]

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?

Comment 48

[Simon Montagu :smontagu]

>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

[Lina Kemmel]

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

[buster]

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.

Comment 51

[Frank Tang]

In 
===================================================================
RCS file: /cvsroot/mozilla/xpfe/components/prefwindow/resources/content/
Makefile.in,v
retrieving revision 1.37
diff -u -r1.37 Makefile.in
--- Makefile.in 2000/08/25 03:18:13     1.37
+++ Makefile.in 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

[Alec Flett]

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

Comment 53

[Mike Kaply [:mkaply]]

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

Comment 54

[Erik van der Poel]

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

[Erik van der Poel]

sr=erik for the following files:

M intl/uconv/src/charsetData.properties
M intl/uconv/src/charsetTitles.properties
M intl/uconv/src/charsetalias.properties
M modules/libpref/src/init/all.js

Comment 56

[Olivier Cahagne]

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

[Erik van der Poel]

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

Comment 58

[Mike Kaply [:mkaply]]

I'm marking this bug fixed.

Ths IBMBIDI code is in.

We will use:

http://bugzilla.mozilla.org/show_bug.cgi?id=75982

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.

Comment 59

[Mike Kaply [:mkaply]]

Sorry for the spam.

This bug verified as fix.