Closed
Bug 89203
(ibmbidi)
Opened 23 years ago
Closed 22 years ago
remove --disable-bidi and #ifdef IBMBIDI before it atrophies further
Categories
(Core :: Layout: Text and Fonts, defect)
Core
Layout: Text and Fonts
Tracking
()
VERIFIED
FIXED
People
(Reporter: old-mozilla, Assigned: Biesinger)
References
Details
Attachments
(3 files, 2 obsolete files)
Reporter | ||
Comment 1•23 years ago
|
||
well it builds on RHL 7.1 and seems to work fairly well, so I'm attaching the
first pass at this. Seeking reviews from the (many) modules owners that are
affected, or others that have familiarity with the various pieces.
For the most part this was a very straight forward (boring) change. In addition
to removing the #ifdefs (and a few #ifndefs) any code that would have been
thrown out if IBMBIDI was defined was also removed. Indenting in some pieces was
adjusted to fit the rest of the file after the conditionals were removed.
There were a few locations that I have doubts about which need closer review:
nsBlockFrame.cpp around lines 4368 and 4545
nsTestFrame.cpp around line 3690
nsFormFrame.cpp around line 972
I don't have access to any bidi testcase to see if I broke anything (or the
ability to read any language that runs other than LTR...) so any sanity check
someone can do in this environment (or pointers to testcases with pictures to
compare against) would be a good thing, not because I intentionally changed
anything, but because I could very well have missed a 'n' that made an #ifdef
into a #ifndef... (regexp searches on the patch say I didn't.)
Be warned that the half life of this patch is alarmingly short thanks to it's
size, it's suffered bit rot twice already in the time it took to code it up. :(
Reporter | ||
Comment 2•23 years ago
|
||
Comment 3•23 years ago
|
||
Wow! There's a lot of work in that patch. I can only see one place which needs
fixing, in layout/html/base/src/nsLineLayout.cpp, where you have
-#ifndef IBMBIDI
// XXX what about NS_STYLE_DIRECTION_RTL?
if (NS_STYLE_DIRECTION_RTL == psd->mDirection) {
*aDeltaWidth = 0;
return PR_TRUE;
}
-#endif
where you should have:
-#ifndef IBMBIDI
-// XXX what about NS_STYLE_DIRECTION_RTL?
- if (NS_STYLE_DIRECTION_RTL == psd->mDirection) {
- *aDeltaWidth = 0;
- return PR_TRUE;
- }
-#endif
There is definitely something funny going on in nsTextFrame.cpp around line
3690: it looks like a change went in after BIDI and the #ifdef'd section wasn't
updated accordingly. I'll open a bug for that when I find who to point the
finger at :-)
The places you query in nsBlockFrame.cpp could do with recasting. We often added
#ifdef'd code in such a way as to change the surrounding code as little as
possible, and the results are sometimes inelegant, as here.
nsFormFrame.cpp looks OK to me
What about makefiles? ifdef IBMBIDI appears in the following:
mozilla\editor\base\Makefile.in
mozilla\intl\unicharutil\src\Makefile.in
mozilla\layout\base\public\Makefile.in
mozilla\layout\base\src\Makefile.in
mozilla\layout\html\base\src\Makefile.in
mozilla\editor\base\makefile.win
mozilla\intl\unicharutil\src\makefile.win
mozilla\layout\base\public\makefile.win
mozilla\layout\base\src\makefile.win
mozilla\layout\html\base\src\makefile.win
Reporter | ||
Comment 5•23 years ago
|
||
Reporter | ||
Comment 6•23 years ago
|
||
second draft addresses:
* that ifNdef I missed in nsLineLayout.cpp ... nice eyes Simon.
* -b removed from diff command so this one includes my indention adjustments as
well.
* nsBlockFrame.cpp adjustments
* makefiles
This should catch all the files, I did a more exhaustive set of greps this time.
Updated•23 years ago
|
Status: UNCONFIRMED → NEW
Ever confirmed: true
Reporter | ||
Comment 7•23 years ago
|
||
This patch is looking fairly stable... I'm seeing a couple problems, but I've
just verified that they existed before I did anything so I'm not worried about
them. Adding appropriate keywords, Michael since you own bidi can you take the
first pass review please, probably the same list of folks that needed to review
stuff for it's original integration should be on the list to at least look at
this... about the only thing I'm unsure of is the mac makefile change.
also a note to anyone wanting to test the patch, it doesn't address any files
that are built at build time (i.e. config-defs.h) so you'll want to start from a
very empty build or else you'll still have the symbol defined.
Comment 8•23 years ago
|
||
another thing which occurs to me...
In content/shared/public/nsLayoutAtomList, we originally inserted the bidi
additions in their proper alphabetical place in the various categories (see
attachment 26830 [details] [diff] [review] to bug 70961).
Reviewing that patch, Chris Waterson said:
>In nsLayoutAtomList.h, let's just lump them all into one #ifdef instead of
>trying to preserve alpha order. We can re-sort when/if we remove #ifdefs.
...so you might want to do the resorting now, too.
Reporter | ||
Comment 9•23 years ago
|
||
OK, but what kind of atom is "endsInDiacritic"? frame property? language
specific? other?
Once I get this in I'll attach a new patch that accomidates the changes that
went in before the tree closed as well.
Comment 10•23 years ago
|
||
endsInDiacritic is a frame property.
Reporter | ||
Comment 11•23 years ago
|
||
Reporter | ||
Comment 12•23 years ago
|
||
this patch takes the above comment into account and adjusts for conflicts
integrated since the second patch.
Reporter | ||
Comment 13•23 years ago
|
||
I'm continuing to maintain this patch in my own tree, but at this point it takes
longer to generate a patch than to deal with the bit rot... so unless someone
asks for a new copy I'll not bother to attach it.
Reporter | ||
Comment 14•23 years ago
|
||
adding cc for a couple of folks that seemed to think this was a good idea in the
previous bugs... perhaps they can help drum up some interest?
Comment 15•23 years ago
|
||
I'm not convinced that we can remove all the IBMBIDIs at once and not break
things.
Perhaps it would be better to somehow stage this?
Maybe get rid of --disable-bidi in configure.in, then get rid of it in headers,
than C files.
Theoretically, if IBMBIDI was always on, then the #ifdef pieces could be removed
individually and not break anything.
Reporter | ||
Comment 16•23 years ago
|
||
If you get rid of it in the headers first, then you'll have build failures
because declared methods aren't defined. If you get rid of it in the
implementation files first then you'll have build failures because you're
defining undeclared methods in classes. (see bug 88509 for examples of both)
If you want to "stage" this then you need to take it in logical chunks, a
component at a time, dealing with cross component interface changing issues (I
think there are a couple, depending on how you define "cross component
interfaces") Of course if anyone still has an old configuration laying around
with the #define in it they'll have potential chaos as various components are
staged over. (I suspect this number is small, as normal trees would see the
change to configure and rebuild the config files, if they see the --disable-bidi
directive it will ignore it and they'll just get bidi reactivated.)
OTOH if we just whack it out in one clean cut then anyone with old configs that
disabled bidi will simply have bidi turned on, and probably not notice it.
(summary of the above: I say it's not only easier, but cleaner to do it as one
chunk)
I've been running this on both my boxes since I submitted the patch, nothing out
of the ordinary, several rebuilds including clobber builds and one from scratch
(co, patch, build). I'm still searching for bidi testcases.
Comment 17•23 years ago
|
||
Mass-move all BiDi Hebrew and Arabic qa to me, zach@zachlipton.com.
Thank you Gilad for your service to this component, and best of luck to you
in the future.
Sholom.
QA Contact: giladehven → zach
Comment 18•22 years ago
|
||
*** Bug 179087 has been marked as a duplicate of this bug. ***
Updated•22 years ago
|
Summary: remove --disable-bidi before it atrophies further → remove --disable-bidi and #ifdef IBMBIDI before it atrophies further
Just remove the configure option and force IBMBIDI to be always on. Then run
through layout and remove all the code inside !#ifdef IBMBIDI.
Comment 20•22 years ago
|
||
Yeah, what roc said. Once the non-bidi code is never built, we can remove it in
sane reviewable chunks...
Updated•22 years ago
|
Alias: ibmbidi
Assignee | ||
Comment 21•22 years ago
|
||
so, in case anybody is interested in this, here are the configure.in changes to
always enable bidi
Assignee | ||
Comment 22•22 years ago
|
||
so, tell me - is the idea of the last patch what is wanted? Should I ask for
review of it?
Yes please!
Assignee | ||
Comment 24•22 years ago
|
||
this should be a better patch (the AC_SUBST(IBMBIDI) line later in the file
needs an IBMBIDI=1 line...)
Attachment #109997 -
Attachment is obsolete: true
Assignee | ||
Updated•22 years ago
|
Attachment #109998 -
Flags: review?(seawood)
Comment 25•22 years ago
|
||
Does anyone have any numbers (footprint/performance) of a bidi vs non-bidi
build? Or are we beyond the point of caring about such things? I'm not quite
sure what atrophying is being referenced in the subject as many people still
build with --disable-bidi including the myotonic tinderbox.
As far as I know --disable-bidi gets very little testing. I don't think
Mozilla.org or Netscape ship anything --disable-bidi.
I've never seen any performance issues that could be traced back to IBMBIDI.
Before we decide to keep --disable-bidi I'd like to hear someone who uses it
make a case for why they need it.
Comment 27•22 years ago
|
||
Chris Seawood, reference the bug in my opening comment, 88509 for an example of
the atrophy, basically nobody builds or tests with --disable-bidi, that you
found a tinderbox with that setting amazes me to no end.
At the time this bug was raised, as a result of this comment:
http://bugzilla.mozilla.org/show_bug.cgi?id=86517#c49 people using
--disable-bidi were being broken on average twice a week (working from memory
here, but that's what I recall it feeling like... it was very bad.) by other
folks who were clearly not testing, or even compiling, with --disable-bidi.
My quick look at performance/footprint said that disabling bidi didn't really
save anything meaningfull from an end user perspective, so I simply dropped it
from my configs somewhere around August 2001.
Comment 28•22 years ago
|
||
roc, developers working on the full featured browsers probably aren't going to
ship a product with bidi disabled. However, the code base is used by more than
just the people releasing Mozilla & Netscape. Before we remove the bidi switch,
I'd like to hear that we've at least considered the impact upon embeddors which
would be the ones most likely to use the switch. Since we're talking about
increasing the minimal size of the base configuration (with everthing optional
disabled), I'd like to know what sort of increase we're looking at.
Chris Abbey, the opening of this bug does pre-date myotonic's switch to a
minimal build configuration
(http://bugzilla.mozilla.org/show_bug.cgi?id=18352#c80). How relevant is this
bug still?
Comment 29•22 years ago
|
||
That's a good question. While the bug you reference mentions --disable-bidi, it
isn't purely speaking, an extension... note the list of files in my original
sequence of patches, there isn't a single extension dir in there. My
understanding was that bidi was ifdef'd in purely as a testing statement, and
that it was planned to always be turned on. ( ref:
http://bugzilla.mozilla.org/show_bug.cgi?id=88509#c2 ).
The presence of --disable-bidi in the permutations of configs discussed in
http://bugzilla.mozilla.org/show_bug.cgi?id=18352#c75 would prevent the type of
atrophy/bit-rot that this bug was intending to address. If myotonic is disabling
*everything* then it wont catch bustage such as was in 88509, where another
"extension" had to be enabled for the failure to occur.
/me heads out to inlaws for Christmass... happy holidays all.
Comment 30•22 years ago
|
||
smontagu, haven't you done tests of the footprint win of disabling bidi? I
don't recall your results, unfortunately...
cls, the real issue here is that we already have some layout bugs with
--disable-bidi only and as things stand we will have more and more of those...
Comment 31•22 years ago
|
||
The last time I did a head-to-head footprint comparison of builds with bidi
enabled and disabled was just before the code was checked in, and since then the
bidi footprint has certainly increased in some areas and decreased in others. I
will do it again today.
Comment 32•22 years ago
|
||
Comparing linux builds with --disable-debug, with and without --disable-bidi:
w/o bidi w/ bidi bloat % bloat
-------- ------- ----- -------
libeditor.so 1372160 1376256 4096 0.30
libgkcontent.so 6004736 6029312 24576 0.41
libgklayout.so 3514368 3600384 86016 2.45
Total of dist/bin 52756480 52871168 114688 0.22
Comment 33•22 years ago
|
||
I don't understand why we would offer embedders a way of disabling bidi but not
a way of disabling any other subset of our standards support, e.g. disabling
CSS2 tables or the data: channel or anonymous FTP or whatever.
Comment 34•22 years ago
|
||
embedders can remove data and ftp (although removing just anonymous is probably
not an option)
Comment 35•22 years ago
|
||
You meant "gopher and ftp", right? data: is part of necko, not necko2.
None of which affects hixie's point that this is the _only_ optional layout
feature at the moment, and it's optional through historical accident only, not
because it was planned to be optional....
Comment 36•22 years ago
|
||
not really, necko is modular, an embedder is capable of modifying the make files
(in fact i believe they do).
oh, and one of the people who works for a major mozilla.org (read non aol)
embedder just filed a bug where their build config indeed included disable bidi
-- embedders do care about footprint, and in this case their target audience in
most cases will not include people who need bidi. [the bug itself wasn't related
to that, i just noticed that it was there].
isn't without-xul partially a layout option? (it's also content, but isn't it
paritally layout?)
Comment 37•22 years ago
|
||
Let me phrase this more strongly.
Anyone who wants this to stay in the build has to provide the QA resources to
test Gecko without BIDI. If no-one is ready to provide these resources, then we
should remove it.
Comment 38•22 years ago
|
||
Even if we really want to allow the possibility of disabling support for
bidirectional languages (and I don't concede that we do, I am just making a
hypothetical assumption without prejudice), --disable-bidi is not the right way
to do that. Comment 35 is 100% correct: IBM's work was checked in within #ifdefs
to make it easier to check in gradually and turn on the switch later, and so
that it could be turned off if there were regressions, not so that bidi could be
an optional feature. From the very beginning, the checkin included fixes for
layout bugs not directly related to bidi support.
By the way, I am also not volunteering for the job of going through the sources
and working out exactly what *is* the right way to do it. A first approximation
would be to retain the #ifdef only in files whose name includes "Bidi" and their
callers.
Comment 39•22 years ago
|
||
Comment on attachment 109998 [details] [diff] [review]
better patch
r=cls
Attachment #109998 -
Flags: review?(seawood) → review+
Assignee | ||
Comment 40•22 years ago
|
||
Comment on attachment 109998 [details] [diff] [review]
better patch
Checking in configure.in;
/cvsroot/mozilla/configure.in,v <-- configure.in
new revision: 1.1148; previous revision: 1.1147
done
patch checked in, bidi is always on now
Attachment #109998 -
Attachment is obsolete: true
biesi, if you want to take on the task of removing all the #ifdef IBMBIDIs (and
all the code that is only built when IBMBIDI is not defined), I'd be happy to
give you a blanket r+sr in advance for that cleanup.
Assignee | ||
Comment 42•22 years ago
|
||
roc, thanks for the r+sr - I'll remove the ifdefs.
Assignee: mkaply → cbiesinger
Assignee | ||
Comment 43•22 years ago
|
||
removal of the ifdefs from gfx/ and editor/ done
left: widget, layout, content
Assignee | ||
Comment 44•22 years ago
|
||
widget now finished too
Assignee | ||
Comment 45•22 years ago
|
||
content also finished, only layout left.
Comment 46•22 years ago
|
||
I'd almost prefer that this wasn't done in layout, because I occasionally find
that the changes were wrong, and it's helpful to know immediately when looking
at the code what was part of the BiDi landing, and what the code looked like
before. Perhaps remove as we re-review the code would be a better strategy for
layout?
Comment 47•22 years ago
|
||
I agree with David with respect to places where #ifdef IBMBIDI is scattered
throughout a method (e.g. nsTextFrame::MeasureText or
nsLineLayout::HorizontalAlignFrames) but there should be no objection to
removing it around whole blocks of code, especially where it is clear from
comments or names that they are Bidi-related.
I have already been removing the #ifdefs in cases like that as I touch them in
other bugs.
Assignee | ||
Comment 48•22 years ago
|
||
ok given that the rest of the changes, all in layout, require an actual
understanding, I guess I won't be able to do it.
Who wants to own this? roc? smontagu? dbaron? someone else?
Let's close it. We'll clean up stray ifdefs on the fly where it makes sense to
do so. Thanks!
Status: NEW → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
Comment 51•22 years ago
|
||
*** Bug 91035 has been marked as a duplicate of this bug. ***
Component: Layout: BiDi Hebrew & Arabic → Layout: Text
QA Contact: zach → layout.fonts-and-text
Comment 52•16 years ago
|
||
It's passed almost 6 years from the last comment. What's happening on this? When I'm reading the codebase, IBMBIDI annoys me. If this is not too much task for a newbie, I can work on this. First of all, do I need to consider that IBMBIDI isn't defined, in this age(yes, I use the word 'age'!)? Or, I can just remove #ifdef IBMBIDIs?
Comment 53•16 years ago
|
||
Ryo, see comment 46.
Comment 54•16 years ago
|
||
(In reply to comment #53)
> Ryo, see comment 46.
Thanks for replying. Well, I have filed a follow-up bug of this at bug 491863. I posted my opinions there(bug 491863 comment 3). Please read it. And from now, we should discuss about this there.
You need to log in
before you can comment on or make changes to this bug.
Description
•