Summary: | It is needlessly expensive to find the generating node from an anonymous renderer of a pseudoelement. | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Carol Szabo <carol> | ||||||||
Component: | Layout and Rendering | Assignee: | Carol Szabo <carol> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | commit-queue, dglazkov, eric, hyatt, jamesr, webkit.review.bot | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | PC | ||||||||||
OS: | All | ||||||||||
Bug Depends on: | |||||||||||
Bug Blocks: | 52126, 53037 | ||||||||||
Attachments: |
|
Description
Carol Szabo
2011-01-24 10:53:39 PST
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. Created attachment 79952 [details]
Proposed Patch
Attachment 79952 [details] did not build on chromium: Build output: http://queues.webkit.org/results/7493327 Created attachment 79956 [details]
Proposed Patch. Fixed chromium build issue
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. 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.
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.
Comment on attachment 80288 [details]
Proposed Patch
r=me
Comment on attachment 80288 [details] Proposed Patch Clearing flags on attachment: 80288 Committed r77191: <http://trac.webkit.org/changeset/77191> All reviewed patches have been landed. Closing bug. |