[webkit-reviews] review granted: [Bug 22699] Enable NodeList caching for getElementsByTagName : [Attachment 26261] Sigh, forgot to save a file before diffing.

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted David Smith <catfish.man at gmail.com>'s
request for review:
Bug 22699: Enable NodeList caching for getElementsByTagName
https://bugs.webkit.org/show_bug.cgi?id=22699

Attachment 26261: Sigh, forgot to save a file before diffing.
https://bugs.webkit.org/attachment.cgi?id=26261&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
> +	   * dom/QualifiedName.h: Move QNameHash to the header and make it work
on QualifiedNames
> +	   (WebCore::hashComponents):
> +	   (WebCore::QualifiedNameHash::hash):
> +	   (WebCore::QualifiedNameHash::equal):

If you're not going to comment on the individual functions in the .h file,
please just remove the function names. Either way is OK with me. In cpp files
it's probably less good to remove the function names, even if you aren't
commenting on each one.

> +	   (WTF::):

Please don't leave bogus lines like this one in the change log.

> +    pair<NodeListsNodeData::TagCacheMap::iterator, bool> result =
data->nodeLists()->m_tagNodeListCaches.add(QualifiedName(nullAtom, name,
namespaceURI), 0);
> +    if (result.second)
> +	   result.first->second = new DynamicNodeList::Caches;
> +    
> +    return TagNodeList::create(this, namespaceURI.isEmpty() ? nullAtom :
AtomicString(namespaceURI), name, result.first->second);

It's inefficient to convert namespaceURI to an AtomicString twice here. It
should be made into an AtomicString only once and reused. The best way to do
this is to change the argument into an AtomicString in the first place in
Node.h. This should be done for isDefaultNamespace, lookupPrefix,
lookupNamespacePrefix, and getElementsByTagNameNS, since all those functions
just end up making an AtomicString a moment later. Maybe in a separate patch,
though.

> +    static unsigned hash(const QualifiedName& name) {
> +	   return hash(name.impl());
> +    }

Either the function definition should to be all on one line, or use our normal
brace style (function start braces go on their own line).

> +    static unsigned hash(const QualifiedName::QualifiedNameImpl* name) {    

> +	   QualifiedNameComponents c = { name->m_prefix.impl(),
name->m_localName.impl(), name->m_namespace.impl() };
> +	   return hashComponents(c);
> +    }

Same comment here.

> +    static bool equal(const QualifiedName& a, const QualifiedName& b) {
return a == b; }
> +    
> +    static bool equal(const QualifiedName::QualifiedNameImpl* a, const
QualifiedName::QualifiedNameImpl* b) { return a == b; }

I think these would read better if they didn't have a space between them.

r=me