Merge "Cache the layout result if the font feature is specified" into main

17 files changed, 208 insertions, 79 deletions

diff --git a/include/minikin/Debug.h b/include/minikin/Debug.h
index 4308c90..20d8d8d 100644
--- a/include/minikin/Debug.h
+++ b/include/minikin/Debug.h
@@ -25,6 +25,7 @@ struct Point;
 struct MinikinRect;
 struct MinikinExtent;
 struct MinikinPaint;
+struct FontFeature;
 class Range;
 class U16StringPiece;
 class LayoutPiece;
@@ -40,6 +41,8 @@ std::string toString(const Range& range);
 std::string toString(const MinikinExtent& extent);
 std::string toString(const LayoutPiece& layout);
 std::string toString(const MinikinPaint& paint);
+std::string toString(const FontFeature& feature);
+std::string toString(const std::vector<FontFeature>& features);
 
 }  // namespace debug
 
diff --git a/include/minikin/FontFeature.h b/include/minikin/FontFeature.h
new file mode 100644
index 0000000..c47594b
--- /dev/null
+++ b/include/minikin/FontFeature.h
@@ -0,0 +1,77 @@
+/*
+ * Copyright (C) 2021 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#ifndef MINIKIN_FONT_FEATURE_H
+#define MINIKIN_FONT_FEATURE_H
+
+#include <hb.h>
+
+#include <ostream>
+#include <string_view>
+#include <vector>
+
+namespace minikin {
+
+struct MinikinPaint;
+
+// Subset of the hb_feature_t since we don't allow setting features on ranges.
+struct FontFeature {
+    hb_tag_t tag;
+    uint32_t value;
+
+    static std::vector<FontFeature> parse(std::string_view fontFeatureString);
+};
+
+/**
+ * Returns the final set of font features based on the features requested by this paint object and
+ * extra defaults or implied font features.
+ *
+ * Features are included from the paint object if they are:
+ *   1) in a supported range
+ *
+ * Default features are added based if they are:
+ *   1) implied due to Paint settings such as letterSpacing
+ *   2) default features that do not conflict with requested features
+ */
+std::vector<hb_feature_t> cleanAndAddDefaultFontFeatures(const MinikinPaint& features);
+
+// For gtest output
+inline std::ostream& operator<<(std::ostream& os, const FontFeature& feature) {
+    return os << static_cast<char>(feature.tag >> 24) << static_cast<char>(feature.tag >> 16)
+              << static_cast<char>(feature.tag >> 8) << static_cast<char>(feature.tag) << " "
+              << feature.value;
+}
+
+inline std::ostream& operator<<(std::ostream& os, const std::vector<FontFeature>& features) {
+    for (size_t i = 0; i < features.size(); ++i) {
+        if (i != 0) {
+            os << ", ";
+        }
+        os << features;
+    }
+    return os;
+}
+
+constexpr bool operator==(const FontFeature& l, const FontFeature& r) {
+    return l.tag == r.tag && l.value == r.value;
+}
+
+constexpr bool operator!=(const FontFeature& l, const FontFeature& r) {
+    return !(l == r);
+}
+
+}  // namespace minikin
+#endif  // MINIKIN_LAYOUT_H
diff --git a/include/minikin/Hasher.h b/include/minikin/Hasher.h
index 4a76b29..4e20195 100644
--- a/include/minikin/Hasher.h
+++ b/include/minikin/Hasher.h
@@ -18,9 +18,9 @@
 #define MINIKIN_HASHER_H
 
 #include <cstdint>
-
 #include <string>
 
+#include "minikin/FontFeature.h"
 #include "minikin/Macros.h"
 
 namespace minikin {
@@ -57,6 +57,20 @@ public:
         return update(bits.i);
     }
 
