1062058 - refactor userfont code to use a font entry that contains actual platform font

Status: RESOLVED FIXED

(Core :: Graphics: Text, defect)

Description

[John Daggett (:jtd)]

As part of the work for reworking font matching (bug 998869), I'd like to refactor the userfont code so that there's a font entry object for userfonts that contains the underlying platform font entry. The existing code uses a proxy font entry which gets replaced by a platform font entry after the load completes.

This is also needed for supporting the font loading API (bug 1028497), since fonts can be loaded even when they aren't yet included in the set of fonts associated with a given document (i.e. the FontFace constructor is called but the constructed object is not explicitly added to the FontFaceSet collection).

This is probably a good time to do a bit of refactoring anyways, as we should probably push the loading code out of gfxUserFontSet and into the font entry object associated with a single userfont.

Comment 1

[John Daggett (:jtd)]

Attachment: patch, avoid passing around proxy font entry object (wip)

Comment 2

[John Daggett (:jtd)]

Attachment: patch, rename userfont classes and rework flags (wip)

Comment 3

[John Daggett (:jtd)]

Attachment: patch, make userfont font entry a container for platform font entry (wip)

Comment 4

[John Daggett (:jtd)]

Attachment: patch, part 1 - avoid passing around proxy font entry object

Comment 5

[John Daggett (:jtd)]

Attachment: patch, part 2 - rename userfont classes and rework flags

s/gfxProxyFontEntry/gfxUserFontEntry/g
s/gfxMixedFontFamily/gfxUserFontFamily/g

Switch to using three flags for userfonts, one for local ones, another for data fonts and a separate flag for the container.

Comment 6

[John Daggett (:jtd)]

Attachment: patch, part 3 - make userfont font entry a container for platform font entry

Make gfxUserFontEntry a container of a platform font entry. When looking up userfonts there's now an extra step of indirection but we can eliminate the whole swizzle around the font entry thingy the current code does. Also, simplified a lot of the interfaces so that most loading behavior lives within gfxUserFontEntry and not in the gfxUserFontSet code.

Comment 7

[John Daggett (:jtd)]

One thing I didn't do yet is to deal with the call to gfxFontFamily::ResetCharacterMap that I trimmed out. The family charmap is only used for non-userfonts so it doesn't really need to be called. However, I'd like to make the code for that a little cleaner to prevent confusion down the road. I'll do that in a followup patch.

Comment 8

[John Daggett (:jtd)]

tryserver build
https://tbpl.mozilla.org/?tree=Try&rev=0ed63987c0b4

Comment 9

[Cameron McCormack (:heycam)]

Comment on attachment 8483988 [details] [diff] [review]
patch, part 1 - avoid passing around proxy font entry object

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

r=me with these comments/questions addressed.

::: gfx/thebes/gfxAndroidPlatform.cpp
@@ +310,5 @@
> +gfxAndroidPlatform::MakePlatformFont(const nsAString& aFontName,
> +                                     uint16_t aWeight,
> +                                     int16_t aStretch,
> +                                     bool aItalic,
> +                                     const uint8_t *aFontData,

"*" next to type, while you're touching this line.

@@ +335,1 @@
>  }

The implementations of these two methods is the same in most platform classes.  Would it make sense to move them up to gfxPlatform as a default implementation, and then the couple of platforms that need different behaviour can override them?

::: gfx/thebes/gfxAndroidPlatform.h
@@ +50,5 @@
> +    virtual gfxFontEntry* MakePlatformFont(const nsAString& aFontName,
> +                                           uint16_t aWeight,
> +                                           int16_t aStretch,
> +                                           bool aItalic,
> +                                           const uint8_t *aFontData,

"*" next to type.

@@ +55,5 @@
> +                                           uint32_t aLength);
> +    virtual gfxFontEntry* LookupLocalFont(const nsAString& aFontName,
> +                                          uint16_t aWeight,
> +                                          int16_t aStretch,
> +                                          bool aItalic);

Maybe swap the order of these two declarations to match all the other platform classes?

