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

Elements in a region should be assignable to a named flow.

The moveToFlowThreadIfNeeded() method is used to create the FlowThread before calling shouldCreateRenderer() which previously returned false because there wasn't any FlowThread to act as parent renderer which caused the dom children of regions not being collected in other flow threads.

The fix was imported and adapted from WebKit https://bugs.webkit.org/show_bug.cgi?id=74144

BUG=228566

[Mihai Maerean, 2013-05-08 11:54:12]

please review

[abarth-chromium, 2013-05-08 19:54:55]

I'm not a good reviewer for this change.  jchaffraix is probably a better
choice.

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

Please file the description too, it's not because it's moved from a WebKit bug
that we should have a description with 0 information.

https://codereview.chromium.org/15027005/diff/1/LayoutTests/fast/regions/flow...
File LayoutTests/fast/regions/flow-body-in-html.html (right):

https://codereview.chromium.org/15027005/diff/1/LayoutTests/fast/regions/flow...
LayoutTests/fast/regions/flow-body-in-html.html:3: <title>103685 - [CSS Regions]
Universal child selector on region breaks the rendering of its content</title>
This doesn't match the bug bug you mentioned (nor is the link with this new
feature really easy to draw)

https://codereview.chromium.org/15027005/diff/1/LayoutTests/fast/regions/regi...
File LayoutTests/fast/regions/region-content-flown-into-region.html (right):

https://codereview.chromium.org/15027005/diff/1/LayoutTests/fast/regions/regi...
LayoutTests/fast/regions/region-content-flown-into-region.html:89: 
The SVG case, though handled in the code, is never tested.

https://codereview.chromium.org/15027005/diff/1/LayoutTests/fast/regions/univ...
File
LayoutTests/fast/regions/universal-selector-children-to-the-same-region.html
(right):

https://codereview.chromium.org/15027005/diff/1/LayoutTests/fast/regions/univ...
LayoutTests/fast/regions/universal-selector-children-to-the-same-region.html:3:
<title>103685 - [CSS Regions] Universal child selector on region breaks the
rendering of its content</title>
Same comment.

https://codereview.chromium.org/15027005/diff/1/Source/core/dom/Element.cpp
File Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/15027005/diff/1/Source/core/dom/Element.cpp#n...
Source/core/dom/Element.cpp:2560: bool
Element::moveToFlowThreadIsNeeded(RefPtr<RenderStyle>& cachedStyle)
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.

https://codereview.chromium.org/15027005/diff/1/Source/core/dom/Element.cpp#n...
Source/core/dom/Element.cpp:2574: cachedStyle = styleForRenderer();
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.

https://codereview.chromium.org/15027005/diff/1/Source/core/dom/Element.cpp#n...
Source/core/dom/Element.cpp:2581: return
!document()->renderView()->flowThreadController()->isContentNodeRegisteredWithAnyNamedFlow(this);
Is it possible for the document not to be attached yet when this code gets
called?

https://codereview.chromium.org/15027005/diff/1/Source/core/dom/NodeRendering...
File Source/core/dom/NodeRenderingContext.cpp (left):

https://codereview.chromium.org/15027005/diff/1/Source/core/dom/NodeRendering...
Source/core/dom/NodeRenderingContext.cpp:145: RenderObject*
NodeRenderingContext::parentRenderer() const
That's a bad change on its own. Why would parentRenderer modify this?

https://codereview.chromium.org/15027005/diff/1/Source/core/dom/NodeRendering...
Source/core/dom/NodeRenderingContext.cpp:168: bool
NodeRenderingContext::shouldCreateRenderer() const
Ditto.

https://codereview.chromium.org/15027005/diff/1/Source/core/dom/PseudoElement.h
File Source/core/dom/PseudoElement.h (right):

https://codereview.chromium.org/15027005/diff/1/Source/core/dom/PseudoElement...
Source/core/dom/PseudoElement.h:51: virtual bool
moveToFlowThreadIsNeeded(RefPtr<RenderStyle>& cachedStyle) OVERRIDE
I am really not a huge fan of this function naming. Usually such functions
starts with "should" and here it would make IMO a better name:
shouldMoveElementToFlowThread

https://codereview.chromium.org/15027005/diff/1/Source/core/dom/PseudoElement...
Source/core/dom/PseudoElement.h:53: UNUSED_PARAM(cachedStyle);
If you didn't put the argument in the function signature, you wouldn't need this
line.

