Bug 116167 – DocumentOrderedMap doesn't need to have two HashMaps

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Ryosuke Niwa]

DocumentOrderedMap has m_map and m_duplicateCounts but it doesn’t have to.

[Ryosuke Niwa]

Created attachment 201913 [details]
Cleanup + accidental bug fix

[Ryosuke Niwa]

<rdar://problem/13895825>

[Darin Adler]

I might get a chance to review this later. Ryosuke, bug 80224 is also about a problem in this class.

[Geoffrey Garen]

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.

[Ryosuke Niwa]

(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.

[Geoffrey Garen]

> 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."

[Geoffrey Garen]

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&.

[Ryosuke Niwa]

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.

[Ryosuke Niwa]

Committed r150187: <http://trac.webkit.org/changeset/150187>