[Bug 458169] implement downloadable font support on Linux

[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=458169





--- Comment #4 from Robert O'Callahan (:roc) (Mozilla Corporation) <roc at ocallahan.org>  2008-11-24 00:57:56 PST ---
Basically looks great to me.

+     * Ownership of the returned gfxFontEntry is passed to the caller,
+     * who must either AddRef() or delete.

Should these functions be changed (in a separate patch) to return
already_AddRefed?

+    // Helper function to be called from InitPattern() to change the pattern
+    // so that it matches the CSS style descriptors and so gets properly
+    // sorted in font selection.  This also avoids synthetic style effects
+    // being added by the renderer when the style of the font itself does not
+    // match the descriptor provided by the author.
+    void ConformPattern();

I'd like a better name here. Perhaps "AdjustPatternToCSS"?

+ * gfxWebFcFontEntry:

I think gfxDownloadedFcFontEntry would be a better name.

+    FcPatternGetInteger(mPattern, FC_WEIGHT, 0, &fontWeight);
+    int cssWeight = gfxFontconfigUtils::FcWeightForBaseWeight(mWeight);
+    if (cssWeight != fontWeight) {
+        FcPatternDel(mPattern, FC_WEIGHT);
+        FcPatternAddInteger(mPattern, FC_WEIGHT, cssWeight);
+    }

Is there a reason not to do Del/Add unconditionally here? Ditto for FC_SLANT
and setting FC_FULLNAME.

+        PRUint8 savedStyle = aStyle.style;
+        aStyle.style = FONT_STYLE_NORMAL;
+        fontEntry = static_cast<gfxFcFontEntry*>
+            (mUserFontSet->FindFontEntry(utf16Family, aStyle, needsBold));
+        aStyle.style = savedStyle;

This is yuck. Can we make aStyle a const reference and just use a temporary
copy here?

+            // User fonts are already filtered by slant (but not size) in
+            // mUserFontSet->FindFontEntry().

Aren't you working around that by retrying FindFontEntry with
FONT_STYLE_NORMAL, in FindFontPattern?

+    if (!(list->Contains(fontName))) {

Lose unnecessary parens.

+PRBool
+gfxPlatformGtk::IsFontFormatSupported(nsIURI *aFontURI, PRUint32 aFormatFlags)
+{
+    // reject based on format flags
+    if (aFormatFlags & (gfxUserFontSet::FLAG_FORMAT_EOT |
gfxUserFontSet::FLAG_FORMAT_SVG)) {
+        return PR_FALSE;
+    }

Can we avoid blacklisting and write this code to just return true for the
formats we know we can support?

jdaggett, do you want to review this too?

-- 
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.