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

Source/core/dom/Element.cpp

 /*
  * Copyright (C) 1999 Lars Knoll (knoll@kde.org)
  *           (C) 1999 Antti Koivisto (koivisto@kde.org)
  *           (C) 2001 Peter Kelly (pmk@post.com)
  *           (C) 2001 Dirk Mueller (mueller@kde.org)
  *           (C) 2007 David Smith (catfish.man@gmail.com)
  * Copyright (C) 2004, 2005, 2006, 2007, 2008, 2009, 2010, 2012, 2013 Apple Inc. All rights reserved.
  *           (C) 2007 Eric Seidel (eric@webkit.org)
  *
  * This library is free software; you can redistribute it and/or
@@ skipping 2539 matching lines @@
 }
 
 RenderRegion* Element::renderRegion() const
 {
     if (renderer() && renderer()->isRenderRegion())
         return toRenderRegion(renderer());
 
     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();
 
     DEFINE_STATIC_LOCAL(AtomicString, undefinedState, ("undefined", AtomicString::ConstructFromLiteral));
     if (!RuntimeEnabledFeatures::cssRegionsEnabled() || !renderRegion())
         return undefinedState;
 
     switch (renderRegion()->regionState()) {
     case RenderRegion::RegionFit: {
@@ skipping 572 matching lines @@
     return 0;
 }
 
 Attribute* UniqueElementData::attributeItem(unsigned index)
 {
     ASSERT_WITH_SECURITY_IMPLICATION(index < length());
     return &m_attributeVector.at(index);
 }
 
 } // namespace WebCore