Closed Bug 215963 Opened 17 years ago Closed 8 years ago

clean up nsJISx4501LineBreaker (sic)

Categories

(Core :: Internationalization, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla1.6alpha

People

(Reporter: dbaron, Assigned: dbaron)

References

(Blocks 1 open bug)

Details

(Whiteboard: [patch])

Attachments

(2 files, 2 obsolete files)

I was looking at nsJISx4501LineBreaker, and it's a bit of a mess.  There are a
bunch of loops that have the first iteration of the loop manually pulled out.  I
have a patch to fix those, and fix the places where tabs are used in the file,
and a few other little bits of cleanup.

It probably needs a bit more testing than I've given it, and it's also worth
double-checking that the stuff I pushed back into the loop really is equivalent.
Status: NEW → ASSIGNED
Priority: -- → P2
Whiteboard: [patch]
Target Milestone: --- → mozilla1.6alpha
Attached patch patch (obsolete) — Splinter Review
Actually, now I see that my change to nsJISx4501LineBreaker::Prev wasn't quite
keeping things the same (though it would be nice to know if the difference in
|oPrev| output was intentional).
Actually, I just realized that the standard is really "JIS X 4051", so the
changes to comments should not be there (and there should probably be others in
the reverse direction).
And while I'm on the topic, the parameters to the constructor are unused, and
should probably be eliminated, along with the whole concept of a line breaker
factory (which exists only to pass those parameters, I think).
Summary: clean up nsJISx4501LineBreaker → clean up nsJISx4501LineBreaker (sic)
While you're at it, you may also consider renaming *4501* to *4051* risking the
loss of the file change history. 

BTW, the parameters to the constructor arre not used now, but when
language/script-dependent line breakers are implemented, they'd be of use.  See
bug 203016. 
Depends on: line-breaking
The files can be copied in the cvs repository in order to rename them.

These are the changes that can happen without renaming files, though.
I've also been wondering whether we should rewrite the existing linebreaking
code based on UAX #14.  Doing so would handle a lot of cases better -- for
example, hyphens (which are currently handled differently for CJK and non-CJK,
and I think wrong in both cases).  Doing this would mean that the nsTextFrame
ASCII versions would have to call the linebreaker rather than considering only
spaces.  But I should probably file another bug about that...
I'll take a look at the patch.

As for UAX #14, see bug 56652 comment #18. See also bugs listed in bug 206152
for other line-breaker issues. There may be other bugs I didn't find. 
Comment on attachment 129783 [details] [diff] [review]
fix spelling mistakes

r=jshin
Attachment #129783 - Flags: review?(jshin) → review+
No longer depends on: line-breaking
Attachment #129783 - Flags: superreview?(bz-vacation)
Comment on attachment 129783 [details] [diff] [review]
fix spelling mistakes

> nsLWBreakerFImp::GetBreaker

Could we change the code at the end of this function to not use 'NULL' and to
use NS_ADDREF instead of calling AddRef() manually?

sr=bzbarsky with that.
Attachment #129783 - Flags: superreview?(bz-vacation) → superreview+
I'm actually going to put that in the next patch (actually get rid of the null
checks entirely, I think), since I'd rather not check in substantive changes
with spelling corrections.
This is the previous real patch, merged with the spelling fixes, and with the
changes suggested in comment 3, comment 11, and comment 12.
Attachment #129675 - Attachment is obsolete: true
Attachment #129676 - Attachment is obsolete: true
Attachment #131581 - Flags: superreview?(bz-vacation)
Attachment #131581 - Flags: review?(jshin)
Comment on attachment 131581 [details] [diff] [review]
patch (diff -uw, for review, i.e., without whitespace changes)

sr=bzbarsky, but yes, the oPrev thing looks wrong...
Attachment #131581 - Flags: superreview?(bz-vacation) → superreview+
Comment on attachment 131581 [details] [diff] [review]
patch (diff -uw, for review, i.e., without whitespace changes)

r=jshin
Attachment #131581 - Flags: review?(jshin) → review+
Comment on attachment 131581 [details] [diff] [review]
patch (diff -uw, for review, i.e., without whitespace changes)

Patch checked in to trunk, 2003-09-23 00:30 -0700.
dbaron:

Why is this bug still opened?
QA Contact: amyy → i18n
Closing this bug. Please feel free to open if something else is required.
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.