[webkit-reviews] review denied: [Bug 80269] [Microdata] Implement PropertyNodeList interface. : [Attachment 144807] Updated patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has denied Arko Saha <arko at motorola.com>'s
request for review:
Bug 80269: [Microdata] Implement PropertyNodeList interface.
https://bugs.webkit.org/show_bug.cgi?id=80269

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

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


> Source/WebCore/dom/Node.h:644
> +    PassRefPtr<PropertyNodeList> propertyNodeList(const Vector<Element*>&,
const String&);
> +    void removeCachedPropertyNodeList(PropertyNodeList*, const String&);

You need to update invalidateNodeListsCacheAfterAttributeChanged and
invalidateCachesThatDependOnAttributes. r- because of this.
Also, please add a test case (i.e. modify element attributes and add/remove
elements dynamically that demonstrates that the current patch doesn't update
property node list properly).

> Source/WebCore/dom/PropertyNodeList.cpp:46
> +    : DynamicSubtreeNodeList(itemElement->fastHasAttribute(itemrefAttr) ?
static_cast<Node*>(itemElement->document()) : itemElement)

Here's no point in casing document() to Node* since Document is a Node.

> Source/WebCore/dom/PropertyNodeList.cpp:63
> +bool PropertyNodeList::isValidAncestor(const Node* testElement, const Node
*other) const
> +{
> +    // Return true if other is an ancestor of testElement and any ancestor
of testElement
> +    // doesn't have an itemscope attribute, otherwise false

Instead of adding a comment like this, we should give a descriptive name to the
function.
How about elementIsPropertyOfRefElement(const Node* testElement, const Node*
refElement)?
Nit: other isn't a great variable name here.

> Source/WebCore/dom/PropertyNodeList.cpp:69
> +	       // Don't return false if node is the microdata top level item.
Microdata top level item
> +	       // always have itemscope attribute specified.
> +	       if (element->fastHasAttribute(itemscopeAttr) && node !=
m_itemElement)

It's better to define an inline helper function whose name reflect this
comment:
bool ALWAYS_INLINE PropertyNodeList::isTopLevelItem(Node* node)
{
    return node == m_itemElement;
}
Or maybe
bool ALWAYS_INLINE PropertyNodeList::elementDefinesDifferentItem()
{
    return element->fastHasAttribute(itemscopeAttr) && node != m_itemElement;
}

> Source/WebCore/dom/PropertyNodeList.cpp:88
> +	       validAncestor = true;
> +	       break;

I would move the return statement here.

> Source/WebCore/dom/PropertyNodeList.cpp:92
> +    return validAncestor &&
testElement->itemProp()->tokens().contains(m_name);

And just return false here. You don't need validAncestor then.

> Source/WebCore/dom/PropertyNodeList.cpp:101
> +	   Node* node = item(i);
> +	   HTMLElement* element = toHTMLElement(node);
> +	   propertyValue.append(element->itemValue());

I would combine these 3 lines into one:
propertyValue.append(toElement(item(i))->itemValue());

> Source/WebCore/dom/PropertyNodeList.h:59
> +    virtual bool nodeMatches(Element*) const;

Please use OVERRIDE macro.

> Source/WebCore/html/MicroDataItemValue.idl:36
> +    interface [
> +	   Conditional=MICRODATA,
> +	   CustomToJSObject
> +    ] MicroDataItemValue {
> +    };

Clever! But we should probably not expose the interface object;
OmitConstructor?

>
LayoutTests/fast/dom/MicroData/propertynodelist-getvalues-array-values-obtained
-from-itemvalue-of-each-element.html:51
> +    shouldBe("valuesArray[i]", "propertyNodeList[i].itemValue");

You should also check that the interface name of each item in the list is not
MicroDataItemValue.