833169 - need a way to identify font family name within shaper code

Status: RESOLVED FIXED

(Core :: Graphics: Text, defect)

Description

[John Daggett (:jtd)]

Bug 821442 trimmed out the mFamily field from the gfxFontEntry struct.  Unfortunately, the side effect of that is that there is no way to reference the name of the font family associated with a particular font entry.  Part of the problem appears to be that the font activation cache shares gfxFontEntry objects across font families, which makes it impossible to now distinguish betweeen font families based on the gfxFontEntry.

Seems like the simplest solution is to figure out how to pass down the matched family name to the shaper.

Ex:

  @font-face {
    family-name: fontA;
    src: url(font.ttf);
  }
  
  @font-face {
    family-name: fontB;
    src: url(font.ttf);
  }

  @font-feature-values fontA {
    @styleset { bongo : 5; }
  }

  #test1 { font-family: fontA; font-variant-alternates: styleset(bongo); }
  #test2 { font-family: fontB; font-variant-alternates: styleset(bongo); }

In this example, the feature 'ss05' will be applied to the text in element 'test1' but *not* to the text in 'test2', as the defined scope of bongo only includes 'fontA'.

http://dev.w3.org/csswg/css3-fonts/#font-feature-values

Comment 1

[John Daggett (:jtd)]

So I'm thinking the way to do this is to set up an atomized string in gfxFontEntry to point to the family name, constructed initially from the family used in containing family.  Within the user font cache code, if the same font is being added to a second family with a different name from the one used in the cache (i.e. the very rare case), clone the font entry and set it's name to the second family.  This would work around the problem of having a font entry shared across families.

Comment 2

[Jonathan Kew [:jfkthame]]

If you do this, you'll need to be careful about ownership and lifetime of the downloaded font data and the underlying platform font object/reference (HFONT, CGFont, FT_Face, whatever) that the font entry holds.

Comment 3

[John Daggett (:jtd)]

Attachment: patch, add font family atom to font entry objects

Add an atomized string version of the family name to the font entry and initialize it when adding a font entry to a family.  Tweak the user font cache to include the family name as part of the cache, which means that if a site uses a different font family name for the same font data it won't be cached.  I don't think this is a common pattern so I think that should be fine.

Also trimmed out the font family code in the pango font entry object, since the family name will be initialized from the gfxFontFamily name.  There was also an extra call to CacheFont in the codepath for data url fonts which I trimmed out.

Comment 4

[Jonathan Kew [:jfkthame]]

Yes, including the family name as part of the cache key should be OK, as the typical use-case for the user font cache is when a common stylesheet is used across multiple pages of a site. I expect it'd be -highly- unusual for a site to use the same resources under different CSS family names.

Is it really worth atomizing the family name here - wouldn't it be simpler to just store the family name as an nsString? Shared string buffers should mean that's still reasonably cheap.

