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

Source/core/rendering/TextAutosizer.cpp

 /*
  * Copyright (C) 2012 Google Inc. All rights reserved.
  * Copyright (C) 2012 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.
  *
  * This library is distributed in the hope that it will be useful,
@@ skipping 10 matching lines @@
 #include "config.h"
 
 #include "core/rendering/TextAutosizer.h"
 
 #include "core/dom/Document.h"
 #include "core/html/HTMLElement.h"
 #include "core/inspector/InspectorInstrumentation.h"
 #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"
 #include "core/rendering/style/RenderStyle.h"
 #include "core/rendering/style/StyleInheritedData.h"
 
 #include <algorithm>
 #include <wtf/StdLibExtras.h>
 #include <wtf/Vector.h>
 
@@ skipping 42 matching lines @@
 
 static RenderListItem* getAncestorListItem(const RenderObject* renderer)
 {
     RenderObject* ancestor = renderer->parent();
     while (ancestor && (ancestor->isRenderInline() || ancestor->isAnonymousBlock()))
         ancestor = ancestor->parent();
 
     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)
 {
 }
 
 TextAutosizer::~TextAutosizer()
 {
 }
 
 void TextAutosizer::recalculateMultipliers()
@@ skipping 106 matching lines @@
     ASSERT(isAutosizingContainer(container));
 
     float localMultiplier = containerShouldBeAutosized(container) ? multiplier: 1;
 
     RenderObject* descendant = nextInPreOrderSkippingDescendantsOfContainers(subtreeRoot, subtreeRoot);
     while (descendant) {
         if (descendant->isText()) {
             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)) {
             RenderBlock* descendantBlock = toRenderBlock(descendant);
             TextAutosizingClusterInfo descendantClusterInfo(descendantBlock);
             if (isWiderDescendant(descendantBlock, clusterInfo) || isIndependentDescendant(descendantBlock))
                 processCluster(descendantClusterInfo, descendantBlock, descendantBlock, windowInfo);
             else if (isNarrowDescendant(descendantBlock, clusterInfo)) {
                 // Narrow descendants are processed together later to be able to apply the same multiplier
                 // to each of them if necessary.
                 clusterInfo.narrowDescendants.append(descendantClusterInfo);
             } else
                 processContainer(multiplier, descendantBlock, clusterInfo, descendantBlock, windowInfo);
         }
         descendant = nextInPreOrderSkippingDescendantsOfContainers(descendant, subtreeRoot);
     }
 }
 
 void TextAutosizer::setMultiplier(RenderObject* renderer, float multiplier)
 {
     RefPtr<RenderStyle> newStyle = RenderStyle::clone(renderer->style());
     newStyle->setTextAutosizingMultiplier(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.
     const float pleasantSize = 16;
 
     // Multiply fonts that the page author has specified to be larger than
     // pleasantSize by less and less, until huge fonts are not increased at all.
     // For specifiedSize between 0 and pleasantSize we directly apply the
     // multiplier; hence for specifiedSize == pleasantSize, computedSize will be
     // multiplier * pleasantSize. For greater specifiedSizes we want to
@@ skipping 369 matching lines @@
         if (i + 1 < clusterInfos.size()) {
             float currentWidth = clusterInfos[i].root->contentLogicalWidth();
             float nextWidth = clusterInfos[i + 1].root->contentLogicalWidth();
             if (currentWidth - nextWidth > maxWidthDifferenceWithinGroup)
                 groups.grow(groups.size() + 1);
         }
     }
 }
 
 } // namespace WebCore