- Revision
- 143060
- Author
- espr...@chromium.org
- Date
- 2013-02-15 15:11:47 -0800 (Fri, 15 Feb 2013)
Log Message
RenderQuote should not mark renderers as needing layout during layout
https://bugs.webkit.org/show_bug.cgi?id=109876
Reviewed by Ojan Vafai.
Source/WebCore:
Marking RenderQuotes as needing pref width recalcs and layouts during a
layout is dangerous since an ancestor may mark itself as having completed
layout, but then some subtree still thinks it needs layout.
Instead, since the only time we create RenderQuote instances is inside
PseudoElement, we can call attachQuote inside PseudoElement::attach during
the regular tree mutating cycle. We can then use RenderQuote::styleDidChange
to update the kind of quotes on normal style changes.
This makes RenderQuote behave much more similarly to DOM nodes and means
we no longer need to set dirty bits during layout.
Test: fast/css-generated-content/quote-layout-focus-crash.html
* dom/PseudoElement.cpp:
(WebCore::PseudoElement::attach): Now call attachQuote().
* rendering/RenderQuote.cpp:
(WebCore::RenderQuote::~RenderQuote):
(WebCore::RenderQuote::willBeRemovedFromTree):
(WebCore::RenderQuote::styleDidChange):
(WebCore::RenderQuote::updateText):
(WebCore::RenderQuote::attachQuote):
(WebCore::RenderQuote::detachQuote):
(WebCore::RenderQuote::updateDepth):
* rendering/RenderQuote.h:
(RenderQuote):
LayoutTests:
* fast/block/float/float-not-removed-from-pre-block-expected.txt:
* fast/css-generated-content/quote-layout-focus-crash-expected.txt: Added.
* fast/css-generated-content/quote-layout-focus-crash.html: Added.
Modified Paths
Added Paths
Diff
Modified: trunk/LayoutTests/ChangeLog (143059 => 143060)
--- trunk/LayoutTests/ChangeLog 2013-02-15 23:07:44 UTC (rev 143059)
+++ trunk/LayoutTests/ChangeLog 2013-02-15 23:11:47 UTC (rev 143060)
@@ -1,3 +1,14 @@
+2013-02-15 Elliott Sprehn <espr...@chromium.org>
+
+ RenderQuote should not mark renderers as needing layout during layout
+ https://bugs.webkit.org/show_bug.cgi?id=109876
+
+ Reviewed by Ojan Vafai.
+
+ * fast/block/float/float-not-removed-from-pre-block-expected.txt:
+ * fast/css-generated-content/quote-layout-focus-crash-expected.txt: Added.
+ * fast/css-generated-content/quote-layout-focus-crash.html: Added.
+
2013-02-15 Rik Cabanier <caban...@adobe.com>
Add platform support for -webkit-background-blend-mode to CG context
Modified: trunk/LayoutTests/fast/block/float/float-not-removed-from-pre-block-expected.txt (143059 => 143060)
--- trunk/LayoutTests/fast/block/float/float-not-removed-from-pre-block-expected.txt 2013-02-15 23:07:44 UTC (rev 143059)
+++ trunk/LayoutTests/fast/block/float/float-not-removed-from-pre-block-expected.txt 2013-02-15 23:11:47 UTC (rev 143060)
@@ -1,3 +1,3 @@
Bug 101970: Heap-use-after-free in WebCore::RenderLayerModelObject::hasSelfPaintingLayer
Test passes if it does not crash.
-
+
Added: trunk/LayoutTests/fast/css-generated-content/quote-layout-focus-crash-expected.txt (0 => 143060)
--- trunk/LayoutTests/fast/css-generated-content/quote-layout-focus-crash-expected.txt (rev 0)
+++ trunk/LayoutTests/fast/css-generated-content/quote-layout-focus-crash-expected.txt 2013-02-15 23:11:47 UTC (rev 143060)
@@ -0,0 +1,3 @@
+Bug 109616 - ASSERT(!renderer()->needsLayout()) when calling Element::focus() with generated content
+
+
Added: trunk/LayoutTests/fast/css-generated-content/quote-layout-focus-crash.html (0 => 143060)
--- trunk/LayoutTests/fast/css-generated-content/quote-layout-focus-crash.html (rev 0)
+++ trunk/LayoutTests/fast/css-generated-content/quote-layout-focus-crash.html 2013-02-15 23:11:47 UTC (rev 143060)
@@ -0,0 +1,47 @@
+<!DOCTYPE html>
+
+<style>
+ /* Must be positioned absolute or static, must have margins to push it out view. */
+ .positioned {
+ position: absolute;
+ margin-top: 100%;
+ }
+
+ /* Any kind of quote will do, can be either :before or :after */
+ .positioned:before,
+ .focusable:before {
+ content: open-quote;
+ }
+</style>
+
+<p>Bug 109616 - ASSERT(!renderer()->needsLayout()) when calling Element::focus() with generated content</p>
+
+<!--
+ This is testing a case where RenderQuote::updateDepth would mark the RenderQuote
+ and its ancestors as needing layout in the middle of a layout of its ancestor.
+ When its ancestor finishes the layout it will mark itself and the ancestors
+ farther up as no longer needing layout. The end result is some subtree
+ needing layout, but the RenderView and possibly other ancestors of the subtree
+ not needing layout.
+
+ ex.
+
+ RenderView <- !needsLayout
+ \
+ RenderBlock (.focusable) <- needsLayout
+ \
+ RenderBlock (generated content) <- needsLayout
+ \
+ RenderQuote <- needsLayout
+-->
+
+<div class="positioned"></div>
+<div class="focusable" tabindex="1"></div>
+
+<script>
+ if (window.testRunner)
+ testRunner.dumpAsText();
+ // .focusable still needs layout at this point, but RenderView doesn't
+ // think any descendants need layout.
+ document.querySelector('.focusable').focus();
+</script>
Modified: trunk/Source/WebCore/ChangeLog (143059 => 143060)
--- trunk/Source/WebCore/ChangeLog 2013-02-15 23:07:44 UTC (rev 143059)
+++ trunk/Source/WebCore/ChangeLog 2013-02-15 23:11:47 UTC (rev 143060)
@@ -1,3 +1,37 @@
+2013-02-15 Elliott Sprehn <espr...@chromium.org>
+
+ RenderQuote should not mark renderers as needing layout during layout
+ https://bugs.webkit.org/show_bug.cgi?id=109876
+
+ Reviewed by Ojan Vafai.
+
+ Marking RenderQuotes as needing pref width recalcs and layouts during a
+ layout is dangerous since an ancestor may mark itself as having completed
+ layout, but then some subtree still thinks it needs layout.
+
+ Instead, since the only time we create RenderQuote instances is inside
+ PseudoElement, we can call attachQuote inside PseudoElement::attach during
+ the regular tree mutating cycle. We can then use RenderQuote::styleDidChange
+ to update the kind of quotes on normal style changes.
+
+ This makes RenderQuote behave much more similarly to DOM nodes and means
+ we no longer need to set dirty bits during layout.
+
+ Test: fast/css-generated-content/quote-layout-focus-crash.html
+
+ * dom/PseudoElement.cpp:
+ (WebCore::PseudoElement::attach): Now call attachQuote().
+ * rendering/RenderQuote.cpp:
+ (WebCore::RenderQuote::~RenderQuote):
+ (WebCore::RenderQuote::willBeRemovedFromTree):
+ (WebCore::RenderQuote::styleDidChange):
+ (WebCore::RenderQuote::updateText):
+ (WebCore::RenderQuote::attachQuote):
+ (WebCore::RenderQuote::detachQuote):
+ (WebCore::RenderQuote::updateDepth):
+ * rendering/RenderQuote.h:
+ (RenderQuote):
+
2013-02-15 Sheriff Bot <webkit.review....@gmail.com>
Unreviewed, rolling out r143044.
Modified: trunk/Source/WebCore/dom/PseudoElement.cpp (143059 => 143060)
--- trunk/Source/WebCore/dom/PseudoElement.cpp 2013-02-15 23:07:44 UTC (rev 143059)
+++ trunk/Source/WebCore/dom/PseudoElement.cpp 2013-02-15 23:11:47 UTC (rev 143060)
@@ -30,6 +30,7 @@
#include "ContentData.h"
#include "NodeRenderingContext.h"
#include "RenderObject.h"
+#include "RenderQuote.h"
namespace WebCore {
@@ -82,9 +83,11 @@
for (const ContentData* content = style->contentData(); content; content = content->next()) {
RenderObject* child = content->createRenderer(document(), style);
- if (renderer->isChildAllowed(child, style))
+ if (renderer->isChildAllowed(child, style)) {
renderer->addChild(child);
- else
+ if (child->isQuote())
+ toRenderQuote(child)->attachQuote();
+ } else
child->destroy();
}
}
Modified: trunk/Source/WebCore/rendering/RenderQuote.cpp (143059 => 143060)
--- trunk/Source/WebCore/rendering/RenderQuote.cpp 2013-02-15 23:07:44 UTC (rev 143059)
+++ trunk/Source/WebCore/rendering/RenderQuote.cpp 2013-02-15 23:11:47 UTC (rev 143060)
@@ -54,10 +54,15 @@
void RenderQuote::willBeRemovedFromTree()
{
RenderText::willBeRemovedFromTree();
-
detachQuote();
}
+void RenderQuote::styleDidChange(StyleDifference diff, const RenderStyle* oldStyle)
+{
+ RenderText::styleDidChange(diff, oldStyle);
+ updateText();
+}
+
typedef HashMap<AtomicString, const QuotesData*, CaseFoldingHash> QuotesMap;
static const QuotesMap& quotesDataLanguageMap()
@@ -245,27 +250,9 @@
void RenderQuote::updateText()
{
- computePreferredLogicalWidths(0);
+ setText(originalText());
}
-void RenderQuote::computePreferredLogicalWidths(float lead)
-{
-#ifndef NDEBUG
- // FIXME: We shouldn't be modifying the tree in computePreferredLogicalWidths.
- // Instead, we should properly hook the appropriate changes in the DOM and modify
- // the render tree then. When that's done, we also won't need to override
- // computePreferredLogicalWidths at all.
- // https://bugs.webkit.org/show_bug.cgi?id=104829
- SetLayoutNeededForbiddenScope layoutForbiddenScope(this, false);
-#endif
-
- if (!m_attached)
- attachQuote();
- setTextInternal(originalText());
-
- RenderText::computePreferredLogicalWidths(lead);
-}
-
const QuotesData* RenderQuote::quotesData() const
{
if (QuotesData* customQuotes = style()->quotes())
@@ -285,14 +272,8 @@
ASSERT(view());
ASSERT(!m_attached);
ASSERT(!m_next && !m_previous);
+ ASSERT(isRooted());
- // FIXME: Don't set pref widths dirty during layout. See updateDepth() for
- // more detail.
- if (!isRooted()) {
- setNeedsLayoutAndPrefWidthsRecalc();
- return;
- }
-
if (!view()->renderQuoteHead()) {
view()->setRenderQuoteHead(this);
m_attached = true;
@@ -370,12 +351,8 @@
break;
}
}
- // FIXME: Don't call setNeedsLayout or dirty our preferred widths during layout.
- // This is likely to fail anyway as one of our ancestor will call setNeedsLayout(false),
- // preventing the future layout to occur on |this|. The solution is to move that to a
- // pre-layout phase.
if (oldDepth != m_depth)
- setNeedsLayoutAndPrefWidthsRecalc();
+ updateText();
}
} // namespace WebCore
Modified: trunk/Source/WebCore/rendering/RenderQuote.h (143059 => 143060)
--- trunk/Source/WebCore/rendering/RenderQuote.h 2013-02-15 23:07:44 UTC (rev 143059)
+++ trunk/Source/WebCore/rendering/RenderQuote.h 2013-02-15 23:11:47 UTC (rev 143060)
@@ -36,17 +36,18 @@
RenderQuote(Document*, const QuoteType);
virtual ~RenderQuote();
void attachQuote();
+
+ virtual void updateText() OVERRIDE;
+
+private:
void detachQuote();
-private:
virtual void willBeDestroyed() OVERRIDE;
virtual const char* renderName() const OVERRIDE { return "RenderQuote"; };
virtual bool isQuote() const OVERRIDE { return true; };
virtual PassRefPtr<StringImpl> originalText() const OVERRIDE;
+ virtual void styleDidChange(StyleDifference, const RenderStyle*) OVERRIDE;
- virtual void updateText() OVERRIDE;
- virtual void computePreferredLogicalWidths(float leadWidth) OVERRIDE;
-
// We don't override insertedIntoTree to call attachQuote() as it would be attached
// too early and get the wrong depth since generated content is inserted into anonymous
// renderers before going into the main render tree. Once we can ensure that insertIntoTree,
