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

17 files changed, 79 insertions, 208 deletions

diff --git a/include/minikin/Debug.h b/include/minikin/Debug.h
index 20d8d8d..4308c90 100644
--- a/include/minikin/Debug.h
+++ b/include/minikin/Debug.h
@@ -25,7 +25,6 @@ struct Point;
 struct MinikinRect;
 struct MinikinExtent;
 struct MinikinPaint;
-struct FontFeature;
 class Range;
 class U16StringPiece;
 class LayoutPiece;
@@ -41,8 +40,6 @@ 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
deleted file mode 100644
index c47594b..0000000
--- a/include/minikin/FontFeature.h
+++ /dev/null
@@ -1,77 +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_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 4e20195..4a76b29 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,20 +57,6 @@ 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 fa572fb..74300a2 100644
--- a/include/minikin/LayoutCache.h
+++ b/include/minikin/LayoutCache.h
@@ -55,7 +55,6 @@ public:
               mStartHyphen(startHyphen),
               mEndHyphen(endHyphen),
               mIsRtl(dir),
-              mFontFeatureSettings(paint.fontFeatureSettings),
               mHash(computeHash()) {}
 
     bool operator==(const LayoutCacheKey& o) const {
@@ -65,7 +64,6 @@ 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));
     }
 
@@ -79,7 +77,6 @@ public:
     void freeText() {
         delete[] mChars;
         mChars = NULL;
-        mFontFeatureSettings.clear();
     }
 
     uint32_t getMemoryUsage() const { return sizeof(LayoutCacheKey) + sizeof(uint16_t) * mNchars; }
@@ -102,7 +99,6 @@ 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;
@@ -124,7 +120,6 @@ 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 04cab1d..54b476f 100644
--- a/include/minikin/MinikinPaint.h
+++ b/include/minikin/MinikinPaint.h
@@ -23,7 +23,6 @@
 #include "minikin/FamilyVariant.h"
 #include "minikin/FontCollection.h"
 #include "minikin/FontFamily.h"