https://codereview.chromium.org/15027005/diff/1/Source/core/dom/Text.cpp
File Source/core/dom/Text.cpp (left):

https://codereview.chromium.org/15027005/diff/1/Source/core/dom/Text.cpp#oldc...
Source/core/dom/Text.cpp:205: bool Text::textRendererIsNeeded(const
NodeRenderingContext& context)
Again, losing const-ness makes me sad.

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

https://codereview.chromium.org/15027005/diff/1/LayoutTests/fast/regions/flow...
File LayoutTests/fast/regions/flow-body-in-html.html (right):

https://codereview.chromium.org/15027005/diff/1/LayoutTests/fast/regions/flow...
LayoutTests/fast/regions/flow-body-in-html.html:3: <title>103685 - [CSS Regions]
Universal child selector on region breaks the rendering of its content</title>
On 2013/05/09 15:30:18, Julien Chaffraix wrote:
> This doesn't match the bug bug you mentioned (nor is the link with this new
> feature really easy to draw)
There's a second webkit bug (103685) that is marked as duplicate of the original
bug being ported (74144). This test is intended for that duplicate bug's
specific nuances. I will change this title to point to 74144.

https://codereview.chromium.org/15027005/diff/1/LayoutTests/fast/regions/regi...
File LayoutTests/fast/regions/region-content-flown-into-region.html (right):

https://codereview.chromium.org/15027005/diff/1/LayoutTests/fast/regions/regi...
LayoutTests/fast/regions/region-content-flown-into-region.html:89: 
On 2013/05/09 15:30:18, Julien Chaffraix wrote:
> The SVG case, though handled in the code, is never tested.
Ok, will do.

https://codereview.chromium.org/15027005/diff/1/Source/core/dom/Element.cpp
File Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/15027005/diff/1/Source/core/dom/Element.cpp#n...
Source/core/dom/Element.cpp:2560: bool
Element::moveToFlowThreadIsNeeded(RefPtr<RenderStyle>& cachedStyle)
On 2013/05/09 15:30:18, Julien Chaffraix wrote:
> 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.
styleForRenderer not being const (as you might expect) is what's causing this
method to not be const.

https://codereview.chromium.org/15027005/diff/1/Source/core/dom/Element.cpp#n...
Source/core/dom/Element.cpp:2574: cachedStyle = styleForRenderer();
On 2013/05/09 15:30:18, Julien Chaffraix wrote:
> 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.
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).

https://codereview.chromium.org/15027005/diff/1/Source/core/dom/Element.cpp#n...
Source/core/dom/Element.cpp:2581: return
!document()->renderView()->flowThreadController()->isContentNodeRegisteredWithAnyNamedFlow(this);
On 2013/05/09 15:30:18, Julien Chaffraix wrote:
> Is it possible for the document not to be attached yet when this code gets
> called?
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.

https://codereview.chromium.org/15027005/diff/1/Source/core/dom/NodeRendering...
File Source/core/dom/NodeRenderingContext.cpp (left):

https://codereview.chromium.org/15027005/diff/1/Source/core/dom/NodeRendering...
Source/core/dom/NodeRenderingContext.cpp:145: RenderObject*
NodeRenderingContext::parentRenderer() const
On 2013/05/09 15:30:18, Julien Chaffraix wrote:
> That's a bad change on its own. Why would parentRenderer modify this?
The bug was caused by a "race condition": it wasn't clear in the old code when
the NamedFlowThread should be created.
The fix consists in creating the NamedFlowThread when we encounter the 1st
element that needs to be flowed into it.
The parent renderer for such an element is its flow thread. If the flow thread
doesn't exist, it must be created.
This is why this method is not const.

https://codereview.chromium.org/15027005/diff/1/Source/core/dom/PseudoElement.h
File Source/core/dom/PseudoElement.h (right):

https://codereview.chromium.org/15027005/diff/1/Source/core/dom/PseudoElement...
Source/core/dom/PseudoElement.h:51: virtual bool
moveToFlowThreadIsNeeded(RefPtr<RenderStyle>& cachedStyle) OVERRIDE
On 2013/05/09 15:30:18, Julien Chaffraix wrote:
> I am really not a huge fan of this function naming. Usually such functions
> starts with "should" and here it would make IMO a better name:
> shouldMoveElementToFlowThread
Ok, I will rename it.
PS: it may be possible in a future spec version to also flow nodes, not just
elements. So I would drop the "Element" word from the suggested name.