::: gfx/thebes/gfxFT2FontList.h
@@ +42,5 @@
> +    CreateFontEntry(const nsAString& aFontName,
> +                    uint16_t aWeight,
> +                    int16_t aStretch,
> +                    bool aItalic,
> +                    const uint8_t *aFontData,

"*" next to type.

::: gfx/thebes/gfxGDIFontList.h
@@ +320,5 @@
> +    virtual gfxFontEntry* MakePlatformFont(const nsAString& aFontName,
> +                                           uint16_t aWeight,
> +                                           int16_t aStretch,
> +                                           bool aItalic,
> +                                           const uint8_t *aFontData,

"*" next to type.

::: gfx/thebes/gfxMacPlatformFontList.h
@@ +89,5 @@
> +    virtual gfxFontEntry* MakePlatformFont(const nsAString& aFontName,
> +                                           uint16_t aWeight,
> +                                           int16_t aStretch,
> +                                           bool aItalic,
> +                                           const uint8_t *aFontData,

"*" next to type.

::: gfx/thebes/gfxMacPlatformFontList.mm
@@ +724,5 @@
>  #ifdef PR_LOGGING
>          LOG_FONTLIST(("(fontlist-singleface) face name: %s\n",
>                        NS_ConvertUTF16toUTF8(singleFaceFonts[i]).get()));
>  #endif
> +        gfxFontEntry *fontEntry = LookupLocalFont(singleFaceFonts[i],

"*" next to type.

@@ +742,5 @@
>              if (!mFontFamilies.GetWeak(key)) {
>                  gfxFontFamily *familyEntry =
>                      new gfxSingleFaceMacFontFamily(familyName);
> +                fontEntry->mIsUserFont = false;
> +                fontEntry->mIsLocalUserFont = false;

Can you add a comment to say that these get set to true in LookupLocalFont?  Or perhaps we can split LookupLocalFont out into a version that doesn't set those two members to true?

::: gfx/thebes/gfxPangoFonts.cpp
@@ +1591,5 @@
>      gFTLibrary = nullptr;
>  }
>  
>  /* static */ gfxFontEntry *
> +gfxPangoFontGroup::NewFontEntry(const nsAString& aFullname,

Is there a difference between a "full name" and a "font name"?  If not, please call it aFontName to match various other methods.

@@ +1680,5 @@
> +gfxPangoFontGroup::NewFontEntry(const nsAString& aFullname,
> +                                uint16_t aWeight,
> +                                int16_t aStretch,
> +                                bool aItalic,
> +                                const uint8_t *aFontData,

"*" next to type.

::: gfx/thebes/gfxPangoFonts.h
@@ +41,5 @@
>  
>      static void Shutdown();
>  
>      // Used for @font-face { src: local(); }
> +    static gfxFontEntry *NewFontEntry(const nsAString& aFontName,

"*" next to type.

@@ +49,2 @@
>      // Used for @font-face { src: url(); }
> +    static gfxFontEntry *NewFontEntry(const nsAString& aFontName,

"*" next to type.

::: gfx/thebes/gfxPlatformGtk.cpp
@@ +177,5 @@
> +gfxPlatformGtk::MakePlatformFont(const nsAString& aFontName,
> +                                 uint16_t aWeight,
> +                                 int16_t aStretch,
> +                                 bool aItalic,
> +                                 const uint8_t *aFontData,

"*" next to type.

::: gfx/thebes/gfxQtPlatform.cpp
@@ +144,5 @@
> +gfxQtPlatform::MakePlatformFont(const nsAString& aFontName,
> +                                uint16_t aWeight,
> +                                int16_t aStretch,
> +                                bool aItalic,
> +                                const uint8_t *aFontData,

"*" next to type.

::: gfx/thebes/gfxWindowsPlatform.cpp
@@ +925,5 @@
> +gfxWindowsPlatform::MakePlatformFont(const nsAString& aFontName,
> +                                     uint16_t aWeight,
> +                                     int16_t aStretch,
> +                                     bool aItalic,
> +                                     const uint8_t *aFontData,

"*" next to type.

Comment 10

[Cameron McCormack (:heycam)]

Comment on attachment 8483989 [details] [diff] [review]
patch, part 2 - rename userfont classes and rework flags

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

Overall comment: lines that you touch that have "*"s next to identifiers instead of the types should be fixed up, so we can make some progress towards gfx/thebes/ code following the style guide.

r=me with these comments addressed.

::: gfx/thebes/gfxFont.h
@@ +558,4 @@
>      bool             mIsValid     : 1;
>      bool             mIsBadUnderlineFont : 1;
> +    bool             mIsUserFontContainer : 1;
> +    bool             mIsDataUserFont  : 1;

There is some inconsistent spacing in here.  For this line, just have one space before the ":"?

Also, I would love to see some comments documenting these flags.  It's not entirely obvious what "user font container" means, for example, just looking at the declaration here.

::: gfx/thebes/gfxMacPlatformFontList.h
@@ +32,5 @@
>  
>      // for use with data fonts
>      MacOSFontEntry(const nsAString& aPostscriptName, CGFontRef aFontRef,
>                     uint16_t aWeight, uint16_t aStretch, uint32_t aItalicStyle,
> +                   bool aIsDataUserFont, bool aIsLocal);

"aIsLocalUserFont", to mirror aIsDataUserFont -> mIsDataUserFont?

::: gfx/thebes/gfxMacPlatformFontList.mm
@@ +311,1 @@
>      mIsLocalUserFont = aIsLocal;

Assert in here that these two flags are never both set to true.  And in any other constructor that takes these two bool arguments.

@@ +741,5 @@
>              // add only if doesn't exist already
>              if (!mFontFamilies.GetWeak(key)) {
>                  gfxFontFamily *familyEntry =
>                      new gfxSingleFaceMacFontFamily(familyName);
> +                fontEntry->mIsDataUserFont = false;

This line can be removed now that LookupLocalFont only sets mIsLocalUserFont.

::: gfx/thebes/gfxUserFontSet.cpp
@@ +675,3 @@
>                                           aWeight, aStretch, aItalicStyle,
>                                           aFeatureSettings, aLanguageOverride,
>                                           aUnicodeRanges)) {

Fix indentation.

@@ +843,1 @@
>                     void*               aUserArg)

Fix indentation by either removing spaces on the other two lines, or inserting one more space on the aFamily line.

::: gfx/thebes/gfxUserFontSet.h
@@ +536,5 @@
>                        int32_t aStretch,
>                        uint32_t aItalicStyle,
>                        const nsTArray<gfxFontFeature>& aFeatureSettings,
>                        uint32_t aLanguageOverride,
>                        gfxSparseBitSet *aUnicodeRanges);

Fix the indentation of the other argument lines?

::: layout/style/nsFontFaceLoader.cpp
@@ +110,5 @@
>      // We've been canceled
>      return;
>    }
>  
> +  gfxUserFontEntry* pe = loader->mFontEntry.get();

Rename "pe" to "ufe"?

@@ +413,1 @@
>                                    // in case we need to cancel it

Fix alignment.

Comment 11

[Cameron McCormack (:heycam)]

Comment on attachment 8483992 [details] [diff] [review]
patch, part 3 - make userfont font entry a container for platform font entry

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

Overall comment: again please move "*"s to match the style guide while touching lines that are non-conforming (and on new lines of course).

::: gfx/thebes/gfxFont.h
@@ +931,5 @@
>                                          FontListSizes* aSizes) const;
>      virtual void AddSizeOfIncludingThis(mozilla::MallocSizeOf aMallocSizeOf,
>                                          FontListSizes* aSizes) const;
>  
>      // Only used for debugging checks - does a linear search

This claim about only being used for debugging checks seems to be wrong.  gfxUserFontSet::FindFamilyFor uses it.  Fix it while you're here?

::: gfx/thebes/gfxPangoFonts.cpp
@@ +813,5 @@
>      gfxFontFamily *family = mUserFontSet->LookupFamily(utf16Family);
>      if (family) {
> +        gfxUserFontEntry *userFontEntry = nullptr;
> +        userFontEntry = mUserFontSet->FindUserFontEntry(family, style, needsBold,
> +                                                        aWaitForUserFont);

Combine the variable declaration and assignment.

::: gfx/thebes/gfxUserFontSet.h
@@ +102,5 @@
>              if (mAvailableFonts[--i] == aFontEntry) {
>                  mAvailableFonts.RemoveElementAt(i);
>                  break;
>              }
>          }

Instead of this loop, how about we just replace it with

  mAvailableFonts.RemoveElement(aFontEntry.get());

?

@@ +418,5 @@
>      };
>  
> +    void SetLocalRulesUsed(bool aLocalRulesUsed) {
> +        mLocalRulesUsed = aLocalRulesUsed;
> +    }

Looks like this is only ever called with true.  Remove the argument?

::: layout/style/nsFontFaceLoader.cpp
@@ +759,4 @@
>    for (uint32_t i = 0; i < mRules.Length(); ++i) {
> +    if ((!isUserFontContainer &&
> +         mRules[i].mUserFontEntry->GetPlatformFontEntry() == aFontEntry) ||
> +        (mRules[i].mUserFontEntry == aFontEntry)) {

I find this a bit confusing.  It looks like you want to match

(a) aFontEntry, a platform font entry, against the array's user font containers' platform font entries
(b) aFontEntry, a user font container, against the array's user font containers

but you end up also checking for

(c) aFontEntry, a user font container, against the array's user font containers' platform font entries

which will never match, but still.

Am I right in thinking that layout/inspector/nsFontFace.cpp will always call this with a platform font entry, and that the FindRuleForEntry call in this file is only ever made with a user font container?  It might make sense to split these two uses up:

nsUserFontSet::FindRuleForPlatformFontEntry(gfxFontEntry* aFontEntry)
{
  NS_ASSERTION(!aFontEntry->mIsUserFontContainer);
  ...
}

nsUserFontSet::FindRuleForUserFontEntry(gfxUserFontEntry* aUserFontEntry)
{
  ...
}

WDYT?

Comment 12

[John Daggett (:jtd)]

Attachment: patch, part 3 v3 - make userfont font entry a container for platform font entry

Updated based on review comments.

(In reply to Cameron McCormack (:heycam) from comment #11)

> >      // Only used for debugging checks - does a linear search
> 
> This claim about only being used for debugging checks seems to be wrong. 
> gfxUserFontSet::FindFamilyFor uses it.  Fix it while you're here?

And that function is dead code. Trimming that one makes the comment true again. :)

> ::: layout/style/nsFontFaceLoader.cpp
> @@ +759,4 @@
> >    for (uint32_t i = 0; i < mRules.Length(); ++i) {
> > +    if ((!isUserFontContainer &&
> > +         mRules[i].mUserFontEntry->GetPlatformFontEntry() == aFontEntry) ||
> > +        (mRules[i].mUserFontEntry == aFontEntry)) {
> 
> I find this a bit confusing.  It looks like you want to match
> 
> (a) aFontEntry, a platform font entry, against the array's user font
> containers' platform font entries
> (b) aFontEntry, a user font container, against the array's user font
> containers

Separated into two methods.

Comment 13

[Cameron McCormack (:heycam)]

Comment on attachment 8485582 [details] [diff] [review]
patch, part 3 v3 - make userfont font entry a container for platform font entry

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

r=me with these two things.

::: gfx/thebes/gfxFont.h
@@ +932,5 @@
>      virtual void AddSizeOfIncludingThis(mozilla::MallocSizeOf aMallocSizeOf,
>                                          FontListSizes* aSizes) const;
>  
>      // Only used for debugging checks - does a linear search
> +    bool ContainsFace(gfxFontEntry* aFontEntry);

Now that this truly is only used for debugging, can you make it #ifdef DEBUG?

::: layout/style/nsFontFaceLoader.cpp
@@ +755,4 @@
>  nsCSSFontFaceRule*
>  nsUserFontSet::FindRuleForEntry(gfxFontEntry* aFontEntry)
>  {
>    for (uint32_t i = 0; i < mRules.Length(); ++i) {

Assert in here that aFontEntry is not a user font entry, and call it FindRuleForPlatformFontEntry or FindRuleForNonUserFontEntry, to make it clear that you need to choose the right method.

Comment 14

[John Daggett (:jtd)]

Pushed to inbound, with changes based on review comments:
https://hg.mozilla.org/integration/mozilla-inbound/rev/33afab4b84f9
https://hg.mozilla.org/integration/mozilla-inbound/rev/41f6469b5218
https://hg.mozilla.org/integration/mozilla-inbound/rev/c47072d34566

> Assert in here that aFontEntry is not a user font entry, and call it
> FindRuleForPlatformFontEntry or FindRuleForNonUserFontEntry, to make it
> clear that you need to choose the right method.

I put in an assert but I didn't change the name. Basically, the only type of font entry code outside of gfx should see is a platform font entry, since those represent actual, real fonts. Okayed this with cam on irc.

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/33afab4b84f9
https://hg.mozilla.org/mozilla-central/rev/41f6469b5218
https://hg.mozilla.org/mozilla-central/rev/c47072d34566