[webkit-reviews] review denied: [Bug 137418] Implement closest() API : [Attachment 239265] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Dhi Aurrahman
<diorahman at rockybars.com>'s request for review:
Bug 137418: Implement closest() API
https://bugs.webkit.org/show_bug.cgi?id=137418

Attachment 239265: Patch
https://bugs.webkit.org/attachment.cgi?id=239265&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=239265&action=review


review- because of the bad cast

> Source/WebCore/dom/Element.cpp:2329
> +    SelectorQuery* selectorQuery =
document().selectorQueryForString(selector, ec);
> +    if (selectorQuery)
> +	   return selectorQuery->closest(*this);
> +    return nullptr;

WebKit coding style prefers early return, meaning:

    if (!selectorQuery)
	return nullptr;

> Source/WebCore/dom/SelectorQuery.cpp:128
> +    Element* currentNode = (Element *) &rootNode;

The formatting here isn’t right, but more importantly this is a bad cast if the
root node is a document node. That needs to be fixed.

> Source/WebCore/dom/SelectorQuery.cpp:151
> +    unsigned selectorCount = m_selectors.size();
> +    for (unsigned i = 0; i < selectorCount; ++i) {
> +	   Element* closestElement = selectorClosest(m_selectors[i],
targetElement, targetElement);

This should use a modern for loop:

    for (auto& selector : m_selectors) {
	if (Element* closestElement = selectorClosest(selector, targetElement,
targetElement))
	    return closestElement;
    }