Set multiplier to 1 on blocks with no direct text children.

Source/core/rendering/FastTextAutosizer.cpp

 /*
  * Copyright (C) 2013 Google Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are
  * met:
  *
  *     * Redistributions of source code must retain the above copyright
  * notice, this list of conditions and the following disclaimer.
  *     * Redistributions in binary form must reproduce the above
@@ skipping 354 matching lines @@
     ASSERT(shouldHandleLayout());
 
     if (prepareForLayout(block) == StopLayout)
         return;
 
     if (Cluster* cluster = maybeCreateCluster(block))
         m_clusterStack.append(adoptPtr(cluster));
 
     // Cells in auto-layout tables are handled separately by inflateAutoTable.
     bool isAutoTableCell = block->isTableCell() && !toRenderTableCell(block)->table()->style()->isFixedTableLayout();
-    if (block->childrenInline() && block->firstChild() && !isAutoTableCell)
+    if (!isAutoTableCell && !m_clusterStack.isEmpty())
         inflate(block);
 }
 
 void FastTextAutosizer::inflateListItem(RenderListItem* listItem, RenderListMarker* listItemMarker)
 {
     if (!shouldHandleLayout())
         return;
     ASSERT(listItem && listItemMarker);
 
     if (prepareForLayout(listItem) == StopLayout)
@@ skipping 48 matching lines @@
 #ifndef NDEBUG
         m_blocksThatHaveBegunLayout.clear();
 #endif
     // Tables can create two layout scopes for the same block so the isEmpty
     // check below is needed to guard against endLayout being called twice.
     } else if (!m_clusterStack.isEmpty() && currentCluster()->m_root == block) {
         m_clusterStack.removeLast();
     }
 }
 
 void FastTextAutosizer::inflate(RenderBlock* block)

[pdr., 2014/06/05 20:13:49]

I think this would be a little cleaner if this returned the multiplier and took
a float multiplier as an argument that has a default value of 0. This would
remove the need for a second function and the by-ref float.

[skobes, 2014/06/05 21:19:19]

Done.

 {
+    float multiplier = 0;
+    inflateRecursive(block, multiplier);
+}
+
+void FastTextAutosizer::inflateRecursive(RenderObject* parent, float& multiplier)
+{
     Cluster* cluster = currentCluster();
-    float multiplier = 0;
-    RenderObject* descendant = block->firstChild();
-    while (descendant) {
-        // Skip block descendants because they will be inflate()'d on their own.
-        if (descendant->isRenderBlock()) {
-            descendant = descendant->nextInPreOrderAfterChildren(block);
-            continue;
-        }
-        if (descendant->isText()) {
+    bool hasTextChild = false;
+
+    RenderObject* child = 0;
+    if (parent->isRenderBlock() && parent->childrenInline())

[pdr., 2014/06/05 20:13:49]

The fast firstChild checks only remove a single virtual call so these checks
will actually end up being more expensive. I think we can refactor this as:
if (parent->isRenderBlock() && parent->childrenInline() ||
parent->isRenderInline())
    child = parent->firstChild();

[skobes, 2014/06/05 21:19:20]

We can't directly call parent->firstChild() because firstChild does not exist on
RenderObject.  Or do you mean we should call slowFirstChild()?

+        child = toRenderBlock(parent)->firstChild();
+    else if (parent->isRenderInline())
+        child = toRenderInline(parent)->firstChild();
+
+    while (child) {
+        if (child->isText()) {
+            hasTextChild = true;
             // We only calculate this multiplier on-demand to ensure the parent block of this text
             // has entered layout.
             if (!multiplier)
                 multiplier = cluster->m_flags & SUPPRESSING ? 1.0f : clusterMultiplier(cluster);
-            applyMultiplier(descendant, multiplier);
-            applyMultiplier(descendant->parent(), multiplier); // Parent handles line spacing.
+            applyMultiplier(child, multiplier);
             // FIXME: Investigate why MarkOnlyThis is sufficient.
-            if (descendant->parent()->isRenderInline())
-                descendant->setPreferredLogicalWidthsDirty(MarkOnlyThis);
+            if (parent->isRenderInline())
+                child->setPreferredLogicalWidthsDirty(MarkOnlyThis);
+        } else if (child->isRenderInline()) {
+            inflateRecursive(child, multiplier);
         }
-        descendant = descendant->nextInPreOrder(block);
+        child = child->nextSibling();
+    }
+
+    if (hasTextChild) {
+        applyMultiplier(parent, multiplier); // Parent handles line spacing.
+    } else if (!parent->isListItem()) {
+        // For consistency, a block with no immediate text child should always have a
+        // multiplier of 1 (except for list items which are handled in inflateListItem).
+        applyMultiplier(parent, 1);

[pdr., 2014/06/05 20:13:49]

Should we only do this when textAutosizingMultiplier != 1? Otherwise we will
create many more unique styles which may be unnecessary.

[skobes, 2014/06/05 21:19:19]

applyMultiplier does this check.

     }
 }
 
 bool FastTextAutosizer::shouldHandleLayout() const
 {
     return m_pageInfo.m_settingEnabled && m_pageInfo.m_pageNeedsAutosizing && !m_updatePageInfoDeferred;
 }
 
 void FastTextAutosizer::updatePageInfoInAllFrames()
 {
@@ skipping 633 matching lines @@
 FastTextAutosizer::DeferUpdatePageInfo::~DeferUpdatePageInfo()
 {
     if (FastTextAutosizer* textAutosizer = m_mainFrame->document()->fastTextAutosizer()) {
         ASSERT(textAutosizer->m_updatePageInfoDeferred);
         textAutosizer->m_updatePageInfoDeferred = false;
         textAutosizer->updatePageInfoInAllFrames();
     }
 }
 
 } // namespace WebCore