67576 - nsCharDetModule.cpp needs to use NSGETMODULE macros

Status: VERIFIED FIXED

(Core :: Internationalization, defect)

Description

[Judson Valeski]

everything under the mozilla/intl/chardet directory is using much more 
registration and factory code than it needs to. On top of that, because it's 
doing all of it's registration by hand, this module cannot be merged into other 
libraries at build time if consolodation is turned on.

Comment 1

[Judson Valeski]

Specifically the NS_IMPL_NSGETMODULE should be used for the module itself.

The other, most significant gain by doing this is that the charset service can 
register as an observer inside it's (to be implemented) Init method. If 
nsMetaCharsetObserver.cpp uses the NS_GENERIC_FACTORY_CONSTRUCTOR_INIT() macro 
to replace all the factory code, it's ::Init() method will be called when the 
module is loaded, and thus observer registration will be self contained and 
there will be no need for nsIMetaCharsetService at all, and the applevel 
registration that's currently happening can be completely removed.

I started on doing all of this, and gave up when I saw all of the registry 
manipulation that was going on in mozilla/intl/chardet/src/nsCharDetModule.cpp. 
Why does that registry stuff need to be there? Can't the code just leverage 
what's in NSGETMODULE?

Comment 2

[nhottanscp]

Reassign to ftang.

Comment 3

[Frank Tang]

roy- I think you know how to do this :)
Also, see valenski's comment about self Init.

valenski- the question is why this module will be loaded if there are no code to
load it ? Maybe we don't need to answer this question if we merge the code in
this module to combine with outer intl module as roy is currently working on.

Comment 4

[Doug Turner (:dougt)]

Frank, that is the wrong way to think.  Every developer needs to be thinking
about how they can make themselves more componentized, how they can delay from
being loaded, and how they can be make smaller.  I think that it would total
suck ass if everytime I embedded mozilla, the entire i18n uber library would be
loaded.  

Maybe a better question to ask you (and probably take offline), is what is the
minimun that is required of i18n to display the top 100 sites in English?  We
should factor that required piece out, and make the rest of i18n both optional
and not part of the inital library load.

Comment 5

[Roy Yokoyama]

Jud/Dougt: I believe this bug is duplicate of
http://bugzilla.mozilla.org/show_bug.cgi?id=22921.

Dougt: We have already done what you are asking.  We consolidated the i18n libs
and implemented so that only required i18n libs get loaded at the start up and
we saved approx 150k of footprint for the top 100 sites in English (see 65685)
Bottomline is that everytime you embed Mozilla, ONLY the minimum i18n libs get
loaded :)  Moreover, I am also onto this bug (49527) to combine these minimum
i18n libs into one so that we can speedup the startup time and minimize the
footprint.

Comment 6

[Doug Turner (:dougt)]

Roy, thats great.  I misunderstood Frank.

Comment 7

[Teruko Kobayashi]

Changed QA contact to [e-mail].

Comment 8

[Roy Yokoyama]

Attachment: Use of NS_GETMODULE in intl\chardet files

Comment 9

[Roy Yokoyama]

Attachment: Need to update appShell

Comment 10

[Roy Yokoyama]

Attachment: Update content\html and content\xml

Comment 11

[Roy Yokoyama]

Attachment: Update webshell\tests

Comment 12

[Roy Yokoyama]

Attachment: IDL

Comment 13

[Roy Yokoyama]

I think I did everything we need to convert CharDet to use NSGETMODULE.
However, I have problem with the patch, for example, when I visit http:
\\cn.yahoo.com.  QueryInterface() generates exeption.  nsParser has
two CharDet observers; but I believe, I should be one.
What else I am missing?  Any comments anyone?

Comment 14

[Roy Yokoyama]

Doh, this bug is now the smoketest blocker for embedding.  How could this parser
cause the problem?

Comment 15

[Roy Yokoyama]

Attachment: conversion of Classic CharDet

Comment 16

[Roy Yokoyama]

