Move HasSpaceInLigaturesOrKerning to HarfBuzzFace

Previously a set if bechmark stories triggered the expensive glyph
collection in GSUB and GPOS way too many times. This was found while
investigating and hb_set performance issue 781794. browse:news:cnn
triggered the call and GSUB/GPOS scanning method 1395 times, and was
reduced to two. browse:news:hackernews 33 times reduced to 4,
browse:social:twitter 58 times reduced to 4,
browse:social:twitter_infinite_scroll 38 times reduced to 4 as well.

We only need to do this work once per font face, independent of size -
whereas previously it was once per Font object, which, depending on
layout of the page, triggers much more frequently.

Test: FontPlatformDataTest
Bug: 791475
Change-Id: I15757abab2037677d5ea4c640ac26ab99da4649c
Reviewed-on: https://chromium-review.googlesource.com/809106
Commit-Queue: Dominik Röttsches <drott@chromium.org>
Reviewed-by: Emil A Eklund <eae@chromium.org>
Cr-Commit-Position: refs/heads/master@{#522394}

third_party/WebKit/Source/platform/fonts/FontPlatformData.cpp

@@ -29,6 +29,7 @@
 #include "platform/text/Character.h"
 #include "platform/wtf/ByteSwap.h"
 #include "platform/wtf/HashMap.h"
+#include "platform/wtf/text/CharacterNames.h"
 #include "platform/wtf/text/StringHash.h"
 #include "platform/wtf/text/WTFString.h"
 
@@ -254,54 +255,13 @@ HarfBuzzFace* FontPlatformData::GetHarfBuzzFace() const {
   return harf_buzz_face_.get();
 }
 
-static inline bool TableHasSpace(hb_face_t* face,
-                                 hb_set_t* glyphs,
-                                 hb_tag_t tag,
-                                 hb_codepoint_t space) {
-  unsigned count = hb_ot_layout_table_get_lookup_count(face, tag);
-  for (unsigned i = 0; i < count; i++) {
-    hb_ot_layout_lookup_collect_glyphs(face, tag, i, glyphs, glyphs, glyphs,
-                                       nullptr);
-    if (hb_set_has(glyphs, space))
-      return true;
-  }
-  return false;
-}
-
 bool FontPlatformData::HasSpaceInLigaturesOrKerning(
     TypesettingFeatures features) const {
   HarfBuzzFace* hb_face = GetHarfBuzzFace();
   if (!hb_face)
     return false;
 
-  hb_font_t* font =
-      hb_face->GetScaledFont(nullptr, HarfBuzzFace::NoVerticalLayout);
-  DCHECK(font);
-  hb_face_t* face = hb_font_get_face(font);
-  DCHECK(face);
-
-  hb_codepoint_t space;
-  // If the space glyph isn't present in the font then each space character
-  // will be rendering using a fallback font, which grantees that it cannot
-  // affect the shape of the preceding word.
-  if (!hb_font_get_glyph(font, kSpaceCharacter, 0, &space))
-    return false;
-
-  if (!hb_ot_layout_has_substitution(face) &&
-      !hb_ot_layout_has_positioning(face)) {
-    return false;
-  }
-
-  bool found_space_in_table = false;
-  hb_set_t* glyphs = hb_set_create();
-  if (features & kKerning)
-    found_space_in_table = TableHasSpace(face, glyphs, HB_OT_TAG_GPOS, space);
-  if (!found_space_in_table && (features & kLigatures))
-    found_space_in_table = TableHasSpace(face, glyphs, HB_OT_TAG_GSUB, space);
-
-  hb_set_destroy(glyphs);
-
-  return found_space_in_table;
+  return hb_face->HasSpaceInLigaturesOrKerning(features);
 }
 
 unsigned FontPlatformData::GetHash() const {

third_party/WebKit/Source/platform/fonts/shaping/HarfBuzzFace.cpp

@@ -72,6 +72,15 @@ void HbFaceDeleter::operator()(hb_face_t* face) {
     hb_face_destroy(face);
 }
 
+struct HbSetDeleter {
+  void operator()(hb_set_t* set) {
+    if (set)
+      hb_set_destroy(set);
+  }
+};
+
+using HbSetUniquePtr = std::unique_ptr<hb_set_t, HbSetDeleter>;
+
 static scoped_refptr<HbFontCacheEntry> CreateHbFontCacheEntry(hb_face_t*);
 
 HarfBuzzFace::HarfBuzzFace(FontPlatformData* platform_data, uint64_t unique_id)
@@ -210,6 +219,71 @@ static hb_bool_t HarfBuzzGetGlyphExtents(hb_font_t* hb_font,
   return true;
 }
 
+static inline bool TableHasSpace(hb_face_t* face,
+                                 hb_set_t* glyphs,
+                                 hb_tag_t tag,
+                                 hb_codepoint_t space) {
+  unsigned count = hb_ot_layout_table_get_lookup_count(face, tag);
+  for (unsigned i = 0; i < count; i++) {
+    hb_ot_layout_lookup_collect_glyphs(face, tag, i, glyphs, glyphs, glyphs,
+                                       nullptr);
+    if (hb_set_has(glyphs, space))
+      return true;
+  }
+  return false;
+}
+
+static bool GetSpaceGlyph(hb_font_t* font, hb_codepoint_t& space) {
+  return hb_font_get_nominal_glyph(font, kSpaceCharacter, &space);
+}
+
+bool HarfBuzzFace::HasSpaceInLigaturesOrKerning(TypesettingFeatures features) {
+  const hb_codepoint_t kInvalidCodepoint = static_cast<hb_codepoint_t>(-1);
+  hb_codepoint_t space = kInvalidCodepoint;
+
+  HbSetUniquePtr glyphs(hb_set_create());
+
+  // Check whether computing is needed and compute for gpos/gsub.
+  if (features & kKerning &&
+      harf_buzz_font_data_->space_in_gpos_ ==
+          HarfBuzzFontData::SpaceGlyphInOpenTypeTables::Unknown) {
+    if (space == kInvalidCodepoint && !GetSpaceGlyph(unscaled_font_, space))
+      return false;
+    // Compute for gpos.
+    hb_face_t* face = hb_font_get_face(unscaled_font_);
+    DCHECK(face);
+    harf_buzz_font_data_->space_in_gpos_ =
+        hb_ot_layout_has_positioning(face) &&
+                TableHasSpace(face, glyphs.get(), HB_OT_TAG_GPOS, space)
+            ? HarfBuzzFontData::SpaceGlyphInOpenTypeTables::Present
+            : HarfBuzzFontData::SpaceGlyphInOpenTypeTables::NotPresent;
+  }
+
+  hb_set_clear(glyphs.get());
+
+  if (features & kLigatures &&
+      harf_buzz_font_data_->space_in_gsub_ ==
+          HarfBuzzFontData::SpaceGlyphInOpenTypeTables::Unknown) {
+    if (space == kInvalidCodepoint && !GetSpaceGlyph(unscaled_font_, space))
+      return false;
+    // Compute for gpos.
+    hb_face_t* face = hb_font_get_face(unscaled_font_);
+    DCHECK(face);
+    harf_buzz_font_data_->space_in_gsub_ =
+        hb_ot_layout_has_substitution(face) &&
+                TableHasSpace(face, glyphs.get(), HB_OT_TAG_GSUB, space)
+            ? HarfBuzzFontData::SpaceGlyphInOpenTypeTables::Present
+            : HarfBuzzFontData::SpaceGlyphInOpenTypeTables::NotPresent;
+  }
+
+  return (features & kKerning &&
+          harf_buzz_font_data_->space_in_gpos_ ==
+              HarfBuzzFontData::SpaceGlyphInOpenTypeTables::Present) ||
+         (features & kLigatures &&
+          harf_buzz_font_data_->space_in_gsub_ ==
+              HarfBuzzFontData::SpaceGlyphInOpenTypeTables::Present);
+}
+
 static hb_font_funcs_t* HarfBuzzSkiaGetFontFuncs() {
   hb_font_funcs_t* funcs = FontGlobalContext::GetHarfBuzzFontFuncs();
 

third_party/WebKit/Source/platform/fonts/shaping/HarfBuzzFace.h

@@ -32,6 +32,7 @@
 #define HarfBuzzFace_h
 
 #include "base/memory/scoped_refptr.h"
+#include "platform/fonts/TypesettingFeatures.h"
 #include "platform/fonts/UnicodeRangeSet.h"
 #include "platform/wtf/Allocator.h"
 #include "platform/wtf/HashMap.h"
@@ -64,6 +65,8 @@ class HarfBuzzFace : public RefCounted<HarfBuzzFace> {
   hb_font_t* GetScaledFont(scoped_refptr<UnicodeRangeSet>,
                            VerticalLayoutCallbacks) const;
 
+  bool HasSpaceInLigaturesOrKerning(TypesettingFeatures);
+
  private:
   HarfBuzzFace(FontPlatformData*, uint64_t);
 

third_party/WebKit/Source/platform/fonts/shaping/HarfBuzzFontCache.h

@@ -39,7 +39,12 @@ struct HarfBuzzFontData {
   WTF_MAKE_NONCOPYABLE(HarfBuzzFontData);
 
  public:
-  HarfBuzzFontData() : paint_(), vertical_data_(nullptr), range_set_(nullptr) {}
+  HarfBuzzFontData()
+      : paint_(),
+        space_in_gpos_(SpaceGlyphInOpenTypeTables::Unknown),
+        space_in_gsub_(SpaceGlyphInOpenTypeTables::Unknown),
+        vertical_data_(nullptr),
+        range_set_(nullptr) {}
 
   // The vertical origin and vertical advance functions in HarfBuzzFace require
   // the ascent and height metrics as fallback in case no specific vertical
@@ -95,6 +100,11 @@ struct HarfBuzzFontData {
   float ascent_fallback_;
   float height_fallback_;
 
+  enum class SpaceGlyphInOpenTypeTables { Unknown, Present, NotPresent };
+
+  SpaceGlyphInOpenTypeTables space_in_gpos_;
+  SpaceGlyphInOpenTypeTables space_in_gsub_;
+
   scoped_refptr<OpenTypeVerticalData> vertical_data_;
   scoped_refptr<UnicodeRangeSet> range_set_;
 };