Rework list marker spacing for better web compatibility.

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

Index: third_party/WebKit/Source/core/layout/LayoutListMarker.cpp
diff --git a/third_party/WebKit/Source/core/layout/LayoutListMarker.cpp b/third_party/WebKit/Source/core/layout/LayoutListMarker.cpp
index e44011989136eb710c69f07f7d3fbbd11ed657bc..ae487028c07e756099f9607397cd8efb2ee433ff 100644
--- a/third_party/WebKit/Source/core/layout/LayoutListMarker.cpp
+++ b/third_party/WebKit/Source/core/layout/LayoutListMarker.cpp
@@ -36,7 +36,10 @@
 
 namespace blink {
 
-const int cMarkerPadding = 7;
+// TODO(wkorman): A seemingly arbitrary number tacked on following any image
+// and included as part of margin for any text. Look into history, other
+// browser behavior, and update or remove as appropriate.
+const int cMarkerPaddingPx = 7;
 
 enum SequenceType { NumericSequence, AlphabeticSequence };
 
@@ -1034,8 +1037,11 @@ LayoutRect LayoutListMarker::localSelectionRect() const
     if (!box)
         return LayoutRect(LayoutPoint(), size());
     RootInlineBox& root = inlineBoxWrapper()->root();
-    LayoutUnit newLogicalTop = root.block().style()->isFlippedBlocksWritingMode() ? inlineBoxWrapper()->logicalBottom() - root.selectionBottom() : root.selectionTop() - inlineBoxWrapper()->logicalTop();
-    if (root.block().style()->isHorizontalWritingMode())
+    const ComputedStyle* blockStyle = root.block().style();
+    LayoutUnit newLogicalTop = blockStyle->isFlippedBlocksWritingMode()
+        ? inlineBoxWrapper()->logicalBottom() - root.selectionBottom()
+        : root.selectionTop() - inlineBoxWrapper()->logicalTop();
+    if (blockStyle->isHorizontalWritingMode())
         return LayoutRect(0, newLogicalTop, size().width(), root.selectionHeight());
     return LayoutRect(newLogicalTop, 0, root.selectionHeight(), size().height());
 }
@@ -1090,34 +1096,15 @@ void LayoutListMarker::updateMarginsAndContent()
     updateMargins();
 }
 
