Fix treatment of non-autosizing containers.

This creates a multiplier-inheriting cluster at the boundary where TextAutosizer::containerShouldBeAutosized suppresses or re-enables autosizing.

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

[skobes, 2014-01-13 19:58:32]

Would this do what we need regarding http://crrev.com/135803002?

[pdr., 2014-01-13 20:02:25]

On 2014/01/13 19:58:32, skobes wrote:
> Would this do what we need regarding http://crrev.com/135803002?

Ohh, I like this. Question: should we be using clusters to do this work? If a
cluster also disabled autosizing, we could just push this non-autosized cluster
onto the stack, for example.

[skobes, 2014-01-14 00:17:52]

On 2014/01/13 20:02:25, pdr wrote:
> On 2014/01/13 19:58:32, skobes wrote:
> > Would this do what we need regarding http://crrev.com/135803002?
> 
> Ohh, I like this. Question: should we be using clusters to do this work? If a
> cluster also disabled autosizing, we could just push this non-autosized
cluster
> onto the stack, for example.

Changed to create a cluster in the case where containerShouldBeAutosized returns
false; PTAL.

One difference from the old autosizer is that a container that suppresses
autosizing cannot have a descendent container that is autosized, within the same
cluster.  But that might be ok.  (If we needed to support this, we would need a
separate bool m_shouldAutosize on the Cluster, so that the correct multiplier
can be applied to the descendent container.)

[pdr., 2014-01-14 00:21:07]

On 2014/01/14 00:17:52, skobes wrote:
> On 2014/01/13 20:02:25, pdr wrote:
> > On 2014/01/13 19:58:32, skobes wrote:
> > > Would this do what we need regarding http://crrev.com/135803002?
> > 
> > Ohh, I like this. Question: should we be using clusters to do this work? If
a
> > cluster also disabled autosizing, we could just push this non-autosized
> cluster
> > onto the stack, for example.
> 
> Changed to create a cluster in the case where containerShouldBeAutosized
returns
> false; PTAL.
> 
> One difference from the old autosizer is that a container that suppresses
> autosizing cannot have a descendent container that is autosized, within the
same
> cluster.  But that might be ok.  (If we needed to support this, we would need
a
> separate bool m_shouldAutosize on the Cluster, so that the correct multiplier
> can be applied to the descendent container.)

I think we will need to support this. For example:
<some element that prevents autosizing>
  <div style="position: absolute; width: 500px; height: 500px;">
    this text should be autosized
  </div>
</some element that prevents autosizing>

[skobes (do not use), 2014-01-14 00:27:07]

Note that containerShouldBeAutosized only returns false for form input
tags, rows of links, blocks that don't wrap, and blocks with constrained
height.

Is it common for autosized text to appear within such containers?


On Mon, Jan 13, 2014 at 4:21 PM, <pdr@chromium.org> wrote:

> On 2014/01/14 00:17:52, skobes wrote:
>
>> On 2014/01/13 20:02:25, pdr wrote:
>> > On 2014/01/13 19:58:32, skobes wrote:
>> > > Would this do what we need regarding http://crrev.com/135803002?
>> >
>> > Ohh, I like this. Question: should we be using clusters to do this
>> work? If
>>
> a
>
>> > cluster also disabled autosizing, we could just push this non-autosized
>> cluster
>> > onto the stack, for example.
>>
>
>  Changed to create a cluster in the case where containerShouldBeAutosized
>>
> returns
>
>> false; PTAL.
>>
>
>  One difference from the old autosizer is that a container that suppresses
>> autosizing cannot have a descendent container that is autosized, within
>> the
>>
> same
>
>> cluster.  But that might be ok.  (If we needed to support this, we would
>> need
>>
> a
>
>> separate bool m_shouldAutosize on the Cluster, so that the correct
>> multiplier
>> can be applied to the descendent container.)
>>
>
> I think we will need to support this. For example:
> <some element that prevents autosizing>
>   <div style="position: absolute; width: 500px; height: 500px;">
>     this text should be autosized
>   </div>
> </some element that prevents autosizing>
>
>
> https://codereview.chromium.org/137123004/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[pdr., 2014-01-14 00:38:12]

On 2014/01/14 00:27:07, skobes1 wrote:
> Note that containerShouldBeAutosized only returns false for form input
> tags, rows of links, blocks that don't wrap, and blocks with constrained
> height.
> 
> Is it common for autosized text to appear within such containers?