(Alternatively, if we're going to make atoms for all family names, we should use the atom in the gfxFontFamily as well rather than keeping the name in string form there.)

Comment 5

[John Daggett (:jtd)]

(In reply to Jonathan Kew (:jfkthame) from comment #4)
> Is it really worth atomizing the family name here - wouldn't it be simpler
> to just store the family name as an nsString? Shared string buffers should
> mean that's still reasonably cheap.

I don't follow, the reason I chose to use atoms was to eliminate the duplication of the string.  What do you mean here by "shared strings buffers" exactly?  Atoms seems to serve that purpose nicely but maybe there's some part of the string code I don't know.

> (Alternatively, if we're going to make atoms for all family names, we should
> use the atom in the gfxFontFamily as well rather than keeping the name in
> string form there.)

This seems fine with me.

Comment 6

[Jonathan Kew [:jfkthame]]

(In reply to John Daggett (:jtd) from comment #5)
> (In reply to Jonathan Kew (:jfkthame) from comment #4)
> > Is it really worth atomizing the family name here - wouldn't it be simpler
> > to just store the family name as an nsString? Shared string buffers should
> > mean that's still reasonably cheap.
> 
> I don't follow, the reason I chose to use atoms was to eliminate the
> duplication of the string.  What do you mean here by "shared strings
> buffers" exactly?  Atoms seems to serve that purpose nicely but maybe
> there's some part of the string code I don't know.

IIUC, our strings normally use sharable, refcounted buffers, so that copying a string (provided it is not subsequently modified) does not actually copy the characters, it just makes a new reference to the same buffer. So copying a family name into the font entry should be cheap.

So I think we should just use a standard nsString for the family name here. If you want to look into storing family names as atoms throughout the font code, let's make that a separate followup where we can consider the pros and cons involved.

Comment 7

[John Daggett (:jtd)]

Attachment: patch, add font family name to font entry objects

As suggested in comment 6, simply add a string for the font family name to font entry objects and use these as part of the userfont cache key.  This also eliminates the need for the GetFamilyNameAt() method, so trimmed that out too.

Comment 8

[Jonathan Kew [:jfkthame]]

Comment on attachment 713818 [details] [diff] [review]
patch, add font family name to font entry objects

Review of attachment 713818 [details] [diff] [review]:
-----------------------------------------------------------------

Assuming it passes tests (I don't see why it wouldn't), this LGTM. One suggestion for cleanup below:

::: accessible/src/base/TextAttrs.cpp
@@ +457,1 @@
>    return true;

I'd suggest creating a helper on gfxFontGroup, something like:

  const nsString& PrimaryFamilyName() const {
    return GetFontAt(0)->GetFontEntry()->FamilyName();
  }

for use both here and in nsMathMLChar.cpp.

Comment 9

[Karl Tomlinson (:karlt)]

It would actually make sense to discourage expectations of font groups having a family name (because they don't in general).  An explicit GetFontAt(0) makes it clearer what the client is requesting.

Comment 10

[John Daggett (:jtd)]

Attachment: patch, initialize family names in pango font entries

Pango font entries don't belong to a font family so the font entries need to be initialized explicitly.

Comment 11

[John Daggett (:jtd)]

(In reply to Jonathan Kew (:jfkthame) from comment #8)
> I'd suggest creating a helper on gfxFontGroup, something like:
> 
>   const nsString& PrimaryFamilyName() const {
>     return GetFontAt(0)->GetFontEntry()->FamilyName();
>   }
> 
> for use both here and in nsMathMLChar.cpp.

I didn't add this to the patch, based on karl's reasoning in comment 9.  But I'm fine to add this if you feel it's necessary.

Landed on mozilla-inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/820dff9bdc55
https://hg.mozilla.org/integration/mozilla-inbound/rev/da82798eb275

If Karl has any changes to recommend, I can address those in a follow-up patch.

Comment 12

[Karl Tomlinson (:karlt)]

Comment on attachment 713818 [details] [diff] [review]
patch, add font family name to font entry objects

>     if (aGlyphTable == &gGlyphTableList->mUnicodeTable ||
>-        fm->GetThebesFontGroup()->GetFamilyNameAt(0) == family) {
>+      fm->GetThebesFontGroup()->GetFontAt(0)->GetFontEntry()->
>+      FamilyName() == family) {

Indentation problem.

Comment 13

[Karl Tomlinson (:karlt)]

Comment on attachment 714997 [details] [diff] [review]
patch, initialize family names in pango font entries

Looks good, thanks.  The mFamilyName = aProxyEntry.mFamilyName in gfxUserFcFontEntry is unnecessary AIUI, and it would make clearer the expectations of the different classes if either this is left out of gfxUserFcFontEntry because gfxMixedFontFamily deals with that, or make all user font entry implementations copy the family from the proxy, removing the code in gfxMixedFontFamily.

Comment 14

[John Daggett (:jtd)]

Attachment: patch, fixups as per comment 13

minor fixups based on comment 12, 13

Comment 15

[John Daggett (:jtd)]

Fixup pushed to inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e0ffe719035b

Comment 16

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/820dff9bdc55
https://hg.mozilla.org/mozilla-central/rev/da82798eb275
https://hg.mozilla.org/mozilla-central/rev/e0ffe719035b