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

Changing QA contact to me temporarily to avoid tons of spam to teruko@netscape.com.

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.

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. /be

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.

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.

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

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

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

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.

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.

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.

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 ?

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

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

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!

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

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. :)

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

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.

Sorry for the spam. This bug verified as fix.