[webkit-reviews] review denied: [Bug 211613] Add a helper that returns a Node if it is an Element and otherwise returns its parent Element : [Attachment 398838] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Wenson Hsieh
<wenson_hsieh at apple.com>'s request for review:
Bug 211613: Add a helper that returns a Node if it is an Element and otherwise
returns its parent Element
https://bugs.webkit.org/show_bug.cgi?id=211613

Attachment 398838: Patch

https://bugs.webkit.org/attachment.cgi?id=398838&action=review




--- Comment #9 from Darin Adler <darin at apple.com> ---
Comment on attachment 398838
  --> https://bugs.webkit.org/attachment.cgi?id=398838
Patch

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

There are a few other call sites that could use this. I found
Range::createContextualFragment, HitTestResult::targetNode, and
HitTestResult::innerNonSharedElement.

I don’t think we should land it with this name, but the direction does seem
good.

> Source/WebCore/accessibility/AXObjectCache.cpp:2954
> +    auto* element = node->inclusiveParentElement();

This could be using lineageOfType<Element>, after getting this start, so same
as what I say at the next call site below. Seems like almost half of these are
just trying to get a lineageOfType started!

> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1047
> +    for (auto& element :
lineageOfType<Element>(*node->inclusiveParentElement())) {

For a case like this, it seems the real solution would be to upgrade
lineageOfType so it can take a Node&.

This leads to the idea of just using lineageOfType<Element>().first() always
for this as Chris suggested (could even find a way to optimize that so it
doesn’t unnecessarily loop if the parent is a non-Element ContainerNode). Love
to hear what Antti thinks — I believe he created lineageOfType so he might
remembers why it doesn’t have an overload for Node. I think it was part of our
ambition to refactor in the direction of having an Element-centric DOM where
text nodes would not have to be allocated in advance.

Might also want to make it take a Node*/Element* and not iterate anything in
that case; could really be handy to keep code tidy and not have a lot of
explicit null checks. I would like to hear Antti’s opinion on that, too.

Note also that we intend to rename lineageOfType to lineage.

Given how many of these are just getting an argument to pass to lineageOfType,
seems like we should fix that first. And even some of the ones not using it
seem like they could be using it.

>>>> Source/WebCore/dom/Node.h:125
>>>> +	  Element* inclusiveParentElement();
>>> 
>>> I find this name to be very confusing. It is unclear what an inclusive
parent is supposed to be.
>> 
>> My proposal: firstElementInLineage()
>> 
>> Naming would be consistent with our Element iterators (i.e.
linerageOfType<>() / ancestorsOfType<>()).
> 
> Darin offered this name, with the reasoning that the DOM specification uses
the term "inclusive ancestor" to mean "self or ancestor", similarly "inclusive
descendant" and "inclusive sibling”, so "inclusive parent" means self or parent
element.

Love the firstElementInLineage name!

We should consider returning RefPtr instead of a raw pointer, as in any new
function, better for helping us use smart pointers more consistently in the
future. For a new function with no existing call sites, worth starting off
right.