I don't know for sure, I can guess that it's not common, but my worry is we will
eventually have to support this anyway. WDYT?

Your patch looks good in general btw

[skobes, 2014-01-14 00:55:38]

On 2014/01/14 00:38:12, pdr wrote:
> On 2014/01/14 00:27:07, skobes1 wrote:
> > Note that containerShouldBeAutosized only returns false for form input
> > tags, rows of links, blocks that don't wrap, and blocks with constrained
> > height.
> > 
> > Is it common for autosized text to appear within such containers?
> 
> I don't know for sure, I can guess that it's not common, but my worry is we
will
> eventually have to support this anyway. WDYT?
> 
> Your patch looks good in general btw

Ideally I would like to eliminate the concept of a "container" and do everything
in terms of clusters.  For now we are reusing some static methods on the old
autosizer that have "container" in their names but those can easily just be part
of the logic for whether a block should be a cluster root.

If necessary we can make a cluster that "inherits" its multiplier from the
multiplier that its parent would have had if it hadn't suppressed autosizing,
but I would like to prove that this is necessary before we take on that
complexity.  My theory is that 99% of the time, if an autosized container is
inside a non-autosized container, the autosized container is an independent
descendent, i.e. it would be a separate cluster anyway.

[pdr., 2014-01-14 01:07:46]

On 2014/01/14 00:55:38, skobes wrote:
> On 2014/01/14 00:38:12, pdr wrote:
> > On 2014/01/14 00:27:07, skobes1 wrote:
> > > Note that containerShouldBeAutosized only returns false for form input
> > > tags, rows of links, blocks that don't wrap, and blocks with constrained
> > > height.
> > > 
> > > Is it common for autosized text to appear within such containers?
> > 
> > I don't know for sure, I can guess that it's not common, but my worry is we
> will
> > eventually have to support this anyway. WDYT?
> > 
> > Your patch looks good in general btw
> 
> Ideally I would like to eliminate the concept of a "container" and do
everything
> in terms of clusters.  For now we are reusing some static methods on the old
> autosizer that have "container" in their names but those can easily just be
part
> of the logic for whether a block should be a cluster root.

+1
 
> If necessary we can make a cluster that "inherits" its multiplier from the
> multiplier that its parent would have had if it hadn't suppressed autosizing,
> but I would like to prove that this is necessary before we take on that
> complexity.  My theory is that 99% of the time, if an autosized container is
> inside a non-autosized container, the autosized container is an independent
> descendent, i.e. it would be a separate cluster anyway.

Here's a concrete example of what I think would be a counterexample:
<!DOCTYPE HTML>
<html>
<head>
<style>
  body {
    margin: 50px;
  }
  span {
    position: relative;
  }
  span > .dropdown {
    position: absolute;
    top: 1em;
    left: 0;
    width: 300px;
    border: 1px solid black;
  }
</style>
</head>
<body>
Header links (these are not autosized):<br/>
<span>HeaderLink1</span><span>HeaderLink2</span><span>HeaderLink3<div
class="dropdown">Lots of text that should be autosized</div></span>
</body>
</html>

[skobes, 2014-01-14 01:55:59]

Ok, I fixed it.  PTAL.

[pdr., 2014-01-14 02:50:02]

Great work, I like this approach!

Could you please update the change description?

https://codereview.chromium.org/137123004/diff/110001/Source/core/rendering/F...
File Source/core/rendering/FastTextAutosizer.cpp (right):

https://codereview.chromium.org/137123004/diff/110001/Source/core/rendering/F...
Source/core/rendering/FastTextAutosizer.cpp:94: float multiplier =
cluster->m_autosize ? cluster->m_multiplier : 1.0f;
Double-checking my own understanding:
We no longer call containerShouldBeAutosized here because non-inflating
containers (such as a row of links) will now be a cluster already, and therefore
we can rely on the m_autosize bool?

https://codereview.chromium.org/137123004/diff/110001/Source/core/rendering/F...
Source/core/rendering/FastTextAutosizer.cpp:96: // FIXME: Add an optimization to
not do this walk if it's not needed.
(not for this patch)
I originally added this FIXME for something to do with window sizes (back when
we were stateful), but I've kept it around because it may be the case that we
can skip this if the multiplier is 1 or 0. Is that actually true, or do we need
to "reset" the multipliers from previous layouts?

