[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 #5 from Karl Tomlinson (:karlt) <mozbugz at karlt.net>  2008-11-24 13:35:53 PST ---
(In reply to comment #4)
> +     * 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?

I think so.  I find it confusing to pass around references to objects that
have a reference count of zero.

> 
> +    // 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"?

That sounds fine.

> + * gfxWebFcFontEntry:
> 
> I think gfxDownloadedFcFontEntry would be a better name.

gfxDownloadedFcFontEntry has the advantage of indicating that the font has
already been downloaded.  And I guess src:local() faces would really be web
fonts too (but not downloaded fonts) because the family and style properties
are obtained from the web (even if the font data is not).

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

I'd like to leave these conditional.

For FC_WEIGHT, the only reason is to avoid the reallocation of memory for the
property value and the memmove back and forth of all the trailing properties
and pointers to their values.  The weight is expected to often (maybe usually)
already have the right value.

For FC_SLANT, there is the additional benefit of retaining the distinction
between italic and oblique where possible.

For FC_FULLNAME, if there is an existing value, then that is the best value as
it comes from the OpenType name table.  Appending style to family should only
be a fallback.

> +        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?

Yes, this is yuck.

Constructing a gfxFontStyle always requires a memory allocation because it has
an nsCString member, which is always forced to be non-empty, even though it
doesn't get used here.

What I think would look nicest here would be to change the FindFontPattern()
gfxFontStyle argument to thebes style and weight arguments.  That would avoid
the new gfxFontStyle allocation in SortPreferredFonts, and confine all the
gfxFontStyle yuck to within FindFontPattern.

Then modifying gfxUserFontSet::FindFontEntry arguments so that only the
information actually used needs to be provided, and/or modifying gfxFontStyle
so that the nsCString member can be empty, can be considered as future
improvements.

> 
> +            // 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?

SlantIsAcceptable() also accepts faces with FONT_STYLE_NORMAL/FC_SLANT_ROMAN
when the requested style is not normal/roman (as an oblique can be synthesized
from normal/roman).

> +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?

Maybe.

The editor's draft http://dev.w3.org/csswg/css3-fonts/#font-reference and the
2002 working draft http://www.w3.org/TR/css3-webfonts/#src currently suggests
returning true only for formats we know we can support: "The user agent will
recognize the name of font formats that it supports, and will avoid
downloading fonts in formats that it does not recognize."

This code was copied from the code for Mac and Windows, so I suggest
considering making that change to all platforms in a separate bug, probably
bug 465452.

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