[webkit-reviews] review denied: [Bug 68610] Microdata: Basic implementation of document.getItems() method : [Attachment 110347] 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 68610: Microdata: Basic implementation of document.getItems() method
https://bugs.webkit.org/show_bug.cgi?id=68610

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

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


> Source/WebCore/dom/MicroDataItemList.cpp:58
> +    if (m_typeNames.contains("undefined") || !m_typeNames.size())

Are you trying to detect a case where the script didn't pass type? I don't
think comparing it to the string 'undefined' is the right approach.

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

Please use OVERRIDE macro.

> Source/WebCore/dom/NodeRareData.h:68
> +#if ENABLE(MICRODATA)
> +    typedef HashMap<String, MicroDataItemList*> MicroDataItemListCache;
> +    MicroDataItemListCache m_microDataItemListCache;
> +#endif

You also need to modify NodeListsNodeData::isEmpty.

> Source/WebCore/html/HTMLElement.cpp:201
> +#if ENABLE(MICRODATA)
> +    } else if (attr->name() == itemtypeAttr) {
> +	   itemTypeAttributeChanged();
> +#endif

Please modify invalidateCachesThatDependOnAttributes instead.
m_classNodeListCache is a good one to mimic. r- because of this.