Closed Bug 385516 Opened 13 years ago Closed 12 years ago

Console is spewed with WARNING: empty langgroup


(Core :: Graphics, defect)

Not set





(Reporter: ispiked, Assigned: wasti.redl)





(1 file)

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?
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
Duplicate of this bug: 386683
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...
rob arnold was trying to make us find the right langgroup in bug 382710.  it would be nice to always have *a* langgroup.
Bug 416915 seems to have a patch for this, but I didn't try it yet.
Duplicate of this bug: 416915
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.
Attachment #304775 - Flags: review?(pavlov)
Why you use "x-western" for system fonts? Shouldn't we use system locale?
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.
It doesn't matter, as the langgroup in the font style object is never used anyway.
(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??
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.

Sounds ok. Thank you for your explanation.
Attachment #304775 - Flags: review?(pavlov) → review+
Assignee: nobody → wasti.redl
Attachment #304775 - Flags: approval1.9?
Comment on attachment 304775 [details] [diff] [review]
Add default constructor to gfxFontStyle and use it.

Attachment #304775 - Flags: approval1.9? → approval1.9+
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
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
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
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
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
Closed: 12 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9beta5
You need to log in before you can comment on or make changes to this bug.