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

Source/core/dom/NodeRenderingContext.cpp

 /*
  * Copyright (C) 1999 Lars Knoll (knoll@kde.org)
  *           (C) 1999 Antti Koivisto (koivisto@kde.org)
  *           (C) 2001 Dirk Mueller (mueller@kde.org)
  * Copyright (C) 2004, 2005, 2006, 2007, 2008, 2009, 2010, 2011 Apple Inc. All rights reserved.
  * Copyright (C) 2008, 2009 Torch Mobile Inc. All rights reserved. (http://www.torchmobile.com/)
  * Copyright (C) 2011 Google Inc. All rights reserved.
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Library General Public
@@ skipping 172 matching lines @@
     RenderObject* parentRenderer = this->parentRenderer();
     if (!parentRenderer)
         return false;
     if (!parentRenderer->canHaveChildren())
         return false;
     if (!m_renderingParent->childShouldCreateRenderer(*this))
         return false;
     return true;
 }
 
+bool NodeRenderingContext::shouldMoveToFlowThread() const
+{
+    ASSERT(m_node->isElementNode());
+    Element* element = toElement(m_node);
+
+    Document* doc = m_node->document();
+
+    if (!RuntimeEnabledFeatures::cssRegionsEnabled())
+        return false;
+
+    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 false;
+
+    // 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()
 {
-    ASSERT(m_node->isElementNode());
-    ASSERT(m_style);
-    if (!RuntimeEnabledFeatures::cssRegionsEnabled())
+    if (!shouldMoveToFlowThread())
         return;
 
-    if (m_style->flowThread().isEmpty())
-        return;
+    moveToFlowThread();
+}
 
-    // 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;
+void NodeRenderingContext::moveToFlowThread()
+{
+    ASSERT(m_node->isElementNode());
+    ASSERT(shouldMoveToFlowThread());
 
-    // 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;
-
-#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;
-#endif
-
-    m_flowThread = m_style->flowThread();
+    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);
 }
 
 bool NodeRenderingContext::isOnEncapsulationBoundary() const
 {
     return isOnUpperEncapsulationBoundary() || isLowerEncapsulationBoundary(m_parentDetails.insertionPoint()) || isLowerEncapsulationBoundary(m_node->parentNode());
 }
 
 bool NodeRenderingContext::isOnUpperEncapsulationBoundary() const
 {
     return m_node->parentNode() && m_node->parentNode()->isShadowRoot();
 }
 
 void NodeRenderingContext::createRendererForElementIfNeeded()
 {
     ASSERT(!m_node->renderer());
 
     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;
 
     RenderObject* parentRenderer = this->parentRenderer();
     RenderObject* nextRenderer = this->nextRenderer();
 
     Document* document = element->document();
     RenderObject* newRenderer = element->createRenderer(document->renderArena(), m_style.get());
     if (!newRenderer)
@@ skipping 53 matching lines @@
     newRenderer->setFlowThreadState(parentRenderer->flowThreadState());
 
     RenderObject* nextRenderer = this->nextRenderer();
     textNode->setRenderer(newRenderer);
     // Parent takes care of the animations, no need to call setAnimatableStyle.
     newRenderer->setStyle(m_style.release());
     parentRenderer->addChild(newRenderer, nextRenderer);
 }
 
 }