[CSS Regions] Elements in a region should be assignable to a named flow

Source/core/dom/NodeRenderingContext.cpp

Index: Source/core/dom/NodeRenderingContext.cpp
diff --git a/Source/core/dom/NodeRenderingContext.cpp b/Source/core/dom/NodeRenderingContext.cpp
index 1533f0accef6fcc060669ea4d3e3c6c2d8793282..77fc3b692bea8bd4d6c5a9b5d40e15e0b300ba18 100644
--- a/Source/core/dom/NodeRenderingContext.cpp
+++ b/Source/core/dom/NodeRenderingContext.cpp
@@ -190,40 +190,65 @@ bool NodeRenderingContext::shouldCreateRenderer() const
     return true;
 }
 
-void NodeRenderingContext::moveToFlowThreadIfNeeded()
+bool NodeRenderingContext::shouldMoveToFlowThread() const
 {
     ASSERT(m_node->isElementNode());
-    ASSERT(m_style);
+    Element* element = toElement(m_node);
+
+    Document* doc = m_node->document();
+
     if (!RuntimeEnabledFeatures::cssRegionsEnabled())
-        return;
+        return false;
 
-    if (m_style->flowThread().isEmpty())
-        return;
+    if (doc->webkitIsFullScreen() && doc->webkitCurrentFullScreenElement() == m_node)
+        return false;
+
+    if (m_node->isInShadowTree())
+        return false;
 
     // As per http://dev.w3.org/csswg/css3-regions/#flow-into, pseudo-elements such as ::first-line, ::first-letter, ::before or ::after
     // cannot be directly collected into a named flow.
     if (m_node->isPseudoElement())
-        return;
-
-    // FIXME: Do not collect elements if they are in shadow tree.
-    if (m_node->isInShadowTree())
-        return;
-
-    Document* document = m_node->document();
-    if (document->webkitIsFullScreen() && document->webkitCurrentFullScreenElement() == m_node)
-        return;
+        return false;
 
-#if ENABLE(SVG)
     // Allow only svg root elements to be directly collected by a render flow thread.
     if (m_node->isSVGElement()
         && (!(m_node->hasTagName(SVGNames::svgTag) && m_node->parentNode() && !m_node->parentNode()->isSVGElement())))
+        return false;
+
+    if (!m_style)
+        m_style = element->styleForRenderer();
+    if (!m_style)
+        return false;
+
+    if (m_style->flowThread().isEmpty())
+        return false;
+
+    if (doc->renderView()->flowThreadController()->isContentNodeRegisteredWithAnyNamedFlow(m_node))
+        return false;
+
+    return true;
+}
+
+void NodeRenderingContext::moveToFlowThreadIfNeeded()
+{
+    if (!shouldMoveToFlowThread())
         return;
-#endif
 
-    m_flowThread = m_style->flowThread();
+    moveToFlowThread();
+}
+
+void NodeRenderingContext::moveToFlowThread()
+{
+    ASSERT(m_node->isElementNode());
+    ASSERT(shouldMoveToFlowThread());
+
+    if (!m_style)
+        m_style = toElement(m_node)->styleForRenderer();
+    ASSERT(m_style);
     ASSERT(m_node->document()->renderView());
     FlowThreadController* flowThreadController = m_node->document()->renderView()->flowThreadController();
-    m_parentFlowRenderer = flowThreadController->ensureRenderFlowThreadWithName(m_flowThread);
+    m_parentFlowRenderer = flowThreadController->ensureRenderFlowThreadWithName(m_style->flowThread());
     flowThreadController->registerNamedFlowContentNode(m_node, m_parentFlowRenderer);
 }
 
@@ -243,12 +268,21 @@ void NodeRenderingContext::createRendererForElementIfNeeded()
 
     Element* element = toElement(m_node);
 
+    bool haveTestedForMoveToFlowThread = false;
+    for (const Element* parent = m_node->parentElement(); parent; parent = parent->parentElement())

[Julien - ping for review, 2013/05/29 16:47:20]

That is wrong. You shouldn't need to walk to the root to know if you can add a
child, canHaveChildren() is a local decision because it does propagate
automatically down the tree.

[Mihai Maerean, 2013/06/06 11:45:45]

Makes sense what you say, but i've tested the idea with the code below and the
bug isn't fixed, meaning that the test (region-content-flown-into-region.html)
fails:
if (const Element* parentElement = m_node->parentElement())
    if (const RenderObject* rendererParentElement = parentElement->renderer())
        if (!rendererParentElement->canHaveChildren()){
            moveToFlowThreadIfNeeded();
            haveTestedForMoveToFlowThread = true;
        }

+        if (parent->renderer() && !parent->renderer()->canHaveChildren()) { // This search avoids doing a style resolution for all nodes.

[Julien - ping for review, 2013/05/29 16:47:20]

I think you misunderstood the intent of my comment: shouldCreateRenderer has
several conditions for not creating a renderer, canHaveChildren is only one of
them.

The point was that we now have to resolve style (an expensive operation) for a
corner case even if could bail out based on the current context.

[Mihai Maerean, 2013/06/06 11:45:45]

The following code (that also fixes the bug) is in line with you said, but
causes a lot more calls to styleForRenderer (see below).
Code:
bool NodeRenderingContext::shouldCreateRenderer() const
{
    if (!m_node->document()->shouldCreateRenderers())
        return false;
    if (!m_renderingParent)
        return false;
    RenderObject* parentRenderer = this->parentRenderer();
    if (!parentRenderer)
        return shouldMoveToFlowThread();
    if (!parentRenderer->canHaveChildren()) {
        if (parentRenderer->canDOMChildrenHaveRenderParent()) {
            if (!shouldMoveToFlowThread())
                return false;
        } else
            return false;
    }
    if (!m_renderingParent->childShouldCreateRenderer(*this))
        return false;
    return true;
}
void NodeRenderingContext::createRendererForElementIfNeeded()
{...
    if (!shouldCreateRenderer())
        return;
    moveToFlowThreadIfNeeded();
...}

when opening http://www.whitehouse.gov/
the code in patch set #3 makes a total of 3478 calls to styleForRenderer
the code above: 4204 calls

when opening http://en.wikipedia.org/wiki/Main_Page
patch set #3: 4865 calls
the code above: 5584 calls

The conclusion is that patch set #3 is much faster than the code written above.

Do I leave the current code as it is?

+            moveToFlowThreadIfNeeded();
+            haveTestedForMoveToFlowThread = true;
+        }
+
     if (!shouldCreateRenderer())
         return;
-    m_style = element->styleForRenderer();
+    if (!m_style)
+        m_style = element->styleForRenderer();
     ASSERT(m_style);
 
-    moveToFlowThreadIfNeeded();
+    if (!haveTestedForMoveToFlowThread)

[Julien - ping for review, 2013/05/29 16:47:20]

I have a hard time understanding why we need another call to moveToFlowThread
here. You have done it above if needed and this looks like a stray call. Giving
some explanation about why it is required would definitely help.

[Mihai Maerean, 2013/06/06 11:45:45]

The 1st call to moveToFlowThreadIfNeeded is specifically to fix the bug as it
targets those divs that are inside a region that are flowed into a 2nd flow.

The 2nd call to moveToFlowThreadIfNeeded is for the usual case when elements
collected in a flow are not children of a region.

I will add these notes as code comments.

+        moveToFlowThreadIfNeeded();
 
     if (!element->rendererIsNeeded(*this))
         return;