https://codereview.chromium.org/137123004/diff/110001/Source/core/rendering/F...
Source/core/rendering/FastTextAutosizer.cpp:178: if (!isIndependentDescendant &&
containerCanAutosize == parentClusterCanAutosize)
Double-checking my own understanding:
Is this an optimization for when creating a cluster would have no effect? If so,
could you add that as a comment?

https://codereview.chromium.org/137123004/diff/110001/Source/core/rendering/F...
File Source/core/rendering/FastTextAutosizer.h (right):

https://codereview.chromium.org/137123004/diff/110001/Source/core/rendering/F...
Source/core/rendering/FastTextAutosizer.h:77: bool m_autosize;
Can you add a comment here (or above Cluster) that describes how this works?
Maybe just state that clusters can enable or suppress autosizing for their
non-cluster-subtree children?

[skobes, 2014-01-14 20:46:29]

https://codereview.chromium.org/137123004/diff/110001/Source/core/rendering/F...
File Source/core/rendering/FastTextAutosizer.cpp (right):

https://codereview.chromium.org/137123004/diff/110001/Source/core/rendering/F...
Source/core/rendering/FastTextAutosizer.cpp:94: float multiplier =
cluster->m_autosize ? cluster->m_multiplier : 1.0f;
On 2014/01/14 02:50:03, pdr wrote:
> Double-checking my own understanding:
> We no longer call containerShouldBeAutosized here because non-inflating
> containers (such as a row of links) will now be a cluster already, and
therefore
> we can rely on the m_autosize bool?

Correct.  It was always broken to call containerShouldBeAutosized here, since
the block passed to inflate may not be a container.

https://codereview.chromium.org/137123004/diff/110001/Source/core/rendering/F...
Source/core/rendering/FastTextAutosizer.cpp:96: // FIXME: Add an optimization to
not do this walk if it's not needed.
On 2014/01/14 02:50:03, pdr wrote:
> (not for this patch)
> I originally added this FIXME for something to do with window sizes (back when
> we were stateful), but I've kept it around because it may be the case that we
> can skip this if the multiplier is 1 or 0. Is that actually true, or do we
need
> to "reset" the multipliers from previous layouts?

I think we have to reset them.  It is still kind of stateful since the
RenderStyle holds the multiplier.

Should I remove the FIXME?

https://codereview.chromium.org/137123004/diff/110001/Source/core/rendering/F...
Source/core/rendering/FastTextAutosizer.cpp:178: if (!isIndependentDescendant &&
containerCanAutosize == parentClusterCanAutosize)
On 2014/01/14 02:50:03, pdr wrote:
> Double-checking my own understanding:
> Is this an optimization for when creating a cluster would have no effect? If
so,
> could you add that as a comment?

Correct.  We don't need a cluster for EVERY container, only containers that flip
the autosizing status.

[pdr., 2014-01-14 23:48:07]

On 2014/01/14 20:46:29, skobes wrote:
>
https://codereview.chromium.org/137123004/diff/110001/Source/core/rendering/F...
> File Source/core/rendering/FastTextAutosizer.cpp (right):
> 
>
https://codereview.chromium.org/137123004/diff/110001/Source/core/rendering/F...
> Source/core/rendering/FastTextAutosizer.cpp:94: float multiplier =
> cluster->m_autosize ? cluster->m_multiplier : 1.0f;
> On 2014/01/14 02:50:03, pdr wrote:
> > Double-checking my own understanding:
> > We no longer call containerShouldBeAutosized here because non-inflating
> > containers (such as a row of links) will now be a cluster already, and
> therefore
> > we can rely on the m_autosize bool?
> 
> Correct.  It was always broken to call containerShouldBeAutosized here, since
> the block passed to inflate may not be a container.
> 
>
https://codereview.chromium.org/137123004/diff/110001/Source/core/rendering/F...
> Source/core/rendering/FastTextAutosizer.cpp:96: // FIXME: Add an optimization
to
> not do this walk if it's not needed.
> On 2014/01/14 02:50:03, pdr wrote:
> > (not for this patch)
> > I originally added this FIXME for something to do with window sizes (back
when
> > we were stateful), but I've kept it around because it may be the case that
we
> > can skip this if the multiplier is 1 or 0. Is that actually true, or do we
> need
> > to "reset" the multipliers from previous layouts?
> 
> I think we have to reset them.  It is still kind of stateful since the
> RenderStyle holds the multiplier.
> 
> Should I remove the FIXME?
> 
>
https://codereview.chromium.org/137123004/diff/110001/Source/core/rendering/F...
> Source/core/rendering/FastTextAutosizer.cpp:178: if (!isIndependentDescendant
&&
> containerCanAutosize == parentClusterCanAutosize)
> On 2014/01/14 02:50:03, pdr wrote:
> > Double-checking my own understanding:
> > Is this an optimization for when creating a cluster would have no effect? If
> so,
> > could you add that as a comment?
> 
> Correct.  We don't need a cluster for EVERY container, only containers that
flip
> the autosizing status.

