[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 #13 from Darin Adler <darin at apple.com>  2013-08-17 05:22:41 PST ---
(From update of attachment 208986)
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?

> 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?

> 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?

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