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

Source/core/dom/Element.cpp

Index: Source/core/dom/Element.cpp
diff --git a/Source/core/dom/Element.cpp b/Source/core/dom/Element.cpp
index 856e7406d41552206a98dc8234200660b012162f..cd0e1ee41dd0208e1a50145c38eae846892dfcb8 100644
--- a/Source/core/dom/Element.cpp
+++ b/Source/core/dom/Element.cpp
@@ -2557,6 +2557,30 @@ RenderRegion* Element::renderRegion() const
     return 0;
 }
 
+bool Element::moveToFlowThreadIsNeeded(RefPtr<RenderStyle>& cachedStyle)

[Julien - ping for review, 2013/05/09 15:30:18]

I don't see a good reason why this isn't const as it should just return an
boolean answer based on the Element's and RenderStyle's state.

[Mihai Maerean, 2013/05/09 16:25:55]

styleForRenderer not being const (as you might expect) is what's causing this
method to not be const.

+{
+    Document* doc = document();
+
+    if (!RuntimeEnabledFeatures::cssRegionsEnabled())
+        return false;
+
+    if (doc->webkitIsFullScreen() && doc->webkitCurrentFullScreenElement() == this)
+        return false;
+
+    if (isInShadowTree())
+        return false;
+
+    if (!cachedStyle)
+        cachedStyle = styleForRenderer();

[Julien - ping for review, 2013/05/09 15:30:18]

It smells very bad to have this function fill the argument behind the scene. I
would bet it underlines that the lines between the different objects'
responsibilities was badly drawn.

[Mihai Maerean, 2013/05/09 16:25:55]

Calling styleForRenderer is very slow, so caching is needed. This cached value
has it's lifetime determined by the algorithm that calls
moveToFlowThreadIsNeeded (which is NodeRenderingContext).

[Julien - ping for review, 2013/05/10 00:13:05]

You are missing the point. That we do need to cache the value is fine. That we
need to do it in a seemingly unrelated method by modifying its argument is what
I am pointing out.

+    if (!cachedStyle)
+        return false;
+
+    if (cachedStyle->flowThread().isEmpty())
+        return false;
+
+    return !document()->renderView()->flowThreadController()->isContentNodeRegisteredWithAnyNamedFlow(this);

[Julien - ping for review, 2013/05/09 15:30:18]

Is it possible for the document not to be attached yet when this code gets
called?

[Mihai Maerean, 2013/05/09 16:25:55]

I don't know. How can we tell?
moveToFlowThreadIsNeeded is called when the page is being parsed.
All webkit tests have passed with this code activated.

+}
+
 const AtomicString& Element::webkitRegionOverset() const
 {
     document()->updateLayoutIgnorePendingStylesheets();