Do not reply to this email. You can add comments to this bug at https://bugzilla.mozilla.org/show_bug.cgi?id=458169
--- Comment #6 from Robert O'Callahan (:roc) (Mozilla Corporation) roc@ocallahan.org 2008-11-24 13:40:16 PST --- (In reply to comment #5)
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.
Please file a bug on that.
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.
OK.
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.
OK
// 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).
OK
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.
OK