https://codereview.chromium.org/15027005/diff/1/Source/core/dom/PseudoElement...
Source/core/dom/PseudoElement.h:53: UNUSED_PARAM(cachedStyle);
On 2013/05/09 15:30:18, Julien Chaffraix wrote:
> If you didn't put the argument in the function signature, you wouldn't need
this
> line.
That's true when declaring the function. When the implementation is written in
the header at the declaration point, a missing argument would make the compiler
think it's a different method signature.

https://codereview.chromium.org/15027005/diff/1/Source/core/dom/Text.cpp
File Source/core/dom/Text.cpp (left):

https://codereview.chromium.org/15027005/diff/1/Source/core/dom/Text.cpp#oldc...
Source/core/dom/Text.cpp:205: bool Text::textRendererIsNeeded(const
NodeRenderingContext& context)
On 2013/05/09 15:30:18, Julien Chaffraix wrote:
> Again, losing const-ness makes me sad.
Me too, but there's no other way to make the compiler happy.

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

https://codereview.chromium.org/15027005/diff/1/Source/core/dom/Element.cpp
File Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/15027005/diff/1/Source/core/dom/Element.cpp#n...
Source/core/dom/Element.cpp:2574: cachedStyle = styleForRenderer();
On 2013/05/09 16:25:55, Mihai Maerean wrote:
> On 2013/05/09 15:30:18, Julien Chaffraix wrote:
> > 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.
> 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).

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.

https://codereview.chromium.org/15027005/diff/1/Source/core/dom/NodeRendering...
File Source/core/dom/NodeRenderingContext.cpp (left):

https://codereview.chromium.org/15027005/diff/1/Source/core/dom/NodeRendering...
Source/core/dom/NodeRenderingContext.cpp:145: RenderObject*
NodeRenderingContext::parentRenderer() const
On 2013/05/09 16:25:55, Mihai Maerean wrote:
> On 2013/05/09 15:30:18, Julien Chaffraix wrote:
> > That's a bad change on its own. Why would parentRenderer modify this?
> The bug was caused by a "race condition": it wasn't clear in the old code when
> the NamedFlowThread should be created.
> The fix consists in creating the NamedFlowThread when we encounter the 1st
> element that needs to be flowed into it.
> The parent renderer for such an element is its flow thread. If the flow thread
> doesn't exist, it must be created.
> This is why this method is not const.

I will need to read deeper but it seems wrong that a simple getter is now
modifying the object. This is a definite sign that the change is probably going
in the wrong direction and that the architecture isn't accommodating regions as
it should.

https://codereview.chromium.org/15027005/diff/1/Source/core/dom/PseudoElement.h
File Source/core/dom/PseudoElement.h (right):

https://codereview.chromium.org/15027005/diff/1/Source/core/dom/PseudoElement...
Source/core/dom/PseudoElement.h:53: UNUSED_PARAM(cachedStyle);
On 2013/05/09 16:25:55, Mihai Maerean wrote:
> On 2013/05/09 15:30:18, Julien Chaffraix wrote:
> > If you didn't put the argument in the function signature, you wouldn't need
> this
> > line.
> That's true when declaring the function. When the implementation is written in
> the header at the declaration point, a missing argument would make the
compiler
> think it's a different method signature.

I don't think you are right (argument names don't impact the signature, only the
number of arguments and their types). See among other example RenderBlock.h:

virtual void adjustInlineDirectionLineBounds(int /* expansionOpportunityCount
*/, float& /* logicalLeft */, float& /* logicalWidth */) const { }

[Mihai Maerean, 2013-05-15 12:45:03]

I have uploaded a new patch that incorporates the feedback.
Please review.

[Julien - ping for review, 2013-05-16 15:54:41]

Looks better, one comment though.

https://codereview.chromium.org/15027005/diff/11001/Source/core/dom/NodeRende...
File Source/core/dom/NodeRenderingContext.cpp (right):

https://codereview.chromium.org/15027005/diff/11001/Source/core/dom/NodeRende...
Source/core/dom/NodeRenderingContext.cpp:271: moveToFlowThreadIfNeeded();
There are 2 calls to moveToFlowThreadIfNeeded 8 lines apart which seems
unneeded.

