Disable FTA when max multiplier is 1.

Disable FTA when the maximum multiplier is 1 (e.g., pages with meta viewport width=device-width).

Also handle zero frame width correctly (happens occasionally per crbug.com/340987), and simplify the LayoutScope interface.

BUG=347384
NOTRY=true

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=168397

[pdr., 2014-02-28 22:13:32]

I think the idea here is good but I have some nits about the implementation.

https://codereview.chromium.org/180743004/diff/60001/Source/core/rendering/Fa...
File Source/core/rendering/FastTextAutosizer.cpp (right):

https://codereview.chromium.org/180743004/diff/60001/Source/core/rendering/Fa...
Source/core/rendering/FastTextAutosizer.cpp:80: if (!enabled())
enabled() can end up depending on layout (for the render view info). I don't see
how this call (before layout) can be right. It seems that we will unnecessarily
call updateRenderViewInfo many times.

https://codereview.chromium.org/180743004/diff/60001/Source/core/rendering/Fa...
Source/core/rendering/FastTextAutosizer.cpp:126: m_firstBlock = block;
Can we move m_firstBlock = block above prepareClusterStack? I think this will
prevent us from accidentally depending on a wrong inLayout() in
prepareClusterStack.

https://codereview.chromium.org/180743004/diff/60001/Source/core/rendering/Fa...
Source/core/rendering/FastTextAutosizer.cpp:280: m_maxMultiplier = m_frameWidth
? max(m_baseMultiplier *
m_maxMultiplier is never actually used as a float. Can we replace it with a bool
with a more descriptive name? m_autosizingEnabled?

[skobes, 2014-03-01 01:45:38]

https://codereview.chromium.org/180743004/diff/60001/Source/core/rendering/Fa...
File Source/core/rendering/FastTextAutosizer.cpp (right):

https://codereview.chromium.org/180743004/diff/60001/Source/core/rendering/Fa...
Source/core/rendering/FastTextAutosizer.cpp:80: if (!enabled())
On 2014/02/28 22:13:33, pdr wrote:
> enabled() can end up depending on layout (for the render view info). I don't
see
> how this call (before layout) can be right. It seems that we will
unnecessarily
> call updateRenderViewInfo many times.

I think updateRenderViewInfo doesn't depend on layout.

https://codereview.chromium.org/180743004/diff/60001/Source/core/rendering/Fa...
Source/core/rendering/FastTextAutosizer.cpp:126: m_firstBlock = block;
On 2014/02/28 22:13:33, pdr wrote:
> Can we move m_firstBlock = block above prepareClusterStack? I think this will
> prevent us from accidentally depending on a wrong inLayout() in
> prepareClusterStack.

Done, but prepareClusterStack doesn't call isInLayout.

https://codereview.chromium.org/180743004/diff/60001/Source/core/rendering/Fa...
Source/core/rendering/FastTextAutosizer.cpp:280: m_maxMultiplier = m_frameWidth
? max(m_baseMultiplier *
On 2014/02/28 22:13:33, pdr wrote:
> m_maxMultiplier is never actually used as a float. Can we replace it with a
bool
> with a more descriptive name? m_autosizingEnabled?

Replaced with an enum.

[pdr., 2014-03-01 05:16:43]

https://codereview.chromium.org/180743004/diff/80001/Source/core/rendering/Fa...
File Source/core/rendering/FastTextAutosizer.cpp (right):

https://codereview.chromium.org/180743004/diff/80001/Source/core/rendering/Fa...
Source/core/rendering/FastTextAutosizer.cpp:210: m_firstBlock = 0;
Should we reset m_pageNeedsAutosizing here?

https://codereview.chromium.org/180743004/diff/80001/Source/core/rendering/Fa...
Source/core/rendering/FastTextAutosizer.cpp:254: if (!m_document->settings()
Having these checks in two places doesn't seem right to me.

I think we will always get a fresh style recalc if any of these values change.
Can we just check these once on the first record() and cache them as m_enabled?

https://codereview.chromium.org/180743004/diff/80001/Source/core/rendering/Fa...
Source/core/rendering/FastTextAutosizer.cpp:740:
m_textAutosizer->updateRenderViewInfo();
Won't the enabled check below handle calling updateRenderViewInfo?

https://codereview.chromium.org/180743004/diff/80001/Source/core/rendering/Fa...
File Source/core/rendering/FastTextAutosizer.h (right):

https://codereview.chromium.org/180743004/diff/80001/Source/core/rendering/Fa...
Source/core/rendering/FastTextAutosizer.h:80: enum HasEnoughTextToAutosize {
This can end up being a bit verbose. Typically I see these named something like:
UnknownAmountOfText
HasEnoughText
NotEnoughText

https://codereview.chromium.org/180743004/diff/80001/Source/core/rendering/Fa...
Source/core/rendering/FastTextAutosizer.h:87: PageNeedsAutosizing_Unknown,
Ditto here

[skobes, 2014-03-04 00:00:24]

PTAL.  Made PageAutosizingStatus orthogonal to enabled().

As discussed on chat, we record() even if the page doesn't autosize so that we
don't need to worry about capturing fingerprints on resize.

https://codereview.chromium.org/180743004/diff/80001/Source/core/rendering/Fa...
File Source/core/rendering/FastTextAutosizer.h (right):

https://codereview.chromium.org/180743004/diff/80001/Source/core/rendering/Fa...
Source/core/rendering/FastTextAutosizer.h:80: enum HasEnoughTextToAutosize {
On 2014/03/01 05:16:43, pdr wrote:
> This can end up being a bit verbose. Typically I see these named something
like:
> UnknownAmountOfText
> HasEnoughText
> NotEnoughText

Done.

https://codereview.chromium.org/180743004/diff/80001/Source/core/rendering/Fa...
Source/core/rendering/FastTextAutosizer.h:87: PageNeedsAutosizing_Unknown,
On 2014/03/01 05:16:43, pdr wrote:
> Ditto here

Done.

[pdr., 2014-03-04 00:13:14]

On 2014/03/04 00:00:24, skobes wrote:
> PTAL.  Made PageAutosizingStatus orthogonal to enabled().
> 
> As discussed on chat, we record() even if the page doesn't autosize so that we
> don't need to worry about capturing fingerprints on resize.
> 
>
https://codereview.chromium.org/180743004/diff/80001/Source/core/rendering/Fa...
> File Source/core/rendering/FastTextAutosizer.h (right):
> 
>
https://codereview.chromium.org/180743004/diff/80001/Source/core/rendering/Fa...
> Source/core/rendering/FastTextAutosizer.h:80: enum HasEnoughTextToAutosize {
> On 2014/03/01 05:16:43, pdr wrote:
> > This can end up being a bit verbose. Typically I see these named something
> like:
> > UnknownAmountOfText
> > HasEnoughText
> > NotEnoughText
> 
> Done.
> 
>
https://codereview.chromium.org/180743004/diff/80001/Source/core/rendering/Fa...
> Source/core/rendering/FastTextAutosizer.h:87: PageNeedsAutosizing_Unknown,
> On 2014/03/01 05:16:43, pdr wrote:
> > Ditto here
> 
> Done.

I think this is the right approach. LGTM

[pdr., 2014-03-04 00:13:26]

The CQ bit was checked by pdr@chromium.org

[commit-bot: I haz the power, 2014-03-04 00:13:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skobes@chromium.org/180743004/160001

[pdr., 2014-03-04 03:06:23]

The CQ bit was unchecked by pdr@chromium.org

[pdr., 2014-03-04 03:07:50]

The win bot is... not winning. Adding notry=true and putting this guy back in
the queue (after https://codereview.chromium.org/182443004)

[skobes, 2014-03-04 06:58:12]

The CQ bit was checked by skobes@chromium.org

[commit-bot: I haz the power, 2014-03-04 06:58:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skobes@chromium.org/180743004/160001

[commit-bot: I haz the power, 2014-03-04 06:58:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-04 06:58:33]

Failed to apply patch for Source/core/rendering/FastTextAutosizer.cpp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/core/rendering/FastTextAutosizer.cpp
  Hunk #3 FAILED at 136.
  Hunk #4 FAILED at 196.
  Hunk #5 succeeded at 234 (offset -1 lines).
  Hunk #6 succeeded at 256 (offset -1 lines).
  Hunk #7 succeeded at 282 (offset -1 lines).
  Hunk #8 succeeded at 291 (offset -1 lines).
  Hunk #9 succeeded at 324 (offset -1 lines).
  Hunk #10 succeeded at 521 (offset -1 lines).
  Hunk #11 succeeded at 706 (offset -1 lines).
  2 out of 11 hunks FAILED -- saving rejects to file
Source/core/rendering/FastTextAutosizer.cpp.rej

Patch:       Source/core/rendering/FastTextAutosizer.cpp
Index: Source/core/rendering/FastTextAutosizer.cpp
diff --git a/Source/core/rendering/FastTextAutosizer.cpp
b/Source/core/rendering/FastTextAutosizer.cpp
index
7d9c5172dc9c58b62cd208684b087fa016f9e9ae..bd0810acda0e91a23b4cad15404c8a0c8b1f1cbf
100644
--- a/Source/core/rendering/FastTextAutosizer.cpp
+++ b/Source/core/rendering/FastTextAutosizer.cpp
@@ -63,6 +63,7 @@ FastTextAutosizer::FastTextAutosizer(const Document* document)
     , m_frameWidth(0)
     , m_layoutWidth(0)
     , m_baseMultiplier(0)
+    , m_pageAutosizingStatus(PageAutosizingStatusUnknown)
     , m_firstBlock(0)
 #ifndef NDEBUG
     , m_renderViewInfoPrepared(false)
@@ -115,15 +116,14 @@ void FastTextAutosizer::prepareClusterStack(const
RenderObject* renderer)
 
 void FastTextAutosizer::beginLayout(RenderBlock* block)
 {
-    ASSERT(enabled());
+    ASSERT(enabled() && m_pageAutosizingStatus == PageNeedsAutosizing);
 #ifndef NDEBUG
     m_blocksThatHaveBegunLayout.add(block);
 #endif
 
     if (!m_firstBlock)  {
-        prepareRenderViewInfo();
-        prepareClusterStack(block->parent());
         m_firstBlock = block;
+        prepareClusterStack(block->parent());
     } else if (block == currentCluster()->m_root) {
         // Ignore beginLayout on the same block twice.
         // This can happen with paginated overflow.
@@ -136,13 +136,13 @@ void FastTextAutosizer::beginLayout(RenderBlock* block)
             inflateTable(toRenderTable(block));
     }
 
-    if (block->childrenInline())
+    if (block->childrenInline() && block->firstChild())
         inflate(block);
 }
 
 void FastTextAutosizer::inflateListItem(RenderListItem* listItem,
RenderListMarker* listItemMarker)
 {
-    if (!enabled())
+    if (!enabled() || m_pageAutosizingStatus != PageNeedsAutosizing)
         return;
     ASSERT(listItem && listItemMarker);
 #ifndef NDEBUG
@@ -196,18 +196,18 @@ void FastTextAutosizer::inflateTable(RenderTable* table)
 
 void FastTextAutosizer::endLayout(RenderBlock* block)
 {
-    ASSERT(enabled());
-
-    if (currentCluster()->m_root == block)
-        m_clusterStack.removeLast();
+    ASSERT(enabled() && m_pageAutosizingStatus == PageNeedsAutosizing);
 
     if (block == m_firstBlock) {
         m_firstBlock = 0;
+        m_pageAutosizingStatus = PageAutosizingStatusUnknown;
         m_clusterStack.clear();
         m_superclusters.clear();
 #ifndef NDEBUG
         m_blocksThatHaveBegunLayout.clear();
 #endif
+    } else if (currentCluster()->m_root == block) {
+        m_clusterStack.removeLast();
     }
 }
 
@@ -235,7 +235,7 @@ bool FastTextAutosizer::enabled()
     return m_document->settings()->textAutosizingEnabled();
 }
 
-void FastTextAutosizer::prepareRenderViewInfo()
+void FastTextAutosizer::updateRenderViewInfo()
 {
     RenderView* renderView = toRenderView(m_document->renderer());
     bool horizontalWritingMode =
isHorizontalWritingMode(renderView->style()->writingMode());
@@ -257,6 +257,10 @@ void FastTextAutosizer::prepareRenderViewInfo()
         float deviceScaleAdjustment =
m_document->settings()->deviceScaleAdjustment();
         m_baseMultiplier *= deviceScaleAdjustment;
     }
+
+    m_pageAutosizingStatus = m_frameWidth && (m_baseMultiplier *
(static_cast<float>(m_layoutWidth) / m_frameWidth) > 1.0f)
+        ? PageNeedsAutosizing : PageDoesNotNeedAutosizing;
+
 #ifndef NDEBUG
     m_renderViewInfoPrepared = true;
 #endif
@@ -279,8 +283,8 @@ bool
FastTextAutosizer::clusterWouldHaveEnoughTextToAutosize(const RenderBlock*
 
 bool FastTextAutosizer::clusterHasEnoughTextToAutosize(Cluster* cluster, const
RenderBlock* widthProvider)
 {
-    if (cluster->m_hasEnoughTextToAutosize != Unknown)
-        return cluster->m_hasEnoughTextToAutosize == Yes;
+    if (cluster->m_hasEnoughTextToAutosize != UnknownAmountOfText)
+        return cluster->m_hasEnoughTextToAutosize == HasEnoughText;
 
     const RenderBlock* root = cluster->m_root;
     if (!widthProvider)
@@ -288,12 +292,12 @@ bool
FastTextAutosizer::clusterHasEnoughTextToAutosize(Cluster* cluster, const R
 
     // TextAreas and user-modifiable areas get a free pass to autosize
regardless of text content.
     if (root->isTextArea() || (root->style() && root->style()->userModify() !=
READ_ONLY)) {
-        cluster->m_hasEnoughTextToAutosize = Yes;
+        cluster->m_hasEnoughTextToAutosize = HasEnoughText;
         return true;
     }
 
     if (!TextAutosizer::containerShouldBeAutosized(root)) {
-        cluster->m_hasEnoughTextToAutosize = No;
+        cluster->m_hasEnoughTextToAutosize = NotEnoughText;
         return false;
     }
 
@@ -321,14 +325,14 @@ bool
FastTextAutosizer::clusterHasEnoughTextToAutosize(Cluster* cluster, const R
             length +=
toRenderText(descendant)->text().stripWhiteSpace().length() *
descendant->style()->specifiedFontSize();
 
             if (length >= minimumTextLengthToAutosize) {
-                cluster->m_hasEnoughTextToAutosize = Yes;
+                cluster->m_hasEnoughTextToAutosize = HasEnoughText;
                 return true;
             }
         }
         descendant = descendant->nextInPreOrder(root);
     }
 
-    cluster->m_hasEnoughTextToAutosize = No;
+    cluster->m_hasEnoughTextToAutosize = NotEnoughText;
     return false;
 }
 
@@ -518,7 +522,7 @@ float FastTextAutosizer::multiplierFromBlock(const
RenderBlock* block)
 
     // Block width, in CSS pixels.
     float blockWidth = widthFromBlock(block);
-    float multiplier = min(blockWidth, static_cast<float>(m_layoutWidth)) /
m_frameWidth;
+    float multiplier = m_frameWidth ? min(blockWidth,
static_cast<float>(m_layoutWidth)) / m_frameWidth : 1.0f;
 
     return max(m_baseMultiplier * multiplier, 1.0f);
 }
@@ -703,4 +707,31 @@ RenderObject*
FastTextAutosizer::nextChildSkippingChildrenOfBlocks(const RenderO
     return current->nextInPreOrderAfterChildren(stayWithin);
 }
 
+FastTextAutosizer::LayoutScope::LayoutScope(RenderBlock* block)
+    : m_textAutosizer(block->document().fastTextAutosizer())
+    , m_block(block)
+{
+    if (!m_textAutosizer)
+        return;
+
+    if (!m_textAutosizer->enabled()) {
+        m_textAutosizer = 0;
+        return;
+    }
+
+    if (m_textAutosizer->m_pageAutosizingStatus == PageAutosizingStatusUnknown)
+        m_textAutosizer->updateRenderViewInfo();
+
+    if (m_textAutosizer->m_pageAutosizingStatus == PageNeedsAutosizing)
+        m_textAutosizer->beginLayout(m_block);
+    else
+        m_textAutosizer = 0;
+}
+
+FastTextAutosizer::LayoutScope::~LayoutScope()
+{
+    if (m_textAutosizer)
+        m_textAutosizer->endLayout(m_block);
+}
+
 } // namespace WebCore

[skobes, 2014-03-04 18:11:36]

The CQ bit was checked by skobes@chromium.org

[commit-bot: I haz the power, 2014-03-04 18:11:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skobes@chromium.org/180743004/180001

[commit-bot: I haz the power, 2014-03-04 18:12:04]

Change committed as 168397