[webkit-reviews] review granted: [Bug 89757] Microdata: document.getItems(typeNames) is not returning Microdata items when typeNames aurgument is not specified. : [Attachment 149062] Updated patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Arko Saha <arko at motorola.com>'s
request for review:
Bug 89757: Microdata: document.getItems(typeNames) is not returning Microdata
items when typeNames aurgument is not specified.
https://bugs.webkit.org/show_bug.cgi?id=89757

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

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=149062&action=review


> Source/WebCore/ChangeLog:9
> +	   With r120979 change, it creates MicroDataItemList with m_typeNames =
"http://webkit.org/microdata/undefinedItemType",
> +	   when typeNames argument is not specified. Modified the check in
nodeMatches() accordingly.

Change log should list the regression tests that are failing to make it clear
this is covered by tests.

> Source/WebCore/ChangeLog:16
> +	   (WebCore::undefinedItemType):
> +	   (WebCore):
> +	   (WebCore::MicroDataItemList::MicroDataItemList):
> +	   (WebCore::MicroDataItemList::~MicroDataItemList):
> +	   (WebCore::MicroDataItemList::nodeMatches):

Patches with per-function comments are better. Bogus lines like (WebCore):
should be deleted before landing a change.

> Source/WebCore/dom/MicroDataItemList.cpp:41
> +static String& undefinedItemType()

Return type should be const String& because we don’t want the caller to be able
to modify it.

> Source/WebCore/dom/MicroDataItemList.cpp:49
> -    , m_typeNames(typeNames, node()->document()->inQuirksMode())
> +    , m_typeNames(typeNames, document()->inQuirksMode())

Seems like unrelated cleanup.

> Source/WebCore/dom/MicroDataItemList.cpp:56
> -    String localTypeNames = m_originalTypeNames.isNull() ?
String("http://webkit.org/microdata/undefinedItemType") : m_originalTypeNames;
> -    m_node->nodeLists()->removeCacheWithName(this,
DynamicNodeList::MicroDataItemListType, localTypeNames);
> +    ownerNode()->nodeLists()->removeCacheWithName(this,
DynamicNodeList::MicroDataItemListType, m_originalTypeNames);

Using ownerNode() instead of m_node seems like unrelated cleanup.