22921 - copied nsIModule code.

Status: RESOLVED INVALID

(SeaMonkey :: General, defect, P3)

Description

[Doug Turner (:dougt)]

The following file copied nsIModule.  Please use the macro NS_IMPL_NSGETMODULE
instead.

D:\cmonkey\mozilla\intl\chardet\src\nsCharDetModule.cpp(149):
D:\cmonkey\mozilla\intl\compatibility\src\nsI18nCompatibility.cpp(177):
D:\cmonkey\mozilla\intl\locale\src\unix\nsLocaleModule.cpp(129):
D:\cmonkey\mozilla\intl\lwbrk\src\nsLWBrkModule.cpp(129):
D:\cmonkey\mozilla\intl\strres\src\nsStringBundle.cpp(506):
D:\cmonkey\mozilla\intl\uconv\src\nsUConvModule.cpp(101):
D:\cmonkey\mozilla\intl\uconv\ucvcn\nsUCvCnModule.cpp(313):
D:\cmonkey\mozilla\intl\uconv\ucvja\nsUCvJaModule.cpp(347):
D:\cmonkey\mozilla\intl\uconv\ucvko\nsUCvKoModule.cpp(281):
D:\cmonkey\mozilla\intl\uconv\ucvlatin\nsUCvLatinModule.cpp(976):
D:\cmonkey\mozilla\intl\uconv\ucvtw\nsUCvTwModule.cpp(271):
D:\cmonkey\mozilla\intl\uconv\ucvtw2\nsUCvTw2Module.cpp(361):
D:\cmonkey\mozilla\intl\unicharutil\src\nsUcharUtilModule.cpp(150):

Comment 1

[Frank Tang]

dp, please provide details about this bug. We have no idea what you talk about.

Comment 2

[Suresh Duddi (gone)]

Sure frank. Look at xpcom/sample/nsSampleModule.cpp

We have embarked on a macro version of factory and module. A lot of code for
these can go away. Converting to the above template will reduce our dll size.

Dougt has converted a lot of them. I did a few before the holidays. The ones
that dougt didnt convert have bugs I think. (BTW thanks doug)

Comment 3

[Frank Tang]

nsUcharUtilModule.cpp fix in my tree and send to dp for code review.
nsUcv*Module.cpp, nsCharDetModule.cpp depend on 23350
I will work on nsI18nCompatibility.cpp, nsLocaleModule.cpp,
nsStringBundle.cpp and nsUConvModule.cpp first.

Comment 4

[Frank Tang]

nsI18nCompatibility.cpp is fix in my tree. Send to dp and nhotta for review.

Comment 5

[cata]

Noooo!!!! Please don't do this indiscriminately!!! In some of the cases, we are
using our own Module object some reasons: at initialization time we
are writing stuff into the registry, we delete that stuff at the end and we
create components in a certain way. The places where we are doing that
are: uconv and the ucv* DLLs. So please don't change those.

Thanks,
Cata

Comment 6

[Frank Tang]

Attachment: nsUConvModule.cpp based on r1.2

Comment 7

[Frank Tang]

archive the r1.2 based nsUConvModule.cpp since cata ask me to hold it.

Comment 8

[Frank Tang]

Cannot fix all the module by m13. Move it to M14.

Comment 9

[Frank Tang]

reassign to jbetak

Comment 10

[bobj]

ftang,
What is the status on this?  Which modules have we updated and checked-in?
Is this bug a Beta1 stopper?

Comment 11

[Doug Turner (:dougt)]

This is not a beta1 stopper.  there are other more important things to do.  
(IMO)

Comment 12

[Frank Tang]

Move this to M15

Comment 13

[Frank Tang]

jbetak- I suggest you do nsUCvCnModule.cpp first.

Comment 14

[Frank Tang]

*** Bug 32792 has been marked as a duplicate of this bug. ***

Comment 15

[Blake Ross]

*spam* changing qa contact from [e-mail] to me ([e-mail]) 
on 121 open or resolved (but not verified) bugs.  sorry for the spam everybody, 
but most of these bugs would just remain dormant and not checked by QA 
otherwise.  I'm not sure how so many bugs have nobody as their QA contact, but 
I suspect this is the fault of some sort of bugzilla corruption that happened 
at some point (most of these bugs are in the 20000-26000 range, and I don't see 
where in the activity log that QA contact explicitly changed to 
[e-mail])

Anyways, sorry again for spam.  If you really get annoyed, I'm usually 
available in #mozilla on IRC for torture.

