Reland "Fix defines for icu_subset"

This is a reland of commit 6dc1544

Original change's description:
> Fix defines for icu_subset
>
> Disabling the version suffix, but not supplying a lib suffix
> caused the ICU headers to try to re-define the "icu" namespace.
>
> This reworks the skunicode BUILD.gn rules to isolate the ICU
> specific defines that were causing issues. We now have several
> builds of skunicode that get bundled together (much like how
> the Bazel build works).
>
> This also fixes the plain text editor application, which was
> not using ICU/Harfbuzz as expected.
>
> Change-Id: Ia15dc9ad5d1da826608e6b4f9a137009fb5ed6bc
> Bug: b/40045064
> Reviewed-on: https://skia-review.googlesource.com/c/skia/+/833798
> Reviewed-by: Ben Wagner <bungeman@google.com>

Bug: b/40045064
Change-Id: Ic88db60f1f15f41d59f14d62a0aa9383725d911c
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/835516
Reviewed-by: Ben Wagner <bungeman@google.com>
Commit-Queue: Kevin Lubick <kjlubick@google.com>

BUILD.gn

@@ -2210,10 +2210,10 @@ if (skia_enable_tools) {
     if (skia_tools_require_resources) {
       defines += [ "SK_TOOLS_REQUIRE_RESOURCES" ]
     }
-    deps = [
-      ":flags",
-      "modules/svg",
-    ]
+    deps = [ ":flags" ]
+    if (skia_enable_svg) {
+      deps += [ "modules/svg" ]
+    }
     public_deps = [ ":gpu_tool_utils" ]
   }
 

bazel/external/icu/BUILD.bazel