+    inline Hasher& update(const std::vector<FontFeature>& features) {
+        update(features.size());
+        for (const FontFeature& feature : features) {
+            update(feature);
+        }
+        return *this;
+    }
+
+    inline Hasher& update(const FontFeature& feature) {
+        update(feature.tag);
+        update(feature.value);
+        return *this;
+    }
+
     inline Hasher& updateShorts(const uint16_t* data, uint32_t length) {
         update(length);
         uint32_t i;
diff --git a/include/minikin/LayoutCache.h b/include/minikin/LayoutCache.h
index 74300a2..fa572fb 100644
--- a/include/minikin/LayoutCache.h
+++ b/include/minikin/LayoutCache.h
@@ -55,6 +55,7 @@ public:
               mStartHyphen(startHyphen),
               mEndHyphen(endHyphen),
               mIsRtl(dir),
+              mFontFeatureSettings(paint.fontFeatureSettings),
               mHash(computeHash()) {}
 
     bool operator==(const LayoutCacheKey& o) const {
@@ -64,6 +65,7 @@ public:
                mFontFlags == o.mFontFlags && mLocaleListId == o.mLocaleListId &&
                mFamilyVariant == o.mFamilyVariant && mStartHyphen == o.mStartHyphen &&
                mEndHyphen == o.mEndHyphen && mIsRtl == o.mIsRtl && mNchars == o.mNchars &&
+               mFontFeatureSettings == o.mFontFeatureSettings &&
                !memcmp(mChars, o.mChars, mNchars * sizeof(uint16_t));
     }
 
@@ -77,6 +79,7 @@ public:
     void freeText() {
         delete[] mChars;
         mChars = NULL;
+        mFontFeatureSettings.clear();
     }
 
     uint32_t getMemoryUsage() const { return sizeof(LayoutCacheKey) + sizeof(uint16_t) * mNchars; }
@@ -99,6 +102,7 @@ private:
     StartHyphenEdit mStartHyphen;
     EndHyphenEdit mEndHyphen;
     bool mIsRtl;
+    std::vector<FontFeature> mFontFeatureSettings;
     // Note: any fields added to MinikinPaint must also be reflected here.
     // TODO: language matching (possibly integrate into style)
     android::hash_t mHash;
@@ -120,6 +124,7 @@ private:
                 .update(packHyphenEdit(mStartHyphen, mEndHyphen))
                 .update(mIsRtl)
                 .updateShorts(mChars, mNchars)
+                .update(mFontFeatureSettings)
                 .hash();
     }
 };
diff --git a/include/minikin/MinikinPaint.h b/include/minikin/MinikinPaint.h
index 54b476f..04cab1d 100644
--- a/include/minikin/MinikinPaint.h
+++ b/include/minikin/MinikinPaint.h
@@ -23,6 +23,7 @@
 #include "minikin/FamilyVariant.h"
 #include "minikin/FontCollection.h"
 #include "minikin/FontFamily.h"
