Adjust visual overflow rect for rounded/shifted ascent/descent

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

 /*
  * (C) 1999 Lars Knoll (knoll@kde.org)
  * (C) 2000 Dirk Mueller (mueller@kde.org)
  * Copyright (C) 2004, 2005, 2006, 2007 Apple Inc. All rights reserved.
  * Copyright (C) 2006 Andrew Wellington (proton@wiretapped.net)
  * Copyright (C) 2006 Graham Dennis (graham.dennis@gmail.com)
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Library General Public
  * License as published by the Free Software Foundation; either
@@ skipping 1921 matching lines @@
     float width = isHorizontal ? logicalRightSide - logicalLeftSide
                                : lastTextBox()->logicalBottom() - x;
     float height = isHorizontal ? lastTextBox()->logicalBottom() - y
                                 : logicalRightSide - logicalLeftSide;
     result = enclosingLayoutRect(FloatRect(x, y, width, height));
   }
 
   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();
 
   // Return the width of the minimal left side and the maximal right side.
   LayoutUnit logicalLeftSide = LayoutUnit::max();
   LayoutUnit logicalRightSide = LayoutUnit::min();
   for (InlineTextBox* curr = firstTextBox(); curr; curr = curr->nextTextBox()) {
     LayoutRect logicalVisualOverflow = curr->logicalOverflowRect();
     logicalLeftSide = std::min(logicalLeftSide, logicalVisualOverflow.x());
     logicalRightSide = std::max(logicalRightSide, logicalVisualOverflow.maxX());
   }
 
   LayoutUnit logicalTop = firstTextBox()->logicalTopVisualOverflow();
   LayoutUnit logicalWidth = logicalRightSide - logicalLeftSide;
   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();
   return rect;
 }
 
 LayoutRect LayoutText::localVisualRect() const {
   if (style()->visibility() != EVisibility::kVisible)
     return LayoutRect();
 
@@ skipping 122 matching lines @@
   LayoutRect rect = LayoutRect(
       IntRect(firstRunX(), firstRunY(), linesBox.width(), linesBox.height()));
   LayoutBlock* block = containingBlock();
   if (block && hasTextBoxes())
     block->adjustChildDebugRect(rect);
 
   return rect;
 }
 
 }  // namespace blink