Console is spewed with WARNING: empty langgroup

RESOLVED FIXED in mozilla1.9beta5

Status

()

Core
Graphics
RESOLVED FIXED
11 years ago
10 years ago

People

(Reporter: Adam Guthrie, Assigned: Sebastian Redl)

Tracking

Trunk
mozilla1.9beta5
x86
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment)

(Reporter)

Description

11 years ago
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
solution.
Duplicate of this bug: 386683

Comment 3

11 years ago
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

11 years ago
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

11 years ago
Bug 416915 seems to have a patch for this, but I didn't try it yet.
(Assignee)

Updated

11 years ago
Duplicate of this bug: 416915
(Assignee)

Comment 7

11 years ago
Created attachment 304775 [details] [diff] [review]
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.
Why you use "x-western" for system fonts? Shouldn't we use system locale?

Comment 9

11 years ago
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.
(Assignee)

Comment 10

11 years ago
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??
(Assignee)

Comment 12

11 years ago
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
Sounds ok. Thank you for your explanation.

Updated

10 years ago
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.

a1.9+=damons
Attachment #304775 - Flags: approval1.9? → approval1.9+
Keywords: checkin-needed
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
Status: NEW → RESOLVED
Last Resolved: 10 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.