Closed
Bug 120818
Opened 23 years ago
Closed 23 years ago
De-COMtaminate Bidi utilities
Categories
(Core :: Layout: Text and Fonts, defect)
Core
Layout: Text and Fonts
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.
Assignee | ||
Comment 1•23 years ago
|
||
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.
Assignee | ||
Comment 2•23 years ago
|
||
Assignee | ||
Comment 3•23 years ago
|
||
Assignee | ||
Comment 4•23 years ago
|
||
Assignee | ||
Comment 5•23 years ago
|
||
Assignee | ||
Comment 6•23 years ago
|
||
Assignee | ||
Comment 7•23 years ago
|
||
Assignee | ||
Comment 8•23 years ago
|
||
Assignee | ||
Comment 9•23 years ago
|
||
Assignee | ||
Comment 10•23 years ago
|
||
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
Assignee | ||
Comment 11•23 years ago
|
||
mkaply, can you review this?
Assignee | ||
Comment 12•23 years ago
|
||
This is an attempt to order the changes in a systematic way to make them easier
to digest
Comment 13•23 years ago
|
||
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+
Comment 14•23 years ago
|
||
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).
Assignee | ||
Comment 15•23 years ago
|
||
On reconsideration, I have filed bug 125635 on Conv_06_FE_WithReverse()
etc.
No longer blocks: 125635
Assignee | ||
Comment 16•23 years ago
|
||
... but I will at least change to pass by reference.
Assignee | ||
Comment 17•23 years ago
|
||
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•23 years ago
|
||
Comment on attachment 68636 [details]
walk through the changes
sr=attinasi
Attachment #68636 -
Flags: superreview+
Assignee | ||
Comment 19•23 years ago
|
||
Changed nsCRT::memset to memset and changed interfaces to pass nsString by
reference.
Attachment #67213 -
Attachment is obsolete: true
Assignee | ||
Comment 20•23 years ago
|
||
Assignee | ||
Comment 21•23 years ago
|
||
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
Assignee | ||
Comment 22•23 years ago
|
||
Attachment #70154 -
Attachment is obsolete: true
Comment 23•23 years ago
|
||
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+
Assignee | ||
Comment 24•23 years ago
|
||
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
Assignee | ||
Updated•23 years ago
|
Status: NEW → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 25•23 years ago
|
||
checked in
Comment 26•23 years ago
|
||
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.
Description
•