LGTM

[commit-bot: I haz the power, 2014-01-14 23:48:50]

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

[commit-bot: I haz the power, 2014-01-15 15:27:48]

Retried try job too often on mac_blink_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_blink_...

[commit-bot: I haz the power, 2014-01-15 16:40:20]

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

[commit-bot: I haz the power, 2014-01-15 16:40:23]

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 #2 FAILED at 81.
  Hunk #3 FAILED at 91.
  Hunk #4 succeeded at 147 (offset 2 lines).
  Hunk #5 succeeded at 157 (offset 2 lines).
  Hunk #6 succeeded at 167 (offset 2 lines).
  Hunk #7 succeeded at 212 (offset 2 lines).
  2 out of 7 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
94afe2061edc83d08c33ac53df3c42fda5761c6a..e17c8f27a381d9528cc972e1342a5f02ab097cf7
100644
--- a/Source/core/rendering/FastTextAutosizer.cpp
+++ b/Source/core/rendering/FastTextAutosizer.cpp
@@ -58,7 +58,7 @@ void FastTextAutosizer::record(RenderBlock* block)
     if (!enabled())
         return;
 
-    if (!shouldBeClusterRoot(block))
+    if (!isFingerprintingCandidate(block))
         return;
 
     AtomicString fingerprint = computeFingerprint(block);
@@ -81,8 +81,8 @@ void FastTextAutosizer::beginLayout(RenderBlock* block)
     if (block->isRenderView())
         prepareRenderViewInfo(toRenderView(block));
 
-    if (shouldBeClusterRoot(block))
-        m_clusterStack.append(getOrCreateCluster(block));
+    if (Cluster* cluster = maybeGetOrCreateCluster(block))
+        m_clusterStack.append(cluster);
 }
 
 void FastTextAutosizer::inflate(RenderBlock* block)
@@ -91,15 +91,14 @@ void FastTextAutosizer::inflate(RenderBlock* block)
         return;
     Cluster* cluster = m_clusterStack.last();
 
-    // localMultiplier is used to prevent inflation of some containers such as
a row of links.
-    float localMultiplier = TextAutosizer::containerShouldBeAutosized(block) ?
cluster->m_multiplier : 1;
+    float multiplier = cluster->m_autosize ? cluster->m_multiplier : 1.0f;
 
     // FIXME: Add an optimization to not do this walk if it's not needed.
     for (InlineWalker walker(block); !walker.atEnd(); walker.advance()) {
         RenderObject* inlineObj = walker.current();
         if (inlineObj->isText()) {
-            applyMultiplier(inlineObj, localMultiplier);
-            applyMultiplier(inlineObj->parent(), localMultiplier); // Parent
handles line spacing.
+            applyMultiplier(inlineObj, multiplier);
+            applyMultiplier(inlineObj->parent(), multiplier); // Parent handles
line spacing.
         }
     }
 }
@@ -145,7 +144,7 @@ void FastTextAutosizer::prepareRenderViewInfo(RenderView*
renderView)
 #endif
 }
 
-bool FastTextAutosizer::shouldBeClusterRoot(RenderBlock* block)
+bool FastTextAutosizer::isFingerprintingCandidate(RenderBlock* block)
 {
     // FIXME: move the logic out of TextAutosizer.cpp into this class.
     return block->isRenderView()
@@ -155,8 +154,8 @@ bool FastTextAutosizer::shouldBeClusterRoot(RenderBlock*
block)
 
 bool FastTextAutosizer::clusterWantsAutosizing(RenderBlock* root)
 {
-    // FIXME: this should be slightly different.
-    return TextAutosizer::containerShouldBeAutosized(root);
+    // FIXME: this should call (or re-implement)
TextAutosizer::clusterShouldBeAutosized.
+    return true;
 }
 
 AtomicString FastTextAutosizer::computeFingerprint(RenderBlock* block)
@@ -165,26 +164,38 @@ AtomicString
FastTextAutosizer::computeFingerprint(RenderBlock* block)
     return nullAtom;
 }
 
