Bug 129365 – Extract named items caches in HTMLCollection as a class

• Component: DOM
• Status: RESOLVED FIXED

[Ryosuke Niwa]

m_idCache and m_nameCache combo could be extracted as a class to make HTMLCollection interface more comprehensible.

[Ryosuke Niwa]

Created attachment 225227 [details]
Cleanup

[Ryosuke Niwa]

Created attachment 225234 [details]
Cleanup

[Antti Koivisto]

Comment on attachment 225234 [details]
Cleanup

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

r=me though it would be nice to improve naming consistency and factor more stuff to the class.

> Source/WebCore/ChangeLog:15
> +        Also removed m_isItemRefElementsCacheValid since it was only used by Microdata API removed in r153772.

Hah

> Source/WebCore/html/HTMLAllCollection.cpp:61
> -    if (Vector<Element*>* cache = idCache(name)) {
> +    if (const Vector<Element*>* cache = caches.idCache(name)) {
>          if (index < cache->size())
>              return cache->at(index);
>          index -= cache->size();
>      }
>  
> -    if (Vector<Element*>* cache = nameCache(name)) {
> +    if (const Vector<Element*>* cache = caches.nameCache(name)) {
>          if (index < cache->size())
>              return cache->at(index);
>      }

It would be nice to factor this kind of stuff to the cache class.

> Source/WebCore/html/HTMLCollection.cpp:413
>      updateNameCache();
> +    ASSERT(m_namedItemCaches);

Match the function and variable names (updateNamedElementCache()/m_namedElementCache or similar)

> Source/WebCore/html/HTMLCollection.cpp:423
> -    if (Vector<Element*>* idResults = idCache(name)) {
> +    if (const Vector<Element*>* idResults = m_namedItemCaches->idCache(name)) {
>          if (idResults->size())
>              return idResults->at(0);
>      }
>  
> -    if (Vector<Element*>* nameResults = nameCache(name)) {
> +    if (const Vector<Element*>* nameResults = m_namedItemCaches->nameCache(name)) {
>          if (nameResults->size())
>              return nameResults->at(0);
>      }

and this

> Source/WebCore/html/HTMLCollection.h:41
> +class CollectionNamedItemCaches {

"Item" adds nothing. How about something like CollectionNamedElementCache?

> Source/WebCore/html/HTMLCollection.h:44
> +    const Vector<Element*>* idCache(const AtomicString& id) const { return find(m_idCache, id); }
> +    const Vector<Element*>* nameCache(const AtomicString& name) const { return find(m_nameCache, name); }

findElementsWithID/Name?

> Source/WebCore/html/HTMLCollection.h:64
> +    NodeCacheMap m_idCache;
> +    NodeCacheMap m_nameCache;

Maybe the whole class is the "cache" and these can be something like m_idAttributeMap, m_nameAttributeMap

> Source/WebCore/html/HTMLCollection.h:102
> +    bool hasIdNameCache() const { return !!m_namedItemCaches; }

Please match the variable name.

> Source/WebCore/html/HTMLCollection.h:125
> +    const CollectionNamedItemCaches& namedItemCachesAssertingExistence() const
> +    {

Drop the AssertingExistence part. It is not important.

> Source/WebCore/html/HTMLCollection.h:137
> +    void invalidateIdNameCacheMaps() const;

Match with other naming.

[Ryosuke Niwa]

Committed r164772: <http://trac.webkit.org/changeset/164772>

[Ryosuke Niwa]

(In reply to comment #4)
> Committed r164772: <http://trac.webkit.org/changeset/164772>

Removed the superfluous change log entry in http://trac.webkit.org/changeset/164773.