Remove FontFamily.addFont and make FontFamily immutable.

This is 2nd attempt of 41e02e96
The difference is adding clearElementsEithLock to Font class which
is necessary to delete Fonts object outside of minikin. This method
should be removed once http://b/28119474 is fixed.

Here is original commit message of reverted change.

This lays the groundwork for making SparseBitSet serializable.
FontFamily.addFont is only used when the FontFamily is constructed.
Thus, instead of calling FontFamily.addFont multiple time, passes
Font list to the constructor. By this change, FontFamily can be
immutable now.

By making FontFamily immutable, We can create FontFamily with
pre-calculated SparseBitSet.

Bug: 34042446
Bug: 28119474
Bug: 34378805
Test: minikin_tests has passed
Change-Id: Ice433931196f5ae79a1a7ee0c98020f914aeb5f2

include/minikin/FontFamily.h

@@ -98,64 +98,66 @@ struct FakedFont {
     FontFakery fakery;
 };
 
+struct Font {
+    Font(MinikinFont* typeface, FontStyle style);
+    Font(Font&& o);
+    Font(const Font& o);
+    ~Font();
+
+    MinikinFont* typeface;
+    FontStyle style;
+
+    // TODO: remove this weird function. http://b/28119474
+    // MinikinFont requres mutex lock for destruction, but the mutex lock is not
+    // visible from outside of minikin library.
+    static void clearElementsWithLock(std::vector<minikin::Font>* fonts);
+};
+
 class FontFamily : public MinikinRefCounted {
 public:
-    FontFamily();
-
-    explicit FontFamily(int variant);
-
-    FontFamily(uint32_t langId, int variant)
-        : mLangId(langId),
-        mVariant(variant),
-        mHasVSTable(false),
-        mCoverageValid(false) {
-    }
+    explicit FontFamily(std::vector<Font>&& fonts);
+    FontFamily(int variant, std::vector<Font>&& fonts);
+    FontFamily(uint32_t langId, int variant, std::vector<Font>&& fonts);
 
     ~FontFamily();
 
-    // Add font to family, extracting style information from the font
-    bool addFont(MinikinFont* typeface);
+    // TODO: Good to expose FontUtil.h.
+    static bool analyzeStyle(MinikinFont* typeface, int* weight, bool* italic);
 
-    void addFont(MinikinFont* typeface, FontStyle style);
     FakedFont getClosestMatch(FontStyle style) const;
 
     uint32_t langId() const { return mLangId; }
     int variant() const { return mVariant; }
 
     // API's for enumerating the fonts in a family. These don't guarantee any particular order
-    size_t getNumFonts() const;
-    MinikinFont* getFont(size_t index) const;
-    FontStyle getStyle(size_t index) const;
+    size_t getNumFonts() const { return mFonts.size(); }
+    MinikinFont* getFont(size_t index) const { return mFonts[index].typeface; }
+    FontStyle getStyle(size_t index) const { return mFonts[index].style; }
     bool isColorEmojiFamily() const;
 
-    // Get Unicode coverage. Lifetime of returned bitset is same as receiver. May return nullptr on
-    // error.
-    const SparseBitSet* getCoverage();
+    // Get Unicode coverage.
+    const SparseBitSet& getCoverage() const { return mCoverage; }
 
     // Returns true if the font has a glyph for the code point and variation selector pair.
     // Caller should acquire a lock before calling the method.
-    bool hasGlyph(uint32_t codepoint, uint32_t variationSelector);
+    bool hasGlyph(uint32_t codepoint, uint32_t variationSelector) const;
 
     // Returns true if this font family has a variaion sequence table (cmap format 14 subtable).
-    bool hasVSTable() const;
+    bool hasVSTable() const { return mHasVSTable; }
 
 private:
-    void addFontLocked(MinikinFont* typeface, FontStyle style);
-
-    class Font {
-    public:
-        Font(MinikinFont* typeface, FontStyle style) :
-            typeface(typeface), style(style) { }
-        MinikinFont* typeface;
-        FontStyle style;
-    };
+    void computeCoverage();
+
     uint32_t mLangId;
     int mVariant;
     std::vector<Font> mFonts;
 
     SparseBitSet mCoverage;
     bool mHasVSTable;
-    bool mCoverageValid;
+
+    // Forbid copying and assignment.
+    FontFamily(const FontFamily&) = delete;
+    void operator=(const FontFamily&) = delete;
 };
 
 }  // namespace minikin