@@ -1021,9 +1021,9 @@ cc_library(
         "-Wno-unused-function",
     ],
     defines = [
-        "U_USING_ICU_NAMESPACE=0",
-        "U_DISABLE_RENAMING",
         "SK_USING_THIRD_PARTY_ICU",
+        "U_DISABLE_RENAMING=1",
+        "U_USING_ICU_NAMESPACE=0",
     ],
     includes = [
         "source/common",

modules/skplaintexteditor/BUILD.gn

@@ -30,15 +30,15 @@ if ((skia_use_icu || skia_use_libgrapheme || skia_use_icu4x) &&
     deps = [
       ":word_boundaries",
       "../../modules/skshaper",
+      "../../modules/skunicode",
     ]
   }
 
   skia_source_set("word_boundaries") {
     include_dirs = [ "../.." ]
     public = [ "src/word_boundaries.h" ]
     sources = [ "src/word_boundaries.cpp" ]
-    configs = [ "../../third_party/icu/config:no_cxx" ]
-    deps = [ "//third_party/icu" ]
+    deps = [ "../../modules/skunicode" ]
   }
 
   skia_source_set("editor_app") {

modules/skplaintexteditor/app/editor_application.cpp

@@ -15,8 +15,6 @@
 
 #include "modules/skplaintexteditor/include/editor.h"
 
-#include "third_party/icu/SkLoadICU.h"
-
 #if defined(SK_FONTMGR_FONTCONFIG_AVAILABLE)
 #include "include/ports/SkFontMgr_fontconfig.h"
 #endif
@@ -447,9 +445,6 @@ struct EditorApplication : public sk_app::Application {
 }  // namespace
 
 sk_app::Application* sk_app::Application::Create(int argc, char** argv, void* dat) {
-    if (!SkLoadICU()) {
-        SK_ABORT("SkLoadICU failed.");
-    }
     std::unique_ptr<sk_app::Window> win(sk_app::Window::CreateNativeWindow(dat));
     if (!win) {
         SK_ABORT("CreateNativeWindow failed.");

modules/skplaintexteditor/src/shape.cpp

@@ -26,6 +26,16 @@
 class SkUnicode;
 #endif
 
+#if defined(SK_UNICODE_ICU_IMPLEMENTATION)
+#include "modules/skunicode/include/SkUnicode_icu.h"
+#endif
+#if defined(SK_UNICODE_LIBGRAPHEME_IMPLEMENTATION)
+#include "modules/skunicode/include/SkUnicode_libgrapheme.h"
+#endif
+#if defined(SK_UNICODE_ICU4X_IMPLEMENTATION)
+#include "modules/skunicode/include/SkUnicode_icu4x.h"
+#endif
+
 #include <cfloat>
 #include <climits>
 #include <cstring>

modules/skunicode/BUILD.gn

@@ -14,7 +14,6 @@ declare_args() {
 if (skia_use_icu || skia_use_client_icu || skia_use_libgrapheme ||
     skia_use_icu4x) {
   config("public_config") {
-    include_dirs = [ "include" ]
     defines = [ "SK_UNICODE_AVAILABLE" ]
     if (skia_use_icu) {
       defines += [ "SK_UNICODE_ICU_IMPLEMENTATION" ]
@@ -28,9 +27,6 @@ if (skia_use_icu || skia_use_client_icu || skia_use_libgrapheme ||
     if (skia_use_icu4x) {
       defines += [ "SK_UNICODE_ICU4X_IMPLEMENTATION" ]
     }
-    if (is_component_build) {
-      defines += [ "SKUNICODE_DLL" ]
-    }
   }
 
   config("cpp20") {
@@ -41,24 +37,47 @@ if (skia_use_icu || skia_use_client_icu || skia_use_libgrapheme ||
     }
   }
 
-  component("skunicode") {
+  config("module") {
+    defines = [ "SKUNICODE_IMPLEMENTATION=1" ]
+    if (is_component_build) {
+      defines += [ "SKUNICODE_DLL" ]
+    }
+  }
+
+  component("skunicode_core") {
     check_includes = false
-    public_configs = [ ":public_config" ]
-    public = skia_unicode_public
     deps = [ "../..:skia" ]
-    defines = [ "SKUNICODE_IMPLEMENTATION=1" ]
-    sources = skia_unicode_sources
-    defines += [ "SK_UNICODE_AVAILABLE" ]
     configs += [
+      ":module",
       "../../:skia_private",
-      #":cpp20",
+      "../../third_party/icu/config:no_cxx",
     ]
+    sources = skia_unicode_sources
+  }
 
-    if (skia_use_icu) {
-      sources += skia_unicode_icu_bidi_sources
+  if (skia_use_icu) {
+    component("skunicode_icu") {
+      check_includes = false
+      deps = [
+        ":skunicode_core",
+        "../..:skia",
+      ]
+      configs += [
+        ":module",
+        "../../:skia_private",
+        "../../third_party/icu/config:no_cxx",
+      ]
+
+      # These are explicitly *not* public defines because we don't want them
+      # to leak to dependents.
+      defines = [
+        "U_DISABLE_RENAMING=1",
+        "U_USING_ICU_NAMESPACE=0",
+      ]
+
+      sources = skia_unicode_icu_bidi_sources
       sources += skia_unicode_bidi_full_sources
       sources += skia_unicode_icu_sources
-      defines += [ "SK_UNICODE_ICU_IMPLEMENTATION" ]
 
       # only available for Android at the moment
       if (skia_use_runtime_icu && (is_android || is_linux)) {
@@ -69,29 +88,109 @@ if (skia_use_icu || skia_use_client_icu || skia_use_libgrapheme ||
         sources += skia_unicode_builtin_icu_sources
         deps += [ "//third_party/icu" ]
       }
-      configs += [ "../../third_party/icu/config:no_cxx" ]
     }
-    if (skia_use_client_icu || skia_use_libgrapheme) {
-      if (!skia_use_icu) {
-        sources += skia_unicode_icu_bidi_sources
-      }
+  }
+
+  if (skia_use_client_icu) {
+    component("skunicode_client_icu") {
+      check_includes = false
+      deps = [
+        ":skunicode_core",
+        "../..:skia",
+      ]
+      configs += [
+        ":module",
+        "../../:skia_private",
+        "../../third_party/icu/config:no_cxx",
+      ]
+      defines = [
+        # In order to use the bidi_subset at the same time as "full ICU", we must have
+        # compiled icu with the given defines also being set. This is to make sure the functions
+        # we call are given a suffix of "_skia" to prevent ODR violations if this "subset of ICU"
+        # is compiled alongside a full ICU build also.
+        # See https://chromium.googlesource.com/chromium/deps/icu.git/+/d94ab131bc8fef3bc17f356a628d8e4cd44d65d9/source/common/unicode/uversion.h
+        # for how these are used.
+        "U_DISABLE_RENAMING=0",
+        "U_USING_ICU_NAMESPACE=0",
+        "U_LIB_SUFFIX_C_NAME=_skia",
+        "U_HAVE_LIB_SUFFIX=1",
+        "U_DISABLE_VERSION_SUFFIX=1",
+      ]
+
+      sources = skia_unicode_icu_bidi_sources
       sources += skia_unicode_bidi_subset_sources
+      sources += skia_unicode_client_icu_sources
       deps += [ skia_icu_bidi_third_party_dir ]
     }
+  }
+
+  if (skia_use_libgrapheme) {
+    component("skunicode_libgrapheme") {
+      check_includes = false
+      deps = [
+        ":skunicode_core",
+        "../..:skia",
+      ]
+      configs += [
+        ":module",
+        "../../:skia_private",
+        "../../third_party/icu/config:no_cxx",
+      ]
+      defines = [
+        "U_DISABLE_RENAMING=0",
+        "U_USING_ICU_NAMESPACE=0",
+        "U_LIB_SUFFIX_C_NAME=_skia",
+        "U_HAVE_LIB_SUFFIX=1",
+        "U_DISABLE_VERSION_SUFFIX=1",
+      ]
+
+      sources = skia_unicode_icu_bidi_sources
+      sources += skia_unicode_bidi_subset_sources
+
+      sources += skia_unicode_libgrapheme_sources
+      deps += [
+        skia_icu_bidi_third_party_dir,
+        skia_libgrapheme_third_party_dir,
+      ]
+    }
+  }
+
+  if (skia_use_icu4x) {
+    component("skunicode_icu4x") {
+      check_includes = false
+      deps = [
+        ":skunicode_core",
+        "../..:skia",
+      ]
+      configs += [
+        ":module",
+        "../../:skia_private",
+        "../../third_party/icu/config:no_cxx",
+      ]
+
+      sources = skia_unicode_icu4x_sources
+
+      deps += [ "//third_party/icu4x" ]
+    }
+  }
+
+  group("skunicode") {
+    public_configs = [ ":public_config" ]
+    public_deps = [ ":skunicode_core" ]
 
+    # We have these different flavors of skunicode as independent components because
+    # we have to set different defines for different builds of ICU.
+    if (skia_use_icu) {
+      public_deps += [ ":skunicode_icu" ]
+    }
     if (skia_use_client_icu) {
-      sources += skia_unicode_client_icu_sources
-      defines += [ "SK_UNICODE_CLIENT_IMPLEMENTATION" ]
+      public_deps += [ ":skunicode_client_icu" ]
     }
     if (skia_use_libgrapheme) {
-      sources += skia_unicode_libgrapheme_sources
-      defines += [ "SK_UNICODE_LIBGRAPHEME_IMPLEMENTATION" ]
-      deps += [ skia_libgrapheme_third_party_dir ]
+      public_deps += [ ":skunicode_libgrapheme" ]
     }
     if (skia_use_icu4x) {
-      sources += skia_unicode_icu4x_sources
-      defines += [ "SK_UNICODE_ICU4X_IMPLEMENTATION" ]
-      deps += [ "//third_party/icu4x" ]
+      public_deps += [ ":skunicode_icu4x" ]
     }
   }
 

modules/skunicode/src/SkBidiFactory_icu_subset.cpp

@@ -4,14 +4,6 @@
  * Use of this source code is governed by a BSD-style license that can be
  * found in the LICENSE file.
  */
-// In order to use this file, we must have compiled icu with the define
-// U_LIB_SUFFIX_C_NAME=_skia
-// so the functions we call are given a suffix of "_skia" to prevent
-// ODR violations if this "subset of ICU" is compiled alongside a full ICU build also.
-#define U_LIB_SUFFIX_C_NAME _skia
-#define U_HAVE_LIB_SUFFIX 1
-#define U_DISABLE_RENAMING 0
-
 #include "modules/skunicode/src/SkBidiFactory_icu_subset.h"
 
 #include <unicode/umachine.h>

modules/skunicode/src/SkUnicode_hardcoded.h

@@ -11,7 +11,7 @@
 #include "modules/skunicode/include/SkUnicode.h"
 #include "src/base/SkUTF.h"
 
-class SkUnicodeHardCodedCharProperties : public SkUnicode {
+class SKUNICODE_API SkUnicodeHardCodedCharProperties : public SkUnicode {
 public:
     bool isControl(SkUnichar utf8) override;
     bool isWhitespace(SkUnichar utf8) override;

modules/skunicode/src/SkUnicode_icu4x.cpp

@@ -4,42 +4,33 @@
 * Use of this source code is governed by a BSD-style license that can be
 * found in the LICENSE file.
 */
+#include "modules/skunicode/include/SkUnicode_icu4x.h"
 
-#include <cstdint>
-#include <memory>
-#include <unicode/umachine.h>
 #include "include/core/SkSpan.h"
 #include "include/core/SkString.h"
 #include "include/core/SkTypes.h"
 #include "include/private/base/SkTArray.h"
 #include "include/private/base/SkTo.h"
 #include "modules/skunicode/include/SkUnicode.h"
-#include "modules/skunicode/include/SkUnicode_icu4x.h"
 #include "modules/skunicode/src/SkUnicode_hardcoded.h"
 #include "src/base/SkBitmaskEnum.h"
 #include "src/base/SkUTF.h"
 
-#include <diplomat_runtime.hpp>
-
-//#include <ICU4XBidi.h>
-
+#include <ICU4XBidi.hpp>
+#include <ICU4XCaseMapper.hpp>
+#include <ICU4XCodePointMapData8.hpp>
+#include <ICU4XCodePointSetData.hpp>
 #include <ICU4XDataProvider.hpp>
 #include <ICU4XGraphemeClusterSegmenter.hpp>
 #include <ICU4XLineSegmenter.hpp>
 #include <ICU4XWordSegmenter.hpp>
-#include <ICU4XBidi.hpp>
 
-#include <cstdint>
-#include <unicode/umachine.h>
 #include <algorithm>
 #include <cstdint>
 #include <memory>
 #include <string>
 #include <utility>
 #include <vector>
-#include <ICU4XCodePointSetData.hpp>
-#include <ICU4XCodePointMapData8.hpp>
-#include <ICU4XCaseMapper.hpp>
 
 class SkUnicode_icu4x :  public SkUnicode {
 public:

modules/skunicode/src/SkUnicode_icu_builtin.cpp

@@ -4,17 +4,17 @@
  * Use of this source code is governed by a BSD-style license that can be
  * found in the LICENSE file.
  */
-
 #include "include/private/base/SkFeatures.h"  // IWYU pragma: keep
 #include "modules/skunicode/src/SkUnicode_icupriv.h"
 
-#include <unicode/ubrk.h>
-#include <unicode/uloc.h>
-#include <unicode/utypes.h>
 #include <memory>
 #include <type_traits>
 #include <utility>
 
+#include <unicode/ubrk.h>
+#include <unicode/uloc.h>
+#include <unicode/utypes.h>
+
 namespace {
 
 // ubrk_clone added as draft in ICU69 and Android API 31 (first ICU NDK).