[webkit-reviews] review denied: [Bug 71050] [Microdata] Support for properties attribute. : [Attachment 114902] Patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has denied Arko Saha <nghq36 at motorola.com>'s
request for review:
Bug 71050: [Microdata] Support for properties attribute.
https://bugs.webkit.org/show_bug.cgi?id=71050

Attachment 114902: Patch
https://bugs.webkit.org/attachment.cgi?id=114902&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=114902&action=review


> Source/WebCore/html/CollectionType.h:58
> +	   ItemProperties, // Microdata item properties in the document

Please match the above indent.

> Source/WebCore/html/HTMLElement.h:144
>      mutable RefPtr<DOMSettableTokenList> m_itemProp;
>      mutable RefPtr<DOMSettableTokenList> m_itemRef;
>      mutable RefPtr<DOMSettableTokenList> m_itemType;
> +    mutable RefPtr<HTMLPropertiesCollection> m_properties;

How do all these members impact memory usage?  Should we put them in a RareData
member?

> Source/WebCore/html/HTMLPropertiesCollection.cpp:74
> +	       pending.append(child);

Bad indent.

> Source/WebCore/html/HTMLPropertiesCollection.cpp:82
> +	   for (unsigned itrRef = 0; itrRef < itemRef->length(); ++itrRef) {

itrRef => please use complete words in variable names.	Also, should this be a
size_t?

> Source/WebCore/html/HTMLPropertiesCollection.cpp:83
> +	       AtomicString refId = itemRef->item(itrRef);

refId => please uses complete words in variable names.

> Source/WebCore/html/HTMLPropertiesCollection.cpp:85
> +	       if (Document* document = root->document()) {

Can this really be null?

> Source/WebCore/html/HTMLPropertiesCollection.cpp:119
> +

Extra blank line.

> Source/WebCore/html/HTMLPropertiesCollection.cpp:123
> +unsigned HTMLPropertiesCollection::calcLength() const

calcLength => please use complete works in function names.

> Source/WebCore/html/HTMLPropertiesCollection.cpp:160
> +    for (unsigned i = 0; i < m_properties.size(); ++i) {

unsigned => size_t

> Source/WebCore/html/HTMLPropertiesCollection.cpp:163
> +	   DOMSettableTokenList* itemProp =
toHTMLElement(m_properties[i])->itemProp().get();

itemProp => please use complete words

> Source/WebCore/html/HTMLPropertiesCollection.h:49
> +private:

Blank line before visibility specifiers.

> Source/WebCore/html/HTMLPropertiesCollection.idl:42
> +	   // TODO: override inherited namedItem()

TODO => FIXME