-void LayoutListMarker::updateContent()
+LayoutListMarker::ListStyleCategory LayoutListMarker::listStyleCategory() const
 {
-    // FIXME: This if-statement is just a performance optimization, but it's messy to use the preferredLogicalWidths dirty bit for this.
-    // It's unclear if this is a premature optimization.
-    if (!preferredLogicalWidthsDirty())
-        return;
-
-    m_text = "";
-
-    if (isImage()) {
-        // FIXME: This is a somewhat arbitrary width.  Generated images for markers really won't become particularly useful
-        // until we support the CSS3 marker pseudoclass to allow control over the width and height of the marker box.
-        int bulletWidth = style()->fontMetrics().ascent() / 2;
-        IntSize defaultBulletSize(bulletWidth, bulletWidth);
-        IntSize imageSize = calculateImageIntrinsicDimensions(m_image.get(), defaultBulletSize, DoNotScaleByEffectiveZoom);
-        m_image->setContainerSizeForLayoutObject(this, imageSize, style()->effectiveZoom());
-        return;
-    }
-
-    EListStyleType type = style()->listStyleType();
-    switch (type) {
+    switch (style()->listStyleType()) {
     case NoneListStyle:
-        break;
-    case Circle:
+        return NoneListCategory;
     case Disc:
+    case Circle:
     case Square:
-        m_text = listMarkerText(type, 0); // value is ignored for these types
-        break;
+        return SymbolListCategory;
     case ArabicIndic:
     case Armenian:
     case Bengali:
@@ -1135,12 +1122,12 @@ void LayoutListMarker::updateContent()
     case Georgian:
     case Gujarati:
     case Gurmukhi:
+    case Hebrew:
     case Hangul:
     case HangulConsonant:
     case KoreanHangulFormal:
     case KoreanHanjaFormal:
     case KoreanHanjaInformal:
-    case Hebrew:
     case Hiragana:
     case HiraganaIroha:
     case Kannada:
@@ -1170,11 +1157,57 @@ void LayoutListMarker::updateContent()
     case UpperLatin:
     case UpperRoman:
     case Urdu:
-        m_text = listMarkerText(type, m_listItem->value());
+        return LanguageListCategory;
+    default:
+        return NoneListCategory;
+    }
+}
+
+void LayoutListMarker::updateContent()
+{
+    // FIXME: This if-statement is just a performance optimization, but it's messy to use the preferredLogicalWidths dirty bit for this.
+    // It's unclear if this is a premature optimization.
+    if (!preferredLogicalWidthsDirty())
+        return;
+
+    m_text = "";
+
+    if (isImage()) {
+        // FIXME: This is a somewhat arbitrary width.  Generated images for markers really won't become particularly useful
+        // until we support the CSS3 marker pseudoclass to allow control over the width and height of the marker box.
+        int bulletWidth = style()->fontMetrics().ascent() / 2;
+        IntSize defaultBulletSize(bulletWidth, bulletWidth);
+        IntSize imageSize = calculateImageIntrinsicDimensions(m_image.get(), defaultBulletSize, DoNotScaleByEffectiveZoom);
+        m_image->setContainerSizeForLayoutObject(this, imageSize, style()->effectiveZoom());
+        return;
+    }
+
+    switch (listStyleCategory()) {
+    case NoneListCategory:
+        break;
+    case SymbolListCategory:
+        m_text = listMarkerText(style()->listStyleType(), 0); // value is ignored for these types
+        break;
+    case LanguageListCategory:
+        m_text = listMarkerText(style()->listStyleType(), m_listItem->value());
         break;
     }
 }
 
+LayoutUnit LayoutListMarker::getWidthOfTextWithSuffix(UChar* suffix, int suffixLength) const
+{
+    if (m_text.isEmpty())
+        return 0;
+
+    const Font& font = style()->font();
+    LayoutUnit itemWidth = font.width(m_text);
+    // TODO(wkorman): Look into constructing a text run for both text and suffix
+    // and painting them together.
+    TextRun run = constructTextRun(font, suffix, suffixLength, styleRef(), style()->direction());
+    LayoutUnit suffixSpaceWidth = font.width(run);
+    return itemWidth + suffixSpaceWidth;
+}
+
 void LayoutListMarker::computePreferredLogicalWidths()
 {
     ASSERT(preferredLogicalWidthsDirty());
@@ -1188,77 +1221,24 @@ void LayoutListMarker::computePreferredLogicalWidths()
         return;
     }
 
-    const Font& font = style()->font();
-
     LayoutUnit logicalWidth = 0;
-    EListStyleType type = style()->listStyleType();
-    switch (type) {
-    case NoneListStyle:
+    switch (listStyleCategory()) {
+    case NoneListCategory:
         break;
-    case Circle:
-    case Disc:
-    case Square:
-        logicalWidth = (font.fontMetrics().ascent() * 2 / 3 + 1) / 2 + 2;
+    case SymbolListCategory: {
+        // For these styles, the text of the marker will be the single character itself,
+        // and so the only suffix will be a single space character to provide sufficient
+        // separation between the list marker text and the associated list item text.
+        UChar suffix[1] = { listMarkerSuffix(style()->listStyleType(), m_listItem->value()) };
+        logicalWidth = getWidthOfTextWithSuffix(suffix, 1);
+        }
         break;
-    case ArabicIndic:
-    case Armenian:
-    case Bengali:
-    case Cambodian:
-    case CJKIdeographic:
-    case CjkEarthlyBranch:
-    case CjkHeavenlyStem:
-    case DecimalLeadingZero:
-    case DecimalListStyle:
-    case Devanagari:
-    case EthiopicHalehame:
-    case EthiopicHalehameAm:
-    case EthiopicHalehameTiEr:
-    case EthiopicHalehameTiEt:
-    case Georgian:
-    case Gujarati:
-    case Gurmukhi:
-    case Hangul:
-    case HangulConsonant:
-    case KoreanHangulFormal:
-    case KoreanHanjaFormal:
-    case KoreanHanjaInformal:
-    case Hebrew:
-    case Hiragana:
-    case HiraganaIroha:
-    case Kannada:
-    case Katakana:
-    case KatakanaIroha:
-    case Khmer:
-    case Lao:
-    case LowerAlpha:
-    case LowerArmenian:
-    case LowerGreek:
-    case LowerLatin:
-    case LowerRoman:
-    case Malayalam:
-    case Mongolian:
-    case Myanmar:
-    case Oriya:
-    case Persian:
-    case SimpChineseFormal:
-    case SimpChineseInformal:
-    case Telugu:
-    case Thai:
-    case Tibetan:
-    case TradChineseFormal:
-    case TradChineseInformal:
-    case UpperAlpha:
-    case UpperArmenian:
-    case UpperLatin:
-    case UpperRoman:
-    case Urdu:
-        if (m_text.isEmpty()) {
-            logicalWidth = 0;
-        } else {
-            LayoutUnit itemWidth = font.width(m_text);
-            UChar suffixSpace[2] = { listMarkerSuffix(type, m_listItem->value()), ' ' };
-            LayoutUnit suffixSpaceWidth = font.width(constructTextRun(font, suffixSpace, 2, styleRef(), style()->direction()));
-            logicalWidth = itemWidth + suffixSpaceWidth;
+    case LanguageListCategory: {
+        // For these styles, we have some kind of a language-specific separating
+        // character, followed by a space character to provide sufficient
+        // separation between the list marker text and the associated list item text.
+        UChar suffixSpace[2] = { listMarkerSuffix(style()->listStyleType(), m_listItem->value()), ' ' };
+        logicalWidth = getWidthOfTextWithSuffix(suffixSpace, 2);
         }
         break;
     }
@@ -1273,65 +1253,30 @@ void LayoutListMarker::computePreferredLogicalWidths()
 
 void LayoutListMarker::updateMargins()
 {
-    const FontMetrics& fontMetrics = style()->fontMetrics();
-
     LayoutUnit marginStart = 0;
     LayoutUnit marginEnd = 0;
 
     if (isInside()) {
         if (isImage()) {
-            marginEnd = cMarkerPadding;
-        } else {
-            switch (style()->listStyleType()) {
-            case Disc:
-            case Circle:
-            case Square:
-                marginStart = -1;
-                marginEnd = fontMetrics.ascent() - minPreferredLogicalWidth() + 1;
-                break;
-            default:
-                break;
-            }
+            marginEnd = cMarkerPaddingPx;
+        } else if (listStyleCategory() == SymbolListCategory) {
+            marginStart = minPreferredLogicalWidth();
+            marginEnd = 0;
         }
     } else {
         if (style()->isLeftToRightDirection()) {
-            if (isImage()) {
-                marginStart = -minPreferredLogicalWidth() - cMarkerPadding;
-            } else {
-                int offset = fontMetrics.ascent() * 2 / 3;
-                switch (style()->listStyleType()) {
-                case Disc:
-                case Circle:
-                case Square:
-                    marginStart = -offset - cMarkerPadding - 1;
-                    break;
-                case NoneListStyle:
-                    break;
-                default:
-                    marginStart = m_text.isEmpty() ? LayoutUnit() : -minPreferredLogicalWidth() - offset / 2;
-                }
-            }
+            if (isImage())
+                marginStart = -minPreferredLogicalWidth() - cMarkerPaddingPx;
+            else if (style()->listStyleType() != NoneListStyle)
+                marginStart = m_text.isEmpty() ? LayoutUnit() : -minPreferredLogicalWidth();
             marginEnd = -marginStart - minPreferredLogicalWidth();
         } else {
-            if (isImage()) {
-                marginEnd = cMarkerPadding;
-            } else {
-                int offset = fontMetrics.ascent() * 2 / 3;
-                switch (style()->listStyleType()) {
-                case Disc:
-                case Circle:
-                case Square:
-                    marginEnd = offset + cMarkerPadding + 1 - minPreferredLogicalWidth();
-                    break;
-                case NoneListStyle:
-                    break;
-                default:
-                    marginEnd = m_text.isEmpty() ? 0 : offset / 2;
-                }
-            }
+            if (isImage())
+                marginEnd = cMarkerPaddingPx;
+            else if (style()->listStyleType() != NoneListStyle)
+                marginEnd = 0;
             marginStart = -marginEnd - minPreferredLogicalWidth();
         }
-
     }
 
     mutableStyleRef().setMarginStart(Length(marginStart, Fixed));
@@ -1364,79 +1309,27 @@ IntRect LayoutListMarker::getRelativeMarkerRect() const
         return IntRect(0, 0, m_image->imageSize(this, style()->effectiveZoom()).width(), m_image->imageSize(this, style()->effectiveZoom()).height());
 
     IntRect relativeRect;
-    EListStyleType type = style()->listStyleType();
-    switch (type) {
-    case Disc:
-    case Circle:
-    case Square: {
-        // FIXME: Are these particular rounding rules necessary?
-        const FontMetrics& fontMetrics = style()->fontMetrics();
-        int ascent = fontMetrics.ascent();
-        int bulletWidth = (ascent * 2 / 3 + 1) / 2;
-        relativeRect = IntRect(1, 3 * (ascent - ascent * 2 / 3) / 2, bulletWidth, bulletWidth);
-        break;
-    }
-    case NoneListStyle:
+    switch (listStyleCategory()) {
+    case NoneListCategory:
         return IntRect();
-    case ArabicIndic:
-    case Armenian:
-    case Bengali:
-    case Cambodian:
-    case CJKIdeographic:
-    case CjkEarthlyBranch:
-    case CjkHeavenlyStem:
-    case DecimalLeadingZero:
-    case DecimalListStyle:
-    case Devanagari:
-    case EthiopicHalehame:
-    case EthiopicHalehameAm:
-    case EthiopicHalehameTiEr:
-    case EthiopicHalehameTiEt:
-    case Georgian:
-    case Gujarati:
-    case Gurmukhi:
-    case Hangul:
-    case KoreanHangulFormal:
-    case KoreanHanjaFormal:
-    case KoreanHanjaInformal:
-    case HangulConsonant:
-    case Hebrew:
-    case Hiragana:
-    case HiraganaIroha:
-    case Kannada:
-    case Katakana:
-    case KatakanaIroha:
-    case Khmer:
-    case Lao:
-    case LowerAlpha:
-    case LowerArmenian:
-    case LowerGreek:
-    case LowerLatin:
-    case LowerRoman:
-    case Malayalam:
-    case Mongolian:
-    case Myanmar:
-    case Oriya:
-    case Persian:
-    case SimpChineseFormal:
-    case SimpChineseInformal:
-    case Telugu:
-    case Thai:
-    case Tibetan:
-    case TradChineseFormal:
-    case TradChineseInformal:
-    case UpperAlpha:
-    case UpperArmenian:
-    case UpperLatin:
-    case UpperRoman:
-    case Urdu:
-        if (m_text.isEmpty())
-            return IntRect();
-        const Font& font = style()->font();
-        int itemWidth = font.width(m_text);
-        UChar suffixSpace[2] = { listMarkerSuffix(type, m_listItem->value()), ' ' };
-        int suffixSpaceWidth = font.width(constructTextRun(font, suffixSpace, 2, styleRef(), style()->direction()));
-        relativeRect = IntRect(0, 0, itemWidth + suffixSpaceWidth, font.fontMetrics().height());
+    case SymbolListCategory: {
+        // For these styles, the text of the marker will be the single character itself,
+        // and so the only suffix will be a single space character to provide sufficient
+        // separation between the list marker text and the associated list item text.
+        UChar suffix[1] = { listMarkerSuffix(style()->listStyleType(), m_listItem->value()) };
+        LayoutUnit itemWidth = getWidthOfTextWithSuffix(suffix, 1);
+        relativeRect = IntRect(0, 0, itemWidth, style()->font().fontMetrics().height());
+        }
+        break;
+    case LanguageListCategory: {
+        // For these styles, we have some kind of a language-specific separating
+        // character, followed by a space character to provide sufficient
+        // separation between the list marker text and the associated list item text.
+        UChar suffixSpace[2] = { listMarkerSuffix(style()->listStyleType(), m_listItem->value()), ' ' };
+        LayoutUnit itemWidth = getWidthOfTextWithSuffix(suffixSpace, 2);
+        relativeRect = IntRect(0, 0, itemWidth, style()->font().fontMetrics().height());
+        }
+        break;
     }
 
     if (!style()->isHorizontalWritingMode()) {