Closed Bug 382710 Opened 13 years ago Closed 8 years ago

Atomization of languages


(Core :: Graphics, defect)

Not set





(Reporter: robarnold, Unassigned)



(2 files)

References to langGroups in gfx/ use nsIAtom wherever possible.
Attached patch Proposed patchSplinter Review
Looks like you're passing null in some places and dereferencing (assuming non-null) in others.  You should probably pick one of the two :-)
Comment on attachment 266823 [details] [diff] [review]
Proposed patch

>Index: gfx/thebes/public/gfxFont.h
>     gfxFontStyle(PRUint8 aStyle, PRUint16 aWeight, gfxFloat aSize,
>-                 const nsACString& aLangGroup,
>+                 nsIAtom* aLangGroup,

Please use 'nsIAtom *aLangGroup' not nsIAtom*  (there are about 5 other places you do this.. please fix)

>Index: gfx/thebes/src/gfxFont.cpp
>     nsAutoString family;
>     nsCAutoString lcFamily;
>     nsAutoString genericFamily;
>-    nsCAutoString lang(aLangGroup);
>-    if (lang.IsEmpty())
>-        lang.Assign("x-unicode"); // XXX or should use "x-user-def"?
>+    nsCAutoString lang;
>+    aLangGroup->ToUTF8String(lang);
>+    NS_ASSERTION(!lang.IsEmpty(), "Cannot have empty language");
>+    //if (lang.IsEmpty())
>+    //    lang.Assign("x-unicode"); // XXX or should use "x-user-def"?

if this can be null (you'd have to check the callers) then we need to set lang to x-unicode as we did before.

>-    if (langGroup.IsEmpty()) {
>+    if (langGroup == nsnull) {
>         NS_WARNING("empty langgroup");
>-        langGroup.Assign("x-western");
>+        langGroup = NS_NewPermanentAtom("x-western");
>     }
> }

Just use (!langGroup).  I'm not sure that we want to use permanent atoms -- nothing else in the tree seems to use them which may result in us having 2 different atoms of the same name if they use separate tables.  If they use the same atom table then we should use them here and in the other places.
Component: GFX → GFX: Thebes
QA Contact: general → thebes
Permanent atoms don't use separate tables -- they make the atom stop being reference counted.  (The probably they were originally intended for is now solved by static atoms; there may have been discussion of removing them from the tree, but language groups seem a pretty good use for them.)
Er, "The problem they were originally ..."

That said, if the language groups are all *already* in an atom list, then they'll be static atoms, which is even better.
Attached patch Patch round 2Splinter Review
Issues with NS_NewAtom probably not resolved
(In reply to comment #5)
> That said, if the language groups are all *already* in an atom list, then
> they'll be static atoms, which is even better.

We don't want to use permanent atoms for anything a webpage can introduce; that leads to a permanent memory leak.

The correct way to deal with your x-unicode and x-western atoms is to introduce a call to NS_RegisterStaticAtoms somewhere in gfx initialization, then use the returned pointers whenever you need those atoms.

It's not really a good idea to pass around a null atom pointer unless you really need it; pick some reasonable default to pass in, or use an atom for the empty string (it's nsGkAtoms::_empty in gklayout, although you can't use that from GFX.)
How's this going, is there a new patch coming up Rob? The warnings in bug 385516 are a real pain to anyone who is trying to actually use the console output. 

If this bug will take a long time to fix, we should just decide to remove the warnings now.
Fixed as part of bug 524107.
Closed: 8 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 524107
You need to log in before you can comment on or make changes to this bug.