Closed Bug 120818 Opened 23 years ago Closed 23 years ago

De-COMtaminate Bidi utilities

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla0.9.9

People

(Reporter: smontagu, Assigned: smontagu)

References

(Blocks 2 open bugs)

Details

Attachments

(10 files, 3 obsolete files)

(deleted), patch
Details | Diff | Splinter Review
(deleted), patch
Details | Diff | Splinter Review
(deleted), patch
Details | Diff | Splinter Review
(deleted), patch
Details | Diff | Splinter Review
(deleted), patch
Details | Diff | Splinter Review
(deleted), patch
Details | Diff | Splinter Review
(deleted), patch
Details | Diff | Splinter Review
(deleted), text/html
mkaply
: review+
attinasi
: superreview+
Details
(deleted), patch
Details | Diff | Splinter Review
(deleted), patch
nhottanscp
: review+
Details | Diff | Splinter Review
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.
Blocks: deCOM
Target Milestone: --- → mozilla0.9.9
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.
Blocks: 82383
Attached patch Full diffs (very long) (obsolete) (deleted) — Splinter Review
Attached patch genbidicattable.pl (deleted) — Splinter Review
Attached patch gensymmtable.pl (deleted) — Splinter Review
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
mkaply, can you review this?
Attached file walk through the changes (deleted) —
This is an attempt to order the changes in a systematic way to make them easier to digest
Comment on attachment 68636 [details] walk through the changes Is the FULL_BIDI_ENGINE #ifdef still necessary? This is GREAT cleanup Simon. r=mkaply
Attachment #68636 - Flags: review+
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).
Blocks: 125559
Blocks: 125635
On reconsideration, I have filed bug 125635 on Conv_06_FE_WithReverse() etc.
No longer blocks: 125635
... but I will at least change to pass by reference.
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 on attachment 68636 [details] walk through the changes sr=attinasi
Attachment #68636 - Flags: superreview+
Attached patch Full diffs merged to tip (obsolete) (deleted) — Splinter Review
Changed nsCRT::memset to memset and changed interfaces to pass nsString by reference.
Attachment #67213 - Attachment is obsolete: true
Attached patch macbuild changes (obsolete) (deleted) — Splinter Review
Attached patch Merged to tip again (deleted) — Splinter Review
This includes the macbuild changes, plus the changes of the old nsFormSubmitter.cpp moved to the new nsFormSubmission.cpp
Attachment #69742 - Attachment is obsolete: true
Attached patch revised macbuild changes (deleted) — Splinter Review
Attachment #70154 - Attachment is obsolete: true
Comment on attachment 70209 [details] [diff] [review] revised macbuild changes r=nhotta nsBidiUtilsImp.cpp/.h are they just removed instead of moved to somewhere?
Attachment #70209 - Flags: review+
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
Status: NEW → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
checked in
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%. :)
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.

Attachment

General

Created:
Updated:
Size: