Note: There are a few cases of duplicates in user autocompletion which are being worked on.

clean up nsJISx4501LineBreaker (sic)

RESOLVED FIXED in mozilla1.6alpha

Status

()

Core
Internationalization
P2
normal
RESOLVED FIXED
14 years ago
5 years ago

People

(Reporter: dbaron, Assigned: dbaron)

Tracking

(Blocks: 1 bug)

Trunk
mozilla1.6alpha
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [patch])

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

14 years ago
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.
(Assignee)

Updated

14 years ago
Status: NEW → ASSIGNED
Priority: -- → P2
Whiteboard: [patch]
Target Milestone: --- → mozilla1.6alpha
(Assignee)

Comment 1

14 years ago
Created attachment 129675 [details] [diff] [review]
patch
(Assignee)

Comment 2

14 years ago
Created attachment 129676 [details] [diff] [review]
patch (diff -uw, for review, i.e., without whitespace changes)
(Assignee)

Comment 3

14 years ago
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).
(Assignee)

Comment 4

14 years ago
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).
(Assignee)

Comment 5

14 years ago
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)

Comment 6

14 years ago
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. 

Updated

14 years ago
Depends on: 206152
(Assignee)

Comment 7

14 years ago
Created attachment 129783 [details] [diff] [review]
fix spelling mistakes

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.
(Assignee)

Updated

14 years ago
Attachment #129783 - Flags: review?(jshin)
(Assignee)

Comment 8

14 years ago
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...

Comment 9

14 years ago
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 10

14 years ago
Comment on attachment 129783 [details] [diff] [review]
fix spelling mistakes

r=jshin
Attachment #129783 - Flags: review?(jshin) → review+

Updated

14 years ago
Blocks: 206152
No longer depends on: 206152
(Assignee)

Updated

14 years ago
Attachment #129783 - Flags: superreview?(bz-vacation)

Comment 11

14 years ago
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+
(Assignee)

Comment 12

14 years ago
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.
(Assignee)

Comment 13

14 years ago
Created attachment 131581 [details] [diff] [review]
patch (diff -uw, for review, i.e., without whitespace changes)

This is the previous real patch, merged with the spelling fixes, and with the
changes suggested in comment 3, comment 11, and comment 12.
(Assignee)

Updated

14 years ago
Attachment #129675 - Attachment is obsolete: true
Attachment #129676 - Attachment is obsolete: true
(Assignee)

Updated

14 years ago
Attachment #131581 - Flags: superreview?(bz-vacation)
Attachment #131581 - Flags: review?(jshin)

Comment 14

14 years ago
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 15

14 years ago
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+
(Assignee)

Comment 16

14 years ago
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

Comment 18

5 years ago
Closing this bug. Please feel free to open if something else is required.
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.