[Webkit-unassigned] [Bug 119886] PseudoElement is abusing parent node pointer

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=119886





--- Comment #14 from Antti Koivisto <koivisto at iki.fi>  2013-08-17 07:02:14 PST ---
(In reply to comment #13)
> (From update of attachment 208986 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=208986&action=review
> 
> The part of this patch I don’t understand is which calls to parentNode you decided to change to add checks for pseudo elements and shadow roots. I don’t understand why you had to do this in some places but not others and how you are sure you covered everything. The RenderCounter and RenderListItem cases are the puzzling ones. I don’t see how the code worked before. It was just calling parentNode like tons of other places that call parentNode. How did you know that these were the ones that needed to be changed?

RenderCounter and RenderListItem do traversal where PseudoElements are treated as if they were real elements to compute counter values. The code for going to siblings and down the tree was already using explicit pseudo element tests to find the right node. Traversing up did not need the tests since the parentNode of the PseudoElement was set to point to the host (the asymmetry was a result of violating the "node is its parents child" consistency). 

I changed all call sites that are part of this special type of traversal. We have pretty good test coverage here, some that I had missed were spotted by the tests.

Making PseudoElements actual Elements was probably a mistake in the first place. Benefits are not obvious.

> > Source/WebCore/rendering/RenderCounter.cpp:72
> > +    if (object->node()->isPseudoElement())
> > +        return toPseudoElement(object->node())->hostElement();
> > +    return toElement(object->node())->parentElement();
> 
> I would have written this with a local variable, but I guess we can count on the compiler to get this right?

Everything that is being called is inlined so I think so. But yes, it would look nicer with variables.

> > Source/WebCore/rendering/RenderListItem.cpp:108
> > +    Node* parent = listItemNode->isPseudoElement() ? toPseudoElement(listItemNode)->hostElement() : listItemNode->parentNode();
> >      // We use parentNode because the enclosing list could be a ShadowRoot that's not Element.
> > -    for (Node* parent = listItemNode->parentNode(); parent; parent = parent->parentNode()) {
> > +    for (; parent; parent = parent->parentNode()) {
> 
> Why does the first step of walking up the parent tree need to traverse host element pointers, but not any other steps? Can’t there be a transition to a host later on? Is this covered by tests?

PseudoElements can only ever be leaf nodes (for purpose of this traversal case, they are really not in the tree at all). This sort of tightening of logic is an useful side-effect.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.