[webkit-reviews] review denied: [Bug 33696] let's cache nodelists instead of dynamicnodelist::cache : [Attachment 54443] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied anton muhin <antonm at chromium.org>'s
request for review:
Bug 33696: let's cache nodelists instead of dynamicnodelist::cache
https://bugs.webkit.org/show_bug.cgi?id=33696

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

------- Additional Comments from Darin Adler <darin at apple.com>
> +    , m_classNamesOriginal(classNames)

Instead of adding this and making every ClassNodeList bigger, you should add an
originalString() function to the SpaceSplitString class, which already keeps a
copy of the original string. (If you kept this, then you'd want to give it a
better name.)

> +    ASSERT(list->hasOwnCaches());
> +    UNUSED_PARAM(list);

The ASSERT_UNUSED macro should be used in cases like this.

> +    ASSERT(list == data->m_classNodeListCache.get(className));

The ASSERT_UNUSED macro should be used in cases like this.

> +    ASSERT(list->hasOwnCaches());
> +    UNUSED_PARAM(list);

The ASSERT_UNUSED macro should be used in cases like this.

> +    ASSERT(list == data->m_nameNodeListCache.get(nodeName));

The ASSERT_UNUSED macro should be used in cases like this.

> +    ASSERT(list->hasOwnCaches());
> +    UNUSED_PARAM(list);

The ASSERT_UNUSED macro should be used in cases like this.

> +    ASSERT(list == data->m_tagNodeListCache.get(name));

The ASSERT_UNUSED macro should be used in cases like this.

> +    PassRefPtr<TagNodeList> list = TagNodeList::create(this,
namespaceURI.isEmpty() ? nullAtom : namespaceURI, localNameAtom);

Local variables should have a type of RefPtr, not PassRefPtr. See
<http://webkit.org/coding/RefPtr.html>.

> +    PassRefPtr<NameNodeList> list = NameNodeList::create(this, elementName);


Local variables should have a type of RefPtr, not PassRefPtr. See
<http://webkit.org/coding/RefPtr.html>.

> +    m_rootNode->removeCachedTagNodeList(this, QualifiedName(nullAtom,
m_localName, m_namespaceURI));

We might want to store this as a QualifiedName in the first place.