[Webkit-unassigned] [Bug 68610] Microdata: Basic implementation of document.getItems() method

[bugzilla-daemon at webkit.org]

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


Ryosuke Niwa <rniwa at webkit.org> changed:

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




--- Comment #4 from Ryosuke Niwa <rniwa at webkit.org>  2011-09-26 11:17:35 PST ---
(From update of attachment 108457)
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.

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