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

[bugzilla-daemon at webkit.org]

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





--- Comment #21 from Arko Saha <nghq36 at motorola.com>  2012-02-28 00:56:38 PST ---
Thanks for the review.

(In reply to comment #20)
> (From update of attachment 129205 [details])
> 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);

Ok.

> > 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.
> 

Good point. I will optimize hasNamedItem() as you suggested.

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

Ok, will modify.

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

Ok.

> > 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.

Ok. I will use shouldBeTrue(...) in the 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.