Note that because of your change, we *will* do a style resolution for all
elements even if |shouldCreateRenderer| would return false. If we could keep the
current behavior of having the moveToFlowThreadIfNeeded called after calling
|shouldCreateRenderer| it would be much better but you probably want to have
moved the element before that call.

Btw, the fact that it needs to be called before shouldCreateRenderer could be
tested by adding some content under a <col> (which returns false to
canHaveChildren). Normally you shouldn't see the content but by hoisting it in
your named flow and it should be displayed.

[Mihai Maerean, 2013-05-17 15:11:25]

https://codereview.chromium.org/15027005/diff/11001/Source/core/dom/NodeRende...
File Source/core/dom/NodeRenderingContext.cpp (right):

https://codereview.chromium.org/15027005/diff/11001/Source/core/dom/NodeRende...
Source/core/dom/NodeRenderingContext.cpp:271: moveToFlowThreadIfNeeded();
On 2013/05/16 15:54:41, Julien Chaffraix wrote:
> There are 2 calls to moveToFlowThreadIfNeeded 8 lines apart which seems
> unneeded.
> 
> Note that because of your change, we *will* do a style resolution for all
> elements even if |shouldCreateRenderer| would return false. If we could keep
the
> current behavior of having the moveToFlowThreadIfNeeded called after calling
> |shouldCreateRenderer| it would be much better but you probably want to have
> moved the element before that call.
> 
> Btw, the fact that it needs to be called before shouldCreateRenderer could be
> tested by adding some content under a <col> (which returns false to
> canHaveChildren). Normally you shouldn't see the content but by hoisting it in
> your named flow and it should be displayed.

I have uploaded a new patch that incorporates the feedback to avoid doing a
style resolution for all elements even if |shouldCreateRenderer| would return
false.
Please review.

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

https://codereview.chromium.org/15027005/diff/14001/Source/core/dom/NodeRende...
File Source/core/dom/NodeRenderingContext.cpp (right):

https://codereview.chromium.org/15027005/diff/14001/Source/core/dom/NodeRende...
Source/core/dom/NodeRenderingContext.cpp:272: for (const Element* parent =
m_node->parentElement(); parent; parent = parent->parentElement())
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.

https://codereview.chromium.org/15027005/diff/14001/Source/core/dom/NodeRende...
Source/core/dom/NodeRenderingContext.cpp:273: if (parent->renderer() &&
!parent->renderer()->canHaveChildren()) { // This search avoids doing a style
resolution for all nodes.
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.

https://codereview.chromium.org/15027005/diff/14001/Source/core/dom/NodeRende...
Source/core/dom/NodeRenderingContext.cpp:284: if
(!haveTestedForMoveToFlowThread)
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]

https://codereview.chromium.org/15027005/diff/14001/Source/core/dom/NodeRende...
File Source/core/dom/NodeRenderingContext.cpp (right):

https://codereview.chromium.org/15027005/diff/14001/Source/core/dom/NodeRende...
Source/core/dom/NodeRenderingContext.cpp:272: for (const Element* parent =
m_node->parentElement(); parent; parent = parent->parentElement())
On 2013/05/29 16:47:20, Julien Chaffraix wrote:
> 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.
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;
        }