-FastTextAutosizer::Cluster* FastTextAutosizer::getOrCreateCluster(RenderBlock*
block)
+FastTextAutosizer::Cluster*
FastTextAutosizer::maybeGetOrCreateCluster(RenderBlock* block)
 {
+    if (!TextAutosizer::isAutosizingContainer(block))
+        return 0;
+
+    bool isIndependentDescendant =
TextAutosizer::isIndependentDescendant(block);
+
+    // Create clusters to suppress / unsuppress autosizing based on
containerShouldBeAutosized.
+    bool containerCanAutosize =
TextAutosizer::containerShouldBeAutosized(block);
+    bool parentClusterCanAutosize = !m_clusterStack.isEmpty() &&
m_clusterStack.last()->m_autosize;
+
+    // If the container would not alter the m_autosize bit, it doesn't need to
be a cluster.
+    if (!isIndependentDescendant && containerCanAutosize ==
parentClusterCanAutosize)
+        return 0;
+
     ClusterMap::AddResult addResult = m_clusters.add(block,
PassOwnPtr<Cluster>());
     if (!addResult.isNewEntry)
         return addResult.iterator->value.get();
 
     AtomicString fingerprint = m_fingerprintMapper.get(block);
     if (fingerprint.isNull()) {
-        addResult.iterator->value = adoptPtr(createCluster(block));
+        float multiplier;
+        if (isIndependentDescendant)
+            multiplier = clusterWantsAutosizing(block) ?
computeMultiplier(block) : 1.0f;
+        else
+            multiplier = m_clusterStack.last()->m_multiplier;
+        addResult.iterator->value = adoptPtr(new Cluster(block,
containerCanAutosize, multiplier));
         return addResult.iterator->value.get();
     }
     return addSupercluster(fingerprint, block);
 }
 
-FastTextAutosizer::Cluster* FastTextAutosizer::createCluster(RenderBlock*
block)
-{
-    float multiplier = clusterWantsAutosizing(block) ? computeMultiplier(block)
: 1.0f;
-    return new Cluster(block, multiplier);
-}
-
 FastTextAutosizer::Cluster* FastTextAutosizer::addSupercluster(AtomicString
fingerprint, RenderBlock* returnFor)
 {
     BlockSet& roots = m_fingerprintMapper.getBlocks(fingerprint);
@@ -198,7 +209,7 @@ FastTextAutosizer::Cluster*
FastTextAutosizer::addSupercluster(AtomicString fing
 
     Cluster* result = 0;
     for (BlockSet::iterator it = roots.begin(); it != roots.end(); ++it) {
-        Cluster* cluster = new Cluster(*it, multiplier);
+        Cluster* cluster = new Cluster(*it,
TextAutosizer::containerShouldBeAutosized(*it), multiplier);
         m_clusters.set(*it, adoptPtr(cluster));
 
         if (*it == returnFor)

[commit-bot: I haz the power, 2014-01-15 19:23:18]

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

[commit-bot: I haz the power, 2014-01-15 23:44:11]

Retried try job too often on mac_blink_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_blink_...

[pdr., 2014-01-15 23:48:45]

On 2014/01/15 23:44:11, I haz the power (commit-bot) wrote:
> Retried try job too often on mac_blink_rel for step(s) webkit_tests
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_blink_...

I don't understand why this is failing. I'm going to apply this locally and
investigate.

[commit-bot: I haz the power, 2014-01-16 00:07:44]

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

[skobes, 2014-01-16 00:08:51]

On 2014/01/15 23:48:45, pdr wrote:
> On 2014/01/15 23:44:11, I haz the power (commit-bot) wrote:
> > Retried try job too often on mac_blink_rel for step(s) webkit_tests
> >
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_blink_...
> 
> I don't understand why this is failing. I'm going to apply this locally and
> investigate.

For some reason
virtual/fasttextautosizing/fast/text-autosizing/forum-comments-autosizing.html
passes on Linux but fails on Mac.  Not related to this change though.

[commit-bot: I haz the power, 2014-01-16 02:23:55]

Change committed as 165181