Bug 129365

Summary: Extract named items caches in HTMLCollection as a class
Product: WebKit Reporter: Ryosuke Niwa <rniwa>
Component: DOMAssignee: Ryosuke Niwa <rniwa>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, kling, koivisto
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 129361    
Bug Blocks:    
Attachments:
Description Flags
Cleanup
none
Cleanup koivisto: review+

Description Ryosuke Niwa 2014-02-25 23:13:51 PST
m_idCache and m_nameCache combo could be extracted as a class to make HTMLCollection interface more comprehensible.
Comment 1 Ryosuke Niwa 2014-02-25 23:32:27 PST
Created attachment 225227 [details]
Cleanup
Comment 2 Ryosuke Niwa 2014-02-26 00:47:13 PST
Created attachment 225234 [details]
Cleanup
Comment 3 Antti Koivisto 2014-02-26 14:39:45 PST
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.
Comment 4 Ryosuke Niwa 2014-02-26 19:30:00 PST
Committed r164772: <http://trac.webkit.org/changeset/164772>
Comment 5 Ryosuke Niwa 2014-02-26 19:32:32 PST
(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.