382710 - Atomization of languages

Status: RESOLVED DUPLICATE of bug 524107

(Core :: Graphics, defect)

Description

[Rob Arnold [:robarnold]]

References to langGroups in gfx/ use nsIAtom wherever possible.

Comment 1

[Rob Arnold [:robarnold]]

Attachment: Proposed patch

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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 3

[Stuart Parmenter]

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.

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.)

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 6

[Rob Arnold [:robarnold]]

Attachment: Patch round 2

Issues with NS_NewAtom probably not resolved

Comment 7

[Eli Friedman]

(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.)

Comment 8

[Håkan Waara]

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.

Comment 9

[JP Rosevear [:jpr]]

Fixed as part of bug 524107.