User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.2b) Gecko/20021008 Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.2b) Gecko/20021008 This is a spin-off of bug 167136. Currently, there are several places in layout[1], gfx[2] and intl [3] modules where the tables for character classes are hard-coded (here character class can be either a Unicode/ISO 10646 defined character class like Mn, Mc, and Lo or a Mozilla-specific class like 'being allowed to have a blank glyph) or a set of macros are used to identify chars. of a certain class. From these hard-coded tables, gfx module builds CCMaps. Layout just uses binary search of these tables to identify characters belonging to classes represented in those tables. Intl module([3]) currently doesn't use any bsearch of char. class table. Instead, it relies on a set of macros to identify which class a char. belong to. In gfx module, precompiled CCMaps (PRUint16 arrays) to be generated with ccmapbin.pl (attachment 106178 [details]) in advance and to be put in source files can reduce the start-up time and the memory foot print. In layout, hard-coding precompiled CCMaps would speed up the look-up time (bsearch is replaced by a supposedly faster bitwise operations and pointer-arithmetics). In intl module[3], it could speed up the look-up time but it needs some further investigation about sizes of classes (the number of characters belonging to each class to be identified) to determine whether using precompiled CCMaps is better than the current approach (using a set of macros to figure out whether a given char belong to a class). Besides, the current implementation of linebraking is based on JIS X 4501, but I think it has to be updated to implement UTR 14[4]. Given this status of line breaking implementation in Mozilla, I'd rather leave it out from the list of places to be dealt in in this bug. Perhaps, as an alert that we need to update line braking algorithm, I'll file a new bug for this item. When it's determined how to implement UTR #14, we can decide whether or not the approach to be taken for gfx and layout module is suitable for line breaking algorithm or not(I think for most classes, the answer is positive.) In conclusion, I propose that we put precompiled CCMaps into gfx[1] and layout[2] modules and consider adopting the approarch in intl module [3] later. [1] CJK special char. handling http://lxr.mozilla.org/seamonkey/source/gfx/src/gtk/nsFontMetricsGTK.cpp http://lxr.mozilla.org/seamonkey/source/gfx/src/xlib/nsFontMetricsXlib.cpp Blank glyph handling http://lxr.mozilla.org/seamonkey/source/gfx/src/windows/nsFontMetricsWin.cpp [2] punctuation checking in nsTextFrame is a sure thing: http://lxr.mozilla.org/seamonkey/source/layout/html/base/src/nsTextFrame.cpp#4645 [3] CJK and hangul check in linebreaker is questionable, http://lxr.mozilla.org/seamonkey/source/intl/lwbrk/src/nsJISx4501LineBreaker.cpp [4] Unicode line breaking algorithm UTR #14 http://www.unicode.org/unicode/reports/tr14/ (note that the treatment of Hangul leading consonants in this TR is errorneous and the author is aware of the problem and will fix it sometime soon.) DUTR #29(text boundaries: http://www.unicode.org/unicode/reports/tr29) also has to be refered to. Reproducible: Always Steps to Reproduce: 'To reproduce' this problem is fuzzy at most for this bug. We have to implement the approach I described (originally suggested by Shanjian) and compare the memory foot print, the start-up time, and the performance of Mozilla before and after this change.

Adding Shanjian to CC and 'intl' keyword. I'm not sure of 'the component' designation for this bug. If layout people think it needs to be changed, please let me know. I also tried to reassign this to myself, but Mozilla didn't let me even though I'm suppsed to be able to because I'm the submitter/reporter of this bug. I'll check out Bugzilla update regression bug for this problem. In the mean time, could anyone with the right previlige please assign it to me. Thank you.

Bidi code already does something similar to this, but using a compressed lookup table rather than a binary search. See http://lxr.mozilla.org/seamonkey/source/layout/base/src/bidicattable.h, which is generated from the Unicode character database by http://lxr.mozilla.org/seamonkey/source/layout/tools/genbidicattable.pl. I adapted that idea from http://lxr.mozilla.org/seamonkey/source/intl/unicharutil/tools/gencattable.pl, but we don't seem to use the tables generated there.

I'm wondering where to put this. I'd appreciate any suggestion.

gfx/src/x11shared/dbyte_special_chars.32be gfx/src/x11shared/dbyte_special_chars.64be gfx/src/x11shared/dbyte_special_chars.le Includes the following 6 files: gfx/src/windows/blank_glyph.32be gfx/src/windows/blank_glyph.64be gfx/src/windows/blank_glyph.le layout/html/base/src/punct_marks.32be layout/html/base/src/punct_marks.64be layout/html/base/src/punct_marks.le Could I get r and sr for this and attachment 106447 [details] [diff] [review] and 106449?

Confirming bug. Junshik, you may want to request the reviews from specific people: see <URL:http://bugzilla.mozilla.org/flag-help.html> for how to make the requests in the new Bugzilla setup.

Comment on attachment 106449 [details] a new version of ccmapbin.pl to generate precompiled CCMaps. Asking for review. Where should I put this script if you think it has to go in?

Comment on attachment 106447 [details] [diff] [review] a patch (for layout -punctuation marks and gfx - blank glyphs and cjkspecials) looks good. sr may ask you to replace thing like "pagechar++" with "++pagechar".

jshin, I am unable to review perl code. How about roland?

Comment on attachment 106449 [details] a new version of ccmapbin.pl to generate precompiled CCMaps. Shanjian, thank you for review. (I'll replace post ++ with pre ++ although for built-in types, I guess it doesn't matter much). Roland or Simon, can you review the perl script?

Comment on attachment 106449 [details] a new version of ccmapbin.pl to generate precompiled CCMaps. Simon, while reviewing it, can you also think about where to put this ? It's used in two places so that I think it can go in either places (tools directory)

Sorry, the patch is beyond my very basic knowledge of Perl, so I will have to pass on reviewing it. As for where to put the script, I suggest intl/unicharutil/tools.

This is purely a code-level review, someone else will have to check this code actually does what is expected. However, purely from the perspective of a timeless-level code review, I have 3 comments: 1. Please "use strict". 2. The following code: /^CLASS::/ and ($comments_p->{'CLASS'} = $_) =~ s/CLASS::\s*([a-zA-Z0-9_]+).*$/$1/, next; /^DESCRIPTION::/ and ($comments_p->{'DESC'} = $_) =~ s/DESCRIPTION::\s*//, next; /^FILE::/ and ($comments_p->{'FILE'} = $_) =~ s/FILE::\s*//, next; ...should have ^ in both the m// case and the s/// case otherwise you could perform unexpected changes. 3. The following regexp: s!(?:/\*|//|:)?\s*([^*]+)\s*(?:\*/)?!$1!; ...should use /x and be thoroughly and usefully COMMENTED. As it is it looks like line noise and gives Perl a bad name.

Modified per Ian's comment. Also following Simon's suggestion, I specified where this script would go in in the script. As for testing, see bug 167136 comment #13. Diffing between files gen. by a new version and those by old one yielded NULL result(other than comment change), which means that the new one works as well as the old one.

Nothing significant has changed. Just changed the command line syntax to 'ccmapbin.pl input_file_name [class_name]'. BTW, this is not a part of the build.

attachment 106451 [details] contains 8 other map files like this one (by mistake, I included a bunch of unrelated files there). Because they're all like this one and a bulk of files are xPL , I'm uploading this one (gfx/src/x11shared/dbyte_special_chars.32be) as a sample for review if necessary.

Comment on attachment 109451 [details] a sample precompiled map file (gfx/src/x11shared/dbyte_special_chars.32be) Shanjian, I'm not sure if review is necessary. If not, I'm sorry. Anyway, can you just grant one? This has been tested well as I wrote in bug 167136 comment #13.

I won't be able to get to this for at least a week, more likely 2-3; I have a number of other review requests in my pipe and I'm on vacation, so my computer time is very limited.

Comment on attachment 109451 [details] a sample precompiled map file (gfx/src/x11shared/dbyte_special_chars.32be) Asking Chris for sr, instead (as Boris is on vacation). Chris, can you also super-review attachment 101447 [details] as well (since alecf is also on vacation?) ?

Comment on attachment 106447 [details] [diff] [review] a patch (for layout -punctuation marks and gfx - blank glyphs and cjkspecials) sr=alecf

Comment on attachment 109451 [details] a sample precompiled map file (gfx/src/x11shared/dbyte_special_chars.32be) blizzard seems busy, asking alecf for sr, instead.

Comment on attachment 109451 [details] a sample precompiled map file (gfx/src/x11shared/dbyte_special_chars.32be) sr=alecf on all the generated files..

I found that nsFontMetricsXlib.cpp had changed a little(in such a way to affect my patch ) recently. The change is not so significant but I thought I'd better ask for quick r/sr before asking for 'a' to 1.3b.

Comment on attachment 112416 [details] [diff] [review] basically the same patch as attachment 106447 [details] [diff] [review] Shanjian and Alecf, can you take a quick look and transfer r/sr to this patch (adapted to a little change in nsFontMetricsXlib.cpp since my prev. patch) so that I can ask for a to 1.3b. Thanks

From attachment 112416 [details] [diff] [review]: > @@ -1026,7 +1006,6 @@ > } > FreeCCMap(mUserDefinedCCMap); > FreeCCMap(mEmptyCCMap); > - FreeCCMap(mDoubleByteSpecialCharsCCMap); > > /* Free memory allocated by |CopyFontCharSetMapXlib()| */ > if (mCharSetMap) { > @@ -1179,14 +1158,7 @@ > if (NS_SUCCEEDED(rv)) > mAllowDoubleByteSpecialChars = val; > > - // setup the double byte font special chars glyph map > - nsCompressedCharMap specialchars_ccmapObj; > - for (int i=0; gDoubleByteSpecialChars[i]; i++) { > - specialchars_ccmapObj.SetChar(gDoubleByteSpecialChars[i]); > - } > - mDoubleByteSpecialCharsCCMap = specialchars_ccmapObj.NewCCMap(); > - if (!mDoubleByteSpecialCharsCCMap) > - return NS_ERROR_OUT_OF_MEMORY; > + mDoubleByteSpecialCharsCCMap = gDoubleByteSpecialCharsCCMap; |gDoubleByteSpecialCharsCCMap| is read-write by the code below - which means that either the code below should only write to it once (e.g. init it the first time it is being used) or creates a copy(=|mDoubleByteSpecialCharsCCMap|) and then writes to the copy (|gDoubleByteSpecialCharsCCMap| could then be |const| again). The reason for the architecture change in nsFontMetricsXlib.cpp (which will be ported to nsFontMetricsGTK.cpp soon) is to support multiple independent devices properly (without trying to do any "smart" sharing of data between devices (e.g I perfer to copy any global variables which are r/w), I did not implement that for now to avoid to end in the same hell as the gfx/src/windows/ is currently "in"). The current patch may work, but it should at least protect itself from initing |gDoubleByteSpecialCharsCCMap| multiple times (this applies to both nsFontMetricsXlib.cpp and nsFontMetricsGTK.cpp) or (my preferred solution, but it is not neccesary for now until some more dynamic font handling code lands) copy the ccmap data to |mDoubleByteSpecialCharsCCMap| and then modify it (and do not forget to free |mDoubleByteSpecialCharsCCMap| at the end of the devicecontext lifetime). > PRInt32 scale_minimum = 0; > rv = mPref->GetIntPref("font.scale.outline.min", &scale_minimum); > @@ -2594,8 +2566,15 @@ > if ((aSelf->Convert == DoubleByteConvert) > && (!aFmctx->mAllowDoubleByteSpecialChars)) { > PRUint16* ccmap = aSelf->mCCMap; > - for (int i=0; gDoubleByteSpecialChars[i]; i++) { > - CCMAP_UNSET_CHAR(ccmap, gDoubleByteSpecialChars[i]); > + PRUint16 page = CCMAP_BEGIN_AT_START_OF_MAP; > + PRUint16* specialmap = gDoubleByteSpecialCharsCCMap; Shouldn't this be |mDoubleByteSpecialCharsCCMap| (e.g. s/gDoubleByteSpecialCharsCCMap/mDoubleByteSpecialCharsCCMap/ ? > + while (NextNonEmptyCCMapPage(specialmap, &page)) { > + PRUint32 pagechar = page; > + for (int i=0; i < CCMAP_BITS_PER_PAGE; i++) { > + if (CCMAP_HAS_CHAR(specialmap, pagechar)) > + CCMAP_UNSET_CHAR(ccmap, pagechar); > + pagechar++; > + } Patch looks OK so far (except the nits listed above) ... :)

> it should at least protect itself from initing > |gDoubleByteSpecialCharsCCMap| multiple times (this applies to both > nsFontMetricsXlib.cpp and nsFontMetricsGTK.cpp) With my patch, it's not init'd multiple times at run-time. It's just compiled in at *compile-time*. Even without my patch, it's not init'd multiple times but just once when a device-context is created. (of course, as it is now, it'd be init'd multiple times if multiple device-context's are created.) > |gDoubleByteSpecialCharsCCMap| is read-write by the code below Actually, it's read-only in the code below. It's not |specialmap| (gDoubleByteSpecialCharsCCMap) but |ccmap|(|aSelf->mCCMap|) that is written to according to the constant read-only values stored in |specialmap|(|gDoubleByteSpecialCharsCCMap|) (perhaps, I should have just removed |specialmap| and used |gDoubleByteSpecialCharsCCMap| in its place) > PRUint16* ccmap = aSelf->mCCMap; > + PRUint16 page = CCMAP_BEGIN_AT_START_OF_MAP; > + PRUint16* specialmap = gDoubleByteSpecialCharsCCMap; ...... > + if (CCMAP_HAS_CHAR(specialmap, pagechar)) > + CCMAP_UNSET_CHAR(ccmap, pagechar); However, |ccmap|(|aSelf->mCCMap|) above would be pointing to |gDoubleByteSpecialCharsCCMap| if the static function |SetUpFontCharSetInfo|(which the above code belongs to ) were invoked 'upon' |sub_font| defined as below, which I think isn't happening. 5296 // since they are very oversized compared to western fonts 5297 nsFontXlib* sub_font = FindSubstituteFont(aChar); 5298 NS_ASSERTION(sub_font, "failed to get a special chars substitute font"); 5299 if (sub_font) { 5300 sub_font->mCCMap = mFontMetricsContext->mDoubleByteSpecialCharsCCMap; 5301 AddToLoadedFontsList(sub_font); 5302 } > Shouldn't this be |mDoubleByteSpecialCharsCCMap| (e.g. > s/gDoubleByteSpecialCharsCCMap/mDoubleByteSpecialCharsCCMap/ ? I could have used |aFmctx->mDoubleByteSpecialCharsCCMap| in place of |gDoubleByteSpecialCharsCCMap|, but there's no point of doing that because |mDoubleByteSpecialCharsCCMap| of any |nsFontMetricsXlibContext| is just a pointer to |gDoubleByteSpecialCharsCCMap|. Please, note that we would be able to dispense with |mDoubleByteSpecialCharsCCMap| if it were not used in line 5296 - 5301(quoted above) because unlike |mEmptyCCMap| and |mUserDefinedCCMap|, DoubleByteSpecialChar list is not device/font-specific but truly a global constant. However, because it can be pointed to by |mCCMap| of |nsFontXlib| (as in line 5296-5301) which could be modified at run-time(although it isn't in the current code and I guess it's a really remote possibility), I'm going to upload a new patch which copies |gDoubleByteSpecialCharsCCMap| to |mDoubleByteSpecialCharsCCMap|, which is your preferred solution.

Comment on attachment 112585 [details] [diff] [review] a new patch per Roland's comment Jungshik Shin wrote: > I could have used |aFmctx->mDoubleByteSpecialCharsCCMap| in > place of |gDoubleByteSpecialCharsCCMap|, but there's no point > of doing that because |mDoubleByteSpecialCharsCCMap| of > any |nsFontMetricsXlibContext| is just a pointer to > |gDoubleByteSpecialCharsCCMap|. Erm, but your new patch makes now a copy from it, so |aFmctx->mDoubleByteSpecialCharsCCMap|!=|gDoubleByteSpecialCharsCCMap| ... ... and if you copy it then the original can be |const| (which is good for the footprint) I'll file a adjusted patch in a few secs...

Changes from attachment 112585 [details] [diff] [review]: - Adjusted gfx/src/xlib/nsFontMetricsXlib.cpp and made |gDoubleByteSpecialCharsCCMap| |const| and fixed the nit that |SetUpFontCharSetInfo| should use |aFmctx->mDoubleByteSpecialCharsCCMap| instead of the read-only (and now |const| :) |gDoubleByteSpecialCharsCCMap| ... Note that this patch includes the already-reviewed/superreviewed precompiled map files from attachment 106451 [details] (e.g. mozilla/layout/html/base/src/punct_marks.32be mozilla/layout/html/base/src/punct_marks.64be mozilla/layout/html/base/src/punct_marks.le mozilla/gfx/src/windows/blank_glyph.32be mozilla/gfx/src/windows/blank_glyph.64be mozilla/gfx/src/windows/blank_glyph.le mozilla/gfx/src/x11shared/dbyte_special_chars.32be mozilla/gfx/src/x11shared/dbyte_special_chars.64be mozilla/gfx/src/x11shared/dbyte_special_chars.le ; I've included them that testers can simple drop-in the patch... :)

Besides Roland's changes made in attachement 112945, the following has changed in my local copy. - return NS_ERROR_OUT_OF_MEMORY; + PRUint32 dbmapSize = sizeof(gDoubleByteSpecialCharsCCMap); + mDoubleByteSpecialCharsCCMap = (PRUint16*)PR_Malloc(dbmapSize); + if (!mDoubleByteSpecialCharsCCMap) + return NS_ERROR_OUT_OF_MEMORY; + memcpy(mDoubleByteSpecialCharsCCMap,gDoubleByteSpecialCharsCCMap, dbmapSize); Just to 'preemptively' defend the patch, I got rid of tabs haphazardly left in the above in my local copy. In addition to gDoubleByteSpecialCharsCCMap(in ns...Xlib), I've added 'const' to three other pre-compiled CCMaps in the patch

Suggestion is passing. What about just having a single file static PRUint16 gPuncCharsCCMap[] = { #include "punct_marks.cmap" } and move the #ifef in the .cmap file itself.

Comment on attachment 112416 [details] [diff] [review] basically the same patch as attachment 106447 [details] [diff] [review] clearing review requests to remove them from queues

Comment on attachment 112945 [details] [diff] [review] New patch for 2003-01-20-08-trunk per previous comments asking shanjian and alecf for rewiew. As for rbs' comment, I don't have any strong opinion, but it seems to me that what I did is more in line with 'mozilla convention' of including autogenerated files.

Comment on attachment 112945 [details] [diff] [review] New patch for 2003-01-20-08-trunk per previous comments minor nits: >+static PRUint16 gPuncCharsCCMap[] = can you make this const? >+static PRUint16 gDoubleByteSpecialCharsCCMap[] = { same here >+// reaching here. Needs further investigation. >+static PRUint16 gCharsWithBlankGlyphCCMap[] = { and so forth.. look for all instances of this.. unless.. do the UNSET_CHAR stuff modify the table? you don't need to include the giant character tables with your next patch..I've seen them once now, and they make the patch hard to read..

I should have uploaded my new patch before asking for r/sr because alecf's concerns mostly had been addressed in my local copy(see comment #30). BTW, I had to cast away const in nsFontMetricsGTK.cpp to get it compiled by VC++ (IIRC, that's why I removed |const| in my original patch). It seems like the first argument for |NextNonEmptyCCMapPage| (in nsCompressedCharMap.h) cannot be declared const. Another BTW, I had to add 'uconv' dependency to layout/html/base/src/Makefile.in (it wasn't that way...)

It seems foggy to me to litter the tree with those numerous #if #else when a single clean #include could hide the inner details that nobody really cares about. You could make that single file be the one that is auto-generated if you prefer.

consolidated three incldue files into one as suggested by rbs.

Comment on attachment 115956 [details] [diff] [review] a new patch(streamlined #include) I also changed |NextNonEmptyCCMapPage| to make its first argument const.

Comment on attachment 115956 [details] [diff] [review] a new patch(streamlined #include) Several chunks of diffs from another patch crept into this I'm sorry, but please ignore diff. to nsFontMetricsWin.cpp beginning with @@ -2007,6 +2017,50 @@ and diff to nsFontMetricsWin.h

Might be easier & more expedient for reviewers if you just attach a new patch.

all right. I wanted to reduce bugzilla-mail traffic, but ended up incurring two more...

Thanks. Isn't that cleaner? My r= is ok if you need it to complete the reviews.

Junkshik, I took a brief look and the patch looks fine. FYI, I moved to another project and that keeps me very busy. I no longer took ownership of any mozilla modules. Rbs's review should be good enough for you to go ahead.

Comment on attachment 116064 [details] [diff] [review] same as above with a mix-up from another patch removed looks good. thanks for the const work. the more I think about this, at some point I think it would be nice to consolidate some of these tables into one DLL and make a single service that you could query about membership in the tables. in any case sr=alecf on this stuff.

Thank you all. Fix checked-in.

I'm sorry my patch was backed out because gcc on comet doesn't like sub_font->mCCMap = gDoubleByteSpecialCharsCCMap; when |gDou...| is const. I overlooked it because gcc on my local machine 'condoned' and allowed it (with warning). It seems like I can safely cast away |const| for sub_font, but not 100% sure. (see my comment #26 on nsFontMetricsXlib.cpp) The best seems to be to port what's done in nsFontMetricsXlib.cpp (by Roland) for multiple device context support to nsFontMetricsGTK.cpp, but I have no idea when that porting is planned. In the meantime, I guess I have to remove |const| for gDoubleByteSpecialCharsCCMap in nsFontMetricsGTK.cpp (in other places, it's all right.)

Jungshik Shin wrote: > I'm sorry my patch was backed out because gcc on comet doesn't like > > sub_font->mCCMap = gDoubleByteSpecialCharsCCMap; > > when |gDou...| is const. I overlooked it because gcc on my local > machine 'condoned' and allowed it (with warning). It seems like I can > safely cast away |const| for sub_font, but not 100% sure. > (see my comment #26 on nsFontMetricsXlib.cpp) > > The best seems to be to port what's done in nsFontMetricsXlib.cpp > (by Roland) for multiple device context support > to nsFontMetricsGTK.cpp, but I have no idea when that porting > is planned. I am currently sitting on another ~250kb monster-patch, but after that I should be ready to port that code over to gfx/src/gtk/ (hopefully noone else is working on larger patches for that dir in that time :) . Maybe in two or three weeks... ;-/

Roland, thanks for your answer. > In the meantime, I guess I have to remove |const| > for |gDoubleByteSpecialCharsCCMap| in nsFontMetricsGTK.cpp I'll do this for now (with a comment that |const| needs to be put back later when GTK gets sync'd with Xlib) if nobody objects in 24hours.

fix (with const for |gDouble...| removed) relanded.

Comment on attachment 116064 [details] [diff] [review] same as above with a mix-up from another patch removed removing obsolete review request. Patch got r=rbs in comment#42

this fix might have caused a regression for Bug 54467 and Bug 23605. ex. http://bugzilla.mozilla.gr.jp/attachment.cgi?id=2398&action=view