Bug 53024 – It is needlessly expensive to find the generating node from an anonymous renderer of a pseudoelement.

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Carol Szabo]

In order to correctly implement CSS2.1 counters, one needs to efficiently walk the element tree in order to find the appropriate value for a counter. Counters are currently attached to the renderer tree because only visible elements should affect counter values and because some of them belong to pseudo elements not present in the DOM tree such as those introduced by the :before and :after selector qualifiers.
As counters are attached often to anonymous renderers (those of pseudo elements) and other anonymous renderers (and who knows, maybe even non-anonymous absolutely positioned renderers) may intervene between these renderers and the renderer of their generating node, it is hard to find the start point where to walk the DOM tree from to find the enclosing scope of these counters.

[Carol Szabo]

Proposed fix: This is a limited fix that only addresses the link between the renderers of :before and :after pseudo elements and their generating element.
Currently when renderers are created there is a convention that an anonymous renderer is designated by passing the document as the associated node.
At construction time this property (anonymity) is preserved in a bit field and the m_node member stores a pointer to the document node. This stored pointer is redundant from this point on so the idea of this fix is to reuse the location of this pointer (m_node) to store a pointer to the generating node.
the m_node is private to RenderObject and is never retrieved directly. The node() accessor used to retrieve this value returns 0 for anonymous renderers, thus the behavior will not be affected by the change.
There is already a setNode() accessor to change the value of m_node used by RenderWidget to reset m_node when the RenderWidget is destroyed but its refcount did not reach 0, thus it can't be freed.
I propose introducing a new accessor generatingNode() that would return m_node unconditionally, making it possible to obtain thois information for anonymous nodes.
Also, I propose introducing after() and before() accessors to retrieve the after and before pseudo elements that may be attached to a renderer, as the algorithm to get these is now private to RenderObjectChildren, and is also needed elsewhere such as DumpRenderTree::LayoutController::getCounterValueForId() and the counter evaluation code.

[Carol Szabo]

Created attachment 79952 [details]
Proposed Patch

[WebKit Review Bot]

Attachment 79952 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7493327

[Carol Szabo]

Created attachment 79956 [details]
Proposed Patch. Fixed chromium build issue

[Dave Hyatt]

Comment on attachment 79956 [details]
Proposed Patch. Fixed chromium build issue

View in context: https://bugs.webkit.org/attachment.cgi?id=79956&action=review

> Source/WebCore/rendering/RenderObject.h:157
> +    RenderObject* before() const
> +    {
> +        if (const RenderObjectChildList* children = virtualChildren())
> +            return children->before(this);
> +        return 0;
> +    }
> +    RenderObject* after() const
> +    {
> +        if (const RenderObjectChildList* children = virtualChildren())
> +            return children->after(this);
> +        return 0;
> +    }

These names are just too generic.  They sound like you're getting the previous and next siblings.  I think you need to rename them to beforePseudoElementContainer() and afterPseudoElementContainer().

> Source/WebCore/rendering/RenderObject.h:463
> +    Node* generatingNode() const { return m_node == document() ? 0 : m_node; }

Ultimately this should be set correctly for all pseudo-elements.  I think it's ok to limit it to before/after as a first cut, but we should have a follow-up bug for correcting this for the other pseudo-elements.

[Dave Hyatt]

Comment on attachment 79956 [details]
Proposed Patch. Fixed chromium build issue

Looking at this further, I don't really think you need to touch RenderObject.h.  You're just slowing the code down by making RenderObjectChildList::beforeAfterContainer go back through the RenderObject to fetch virtualChildren(), which is the same list you're already dealing with.

[Carol Szabo]

Created attachment 80288 [details]
Proposed Patch

Refactored beforeAfterContainer (which was not a member of RenderObjectChildren, that is why it needed to go back to RenderObject to find the RenderObjectChildrenList).
Renamed `before` and `after` methods to beforePseudoElementRenderer and afterPseudoElementRenderer respectively. Did not like `Container`, because the returned renderer does not contain the PseudoElement it is either the pseudoelement or its renderer depending on interpretation of what a pseudoelement is. I was always confused by the prior nomenclature thinking that the returned Renderer contains the before and the after pseudoelements' renderers.

[Dave Hyatt]

Comment on attachment 80288 [details]
Proposed Patch

r=me

[WebKit Commit Bot]

Comment on attachment 80288 [details]
Proposed Patch

Clearing flags on attachment: 80288

Committed r77191: <http://trac.webkit.org/changeset/77191>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.