Bug 117378

Summary: Split the 3 paths of SelectorDataList::execute() into 3 separate functions
Product: WebKit Reporter: Benjamin Poulain <benjamin>
Component: New BugsAssignee: Benjamin Poulain <benjamin>
Status: RESOLVED FIXED    
Severity: Normal CC: allan.jensen, commit-queue, esprehn+autocc, rniwa
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch rniwa: review+

Description Benjamin Poulain 2013-06-08 20:50:07 PDT
Split the 3 paths of SelectorDataList::execute() into 3 separate functions
Comment 1 Benjamin Poulain 2013-06-08 20:57:52 PDT
Created attachment 204107 [details]
Patch
Comment 2 Benjamin Poulain 2013-06-09 00:00:38 PDT
Created attachment 204111 [details]
Patch
Comment 3 Ryosuke Niwa 2013-06-09 02:01:05 PDT
Comment on attachment 204111 [details]
Patch

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

> Source/WebCore/ChangeLog:23
> +2013-06-08  Benjamin Poulain  <bpoulain@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<...);
Comment 4 Benjamin Poulain 2013-06-09 13:00:06 PDT
Damn, mixed with the previous patch. Sorry about that.
Comment 5 Benjamin Poulain 2013-06-09 13:58:00 PDT
Committed r151359: <http://trac.webkit.org/changeset/151359>