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

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


> Source/WebCore/ChangeLog:10
> +	   No new tests. (OOPS!)

Clearly, we need a test for this feature.

> Source/WebCore/dom/ItemNodeList.h:37
> +class ItemNodeList : public DynamicNodeList {

I'm not convinced that ItemNodeList is a good name for this class. It sounds
too generic.

Now about MicroDataItemList?

> Source/WebCore/dom/Node.cpp:1137
> +void Node::removeCachedItemNodeList(ItemNodeList* list, const String&
typeName)

I'd like this function name to include microData somewhere.

> Source/WebCore/dom/Node.cpp:1733
> +PassRefPtr<NodeList> Node::getItems(const String& typeNames)

Why should this function live in Node instead of Document?

> Source/WebCore/html/HTMLElement.cpp:986
> +    dispatchSubtreeModifiedEvent();

I don't think we should be dispatching subtree modified here. As far as I
understand it, this function is called in parseMappedAttribute, which means
nothing in the DOM tree has changed. r- because of this.