Bug 1559044 - Improve criteria for deciding whether to kick off download of a @font-face resource, so that we don't defer resources that will actually be needed. r=heycam
authorJonathan Kew <jkew@mozilla.com>
Wed, 03 Jul 2019 15:14:24 +0000
changeset 481695 b6fa827753928bc580ec13edad54d3970e5671f5
parent 481694 3958f2af0e346f7a5ae23a8eba0dc85572301d86
child 481696 61656a6d3217a6a8328aa413f2d7a1f658607cf3
push id36261
push usernbeleuzu@mozilla.com
push dateTue, 09 Jul 2019 03:44:14 +0000
treeherdermozilla-central@b782ed36b2e8 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersheycam
bugs1559044, 1422530
milestone69.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1559044 - Improve criteria for deciding whether to kick off download of a @font-face resource, so that we don't defer resources that will actually be needed. r=heycam The original patch in bug 1422530 checked whether we're currently hiding a given piece of text because we're waiting for a resource to download, and used this as a signal that we should not initiate another download. However, that's not really a good criterion to use when we're doing font selection for a given character, both because it's an indirect signal -- whether painting the text is suppressed will depend on timing and the font-display property -- and because it doesn't consider whether any resource that's already being downloaded will actually be relevant for the specific character we're trying to display. So this patch instead checks for a relevant in-progress load earlier in the font list during the specific FindFontForChar call, so that when unicode-range is used to split a font across multiple subsets, an in-progress load for one subset won't make us defer the downloads of other subsets if they are also needed for the characters present in the text. With this, the repeated blinking no longer happens on the site here, as loading of all the required font subsets is initiated early. Differential Revision: https://phabricator.services.mozilla.com/D35634
gfx/thebes/gfxTextRun.cpp
gfx/thebes/gfxTextRun.h
--- a/gfx/thebes/gfxTextRun.cpp
+++ b/gfx/thebes/gfxTextRun.cpp
@@ -1835,37 +1835,37 @@ bool gfxFontGroup::HasFont(const gfxFont
   for (uint32_t i = 0; i < count; ++i) {
     if (mFonts[i].FontEntry() == aFontEntry) {
       return true;
     }
   }
   return false;
 }
 
-gfxFont* gfxFontGroup::GetFontAt(int32_t i, uint32_t aCh) {
+gfxFont* gfxFontGroup::GetFontAt(int32_t i, uint32_t aCh, bool* aLoading) {
   if (uint32_t(i) >= mFonts.Length()) {
     return nullptr;
   }
 
   FamilyFace& ff = mFonts[i];
   if (ff.IsInvalid() || ff.IsLoading()) {
     return nullptr;
   }
 
   gfxFont* font = ff.Font();
   if (!font) {
     gfxFontEntry* fe = mFonts[i].FontEntry();
     gfxCharacterMap* unicodeRangeMap = nullptr;
     if (fe->mIsUserFontContainer) {
       gfxUserFontEntry* ufe = static_cast<gfxUserFontEntry*>(fe);
       if (ufe->LoadState() == gfxUserFontEntry::STATUS_NOT_LOADED &&
-          ufe->CharacterInUnicodeRange(aCh) && !mSkipDrawing &&
-          !FontLoadingForFamily(ff, aCh)) {
+          ufe->CharacterInUnicodeRange(aCh) && !*aLoading) {
         ufe->Load();
         ff.CheckState(mSkipDrawing);
+        *aLoading = ff.IsLoading();
       }
       fe = ufe->GetPlatformFontEntry();
       if (!fe) {
         return nullptr;
       }
       unicodeRangeMap = ufe->GetUnicodeRangeMap();
     }
     font = fe->FindOrMakeFont(&mStyle, unicodeRangeMap);
@@ -1914,36 +1914,16 @@ bool gfxFontGroup::FamilyFace::EqualsUse
       return true;
     }
   } else if (fe == aUserFont) {
     return true;
   }
   return false;
 }
 
