DocumentOrderedMap has m_map and m_duplicateCounts but it doesn’t have to.
Created attachment 201913 [details] Cleanup + accidental bug fix
<rdar://problem/13895825>
I might get a chance to review this later. Ryosuke, bug 80224 is also about a problem in this class.
Comment on attachment 201913 [details] Cleanup + accidental bug fix View in context: https://bugs.webkit.org/attachment.cgi?id=201913&action=review I started reviewing this and then I learned that the season finale of New Girl was available on iTunes. > Source/WebCore/dom/DocumentOrderedMap.cpp:119 > + entry.orderedList.clear(); // FIXME: Remove the element instead but it can't be too slow. I'm not sure I agree with this FIXME. Removal in the duplicate id case, followed by a getElementById for that id, is not obviously worth optimizing.
(In reply to comment #4) > I'm not sure I agree with this FIXME. Removal in the duplicate id case, followed by a getElementById for that id, is not obviously worth optimizing. I think it's possible for authors to insert an element with some id, and then remove the other element of the same id next. I agree it's probably not worth optimizing the case where there will be more than two elements with the same id, and one of them is removed.
> I think it's possible for authors to insert an element with some id, and then remove the other element of the same id next. I agree it's probably not worth optimizing the case where there will be more than two elements with the same id, and one of them is removed. Sounds reasonable. Maybe the FIXME should say "FIXME: For short lists, remove the element from the list instead of clearing the list."
Comment on attachment 201913 [details] Cleanup + accidental bug fix View in context: https://bugs.webkit.org/attachment.cgi?id=201913&action=review r=me > Source/WebCore/dom/SelectorQuery.cpp:101 > +const CSSSelector* SelectorDataList::canUseIdLookup(Node* rootNode) const Since the return value changed, should we rename this? Something like "selectorForIdLookup"? > Source/WebCore/dom/SelectorQuery.cpp:131 > + const AtomicString idToMatch = idSelector->value(); Should be const AtomicString&.
Comment on attachment 201913 [details] Cleanup + accidental bug fix View in context: https://bugs.webkit.org/attachment.cgi?id=201913&action=review Thanks for the review! >> Source/WebCore/dom/DocumentOrderedMap.cpp:119 >> + entry.orderedList.clear(); // FIXME: Remove the element instead but it can't be too slow. > > I'm not sure I agree with this FIXME. Removal in the duplicate id case, followed by a getElementById for that id, is not obviously worth optimizing. Will rephrase the FIXME. >> Source/WebCore/dom/SelectorQuery.cpp:101 >> +const CSSSelector* SelectorDataList::canUseIdLookup(Node* rootNode) const > > Since the return value changed, should we rename this? Something like "selectorForIdLookup"? Oh, I must have made the exact rename on another checkout :/ Will do that again before I land. >> Source/WebCore/dom/SelectorQuery.cpp:131 >> + const AtomicString idToMatch = idSelector->value(); > > Should be const AtomicString&. Oops, will fix.
Committed r150187: <http://trac.webkit.org/changeset/150187>