Closed
Bug 206379
Opened 22 years ago
Closed 21 years ago
Stop using nsIAtom in nsICharsetConverterManager
Categories
(Core :: Internationalization, defect)
Core
Internationalization
Tracking
()
RESOLVED
FIXED
People
(Reporter: alecf, Assigned: alecf)
References
Details
(Keywords: memory-footprint)
Attachments
(6 files, 7 obsolete files)
360.30 KB,
patch
|
smontagu
:
review+
sfraser_bugs
:
superreview+
|
Details | Diff | Splinter Review |
3.24 KB,
patch
|
alecf
:
superreview+
|
Details | Diff | Splinter Review |
2.34 KB,
patch
|
Details | Diff | Splinter Review | |
2.86 KB,
patch
|
Details | Diff | Splinter Review | |
7.55 KB,
patch
|
alecf
:
superreview+
|
Details | Diff | Splinter Review |
958 bytes,
patch
|
alecf
:
review+
bryner
:
superreview+
|
Details | Diff | Splinter Review |
I have decided to finally take on the enormous task of getting rid of the use of
atoms from nsICharsetConverterManager.
as a part of this, I am also eliminating the requirement to call
GetCharsetAtom() - it is rediculous that every call to GetEncoder/GetDecoder has
to be prefixed by a call to GetCharsetAtom() in order to do alias resolution.
instead, GetEncoder/GetDecoder should just automatically do alias resolution.
As a part of this, I'm making sure that the argument to GetEncoder/GetDecoder
takes a IDL "string" - a const char* since charset names are by definition ASCII...
the ripple effects of these changes are pretty extensive but 90% of the code
this affects is simplified due to reduced string conversion, reduced alias
resolution, and so forth. Overall this should result in some much simpler
codepaths and a generally smaller footprint.
Assignee | ||
Comment 1•22 years ago
|
||
a few other changes:
- removing the old nsICharsetConverterManager, and renaming
nsICharsetConverterManager2 over to nsICharsetConverterManager.
- within intl, this resulted in interface changes in nsIPlatformCharset,
nsIConverterInputStream, nsICharsetAlias, nsIScriptableUConv, and nsIPosixLocale
- all mostly simplification
Assignee | ||
Comment 2•21 years ago
|
||
*whew* - I finally finished this. its going to be a beast!
Well, I'm almost finished - I've done the work on linux and windows.. next comes
mac and then we gotta notify the ports due to impacts in gfx/widget
Assignee | ||
Comment 3•21 years ago
|
||
ok, here's the complete patch. Whew.
Assignee | ||
Comment 4•21 years ago
|
||
Comment on attachment 124590 [details] [diff] [review]
remove nsIAtom from most of the API
Requesting reviews from Simon and Blizzard. This is a huge patch I know but it
reallys simplifies some code and thus is a great footprint win.
for my own planning I'd REALLY like to get some ETAs on this.. if you're
super-busy, please suggest an alternate reviewer - blizzard, I don't know who
else does i18n super-reviews?
In my next comment, I'll explain all aspects of the bug.
Attachment #124590 -
Flags: superreview?(blizzard)
Attachment #124590 -
Flags: review?(smontagu)
Assignee | ||
Comment 5•21 years ago
|
||
ok, here's what's going on here... for the reviewers:
1) I got rid of nsICharsetConverterManager2 and rolled it into
nsICharsetConverterManager, so there is now a single interface for this stuff,
and it is in IDL.
2)
I got rid of all this silly atom stuff. the atoms were really just being used as
a cheesy way to force people to go through nsICharsetAlias. In the old world,
you used nsICharsetConverterManager2::GetCharsetAtom() to convert an charset
string like "x-sjis" to its equivalent, like "Shift_JIS" and then THAT string
would be converted to an atom, that you'd pass to
GetUnicodeEncoder/GetUnicodeDecoder.
This was just silly. There is really no value in using atoms as the charset
strings were not used in any hashtable, and the atoms were often short-lived. it
meant creating this temporary object on the heap (and storing it in the atom
table) using it once, and then usually destroying it immediately afterwards.
Now, GetUnicodeEncoder/Decoder goes through nsICharsetAlias every time - there
was not a single consumer that wouldn't have wanted to go through the alias
resolver.
Since I didn't need Atoms anymore, I switched over to just using strings. For
absolute simplicity, I used the IDL type "string" because these strings are
usually VERY short (less than 16 characters) and ALWAYS ASCII. By definition,
charset names cannot be encoded in any other charset. The advantages of using
nsACString& were outweighed by the overhead of creating an nsDependentCString. I
wrestled with this decision quite a bit, but in the end I think "string" is
really the right thing here.
3) This also meant simplifying other methods in nsICharsetConverterManager to
take ASCII strings rather than atoms. The one exception was
getCharsetLangGroup() - this should eventually be changed to use raw strings,
but that method doesn't really touch the code I was changing. I'd rather do that
in a seperate patch (this one is big enough!)
4) Some of this conversion had a rippling effect through the i18n APIs - which
included switching lots of i18n APIs over to using nsACString& or "string" - I
tried to simplify where it made sense, and avoid unnecessarily changing excess
code. I hope the reviewers will bear with me when they see calls to
NS_LossyConvertUCS2toASCII().
5) And of course, there are all the changes to the rest of the tree. In some
cases it was as simple as changing the #include's from
nsICharsetConverterManager2.h to nsICharsetConverterManager.h, and in other
cases it involved reworking of existing APIs elsewhere in the tree. I almost
every case it resulted in simpler code, and almost always faster because of less
UCS2/ASCII/UTF8/etc conversion of charset names.
6) Finally, I revved the IID's in all the interfaces I changed, except for
mailnews - revving IIDs was a huge pain and I really ran out of steam by the
time I got to mailnews. My guess is that there aren't really any
plugins/add-on's that use the mail APIs, or at least the ones I changed, which
were more low level (nsIMessenger, nsIMailNewsUrl, etc)
*whew*
Status: NEW → ASSIGNED
Assignee | ||
Comment 6•21 years ago
|
||
Comment on attachment 124590 [details] [diff] [review]
remove nsIAtom from most of the API
I'm just remembering that simon (fraser) also does reviews, so I'm asking him
as well.
Oh two other things:
- for anyone reading this bug or wants to help out, I can certainly use the
review - even the nitpicking reviews will lighten the load for simon and simon.
- we'll need ports work done here - you just need to apply the patch and then
start building. I'll try to spread the word.
Attachment #124590 -
Flags: superreview?(blizzard) → superreview?(sfraser)
Comment 7•21 years ago
|
||
Alec, the code in nsCSSLoader no longer needs to resolve charset aliases with
this change, does it?
Comment 8•21 years ago
|
||
I tried building this on linux w/gtk2 and i'm seeing some bustage:
/home/dwitte/builds/scratch/mozilla/widget/src/gtk2/nsClipboard.cpp: In
function `void ConvertHTMLtoUCS2(guchar*, int, PRUnichar**, PRInt32&)':
/home/dwitte/builds/scratch/mozilla/widget/src/gtk2/nsClipboard.cpp:616: error: no
matching function for call to `nsDerivedSafe<nsICharsetConverterManager>::
GetUnicodeDecoder(nsAutoString*, nsGetterAddRefs<nsIUnicodeDecoder>)'
../../../dist/include/uconv/nsICharsetConverterManager.h:75: error: candidates
are: virtual nsresult nsICharsetConverterManager::GetUnicodeDecoder(const
char*, nsIUnicodeDecoder**)
gmake[5]: *** [nsClipboard.o] Error 1
throwing in an NS_LossyConvertUCS2toASCII().get() fixed that problem, and then:
/home/dwitte/builds/scratch/mozilla/xpfe/components/search/src/nsInternetSearchService.cpp:
In
member function `const char* const
InternetSearchDataSource::MapScriptCodeToCharsetName(unsigned int)':
/home/dwitte/builds/scratch/mozilla/xpfe/components/search/src/nsInternetSearchService.cpp:3684:
error: brace-enclosed
initializer used to initialize `const char* const'
[repeated a bunch of times, same line#, for all variables in that initializer block]
I gave up at that point - ping me sometime if you'd like me to do another
build/smoketest run (or, if you can't reproduce, I can send you my .mozconfig).
Comment 9•21 years ago
|
||
Comment on attachment 124590 [details] [diff] [review]
remove nsIAtom from most of the API
Firstly, thanks a lot for fixing all these cases. It must have been really
tedious.
> there was not a single consumer that wouldn't have wanted to go through
> the alias resolver.
It's strange that you reached that conclusion after fixing all this.
At the beginning, I thought similarly (although I suspected that there might
be a few exceptions). After going through the patch, I concluded that
what I had thought of as an exception (no need to resolve aliases) is
actually the norm (at least for |GetUnicodeEncoder|.). Given this,
I think we're better off with, in nsICharsetConverterManager,
nsIUnicode(En|De)coder GetUnicode(En|De)coder(in string, in bool)
than with
nsIUnicode(En|De)coder GetUnicode(En|De)coder(in string)
>Index: extensions/spellcheck/src/nsSpellCheckUtils.cpp
....
>@@ -112,7 +112,7 @@
>
> nsresult rv;
>
>- nsCOMPtr <nsICharsetConverterManager2> ccm2 = do_GetService(NS_CHARSETCONVERTERMANAGER_CONTRACTID, &rv);
>+ nsCOMPtr <nsICharsetConverterManager> ccm2 = do_GetService(NS_CHARSETCONVERTERMANAGER_CONTRACTID, &rv);
Nit. Why don't you just drop '2' in ccm2? There are several other cases like
this.
> NS_ENSURE_SUCCESS(rv, rv);
>
> nsCOMPtr <nsIAtom> charsetAtom;
Here GetUnicode(En|De)coders are still called the old way with charsetAtom.
>Index: extensions/xmlextras/base/src/nsXMLHttpRequest.cpp
> nsCOMPtr<nsIUnicodeDecoder> decoder;
>- rv = ccm->GetUnicodeDecoder(&dataCharset,getter_AddRefs(decoder));
>+ rv = ccm->GetUnicodeDecoder(NS_LossyConvertUCS2toASCII(dataCharset).get(),
>+ getter_AddRefs(decoder));
How about keeping GetUnicode(En|De)coder(nsAString &,...) so that we don't have
to call NS_LossyConvertUCS2toASCII(...).get() on many occasions like the
above?
>Index: intl/locale/src/unix/nsCollationUnix.cpp
>@@ -165,7 +161,7 @@
> PRUnichar* mappedCharset = NULL;
> res = platformCharset->GetDefaultCharsetForLocale(aLocale.get(), &mappedCharset);
> if (NS_SUCCEEDED(res) && mappedCharset) {
>- mCollation->SetCharset(mappedCharset);
>+ mCollation->SetCharset(NS_LossyConvertUCS2toASCII(mappedCharset).get());
> nsMemory::Free(mappedCharset);
Wouldn't it be better to use nsXPIDLString for mappedCharset
(in nsCollationXXX.cpp)? While we're at it, we may as well want to change
|GetDefaultCharsetForLocale| to take 'char **' instead of 'PRUnichar **' as
the 2nd argument? It's only invoked in half dozen places (nsCollationXXX.cpp
and nsDateTimeFormatXXX.cpp) and defined in nsXXXCharset.cpp
where |MapToCharset| and |GetDefaultCharsetForLocale| are redefined anyway...
>Index: intl/uconv/src/nsPlatformCharset.h
>+ nsresult MapToCharset(short script, short region, nsACString& outCharset);
>+ nsresult MapToCharset(nsAString& inANSICodePage, nsACString& outCharset);
>Index: intl/uconv/idl/nsICharsetConverterManager.idl
>+%{ C++
>+#include "nsIUnicodeDecoder.h"
>+#include "nsIUnicodeEncoder.h"
>+#include "nsString.h"
>+
>+
>+// XXX change to NS_CHARSETCONVERTERMANAGER_CID
>+#define NS_ICHARSETCONVERTERMANAGER_CID \
>+ {0x3c1c0163, 0x9bd0, 0x11d3, { 0x9d, 0x9, 0x0, 0x50, 0x4, 0x0, 0x7, 0xb2}}
>+
>+// XXX change to NS_CHARSETCONVERTERMANAGER_PID
>+#define NS_CHARSETCONVERTERMANAGER_CONTRACTID "@mozilla.org/charset-converter-manager;1"
>+%}
Do you care to make a new file nsUConvCID.h, put two defines there and include
it wherever nsICharsetConverterManager.h is included? Sorry it's more work,
but you're touching all files including 'nsICharsetConverterManage*.h'
anyway... Then, my patch for bug 162765 (bug 162765 comment #32) can be a
bit smaller :-)
>Index: intl/uconv/public/MANIFEST
Isn't this only for Mac build with Codewarrior(?) ? Now that we switched
over to gmake-based build on Mac, I guess we don't have to worry about this
any more.
>Index: content/html/style/src/nsCSSLoader.cpp
>+static nsresult ResolveCharset(const nsACString& aCharsetAlias,
>+ nsACString& aCharset)
bz>the code in nsCSSLoader no longer needs to resolve charset aliases with
bz>this change, does it?
I guess it depends on what we want/need to store in |mCharset|, the
canonical resolved name or 'raw value'?
>Index: gfx/src/freetype/nsFreeType.cpp
>+ nsresult res;
>+ res = charSetManager->GetUnicodeEncoder(fei->mConverterName, &fei->mConverter);
Under gfx/src, there are a few places where skipping the alias
resolution is desirable because 'mConverterName' (charset) is guaranteed
(charsets are not coming from ext. source but are hard-coded in
nsFontMetrics(Xlib|Gtk) or properties files
to match one of font encoding names (e.g. sun.unicode.india-0) and going
through the alias resolution (that is not necessary) might have some perf.
implication. Would it be a bad idea to add an optional third argument to
|GetUnicodeEncoder| indicate the alias resolution has to be skipped?
|GetUnicodeDecoder| does not need this facility. Ah.... I realized that
we can't have an optional argument in idl (can we?).. Do we want a second
method (GetUnicodeEncoderNoResolution) ?
>Index: gfx/src/gtk/nsFontMetricsGTK.cpp
>+ // used NS_NewAtom before??
>+ nsIUnicodeEncoder* converter = nsnull;
>+ res = gCharSetManager->GetUnicodeEncoder(aSelf->mCharSet, &converter);
>+ if (NS_SUCCEEDED(res)) {
This is an example of what I wrote about above. nsFontMetricsGTK.cpp
is skipping the alias resolution and invokes GetUnicodeEncoder(string,...)
directly because there's no need to resolve charset aliases.
>- nsCOMPtr<nsIAtom> charset;
>- nsresult res = gCharSetManager->GetCharsetAtom2(aCharSetInfo->mCharSet,
>- getter_AddRefs(charset));
>- if (NS_SUCCEEDED(res)) {
>- res = gCharSetManager->GetCharsetLangGroup(charset,
>+ nsresult res;
>+
>+ res = gCharSetManager->GetCharsetLangGroup(aCharSetInfo->mCharSet,
> &aCharSetInfo->mLangGroup);
What I wrote about GetUnicodeEncoder applies to GetCharsetLangGroup
as well.
>Index: gfx/src/mac/nsMacUnicodeFontInfo.cpp
> // This function uses the charset converter manager (CCM) to get a pointer on
>@@ -543,22 +547,18 @@
>- nsCOMPtr<nsIAtom> charset;
>- rv = gCharsetManager->GetCharsetAtom(value.get(), getter_AddRefs(charset));
>- if (NS_FAILED(rv)) return rv;
>-
>- rv = gCharsetManager->GetUnicodeEncoder(charset, aConverter);
>+ rv = gCharsetManager->GetUnicodeEncoder(value.get(), aConverter);
Here again, we don't want/need the charset alias resolution. We're getting
charset names from maccharset.properties that are already in the canonical
form.
>Index: gfx/src/ps/nsPostScriptObj.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/gfx/src/ps/nsPostScriptObj.cpp,v
>@@ -3076,21 +3076,11 @@
> if (psnativecode) {
....
>+ nsCOMPtr<nsICharsetConverterManager> ccMain =
>+ do_GetService(kCharsetConverterManagerCID, &res);
>+
>+ if (NS_SUCCEEDED(res)) {
>+ res = ccMain->GetUnicodeEncoder(psnativecode.get(), &linfo->mEncoder);
I have to check, but it seems like we could have avoided the alias
resolution because 'psnativecode' also comes from one of properties/pref.
file as long as end-users can be 'disciplined' to use the canonical
charset names only when they edit them. If that's the case, this is another
case where we don't want/need the alias resolution.
>Index: gfx/src/windows/nsFontMetricsWin.cpp
> // Helper to determine if a font has a private encoding that we know something about
> static nsresult
>-GetEncoding(const char* aFontName, nsString& aValue)
>+GetEncoding(const char* aFontName, nsCString& aValue)
>+ if (gFontEncodingProperties) {
>@@ -1299,13 +1302,11 @@
>+GetConverterCommon(const char* aEncoding, nsIUnicodeEncoder** aConverter)
> {
>- if (NS_FAILED(rv)) return rv;
>- rv = gCharsetManager->GetUnicodeEncoder(charset, aConverter);
>+ nsresult rv;
>+ rv = gCharsetManager->GetUnicodeEncoder(aEncoding, aConverter);
Yet another case :-). fontEncoding.properties file is supposed to
use the canonical names only. Actually, as is the case of X11 font
encoding names, 'charset' names in fontEncoding.properties don't
have (are NOt supposed to have) any alias in charsetalias.properties.
>- rv = gCharsetManager->GetUnicodeEncoder(charset, &gUserDefinedConverter);
>+ rv = gCharsetManager->GetUnicodeEncoder("x-user-defined", &gUserDefinedConverter);
Another case :-) The old code should not have used 'charsetAtom' but instead
should have directly invoked GetUnicodeEncoder("x-user-defined",....)
as in the following code in nsFontMetricsXlib.cpp
>Index: gfx/src/xlib/nsFontMetricsXlib.cpp
>- res = mFontMetricsContext->mCharSetManager->GetUnicodeEncoder(charset, &ud_conv);
>+ res = mFontMetricsContext->mCharSetManager->GetUnicodeEncoder("x-user-defined", &ud_conv);
>@@ -2539,17 +2530,15 @@
>+ nsresult res;
>+ // used to use NS_NewAtom??
> nsCOMPtr<nsIUnicodeEncoder> converter;
>- res = aFmctx->mCharSetManager->GetUnicodeEncoder(charset,
>+ res = aFmctx->mCharSetManager->GetUnicodeEncoder(aEntry->mInfo->mCharSet,
the same case as in nsFontMetfics(GTK|Win).cpp, nsFreetype?.cpp,
and my patch for bug 176290...
>@@ -2580,64 +2569,55 @@
>+ nsIUnicodeEncoder* converter = nsnull;
>+ res = aFmctx->mCharSetManager->GetUnicodeEncoder(aSelf->mCharSet, &converter);
ditto...
>@@ -4040,18 +4020,15 @@
>+ res = aFmctx->mCharSetManager->GetCharsetLangGroup(aCharSetInfo->mCharSet,
>+ &aCharSetInfo->mLangGroup);
again, we don't need the alias resolution...
>Index: htmlparser/src/nsScannner.cpp
>@@ -204,22 +204,22 @@
>
> nsCOMPtr<nsICharsetAlias> calias(do_GetService(kCharsetAliasCID, &res));
> NS_ASSERTION( nsnull != calias, "cannot find charset alias");
>- nsAutoString charsetName; charsetName.Assign(aCharset);
>+ nsCAutoString charsetName = NS_LossyConvertUCS2toASCII(aCharset);
....
>+ res = calias->GetPreferred(charsetName, charsetName);
>@@ -229,7 +229,7 @@
...
>- res = ccm->GetUnicodeDecoder(&mCharset, &decoder);
>+ res = ccm->GetUnicodeDecoder(mCharset.get(), &decoder);
mCharset is already a canonical name... so even for GetUnicodeDecoder,
we sometimes want to skip the alias resolution.
>Index: layout/html/forms/src/nsIsIndexFrame.cpp
>@@ -520,7 +522,7 @@
>@@ -530,7 +532,7 @@
> NS_GET_IID(nsICharsetConverterManager),
> (nsISupports**)&ccm);
> if(NS_SUCCEEDED(rv) && (nsnull != ccm)) {
>- rv = ccm->GetUnicodeEncoder(&charset, encoder);
>+ rv = ccm->GetUnicodeEncoder(charset.get(), encoder);
This is a bit murky case. The current code doesn't go via charsetAtom
(i.e. no alias resolution) and that's probably all right provided that
|GetSubmitCharset| returns the canonical name.
>Index: widget/src/gtk/nsClipboard.cpp
> nsCOMPtr<nsICharsetConverterManager> ccm = do_GetService(NS_CHARSETCONVERTERMANAGER_CONTRACTID, &rv);
>- rv = ccm->GetUnicodeDecoder(&platformCharset, getter_AddRefs(decoder));
>+ rv = ccm->GetUnicodeDecoder(platformCharset.get(),
>+ getter_AddRefs(decoder));
I'm a bit suspicious of what's going on in nsClipboard.cpp (that's off the
topic here, though). Anyway, here the
current code doesn't go via charsetAtom, either. Some charset names used
here are hard-coded and others are likely to have been stored in canonical
names so that going through the alias resolution again could be a waste.
>Index: widget/src/gtk/nsGtkIMEHelper.cpp
>+ manager->GetUnicodeDecoder(charset.get(), &mDecoder);
again no atom here.
>Index: widget/src/gtk/nsWindow.cpp
>+ rv = ccm->GetUnicodeEncoder(platformCharset.get(), getter_AddRefs(encoder));
ditto...
>Index: widget/src/mac/nsMacControl.cpp
>+ nsCAutoString fileSystemCharset;
> GetFileSystemCharset(fileSystemCharset);
>+ rv = ccm->GetUnicodeEncoder(fileSystemCharset.get(), &mUnicodeEncoder);
ditto...
>Index: widget/src/windows/nsFilePicker.cpp
>+ rv = ccm->GetUnicodeEncoder(fileSystemCharset.get(), &mUnicodeEncoder);
same here...
>Index: widget/src/xpwidgets/nsPrimitiveHelpers.cpp
> nsCOMPtr<nsICharsetConverterManager> ccm =
> do_GetService(NS_CHARSETCONVERTERMANAGER_CONTRACTID, &rv);
>- rv = ccm->GetUnicodeDecoder(&platformCharset, getter_AddRefs(decoder));
>+ rv = ccm->GetUnicodeDecoder(platformCharset.get(),
>+ getter_AddRefs(decoder));
same here...
>Index: xpfe/appshell/src/nsAppShellService.cpp
>- rv = ccm->GetUnicodeDecoder(&aCharset, getter_AddRefs(decoder));
>+ rv = ccm->GetUnicodeDecoder(aCharset.get(), getter_AddRefs(decoder));
again..
>Index: xpfe/components/bookmarks/src/nsBookmarksService.cpp
>- rv = charsetConv->GetUnicodeDecoder(&defaultCharset, getter_AddRefs(mUnicodeDecoder));
>+ rv = charsetConv->GetUnicodeDecoder(defaultCharset.get(),
>+ getter_AddRefs(mUnicodeDecoder));
> }
>@@ -1069,13 +1072,13 @@
....
>- nsAutoString charsetName;
>+ nsCAutoString charsetName;
> if (NS_SUCCEEDED(rv = gCharsetAlias->GetPreferred(charset, charsetName)))
> {
> if (!charsetName.IsEmpty())
>@@ -1092,7 +1095,7 @@
> (nsISupports**)&charsetConv);
> if (NS_SUCCEEDED(rv) && (charsetConv))
> {
>- rv = charsetConv->GetUnicodeDecoder(&charset, decoder);
>+ rv = charsetConv->GetUnicodeDecoder(charset.get(), decoder);
It doesn't look like the resolved charset name is stored anywhere.
Why don't we just let GetUnicodeDecoder do the resolution?
>Index: xpfe/components/search/src/nsInternetSearchService.cpp
>+InternetSearchDataSource::MapScriptCodeToCharsetName(PRUint32 aScriptCode)
> {
>+ if (aScriptCode < NS_ARRAY_LENGTH(scriptList))
>+ aScriptCode = 0;
Shouldn't it be
if (aScriptCode >= NS_ARRAY_LENGTH(scriptList))
Comment 10•21 years ago
|
||
followup to comment 8: killing the spurious braces fixed the bustage - build
seems to work fine now
Assignee | ||
Comment 11•21 years ago
|
||
jshin: thank you SO much for the review. I know it was a beast. Appearently I
just didn't understand the alias resolution stuff as well as I thought! since
you seem to have found the cases where we don't need the resolution, I will
change those to GetUnicode[En|De]coderRaw() - a version which does not use the
alias resolution. I thought about the extra boolean but I think a seperate
function call is more concise and makes the consumers easier to understand.
As for some of the other stuff, like:
- why we don't switch to nsACString& from char** and so forth - I am trying to
fix the minimal case, which as you can see is pretty huge thus far. Do you mind
if I get those in another patch, a second round of fixes?
- As for nsUConvCID.h, do you mind if I get that in another patch as well? I
agree that those should be in a seperate file but there are SO many places that
#include nsICharsetConverterManager.h, that it would be very painful to revisit
them all.
bz: thanks also for the review, it sounds like jshin has covered that case, I'll
use the GetCharsetEncoderRaw()
dwitte: Is there a missing brace in my patch, or is there just missing stuff? I
forgot that gtk2 will be one of the ports that I need porting work on - thanks
for helping! is there any chance you can post a patch with the changes you had
to make, and I'll incorporate it into my work? (I'm assuming the changes will be
in new files that this patch doesn't cover)
Assignee | ||
Comment 12•21 years ago
|
||
*** Bug 161814 has been marked as a duplicate of this bug. ***
Comment 13•21 years ago
|
||
Here's the OS/2 patch
Comment 14•21 years ago
|
||
Alec, I didn't read any part of this patch, actually... so no review here.
jshin, the CSSLoader just wants a string it can pass to GetUnicodeDecoder to
come out with a decoder. It sometimes has strings that do need resolution (eg
coming from HTTP or from the sheet) and sometimes strings that do not need
resolution (coming from nsIDocument). Right now we resolve aliases by hand in a
bunch of places in that code; I ended up writing a helper function to do it,
because it was such a pain. So from the CSSLoader point of view, it would be
much preferable to just have intl handle things reasonably and not have to do
all that work.
Comment 15•21 years ago
|
||
Comment on attachment 124590 [details] [diff] [review]
remove nsIAtom from most of the API
>Index: intl/locale/src/unix/nsDateTimeFormatUnix.cpp
>===================================================================
>@@ -118,20 +118,20 @@
...
> if (NS_SUCCEEDED(res)) {
>- res = charsetConverterManager->GetCharsetAtom(mCharset.get(), getter_AddRefs(charsetAtom));
> if (NS_SUCCEEDED(res)) {
>- res = charsetConverterManager->GetUnicodeDecoder(charsetAtom, getter_AddRefs(mDecoder));
>+ res = charsetConverterManager->GetUnicodeDecoder(mCharset.get(), getter_AddRefs(mDecoder));
> }
> }
>
You can lose one of the two |if (NS_SUCCEEDED(res)) {| lines here
>Index: intl/uconv/idl/nsICharsetConverterManager.idl
...
>+ * Here Charsets are indentified by nsIAtom's. I know, we could have our own
...
This comment block needs updating.
Also, the new files should be MPL-trilicensed not NPL-trilicensed.
This is as far as I've got today, I'll try to finish the rest tomorrow.
Comment 16•21 years ago
|
||
alecf: np... this stuff looks nice :)
as requested, this patch does a couple things...
1) fixes bustage in xpfe/components/nsInternetSearchService.cpp
2) applies your work to the gtk2 port
3) fixes some general string-class usage in gtk & gtk2 (not required for this
work, but I just noticed some silly stuff on a cursory glance over the code,
and felt like fixing it)
if you'd rather we stick to basics, I can post a patch without the misc string
changes.
(you need to apply this patch _on top_ of yours, btw... also, i've
compiletested gtk2, but not the gtk portion which I modified, so your request
for gtk port testing still applies)
Assignee | ||
Comment 17•21 years ago
|
||
cool, thanks for the patch - I'm really surprised to see that bustage in gtk and
nsInternetSearchService, I've gotta say. The original patch still builds on
gtk/osx/win32 for me.. odd (perhaps it isn't applying cleanly now?)
But is there any chance we could do without the string cleanup? I will GLADLY
sr= any string cleanup later, but I want to keep this patch to the absolute
minimum, given its current size...(364k without the os2 or gtk2 patches)
Comment 18•21 years ago
|
||
>cool, thanks for the patch - I'm really surprised to see that bustage in gtk and
>nsInternetSearchService, I've gotta say. The original patch still builds on
>gtk/osx/win32 for me.. odd (perhaps it isn't applying cleanly now?)
it applies cleanly with a couple fuzzes... I'm using gcc3.3, maybe it's related
to that?
>But is there any chance we could do without the string cleanup? I will GLADLY
>sr= any string cleanup later, but I want to keep this patch to the absolute
>minimum, given its current size...(364k without the os2 or gtk2 patches)
sure thing, coming right up.
Comment 19•21 years ago
|
||
this avoids the string cleanup (except for one spot in
widget/src/gtk/nsClipboard.cpp which WAS related to your changes, where I made
an obvious correction)
as before, this applies on top of your patch.
Attachment #124816 -
Attachment is obsolete: true
Comment 20•21 years ago
|
||
Alec, all you wrote is reasonable.
BTW, what are you gonna do with GetCharsetLangGroup()? It's not used as often as
GetUnicode(De|En)coder, but it has the same issue. In nsFontMetrics(GTK|Xlib),
charset names are coming from the hard-coded arrays (nsFontCharSetInfo types)
with canonical names (the current code should have used NS_NewAtom instead of
GetCharsetAtom2()) so that no resolution is necessary. In a few other places
(mailnews, intl/locale), it's coming from external sources and we need to
resolve aliases.
> jshin, the CSSLoader just wants a string it can pass to
> GetUnicodeDecoder to come out with a decoder.
OK. I see. So, CSSLoader doesn't care whether mCharset is canonical or not.
When mCharset is used to get a UnicodeDecoder for CSSLoader
(nsIUnicharStreamLoader), we just have to make sure that GetCharsetDecoder()
(that does resolve aliases) is invoked. I tracked it down and found that it's
http://lxr.mozilla.org/seamonkey/source/intl/uconv/src/nsConverterInputStream.cpp#50
At the moment, it's not resolving aliases. Why doesn't it? If it did,
ResolveCharset() in nsCSSLoader.cpp wouldn't be necessary even without Alec's
patch, would it?
Anyway, in nsConverterInputStream we have to make sure that GetUnicodeDecoder()
with the alias resolution is called.
In addition, I guess we have to make it clear that charset in
nsIUniCharStreamLoader is not necessarily a canonical name.
(http://lxr.mozilla.org/seamonkey/source/netwerk/base/public/nsIUnicharStreamLoader.idl#109)
Assignee | ||
Comment 21•21 years ago
|
||
dan, again thanks for helping me out here! This is very helpful
mike, thanks for the OS/2 stuff, I'm incorporating it now
jshin, I'm making a getCharsetLangGroupRaw() to cover the same case
it sounds like the nsCSSLoader stuff is going to be pretty straight forward, and
that I may have possibly fixed a bug by using the alias-resolving version of
GetUnicodeDecoder :) Thank you SO much for going through and figuring out which
cases needed the raw version...that makes my life much easier.
Anyhow, I'm updating the patch to address all of jshin and simon's reviews.
Assignee | ||
Comment 22•21 years ago
|
||
ok, here's an updated patch that incorporates the os2 and gtk2 patches above..
there is appearently one minor issue with the mail compose window that I will
be investigating.. but I think that will be a .js patch that is a supplement to
this one... (so this patch can be further reviewed without fear that it will be
updated again)
Attachment #124590 -
Attachment is obsolete: true
Attachment #124762 -
Attachment is obsolete: true
Attachment #124828 -
Attachment is obsolete: true
Assignee | ||
Updated•21 years ago
|
Attachment #124590 -
Flags: superreview?(sfraser)
Attachment #124590 -
Flags: review?(smontagu)
Assignee | ||
Comment 23•21 years ago
|
||
ok, I could really use some more reviews on attachment 124846 [details] [diff] [review] - especially from
jshin and smontagu.
The one thing I forgot was to change all the "ccm2" variables over to 'ccm' -
I've done that now in my local tree, but its not a large enough change to
justify a new patch.
Assignee | ||
Comment 24•21 years ago
|
||
Comment on attachment 124846 [details] [diff] [review]
remove nsIAtom v2
looking for a r=smontagu AND a r=jshin... thanks guys!
Attachment #124846 -
Flags: review?(smontagu)
Assignee | ||
Comment 25•21 years ago
|
||
ok, this fixes all the stuff in MsgComposeCommands.js, which was doing all
sorts of goofy things with charsets and such. This gets applied in addition to
the above patch. I used LXR to search for any other occurences of this stuff,
and this is the only one I found.. go figure!
Comment 26•21 years ago
|
||
To nhotta: Can you answer my question about nsTextToSubURI in the following (I'm
sorry it's in the middle.)
re: attachment 124846 [details] [diff] [review]
Summary :
1. I ended up checking (mostly) raw vs non-raw.
(Simon may wish to spend more time on nsCharsetMenu.cpp,
nsScriptLoader.cpp, nsCharsetConverterManager.cpp)
2. Some observations:
a. Whereever a string literal is directly passed to GetUnicodeXXX() or
NS_NewAtom("...") is used, it's completly safe to use
GetUnicodeXXXRaw(). In intl/uconv, gfx/src, mailnews and other places,
there are many cases of non-raw version of GetUnicodeXXX invoked
with UTF-8, GB2312, EUC-KR, x-euc-tw, ISO-8859-7, ISO-8859-1,
x-user-defined, x-imap4-modified-utf. An example is shown below:
Index: extensions/xmlextras/base/src/nsDOMParser.cpp
+ rv = charsetConv->GetUnicodeEncoder("UTF-8",
getter_AddRefs(encoder));
A subset of the canonical charset names are listed in
http://lxr.mozilla.org/seamonkey/source/intl/uconv/src/charsetalias.properties
Why a subset? Because XLFD names and other internal charset names
don't need the alias resolution and developers are supposed to use
correct names for them when hard-coding them in the source code.
Others are listed in
http://lxr.mozilla.org/seamonkey/source/intl/uconv/src/charsetdata.properties
b. Unless it's obviously a mistake in the old code(as we found
earlier), we can replace GetUnicode(En|De)code(char *,...) or
GetUnicode(En|De)code(NS_NewAtom(char *),...)
with GetUnicode(En|De)coderRaw(char *,...). In most of cases, charset
names passed are already canonical because they come from
platformCharset/filesystem charset/other sources known to provide
the canonical name. In what follows, I list some cases (but not all).
Doing this would not introduce a regression although we may be
preserving bugs as well.
c. The opposite case of b. Unless it's obvious (I identified several
in the previous comment and found a couple of more here),
we can just use non-raw methods when charsetAtom is used in the old
code. This item has null effect on the patch, though. :-)
d. When the resolved charsetAtom is stored in the old code (in member
variables), we have to store the resolved charset name. Your patch
already did that, but there's a case you might have missed.
(nsIContentSerializer. see comment further down.)
3. As far as 'raw vs non-raw' is concerned,
I just realized that I could have edited the patch directly
and sent it to you off-line instead of commenting on that. It'd have
taken less time ...... Stupid of me....
Index: extensions/xmlextras/base/src/nsXMLHttpRequest.cpp
- rv = ccm->GetUnicodeDecoder(&dataCharset,getter_AddRefs(decoder));
+ rv = ccm->GetUnicodeDecoder(NS_LossyConvertUCS2toASCII(dataCharset).get(),
+ getter_AddRefs(decoder));
I guess dataCharset is already canonicalized. In intl/chardet and other places,
we return canonical names to implementations of nsIDocument. At least that's
what's assumed by the current code (item 2b)
Index: intl/locale/src/mac/nsDateTimeFormatMac.cpp
@@ -316,14 +316,12 @@
- res = charsetConverterManager->GetCharsetAtom(mUseDefaultLocale ?
mSystemCharset.get() : mCharset.get(),
- getter_AddRefs(charsetAtom));
- if (NS_SUCCEEDED(res)) {
- res = charsetConverterManager->GetUnicodeDecoder(charsetAtom,
getter_AddRefs(mDecoder));
- }
+ res = charsetConverterManager->GetUnicodeDecoder(mUseDefaultLocale ?
mSystemCharset.get() :
+ mCharset.get(),
I'm 99.9% sure that we don't need the alias resolution in
nsDateTimeFormatXXX.cpp (where XXX is Mac, Win, OS2, UNIX, BeOS, etc)
because all of them use charset name provided by implementations of
nsIPlatformCharset (nsXXXCharset.cpp) that should
return canonical names, but the old code used the resolution. If you
want to be safe, we can just use non-raw versions.
In nsPluginHostImpl.cpp and nsPrefMigration.cpp, the alias
resolution(charsetAtom) is not used for platformCharset in the old code.
There are several other cases in profile/src/nsProfileAccess.cpp and
in widget/src/*. (item 2b)
Index: modules/plugin/base/src/nsPluginHostImpl.cpp
- rv = ccm->GetUnicodeDecoder(&platformCharset, aUnicodeDecoder);
+ rv = ccm->GetUnicodeDecoder(platformCharset.get(),
Index: profile/pref-migrator/src/nsPrefMigration.cpp
- rv = ccm->GetUnicodeDecoder(&aCharset, getter_AddRefs(decoder));
+ rv = ccm->GetUnicodeDecoder(aCharset, getter_AddRefs(decoder));
@@ -2479,8 +2473,9 @@
...
- nsAutoString charSet;
+ nsCAutoString charSet;
rv = GetPlatformCharset(charSet);
....
@@ -2504,7 +2499,7 @@
...
// A wrapper function to call the interface to get a platform file charset.
nsresult
-nsPrefConverter::GetPlatformCharset(nsAutoString& aCharset)
+nsPrefConverter::GetPlatformCharset(nsCString& aCharset)
....
rv = platformCharset->GetCharset(kPlatformCharsetSel_4xPrefsJS, aCharset);
Below is an example case where the old code doesn't use charset alias
resolution. Just using raw version wouldn't introduce a new bug here.
Index: layout/html/forms/src/nsIsIndexFrame.cpp
- nsAutoString charset;
+ nsCAutoString charset;
@@ -530,7 +532,7 @@
- rv = ccm->GetUnicodeEncoder(&charset, encoder);
+ rv = ccm->GetUnicodeEncoder(charset.get(), encoder);
Index: intl/uconv/src/nsTextToSubURI.cpp
I'm not sure why charsets are not resolved in two cases while it's resolved
via charsetAtom in one case, here. I'll CC to nhotta. By using non-raw version
where charsetAtom is not used in the current code, we may be fixing a bug
or we may be introducing an unnecessary overhead.
- rv = ccm->GetUnicodeEncoder(&charsetStr, &encoder);
+ rv = ccm->GetUnicodeEncoder(charset, &encoder);
@@ -139,9 +137,8 @@
- rv = ccm->GetUnicodeDecoder(&charsetStr, &decoder);
+ rv = ccm->GetUnicodeDecoder(charset, &decoder);
@@ -200,17 +197,13 @@
- rv = charsetConverterManager->GetUnicodeDecoder(charsetAtom,
+ rv = charsetConverterManager->GetUnicodeDecoder(aCharset.get(),
Index: dom/src/base/nsGlobalWindow.cpp
@@ -3487,7 +3487,8 @@
- result = ccm->GetUnicodeEncoder(&charset, getter_AddRefs(encoder));
+ result = ccm->GetUnicodeEncoder(NS_LossyConvertUCS2toASCII(charset).get(),
@@ -3584,7 +3585,8 @@
- result = ccm->GetUnicodeDecoder(&charset, getter_AddRefs(decoder));
+ result = ccm->GetUnicodeDecoder(NS_LossyConvertUCS2toASCII(charset).get(),
Here (and in other places) I think we're safe to call Get...Raw (item 2b)
Index: intl/uconv/tests/TestUConv.cpp
...
- // test alias resolving capability
- nsAutoString csAlias(NS_LITERAL_STRING("iso-10646-ucs-basic"));
- nsAutoString csName(NS_LITERAL_STRING("UTF-16BE"));
- res = ccMan->GetCharsetAtom(csAlias.get(), getter_AddRefs(csAtom));
....
-
- // test self returning if alias was not found
- nsAutoString csAlias2(NS_LITERAL_STRING("Totally_dummy_charset_name"));
I guess we need to keep alias resolving test. Now that charsetAtom
is gone, we can use GetPrefered() for testing alias resolution, instead.
Index: intl/uconv/tests/convperf.cpp
@@ -130,8 +130,7 @@
- res = ccMain->GetUnicodeDecoder(&str, &decoder);
+ res = ccMain->GetUnicodeDecoder(argv[tocodeind], &decoder);
@@ -142,8 +141,7 @@
- res = ccMain->GetUnicodeEncoder(&str, &encoder);
+ res = ccMain->GetUnicodeEncoder(argv[fromcodeind], &encoder);
I guess convperf.cpp is called multiple times(possibly hundreds of times)
by a script (that provides the canonical names) for perf. estimate so that
we have to skip the alias resolution (by using 'raw' version) as is done
in the old code.
Index: content/base/src/nsHTMLContentSerializer.h
NS_IMETHOD Init(PRUint32 flags, PRUint32 aWrapColumn,
- nsIAtom* aCharSet, PRBool aIsCopying);
+ const char* aCharSet, PRBool aIsCopying);
....
- nsCOMPtr<nsIAtom> mCharSet;
+ nsCString mCharSet;
One problem with removing charsetAtom is that we lose the ability to
tell whether 'charset' is canonical or not.
I think it's important to update the documentation of interfaces and related
methods where we replace 'nsCOMPtr<nsIAtom> mCharSet' with 'nsCString
mCharset'. For instance, nsIContentSerializer
(http://lxr.mozilla.org/seamonkey/source/content/base/public/nsIContentSerializer.h#61)
should make it clear that mCharset(charset attribute) must be canonical
and Init() must resolve aCharset with GetPrefered() before storing it in
mCharset. Implementations of nsIContentSerializer have to be updated to
reflect this. Consumers of 'mCharset' can just call non-raw version of
GetUnicode(En|De)coder, but in that case we end up resolving aliases every
time it's called instead of just once in Init().
Alternatively, we can mandate that nsIContentSerializer::Init() be always
called with a resolved charset.
Index: content/html/style/src/nsCSSLoader.cpp
@@ -382,7 +382,7 @@
static nsresult GetCharsetFromData(const unsigned char* aStyleSheetData,
PRUint32 aDataLength,
- nsAString& aCharset)
+ nsACString& aCharset)
......
result = ResolveCharset(charsetCandidate, charset);
......
+ aCharset = charset;
Why do you keep ResolveCharset()? According to bz,
nsCSSLoader.cpp doesn't care whether it's resolved or not. Then, we can
remove it, can't we?
Index: gfx/src/mac/nsMacUnicodeFontInfo.cpp
- rv = gCharsetManager->GetCharsetAtom(value.get(), getter_AddRefs(charset));
- rv = gCharsetManager->GetUnicodeEncoder(charset, aConverter);
+ rv = gCharsetManager->GetUnicodeEncoder(value.get(), aConverter);
charset names come from gfx/src/mac/fontEncoding.properties file where
all charsets are canonical.
Index: gfx/src/ps/nsPostScriptObj.cpp
+ if (NS_SUCCEEDED(res)) {
+ res = ccMain->GetUnicodeEncoderRaw(psnativecode.get(), &linfo->mEncoder);
Sorry I was wrong here. At the moment, the sample user pref. shipped
(gfx/src/ps/sample.unixpsfonts.properties) has 'euc-jp' (instead of
canonical 'EUC-JP'). I'll file a new bug after this patch is landed.
In the meantime, non-raw version has to be used here.
Index: widget/src/gtk/nsClipboard.cpp
@@ -1420,7 +1417,7 @@
- rv = ccm->GetUnicodeDecoder(&charset, getter_AddRefs(decoder));
+ rv = ccm->GetUnicodeDecoder(charset.get(), getter_AddRefs(decoder));
Index: widget/src/gtk2/nsClipboard.cpp
- rv = ccm->GetUnicodeDecoder(&charset, getter_AddRefs(decoder));
+ rv = ccm->GetUnicodeDecoder(charset.get(), getter_AddRefs(decoder));
Index: widget/src/os2/nsFilePicker.cpp
if (NS_SUCCEEDED(rv)) {
- rv = ccm->GetUnicodeEncoder(&fileSystemCharset, &mUnicodeEncoder);
+ rv = ccm->GetUnicodeEncoder(fileSystemCharset.get(), &mUnicodeEncoder);
@@ -553,13 +553,13 @@
..
- rv = ccm->GetUnicodeDecoder(&fileSystemCharset, &mUnicodeDecoder);
+ rv = ccm->GetUnicodeDecoder(fileSystemCharset.get(), &mUnicodeDecoder);
Index: xpfe/components/bookmarks/src/nsBookmarksService.cpp
- rv = charsetConv->GetUnicodeDecoder(&defaultCharset,
getter_AddRefs(mUnicodeDecoder));
+ rv = charsetConv->GetUnicodeDecoder(defaultCharset.get(),
Index: mailnews/import/src/nsImportService.cpp
- rv = ccm2->GetUnicodeDecoder(charsetAtom, &m_pDecoder);
+ rv = ccm2->GetUnicodeDecoder(m_sysCharset.get(), &m_pDecoder);
I think we can use ...Raw here, but can leave it alone to be safe. (item 2c in
the summary)
Assignee | ||
Comment 27•21 years ago
|
||
ok, here's a cumulative patch which includes all changes so far, as well as
jshin's patch (Sent over e-mail) which changed more consumers over to the
get*Raw() versions, and includes the MsgComposeCommands.js that brings the
compose window back from the dead.
Attachment #124846 -
Attachment is obsolete: true
Attachment #124870 -
Attachment is obsolete: true
Assignee | ||
Comment 28•21 years ago
|
||
Comment on attachment 124938 [details] [diff] [review]
remove nsIAtom v2.1
ok, again looking for an r=smontagu, r=jshin
It sounds like people want to know what's going on with nsCharsetMenu.cpp too..
I'll explain that in the next comment.
Attachment #124938 -
Flags: review?(smontagu)
Comment 29•21 years ago
|
||
Index: mailnews/base/util/nsMsgUtf7Utils.cpp
.....
- res = ccm->GetUnicodeEncoder(&aCharset, &encoder);
+ res = ccm->GetUnicodeEncoder("x-imap4-modified-utf7", &encoder);
It seems like my patch for 'Raw-love' didn't get applied :-)
TestUConv.cpp still doesn't test the alias resolution with GetPrefered(). And
how about removing ResolveCharset() in nsCSSLoader.cpp?
Assignee | ||
Comment 30•21 years ago
|
||
what the heck.. I thought it went on ok. I'll reapply it.. good thing you
double-checked!
Assignee | ||
Comment 31•21 years ago
|
||
what the heck.. I thought it went on ok. I'll reapply it.. good thing you
double-checked!
Assignee | ||
Comment 32•21 years ago
|
||
ok, one more try here. this includes
- update against the trunk to catch the nsCharsetMenu.cpp changes
- jshin's patch that he sent to me to use more Raw() methods
- MsgComposeCommands.js changes
- change from ccm2 to ccm in a bunch of places.
Assignee | ||
Updated•21 years ago
|
Attachment #124938 -
Attachment is obsolete: true
Comment 33•21 years ago
|
||
alecf, thank you for taking care of this beast. r=jshin
as we talked off-line, I'll hunt for more spots for 'Raw' after this gets landed.
Comment 34•21 years ago
|
||
more bustage... sorry alecf :/
this fixes firebird's forked nsBookmarksService.cpp. I did this by blindly
applying the changes to mozilla's version, so please double-check I did the
right thing.
also removes a spurious #include I noticed (you had two of the exact same
#include in that file)
thx for fixing message compose, it works now. :)
Comment 35•21 years ago
|
||
So the converter input stream calls GetUnicodeDecoder, which now does alias
resolution? Again, could we remove all the code in nsCSSLoader that is not
needed as a result?
Comment 36•21 years ago
|
||
bz, let's lift this behemoth patch off alecf's shoulder :-) While
bugzilla was down (6 - 7 hrs ago), I told alecf that I'd take care of
nsCSSLoader.cpp and TestUconv.cpp after the current patch gets landed. Sorry I
forgot to mention that in my prev. comment.
Assignee | ||
Comment 37•21 years ago
|
||
Comment on attachment 124846 [details] [diff] [review]
remove nsIAtom v2
(cancelling review requests on old patches)
Attachment #124846 -
Flags: review?(smontagu)
Assignee | ||
Comment 38•21 years ago
|
||
Comment on attachment 124994 [details] [diff] [review]
fix firebird bustage from v2.11 patch
sr=alecf on this. Looks like you did the right thing.
Attachment #124994 -
Flags: superreview+
Assignee | ||
Comment 39•21 years ago
|
||
Comment on attachment 124982 [details] [diff] [review]
remove nsIAtom v2.11
oops, for got to request a new review from smontagu.
Simon, can you review this? it already has r=jshin and he's found lots of good
stuff, but I'd like a second set of i18n eyes on this just to make 100% sure.
I'd really like to land this soon, please find me on IRC if you have any
questions...
Attachment #124982 -
Flags: review?(smontagu)
Comment 40•21 years ago
|
||
Comment on attachment 124982 [details] [diff] [review]
remove nsIAtom v2.11
I've been right through this at last and it looks OK, except what I said before
about the comments in intl/uconv/idl/nsICharsetConverterManager.idl which you
have left unchanged so they are now not true:
+ * Here Charsets are indentified by nsIAtom's. I know, we could have our own
+ * interface for charsets (something like nsICharacterSet). But for now, I
+ * will attempt to use Atom's.
r=smontagu
Attachment #124982 -
Flags: review?(smontagu) → review+
Assignee | ||
Comment 41•21 years ago
|
||
Comment on attachment 124982 [details] [diff] [review]
remove nsIAtom v2.11
oops! yeah, I'll fix the MPL-trilicense stuff and the comments before landing.
Simon (fraser) - I have r=jshin and r=smontagu, both strong i18n reviewers, and
we've gone a few rounds finding the right patch. do you mind giving this a
quick once-over?
Attachment #124982 -
Flags: superreview?(sfraser)
Comment 42•21 years ago
|
||
Comment on attachment 124982 [details] [diff] [review]
remove nsIAtom v2.11
rs=sfraser
Attachment #124982 -
Flags: superreview?(sfraser) → superreview+
Comment 43•21 years ago
|
||
There are a couple of spots missed in intl/ctl/src. The diff for
intl/ctl/src/nsUnicodeToTIS620.cpp in attachment 124982 [details] [diff] [review] has to be replaced with
this (with diffs for two files in intl/ctl/src).
Assignee | ||
Comment 44•21 years ago
|
||
ok! this has landed, thanks reviewers.. looks like we got a minor codesize
improvement (~5k on most platforms)
I think I see minor Ts/Tp performance improvements too, though it may be too
early to tell.
Comment 45•21 years ago
|
||
Comment on attachment 124938 [details] [diff] [review]
remove nsIAtom v2.1
removing obsolete review request
Attachment #124938 -
Flags: review?(smontagu)
Comment 46•21 years ago
|
||
This checkin have added a "may be used uninitialized" warning to brad Tbox:
+gfx/src/gtk/nsFontMetricsGTK.cpp:1372
+ `nsresult res' might be used uninitialized in this function
Indeed, the code in the nsFontMetricsGTK::Init has form:
nsresult res;
...
if (!gInitialized) {
res = ...
...
}
...
if (mLangGroup) {
...
res = ...
...
}
if (mLangGroup.get() == gUserDefined) {
res = ...
...
}
else {
return res
}
If none of the "if" conditions are true, the function will return an
uninitialized nsresult.
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/gfx/src/gtk/nsFontMetricsGTK.cpp&rev=&cvsroot=/cvsroot&rev=1.255&mark=1410,1435,1454-1456#1410
P.S. Should this function be even calling mLangGroup.get() when it is not even
sure mLangGroup is non-NULL???
Comment 47•21 years ago
|
||
Is anyone looking at the "speedracer" tinderbox bustage
(http://tinderbox.mozilla.org/showbuilds.cgi?tree=SeaMonkey-Ports) ?
Comment 48•21 years ago
|
||
Line 361 of nsCharsetConverterManager.cpp appears to declare an unused variable.
Assignee | ||
Comment 49•21 years ago
|
||
I don't see the warning/unused variable in nsCharsetConverterManager.cpp?
(though I fixed the one in gfx)
Comment 50•21 years ago
|
||
I'd like to submit this patch to the mozdev spell checker folks to fix the
spell checker if Alec says my fix is correct.
Assignee | ||
Comment 51•21 years ago
|
||
Comment on attachment 125527 [details] [diff] [review]
patch for the mozdev spell checker
you are correct, sir... though personally I'd change "mCharset" to be an
nsCString rather than an nsString, then you can avoid all temporaries and
conversions...
(and if you do still have conversions, take a look at bug 209220 for the next
set of conversions - that one fixes APIs on basically all of
gecko/docshell/editor/etc...)
Assignee | ||
Comment 52•21 years ago
|
||
oops, I should mark this fixed now, huh.
Status: ASSIGNED → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
Comment 53•21 years ago
|
||
> oops, I should mark this fixed now, huh.
Should I file a new bug for comment #46, then?
Comment on attachment 124982 [details] [diff] [review]
remove nsIAtom v2.11
>Index: xpfe/bootstrap/nsAppRunner.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/xpfe/bootstrap/nsAppRunner.cpp,v
>retrieving revision 1.398
>diff -u -r1.398 nsAppRunner.cpp
>--- xpfe/bootstrap/nsAppRunner.cpp 22 Apr 2003 00:20:24 -0000 1.398
>+++ xpfe/bootstrap/nsAppRunner.cpp 5 Jun 2003 07:40:57 -0000
>@@ -1549,8 +1549,11 @@
> if (HandleDumpArguments(argc, argv))
> return 0;
>
>+ printf("Checking for trace malloc..\n");
> #ifdef NS_TRACE_MALLOC
>+ printf("Trace malloc: %d args\n", argc);
> argc = NS_TraceMallocStartupArgs(argc, argv);
>+ printf("Trace malloc: now have %d args\n", argc);
> #endif
>
> #if defined(MOZ_WIDGET_GTK) || defined(MOZ_WIDGET_GTK2)
I'm guessing you didn't mean to check this bit in, especially since the first
printf happens in a non-debug build. I just undid this change.
Also, there were some atom leaks that showed up on tinderbox as a result of
this change.
Assignee | ||
Comment 55•21 years ago
|
||
dbaron: thanks for cleaning up after me - I had the printfs removed in my tree
but I was on vacation until today
I have a fix for the atom leaks, that I'll attach here soon.
Comment 56•21 years ago
|
||
This is an additional patch for nsCSSLoader.cpp mentioned in comment #35 (and a
couple of test programs in intl/uconv/test). Let me know if this had better be
dealt with in a new bug.
Comment 57•21 years ago
|
||
Comment on attachment 125746 [details] [diff] [review]
extra patch (mentioned in comment #35)
r+sr=me on the nsCSSLoader changes. I don't know enough about the test app to
review those changes, but you should probably be checking the return values of
all the calls in addition to the strings they return...
Assignee | ||
Comment 58•21 years ago
|
||
Comment on attachment 125746 [details] [diff] [review]
extra patch (mentioned in comment #35)
sr=alecf - this bug is fine, other cleanup work is going here too
Attachment #125746 -
Flags: superreview+
Comment 59•21 years ago
|
||
Thanks for r/sr.
nsCSSLoader.cpp got checked in.
I added the return value checking to TestUConv.cpp, but haven't landed it yet
because I thought it might as well go with a few other extra changes I'm
planning to do (cases in which I was '98%' sure we could use 'Raw' version, but
left out to be safe)
Comment 60•21 years ago
|
||
Sorry, I mistyped my previous comment - the declaration of the unused variable
nsAutoString pref; is on line 381 of nsCharsetConverterManager.cpp, not 361.
Attachment #126724 -
Flags: review?(alecf)
Assignee | ||
Comment 62•21 years ago
|
||
Comment on attachment 126724 [details] [diff] [review]
fix atom leaks
argh, sorry I had this in my tree r/sr=alecf
Attachment #126724 -
Flags: review?(alecf) → review+
Attachment #126724 -
Flags: superreview?(bryner)
Updated•21 years ago
|
Attachment #126724 -
Flags: superreview?(bryner) → superreview+
Comment on attachment 126724 [details] [diff] [review]
fix atom leaks
Leak fix checked in to trunk, 2003-06-30 14:50 -0700.
You need to log in
before you can comment on or make changes to this bug.
Description
•