+#include "minikin/FontFeature.h"
 #include "minikin/Hasher.h"
 
 namespace minikin {
@@ -45,7 +46,7 @@ enum MinikinFontFlags {
 };
 
 // Possibly move into own .h file?
-// Note: if you add a field here, either add it to LayoutCacheKey or to skipCache()
+// Note: if you add a field here, either add it to LayoutCacheKey
 struct MinikinPaint {
     MinikinPaint(const std::shared_ptr<FontCollection>& font)
             : size(0),
@@ -59,7 +60,7 @@ struct MinikinPaint {
               fontFeatureSettings(),
               font(font) {}
 
-    bool skipCache() const { return !fontFeatureSettings.empty(); }
+    bool skipCache() const;
 
     float size;
     float scaleX;
@@ -70,7 +71,7 @@ struct MinikinPaint {
     uint32_t localeListId;
     FontStyle fontStyle;
     FamilyVariant familyVariant;
-    std::string fontFeatureSettings;
+    std::vector<FontFeature> fontFeatureSettings;
     std::shared_ptr<FontCollection> font;
 
     void copyFrom(const MinikinPaint& paint) { *this = paint; }
@@ -100,7 +101,7 @@ struct MinikinPaint {
                 .update(localeListId)
                 .update(fontStyle.identifier())
                 .update(static_cast<uint8_t>(familyVariant))
-                .updateString(fontFeatureSettings)
+                .update(fontFeatureSettings)
                 .update(font->getId())
                 .hash();
     }
diff --git a/libs/minikin/Debug.cpp b/libs/minikin/Debug.cpp
index 42d6d8e..8168a77 100644
--- a/libs/minikin/Debug.cpp
+++ b/libs/minikin/Debug.cpp
@@ -21,6 +21,7 @@
 
 #include <sstream>
 
+#include "minikin/FontFeature.h"
 #include "minikin/FontFileParser.h"
 #include "minikin/LayoutCore.h"
 #include "minikin/LocaleList.h"
@@ -79,6 +80,18 @@ std::string toString(const MinikinExtent& extent) {
     return ss.str();
 }
 
+std::string toString(const FontFeature& feature) {
+    std::stringstream ss;
+    ss << feature;
+    return ss.str();
+}
+
+std::string toString(const std::vector<FontFeature>& features) {
+    std::stringstream ss;
+    ss << features;
+    return ss.str();
+}
+
 std::string toString(const LayoutPiece& layout) {
     std::stringstream ss;
     ss << "{advance=" << layout.advance() << ", extent=" << toString(layout.extent())
diff --git a/libs/minikin/FeatureFlags.h b/libs/minikin/FeatureFlags.h
index 3c2e455..f1da23d 100644
--- a/libs/minikin/FeatureFlags.h
+++ b/libs/minikin/FeatureFlags.h
@@ -39,6 +39,14 @@ inline bool word_style_auto() {
 #endif  // __ANDROID__
 }
 
+inline bool inter_character_justification() {
+#ifdef __ANDROID__
+    return com_android_text_flags_inter_character_justification();
+#else
+    return true;
+#endif  // __ANDROID__
+}
+
 }  // namespace features
 
 #endif  // FEATURE_FLAGS
diff --git a/libs/minikin/FontFeatureUtils.cpp b/libs/minikin/FontFeatureUtils.cpp
index e1ec065..cc0749a 100644
--- a/libs/minikin/FontFeatureUtils.cpp
+++ b/libs/minikin/FontFeatureUtils.cpp
@@ -14,12 +14,29 @@
  * limitations under the License.
  */
 
-#include "FontFeatureUtils.h"
-
 #include "StringPiece.h"
+#include "minikin/FontFeature.h"
+#include "minikin/MinikinPaint.h"
 
 namespace minikin {
 
+std::vector<FontFeature> FontFeature::parse(std::string_view fontFeatureSettings) {
+    std::vector<FontFeature> features;
+
+    SplitIterator it(StringPiece(fontFeatureSettings), ',');
+    while (it.hasNext()) {
+        StringPiece featureStr = it.next();
+        static hb_feature_t feature;
+        // We do not allow setting features on ranges. As such, reject any setting that has
+        // non-universal range.
+        if (hb_feature_from_string(featureStr.data(), featureStr.size(), &feature) &&
+            feature.start == 0 && feature.end == (unsigned int)-1) {
+            features.push_back({feature.tag, feature.value});
+        }
+    }
+    return features;
+}
+
 std::vector<hb_feature_t> cleanAndAddDefaultFontFeatures(const MinikinPaint& paint) {
     std::vector<hb_feature_t> features;
     // Disable default-on non-required ligature features if letter-spacing
@@ -40,27 +57,17 @@ std::vector<hb_feature_t> cleanAndAddDefaultFontFeatures(const MinikinPaint& pai
     static constexpr hb_tag_t halt_tag = HB_TAG('h', 'a', 'l', 't');
     static constexpr hb_tag_t palt_tag = HB_TAG('p', 'a', 'l', 't');
 
-    SplitIterator it(paint.fontFeatureSettings, ',');
-    while (it.hasNext()) {
-        StringPiece featureStr = it.next();
-        static hb_feature_t feature;
-        // We do not allow setting features on ranges. As such, reject any setting that has
-        // non-universal range.
-        if (hb_feature_from_string(featureStr.data(), featureStr.size(), &feature) &&
-            feature.start == 0 && feature.end == (unsigned int)-1) {
-            // OpenType requires disabling default `chws` feature if glyph-width features.
-            // https://docs.microsoft.com/en-us/typography/opentype/spec/features_ae#tag-chws
-            // Here, we follow Chrome's impl: not enabling default `chws` feature if `palt` or
-            // `halt` is enabled.
-            // https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/platform/fonts/shaping/font_features.cc;drc=77a9a09de0688ca449f5333a305ceaf3f36b6daf;l=215
-            if (default_enable_chws &&
-                (feature.tag == chws_tag ||
-                 (feature.value && (feature.tag == halt_tag || feature.tag == palt_tag)))) {
-                default_enable_chws = false;
-            }
-
-            features.push_back(feature);
+    for (const FontFeature& feature : paint.fontFeatureSettings) {
+        // OpenType requires disabling default `chws` feature if glyph-width features.
+        // https://docs.microsoft.com/en-us/typography/opentype/spec/features_ae#tag-chws
+        // Here, we follow Chrome's impl: not enabling default `chws` feature if `palt` or
+        // `halt` is enabled.
+        // https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/platform/fonts/shaping/font_features.cc;drc=77a9a09de0688ca449f5333a305ceaf3f36b6daf;l=215
+        if (feature.tag == chws_tag ||
+            (feature.value && (feature.tag == halt_tag || feature.tag == palt_tag))) {
+            default_enable_chws = false;
         }
+        features.push_back({feature.tag, feature.value, 0, ~0u});
     }
 
     if (default_enable_chws) {
diff --git a/libs/minikin/FontFeatureUtils.h b/libs/minikin/FontFeatureUtils.h
deleted file mode 100644
index 1a02173..0000000
--- a/libs/minikin/FontFeatureUtils.h
+++ /dev/null
@@ -1,40 +0,0 @@
-/*
- * Copyright (C) 2021 The Android Open Source Project
- *
- * Licensed under the Apache License, Version 2.0 (the "License");
- * you may not use this file except in compliance with the License.
- * You may obtain a copy of the License at
- *
- *      http://www.apache.org/licenses/LICENSE-2.0
- *
- * Unless required by applicable law or agreed to in writing, software
- * distributed under the License is distributed on an "AS IS" BASIS,
- * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
- * See the License for the specific language governing permissions and
- * limitations under the License.
- */
-
-#ifndef MINIKIN_FONT_FEATURE_UTILS_H
-#define MINIKIN_FONT_FEATURE_UTILS_H
-
-#include <hb.h>
-
-#include "minikin/MinikinPaint.h"
-
-namespace minikin {
-
-/**
- * Returns the final set of font features based on the features requested by this paint object and
- * extra defaults or implied font features.
- *
- * Features are included from the paint object if they are:
- *   1) in a supported range
- *
- * Default features are added based if they are:
- *   1) implied due to Paint settings such as letterSpacing
- *   2) default features that do not conflict with requested features
- */
-std::vector<hb_feature_t> cleanAndAddDefaultFontFeatures(const MinikinPaint& paint);
-
-}  // namespace minikin
-#endif  // MINIKIN_LAYOUT_UTILS_H
diff --git a/libs/minikin/LayoutCore.cpp b/libs/minikin/LayoutCore.cpp
index b89958e..10c932a 100644
--- a/libs/minikin/LayoutCore.cpp
+++ b/libs/minikin/LayoutCore.cpp
@@ -34,11 +34,11 @@
 #include <vector>
 
 #include "BidiUtils.h"
-#include "FontFeatureUtils.h"
 #include "LayoutUtils.h"
 #include "LocaleListCache.h"
 #include "MinikinInternal.h"
 #include "minikin/Emoji.h"
+#include "minikin/FontFeature.h"
 #include "minikin/HbUtils.h"
 #include "minikin/LayoutCache.h"
 #include "minikin/LayoutPieces.h"
diff --git a/libs/minikin/MinikinInternal.cpp b/libs/minikin/MinikinInternal.cpp
index d02f71f..5b81406 100644
--- a/libs/minikin/MinikinInternal.cpp
+++ b/libs/minikin/MinikinInternal.cpp
@@ -17,9 +17,12 @@
 
 #define LOG_TAG "Minikin"
 
+#include "MinikinInternal.h"
+
 #include <log/log.h>
 
-#include "MinikinInternal.h"
+#include "FeatureFlags.h"
+#include "minikin/MinikinPaint.h"
 
 namespace minikin {
 
@@ -45,4 +48,12 @@ bool isVariationSelector(uint32_t codePoint) {
     return isBMPVariationSelector(codePoint) || isVariationSelectorSupplement(codePoint);
 }
 
+bool MinikinPaint::skipCache() const {
+    if (features::inter_character_justification()) {
+        return false;  // if the flag is on, do not skip the cache.
+    } else {
+        return !fontFeatureSettings.empty();
+    }
+}
+
 }  // namespace minikin
diff --git a/libs/minikin/StringPiece.h b/libs/minikin/StringPiece.h
index befb312..84c7d17 100644
--- a/libs/minikin/StringPiece.h
+++ b/libs/minikin/StringPiece.h
@@ -29,6 +29,7 @@ public:
     StringPiece(const char* data) : mData(data), mLength(data == nullptr ? 0 : strlen(data)) {}
     StringPiece(const char* data, size_t length) : mData(data), mLength(length) {}
     StringPiece(const std::string& str) : mData(str.data()), mLength(str.size()) {}
+    StringPiece(std::string_view str) : mData(str.data()), mLength(str.size()) {}
 
     inline const char* data() const { return mData; }
     inline size_t length() const { return mLength; }
diff --git a/tests/perftests/Android.bp b/tests/perftests/Android.bp
index 362ba10..c4260b3 100644
--- a/tests/perftests/Android.bp
+++ b/tests/perftests/Android.bp
@@ -46,5 +46,7 @@ cc_benchmark {
         "libicu",
         "liblog",
         "libcutils",
+        "aconfig_text_flags_c_lib",
+        "server_configurable_flags",
     ],
 }
diff --git a/tests/stresstest/Android.bp b/tests/stresstest/Android.bp
index 7e0379c..420a4b1 100644
--- a/tests/stresstest/Android.bp
+++ b/tests/stresstest/Android.bp
@@ -42,6 +42,8 @@ cc_test {
         "libutils",
         "libz",
         "libcutils",
+        "aconfig_text_flags_c_lib",
+        "server_configurable_flags",
     ],
 
     srcs: [
diff --git a/tests/unittest/FontFeatureTest.cpp b/tests/unittest/FontFeatureTest.cpp
index 7f9cdf4..6f01842 100644
--- a/tests/unittest/FontFeatureTest.cpp
+++ b/tests/unittest/FontFeatureTest.cpp
@@ -14,10 +14,12 @@
  * limitations under the License.
  */
 
+#include <com_android_text_flags.h>
+#include <flag_macros.h>
 #include <gtest/gtest.h>
 
-#include "FontFeatureUtils.h"
 #include "FontTestUtils.h"
+#include "minikin/FontFeature.h"
 #include "minikin/MinikinPaint.h"
 
 namespace minikin {
@@ -53,7 +55,7 @@ TEST_F(DefaultFontFeatureTest, default) {
 
 TEST_F(DefaultFontFeatureTest, disable) {
     auto paint = MinikinPaint(font);
-    paint.fontFeatureSettings = "\"chws\" off";
+    paint.fontFeatureSettings = FontFeature::parse("\"chws\" off");
 
     auto f = cleanAndAddDefaultFontFeatures(paint);
     std::sort(f.begin(), f.end(), compareFeatureTag);
@@ -65,7 +67,7 @@ TEST_F(DefaultFontFeatureTest, disable) {
 
 TEST_F(DefaultFontFeatureTest, preserve) {
     auto paint = MinikinPaint(font);
-    paint.fontFeatureSettings = "\"ruby\" on";
+    paint.fontFeatureSettings = FontFeature::parse("\"ruby\" on");
 
     auto f = cleanAndAddDefaultFontFeatures(paint);
     std::sort(f.begin(), f.end(), compareFeatureTag);
@@ -95,7 +97,7 @@ TEST_F(DefaultFontFeatureTest, large_letter_spacing) {
 
 TEST_F(DefaultFontFeatureTest, halt_disable_chws) {
     auto paint = MinikinPaint(font);
-    paint.fontFeatureSettings = "\"halt\" on";
+    paint.fontFeatureSettings = FontFeature::parse("\"halt\" on");
 
     auto f = cleanAndAddDefaultFontFeatures(paint);
     EXPECT_EQ(1u, f.size());
@@ -105,7 +107,7 @@ TEST_F(DefaultFontFeatureTest, halt_disable_chws) {
 
 TEST_F(DefaultFontFeatureTest, palt_disable_chws) {
     auto paint = MinikinPaint(font);
-    paint.fontFeatureSettings = "\"palt\" on";
+    paint.fontFeatureSettings = FontFeature::parse("\"palt\" on");
 
     auto f = cleanAndAddDefaultFontFeatures(paint);
     EXPECT_EQ(1u, f.size());
@@ -116,7 +118,7 @@ TEST_F(DefaultFontFeatureTest, palt_disable_chws) {
 TEST_F(DefaultFontFeatureTest, halt_disable_chws_large_letter_spacing) {
     auto paint = MinikinPaint(font);
     paint.letterSpacing = 1.0;  // em
-    paint.fontFeatureSettings = "\"halt\" on";
+    paint.fontFeatureSettings = FontFeature::parse("\"halt\" on");
 
     auto f = cleanAndAddDefaultFontFeatures(paint);
     std::sort(f.begin(), f.end(), compareFeatureTag);
@@ -133,7 +135,7 @@ TEST_F(DefaultFontFeatureTest, halt_disable_chws_large_letter_spacing) {
 TEST_F(DefaultFontFeatureTest, palt_disable_chws_large_letter_spacing) {
     auto paint = MinikinPaint(font);
     paint.letterSpacing = 1.0;  // em
-    paint.fontFeatureSettings = "\"palt\" on";
+    paint.fontFeatureSettings = FontFeature::parse("\"palt\" on");
 
     auto f = cleanAndAddDefaultFontFeatures(paint);
     std::sort(f.begin(), f.end(), compareFeatureTag);
@@ -147,4 +149,27 @@ TEST_F(DefaultFontFeatureTest, palt_disable_chws_large_letter_spacing) {
     EXPECT_TRUE(f[2].value);
 }
 
+class FontFeatureTest : public testing::Test {
+protected:
+    std::shared_ptr<FontCollection> font;
+
+    virtual void SetUp() override { font = buildFontCollection("Ascii.ttf"); }
+};
+
+TEST_F_WITH_FLAGS(FontFeatureTest, do_not_skip_cache_if_flagEnabled,
+                  REQUIRES_FLAGS_ENABLED(ACONFIG_FLAG(com::android::text::flags,
+                                                      inter_character_justification))) {
+    auto paint = MinikinPaint(font);
+    paint.fontFeatureSettings = FontFeature::parse("\"palt\" on");
+    EXPECT_FALSE(paint.skipCache());
+}
+
+TEST_F_WITH_FLAGS(FontFeatureTest, do_not_skip_cache_if_flagDisabled,
+                  REQUIRES_FLAGS_DISABLED(ACONFIG_FLAG(com::android::text::flags,
+                                                       inter_character_justification))) {
+    auto paint = MinikinPaint(font);
+    paint.fontFeatureSettings = FontFeature::parse("\"palt\" on");
+    EXPECT_TRUE(paint.skipCache());
+}
+
 }  // namespace minikin
diff --git a/tests/unittest/LayoutCacheTest.cpp b/tests/unittest/LayoutCacheTest.cpp
index b1d60ba..5d20456 100644
--- a/tests/unittest/LayoutCacheTest.cpp
+++ b/tests/unittest/LayoutCacheTest.cpp
@@ -246,11 +246,11 @@ TEST(LayoutCacheTest, cacheMissTest) {
         SCOPED_TRACE("Different font feature settings");
         auto collection = buildFontCollection("Ascii.ttf");
         MinikinPaint paint1(collection);
-        paint1.fontFeatureSettings = "";
+        paint1.fontFeatureSettings = FontFeature::parse("");
         layoutCache.getOrCreate(text1, Range(0, text1.size()), paint1, false /* LTR */,
                                 StartHyphenEdit::NO_EDIT, EndHyphenEdit::NO_EDIT, false, layout1);
         MinikinPaint paint2(collection);
-        paint2.fontFeatureSettings = "'liga' on";
+        paint2.fontFeatureSettings = FontFeature::parse("'liga' on");
         layoutCache.getOrCreate(text1, Range(0, text1.size()), paint2, false /* LTR */,
                                 StartHyphenEdit::NO_EDIT, EndHyphenEdit::NO_EDIT, false, layout2);
         EXPECT_NE(layout1.get(), layout2.get());
diff --git a/tests/unittest/LayoutCoreTest.cpp b/tests/unittest/LayoutCoreTest.cpp
index 139a7ea..264454a 100644
--- a/tests/unittest/LayoutCoreTest.cpp
+++ b/tests/unittest/LayoutCoreTest.cpp
@@ -54,7 +54,7 @@ static LayoutPiece buildLayout(const std::string& text, std::shared_ptr<FontColl
                                const std::string fontFeaturesSettings) {
     MinikinPaint paint(fc);
     paint.size = 10.0f;  // make 1em = 10px
-    paint.fontFeatureSettings = fontFeaturesSettings;
+    paint.fontFeatureSettings = FontFeature::parse(fontFeaturesSettings);
     return buildLayout(text, paint);
 }