[webkit-reviews] review granted: [Bug 117378] Split the 3 paths of SelectorDataList::execute() into 3 separate functions : [Attachment 204111] Patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has granted Benjamin Poulain
<benjamin at webkit.org>'s request for review:
Bug 117378: Split the 3 paths of SelectorDataList::execute() into 3 separate
functions
https://bugs.webkit.org/show_bug.cgi?id=117378

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

------- Additional Comments from Ryosuke Niwa <rniwa at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=204111&action=review


> Source/WebCore/ChangeLog:23
> +2013-06-08  Benjamin Poulain  <bpoulain at apple.com>
> +
> +	   Scrolling with platformWidget and delegateScrolling is incorrectly
clamped
> +	   https://bugs.webkit.org/show_bug.cgi?id=117369
> +	   <rdar://problem/13985064>

Please remove this change log entry before you land it.

> Source/WebCore/dom/SelectorQuery.cpp:-106
> -    // We need to return the matches in document order. To use id lookup
while there is possiblity of multiple matches
> -    // we would need to sort the results. For now, just traverse the
document in that case.
> -    if (m_selectors.size() != 1)
> -	   return 0;

Should we assert this instead?

> Source/WebCore/dom/SelectorQuery.cpp:132
> +	   const Vector<Element*>* elements =
rootNode->treeScope()->getAllElementsById(idToMatch);

Should we also split this block into a separate function?

> Source/WebCore/dom/SelectorQuery.cpp:148
> +    if (!element || !(isTreeScopeRoot(rootNode) ||
element->isDescendantOf(rootNode)))
> +	   return;
> +    if (selectorMatches(selectorData, element, rootNode))

Maybe cleaner to combine these two if statements as in:
if (element && (isTreeScopeRoot(rootNode) || element->isDescendantOf(rootNode))
&& selectorMatches(selectorData, element, rootNode))
    matchedElements.append(element);

> Source/WebCore/dom/SelectorQuery.cpp:186
> +    unsigned selectorCount = m_selectors.size();
> +    if (selectorCount == 1) {

Since this local variable is only used in one place, we can just do
m_selectors.size() == 1 instead.

> Source/WebCore/dom/SelectorQuery.cpp:194
> +	   if (const CSSSelector* idSelector = selectorForIdLookup(rootNode,
selectorData.selector)) {
> +	       executeFastPathForIdSelector<firstMatchOnly>(rootNode,
selectorData, idSelector, matchedElements);
> +	       return;
> +	   }
> +
> +	   executeSingleSelectorData<firstMatchOnly>(rootNode, selectorData,
matchedElements);
> +	   return;

Since we have a single statement inside if, it might be beter to simply use
else instead:
if (const CSSSelector* idSelector = ~)
    executeFastPathForIdSelector<...);
else
    executeSingleSelectorData<...);