Bug 117378 – Split the 3 paths of SelectorDataList::execute() into 3 separate functions

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Benjamin Poulain]

Split the 3 paths of SelectorDataList::execute() into 3 separate functions

[Benjamin Poulain]

Created attachment 204107 [details]
Patch

[Benjamin Poulain]

Created attachment 204111 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 204111 [details]
Patch

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

> Source/WebCore/ChangeLog:23
> [phone]  Benjamin Poulain  <[e-mail]>
> +
> +        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<...);

[Benjamin Poulain]

Damn, mixed with the previous patch. Sorry about that.

[Benjamin Poulain]

Committed r151359: <http://trac.webkit.org/changeset/151359>