Last Comment Bug 215963 - clean up nsJISx4501LineBreaker (sic)
: clean up nsJISx4501LineBreaker (sic)
Status: RESOLVED FIXED
[patch]
:
Product: Core
Classification: Components
Component: Internationalization (show other bugs)
: Trunk
: All All
: P2 normal with 1 vote (vote)
: mozilla1.6alpha
Assigned To: David Baron :dbaron: ⌚️UTC-7
:
:
Mentors:
Depends on:
Blocks: line-breaking
  Show dependency treegraph
 
Reported: 2003-08-12 11:53 PDT by David Baron :dbaron: ⌚️UTC-7
Modified: 2012-03-08 12:18 PST (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (13.01 KB, patch)
2003-08-12 11:55 PDT, David Baron :dbaron: ⌚️UTC-7
no flags Details | Diff | Splinter Review
patch (diff -uw, for review, i.e., without whitespace changes) (11.76 KB, patch)
2003-08-12 11:57 PDT, David Baron :dbaron: ⌚️UTC-7
no flags Details | Diff | Splinter Review
fix spelling mistakes (30.67 KB, patch)
2003-08-13 22:43 PDT, David Baron :dbaron: ⌚️UTC-7
jshin1987: review+
bzbarsky: superreview+
Details | Diff | Splinter Review
patch (diff -uw, for review, i.e., without whitespace changes) (12.30 KB, patch)
2003-09-16 20:46 PDT, David Baron :dbaron: ⌚️UTC-7
jshin1987: review+
bzbarsky: superreview+
Details | Diff | Splinter Review

Description David Baron :dbaron: ⌚️UTC-7 2003-08-12 11:53:57 PDT
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.
Comment 1 David Baron :dbaron: ⌚️UTC-7 2003-08-12 11:55:50 PDT
Created attachment 129675 [details] [diff] [review]
patch
Comment 2 David Baron :dbaron: ⌚️UTC-7 2003-08-12 11:57:47 PDT
Created attachment 129676 [details] [diff] [review]
patch (diff -uw, for review, i.e., without whitespace changes)
Comment 3 David Baron :dbaron: ⌚️UTC-7 2003-08-12 12:25:28 PDT
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).
Comment 4 David Baron :dbaron: ⌚️UTC-7 2003-08-13 18:31:11 PDT
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).
Comment 5 David Baron :dbaron: ⌚️UTC-7 2003-08-13 18:37:07 PDT
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).
Comment 6 Jungshik Shin 2003-08-13 19:44:11 PDT
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. 
Comment 7 David Baron :dbaron: ⌚️UTC-7 2003-08-13 22:43:47 PDT
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.
Comment 8 David Baron :dbaron: ⌚️UTC-7 2003-08-13 22:57:32 PDT
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 Jungshik Shin 2003-08-13 23:08:28 PDT
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 Jungshik Shin 2003-08-14 19:15:45 PDT
Comment on attachment 129783 [details] [diff] [review]
fix spelling mistakes

r=jshin
Comment 11 Boris Zbarsky [:bz] (still a bit busy) 2003-09-16 18:21:01 PDT
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.
Comment 12 David Baron :dbaron: ⌚️UTC-7 2003-09-16 20:13:10 PDT
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.
Comment 13 David Baron :dbaron: ⌚️UTC-7 2003-09-16 20:46:27 PDT
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.
Comment 14 Boris Zbarsky [:bz] (still a bit busy) 2003-09-17 07:26:10 PDT
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...
Comment 15 Jungshik Shin 2003-09-23 00:27:20 PDT
Comment on attachment 131581 [details] [diff] [review]
patch (diff -uw, for review, i.e., without whitespace changes)

r=jshin
Comment 16 David Baron :dbaron: ⌚️UTC-7 2003-09-23 00:31:59 PDT
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.
Comment 17 Masayuki Nakano [:masayuki] (Mozilla Japan) 2008-02-05 13:00:08 PST
dbaron:

Why is this bug still opened?
Comment 18 Atul Aggarwal 2012-03-08 12:18:07 PST
Closing this bug. Please feel free to open if something else is required.

Note You need to log in before you can comment on or make changes to this bug.