Chrome not considering Underline Position from Font

Source/core/rendering/InlineTextBox.cpp

 /*
  * (C) 1999 Lars Knoll (knoll@kde.org)
  * (C) 2000 Dirk Mueller (mueller@kde.org)
  * Copyright (C) 2004, 2005, 2006, 2007, 2008, 2009, 2010, 2011 Apple Inc. All rights reserved.
  *
  * 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
  * version 2 of the License, or (at your option) any later version.
  *
@@ skipping 892 matching lines @@
         break;
     }
 
     return strokeStyle;
 }
 
 static int computeUnderlineOffset(const TextUnderlinePosition underlinePosition, const FontMetrics& fontMetrics, const InlineTextBox* inlineTextBox, const float textDecorationThickness)
 {
     // Compute the gap between the font and the underline. Use at least one
     // pixel gap, if underline is thick then use a bigger gap.
-    const int gap = std::max<int>(1, ceilf(textDecorationThickness / 2.f));
+    int gap = std::max<int>(1, ceilf(textDecorationThickness / 2.f));
+
+    // Underline position of zero means draw underline on Baseline Position,
+    // in Blink we need at least 1-pixel gap to adding following check. Default convention
+    // is Positive underline Pos means draw above baseline while Negative means draw below
+    // baseline, as Blink has Y increasing downwards so negating the value.
+    if (fontMetrics.underlinePosition())
+        gap = -fontMetrics.underlinePosition();

[eae, 2014/02/28 17:37:59]

Computing the gap and then immediately overwriting the value seems wasteful.

How about

if (fontMetrics.underlinePosition())
  gap = -fontMetrics.underlinePosition();
else
  gap = std::max<int>(1, ceilf(textDecorationThickness / 2.f));

or

const int gap = fontMetrics.underlinePosition()
    ? -fontMetrics.underlinePosition();
    : std::max<int>(1, ceilf(textDecorationThickness / 2.f));

preserving the comments, obviously.

[h.joshi, 2014/03/01 03:36:12]

Will make changes in next patch.

On 2014/02/28 17:37:59, eae wrote:

 
     // FIXME: We support only horizontal text for now.
     switch (underlinePosition) {
     case TextUnderlinePositionAuto:
         return fontMetrics.ascent() + gap; // Position underline near the alphabetic baseline.
     case TextUnderlinePositionUnder: {
         // Position underline relative to the under edge of the lowest element's content box.
         const float offset = inlineTextBox->root()->maxLogicalTop() - inlineTextBox->logicalTop();
         if (offset > 0)
             return inlineTextBox->logicalHeight() + gap + offset;
@@ skipping 638 matching lines @@
     printedCharacters = fprintf(stderr, "\t%s %p", obj->renderName(), obj);
     const int rendererCharacterOffset = 24;
     for (; printedCharacters < rendererCharacterOffset; printedCharacters++)
         fputc(' ', stderr);
     fprintf(stderr, "(%d,%d) \"%s\"\n", start(), start() + len(), value.utf8().data());
 }
 
 #endif
 
 } // namespace WebCore