Reland: Compile FreeType with HarfBuzz support

FreeType's autohinter uses HarfBuzz API to collect additional GSUB and GPOS mappings to detect ligatures that should be aligned by the autohinter. Previously we were not able to build FreeType with HarfBuzz support because of the cyclic dependency. This CL resolves the cyclic dependency by building a bootstrap FreeType in order to build HarfBuzz' hb_ft_* functions as a static library called harfbuzz-ng-ft. Then we build harfbuzz-ng separately (which does not depend on FreeType), then we build FreeType depending on harfbuzz-ng and harfbuzz-ng-ft. This CL also removes the previous pangoft2 link hack since the :harfbuzz-ng target now brings all symbols required by pangoft2. This resolves issues with fi and ffi ligatures in Roboto looking like they were shifted to a different baseline. I tried developing a pixel test for this, which works if I force usage of the FreeType autohinter through SkPaint::kSlight_Hinting, however we are currently unable to automatically test this since our Linux layout tests do not exercise the autohinting code and do not set this hinting mode, probably due to the special fontconfig settings that we are using for the layout tests. Manually verifying the Roboto ligatures however confirms that this works. Reland after revert in https://codereview.chromium.org/2879843003/ BUG=617168 Review-Url: https://codereview.chromium.org/2880223002 Cr-Commit-Position: refs/heads/master@{#472413}
Denger-Network · May 17, 2017 · fbec380 · fbec380
1 parent 1d357b6
commit fbec380
Show file tree

Hide file tree

Showing 9 changed files with 159 additions and 63 deletions.
diff --git a/build/config/freetype/BUILD.gn b/build/config/freetype/BUILD.gn
@@ -3,17 +3,7 @@
 # found in the LICENSE file.
 
 import("//build/config/features.gni")