Comment 16

[[e-mail] (away - not reading bugmail)]

reassigning to ftang for resource reallocation

Comment 17

[Frank Tang]

mark nsbeta3 and perf

Comment 18

[Frank Tang]

nsbeta3- per i18n bug meeting.
dp- what is your opinion. we are open to re-evaluate this bug for nsbeta3 if you 
think it is very important.

Comment 19

[Blake Ross]

If this will help performance and code footprint (the main goals of beta3), why 
nsbeta3- ?

Comment 20

[Judson Valeski]

yea, I'm asking for reconsideration.

Comment 21

[Frank Tang]

The following module are done
nsI18nCompatibility
ucvtw
ucvja
nsLWBrkModule
nsStringBundle
nsUcharUtilModule

The following are not done
uconv
ucvtw2
ucvko
ucvlatin
ucvcn
ucvibm
nsCharDetModule
nslocale
nsI18nCompatibility

dp said he don't think this is important.

Comment 22

[Doug Turner (:dougt)]

dp - why now does this not matter.  Your comments on 01-03:

We have embarked on a macro version of factory and module. A lot of code for
these can go away. Converting to the above template will reduce our dll size.


I think now more than ever, DLL size is important.  

Comment 23

[Jaime Rodriguez, Jr.]

Marking as nsbeta3- per I18N Bug Triage.

DP told Ftang that this issue is not of high importance.

Comment 24

[Doug Turner (:dougt)]

I, the reporter, and DP inital comment disagree with this.  

Comment 25

[Frank Tang]

nsbeta3- per i18n bug meeting. I don't think we have enough time to fix and 
test this. 

Comment 26

[Frank Tang]

reassign to yokoyama

plese try to fix
ucvtw2
ucvko
ucvcn

first. and then
ucvlatin
ucvibm

Comment 27

[Roy Yokoyama]

Attachment: ucvtw2 converted module

Comment 28

[Roy Yokoyama]

Frank: Some of the charsets (eg. UnicodeToISO2022KR) in nsUCvKoCID.h are not in
nsUCvKoModule.cpp.  Is this correct?

Comment 29

[Roy Yokoyama]

Attachment: Ooops, compiler error. Here is the update. Disregard the previous attachement (ucvtw2)

Comment 30

[Roy Yokoyama]

Attachment: ucvko converted module

Comment 31

[Roy Yokoyama]

Cata: How can I verify the conversion? I can view a Korean site with my patch,
but I don't think it's good enough.  Do you have a test plan for all the encodings?

adding [e-mail] to cc

Comment 32

[Judson Valeski]

nice work! r=valeski on the 12/08/00 16:42 and 12/08/00 17:32 patches.

Comment 33

[Roy Yokoyama]

Attachment: ucvcn converted module. This was very tricky.

Comment 34

[Judson Valeski]

nice work again. r=valeski on 12/12/00 10:30 patch

Comment 35

[cata]

It's ok, testing with a page using the respective encoding is the best test.

Comment 36

[Roy Yokoyama]

Cata: Do you have a list of URLs for respective encodings?  I checked against
home.netscape.com/ja, /ko etc and Yahoo.

Comment 37

[cata]

Nope. You should ask Kat, I'm sure he has.

Comment 38

[Roy Yokoyama]

cc'ing: momoi-san
momoi-san: Do you have a list of URLs to do a quick testing on Char Encodings?

Comment 39

[timeless]

<offtopic>
Changing keywords, clobbering milestone. yokoyama could you or a manager select 
a current target milestone (mozilla0.8 currently is the next milestone, other 
candidates include mozilla0.9 and mozilla1.0).

fwiw, these days we're moving towards using cvsblame to show authorship instead 
of inline ownership marks.

+ * The Original Code is Mozilla Communicator client code.
Do we say this elsewhere?
</offtopic>

Comment 40

[Roy Yokoyama]

Attachment: ucvlatin converted module (16 more files to add)

Comment 41

[Roy Yokoyama]

Attachment: ucvlatin converted module (1/16)

Comment 42

[Roy Yokoyama]

Attachment: ucvlatin converted module (2/16)

Comment 43

[Roy Yokoyama]

Attachment: ucvlatin converted module (3/16)

Comment 44

[Roy Yokoyama]

Attachment: ucvlatin converted module (4/16)

Comment 45

[Roy Yokoyama]

Attachment: ucvlatin converted module (5/16)

Comment 46

[Roy Yokoyama]

Attachment: ucvlatin converted module (6/16)