-#include "minikin/FontFeature.h"
 #include "minikin/Hasher.h"
 
 namespace minikin {
@@ -46,7 +45,7 @@ enum MinikinFontFlags {
 };
 
 // Possibly move into own .h file?
-// Note: if you add a field here, either add it to LayoutCacheKey
+// Note: if you add a field here, either add it to LayoutCacheKey or to skipCache()
 struct MinikinPaint {
     MinikinPaint(const std::shared_ptr<FontCollection>& font)
             : size(0),
@@ -60,7 +59,7 @@ struct MinikinPaint {
               fontFeatureSettings(),
               font(font) {}
 
-    bool skipCache() const;
+    bool skipCache() const { return !fontFeatureSettings.empty(); }
 
     float size;
     float scaleX;
@@ -71,7 +70,7 @@ struct MinikinPaint {
     uint32_t localeListId;
     FontStyle fontStyle;
     FamilyVariant familyVariant;
-    std::vector<FontFeature> fontFeatureSettings;
+    std::string fontFeatureSettings;
     std::shared_ptr<FontCollection> font;
 
     void copyFrom(const MinikinPaint& paint) { *this = paint; }
@@ -101,7 +100,7 @@ struct MinikinPaint {
                 .update(localeListId)
                 .update(fontStyle.identifier())
                 .update(static_cast<uint8_t>(familyVariant))
-                .update(fontFeatureSettings)
+                .updateString(fontFeatureSettings)
                 .update(font->getId())
                 .hash();
     }
diff --git a/libs/minikin/Debug.cpp b/libs/minikin/Debug.cpp
index 8168a77..42d6d8e 100644
--- a/libs/minikin/Debug.cpp
+++ b/libs/minikin/Debug.cpp
@@ -21,7 +21,6 @@
 
 #include <sstream>
 
-#include "minikin/FontFeature.h"
 #include "minikin/FontFileParser.h"
 #include "minikin/LayoutCore.h"
 #include "minikin/LocaleList.h"
@@ -80,18 +79,6 @@ 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 f1da23d..3c2e455 100644
--- a/libs/minikin/FeatureFlags.h
+++ b/libs/minikin/FeatureFlags.h
@@ -39,14 +39,6 @@ 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 cc0749a..e1ec065 100644
--- a/libs/minikin/FontFeatureUtils.cpp
+++ b/libs/minikin/FontFeatureUtils.cpp
@@ -14,29 +14,12 @@
  * 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
@@ -57,17 +40,27 @@ 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');
 
-    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;
+    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);
         }
-        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
new file mode 100644
index 0000000..1a02173
--- /dev/null
+++ b/libs/minikin/FontFeatureUtils.h
@@ -0,0 +1,40 @@
+/*
+ * 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 10c932a..b89958e 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 5b81406..d02f71f 100644
--- a/libs/minikin/MinikinInternal.cpp
+++ b/libs/minikin/MinikinInternal.cpp
@@ -17,12 +17,9 @@
 
 #define LOG_TAG "Minikin"
 
-#include "MinikinInternal.h"
-
 #include <log/log.h>
 
-#include "FeatureFlags.h"
-#include "minikin/MinikinPaint.h"
+#include "MinikinInternal.h"
 
 namespace minikin {
 
@@ -48,12 +45,4 @@ 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 84c7d17..befb312 100644
--- a/libs/minikin/StringPiece.h
+++ b/libs/minikin/StringPiece.h
@@ -29,7 +29,6 @@ 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 c4260b3..362ba10 100644
--- a/tests/perftests/Android.bp
+++ b/tests/perftests/Android.bp
@@ -46,7 +46,5 @@ 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 420a4b1..7e0379c 100644
--- a/tests/stresstest/Android.bp
+++ b/tests/stresstest/Android.bp
@@ -42,8 +42,6 @@ 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 6f01842..7f9cdf4 100644
--- a/tests/unittest/FontFeatureTest.cpp
+++ b/tests/unittest/FontFeatureTest.cpp
@@ -14,12 +14,10 @@
  * 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 {
@@ -55,7 +53,7 @@ TEST_F(DefaultFontFeatureTest, default) {
 
 TEST_F(DefaultFontFeatureTest, disable) {
     auto paint = MinikinPaint(font);
-    paint.fontFeatureSettings = FontFeature::parse("\"chws\" off");
+    paint.fontFeatureSettings = "\"chws\" off";
 
     auto f = cleanAndAddDefaultFontFeatures(paint);
     std::sort(f.begin(), f.end(), compareFeatureTag);
@@ -67,7 +65,7 @@ TEST_F(DefaultFontFeatureTest, disable) {
 
 TEST_F(DefaultFontFeatureTest, preserve) {
     auto paint = MinikinPaint(font);
-    paint.fontFeatureSettings = FontFeature::parse("\"ruby\" on");
+    paint.fontFeatureSettings = "\"ruby\" on";
 
     auto f = cleanAndAddDefaultFontFeatures(paint);
     std::sort(f.begin(), f.end(), compareFeatureTag);
@@ -97,7 +95,7 @@ TEST_F(DefaultFontFeatureTest, large_letter_spacing) {
 
 TEST_F(DefaultFontFeatureTest, halt_disable_chws) {
     auto paint = MinikinPaint(font);
-    paint.fontFeatureSettings = FontFeature::parse("\"halt\" on");
+    paint.fontFeatureSettings = "\"halt\" on";
 
     auto f = cleanAndAddDefaultFontFeatures(paint);
     EXPECT_EQ(1u, f.size());
@@ -107,7 +105,7 @@ TEST_F(DefaultFontFeatureTest, halt_disable_chws) {
 
 TEST_F(DefaultFontFeatureTest, palt_disable_chws) {
     auto paint = MinikinPaint(font);
-    paint.fontFeatureSettings = FontFeature::parse("\"palt\" on");
+    paint.fontFeatureSettings = "\"palt\" on";
 
     auto f = cleanAndAddDefaultFontFeatures(paint);
     EXPECT_EQ(1u, f.size());
@@ -118,7 +116,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 = FontFeature::parse("\"halt\" on");
+    paint.fontFeatureSettings = "\"halt\" on";
 
     auto f = cleanAndAddDefaultFontFeatures(paint);
     std::sort(f.begin(), f.end(), compareFeatureTag);
@@ -135,7 +133,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 = FontFeature::parse("\"palt\" on");
+    paint.fontFeatureSettings = "\"palt\" on";
 
     auto f = cleanAndAddDefaultFontFeatures(paint);
     std::sort(f.begin(), f.end(), compareFeatureTag);
@@ -149,27 +147,4 @@ 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 5d20456..b1d60ba 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 = FontFeature::parse("");
+        paint1.fontFeatureSettings = "";
         layoutCache.getOrCreate(text1, Range(0, text1.size()), paint1, false /* LTR */,
                                 StartHyphenEdit::NO_EDIT, EndHyphenEdit::NO_EDIT, false, layout1);
         MinikinPaint paint2(collection);
-        paint2.fontFeatureSettings = FontFeature::parse("'liga' on");
+        paint2.fontFeatureSettings = "'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 264454a..139a7ea 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 = FontFeature::parse(fontFeaturesSettings);
+    paint.fontFeatureSettings = fontFeaturesSettings;
     return buildLayout(text, paint);
 }