RenderTreeBuilder => RenderTreeBuilderForElement and RenderTreeBuilderForText

RenderTreeBuilder => RenderTreeBuilderForElement and RenderTreeBuilderForText

Previously RenderTreeBuilder::createRendererForTextIfNeeded()
and RenderTreeBuilder::createRendererForElementIfNeeded() are called
from Element and Text, respectively.

This refactoring separates logics needed for creating renderer
for Element and for Text, reducing isElement and isText checks.

More importantly, this refactoring exposes 
RenderTreeBuilderForText::createRenderer()
so that we can test Text::textRendererIsNeeded() only once when
we reattach whitespace siblings
(for https://codereview.chromium.org/684633006/).

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

[Xianzhu, 2014-11-05 01:26:29]

Patchset #1 (id:1) has been deleted

[Xianzhu, 2014-11-05 01:26:35]

Patchset #1 (id:20001) has been deleted

[Xianzhu, 2014-11-05 01:30:33]

wangxianzhu@chromium.org changed reviewers:
+ esprehn@chromium.org

[Xianzhu, 2014-11-05 01:30:34]

PTAL.

[esprehn, 2014-11-05 23:20:26]

I'm not sure this is the right approach, you don't want to create a Builder for
something that already has a renderer, you could pass the result of
textRendererIsNeeded into the builder though just like we do the style.

Do specialize the builder I think we might want templates instead of the two
subclasses.

https://codereview.chromium.org/699213003/diff/40001/Source/core/dom/RenderTr...
File Source/core/dom/RenderTreeBuilder.h (left):

https://codereview.chromium.org/699213003/diff/40001/Source/core/dom/RenderTr...
Source/core/dom/RenderTreeBuilder.h:48: ASSERT(!node->renderer());
This assert is still valid, we shouldn't create one of these for something that
already has a renderer. You'd also fail the following assert for needsAttach()
if you had one.

https://codereview.chromium.org/699213003/diff/40001/Source/core/dom/RenderTr...
File Source/core/dom/RenderTreeBuilder.h (right):

https://codereview.chromium.org/699213003/diff/40001/Source/core/dom/RenderTr...
Source/core/dom/RenderTreeBuilder.h:60: virtual bool rendererIsNeeded() const;
Does the compiler devirtualize this?

[Xianzhu, 2014-11-06 01:26:23]

PTAL.

The new patch
- moves shouldCreateRenderer logic for text out of RenderTreeBuilder into Text;
- avoid virtual, isText|isElement, toText|toElement in RenderTreeBuilder.

[Xianzhu, 2014-11-12 18:58:44]

ping...

[Xianzhu, 2014-11-14 17:05:11]

On 2014/11/12 18:58:44, Xianzhu wrote:
> ping...

ping...

[esprehn, 2014-11-14 21:58:09]

Your description is wrong, but the patch looks good. Please fix before landing.

lgtm

[Xianzhu, 2014-11-14 22:18:34]

On 2014/11/14 21:58:09, esprehn wrote:
> Your description is wrong, but the patch looks good. Please fix before
landing.
> 
Done.

> lgtm

[Xianzhu, 2014-11-14 22:18:45]

The CQ bit was checked by wangxianzhu@chromium.org

[commit-bot: I haz the power, 2014-11-14 22:19:54]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/699213003/100001

[commit-bot: I haz the power, 2014-11-14 22:41:38]

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

[commit-bot: I haz the power, 2014-11-14 22:41:39]

Try jobs failed on following builders:
  mac_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/30970)

[Xianzhu, 2014-11-15 00:43:09]

The CQ bit was checked by wangxianzhu@chromium.org

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

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/699213003/120001

[commit-bot: I haz the power, 2014-11-15 01:45:57]

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

[commit-bot: I haz the power, 2014-11-15 01:45:59]

Try jobs failed on following builders:
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/3...)

[Xianzhu, 2014-11-15 05:49:30]

The CQ bit was checked by wangxianzhu@chromium.org

