385516 - Console is spewed with WARNING: empty langgroup

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Adam Guthrie]

I get this warning all the time:

WARNING: empty langgroup

IIRC this was introduced when the patch for bug 333357 landed, which started assigning system fonts a style (or something).

Can we just remove this warning, or do we need to really assign the system font a language group somehow?

Comment 1

[Rob Arnold [:robarnold]]

While creating the patch for bug 382710, I came across what I'm pretty sure is
the only source for these warnings.

nsThebesDeviceContext::GetSystemFont calls the gfxFontStyle with an empty
langgroup; all other code gets the langgroup from an nsIAtom, so those strings
should be non-empty.

A simple fix is to change nsThebesDeviceContext::GetSystemFont to use the same
langgroup that gets substituted anyways, but I don't know if that is a correct
solution.

Comment 3

[Scott MacGregor]

dbaron / pavlov, if one of you says it's ok to remove the warning or to not pass in an empty string as th elanggroup in nsThebesDeviceContext::GetSystemFont, then I'm happy to make a patch to do that. I'm getting a fair number of these warnings dumped to the console that it's making it hard for me to see other problems getting reported...

Comment 4

[Stuart Parmenter]

rob arnold was trying to make us find the right langgroup in bug 382710.  it would be nice to always have *a* langgroup.

Comment 5

[Peter Weilbacher]

Bug 416915 seems to have a patch for this, but I didn't try it yet.

Comment 7

[Sebastian Redl]

Attachment: Add default constructor to gfxFontStyle and use it.

There are three places that call gfxFontStyle's constructor with an empty langGroup: nsThebesDeviceContext::GetSystemFont, and the constructors of BeOS and GTK2 nsSystemFonts*. All three just want an empty gfxFontStyle to use as an out parameter.

The attached patch gives gfxFontStyle a default constructor and makes these three places use it instead of the full constructor. This is, I think, a superior approach to passing an explicit langGroup (not to mention all the other parameters) on construction, as I did in bug 416915.

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Why you use "x-western" for system fonts? Shouldn't we use system locale?

Comment 9

[Peter Weilbacher]

I would think so, too. That's what I tried to do with the code snipped in bug 416915 comment 7 but I couldn't get it to compile (some error about string methods?!). And it would also require to change from nsFont to gfxFont in GetSystemFont.

Comment 10

[Sebastian Redl]

It doesn't matter, as the langgroup in the font style object is never used anyway.

Comment 11

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #10)
> It doesn't matter, as the langgroup in the font style object is never used
> anyway.

Why? If so, why the WARNING appears??

Comment 12

[Sebastian Redl]

Sorry, I was unclear.

The font style object is fully capable of supporting langgroups and thus warns if it is constructed with an empty one. That's the warning that appears.

However, the only code that currently makes use of gfxFontStyle is the nsThebesDeviceContext class, which takes the information and populates a gfxFont with it. If you look at the gfxFont class [1], though, you'll find that it doesn't have a field for the langgroup, so the langgroup from the style object is never copied over.

As such, it is not important that the field is never filled in the first place. The issue here is not that - it's probably a bug, but not this bug. This bug is about constructing gfxFontStyle objects as out parameters and getting the console cluttered with warnings because an empty langgroup is passed, which the fully-initializing constructor doesn't like.

A better place for actually finding and using a langgroup is probably bug 382710, which is headed in that direction.

[1] http://mxr.mozilla.org/firefox/source/gfx/thebes/public/gfxFont.h#355

Comment 13

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Sounds ok. Thank you for your explanation.

Comment 14

[Damon Sicore (:damons)]

Comment on attachment 304775 [details] [diff] [review]
Add default constructor to gfxFontStyle and use it.

a1.9+=damons

Comment 15

[Reed Loden [:reed]]

Checking in gfx/src/thebes/nsSystemFontsBeOS.cpp;
/cvsroot/mozilla/gfx/src/thebes/nsSystemFontsBeOS.cpp,v  <--  nsSystemFontsBeOS.cpp
new revision: 1.5; previous revision: 1.4
done
Checking in gfx/src/thebes/nsSystemFontsGTK2.cpp;
/cvsroot/mozilla/gfx/src/thebes/nsSystemFontsGTK2.cpp,v  <--  nsSystemFontsGTK2.cpp
new revision: 1.14; previous revision: 1.13
done
Checking in gfx/src/thebes/nsThebesDeviceContext.cpp;
/cvsroot/mozilla/gfx/src/thebes/nsThebesDeviceContext.cpp,v  <--  nsThebesDeviceContext.cpp
new revision: 1.70; previous revision: 1.69
done
Checking in gfx/thebes/public/gfxFont.h;
/cvsroot/mozilla/gfx/thebes/public/gfxFont.h,v  <--  gfxFont.h
new revision: 1.100; previous revision: 1.99
done
Checking in gfx/thebes/src/gfxFont.cpp;
/cvsroot/mozilla/gfx/thebes/src/gfxFont.cpp,v  <--  gfxFont.cpp
new revision: 1.86; previous revision: 1.85
done