[webkit-reviews] review denied: [Bug 53024] It is needlessly expensive to find the generating node from an anonymous renderer of a pseudoelement. : [Attachment 79956] Proposed Patch. Fixed chromium build issue

[bugzilla-daemon at webkit.org]

Dave Hyatt <hyatt at apple.com> has denied Carol Szabo <carol.szabo at nokia.com>'s
request for review:
Bug 53024: It is needlessly expensive to find the generating node from an
anonymous renderer of a pseudoelement.
https://bugs.webkit.org/show_bug.cgi?id=53024

Attachment 79956: Proposed Patch. Fixed chromium build issue
https://bugs.webkit.org/attachment.cgi?id=79956&action=review

------- Additional Comments from Dave Hyatt <hyatt at apple.com>
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.