-bool gfxFontGroup::FontLoadingForFamily(const FamilyFace& aFamily,
-                                        uint32_t aCh) const {
-  if (aFamily.IsSharedFamily()) {
-    return false;
-  }
-  uint32_t count = mFonts.Length();
-  for (uint32_t i = 0; i < count; ++i) {
-    const FamilyFace& ff = mFonts[i];
-    if (!ff.IsSharedFamily() && ff.IsLoading() &&
-        ff.OwnedFamily() == aFamily.OwnedFamily()) {
-      const gfxUserFontEntry* ufe =
-          static_cast<gfxUserFontEntry*>(ff.FontEntry());
-      if (ufe->CharacterInUnicodeRange(aCh)) {
-        return true;
-      }
-    }
-  }
-  return false;
-}
-
 gfxFont* gfxFontGroup::GetDefaultFont() {
   if (mDefaultFont) {
     return mDefaultFont.get();
   }
 
   gfxPlatformFontList* pfl = gfxPlatformFontList::PlatformFontList();
   FontFamily family = pfl->GetDefaultFont(&mStyle);
   MOZ_ASSERT(!family.IsNull(),
@@ -2055,16 +2035,17 @@ gfxFont* gfxFontGroup::GetDefaultFont() 
   }
 
   return mDefaultFont.get();
 }
 
 gfxFont* gfxFontGroup::GetFirstValidFont(uint32_t aCh,
                                          StyleGenericFontFamily* aGeneric) {
   uint32_t count = mFonts.Length();
+  bool loading = false;
   for (uint32_t i = 0; i < count; ++i) {
     FamilyFace& ff = mFonts[i];
     if (ff.IsInvalid()) {
       continue;
     }
 
     // already have a font?
     gfxFont* font = ff.Font();
@@ -2077,27 +2058,32 @@ gfxFont* gfxFontGroup::GetFirstValidFont
 
     // Need to build a font, loading userfont if not loaded. In
     // cases where unicode range might apply, use the character
     // provided.
     if (ff.IsUserFontContainer()) {
       gfxUserFontEntry* ufe =
           static_cast<gfxUserFontEntry*>(mFonts[i].FontEntry());
       bool inRange = ufe->CharacterInUnicodeRange(aCh);
-      if (ufe->LoadState() == gfxUserFontEntry::STATUS_NOT_LOADED && inRange &&
-          !mSkipDrawing && !FontLoadingForFamily(ff, aCh)) {
-        ufe->Load();
-        ff.CheckState(mSkipDrawing);
+      if (inRange) {
+        if (!loading &&
+            ufe->LoadState() == gfxUserFontEntry::STATUS_NOT_LOADED) {
+          ufe->Load();
+          ff.CheckState(mSkipDrawing);
+        }
+        if (ff.IsLoading()) {
+          loading = true;
+        }
       }
       if (ufe->LoadState() != gfxUserFontEntry::STATUS_LOADED || !inRange) {
         continue;
       }
     }
 
-    font = GetFontAt(i, aCh);
+    font = GetFontAt(i, aCh, &loading);
     if (font) {
       if (aGeneric) {
         *aGeneric = mFonts[i].Generic();
       }
       return font;
     }
   }
   if (aGeneric) {
@@ -2815,23 +2801,28 @@ gfxFont* gfxFontGroup::FindFontForChar(u
     // to continue the preceding font run.
     if (aPrevMatchedFont && aPrevMatchedFont->HasCharacter(aCh)) {
       return aPrevMatchedFont;
     }
   }
 
   // To optimize common cases, try the first font in the font-group
   // before going into the more detailed checks below
+  uint32_t fontListLength = mFonts.Length();
   uint32_t nextIndex = 0;
   bool isJoinControl = gfxFontUtils::IsJoinControl(aCh);
   bool wasJoinCauser = gfxFontUtils::IsJoinCauser(aPrevCh);
   bool isVarSelector = gfxFontUtils::IsVarSelector(aCh);
 
+  // Whether we've seen a font that is currently loading a resource that may
+  // provide this character (so we should not start a new load).
+  bool loading = false;
+
   if (!isJoinControl && !wasJoinCauser && !isVarSelector) {
-    gfxFont* firstFont = GetFontAt(0, aCh);
+    gfxFont* firstFont = GetFontAt(0, aCh, &loading);
     if (firstFont) {
       if (firstFont->HasCharacter(aCh)) {
         *aMatchType = {FontMatchType::Kind::kFontGroup, mFonts[0].Generic()};
         return firstFont;
       }
 
       gfxFont* font = nullptr;
       if (mFonts[0].CheckForFallbackFaces()) {
@@ -2842,16 +2833,20 @@ gfxFont* gfxFontGroup::FindFontForChar(u
         // such as Italic or Black have reduced character sets compared
         // to the family's Regular face.
         font = FindFallbackFaceForChar(mFonts[0], aCh);
       }
       if (font) {
         *aMatchType = {FontMatchType::Kind::kFontGroup, mFonts[0].Generic()};
         return font;
       }
+    } else {
+      if (fontListLength > 0) {
+        loading = loading || mFonts[0].IsLoadingFor(aCh);
+      }
     }
 
     // we don't need to check the first font again below
     ++nextIndex;
   }
 
   if (aPrevMatchedFont) {
     // Don't switch fonts for control characters, regardless of
@@ -2878,20 +2873,22 @@ gfxFont* gfxFontGroup::FindFontForChar(u
     if (aPrevMatchedFont) {
       return aPrevMatchedFont;
     }
     // VS alone. it's meaningless to search different fonts
     return nullptr;
   }
 
   // 1. check remaining fonts in the font group
-  uint32_t fontListLength = mFonts.Length();
   for (uint32_t i = nextIndex; i < fontListLength; i++) {
     FamilyFace& ff = mFonts[i];
     if (ff.IsInvalid() || ff.IsLoading()) {
+      if (ff.IsLoadingFor(aCh)) {
+        loading = true;
+      }
       continue;
     }
 
     // if available, use already made gfxFont and check for character
     gfxFont* font = ff.Font();
     if (font) {
       if (font->HasCharacter(aCh)) {
         *aMatchType = {FontMatchType::Kind::kFontGroup, ff.Generic()};
@@ -2907,35 +2904,39 @@ gfxFont* gfxFontGroup::FindFontForChar(u
       // the cmap of the platform font entry
       gfxUserFontEntry* ufe = static_cast<gfxUserFontEntry*>(fe);
 
       // never match a character outside the defined unicode range
       if (!ufe->CharacterInUnicodeRange(aCh)) {
         continue;
       }
 
-      // load if not already loaded but only if no other font in similar
-      // range within family is loading
-      if (ufe->LoadState() == gfxUserFontEntry::STATUS_NOT_LOADED &&
-          !mSkipDrawing && !FontLoadingForFamily(ff, aCh)) {
+      // Load if not already loaded, unless we've already seen an in-
+      // progress load that is expected to satisfy this request.
+      if (!loading && ufe->LoadState() == gfxUserFontEntry::STATUS_NOT_LOADED) {
         ufe->Load();
         ff.CheckState(mSkipDrawing);
       }
+
+      if (ff.IsLoading()) {
+        loading = true;
+      }
+
       gfxFontEntry* pfe = ufe->GetPlatformFontEntry();
       if (pfe && pfe->HasCharacter(aCh)) {
-        font = GetFontAt(i, aCh);
+        font = GetFontAt(i, aCh, &loading);
         if (font) {
           *aMatchType = {FontMatchType::Kind::kFontGroup, mFonts[i].Generic()};
           return font;
         }
       }
     } else if (fe->HasCharacter(aCh)) {
       // for normal platform fonts, after checking the cmap
       // build the font via GetFontAt
-      font = GetFontAt(i, aCh);
+      font = GetFontAt(i, aCh, &loading);
       if (font) {
         *aMatchType = {FontMatchType::Kind::kFontGroup, mFonts[i].Generic()};
         return font;
       }
     }
 
     // check other family faces if needed
     if (ff.CheckForFallbackFaces()) {
--- a/gfx/thebes/gfxTextRun.h
+++ b/gfx/thebes/gfxTextRun.h
@@ -11,16 +11,17 @@
 
 #include "gfxTypes.h"
 #include "gfxPoint.h"
 #include "gfxFont.h"
 #include "gfxFontConstants.h"
 #include "gfxSkipChars.h"
 #include "gfxPlatform.h"
 #include "gfxPlatformFontList.h"
+#include "gfxUserFontSet.h"
 #include "mozilla/MemoryReporting.h"
 #include "mozilla/RefPtr.h"
 #include "nsPoint.h"
 #include "nsString.h"
 #include "nsTArray.h"
 #include "nsTextFrameUtils.h"
 #include "DrawMode.h"
 #include "harfbuzz/hb.h"
@@ -29,18 +30,16 @@
 #include "X11UndefineNone.h"
 
 #ifdef DEBUG
 #  include <stdio.h>
 #endif
 
 class gfxContext;
 class gfxFontGroup;
-class gfxUserFontEntry;
-class gfxUserFontSet;
 class nsAtom;
 class nsLanguageAtomService;
 class gfxMissingFontRecorder;
 
 namespace mozilla {
 class PostTraversalTask;
 class SVGContextPaint;
 enum class StyleHyphens : uint8_t;
@@ -1208,16 +1207,27 @@ class gfxFontGroup final : public gfxTex
     bool IsLoading() const { return mLoading; }
     bool IsInvalid() const { return mInvalid; }
     void CheckState(bool& aSkipDrawing);
     void SetLoading(bool aIsLoading) { mLoading = aIsLoading; }
     void SetInvalid() { mInvalid = true; }
     bool CheckForFallbackFaces() const { return mCheckForFallbackFaces; }
     void SetCheckForFallbackFaces() { mCheckForFallbackFaces = true; }
 
+    // Return true if we're currently loading (or waiting for) a resource that
+    // may support the given character.
+    bool IsLoadingFor(uint32_t aCh) {
+      if (!IsLoading()) {
+        return false;
+      }
+      MOZ_ASSERT(IsUserFontContainer());
+      return static_cast<gfxUserFontEntry*>(FontEntry())
+          ->CharacterInUnicodeRange(aCh);
+    }
+
     void SetFont(gfxFont* aFont) {
       NS_ASSERTION(aFont, "font pointer must not be null");
       NS_ADDREF(aFont);
       if (mFontCreated) {
         NS_RELEASE(mFont);
       } else if (mHasFontEntry) {
         NS_RELEASE(mFontEntry);
         mHasFontEntry = false;
@@ -1303,24 +1313,31 @@ class gfxFontGroup final : public gfxTex
 
   already_AddRefed<gfxTextRun> MakeBlankTextRun(
       uint32_t aLength, const Parameters* aParams,
       mozilla::gfx::ShapedTextFlags aFlags, nsTextFrameUtils::Flags aFlags2);
 
   // Initialize the list of fonts
   void BuildFontList();
 
-  // Get the font at index i within the fontlist.
+  // Get the font at index i within the fontlist, for character aCh (in case
+  // of fonts with multiple resources and unicode-range partitioning).
   // Will initiate userfont load if not already loaded.
-  // May return null if userfont not loaded or if font invalid
-  gfxFont* GetFontAt(int32_t i, uint32_t aCh = 0x20);
+  // May return null if userfont not loaded or if font invalid.
+  // If *aLoading is true, a relevant resource is already being loaded so no
+  // new download will be initiated; if a download is started, *aLoading will
+  // be set to true on return.
+  gfxFont* GetFontAt(int32_t i, uint32_t aCh, bool* aLoading);
 
-  // Whether there's a font loading for a given family in the fontlist
-  // for a given character
-  bool FontLoadingForFamily(const FamilyFace& aFamily, uint32_t aCh) const;
+  // Simplified version of GetFontAt() for use where we just need a font for
+  // metrics, math layout tables, etc.
+  gfxFont* GetFontAt(int32_t i, uint32_t aCh = 0x20) {
+    bool loading = false;
+    return GetFontAt(i, aCh, &loading);
+  }
 
   // will always return a font or force a shutdown
   gfxFont* GetDefaultFont();
 
   // Init this font group's font metrics. If there no bad fonts, you don't need
   // to call this. But if there are one or more bad fonts which have bad
   // underline offset, you should call this with the *first* bad font.
   void InitMetricsForBadFont(gfxFont* aBadFont);