-
-declare_args() {
-  # Blink needs a recent and properly build-configured FreeType version to
-  # support OpenType variations, color emoji and avoid security bugs. By default
-  # we ship and link such a version as part of Chrome. For distributions that
-  # prefer to keep linking to the version the system, FreeType must be newer
-  # than version 2.7.1 and have color bitmap support compiled in. WARNING:
-  # System FreeType configurations other than as described WILL INTRODUCE TEXT
-  # RENDERING AND SECURITY REGRESSIONS.
-  use_system_freetype = false
-}
+import("//build/config/freetype/freetype.gni")
 
 group("freetype") {
   if (use_system_freetype) {

diff --git a/build/config/freetype/freetype.gni b/build/config/freetype/freetype.gni
@@ -0,0 +1,14 @@
+# Copyright 2017 The Chromium Authors. All rights reserved.
+# Use of this source code is governed by a BSD-style license that can be
+# found in the LICENSE file.
+
+declare_args() {
+  # Blink needs a recent and properly build-configured FreeType version to
+  # support OpenType variations, color emoji and avoid security bugs. By default
+  # we ship and link such a version as part of Chrome. For distributions that
+  # prefer to keep linking to the version the system, FreeType must be newer
+  # than version 2.7.1 and have color bitmap support compiled in. WARNING:
+  # System FreeType configurations other than as described WILL INTRODUCE TEXT
+  # RENDERING AND SECURITY REGRESSIONS.
+  use_system_freetype = false
+}
diff --git a/chrome/browser/ui/libgtkui/BUILD.gn b/chrome/browser/ui/libgtkui/BUILD.gn
@@ -99,13 +99,6 @@ template("libgtkui") {
 
     defines = [ "LIBGTKUI_IMPLEMENTATION" ]
 
-    # GTK pulls pangoft2 as dependency, and pangoft2 depends on harfbuzz.
-    # To avoid missing indirectly referenced harfbuzz symbols from pango,
-    # some hack is required when bundled harfbuzz is used and component build is
-    # disabled.
-    # See crbug.com/462689 for details.
-    all_dependent_configs = [ "//third_party/harfbuzz-ng:pangoft2_link_hack" ]
-
     deps = invoker.deps + [
              "//base",
              "//base:i18n",
@@ -120,6 +113,11 @@ template("libgtkui") {
              "//content/public/browser",
              "//printing",
              "//skia",
+
+             # GTK pulls pangoft2, which requires HarfBuzz symbols. Since we
+             # link our own HarfBuzz, avoid mixing symbols from system HarfBuzz
+             # and own ones, hence the dependency to harfbuzz-ng here.
+             "//third_party/harfbuzz-ng",
              "//ui/aura",
              "//ui/base",
              "//ui/base/ime",

diff --git a/remoting/host/it2me/BUILD.gn b/remoting/host/it2me/BUILD.gn
@@ -52,13 +52,14 @@ source_set("common") {
   ]
 
   if (is_desktop_linux) {
-    # GTK2 pulls pangoft2 as dependency, and pangoft2 depends on harfbuzz.
-    # To avoid missing indirectly referenced harfbuzz symbols from pango,
-    # some hack is required when bundled harfbuzz is used and component build is
-    # disabled.
-    # See crbug.com/462689 for details.
-    all_dependent_configs = [ "//third_party/harfbuzz-ng:pangoft2_link_hack" ]
-    deps += [ "//build/config/linux/gtk" ]
+    deps += [
+      "//build/config/linux/gtk",
+
+      # GTK pulls pangoft2, which requires HarfBuzz symbols. Since we
+      # link our own HarfBuzz, avoid mixing symbols from system HarfBuzz
+      # and own ones, hence the dependency to harfbuzz-ng here.
+      "//third_party/harfbuzz-ng",
+    ]
   }
 }
 

diff --git a/third_party/WebKit/LayoutTests/TestExpectations b/third_party/WebKit/LayoutTests/TestExpectations
@@ -267,6 +267,8 @@ crbug.com/711807 external/wpt/css/CSS2/normal-flow/width-inherit-001.xht [ Skip
 
 # ====== Layout team owned tests to here ======
 
+crbug.com/617168 [ Linux ] fast/text/shaping/same-script-different-lang.html [ NeedsRebaseline ]
+
 # ====== LayoutNG-only failures from here ======
 # LayoutNG - is a new layout system for Blink.
 

diff --git a/third_party/freetype/BUILD.gn b/third_party/freetype/BUILD.gn
@@ -3,6 +3,8 @@
 # found in the LICENSE file.
 
 import("//build/config/chromecast_build.gni")
+import("//build/config/freetype/freetype.gni")
+import("//third_party/harfbuzz-ng/harfbuzz.gni")
 
 config("freetype_config") {
   include_dirs = [
@@ -28,6 +30,55 @@ config("freetype-warnings") {
   }
 }
 
+# This component is used to resolve a cyclic dependency between HarfBuzz and
+# FreeType. HarfBuzz needs basic FreeType glyph information symbols for its
+# hb-ft.* functions, FreeType needs HarfBuzz' OpenType parsing functionality in
+# the autohinting code. We start by building a minimum FreeType here - enough to
+# satisfy harfbuzz-ng-ft symbol requirements. Then we can build harfbuzz-ng-ft
+# based on the minimal FreeType and harfbuzz-ng which does not depend on
+# FreeType itself. Then we build FreeType depending on harfbuzz-ng-ft and
+# harfbuzz-ng.
+static_library("bootstrap_freetype_for_harfbuzz") {
+  visibility = [ "//third_party/harfbuzz-ng:harfbuzz-ng-ft", ":freetype" ]
+
+  defines = []
+
+  sources = [
+    "include/freetype-custom-config/ftmodule.h",
+    "include/freetype-custom-config/ftoption.h",
+    "src/src/base/ftbase.c",
+    "src/src/base/ftbitmap.c",
+    "src/src/base/ftsystem.c",
+  ]
+
+  if (is_mac && !is_component_build) {
+    defines += [ "MAC_RESTRICT_VISIBILITY" ]
+  }
+
+  defines += [
+    "FT2_BUILD_LIBRARY",
+    "DARWIN_NO_CARBON",
+
+    # Long directory name to avoid accidentally using wrong headers.
+    # GN currently does not escape '<' and '>' when generating xml based Visual
+    # Studio project files. As a result, use quotes instead of pointy brackets
+    # in these defines.
+    "FT_CONFIG_MODULES_H=\"freetype-custom-config/ftmodule.h\"",
+    "FT_CONFIG_OPTIONS_H=\"freetype-custom-config/ftoption.h\"",
+  ]
+
+  if (is_win && is_component_build) {
+    # Used for managing declspec(dllimport/export) visibility.
+    defines += [ "FT2_BUILD_DLL" ]
+  }
+
+  public_configs = [ ":freetype_config" ]
+  configs -= [ "//build/config/compiler:chromium_code" ]
+  configs += [ "//build/config/compiler:no_chromium_code" ]
+
+  configs += [ ":freetype-warnings" ]
+}
+
 component("freetype") {
   if (is_linux) {
     output_name = "freetype"
@@ -40,9 +91,7 @@ component("freetype") {
     "include/freetype-custom-config/ftmodule.h",
     "include/freetype-custom-config/ftoption.h",
     "src/src/autofit/autofit.c",
-    "src/src/base/ftbase.c",
     "src/src/base/ftbbox.c",
-    "src/src/base/ftbitmap.c",
     "src/src/base/ftfntfmt.c",
     "src/src/base/ftfstype.c",
     "src/src/base/ftgasp.c",
@@ -51,7 +100,6 @@ component("freetype") {
     "src/src/base/ftlcdfil.c",
     "src/src/base/ftmm.c",
     "src/src/base/ftstroke.c",
-    "src/src/base/ftsystem.c",
     "src/src/base/fttype1.c",
     "src/src/cff/cff.c",
     "src/src/gzip/ftgzip.c",
@@ -118,4 +166,19 @@ component("freetype") {
     "//third_party/libpng",
     "//third_party/zlib",
   ]
+
+  if (!use_system_freetype) {
+    deps += [
+    ":bootstrap_freetype_for_harfbuzz"
+    ]
+  }
+
+  if (!use_system_harfbuzz) {
+    deps += [
+      "//third_party/harfbuzz-ng:harfbuzz-ng-ft",
+      "//third_party/harfbuzz-ng:harfbuzz-ng-without-freetype",
+    ]
+  } else {
+    deps += [ "//third_party/harfbuzz-ng:harfbuzz-ng" ]
+  }
 }
diff --git a/third_party/freetype/include/freetype-custom-config/ftoption.h b/third_party/freetype/include/freetype-custom-config/ftoption.h
@@ -268,7 +268,7 @@ FT_BEGIN_HEADER
   /*                                                                       */
   /*   Define this macro if you want to enable this `feature'.             */
   /*                                                                       */
-/* #define FT_CONFIG_OPTION_USE_HARFBUZZ */
+#define FT_CONFIG_OPTION_USE_HARFBUZZ
 
 
   /*************************************************************************/

diff --git a/third_party/harfbuzz-ng/BUILD.gn b/third_party/harfbuzz-ng/BUILD.gn
@@ -7,15 +7,7 @@ import("//build/config/features.gni")
 import("//build/config/linux/pkg_config.gni")
 import("//build/config/ui.gni")
 import("//testing/libfuzzer/fuzzer_test.gni")
-
-declare_args() {
-  # Blink uses a cutting-edge version of Harfbuzz; most Linux distros do not
-  # contain a new enough version of the code to work correctly. However,
-  # ChromeOS chroots (i.e, real ChromeOS builds for devices) do contain a
-  # new enough version of the library, and so this variable exists so that
-  # ChromeOS can build against the system lib and keep binary sizes smaller.
-  use_system_harfbuzz = false
-}
+import("//third_party/harfbuzz-ng/harfbuzz.gni")
 
 if (use_system_harfbuzz) {
   import("//build/config/linux/pkg_config.gni")
@@ -26,6 +18,16 @@ if (use_system_harfbuzz) {
     public_configs = [ ":harfbuzz_pkgconfig" ]
   }
 } else {
+  # The :harfbuzz-ng target combines the staged build steps required to resolve
+  # the cyclic dependency between parts of FreeType and HarfBuzz.
+  group("harfbuzz-ng") {
+    public_deps = [
+      ":harfbuzz-ng-ft",
+      ":harfbuzz-ng-without-freetype",
+      "//build/config/freetype",
+    ]
+  }
+
   config("harfbuzz-ng_config") {
     include_dirs = [ "src" ]
   }
@@ -48,17 +50,7 @@ if (use_system_harfbuzz) {
     }
   }
 
-  # See also chrome/browser/ui/libgtkui/BUILD.gn which pulls this.
-  config("pangoft2_link_hack") {
-    if (is_linux && use_pango && !is_chromeos && !is_official_build &&
-        current_cpu != "arm" && current_cpu != "mipsel" && !is_component_build) {
-      # These symbols are referenced from libpangoft2, which will be
-      # dynamically linked later.
-      ldflags = [ "-Wl,-uhb_ft_face_create_cached,-uhb_glib_get_unicode_funcs" ]
-    }
-  }
-
-  static_library("harfbuzz-ng") {
+  static_library("harfbuzz-ng-without-freetype") {
     sources = [
       "src/hb-atomic-private.hh",
       "src/hb-blob.cc",
@@ -164,6 +156,11 @@ if (use_system_harfbuzz) {
       "src/hb.h",
     ]
 
+    if (is_component_build && !is_win) {
+      configs -= [ "//build/config/gcc:symbol_visibility_hidden" ]
+      configs += [ "//build/config/gcc:symbol_visibility_default" ]
+    }
+
     defines = [
       "HAVE_OT",
       "HAVE_ICU",
@@ -198,20 +195,6 @@ if (use_system_harfbuzz) {
       ]
     }
 
-    # When without -fvisibility=hidden for pango to use the harfbuzz
-    # in the tree, all symbols pango needs must be included, or
-    # pango uses mixed versions of harfbuzz and leads to crash.
-    # See crbug.com/462689.
-    if (is_linux && use_pango && !is_chromeos && !is_official_build &&
-        current_cpu != "arm" && current_cpu != "mipsel") {
-      deps += [ "//build/config/freetype" ]
-      configs -= [ "//build/config/gcc:symbol_visibility_hidden" ]
-      configs += [ "//build/config/gcc:symbol_visibility_default" ]
-      sources += [
-        "src/hb-ft.cc",
-        "src/hb-ft.h",
-      ]
-    }
     if (use_glib) {
       configs += [ "//build/config/linux:glib" ]
       sources += [
@@ -220,6 +203,39 @@ if (use_system_harfbuzz) {
       ]
     }
   }
+
+  static_library("harfbuzz-ng-ft") {
+    sources = [
+      "src/hb-ft.cc",
+      "src/hb-ft.h",
+    ]
+
+    if (is_component_build && !is_win) {
+      configs -= [ "//build/config/gcc:symbol_visibility_hidden" ]
+      configs += [ "//build/config/gcc:symbol_visibility_default" ]
+    }
+
+    configs -= [ "//build/config/compiler:chromium_code" ]
+    configs += [
+      "//build/config/compiler:no_chromium_code",
+
+      # Must be after no_chromium_code for warning flags to be ordered
+      # correctly.
+      ":harfbuzz_warnings",
+    ]
+    public_configs = [ ":harfbuzz-ng_config" ]
+
+    defines = [
+      "HAVE_OT",
+      "HAVE_ICU",
+      "HAVE_ICU_BUILTIN",
+      "HB_NO_MT",
+    ]
+
+    deps = [
+      "//third_party/freetype:bootstrap_freetype_for_harfbuzz",
+    ]
+  }
 }
 
 fuzzer_test("harfbuzz_fuzzer") {

diff --git a/third_party/harfbuzz-ng/harfbuzz.gni b/third_party/harfbuzz-ng/harfbuzz.gni
@@ -0,0 +1,12 @@
+# Copyright 2017 The Chromium Authors. All rights reserved.
+# Use of this source code is governed by a BSD-style license that can be
+# found in the LICENSE file.
+
+declare_args() {
+  # Blink uses a cutting-edge version of Harfbuzz; most Linux distros do not
+  # contain a new enough version of the code to work correctly. However,
+  # ChromeOS chroots (i.e, real ChromeOS builds for devices) do contain a
+  # new enough version of the library, and so this variable exists so that
+  # ChromeOS can build against the system lib and keep binary sizes smaller.
+  use_system_harfbuzz = false
+}