[Webkit-unassigned] [Bug 80490] [Microdata] Implement cache mechanism for HTMLPropertiesCollection.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=80490


Ryosuke Niwa <rniwa at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #131853|review?                     |review-
               Flag|                            |




--- Comment #10 from Ryosuke Niwa <rniwa at webkit.org>  2012-03-21 23:31:27 PST ---
(From update of attachment 131853)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.