120818 - De-COMtaminate Bidi utilities

Status: RESOLVED FIXED

(Core :: Layout: Text and Fonts, defect)

Description

[Simon Montagu :smontagu]

Some of the functionality in the nsIBidi and nsIBidiUtils interfaces is only
ever used by layout, and could be moved into the non-XPCOM nsBidiPresUtils.

Comment 1

[Simon Montagu :smontagu]

Let me revise that statement after working through the interfaces with lxr ...
the ONLY methods in either nsIBidi or nsIUBidiUtils that are used outside layout
are Conv_FE_06 and its variants, which are used only in layout and content. The
macros in nsIUBidiUtils.h are also used in both layout and content.

I suggest moving the common parts into content/shared and the layout-specific
parts into nsBidiPresUtils.

Comment 2

[Simon Montagu :smontagu]

Attachment: Full diffs (very long)

Comment 3

[Simon Montagu :smontagu]

Attachment: Diffs not including new and old files

Comment 4

[Simon Montagu :smontagu]

Attachment: New nsBidiUtils.h against old nsIUBidiUtils.h

Comment 5

[Simon Montagu :smontagu]

Attachment: new nsBidiUtils.cpp against old nsBidiUtilsImp.cpp

Comment 6

[Simon Montagu :smontagu]

Attachment: New nsBidi.h against old nsIBidi.h

Comment 7

[Simon Montagu :smontagu]

Attachment: New nsBidi.cpp against old nsBidiImp.cpp

Comment 8

[Simon Montagu :smontagu]

Attachment: genbidicattable.pl

Comment 9

[Simon Montagu :smontagu]

Attachment: gensymmtable.pl

Comment 10

[Simon Montagu :smontagu]

The full diffs are very long (15,000+ lines), but most of the changes are moving
complete files around.

/intl/unicharutil/public/nsIUBidiUtils.h --> /content/shared/public/nsBidiUtils.h
/intl/unicharutil/src/nsBidiImp.h            /content/shared/src/nsBidiUtils.cpp
/intl/unicharutil/src/nsBidiImp.cpp

/intl/unicharutil/public/nsIBidi.h       --> /layout/base/public/nsBidi.h
/intl/unicharutil/src/nsBidiImp.h            /layout/base/src/nsBidi.cpp
/intl/unicharutil/src/nsBidiImp.cpp

/intl/unicharutil/tools/genbidicattable.pl --> /layout/tools/genbidicattable.pl
/intl/unicharutil/tools/gensymmtable.pl    --> /layout/tools/gensymmtable.pl
/intl/unicharutil/tools/BidiMirroring.txt  --> /layout/tools/BidiMirroring.txt

These perl scripts generate the following two header files, which are identical
to the originals.

/intl/unicharutil/src/bidicattable.h       --> /layout/base/src/bidicattable.h
/intl/unicharutil/src/symmtable.h          --> /layout/base/src/symmtable.h

Comment 11

[Simon Montagu :smontagu]

mkaply, can you review this?

Comment 12

[Simon Montagu :smontagu]

Attachment: walk through the changes

This is an attempt to order the changes in a systematic way to make them easier
to digest

Comment 13

[Mike Kaply [:mkaply]]

Comment on attachment 68636 [details]
walk through the changes

Is the FULL_BIDI_ENGINE #ifdef still necessary?

This is GREAT cleanup Simon.

r=mkaply

Comment 14

[Johnny Stenback (:jst)]

I found this while reviewing bug 120682, and I was about to pass out when I saw
it :-) nsIUBidiUtils::Conv_06_FE_WithReverse() takes an in-parameter of type
const nsString by *value*! Can't get much more inefficient than that, pass by
reference, and make it a const ns_A_String reference, not a const nsString
reference. Same for all other nsString's in that interface. Same problem in
other methods in that interface too!

(reporting here in stead of in a new bug per smontagu's request).

Comment 15

[Simon Montagu :smontagu]

On reconsideration, I have filed bug 125635 on Conv_06_FE_WithReverse()
 etc.

Comment 16

[Simon Montagu :smontagu]

... but I will at least change to pass by reference.

Comment 17

[Simon Montagu :smontagu]

The #ifdef FULL_BIDI_ENGINE marks part of the ICU Bidi engine which we don't
use, so are #ifdef-ed out to avoid bloat, rather than just deleting them from
the source. This way if we need them later we will know they are there instead
of reinventing them.

Comment 18

[Marc Attinasi]

Comment on attachment 68636 [details]
walk through the changes

sr=attinasi

Comment 19

[Simon Montagu :smontagu]

Attachment: Full diffs merged to tip

Changed nsCRT::memset to memset and changed interfaces to pass nsString by
reference.

Comment 20

[Simon Montagu :smontagu]

Attachment: macbuild changes

Comment 21

[Simon Montagu :smontagu]

Attachment: Merged to tip again

This includes the macbuild changes, plus the changes of the old
nsFormSubmitter.cpp moved to the new nsFormSubmission.cpp

Comment 22

[Simon Montagu :smontagu]

Attachment: revised macbuild changes

Comment 23

[nhottanscp]

Comment on attachment 70209 [details] [diff] [review]
revised macbuild changes

r=nhotta

nsBidiUtilsImp.cpp/.h are they just removed instead of moved to somewhere?

Comment 24

[Simon Montagu :smontagu]

Parts of nsBidiUtilsImp.cpp/h have morphed into
content/shared/nsBidiUtils.cpp/h, and other parts have moved into
layout/base/nsBidiPresUtils.cpp/h and layout/base/nsBidi.cpp/h

Comment 25

[Simon Montagu :smontagu]

checked in

Comment 26

[Boris Zbarsky [:bzbarsky]]

Testing this on view-source of ynet.co.il, the time spent in bidi continuation
stuff dropped from 13.7% of total time to 9.2%.  :)