All right, we've resolved the nsParser error and I am going to post a new set 
of patches that only apply to this bug.  (meaning? just the NSGETMODULE 
conversion. I'll summarise the patch set in few minutes)

Comment 17

[Roy Yokoyama]

Attachment: use of NS_GETMODULE macro

Comment 18

[Roy Yokoyama]

Only one patch is required for this bug. (see 04/02/01 15:41 use of 
NS_GETMODULE macro)

valeski: can you review the patch?  

Comment 19

[Judson Valeski]

There are several instances of ::QueryInterface() being explicitly implemented.
Can't the NS_IMPL_ISUPPORTS() macros be used in those cases?

Comment 20

[Roy Yokoyama]

Attachment: Use of NS_IMPL_ISUPPORTS macro

Comment 21

[Roy Yokoyama]

Attached a new patch for using NS_IMPL_ISUPPORTS macro.
valeski: can you review the patch?

Comment 22

[Judson Valeski]

- nsDocumentCharsetInfo.h
* you're declaring the methods of nsIDocumentCharsetInfo explicitly. xpidl
generates a macro at compile time which you can use. The following code goes from...

+class nsDocumentCharsetInfo : public nsIDocumentCharsetInfo
+{
+  NS_DECL_ISUPPORTS
+
+public:
+  nsDocumentCharsetInfo ();
+  virtual ~nsDocumentCharsetInfo ();
+
+  NS_IMETHOD SetForcedCharset(nsIAtom * aCharset);
+  NS_IMETHOD GetForcedCharset(nsIAtom ** aResult);
+
+  NS_IMETHOD SetForcedDetector(PRBool aForced);
+  NS_IMETHOD GetForcedDetector(PRBool * aResult);
+
+  NS_IMETHOD SetParentCharset(nsIAtom * aCharset);
+  NS_IMETHOD GetParentCharset(nsIAtom ** aResult);
+
+private:
+  nsCOMPtr<nsIAtom> mForcedCharset;
+  nsCOMPtr<nsIAtom> mParentCharset;
+};

To...
+class nsDocumentCharsetInfo : public nsIDocumentCharsetInfo
+{
+  NS_DECL_ISUPPORTS
+
+public:
+  nsDocumentCharsetInfo ();
+  virtual ~nsDocumentCharsetInfo ();
+
+  NS_DECL_NSIDOCUMENTCHARSETINFO
+
+private:
+  nsCOMPtr<nsIAtom> mForcedCharset;
+  nsCOMPtr<nsIAtom> mParentCharset;
+};

Which is *much* cleaner and easier to maintain if nsIDocuemntCharsetInfo ever
changes.

* why is NS_DECL_ISUPPORTS declared w/ private scope? It's generally public


nsMetaCharsetObserver.h -
* this class definition can also leverage the macro'ized NS_DECL_NSI"FOO" stuff
for nsIObserver

nsXMLEncodingObserver.h -
* same here for nsIObserver.

After those changes, r=valeski.

Comment 23

[Roy Yokoyama]

Attachment: As per Jud's suggestion

Comment 24

[Roy Yokoyama]

>Which is *much* cleaner and easier to maintain if nsIDocuemntCharsetInfo 
I used NS_DECL_NSIDOCUMENTCHARSETINFO and NS_DECL_NSIOBSERVER in the new patch.

>* why is NS_DECL_ISUPPORTS declared w/ private scope? It's generally public
As a matter of fact, NS_DECL_ISUPPORTS is defined as
public:                                                                     \
  NS_IMETHOD QueryInterface(REFNSIID aIID,                                  \
                            void** aInstancePtr);                           \
  NS_IMETHOD_(nsrefcnt) AddRef(void);                                       \
  NS_IMETHOD_(nsrefcnt) Release(void);                                      \
protected:                                                                  \
  nsrefcnt mRefCnt;                                                         \
  NS_DECL_OWNINGTHREAD                                                      \
public:

Thus it's public scope. :)

Comment 25

[Roy Yokoyama]

erik: can you /sr=?

Comment 26

[Brendan Eich [:brendan]]

Sorry, I started this and had a crash, lost track of the bug.  Here goes.

I didn't track all the #include changes, but wanted to comment that if you're
including files that directly define macros and types used in the including .cpp
or .h file, great.  If you removed #includes because *some other .h file*
happens to nest an include of the same file, that's generally not so great. 
Direct dependencies should be stated.  Sometimes, a "give me these five .h files
with one #include" master.h file helps, but it can lead to over-including, and
shouldn't be promulgated in public APIs (nspr.h is the classic case).

In this modern day, no need for explicit GetService and ReleaseService calls,
and goto done, etc.  Please use

    nsCOMPtr<nsIRegistry> registry(do_GetService(NS_REGISTRY_CONTRACTID));

instead.

sfraser pointed this out recently: make strings const in declarations such as 

static const char *const gRussian[5] = {...};

(note the patch does not have const after the * and before the declarator name).
This helps compilers share strings and put them in readonly memory.

Nit: mItems and the loop variable i in nsCyrillicDetector need not be PRUint8,
it only slows things down to make them so, on common architectures.  If you do
pack mItems with other members to save space per instance (e.g., you could move
it to the end and pack it with mDone, by changing mDone from PRBool to
PRPackedBool), you still might leave i's type PRUintn (a natural register-sized
int), for smaller/faster code.