libs/minikin/FontCollection.cpp

@@ -96,17 +96,13 @@ FontCollection::FontCollection(const vector<FontFamily*>& typefaces) :
             continue;
         }
         family->RefLocked();
-        const SparseBitSet* coverage = family->getCoverage();
-        if (coverage == nullptr) {
-            family->UnrefLocked();
-            continue;
-        }
+        const SparseBitSet& coverage = family->getCoverage();
         mFamilies.push_back(family);  // emplace_back would be better
         if (family->hasVSTable()) {
             mVSFamilyVec.push_back(family);
         }
-        mMaxChar = max(mMaxChar, coverage->length());
-        lastChar.push_back(coverage->nextSetBit(0));
+        mMaxChar = max(mMaxChar, coverage.length());
+        lastChar.push_back(coverage.nextSetBit(0));
     }
     nTypefaces = mFamilies.size();
     LOG_ALWAYS_FATAL_IF(nTypefaces == 0,
@@ -130,7 +126,7 @@ FontCollection::FontCollection(const vector<FontFamily*>& typefaces) :
                 FontFamily* family = mFamilies[j];
                 mFamilyVec.push_back(family);
                 offset++;
-                uint32_t nextChar = family->getCoverage()->nextSetBit((i + 1) << kLogCharsPerPage);
+                uint32_t nextChar = family->getCoverage().nextSetBit((i + 1) << kLogCharsPerPage);
 #ifdef VERBOSE_DEBUG
                 ALOGD("nextChar = %d (j = %zd)\n", nextChar, j);
 #endif
@@ -197,7 +193,7 @@ uint32_t FontCollection::calcFamilyScore(uint32_t ch, uint32_t vs, int variant,
 //   variation sequence's base character.
 uint32_t FontCollection::calcCoverageScore(uint32_t ch, uint32_t vs, FontFamily* fontFamily) const {
     const bool hasVSGlyph = (vs != 0) && fontFamily->hasGlyph(ch, vs);
-    if (!hasVSGlyph && !fontFamily->getCoverage()->get(ch)) {
+    if (!hasVSGlyph && !fontFamily->getCoverage().get(ch)) {
         // The font doesn't support either variation sequence or even the base character.
         return kUnsupportedFontScore;
     }
@@ -416,7 +412,7 @@ void FontCollection::itemize(const uint16_t *string, size_t string_size, FontSty
         if (lastFamily != nullptr) {
             if (isStickyWhitelisted(ch)) {
                 // Continue using existing font as long as it has coverage and is whitelisted
-                shouldContinueRun = lastFamily->getCoverage()->get(ch);
+                shouldContinueRun = lastFamily->getCoverage().get(ch);
             } else if (isVariationSelector(ch)) {
                 // Always continue if the character is a variation selector.
                 shouldContinueRun = true;
@@ -436,7 +432,7 @@ void FontCollection::itemize(const uint16_t *string, size_t string_size, FontSty
                 if (utf16Pos != 0 &&
                         ((U_GET_GC_MASK(ch) & U_GC_M_MASK) != 0 ||
                          (isEmojiModifier(ch) && isEmojiBase(prevCh))) &&
-                        family && family->getCoverage()->get(prevCh)) {
+                        family && family->getCoverage().get(prevCh)) {
                     const size_t prevChLength = U16_LENGTH(prevCh);
                     run->end -= prevChLength;
                     if (run->start == run->end) {

libs/minikin/FontFamily.cpp

@@ -64,45 +64,56 @@ uint32_t FontStyle::pack(int variant, int weight, bool italic) {
     return (weight & kWeightMask) | (italic ? kItalicMask : 0) | (variant << kVariantShift);
 }
 
-FontFamily::FontFamily() : FontFamily(0 /* variant */) {
+Font::Font(MinikinFont* typeface, FontStyle style)
+    : typeface(typeface), style(style) {
+    typeface->Ref();
 }
 
-FontFamily::FontFamily(int variant) : FontFamily(FontLanguageListCache::kEmptyListId, variant) {
+Font::Font(Font&& o) {
+    typeface = o.typeface;
+    style = o.style;
+    o.typeface = nullptr;
 }
 
-FontFamily::~FontFamily() {
-    for (size_t i = 0; i < mFonts.size(); i++) {
-        mFonts[i].typeface->UnrefLocked();
-    }
+Font::Font(const Font& o) {
+    typeface = o.typeface;
+    typeface->Ref();
+    style = o.style;
 }
 
-bool FontFamily::addFont(MinikinFont* typeface) {
-    android::AutoMutex _l(gMinikinLock);
-    const uint32_t os2Tag = MinikinFont::MakeTag('O', 'S', '/', '2');
-    HbBlob os2Table(getFontTable(typeface, os2Tag));
-    if (os2Table.get() == nullptr) return false;
-    int weight;
-    bool italic;
-    if (analyzeStyle(os2Table.get(), os2Table.size(), &weight, &italic)) {
-        //ALOGD("analyzed weight = %d, italic = %s", weight, italic ? "true" : "false");
-        FontStyle style(weight, italic);
-        addFontLocked(typeface, style);
-        return true;
-    } else {
-        ALOGD("failed to analyze style");
+Font::~Font() {
+    if (typeface == nullptr) {
+        return;
     }
-    return false;
+    typeface->UnrefLocked();
 }
 
-void FontFamily::addFont(MinikinFont* typeface, FontStyle style) {
+void Font::clearElementsWithLock(std::vector<minikin::Font>* fonts) {
     android::AutoMutex _l(gMinikinLock);
-    addFontLocked(typeface, style);
+    fonts->clear();
 }
 
-void FontFamily::addFontLocked(MinikinFont* typeface, FontStyle style) {
-    typeface->RefLocked();
-    mFonts.push_back(Font(typeface, style));
-    mCoverageValid = false;
+FontFamily::FontFamily(std::vector<Font>&& fonts) : FontFamily(0 /* variant */, std::move(fonts)) {
+}
+
+FontFamily::FontFamily(int variant, std::vector<Font>&& fonts)
+    : FontFamily(FontLanguageListCache::kEmptyListId, variant, std::move(fonts)) {
+}
+
+FontFamily::FontFamily(uint32_t langId, int variant, std::vector<Font>&& fonts)
+    : mLangId(langId), mVariant(variant), mFonts(std::move(fonts)), mHasVSTable(false) {
+    computeCoverage();
+}
+
+FontFamily::~FontFamily() {
+}
+
+bool FontFamily::analyzeStyle(MinikinFont* typeface, int* weight, bool* italic) {
+    android::AutoMutex _l(gMinikinLock);
+    const uint32_t os2Tag = MinikinFont::MakeTag('O', 'S', '/', '2');
+    HbBlob os2Table(getFontTable(typeface, os2Tag));
+    if (os2Table.get() == nullptr) return false;
+    return ::minikin::analyzeStyle(os2Table.get(), os2Table.size(), weight, italic);
 }
 
 // Compute a matching metric between two styles - 0 is an exact match
@@ -146,18 +157,6 @@ FakedFont FontFamily::getClosestMatch(FontStyle style) const {
     return result;
 }
 
-size_t FontFamily::getNumFonts() const {
-    return mFonts.size();
-}
-
-MinikinFont* FontFamily::getFont(size_t index) const {
-    return mFonts[index].typeface;
-}
-
-FontStyle FontFamily::getStyle(size_t index) const {
-    return mFonts[index].style;
-}
-
 bool FontFamily::isColorEmojiFamily() const {
     const FontLanguages& languageList = FontLanguageListCache::getById(mLangId);
     for (size_t i = 0; i < languageList.size(); ++i) {
@@ -168,30 +167,24 @@ bool FontFamily::isColorEmojiFamily() const {
     return false;
 }
 
-const SparseBitSet* FontFamily::getCoverage() {
-    if (!mCoverageValid) {
-        const FontStyle defaultStyle;
-        MinikinFont* typeface = getClosestMatch(defaultStyle).font;
-        const uint32_t cmapTag = MinikinFont::MakeTag('c', 'm', 'a', 'p');
-        HbBlob cmapTable(getFontTable(typeface, cmapTag));
-        if (cmapTable.get() == nullptr) {
-            ALOGE("Could not get cmap table size!\n");
-            // Note: This means we will retry on the next call to getCoverage, as we can't store
-            //       the failure. This is fine, as we assume this doesn't really happen in practice.
-            return nullptr;
-        }
-        // TODO: Error check?
-        CmapCoverage::getCoverage(mCoverage, cmapTable.get(), cmapTable.size(), &mHasVSTable);
+void FontFamily::computeCoverage() {
+    android::AutoMutex _l(gMinikinLock);
+    const FontStyle defaultStyle;
+    MinikinFont* typeface = getClosestMatch(defaultStyle).font;
+    const uint32_t cmapTag = MinikinFont::MakeTag('c', 'm', 'a', 'p');
+    HbBlob cmapTable(getFontTable(typeface, cmapTag));
+    if (cmapTable.get() == nullptr) {
+        ALOGE("Could not get cmap table size!\n");
+        return;
+    }
+    // TODO: Error check?
+    CmapCoverage::getCoverage(mCoverage, cmapTable.get(), cmapTable.size(), &mHasVSTable);
 #ifdef VERBOSE_DEBUG
-        ALOGD("font coverage length=%d, first ch=%x\n", mCoverage.length(),
-                mCoverage.nextSetBit(0));
+    ALOGD("font coverage length=%d, first ch=%x\n", mCoverage.length(), mCoverage.nextSetBit(0));
 #endif
-        mCoverageValid = true;
-    }
-    return &mCoverage;
 }
 
-bool FontFamily::hasGlyph(uint32_t codepoint, uint32_t variationSelector) {
+bool FontFamily::hasGlyph(uint32_t codepoint, uint32_t variationSelector) const {
     assertMinikinLocked();
     if (variationSelector != 0 && !mHasVSTable) {
         // Early exit if the variation selector is specified but the font doesn't have a cmap format
@@ -208,9 +201,4 @@ bool FontFamily::hasGlyph(uint32_t codepoint, uint32_t variationSelector) {
     return result;
 }
 
-bool FontFamily::hasVSTable() const {
-    LOG_ALWAYS_FATAL_IF(!mCoverageValid, "Do not call this method before getCoverage() call");
-    return mHasVSTable;
-}
-
 }  // namespace minikin

sample/example.cpp

@@ -45,9 +45,9 @@ FontCollection *makeFontCollection() {
         "/system/fonts/Roboto-LightItalic.ttf"
     };
 
-    FontFamily *family = new FontFamily();
     FT_Face face;
     FT_Error error;
+    std::vector<Font> fonts;
     for (size_t i = 0; i < sizeof(fns)/sizeof(fns[0]); i++) {
         const char *fn = fns[i];
         printf("adding %s\n", fn);
@@ -56,16 +56,16 @@ FontCollection *makeFontCollection() {
             printf("error loading %s, %d\n", fn, error);
         }
         MinikinFont *font = new MinikinFontFreeType(face);
-        family->addFont(font);
+        fonts.push_back(Font(font, FontStyle()));
     }
+    FontFamily *family = new FontFamily(std::move(fonts));
     typefaces.push_back(family);
 
 #if 1
-    family = new FontFamily();
     const char *fn = "/system/fonts/DroidSansDevanagari-Regular.ttf";
     error = FT_New_Face(library, fn, 0, &face);
     MinikinFont *font = new MinikinFontFreeType(face);
-    family->addFont(font);
+    family = new FontFamily(std::vector<Font>({ Font(font, FontStyle()) }));
     typefaces.push_back(family);
 #endif
 

sample/example_skia.cpp

@@ -54,21 +54,21 @@ FontCollection *makeFontCollection() {
         "/system/fonts/Roboto-LightItalic.ttf"
     };
 
-    FontFamily *family = new FontFamily();
+    std::vector<Font> fonts;
     for (size_t i = 0; i < sizeof(fns)/sizeof(fns[0]); i++) {
         const char *fn = fns[i];
         sk_sp<SkTypeface> skFace = SkTypeface::MakeFromFile(fn);
         MinikinFont *font = new MinikinFontSkia(std::move(skFace));
-        family->addFont(font);
+        fonts.push_back(Font(font, FontStyle()));
     }
+    FontFamily *family = new FontFamily(std::move(fonts));
     typefaces.push_back(family);
 
 #if 1
-    family = new FontFamily();
     const char *fn = "/system/fonts/DroidSansDevanagari-Regular.ttf";
     sk_sp<SkTypeface> skFace = SkTypeface::MakeFromFile(fn);
     MinikinFont *font = new MinikinFontSkia(std::move(skFace));
-    family->addFont(font);
+    family = new FontFamily(std::vector<Font>({ Font(font, FontStyle()) }));
     typefaces.push_back(family);
 #endif
 

tests/unittest/FontCollectionItemizeTest.cpp

@@ -668,14 +668,14 @@ TEST_F(FontCollectionItemizeTest, itemize_vs_sequence_but_no_base_char) {
     const std::string kVSTestFont = kTestFontDir "VarioationSelectorTest-Regular.ttf";
 
     std::vector<FontFamily*> families;
-    FontFamily* family1 = new FontFamily(VARIANT_DEFAULT);
     MinikinAutoUnref<MinikinFont> font(new MinikinFontForTest(kLatinFont));
-    family1->addFont(font.get());
+    FontFamily* family1 = new FontFamily(VARIANT_DEFAULT,
+            std::vector<Font>{ Font(font.get(), FontStyle()) });
     families.push_back(family1);
 
-    FontFamily* family2 = new FontFamily(VARIANT_DEFAULT);
     MinikinAutoUnref<MinikinFont> font2(new MinikinFontForTest(kVSTestFont));
-    family2->addFont(font2.get());
+    FontFamily* family2 = new FontFamily(VARIANT_DEFAULT,
+            std::vector<Font>{ Font(font2.get(), FontStyle()) });
     families.push_back(family2);
 
     FontCollection collection(families);
@@ -811,11 +811,11 @@ TEST_F(FontCollectionItemizeTest, itemize_LanguageScore) {
         std::vector<FontFamily*> families;
 
         // Prepare first font which doesn't supports U+9AA8
-        FontFamily* firstFamily = new FontFamily(
-                FontStyle::registerLanguageList("und"), 0 /* variant */);
         MinikinAutoUnref<MinikinFont> firstFamilyMinikinFont(
                 new MinikinFontForTest(kNoGlyphFont));
-        firstFamily->addFont(firstFamilyMinikinFont.get());
+        FontFamily* firstFamily = new FontFamily(
+                FontStyle::registerLanguageList("und"), 0 /* variant */,
+                std::vector<Font>({ Font(firstFamilyMinikinFont.get(), FontStyle()) }));
         families.push_back(firstFamily);
 
         // Prepare font families
@@ -824,10 +824,10 @@ TEST_F(FontCollectionItemizeTest, itemize_LanguageScore) {
         std::unordered_map<MinikinFont*, int> fontLangIdxMap;
 
         for (size_t i = 0; i < testCase.fontLanguages.size(); ++i) {
-            FontFamily* family = new FontFamily(
-                    FontStyle::registerLanguageList(testCase.fontLanguages[i]), 0 /* variant */);
             MinikinAutoUnref<MinikinFont> minikin_font(new MinikinFontForTest(kJAFont));
-            family->addFont(minikin_font.get());
+            FontFamily* family = new FontFamily(
+                    FontStyle::registerLanguageList(testCase.fontLanguages[i]), 0 /* variant */,
+                    std::vector<Font>({ Font(minikin_font.get(), FontStyle()) }));
             families.push_back(family);
             fontLangIdxMap.insert(std::make_pair(minikin_font.get(), i));
         }
@@ -1417,13 +1417,12 @@ TEST_F(FontCollectionItemizeTest, itemizeShouldKeepOrderForVS) {
     MinikinAutoUnref<MinikinFont> fontA(new MinikinFontForTest(kZH_HansFont));
     MinikinAutoUnref<MinikinFont> fontB(new MinikinFontForTest(kZH_HansFont));
 
-    MinikinAutoUnref<FontFamily> dummyFamily(new FontFamily());
-    MinikinAutoUnref<FontFamily> familyA(new FontFamily());
-    MinikinAutoUnref<FontFamily> familyB(new FontFamily());
-
-    dummyFamily->addFont(dummyFont.get());
-    familyA->addFont(fontA.get());
-    familyB->addFont(fontB.get());
+    MinikinAutoUnref<FontFamily> dummyFamily(new FontFamily(
+            std::vector<Font>({ Font(dummyFont.get(), FontStyle()) })));
+    MinikinAutoUnref<FontFamily> familyA(new FontFamily(
+            std::vector<Font>({ Font(fontA.get(), FontStyle()) })));
+    MinikinAutoUnref<FontFamily> familyB(new FontFamily(
+            std::vector<Font>({ Font(fontB.get(), FontStyle()) })));
 
     std::vector<FontFamily*> families =
             { dummyFamily.get(), familyA.get(), familyB.get() };
@@ -1453,13 +1452,12 @@ TEST_F(FontCollectionItemizeTest, itemizeShouldKeepOrderForVS2) {
     MinikinAutoUnref<MinikinFont> noCmapFormat14Font(
             new MinikinFontForTest(kNoCmapFormat14Font));
 
-    MinikinAutoUnref<FontFamily> dummyFamily(new FontFamily());
-    MinikinAutoUnref<FontFamily> hasCmapFormat14Family(new FontFamily());
-    MinikinAutoUnref<FontFamily> noCmapFormat14Family(new FontFamily());
-
-    dummyFamily->addFont(dummyFont.get());
-    hasCmapFormat14Family->addFont(hasCmapFormat14Font.get());
-    noCmapFormat14Family->addFont(noCmapFormat14Font.get());
+    MinikinAutoUnref<FontFamily> dummyFamily(new FontFamily(
+            std::vector<Font>({ Font(dummyFont.get(), FontStyle()) })));
+    MinikinAutoUnref<FontFamily> hasCmapFormat14Family(new FontFamily(
+            std::vector<Font>({ Font(hasCmapFormat14Font.get(), FontStyle()) })));
+    MinikinAutoUnref<FontFamily> noCmapFormat14Family(new FontFamily(
+            std::vector<Font>({ Font(noCmapFormat14Font.get(), FontStyle()) })));
 
     std::vector<FontFamily*> families =
             { dummyFamily.get(), hasCmapFormat14Family.get(), noCmapFormat14Family.get() };

tests/unittest/FontCollectionTest.cpp

@@ -57,9 +57,9 @@ void expectVSGlyphs(const FontCollection* fc, uint32_t codepoint, const std::set
 }
 
 TEST(FontCollectionTest, hasVariationSelectorTest) {
-  MinikinAutoUnref<FontFamily> family(new FontFamily());
   MinikinAutoUnref<MinikinFont> font(new MinikinFontForTest(kVsTestFont));
-  family->addFont(font.get());
+  MinikinAutoUnref<FontFamily> family(new FontFamily(
+          std::vector<Font>({ Font(font.get(), FontStyle()) })));
   std::vector<FontFamily*> families({family.get()});
   MinikinAutoUnref<FontCollection> fc(new FontCollection(families));
 

tests/unittest/FontFamilyTest.cpp

@@ -464,8 +464,10 @@ void expectVSGlyphs(FontFamily* family, uint32_t codepoint, const std::set<uint3
 TEST_F(FontFamilyTest, hasVariationSelectorTest) {
     MinikinAutoUnref<MinikinFontForTest>
             minikinFont(new MinikinFontForTest(kVsTestFont));
-    MinikinAutoUnref<FontFamily> family(new FontFamily);
-    family->addFont(minikinFont.get());
+    MinikinAutoUnref<FontFamily> family(
+            new FontFamily(std::vector<Font>{
+                    Font(minikinFont.get(), FontStyle())
+            }));
 
     android::AutoMutex _l(gMinikinLock);
 
@@ -478,23 +480,23 @@ TEST_F(FontFamilyTest, hasVariationSelectorTest) {
     const uint32_t kVS20 = 0xE0103;
 
     const uint32_t kSupportedChar1 = 0x82A6;
-    EXPECT_TRUE(family->getCoverage()->get(kSupportedChar1));
+    EXPECT_TRUE(family->getCoverage().get(kSupportedChar1));
     expectVSGlyphs(family.get(), kSupportedChar1, std::set<uint32_t>({kVS1, kVS17, kVS18, kVS19}));
 
     const uint32_t kSupportedChar2 = 0x845B;
-    EXPECT_TRUE(family->getCoverage()->get(kSupportedChar2));
+    EXPECT_TRUE(family->getCoverage().get(kSupportedChar2));
     expectVSGlyphs(family.get(), kSupportedChar2, std::set<uint32_t>({kVS2, kVS18, kVS19, kVS20}));
 
     const uint32_t kNoVsSupportedChar = 0x537F;
-    EXPECT_TRUE(family->getCoverage()->get(kNoVsSupportedChar));
+    EXPECT_TRUE(family->getCoverage().get(kNoVsSupportedChar));
     expectVSGlyphs(family.get(), kNoVsSupportedChar, std::set<uint32_t>());
 
     const uint32_t kVsOnlySupportedChar = 0x717D;
-    EXPECT_FALSE(family->getCoverage()->get(kVsOnlySupportedChar));
+    EXPECT_FALSE(family->getCoverage().get(kVsOnlySupportedChar));
     expectVSGlyphs(family.get(), kVsOnlySupportedChar, std::set<uint32_t>({kVS3, kVS19, kVS20}));
 
     const uint32_t kNotSupportedChar = 0x845C;
-    EXPECT_FALSE(family->getCoverage()->get(kNotSupportedChar));
+    EXPECT_FALSE(family->getCoverage().get(kNotSupportedChar));
     expectVSGlyphs(family.get(), kNotSupportedChar, std::set<uint32_t>());
 }
 
@@ -518,11 +520,9 @@ TEST_F(FontFamilyTest, hasVSTableTest) {
 
         MinikinAutoUnref<MinikinFontForTest> minikinFont(
                 new MinikinFontForTest(testCase.fontPath));
-        MinikinAutoUnref<FontFamily> family(new FontFamily);
-        family->addFont(minikinFont.get());
+        MinikinAutoUnref<FontFamily> family(new FontFamily(
+                std::vector<Font>{ Font(minikinFont.get(), FontStyle()) }));
         android::AutoMutex _l(gMinikinLock);
-        family->getCoverage();
-
         EXPECT_EQ(testCase.hasVSTable, family->hasVSTable());
     }
 }

tests/util/FontTestUtils.cpp

@@ -48,16 +48,7 @@ FontCollection* getFontCollection(const char* fontDir, const char* fontXml) {
             }
         }
 
-        xmlChar* lang = xmlGetProp(familyNode, (const xmlChar*)"lang");
-        FontFamily* family;
-        if (lang == nullptr) {
-            family = new FontFamily(variant);
-        } else {
-            uint32_t langId = FontStyle::registerLanguageList(
-                    std::string((const char*)lang, xmlStrlen(lang)));
-            family = new FontFamily(langId, variant);
-        }
-
+        std::vector<Font> fonts;
         for (xmlNode* fontNode = familyNode->children; fontNode; fontNode = fontNode->next) {
             if (xmlStrcmp(fontNode->name, (const xmlChar*)"font") != 0) {
                 continue;
@@ -80,13 +71,23 @@ FontCollection* getFontCollection(const char* fontDir, const char* fontXml) {
             if (index == nullptr) {
                 MinikinAutoUnref<MinikinFontForTest>
                         minikinFont(new MinikinFontForTest(fontPath));
-                family->addFont(minikinFont.get(), FontStyle(weight, italic));
+                fonts.push_back(Font(minikinFont.get(), FontStyle(weight, italic)));
             } else {
                 MinikinAutoUnref<MinikinFontForTest>
                         minikinFont(new MinikinFontForTest(fontPath, atoi((const char*)index)));
-                family->addFont(minikinFont.get(), FontStyle(weight, italic));
+                fonts.push_back(Font(minikinFont.get(), FontStyle(weight, italic)));
             }
         }
+
+        xmlChar* lang = xmlGetProp(familyNode, (const xmlChar*)"lang");
+        FontFamily* family;
+        if (lang == nullptr) {
+            family = new FontFamily(variant, std::move(fonts));
+        } else {
+            uint32_t langId = FontStyle::registerLanguageList(
+                    std::string((const char*)lang, xmlStrlen(lang)));
+            family = new FontFamily(langId, variant, std::move(fonts));
+        }
         families.push_back(family);
     }
     xmlFreeDoc(doc);