Adjust visual overflow rect for rounded/shifted ascent/descent

third_party/WebKit/Source/core/layout/LayoutText.cpp

Index: third_party/WebKit/Source/core/layout/LayoutText.cpp
diff --git a/third_party/WebKit/Source/core/layout/LayoutText.cpp b/third_party/WebKit/Source/core/layout/LayoutText.cpp
index d34eda4f1c0fff3c89964ec7c5b100b0120097ca..3ddf8da83962d55dbc6cf2278acb6df99943fb0e 100644
--- a/third_party/WebKit/Source/core/layout/LayoutText.cpp
+++ b/third_party/WebKit/Source/core/layout/LayoutText.cpp
@@ -1939,6 +1939,20 @@ LayoutRect LayoutText::linesBoundingBox() const {
   return result;
 }
 
+static int visualOverflowAdjustmentForAscent(const ComputedStyle& style) {
+  return style.font()
+      .primaryFont()
+      ->getFontMetrics()
+      .visualOverflowAdjustmentForAscent();
+}
+
+static int visualOverflowAdjustmentForDescent(const ComputedStyle& style) {
+  return style.font()
+      .primaryFont()
+      ->getFontMetrics()
+      .visualOverflowAdjustmentForDescent();
+}
+
 LayoutRect LayoutText::visualOverflowRect() const {
   if (!firstTextBox())
     return LayoutRect();
@@ -1957,6 +1971,19 @@ LayoutRect LayoutText::visualOverflowRect() const {
   LayoutUnit logicalHeight =
       lastTextBox()->logicalBottomVisualOverflow() - logicalTop;
 
+  const auto& firstStyle = styleRef(firstTextBox()->isFirstLineStyle());

[eae, 2017/04/07 16:34:56]

If I understand the logic right you are inflating the visual overflow rect by
one pixel for both the ascent and decent if they were rounded down.

That certainly makes sense but the computations and the API for it seems rather
convoluted.

Instead of adding overflowAdjustmentForAscent etc and computing the adjustment
values in FontMetrics and SimpleFontData why not compare the ascent/decent and
floatAscent/floatDecent values here?


Something like:

  FontMetrics* metrics = ...;
  ascentAdjustment = metrics.floatAscent() > metrics.ascent() ? 1 : 0;
  decentAdjustment = metrics.floatDecent() > metrics.decent() ? 1 : 0;
  logicalTop -= ascentAdjustment;
  logicalHeight += ascentAdjustment + descentAdjustment;

[Xianzhu, 2017/04/07 17:27:11]

This won't work because
1. in many cases floatAscent() and floatDescent() are also rounded before they
are set:
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/fonts...
 
2. rounding is not the only cause, but also shifting:
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/fonts...

Perhaps we should remove both of the above code to let floatAscent/floatDescent
always equal to the orignal ascent/descent, but I would like that to be a
follow-up because we need to merge this patch into M58 and it should be on the
safe side.

+  int ascentAdjustment = visualOverflowAdjustmentForAscent(firstStyle);
+  int descentAdjustment = visualOverflowAdjustmentForDescent(firstStyle);
+  if (lastTextBox()->isFirstLineStyle() != firstTextBox()->isFirstLineStyle()) {
+    const auto& lastStyle = styleRef(lastTextBox()->isFirstLineStyle());

[eae, 2017/04/07 16:34:56]

The names here are bad. It's not really an ascentAjdutsment and a
descentAdjustment, it is a placement and height adjustment based on the rounding
of the ascent/decent values.

This could really use a couple of comments. I know this code pretty well but it
still took me a couple of minutes to understand what you're trying to do here. 

[Xianzhu, 2017/04/07 17:27:11]

Renamed visualOverflowAdjustmentXxx to visualOverflowInflationXxx, and
xxxAdjustment to xXXAdjustment to inflationForXxx. Also simplified code here.

+    ascentAdjustment = std::max(ascentAdjustment,
+                                visualOverflowAdjustmentForAscent(lastStyle));
+    descentAdjustment = std::max(descentAdjustment,
+                                 visualOverflowAdjustmentForDescent(lastStyle));
+  }
+  logicalTop -= ascentAdjustment;
+  logicalHeight += ascentAdjustment + descentAdjustment;
+
   LayoutRect rect(logicalLeftSide, logicalTop, logicalWidth, logicalHeight);
   if (!style()->isHorizontalWritingMode())
     rect = rect.transposedRect();