In the cases at hand, these classes appear to define singletons (I think -- pls.
correct me if I am wrong), so no big deal -- but good practices tend to spread,
so maybe it's worth picking this nit?  Anyway, sorry to pick on code that you
merely moved, which pre-existed your patch.

Let me know if you agree with any of these comments.  I think the do_GetService
one is worth fixing.  I'll sr= whatever you come back with, and I'll be prompt.
Thanks for your patience.

/be

Comment 27

[Roy Yokoyama]

Attachment: As per Brendan's suggestion

Comment 28

[Roy Yokoyama]

Brendan: Thanks for your suggestions.  

-Added nsCOMPtr<nsIRegistry> registry(do_GetService(NS_REGISTRY_CONTRACTID));
and removed explicit GetService and ReleaseService calls, and goto done

-Changed i's type for PRUint8 to PRUintn in nsCyrillicDetector.*

However, I had trouble compiling with
static const char *const gRussian[5] = {...};
I tried other combination just to make sure, such as 
static char *const gRussian[5] = {...};
No cigar. :0

Comment 29

[Brendan Eich [:brendan]]

What compiler were you using that barfed on const char *const gRussian[5]={...}
where the ... were all string literals?  Cc'ing sfraser, in case I've
misunderstood the bug he filed about lack of const strings.

sr=[e-mail] in any event.

/be

Comment 30

[Roy Yokoyama]

I am using MSVC++6 on W2K.

Comment 31

[Simon Fraser [no longer active]]

maybe it doesn't like the static in static const char* const? I know of other 
uses of 'const char* const foo = {}' in the tree

Comment 32

[Roy Yokoyama]

Doh, Sorry my mistake.
Error message is about the function parameter declaration;
not the string const declaration. 

error: "cannot convert parameter 3 from 'const char *const [5]' to const char**"
in nsCyrXPCOMStringDetector(PRUint8 aItems, PRUint8 ** aCyrillicClass, 
                                const char **aCharsets);

sfraser: any trick I can try here?

Comment 33

[Simon Fraser [no longer active]]

Does declaring nsCyrXPCOMStringDetector as:

nsCyrXPCOMStringDetector(PRUint8 aItems, PRUint8 ** aCyrillicClass, const char *
const *aCharsets);

or 

nsCyrXPCOMStringDetector(PRUint8 aItems, PRUint8 ** aCyrillicClass, const char *
const aCharsets[]);

work?

Comment 34

[Roy Yokoyama]

Let's see......
Yes
nsCyrXPCOMStringDetector(PRUint8 aItems, PRUint8 ** aCyrillicClass, const char *
const *aCharsets);
works in Win32.
I'll keep this trick at hand for later usage.  Thank you everyone.

Comment 35

[Mike Kaply [:mkaply]]

After this change went in, we (OS/2) are experiencing traps in CHARDET whenever 
we select auto detectors

See bug 

http://bugzilla.mozilla.org/show_bug.cgi?id=76343

I traced the trap and it appears that what is happening is that mObserver in 
nsDetectionAdapter is getting overriden.

Essentially the new for mObserver and a later new are returning the same memory, 
so when RawBuffer access mObserver it is trash.

Does anyone have any idea why this change would have caused this?

I can't even find where mObserver is supposed to get freed in this code. It's 
not a COMPtr.

Comment 36

[Frank Tang]

v=ftang

Comment 37

[kill this account]

we understand the mObserver overwrite problem and are fixing it in bug
http://bugzilla.mozilla.org/show_bug.cgi?id=76152