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)

Product:
Core
Component:
Layout: Text and Fonts
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED

People

(Reporter: old-mozilla, Assigned: Biesinger)

References

Details

Attachments

(3 files, 2 obsolete files)

first pass patch (bzip'd due to size)
31.25 KB, application/bzip2
Details
second draft patch
34.64 KB, application/bzip
Details
third turn of the crank
34.44 KB, application/bzip2
Details
seecommentary in bug 88509 and bug 86517; (very large) patch forthcoming... build in progress.
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. :(
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
Opened bug 89253 for the nsTextFrame.cpp question
Attached file second draft patch —
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
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.
Keywords: patch, review
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.
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.
endsInDiacritic is a frame property.
this patch takes the above comment into account and adjusts for conflicts integrated since the second patch.
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.
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?
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.
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.
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
*** Bug 179087 has been marked as a duplicate of this bug. ***
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.
Yeah, what roc said. Once the non-bidi code is never built, we can remove it in sane reviewable chunks...
Alias: ibmbidi
so, in case anybody is interested in this, here are the configure.in changes to always enable bidi
so, tell me - is the idea of the last patch what is wanted? Should I ask for review of it?
Attached patch better patch (obsolete) — — Splinter Review
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
Attachment #109998 - Flags: review?(seawood)
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.
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.
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?
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.
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...
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.
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
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.
embedders can remove data and ftp (although removing just anonymous is probably not an option)
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....
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?)
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.
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 on attachment 109998 [details] [diff] [review] better patch r=cls
Attachment #109998 - Flags: review?(seawood) → review+
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.
roc, thanks for the r+sr - I'll remove the ifdefs.
Assignee: mkaply → cbiesinger
removal of the ifdefs from gfx/ and editor/ done left: widget, layout, content
widget now finished too
content also finished, only layout left.
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?
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.
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
Yes, I was going to suggest the same thing.
Status: RESOLVED → VERIFIED
*** 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
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?
(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.

Attachment

General

Creator:
Created:
Updated:
Size: