Summary: | Split the 3 paths of SelectorDataList::execute() into 3 separate functions | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Benjamin Poulain <benjamin> | ||||||
Component: | New Bugs | Assignee: | 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
Benjamin Poulain
2013-06-08 20:50:07 PDT
Created attachment 204107 [details]
Patch
Created attachment 204111 [details]
Patch
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<...); Damn, mixed with the previous patch. Sorry about that. Committed r151359: <http://trac.webkit.org/changeset/151359> |