[webkit-reviews] review denied: [Bug 80490] [Microdata] Implement cache mechanism for HTMLPropertiesCollection. : [Attachment 131853] Updated patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has denied Arko Saha <nghq36 at motorola.com>'s
request for review:
Bug 80490: [Microdata] Implement cache mechanism for HTMLPropertiesCollection.
https://bugs.webkit.org/show_bug.cgi?id=80490

Attachment 131853: Updated patch
https://bugs.webkit.org/attachment.cgi?id=131853&action=review

------- Additional Comments from Ryosuke Niwa <rniwa at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=131853&action=review


r- because you probably want to address some of my comments.

> Source/WebCore/html/HTMLPropertiesCollection.cpp:67
> +static Node* nextNodeOrSibling(Node* base, Node* node)

Could you give this function more descriptive name? It does certainly traverse
next node or sibling but it doesn't explain why it does.

> Source/WebCore/html/HTMLPropertiesCollection.cpp:69
> +    return (node == base) || (node->isHTMLElement() &&
!toHTMLElement(node)->fastHasAttribute(itemscopeAttr))

No need for parenthesis around node == base.

> Source/WebCore/html/HTMLPropertiesCollection.cpp:79
> +    Node* current;
> +    if (!previous)
> +	   current = base;
> +    else
> +	   current = nextNodeOrSibling(base, previous);

I think you should use the ternary operator here.

> Source/WebCore/html/HTMLPropertiesCollection.cpp:84
> +	   if (!current->isHTMLElement())
> +	       continue;
> +	   Element* element = toHTMLElement(current);

It seems odd to check if current is a HTML element then store it to Element*.

> Source/WebCore/html/HTMLPropertiesCollection.cpp:98
> +    if (!base()->isHTMLElement() ||
!toHTMLElement(base())->fastHasAttribute(itemscopeAttr))

Can base() ever return non-Element node? Maybe we want to change the type there
so that we don't have to check whether it's not an element or not.

> Source/WebCore/html/HTMLPropertiesCollection.cpp:103
> +    m_cache.itemRefElements.append(baseElement);
> +    m_cache.hasItemRefElements = true;

We probably want to add a bunch of helper functions in m_cache so that we don't
have to update these member variables manually like this.

> Source/WebCore/html/HTMLPropertiesCollection.cpp:125
> +    for (unsigned i = 0; i < m_cache.itemRefElements.size(); ++i)
> +	   for (Element* element = itemAfter(m_cache.itemRefElements[i], 0);
element; element = itemAfter(m_cache.itemRefElements[i], element))
> +	       ++length;

You need curly brackets for the outer for loop.

> Source/WebCore/html/HTMLPropertiesCollection.cpp:158
> +	   for (unsigned i = 0; i < m_cache.itemRefElements.size(); ++i)

You need curly brackets for this for loop since the statement is more than one
line.

> Source/WebCore/html/HTMLPropertiesCollection.cpp:159
> +	       if ((m_cache.current = itemAfter(m_cache.itemRefElements[i],
0))) {

No extra parenthesis needed.

> Source/WebCore/html/HTMLPropertiesCollection.cpp:165
> +		   m_cache.itemRefElementPosition = i;
> +		   break;
> +	       }
> +	   m_cache.position = 0;
> +	   if (!m_cache.current)
> +	       return 0;

It seems like this entire block is redundant and can be merged into the for
loop below.

> Source/WebCore/html/HTMLPropertiesCollection.cpp:168
> +    Element* e = m_cache.current;

Please don't use one-letter variable name like e.

> Source/WebCore/html/HTMLPropertiesCollection.cpp:174
> +	       e = itemAfter(m_cache.itemRefElements[i], e);

We need to initialize e before each inner loop here (i.e. before for (unsigned
pos = currentPosition;...), right?

> Source/WebCore/html/HTMLPropertiesCollection.cpp:179
> +	       if (e)
> +		   currentPosition++;
> +	       else
> +		   break;

It's probably better to bail out early as in:
if (!e)
    break;
currentPosition++;

> Source/WebCore/html/HTMLPropertiesCollection.cpp:204
> +	       if (m_cache.propertyNames->isEmpty() ||
!m_cache.propertyNames->contains(propertyName))

I think you can just check !m_cache.propertyNames->contains(propertyName). If
m_cache.propertyNames->isEmpty(), then it certainly doesn't contain anything.

> Source/WebCore/html/HTMLPropertiesCollection.cpp:233
> +    invalidateCacheIfNeeded();
> +    updateNameCache();

It seems like updateNameCache can call invalidateCacheIfNeeded.