https://codereview.chromium.org/15027005/diff/14001/Source/core/dom/NodeRende...
Source/core/dom/NodeRenderingContext.cpp:273: if (parent->renderer() &&
!parent->renderer()->canHaveChildren()) { // This search avoids doing a style
resolution for all nodes.
On 2013/05/29 16:47:20, Julien Chaffraix wrote:
> 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.
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?

https://codereview.chromium.org/15027005/diff/14001/Source/core/dom/NodeRende...
Source/core/dom/NodeRenderingContext.cpp:284: if
(!haveTestedForMoveToFlowThread)
On 2013/05/29 16:47:20, Julien Chaffraix wrote:
> 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.
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.

[Mihai Maerean, 2013-06-14 10:01:57]

I have uploaded a new patch that integrates all the feedback.
Please review.

One of the tests causes an ASSERT in ContentShell because of Region Ranges
(RenderFlowThread::removeRenderBoxRegionInfo).
So this fix has revealed a new problem with Region Ranges that needs to be fixed
before this patch can be landed.

[Mihai Maerean, 2013-06-14 10:02:47]

I have uploaded a new patch that integrates all the feedback.
Please review.

One of the tests causes an ASSERT in ContentShell because of Region Ranges
(RenderFlowThread::removeRenderBoxRegionInfo).
So this fix has revealed a new problem with Region Ranges that needs to be fixed
before this patch can be landed.

[esprehn, 2013-06-17 21:41:02]

not lgtm. This is going to be bad for perf. Lets discuss what you're trying to
do and figure out how to do this a better way.

https://codereview.chromium.org/15027005/diff/22001/LayoutTests/fast/regions/...
File LayoutTests/fast/regions/flow-body-in-html.html (right):

https://codereview.chromium.org/15027005/diff/22001/LayoutTests/fast/regions/...
LayoutTests/fast/regions/flow-body-in-html.html:10: -webkit-flow-from: foo;
Indentation is wrong for this CSS.

https://codereview.chromium.org/15027005/diff/22001/LayoutTests/fast/regions/...
File
LayoutTests/fast/regions/universal-selector-children-to-the-same-region.html
(right):

https://codereview.chromium.org/15027005/diff/22001/LayoutTests/fast/regions/...
LayoutTests/fast/regions/universal-selector-children-to-the-same-region.html:7:
-webkit-flow-into: foo;
Missing indentation.

https://codereview.chromium.org/15027005/diff/22001/LayoutTests/fast/regions/...
LayoutTests/fast/regions/universal-selector-children-to-the-same-region.html:10:
-webkit-flow-from: foo;
Same thing.

https://codereview.chromium.org/15027005/diff/22001/Source/core/dom/NodeRende...
File Source/core/dom/NodeRenderingContext.cpp (right):

https://codereview.chromium.org/15027005/diff/22001/Source/core/dom/NodeRende...
Source/core/dom/NodeRenderingContext.cpp:292: for (const Element* parentElement
= m_node->parentElement(); parentElement; parentElement =
parentElement->parentElement()) {
Woah, this isn't right. This is going to cause perf regressions since you walk
up the tree. It's also not correct since it's not Shadow DOM aware.

https://codereview.chromium.org/15027005/diff/22001/Source/core/dom/NodeRende...
Source/core/dom/NodeRenderingContext.cpp:309: getStyleForRenderer(); //
rendererIsNeeded will call NodeRenderingContext::style()
Calling a method with a return value and ignoring it is bad. Lets not do this.

https://codereview.chromium.org/15027005/diff/22001/Source/core/dom/NodeRende...
File Source/core/dom/NodeRenderingContext.h (right):

https://codereview.chromium.org/15027005/diff/22001/Source/core/dom/NodeRende...
Source/core/dom/NodeRenderingContext.h:74: PassRefPtr<RenderStyle>
getStyleForRenderer() const;
styleForRenderer(). Methods shouldn't be prefixed with get* unless they return
through out args.

https://codereview.chromium.org/15027005/diff/22001/Source/core/rendering/Ren...
File Source/core/rendering/RenderObject.cpp (right):

https://codereview.chromium.org/15027005/diff/22001/Source/core/rendering/Ren...
Source/core/rendering/RenderObject.cpp:3137: return m_parent ?
m_parent->canDOMChildrenHaveRenderParent() : false;
m_parent && m_parent->....; No need for the ternary.

[maerean, 2013-06-17 21:58:45]

https://codereview.chromium.org/15027005/diff/22001/Source/core/dom/NodeRende...
File Source/core/dom/NodeRenderingContext.cpp (right):

https://codereview.chromium.org/15027005/diff/22001/Source/core/dom/NodeRende...
Source/core/dom/NodeRenderingContext.cpp:309: getStyleForRenderer(); //
rendererIsNeeded will call NodeRenderingContext::style()
On 2013/06/17 21:41:02, esprehn wrote:
> Calling a method with a return value and ignoring it is bad. Lets not do this.
I have tried using getStyleForRenderer in Element::rendererIsNeeded but
1. that exploded in a big number of changes since rendererIsNeeded is overridden
in the SVG element classes.
2. that is not correct since NodeRenderingContent::m_style can store many
things: styleForRenderer(), document->styleResolver()->defaultStyleForElement()
and parentRenderer->style().

[esprehn, 2013-08-08 04:54:20]

Closing this issue, the work was completed in:
https://chromiumcodereview.appspot.com/18080016