[Webkit-unassigned] [Bug 73156] [Microdata] Implement HTMLPropertiesCollection collection.namedItem()

[bugzilla-daemon at webkit.org]

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


Kentaro Hara <haraken at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #129205|commit-queue?               |commit-queue-
               Flag|                            |




--- Comment #20 from Kentaro Hara <haraken at chromium.org>  2012-02-28 00:45:09 PST ---
(From update of attachment 129205)
View in context: https://bugs.webkit.org/attachment.cgi?id=129205&action=review

Almost looks good to me. Thanks for the patch.

> Source/WebCore/html/HTMLPropertiesCollection.cpp:202
> +    return !namedItems.isEmpty() ? StaticNodeList::adopt(namedItems) : 0;

Nit: return namedItem.isEmpty() ? 0 : StaticNodeList::adopt(namedItems);

> Source/WebCore/html/HTMLPropertiesCollection.cpp:208
> +    getNamedItems(namedItems, name);

Just an out-of-curious question: Don't you have any concern about the performance of hasNamedItem()? It is a bit wasteful to completely get all named items just to know whether there is at least one item. If you have any concern, maybe you can optimize it (e.g. std::sort would not be necessary). If you do not have the concern, the current patch looks good to me.

> Source/WebCore/html/HTMLPropertiesCollection.cpp:210
> +    return !namedItems.isEmpty() ? true : false;

Nit: return namedItems.isEmpty()? false : true;

> Source/WebCore/html/HTMLPropertiesCollection.h:54
> +    bool hasNamedItem(const AtomicString& name) const;

Nit: 'name' is not necessary.

> LayoutTests/fast/dom/MicroData/nameditem-must-be-case-sensitive.html:13
> +shouldBeTrue("element.properties.namedItem('foo').length == '2'");

shouldBe("element.properties.namedItem('foo').length", '2') might be better. It's up to you, but if you want to use shouldBeTrue(...), please use === instead of ==. The semantics of JavaScript == is really chaos.

The same comment for other test cases.

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