Comment 47

[Roy Yokoyama]

Attachment: ucvlatin converted module (7/16)

Comment 48

[Roy Yokoyama]

Attachment: ucvlatin converted module (8/16)

Comment 49

[Roy Yokoyama]

Attachment: ucvlatin converted module (9/16)

Comment 50

[Roy Yokoyama]

Attachment: ucvlatin converted module (10/16)

Comment 51

[Roy Yokoyama]

Attachment: ucvlatin converted module (11/16)

Comment 52

[Roy Yokoyama]

Attachment: ucvlatin converted module (12/16)

Comment 53

[Roy Yokoyama]

Attachment: ucvlatin converted module (13/16)

Comment 54

[Roy Yokoyama]

Attachment: ucvlatin converted module (14/16)

Comment 55

[Roy Yokoyama]

Attachment: ucvlatin converted module (15/16)

Comment 56

[Roy Yokoyama]

Attachment: ucvlatin converted module (16/16) - This is too much. I hope I didn't make any mistake:)

Comment 57

[Roy Yokoyama]

Attachment: ucvibm converted module

Comment 58

[Roy Yokoyama]

Could someone review the code?
- ucvtw2 : reviewed by Jud (2000-12-11 10:34)
- ucvko : reviewed by Jud (2000-12-11 10:34)
- ucvcn : reviewed by Jud (2000-12-12 10:38)
- cvlatin : need a review
- ucvibm : need a review

Momoi-san:  do we have URLs to test respective encodings?  

Comment 59

[Katsuhiko Momoi]

Some internal sites which contain charset tests:

http://slip/i18n_client/browser/fonts/multibyte_tests/
http://babel/tests/Browser/charset/

You will find EUC-TW CNS planes 1 & 2 here:

http://slip/i18n_client/browser/fonts/multibyte_tests/euc-tw/euc-tw.html

I don't think we have too many IBM code pages for testing
internally. One that comes to my mind is Cyrillic 866
at:

http://babel/tests/Browser/charset/charset_test.html

Comment 60

[Roy Yokoyama]

Attachment: Correction for (12/18/00 17:47) ucvlatin converted module

Comment 61

[Roy Yokoyama]

cc: [e-mail]

Comment 62

[Roy Yokoyama]

Frank: as per our code review, please /r for the new ucvlatin as well as ucvibm.
Thanks

Comment 63

[Roy Yokoyama]

/sr by ftang.  This is a big change. I'll notify iQA by email and keep my eyes
open for any new issues.

Comment 64

[rbs]

What about ucvmath!?

Comment 65

[Roy Yokoyama]

Attachment: ucvmath converted module

Comment 66

[Roy Yokoyama]

[e-mail] < can you review the code for me?

Comment 67

[Teruko Kobayashi]

To test the rendering International fonts, please go to
http://babel/tests/dogfood/templates/top_intl_urls.htm

Comment 68

[rbs]

I didn't applied the patch. But I have scanned it and couldn't spot at anything
suspicious. r=rbs.

Comment 69

[Teruko Kobayashi]

Attachment: Test urls

Comment 70

[Roy Yokoyama]

Updating the target milestone.

Comment 71

[Roy Yokoyama]

ftang: can you /sr= for the ucvmath (01/22/01 15:13)?

Comment 72

[Frank Tang]

for ucvmath sr=ftang

Comment 73

[Roy Yokoyama]

All the modules are checked in. :)  Please verify.  
Thanks

Comment 74

[Roy Yokoyama]

I need to convert uconv.dll and others as well.  Reopening.

Comment 75

[Roy Yokoyama]

We have little more module to do:
nsUconvModule
nsChardetModule
nsLocaleModule
nsLwbrkModule
nsUCharUtilModule

Comment 76

[Roy Yokoyama]

Revising the list of components left:
nsUconvModule - need to use NS_GETMODULE
nsChardetModule  (67576 will resolve this)
nsLocaleModule - need to use NS_GETMODULE
nsLwbrkModule - already done
nsUCharUtilModule - already done

I am going to file new bugs for nsLocaleModule and nsUConvModule.  I hate to
attach patches to this bug after seeing long list of patches attached. :)

Comment 77

[Roy Yokoyama]

I filed 2 new bugs:
74438 nor -- All [e-mail] NEW nsUconvModule - need to use NS_GETMODULE
74439 nor -- All [e-mail] NEW nsLocaleModule - need to use NS_GETMODULE

[e-mail]> please close this bug.