[mathml] Simplification and cleanup in stretchy operator shaper code

This CL improves the stretchy operator shaper code after more testing
in MathML layout. The following changes are performed:

* Add an OUT Metrics parameter to StretchyOperatorShaper::Shape and
  improve this function. As discussed in [1], it is expected that
  LayoutNG algorithms perform both text shaping and measurements,
  and pass the shape result to the painter classes, so it is not
  necessary to have a separate GetMetrics method.

* Remove GetGlyphStretchSize and ToMetrics helper functions since
  they are now only used in only one place.

* Remove the stretch axis direction parameter from the version of
  CreateForStretchyMathOperator that outputs a single glyph since
  MathML only supports horizontal mode and vertical operators
  still correspond to horizontal text.

* Improve code documentation and update tests.

[1] https://chromium-review.googlesource.com/c/chromium/src/+/2051923

Bug: 6606
Change-Id: Icc755adf9460ac59b68e0d904c062296700fd3c0
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2144110Reviewed-by: Koji Ishii <kojii@chromium.org>
Commit-Queue: Frédéric Wang <fwang@igalia.com>
Cr-Commit-Position: refs/heads/master@{#758495}

third_party/blink/renderer/platform/fonts/shaping/shape_result.cc

@@ -1453,18 +1453,14 @@ scoped_refptr<ShapeResult> ShapeResult::CreateForSpaces(const Font* font,
 scoped_refptr<ShapeResult> ShapeResult::CreateForStretchyMathOperator(
     const Font* font,
     TextDirection direction,
-    OpenTypeMathStretchData::StretchAxis stretch_axis,
     Glyph glyph_variant,
     float stretch_size) {
-  bool is_horizontal_assembly =
-      stretch_axis == OpenTypeMathStretchData::StretchAxis::Horizontal;
   unsigned start_index = 0;
   unsigned num_characters = 1;
   scoped_refptr<ShapeResult> result =
       ShapeResult::Create(font, start_index, num_characters, direction);
 
-  hb_direction_t hb_direction =
-      is_horizontal_assembly ? HB_DIRECTION_LTR : HB_DIRECTION_TTB;
+  hb_direction_t hb_direction = HB_DIRECTION_LTR;
   unsigned glyph_index = 0;
   scoped_refptr<ShapeResult::RunInfo> run = RunInfo::Create(
       font->PrimaryFont(), hb_direction, CanvasRotationInVertical::kRegular,

third_party/blink/renderer/platform/fonts/shaping/shape_result.h

@@ -146,7 +146,6 @@ class PLATFORM_EXPORT ShapeResult : public RefCounted<ShapeResult> {
   static scoped_refptr<ShapeResult> CreateForStretchyMathOperator(
       const Font*,
       TextDirection,
-      OpenTypeMathStretchData::StretchAxis,
       Glyph,
       float stretch_size);
   static scoped_refptr<ShapeResult> CreateForStretchyMathOperator(

third_party/blink/renderer/platform/fonts/shaping/stretchy_operator_shaper.cc

@@ -27,18 +27,6 @@ inline float HarfBuzzUnitsToFloat(hb_position_t value) {
   return kFloatToHbRatio * value;
 }
 
-inline float GetGlyphStretchSize(
-    FloatRect bounds,
-    OpenTypeMathStretchData::StretchAxis stretch_axis) {
-  return stretch_axis == OpenTypeMathStretchData::StretchAxis::Horizontal
-             ? bounds.Width()
-             : bounds.Height();
-}
-
-inline StretchyOperatorShaper::Metrics ToMetrics(FloatRect bounds) {
-  return {bounds.Width(), -bounds.Y(), bounds.MaxY()};
-}
-
 base::Optional<OpenTypeMathStretchData::AssemblyParameters>
 GetAssemblyParameters(const HarfBuzzFace* harfbuzz_face,
                       Glyph base_glyph,
@@ -138,57 +126,16 @@ GetAssemblyParameters(const HarfBuzzFace* harfbuzz_face,
 
 }  // namespace
 
-StretchyOperatorShaper::Metrics StretchyOperatorShaper::GetMetrics(
-    const Font* font,
-    float target_size) const {
-  const SimpleFontData* primary_font = font->PrimaryFont();
-  const HarfBuzzFace* harfbuzz_face =
-      primary_font->PlatformData().GetHarfBuzzFace();
-  Glyph base_glyph = primary_font->GlyphForCharacter(stretchy_character_);
-
-  FloatRect bounds;
-
-  // Try different glyph variants.
-  for (auto& variant : OpenTypeMathSupport::GetGlyphVariantRecords(
-           harfbuzz_face, base_glyph, stretch_axis_)) {
-    bounds = primary_font->BoundsForGlyph(variant);
-    if (GetGlyphStretchSize(bounds, stretch_axis_) >= target_size)
-      return ToMetrics(bounds);
-  }
-
-  // Try a glyph assembly.
-  auto params = GetAssemblyParameters(harfbuzz_face, base_glyph, stretch_axis_,
-                                      target_size);
-  if (!params)
-    return ToMetrics(bounds);
-
-  bounds = stretch_axis_ == OpenTypeMathStretchData::StretchAxis::Horizontal
-               ? FloatRect(0, 0, params->stretch_size, 0)
-               : FloatRect(0, -params->stretch_size, 0, params->stretch_size);
-
-  for (auto& part : params->parts) {
-    // Include dimension of the part, orthogonal to the stretch axis.
-    auto glyph_bounds = primary_font->BoundsForGlyph(part.glyph);
-    if (stretch_axis_ == OpenTypeMathStretchData::StretchAxis::Horizontal) {
-      glyph_bounds.SetX(0);
-      glyph_bounds.SetWidth(0);
-    } else {
-      glyph_bounds.SetY(0);
-      glyph_bounds.SetHeight(0);
-    }
-    bounds.UniteEvenIfEmpty(glyph_bounds);
-  }
-
-  return ToMetrics(bounds);
-}
-
 scoped_refptr<ShapeResult> StretchyOperatorShaper::Shape(
     const Font* font,
-    float target_size) const {
+    float target_size,
+    Metrics* metrics) const {
   const SimpleFontData* primary_font = font->PrimaryFont();
   const HarfBuzzFace* harfbuzz_face =
       primary_font->PlatformData().GetHarfBuzzFace();
   Glyph base_glyph = primary_font->GlyphForCharacter(stretchy_character_);
+  if (metrics)
+    *metrics = Metrics();
 
   Glyph glyph_variant;
   float glyph_variant_stretch_size;
@@ -198,12 +145,18 @@ scoped_refptr<ShapeResult> StretchyOperatorShaper::Shape(
   for (auto& variant : OpenTypeMathSupport::GetGlyphVariantRecords(
            harfbuzz_face, base_glyph, stretch_axis_)) {
     glyph_variant = variant;
-    auto bounds = primary_font->BoundsForGlyph(glyph_variant);
-    glyph_variant_stretch_size = GetGlyphStretchSize(bounds, stretch_axis_);
+    FloatRect bounds = primary_font->BoundsForGlyph(glyph_variant);
+    if (metrics) {
+      *metrics = {primary_font->WidthForGlyph(variant), -bounds.Y(),
+                  bounds.MaxY()};
+    }
+    glyph_variant_stretch_size =
+        stretch_axis_ == OpenTypeMathStretchData::StretchAxis::Horizontal
+            ? bounds.Width()
+            : bounds.Height();
     if (glyph_variant_stretch_size >= target_size) {
       return ShapeResult::CreateForStretchyMathOperator(
-          font, direction, stretch_axis_, glyph_variant,
-          glyph_variant_stretch_size);
+          font, direction, glyph_variant, glyph_variant_stretch_size);
     }
   }
 
@@ -212,12 +165,31 @@ scoped_refptr<ShapeResult> StretchyOperatorShaper::Shape(
                                       target_size);
   if (!params) {
     return ShapeResult::CreateForStretchyMathOperator(
-        font, direction, stretch_axis_, glyph_variant,
-        glyph_variant_stretch_size);
+        font, direction, glyph_variant, glyph_variant_stretch_size);
   }
 
-  return ShapeResult::CreateForStretchyMathOperator(
-      font, direction, stretch_axis_, std::move(*params));
+  scoped_refptr<ShapeResult> shape_result_for_glyph_assembly =
+      ShapeResult::CreateForStretchyMathOperator(font, direction, stretch_axis_,
+                                                 std::move(*params));
+  if (metrics) {
+    // The OpenType MATH specification does provide any distinction between
+    // the advance width and ink width, so the latter is returned here.
+    FloatRect bounds = shape_result_for_glyph_assembly->ComputeInkBounds();
+    if (stretch_axis_ == OpenTypeMathStretchData::StretchAxis::Horizontal) {
+      *metrics = {bounds.Width(), -bounds.Y(), bounds.MaxY()};
+    } else {
+      // For assemblies growing in the vertical direction, the distribution of
+      // height between ascent and descent is not defined by the OpenType MATH
+      // specification. This code uses MathML Core's convention of
+      // ascent = height and descent = 0.
+      // Additionally, ShapeResult::CreateForStretchyMathOperator uses a text
+      // run that is HB_DIRECTION_TTB in order to stack the parts vertically but
+      // the actual glyph assembly is still horizontal text, so height and width
+      // are inverted.
+      *metrics = {bounds.Height(), bounds.Width(), 0};
+    }
+  }
+  return shape_result_for_glyph_assembly;
 }
 
 }  // namespace blink

third_party/blink/renderer/platform/fonts/shaping/stretchy_operator_shaper.h

@@ -30,22 +30,20 @@ class PLATFORM_EXPORT StretchyOperatorShaper final {
                          OpenTypeMathStretchData::StretchAxis stretch_axis)
       : stretchy_character_(stretchy_character), stretch_axis_(stretch_axis) {}
 
-  // Returns the metrics of the stretched operator for layout purpose.
-  // May be called multiple times; font and direction may vary between calls.
-  // https://mathml-refresh.github.io/mathml-core/#dfn-box-metrics-of-a-stretchy-glyph
   struct Metrics {
     float advance;
     float ascent;
     float descent;
     // TODO(https://crbug.com/1057592): Add italic correction.
   };
-  Metrics GetMetrics(const Font*, float target_size) const;
-
   // Shape the stretched operator. The coordinates of the glyph(s) use the same
-  // origin as the rectangle returned by GetMetrics.
+  // origin as the rectangle assigned to the optional OUT Metrics parameter.
   // May be called multiple times; font and direction may vary between calls.
   // https://mathml-refresh.github.io/mathml-core/#dfn-shape-a-stretchy-glyph
-  scoped_refptr<ShapeResult> Shape(const Font*, float target_size) const;
+  // https://mathml-refresh.github.io/mathml-core/#dfn-box-metrics-of-a-stretchy-glyph
+  scoped_refptr<ShapeResult> Shape(const Font*,
+                                   float target_size,
+                                   Metrics* metrics = nullptr) const;
 
   ~StretchyOperatorShaper() = default;
 

third_party/blink/renderer/platform/fonts/shaping/stretchy_operator_shaper_test.cc

@@ -91,7 +91,8 @@ TEST_F(StretchyOperatorShaperTest, GlyphVariants) {
 
       // Metrics of horizontal size variants.
       {
-        auto metrics = horizontal_shaper.GetMetrics(&math, target_size);
+        StretchyOperatorShaper::Metrics metrics;
+        horizontal_shaper.Shape(&math, target_size, &metrics);
         EXPECT_NEAR(metrics.advance, (i + 1) * 1000, kSizeError);
         EXPECT_NEAR(metrics.ascent, 1000, kSizeError);
         EXPECT_FLOAT_EQ(metrics.descent, 0);
@@ -100,7 +101,8 @@ TEST_F(StretchyOperatorShaperTest, GlyphVariants) {
       // Metrics of vertical size variants.
 
       {
-        auto metrics = vertical_shaper.GetMetrics(&math, target_size);
+        StretchyOperatorShaper::Metrics metrics;
+        vertical_shaper.Shape(&math, target_size, &metrics);
         EXPECT_NEAR(metrics.advance, 1000, kSizeError);
         EXPECT_NEAR(metrics.ascent, (i + 1) * 1000, kSizeError);
         EXPECT_FLOAT_EQ(metrics.descent, 0);
@@ -170,7 +172,8 @@ TEST_F(StretchyOperatorShaperTest, GlyphVariants) {
 
     // Metrics of horizontal assembly.
     {
-      auto metrics = horizontal_shaper.GetMetrics(&math, target_size);
+      StretchyOperatorShaper::Metrics metrics;
+      horizontal_shaper.Shape(&math, target_size, &metrics);
       EXPECT_NEAR(metrics.advance, target_size, kSizeError);
       EXPECT_NEAR(metrics.ascent, 1000, kSizeError);
       EXPECT_FLOAT_EQ(metrics.descent, 0);
@@ -178,7 +181,8 @@ TEST_F(StretchyOperatorShaperTest, GlyphVariants) {
 
     // Metrics of vertical assembly.
     {
-      auto metrics = vertical_shaper.GetMetrics(&math, target_size);
+      StretchyOperatorShaper::Metrics metrics;
+      vertical_shaper.Shape(&math, target_size, &metrics);
       EXPECT_NEAR(metrics.advance, 1000, kSizeError);
       EXPECT_NEAR(metrics.ascent, target_size, kSizeError);
       EXPECT_FLOAT_EQ(metrics.descent, 0);