[webkit-reviews] review granted: [Bug 129365] Extract named items caches in HTMLCollection as a class : [Attachment 225234] Cleanup

[bugzilla-daemon at webkit.org]

Antti Koivisto <koivisto at iki.fi> has granted Ryosuke Niwa <rniwa at webkit.org>'s
request for review:
Bug 129365: Extract named items caches in HTMLCollection as a class
https://bugs.webkit.org/show_bug.cgi?id=129365

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

------- Additional Comments from Antti Koivisto <koivisto at iki.fi>
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.