Closed Bug 491863 Opened 15 years ago Closed 10 years ago

Remove #ifdef IBMBIDI from layout part

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla31

People

(Reporter: ryoqun, Assigned: ehsan.akhgari)

References

Details

Attachments

(1 file)

User-Agent:       Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.0.10) Gecko/2009042523 Ubuntu/9.04 (jaunty) Firefox/3.0.10
Build Identifier: 

As requested by mw22 on irc, I filed a new bug from bug 89203 comment 52. IBMBIDIs are primarily remaining in the layout code. I don't know how much IBMBIDI currently makes sense. Is it just a remnant from old times? Should we support builds without IBMBIDI? Is there anything else(for example, run-time check by if's or pref) than MACROs? If we can move from IBMBIDIs, it will result in the better code readability and the unification of the codepath.

If this is not too much task for a newbie like me, I can work on this.

Reproducible: Always
In other words, if you're confident the IBMBIDI changes are correct, feel free to remove the ifdefs.
IBMBIDI survives indefinitely? Every big commit should be enclosed in macros? I didn't see another use of macros in the same fashion. Before commiting bug 89203 comment 52, and subsequently filing this bug, I guessed since bug 89203 comment 46 is 6 years old, the code inside IBMBIDI have been well tested and reviewed. And it's time to merge the change. I understand bug 89203 comment 46 is a valid argument. But, we can use mercurial to check the changes before and after the removal of IBMBIDIs, although it's not 'immediately'. For newbies like me, IBMBIDIs are a bit annoying while reading. I think the problem is that we favour the people who know well the history of IBMBIDI by leaving it or newcomers like me by removing it.

From comment #1 and bug 89203 comment 53, I found that bug 89203 comment 46 is still in effective. Thanks, that's what I wanted to know. To be honest, IBMBIDIs were so standing out, and bug 89203 is so old. Then I thought I can make people re-visit it and after getting the assumed favourable approval, I wanted to do this major code cleaning up. So my arguments may be biased over the radical side.

All said, if bug 89203 comment 46 still takes precedence over my opinions, I have no objection to close this bug.
These days, big commits generally have appropriate code review.  The still-#ifdefed parts of IBMBIDI are (in my opinion) to mark the parts that still need review.  I don't think there are too many left, though, and we probably should try to remove them.
I see. Then, I can think IBMBIDIs as one kind of XXXed code. Currently, I'm not able to review those code and I now know that just removing IBMBIDI doesn't solve the problem. But, as I study the codebase, I'll hope I can remove them one by one. Because I'm tackling bug 145503, the understanding of existing BIDI code is very important.

You can leave this bug open until I actually start to review the enclosed code by IBMBIDI or you can close it because I'm not sure how much time is needed for me to become such knowledgeable Mozilla hacker. ;)

Thanks for replying!
David, do you still object to this?  I noted that bug 89203 comment 46 is from 11 years ago.  :-)
Flags: needinfo?(dbaron)
Removing the #ifdef IBMBIDI is fine with me; the ones that I considered suspicious code don't seem to be there anymore.  (Not clear whether that's because the suspicious code was fixed or the #ifdefs were just removed, but it doesn't matter.  We can always dig into suspicious things in the CVS history.)
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(dbaron)
Assignee: nobody → ehsan
Attachment #8411424 - Flags: review?(roc)
https://hg.mozilla.org/mozilla-central/rev/972268fe6a0d
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in before you can comment on or make changes to this bug.