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