[commit-bot: I haz the power, 2014-11-15 05:49:59]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/699213003/140001

[commit-bot: I haz the power, 2014-11-15 05:50:08]

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

[commit-bot: I haz the power, 2014-11-15 05:50:09]

Failed to apply patch for Source/core/dom/RenderTreeBuilder.cpp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/core/dom/RenderTreeBuilder.cpp
  Hunk #1 FAILED at 41.
  Hunk #2 FAILED at 85.
  Hunk #3 succeeded at 159 (offset 30 lines).
  Hunk #4 succeeded at 172 (offset 30 lines).
  Hunk #5 succeeded at 190 (offset 30 lines).
  2 out of 5 hunks FAILED -- saving rejects to file
Source/core/dom/RenderTreeBuilder.cpp.rej

Patch:       Source/core/dom/RenderTreeBuilder.cpp
Index: Source/core/dom/RenderTreeBuilder.cpp
diff --git a/Source/core/dom/RenderTreeBuilder.cpp
b/Source/core/dom/RenderTreeBuilder.cpp
index
7572603e73763b11ab81fd4bb3a7ae75ca1aa8b9..ddbf50a72e98eed06b481a054daf93d7014e70fb
100644
--- a/Source/core/dom/RenderTreeBuilder.cpp
+++ b/Source/core/dom/RenderTreeBuilder.cpp
@@ -41,43 +41,34 @@
 
 namespace blink {
 
-RenderObject* RenderTreeBuilder::nextRenderer() const
+RenderObject* RenderTreeBuilderForElement::nextRenderer() const
 {
-    ASSERT(m_renderingParent);
+    if (m_node->isInTopLayer())
+        return NodeRenderingTraversal::nextInTopLayer(m_node);
 
-    Element* element = m_node->isElementNode() ? toElement(m_node) : 0;
-
-    if (element && element->isInTopLayer())
-        return NodeRenderingTraversal::nextInTopLayer(element);
-
-    // Avoid an O(N^2) walk over the children when reattaching all children of
a node.
-    if (m_renderingParent->needsAttach())
-        return 0;
-
-    return NodeRenderingTraversal::nextSiblingRenderer(m_node);
+    return RenderTreeBuilder::nextRenderer();
 }
 
-RenderObject* RenderTreeBuilder::parentRenderer() const
+RenderObject* RenderTreeBuilderForElement::parentRenderer() const
 {
-    ASSERT(m_renderingParent);
-
-    Element* element = m_node->isElementNode() ? toElement(m_node) : 0;
-
-    if (element && m_renderingParent->renderer()) {
+    RenderObject* parentRenderer = RenderTreeBuilder::parentRenderer();
+    if (parentRenderer) {
         // FIXME: Guarding this by m_renderingParent->renderer() isn't quite
right as the spec for
         // top layer only talks about display: none ancestors so putting a
<dialog> inside an
         // <optgroup> seems like it should still work even though this check
will prevent it.
-        if (element->isInTopLayer())
+        if (m_node->isInTopLayer())
             return m_node->document().renderView();
     }
 
-    return m_renderingParent->renderer();
+    return parentRenderer;
 }
 
-bool RenderTreeBuilder::shouldCreateRenderer() const
+bool RenderTreeBuilderForElement::shouldCreateRenderer() const
 {
     if (!m_renderingParent)
         return false;
+
+    // FIXME: Should the following be in SVGElement::rendererIsNeeded()?
     if (m_node->isSVGElement()) {
         // SVG elements only render when inside <svg>, or if the element is an
<svg> itself.
         if (!isSVGSVGElement(*m_node) && !m_renderingParent->isSVGElement())
@@ -85,35 +76,28 @@ bool RenderTreeBuilder::shouldCreateRenderer() const
         if (!toSVGElement(m_node)->isValid())
             return false;
     }
+
     RenderObject* parentRenderer = this->parentRenderer();
     if (!parentRenderer)
         return false;
     if (!parentRenderer->canHaveChildren())
         return false;
-    return true;
+
+    return m_node->rendererIsNeeded(style());
 }
 
-RenderStyle& RenderTreeBuilder::style() const
+RenderStyle& RenderTreeBuilderForElement::style() const
 {
     if (!m_style)
-        m_style = toElement(m_node)->styleForRenderer();
+        m_style = m_node->styleForRenderer();
     return *m_style;
 }
 
-void RenderTreeBuilder::createRendererForElementIfNeeded()
+void RenderTreeBuilderForElement::createRenderer()
 {
-    ASSERT(!m_node->renderer());
-
-    if (!shouldCreateRenderer())
-        return;
-
-    Element* element = toElement(m_node);
     RenderStyle& style = this->style();
 
-    if (!element->rendererIsNeeded(style))
-        return;
-
-    RenderObject* newRenderer = element->createRenderer(&style);
+    RenderObject* newRenderer = m_node->createRenderer(&style);
     if (!newRenderer)
         return;
 
@@ -129,11 +113,11 @@ void RenderTreeBuilder::createRendererForElementIfNeeded()
     newRenderer->setFlowThreadState(parentRenderer->flowThreadState());
 
     RenderObject* nextRenderer = this->nextRenderer();
-    element->setRenderer(newRenderer);
+    m_node->setRenderer(newRenderer);
     newRenderer->setStyle(&style); // setStyle() can depend on renderer()
already being set.
 
-    if (Fullscreen::isActiveFullScreenElement(*element)) {
-        newRenderer = RenderFullScreen::wrapRenderer(newRenderer,
parentRenderer, &element->document());
+    if (Fullscreen::isActiveFullScreenElement(*m_node)) {
+        newRenderer = RenderFullScreen::wrapRenderer(newRenderer,
parentRenderer, &m_node->document());
         if (!newRenderer)
             return;
     }
@@ -142,23 +126,15 @@ void RenderTreeBuilder::createRendererForElementIfNeeded()
     parentRenderer->addChild(newRenderer, nextRenderer);
 }
 
-void RenderTreeBuilder::createRendererForTextIfNeeded()
+void RenderTreeBuilderForText::createRenderer()
 {
-    ASSERT(!m_node->renderer());
-
-    if (!shouldCreateRenderer())
-        return;
-
-    Text* textNode = toText(m_node);
     RenderObject* parentRenderer = this->parentRenderer();
+    RenderStyle* style = parentRenderer->style();
 
-    m_style = parentRenderer->style();
-
-    if (!textNode->textRendererIsNeeded(*m_style, *parentRenderer))
-        return;
+    ASSERT(m_node->textRendererIsNeeded(*style, *parentRenderer));
 
-    RenderText* newRenderer = textNode->createTextRenderer(m_style.get());
-    if (!parentRenderer->isChildAllowed(newRenderer, m_style.get())) {
+    RenderText* newRenderer = m_node->createTextRenderer(style);
+    if (!parentRenderer->isChildAllowed(newRenderer, style)) {
         newRenderer->destroy();
         return;
     }
@@ -168,9 +144,9 @@ void RenderTreeBuilder::createRendererForTextIfNeeded()
     newRenderer->setFlowThreadState(parentRenderer->flowThreadState());
 
     RenderObject* nextRenderer = this->nextRenderer();
-    textNode->setRenderer(newRenderer);
+    m_node->setRenderer(newRenderer);
     // Parent takes care of the animations, no need to call setAnimatableStyle.
-    newRenderer->setStyle(m_style.release());
+    newRenderer->setStyle(style);
     parentRenderer->addChild(newRenderer, nextRenderer);
 }

[Xianzhu, 2014-11-15 06:31:26]

The CQ bit was checked by wangxianzhu@chromium.org

[commit-bot: I haz the power, 2014-11-15 06:32:34]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/699213003/160001

[commit-bot: I haz the power, 2014-11-15 07:46:45]

Committed patchset #7 (id:160001) as 185407