If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Gecko & friends should store/access charsets as ASCII




15 years ago
15 years ago


(Reporter: Alec Flett, Assigned: Alec Flett)


Windows 2000

Firefox Tracking Flags

(Not tracked)



(1 attachment)



15 years ago
now that bug 206379 is fixed, i18n APIs take ASCII charset names directly. This
means that aside from frozen DOM interfaces, we have no reason to have unicode
charset names... 

So I started by tweaking nsIDocument, which has Get/SetDocumentCharacterSet()
and changed them to take nsACString, and then watched the effects ripple out
from there. I ended up changing a ton of interfaces all over the parser, layout,
docshell, intl, etc.. but the effects were fantastic - huge reductions in calls
to NS_LossyConvertUCS2toASCII, elimination of temporary variables,
AssignWithConversion, you name it...

In general, I switched most interfaces over to nsACString, but a few of them
stayed as |const char*| for simplicity of changes... 

Patch forthcoming... it touches over 70 files, but may of the changes are just
+1/-1 so the review should be pretty quick.

Comment 1

15 years ago
Created attachment 125526 [details] [diff] [review]
use ASCII charset names

and here's the patch..

I'd like an r=someone good (jkeiser, bsmedberg, jshin, are all good candidates)
and sr=jst

the patch looks big but it is FAR simpler than it appears - much easier to
review than bug 206379

Comment 2

15 years ago
Comment on attachment 125526 [details] [diff] [review]
use ASCII charset names

requesting r=jkeiser, sr=jst but as mentioned above, I'll take an r= from other
folks as well... this is really straight forward stuff.
Attachment #125526 - Flags: superreview?(jst)
Attachment #125526 - Flags: review?(jkeiser)

Comment 3

15 years ago
Comment on attachment 125526 [details] [diff] [review]
use ASCII charset names

This is sweet, and totally worth the few places where you have to convert back
to Unicode.  Only one thing:

Index: dom/src/base/nsLocation.cpp
@@ -97,12 +97,8 @@ static +  nsCAutoString charset;
+  rv = doc->GetDocumentCharacterSet(aCharset);
   return rv;

The declaration of charset is no longer necessary.
Attachment #125526 - Flags: review?(jkeiser) → review+

Comment 4

15 years ago
(good catch. I've removed it in my tree)
Comment on attachment 125526 [details] [diff] [review]
use ASCII charset names

Nice, sr=jst!
Attachment #125526 - Flags: superreview?(jst) → superreview+

Comment 6

15 years ago
Wow, great. 
These three lines caught my eyes. Perhaps, a result of global replacement :-)

> Index: layout/html/base/src/nsImageFrame.h
> @@ -2746,11 +2746,11 @@ NS_IMETHODIMP nsPluginInstanceOwner::Get
> +      !nsCRT::strncmp(charset.get(), NS_LITERAL_CSTRING("UTF").get(), 3)) {

> Index: mailnews/base/src/nsMessenger.cpp

> @@ -555,7 +555,7 @@ nsMessenger::OpenURL(const char *aURL)
> +  SetDisplayCharset(NS_LITERAL_CSTRING("UTF-8").get());
> @@ -594,7 +594,7 @@ nsMessenger::LoadURL(nsIDOMWindowInterna
> +  SetDisplayCharset(NS_LITERAL_CSTRING("UTF-8").get());

Comment 7

15 years ago
good catch! I'll have those lines as raw strings when I go to check in.

Comment 8

15 years ago
fix is in! thanks everyone
Last Resolved: 15 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.