Inspired by <https://bugs.webkit.org/show_bug.cgi?id=211151#c8>
Created attachment 398838 [details] Patch
lineageOfType<Element>(node).first()
(In reply to Chris Dumez from comment #2) > lineageOfType<Element>(node).first() Actually it looks like lineageOfType() only takes an Element in parameter for some reason.
Comment on attachment 398838 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=398838&action=review > 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.
Comment on attachment 398838 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=398838&action=review >> 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<>()).
(In reply to Chris Dumez from comment #5) > Comment on attachment 398838 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=398838&action=review > > >> 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. 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. > > My proposal: firstElementInLineage() > > Naming would be consistent with our Element iterators (i.e. > linerageOfType<>() / ancestorsOfType<>()). Nice — I find this name clear and consistent.
(In reply to Chris Dumez from comment #5) > My proposal: firstElementInLineage() Great, love that name!
So, should we have this as a Node member function, or a non-member function? - I think that member functions are tidier and easier to read at call sites. - On the other hand, an advantage of a non-member function is that we can have one that takes a Node& and another that takes a Node*. Also, would be nice to either optimize calling this on Element& or Element* to just return the passed-in object, or to make it a compile time error to call it on one. If it’s a member function we'd do that with an override with "= delete" in the Element class. If it’s a non-member function we’d do it with an overload; not sure exactly how to make it a compile time error but I’m sure there’s a way.
Comment on attachment 398838 [details] 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.
(In reply to Darin Adler from comment #9) > There are a few other call sites that could use this. I found > Range::createContextualFragment, HitTestResult::targetNode, and > HitTestResult::innerNonSharedElement. Also Position::element.