[Bug 449356] Refactor gfxPangoFontGroup for user fonts

[bugzilla-daemon at mozilla.org]

Do not reply to this email.  You can add comments to this bug at
https://bugzilla.mozilla.org/show_bug.cgi?id=449356





--- Comment #9 from Robert O'Callahan (:roc) (Mozilla Corporation) <roc at ocallahan.org>  2008-10-21 17:04:45 PDT ---
+    nsTArray<FontSetByLangEntry> mFontSets;

This usually contains one element, right? You probably should make it
nsAutoTArray<FontSetByLangEntry,1>.

nsStringArray looks a lot worse than nsTArray<nsString>. I filed bug 461047 on
that.

+    // mLangStrings owns the memory for keys to mBestLangSupportTable and
+    // mFontsByLang.  The language support and the fonts for each lang are in
+    // separate tables because mFontsByLang is expected to be used only when
+    // no default fonts support the language.  There would be a large number
+    // of fonts in entries for languages using Latin script but these do not
+    // need to be created because default fonts already support these
+    // languages.

As we discussed, let's combine the tables into one and just allow the font list
to be missing from some entries.

Seems like gfxRefPtrWrap should go into XPCOM and be split into "refcounted
object" and "custom-destructor object". How about nsRefObj/nsAutoObj? (The
template parameter doesn't need to be a pointer, after all --- nsAutoObj would
be useful for HDCs, for example).

+    class familyComparator {
+    class langComparator : familyComparator {

FamilyComparator, LangComparator

+        if (existingFamilies.Contains(family, familyComparator())) {
+            continue;
+        }

Doesn't this get a bit slow? Maybe we should make existingFamilies a hash
table?

+            for (PRUint32 l; l < findLangMatch.Length(); ++l) {
+    for (PRUint32 l; l < findLangMatch.Length(); ++l) {

Don't call variables 'l', it's hard to distinguish from '1'

+            if(FcFontSetAdd(fontSet, font)) {

"if (" (3 occurrences)

+        while (!mFcFontSet) {
+            if (mHaveFallbackFonts)
+                return nsnull;
+
+            SortFallbackFonts();
+            mHaveFallbackFonts = PR_TRUE;
+        }

Why is this a 'while' and not an 'if'?

+    gfxRefPtrWrap<FcPattern> mSortPattern;
+    gfxRefPtrWrap<FcFontSet> mFcFontSet;
+    gfxRefPtrWrap<FcCharSet> mCharSet;
+    nsTArray<FontEntry> mFonts;
+    int mFcFontsTrimmed;
+    PRPackedBool mHaveFallbackFonts;

Comment these please.

+        if (mFcFontsTrimmed == mFcFontSet->nfont) {
+            // finished with mFcFontSet
+            mFcFontSet.Reset();
+            mFcFontsTrimmed = 0;

Why reset to 0? That just seems confusing.

+    const cairo_font_options_t *options =
+        gdk_screen_get_font_options(gdk_screen_get_default());

If we're not compiling with system cairo, this might be a problem.

-- 
Configure bugmail: https://bugzilla.mozilla.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.