[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
bugzilla-daemon at webkit.org
Fri May 8 09:34:31 PDT 2020
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
Attachment 398838: Patch
--- Comment #9 from Darin Adler <darin at apple.com> ---
Comment on attachment 398838
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
I don’t think we should land it with this name, but the direction does seem
> + 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!
> + for (auto& element :
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.
>>>> + 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
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
More information about the webkit-reviews