Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
145123010:
Fix supercluster logic. (Closed)
|
Created:
6 years, 10 months ago by skobes Modified:
6 years, 10 months ago Reviewers:
pdr. CC:
blink-reviews, bemjb+rendering_chromium.org, dsinclair, zoltan1, eae+blinkwatch, leviw+renderwatch, jchaffraix+rendering, timvolodine, johnme Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Visibility:
Public. |
DescriptionFix supercluster logic.
The cluster stack now owns the Cluster objects; we no longer "pre-create" a Cluster before its root has entered layout. Instead we connect it to a Supercluster object which holds the set of roots that share its fingerprint, and the common multiplier.
The multiplier for the supercluster is based on the deepest common ancestor of the deepest-block-containing-all-text for all its clusters.
BUG=302005
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=167133
Patch Set 1 : #
Total comments: 6
Patch Set 2 : Address review comments. #Patch Set 3 : Rebase. #Patch Set 4 : Rebaseline. #Patch Set 5 : Fix tests. #Patch Set 6 : Rebaseline. #
Total comments: 2
Patch Set 7 : Address review comment. #
Messages
Total messages: 24 (0 generated)
LGTM with nits https://codereview.chromium.org/145123010/diff/60004/Source/core/rendering/Fa... File Source/core/rendering/FastTextAutosizer.cpp (right): https://codereview.chromium.org/145123010/diff/60004/Source/core/rendering/Fa... Source/core/rendering/FastTextAutosizer.cpp:325: BlockSet dbcats; Please add a comment explaining what a dbcat is, or just use the incredibly long name. https://codereview.chromium.org/145123010/diff/60004/Source/core/rendering/Fa... Source/core/rendering/FastTextAutosizer.cpp:327: // This is a bit of a hack to let us compute dbcat and text length for The old autosizer used this hacky pattern quite a bit and I'm not a fan. Can you extract out the logic from textLength and deepestBlockContainingAllText to call it with just a block? https://codereview.chromium.org/145123010/diff/60004/Source/core/rendering/Fa... File Source/core/rendering/FastTextAutosizer.h (right): https://codereview.chromium.org/145123010/diff/60004/Source/core/rendering/Fa... Source/core/rendering/FastTextAutosizer.h:75: struct Supercluster { Can you add a comment here describing what superclusters are used for?
https://codereview.chromium.org/145123010/diff/60004/Source/core/rendering/Fa... File Source/core/rendering/FastTextAutosizer.cpp (right): https://codereview.chromium.org/145123010/diff/60004/Source/core/rendering/Fa... Source/core/rendering/FastTextAutosizer.cpp:325: BlockSet dbcats; On 2014/02/06 22:03:27, pdr wrote: > Please add a comment explaining what a dbcat is, or just use the incredibly long > name. Done. https://codereview.chromium.org/145123010/diff/60004/Source/core/rendering/Fa... Source/core/rendering/FastTextAutosizer.cpp:327: // This is a bit of a hack to let us compute dbcat and text length for On 2014/02/06 22:03:27, pdr wrote: > The old autosizer used this hacky pattern quite a bit and I'm not a fan. Can you > extract out the logic from textLength and deepestBlockContainingAllText to call > it with just a block? Done. https://codereview.chromium.org/145123010/diff/60004/Source/core/rendering/Fa... File Source/core/rendering/FastTextAutosizer.h (right): https://codereview.chromium.org/145123010/diff/60004/Source/core/rendering/Fa... Source/core/rendering/FastTextAutosizer.h:75: struct Supercluster { On 2014/02/06 22:03:27, pdr wrote: > Can you add a comment here describing what superclusters are used for? Done.
On 2014/02/06 23:55:50, skobes wrote: > https://codereview.chromium.org/145123010/diff/60004/Source/core/rendering/Fa... > File Source/core/rendering/FastTextAutosizer.cpp (right): > > https://codereview.chromium.org/145123010/diff/60004/Source/core/rendering/Fa... > Source/core/rendering/FastTextAutosizer.cpp:325: BlockSet dbcats; > On 2014/02/06 22:03:27, pdr wrote: > > Please add a comment explaining what a dbcat is, or just use the incredibly > long > > name. > > Done. > > https://codereview.chromium.org/145123010/diff/60004/Source/core/rendering/Fa... > Source/core/rendering/FastTextAutosizer.cpp:327: // This is a bit of a hack to > let us compute dbcat and text length for > On 2014/02/06 22:03:27, pdr wrote: > > The old autosizer used this hacky pattern quite a bit and I'm not a fan. Can > you > > extract out the logic from textLength and deepestBlockContainingAllText to > call > > it with just a block? > > Done. > > https://codereview.chromium.org/145123010/diff/60004/Source/core/rendering/Fa... > File Source/core/rendering/FastTextAutosizer.h (right): > > https://codereview.chromium.org/145123010/diff/60004/Source/core/rendering/Fa... > Source/core/rendering/FastTextAutosizer.h:75: struct Supercluster { > On 2014/02/06 22:03:27, pdr wrote: > > Can you add a comment here describing what superclusters are used for? > > Done. LGTM++++
The CQ bit was checked by skobes@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/skobes@chromium.org/145123010/160001
The CQ bit was unchecked by commit-bot@chromium.org
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 #1 succeeded at 95 (offset -2 lines).
Hunk #2 FAILED at 124.
Hunk #3 succeeded at 189 (offset -4 lines).
Hunk #4 succeeded at 224 (offset -4 lines).
Hunk #5 succeeded at 241 (offset -4 lines).
Hunk #6 succeeded at 258 (offset -4 lines).
Hunk #7 succeeded at 302 (offset -4 lines).
Hunk #8 succeeded at 317 (offset -4 lines).
Hunk #9 succeeded at 385 (offset -4 lines).
Hunk #10 succeeded at 483 (offset -4 lines).
1 out of 10 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
37979f9c8caf8e032a321b57c295068e20c88160..d114a1121d779baad1cb486c968034279b82a340
100644
--- a/Source/core/rendering/FastTextAutosizer.cpp
+++ b/Source/core/rendering/FastTextAutosizer.cpp
@@ -97,8 +97,8 @@ void FastTextAutosizer::beginLayout(RenderBlock* block)
return;
}
- if (Cluster* cluster = maybeGetOrCreateCluster(block))
- m_clusterStack.append(cluster);
+ if (Cluster* cluster = maybeCreateCluster(block))
+ m_clusterStack.append(adoptPtr(cluster));
if (block->childrenInline())
inflate(block);
@@ -124,10 +124,13 @@ void FastTextAutosizer::endLayout(RenderBlock* block)
{
if (!enabled())
return;
+
+ if (block->isRenderView()) {
+ m_superclusters.clear();
#ifndef NDEBUG
- if (block->isRenderView())
m_blocksThatHaveBegunLayout.clear();
#endif
+ }
if (currentCluster()->m_root == block)
m_clusterStack.removeLast();
@@ -193,6 +196,12 @@ bool FastTextAutosizer::isFingerprintingCandidate(const
RenderBlock* block)
&& TextAutosizer::isIndependentDescendant(block));
}
+bool FastTextAutosizer::clusterWouldHaveEnoughTextToAutosize(const RenderBlock*
root)
+{
+ Cluster hypotheticalCluster(root, true, 0);
+ return clusterHasEnoughTextToAutosize(&hypotheticalCluster);
+}
+
bool FastTextAutosizer::clusterHasEnoughTextToAutosize(Cluster* cluster)
{
const RenderBlock* root = cluster->m_root;
@@ -222,12 +231,6 @@ float FastTextAutosizer::textLength(Cluster* cluster)
// clusters-sufficient-text-except-in-root.html). This currently
includes text
// from descendant clusters.
- if (descendant->isRenderBlock() &&
m_clusters.contains(toRenderBlock(descendant))) {
- length += textLength(m_clusters.get(toRenderBlock(descendant)));
- descendant = descendant->nextInPreOrderAfterChildren(root);
- continue;
- }
-
if (measureLocalText && descendant->isText()) {
// Note: Using text().stripWhiteSpace().length() instead of
renderedTextLength() because
// the lineboxes will not be built until layout. These values can
be different.
@@ -245,7 +248,7 @@ AtomicString FastTextAutosizer::computeFingerprint(const
RenderBlock* block)
return nullAtom;
}
-FastTextAutosizer::Cluster* FastTextAutosizer::maybeGetOrCreateCluster(const
RenderBlock* block)
+FastTextAutosizer::Cluster* FastTextAutosizer::maybeCreateCluster(const
RenderBlock* block)
{
if (!TextAutosizer::isAutosizingContainer(block))
return 0;
@@ -262,32 +265,26 @@ FastTextAutosizer::Cluster*
FastTextAutosizer::maybeGetOrCreateCluster(const Ren
if (!createClusterThatMightAutosize && 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(new Cluster(block,
containerCanAutosize, parentCluster));
- return addResult.iterator->value.get();
- }
- return addSupercluster(fingerprint, block);
+ return new Cluster(block, containerCanAutosize, parentCluster,
getSupercluster(block));
}
-// FIXME: The supercluster logic does not work yet.
-FastTextAutosizer::Cluster* FastTextAutosizer::addSupercluster(AtomicString
fingerprint, const RenderBlock* returnFor)
+FastTextAutosizer::Supercluster* FastTextAutosizer::getSupercluster(const
RenderBlock* block)
{
- BlockSet& roots = m_fingerprintMapper.getBlocks(fingerprint);
+ AtomicString fingerprint = m_fingerprintMapper.get(block);
+ if (fingerprint.isNull())
+ return 0;
- Cluster* result = 0;
- for (BlockSet::iterator it = roots.begin(); it != roots.end(); ++it) {
- Cluster* cluster = new Cluster(*it,
TextAutosizer::containerShouldBeAutosized(*it), currentCluster());
- m_clusters.set(*it, adoptPtr(cluster));
+ BlockSet* roots = &m_fingerprintMapper.getBlocks(fingerprint);
+ if (roots->size() < 2)
+ return 0;
- if (*it == returnFor)
- result = cluster;
- }
- return result;
+ SuperclusterMap::AddResult addResult = m_superclusters.add(fingerprint,
PassOwnPtr<Supercluster>());
+ if (!addResult.isNewEntry)
+ return addResult.iterator->value.get();
+
+ Supercluster* supercluster = new Supercluster(roots);
+ addResult.iterator->value = adoptPtr(supercluster);
+ return supercluster;
}
const RenderBlock* FastTextAutosizer::deepestCommonAncestor(BlockSet& blocks)
@@ -312,16 +309,10 @@ float FastTextAutosizer::clusterMultiplier(Cluster*
cluster)
ASSERT(m_renderViewInfoPrepared);
if (!cluster->m_multiplier) {
if (TextAutosizer::isIndependentDescendant(cluster->m_root) ||
isWiderDescendant(cluster) || isNarrowerDescendant(cluster)) {
- if (clusterHasEnoughTextToAutosize(cluster)) {
- const RenderBlock* deepestBlockWithAllText =
deepestBlockContainingAllText(cluster);
-
- // Ensure the deepest block containing all text has a valid
contentLogicalWidth.
-
ASSERT(m_blocksThatHaveBegunLayout.contains(deepestBlockWithAllText));
-
- // Block width, in CSS pixels.
- float textBlockWidth =
deepestBlockWithAllText->contentLogicalWidth();
- float multiplier = min(textBlockWidth,
static_cast<float>(m_layoutWidth)) / m_frameWidth;
- cluster->m_multiplier = max(m_baseMultiplier * multiplier,
1.0f);
+ if (cluster->m_supercluster) {
+ cluster->m_multiplier =
superclusterMultiplier(cluster->m_supercluster);
+ } else if (clusterHasEnoughTextToAutosize(cluster)) {
+ cluster->m_multiplier =
multiplierFromBlock(deepestBlockContainingAllText(cluster));
} else {
cluster->m_multiplier = 1.0f;
}
@@ -333,20 +324,53 @@ float FastTextAutosizer::clusterMultiplier(Cluster*
cluster)
return cluster->m_multiplier;
}
-// FIXME: Refactor this to look more like
FastTextAutosizer::deepestCommonAncestor. This is copied
-// from TextAutosizer::findDeepestBlockContainingAllText.
+float FastTextAutosizer::superclusterMultiplier(Supercluster* supercluster)
+{
+ if (!supercluster->m_multiplier) {
+ const BlockSet* roots = supercluster->m_roots;
+ // Set of the deepest block containing all text (DBCAT) of every
cluster.
+ BlockSet dbcats;
+ for (BlockSet::iterator it = roots->begin(); it != roots->end(); ++it)
{
+ dbcats.add(deepestBlockContainingAllText(*it));
+ supercluster->m_anyClusterHasEnoughText |=
clusterWouldHaveEnoughTextToAutosize(*it);
+ }
+ supercluster->m_multiplier = supercluster->m_anyClusterHasEnoughText
+ ? multiplierFromBlock(deepestCommonAncestor(dbcats)) : 1.0f;
+ }
+ ASSERT(supercluster->m_multiplier);
+ return supercluster->m_multiplier;
+}
+
+float FastTextAutosizer::multiplierFromBlock(const RenderBlock* block)
+{
+ ASSERT(m_blocksThatHaveBegunLayout.contains(block));
+
+ // Block width, in CSS pixels.
+ float textBlockWidth = block->contentLogicalWidth();
+ float multiplier = min(textBlockWidth, static_cast<float>(m_layoutWidth)) /
m_frameWidth;
+
+ return max(m_baseMultiplier * multiplier, 1.0f);
+}
+
const RenderBlock* FastTextAutosizer::deepestBlockContainingAllText(Cluster*
cluster)
{
- if (cluster->m_deepestBlockContainingAllText)
- return cluster->m_deepestBlockContainingAllText;
+ if (!cluster->m_deepestBlockContainingAllText)
+ cluster->m_deepestBlockContainingAllText =
deepestBlockContainingAllText(cluster->m_root);
+ return cluster->m_deepestBlockContainingAllText;
+}
+
+// FIXME: Refactor this to look more like
FastTextAutosizer::deepestCommonAncestor. This is copied
+// from TextAutosizer::findDeepestBlockContainingAllText.
+const RenderBlock* FastTextAutosizer::deepestBlockContainingAllText(const
RenderBlock* root)
+{
size_t firstDepth = 0;
- const RenderObject* firstTextLeaf = findTextLeaf(cluster->m_root,
firstDepth, First);
+ const RenderObject* firstTextLeaf = findTextLeaf(root, firstDepth, First);
if (!firstTextLeaf)
- return cluster->m_deepestBlockContainingAllText = cluster->m_root;
+ return root;
size_t lastDepth = 0;
- const RenderObject* lastTextLeaf = findTextLeaf(cluster->m_root, lastDepth,
Last);
+ const RenderObject* lastTextLeaf = findTextLeaf(root, lastDepth, Last);
ASSERT(lastTextLeaf);
// Equalize the depths if necessary. Only one of the while loops below will
get executed.
@@ -368,16 +392,16 @@ const RenderBlock*
FastTextAutosizer::deepestBlockContainingAllText(Cluster* clu
}
if (firstNode->isRenderBlock())
- return cluster->m_deepest…
(message too large)
The CQ bit was checked by skobes@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/skobes@chromium.org/145123010/100002
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/skobes@chromium.org/145123010/100002
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on win_layout for step(s) webkit_lint http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_layout...
The CQ bit was checked by pdr@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/skobes@chromium.org/145123010/100002
The CQ bit was unchecked by commit-bot@chromium.org
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_...
On 2014/02/10 00:04:45, 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_... Looks like virtual/fasttextautosizing/fast/text-autosizing/textarea-fontsize-change.html is failing.
PTAL. The change to make Cluster objects owned by the cluster stack instead of cached in a HashMap had some interesting side effects: (1) textarea-fontsize-change.html regressed because after the textarea is autosized, RenderLayerScrollableArea::updateAfterLayout causes a second layout of the DIV id='inner-editor' inside the textarea, during which the FTA computes a slightly smaller multiplier because the content width has been reduced to accomodate the scrollbar. I updated the test to hide the scrollbars. (2) resize-window.html passes now. It was previously failing because m_clusters retained the original Cluster forever (yay memory leaks), so the multiplier was not recomputed after the resize. (3) cluster-list-item.html crashed for a subtle reason. When FrameView::scrollbarExistenceDidChange causes a second layout, the absolute-positioned LI is laid out by the RenderView's layoutPositionedObjects(), and inflateListItem() tries to compute a multiplier based on the cluster's DBCAT which is the UL. But the UL was not marked as needsLayout(), so it is not in m_blocksThatHaveBegunLayout, and we fire the assertion in multiplierFromBlock(). The core issue is that multiplierFromBlock assumed that the DBCAT would have entered layout. But for a positioned object, the DBCAT may be deeper than the containing block. So the containing block's layoutPositionedObjects() may lay out the positioned object when the DBCAT is not even marked as needing layout. To fix this I have relaxed the assertion in multiplierFromBlock() to permit a DBCAT that hasn't entered layout and doesn't need to. (If it doesn't need layout it should be safe to ask its width.)
LGTM with an optional idea about scrollbar styling. https://codereview.chromium.org/145123010/diff/690001/LayoutTests/fast/text-a... File LayoutTests/fast/text-autosizing/textarea-fontsize-change.html (right): https://codereview.chromium.org/145123010/diff/690001/LayoutTests/fast/text-a... LayoutTests/fast/text-autosizing/textarea-fontsize-change.html:16: overflow-y: hidden; Could we use the following instead? textarea::-webkit-scrollbar { display: none; } I think this will still test the scrollbar codepath.
https://codereview.chromium.org/145123010/diff/690001/LayoutTests/fast/text-a... File LayoutTests/fast/text-autosizing/textarea-fontsize-change.html (right): https://codereview.chromium.org/145123010/diff/690001/LayoutTests/fast/text-a... LayoutTests/fast/text-autosizing/textarea-fontsize-change.html:16: overflow-y: hidden; On 2014/02/13 20:56:37, pdr wrote: > Could we use the following instead? > textarea::-webkit-scrollbar > { > display: none; > } > > I think this will still test the scrollbar codepath. Done.
The CQ bit was checked by skobes@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/skobes@chromium.org/145123010/740001
Message was sent while issue was closed.
Change committed as 167133 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
