Consistently autosize markers in <ul> and <ol> lists.

Source/core/rendering/TextAutosizer.cpp

Index: Source/core/rendering/TextAutosizer.cpp
diff --git a/Source/core/rendering/TextAutosizer.cpp b/Source/core/rendering/TextAutosizer.cpp
index a794fbfaf8e3af44610a3c68ba1e3ea362cf9d73..55f38425316079cd366cd0ab6de170e90b8705ee 100644
--- a/Source/core/rendering/TextAutosizer.cpp
+++ b/Source/core/rendering/TextAutosizer.cpp
@@ -28,6 +28,7 @@
 #include "core/page/Settings.h"
 #include "core/platform/graphics/IntSize.h"
 #include "core/rendering/RenderListItem.h"
+#include "core/rendering/RenderListMarker.h"
 #include "core/rendering/RenderObject.h"
 #include "core/rendering/RenderText.h"
 #include "core/rendering/RenderView.h"
@@ -90,6 +91,38 @@ static RenderListItem* getAncestorListItem(const RenderObject* renderer)
     return (ancestor && ancestor->isListItem()) ? toRenderListItem(ancestor) : 0;
 }
 
+static RenderObject* getAncestorList(const RenderObject* renderer)
+{
+    RenderObject* ancestor = renderer->parent();
+    while (ancestor) {

[Julien - ping for review, 2013/06/20 16:11:51]

Nit: This really like a for-loop disguised as a while.

[timvolodine, 2013/06/21 15:22:32]

Done.

+        Node* parentNode = ancestor->generatingNode();
+        if (parentNode && (parentNode->hasTagName(olTag) || parentNode->hasTagName(ulTag)))
+            return ancestor;
+        ancestor = ancestor->parent();
+    }
+    return 0;
+}
+
+static float getWidestListItemMarker(const RenderObject* renderer)
+{
+    float maxWidth = 0;
+    for (RenderObject* child = renderer->firstChild(); child; child = child->nextSibling()) {
+        if (child->isListItem()) {

[Julien - ping for review, 2013/06/20 16:11:51]

We prefer early return to avoid having a lot of nested levels.

[timvolodine, 2013/06/21 15:22:32]

Done.

+            for (RenderObject* itemChild = child->firstChild(); itemChild; itemChild = itemChild->nextSibling()) {
+                if (itemChild->isListMarker()) {
+                    maxWidth = max<float>(maxWidth, toRenderListMarker(itemChild)->logicalWidth().toFloat());
+                    break;
+                }
+            }
+        }
+    }
+    return maxWidth;
+}
+
+
+
+
+
 TextAutosizer::TextAutosizer(Document* document)
     : m_document(document)
 {
@@ -216,9 +249,12 @@ void TextAutosizer::processContainer(float multiplier, RenderBlock* container, T
             if (localMultiplier != 1 && descendant->style()->textAutosizingMultiplier() == 1) {
                 setMultiplier(descendant, localMultiplier);
                 setMultiplier(descendant->parent(), localMultiplier); // Parent does line spacing.
+
                 if (RenderListItem* listItemAncestor = getAncestorListItem(descendant)) {
-                    if (listItemAncestor->style()->textAutosizingMultiplier() == 1)
-                        setMultiplier(listItemAncestor, localMultiplier);
+                    if (RenderObject* list = getAncestorList(listItemAncestor)) {

[Julien - ping for review, 2013/06/20 16:11:51]

Don't you expect a listItem to have an enclosing list?

[timvolodine, 2013/06/21 15:22:32]

what if the html is broken? even in that case autosizer will not crash.

+                        if (list->style()->textAutosizingMultiplier() == 1)
+                            setMultiplierForList(list, localMultiplier);
+                    }
                 }
             }
         } else if (isAutosizingContainer(descendant)) {
@@ -244,6 +280,23 @@ void TextAutosizer::setMultiplier(RenderObject* renderer, float multiplier)
     renderer->setStyle(newStyle.release());
 }
 
+void TextAutosizer::setMultiplierForList(RenderObject* renderer, float multiplier)
+{
+    ASSERT(Node* parentNode = renderer->generatingNode()
+        && (parentNode->hasTagName(olTag) || parentNode->hasTagName(ulTag)));

[Julien - ping for review, 2013/06/20 16:11:51]

A logical AND in an ASSERT usually means that it should be split in 2. Here:

#ifndef NDEBUG
Node* parentNode = renderer->generatingNode();
ASSERT(parentNode);
ASSERT(parentNode->hasTagName(olTag) || parentNode->hasTagName(ulTag));
#endif

[timvolodine, 2013/06/21 15:22:32]

Done.

+    const float adjustedMargin = (multiplier - 1) * getWidestListItemMarker(renderer);
+    RefPtr<RenderStyle> newListStyle = RenderStyle::clone(renderer->style());
+    newListStyle->setMarginStart(Length(newListStyle->marginStart().value() + adjustedMargin, Fixed));

[Julien - ping for review, 2013/06/20 16:11:51]

Touching the style is not a good idea as it is visible to JS. Why can't you just
modify the computed margin instead?

Also this is completely wrong: if marginStart() is not an absolute value (px,
cm, ...), you are doing arithmetic between different type. An example is that
margin-top: 50% gets converted to 50px (marginTop().value() is 50) in your
computation.

[timvolodine, 2013/06/21 15:22:32]

ok, I've removed this for now. To be addressed in a separate patch at the
render-block level as discussed in chat yesterday.

+    newListStyle->setTextAutosizingMultiplier(multiplier);
+    renderer->setStyle(newListStyle.release());
+
+    // Make sure all list items are autosized consistently.
+    for (RenderObject* child = renderer->firstChild(); child; child = child->nextSibling()) {
+        if (child->isListItem() && child->style()->textAutosizingMultiplier() == 1)
+            setMultiplier(child, multiplier);
+    }
+}
+
 float TextAutosizer::computeAutosizedFontSize(float specifiedSize, float multiplier)
 {
     // Somewhat arbitrary "pleasant" font size.