[webkit-reviews] review granted: [Bug 123823] Factor index cache for NodeLists and HTMLCollections to a class : [Attachment 216071] patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has granted Antti Koivisto <koivisto at iki.fi>'s
request for review:
Bug 123823: Factor index cache for NodeLists and HTMLCollections to a class
https://bugs.webkit.org/show_bug.cgi?id=123823

Attachment 216071: patch
https://bugs.webkit.org/attachment.cgi?id=216071&action=review

------- Additional Comments from Ryosuke Niwa <rniwa at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=216071&action=review


> Source/WebCore/dom/CollectionIndexCache.h:36
> +    unsigned size(const Collection&) const;

It's probably better call this length to match the terminology in
HTMLCollection & NodeList.

> Source/WebCore/dom/CollectionIndexCache.h:46
> +    mutable unsigned m_cachedSize : 31;
> +    mutable unsigned m_cachedSizeValid : 1;

Ditto.

> Source/WebCore/dom/CollectionIndexCache.h:47
> +    mutable unsigned m_cachedCurrentPosition;

We should probably call this m_cachedCurrentIndex for consistency.

> Source/WebCore/dom/CollectionIndexCache.h:82
> +    if (index < m_cachedCurrentPosition - index) {
> +	   // Start is closer to the target.

Instead of adding a comment, we can just define a bool as in:
const bool startIsCloserToTarget = index < m_cachedCurrentPosition - index;

> Source/WebCore/dom/CollectionIndexCache.h:104
> +    if (m_cachedSizeValid && m_cachedSize - index < index -
m_cachedCurrentPosition) {
> +	   // End is closer to the target.

Ditto.

> Source/WebCore/dom/CollectionIndexCache.h:129
> +    if (m_cachedSizeValid && index >= m_cachedSize)

We should assert that !m_cachedCurrentNode || index == m_cachedCurrentPosition.


> Source/WebCore/dom/CollectionIndexCache.h:138
> +    if (m_cachedSizeValid && m_cachedSize - index < index) {

Maybe we can define a boolean like:
const bool endIsCloserToTarget = m_cachedSizeValid && m_cachedSize - index <
index;
?