Unwanted argument to gfxFontStyle constructor hard-coded in some places.

RESOLVED DUPLICATE of bug 385516

Status

()

Core
Graphics
--
trivial
RESOLVED DUPLICATE of bug 385516
10 years ago
10 years ago

People

(Reporter: Sebastian Redl, Unassigned)

Tracking

Trunk
x86
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

10 years ago
User-Agent:       Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9b4pre) Gecko/2008021122 Firefox/3.0b4pre
Build Identifier: 

The constructor of gfxFontStyle in gfx/thebes/src/gfxFont.cpp receives the language group as a string argument. If this string is empty, it emits a warning (line 911) and sets the language group to "x-western".

Two places, nsSystemFontsGTK2 constructor in gfx/src/thebes/nsSystemFontsGTK2.cpp and nsThebesDeviceContext::GetSystemFont() in gfx/src/thebes/nsThebesDeviceContext.cpp, call this constructor with a hard-coded empty string as the language group argument. This leads to a lot of spurious warning in the debug output of the program.

These warnings should be removed, as it clutters debug output and may hide an actual problem, such as a language group setting getting lost. Either calling the constructor with an empty language group is acceptable, in which case the NS_WARNING should be removed, or it is not, in which case the calls should be modified to explicitly pass "x-western" (if that is actually appropriate). As it is, the warning is useless, because there's no way to distinguish between the benign, hard-coded arguments and an actual problem.

Reproducible: Always
(Reporter)

Comment 1

10 years ago
Created attachment 302716 [details] [diff] [review]
A patch to replace the hard-coded arguments with "x-western".

This patch follows the latter route and explicitly passes "x-western" to the constructor.

Comment 2

10 years ago
This is kind of a dupe of bug 385516 but as this has a patch I don't want to resolve as duplicate...

Sebastian, if you make this change in nsSystemFontsGTK2.cpp you should do the same thing in nsSystemFontsBeOS.cpp.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Version: unspecified → Trunk

Comment 3

10 years ago
The commented-out
   //aFont->langGroup = fontStyle.langGroup;
in nsThebesDeviceContext::GetSystemFont() looks suspicious to me, too. Especially as bug 333357 which introduced it doesn't give a reason.
(Reporter)

Comment 4

10 years ago
Created attachment 303750 [details] [diff] [review]
Updated patch, includes BeOS.

Update patch for BeOS file, too. I overlooked it at first.

As for the other, my guess is this: GetSystemFont() doesn't care about the fontStyle it creates. It's just an effectively default-constructed object (hey, maybe gfxFontStyle should have a default constructor) used as the target of gSystemFonts->GetSystemFonts()'s out parameter. It's filled with information there. gSystemFonts is of type nsSystemFonts[OS]. A short perusal of the code shows that none of these OS-specific GetSystemFonts()s actually yield useful information for the langgroup part. The GTK2 and BeOS versions hold their own gfxFontStyle objects, which are covered by my patch. The Windows, Mac and OS/2 versions actually get info from the OS, but none of them fill langGroup.

Thus, what dbaron did was preserve the langGroup that was already in the nsFont instead of overwriting it with the meaningless value in the gfxFontStyle.

Perhaps a comment should be added, pointing this out.
Attachment #302716 - Attachment is obsolete: true
(Reporter)

Comment 5

10 years ago
Preserving this here:

	<dbaron>	Well, not really
	<dbaron>	when I converted that code from nsFont to gfxFontStyle
	<dbaron>	I preserved what the old code did
	<dbaron>	that said, we don't have langGroup info in the system fonts
	<dbaron>	and I'm not entirely sure how we'd get it
	<dbaron>	although maybe it's easy on some platforms
	<dbaron>	(I suspect it actually probably is on linux.)
	<dbaron>	so really what we're doing now isn't ideal
	<dbaron>	and I don't think it was particularly intentional other than "don't change more than I'm trying to right now"

Comment 6

10 years ago
The old code in gfx/src/<platform>/ apparently used the GetLocaleLanguageGroup() method of nsILanguageAtomService to determine the language group. That is still available on trunk so I wonder if we should add it back in to the code in nsThebesDeviceContext::GetSystemFont if we detect that the langGroup was not set by the call to the platform function. Or add it to the platform GetSystemFont functions instead?

Comment 7

10 years ago
Hmm, I noticed that nsThebesDeviceContext::GetSystemFont returns the font info as an nsFont which doesn't even have a langGroup! That's why it was commented out. Otherwise I would have tried something like this:

    // try to replace the language group with something sensible
    nsCOMPtr<nsILanguageAtomService> langService;
    langService = do_GetService(NS_LANGUAGEATOMSERVICE_CONTRACTID);
    if (langService) {
        nsCOMPtr<nsIAtom> atom = langService->GetLocaleLanguageGroup(&rv);
        if (NS_SUCCEEDED(rv)) {
            // the language group is ASCII, so lossy should be fine
            fontStyle.langGroup = NS_LossyConvertUTF16toASCII(atom.ToString());
        }
    }

    rv = gSystemFonts->GetSystemFont(aID, &fontName, &fontStyle);

    if (fontStyle.langGroup.IsEmpty())
        fontStyle.langGroup = NS_LITERAL_CSTRING("x-western");
(Reporter)

Comment 8

10 years ago
From comment #4:
> (hey, maybe gfxFontStyle should have a default constructor)

I have a new patch with this approach, but I'll just file it in #385516 and mark this a dupe.
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 385516
You need to log in before you can comment on or make changes to this bug.