[Webkit-unassigned] [Bug 71050] [Microdata] Support for properties attribute.

[bugzilla-daemon at webkit.org]

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





--- Comment #5 from Arko Saha <nghq36 at motorola.com>  2011-11-14 22:17:52 PST ---
(In reply to comment #3)
> (From update of attachment 114902 [details])
> 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?

m_itemProp, m_itemRef, m_itemType are just HTMLElement(HTMLElement.idl) attributes related to microdata. We set the value of these members from HTMLElement::parseMappedAttribute() when attribute changed accordingly. So I think there is no need to cache.
Currently we don't cache properties collection i.e, m_properties. I thought of adding m_properties to RareData but the list of places where I should invalidate property collection cache was not clear to me so thought of handling it separately, may be in a seperate bug. I think we need to invalidate the cache when any attribute changes and when contents are appended/inserted/removed.

Please let me know your thoughts on the same.

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

Will correct.

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

Will do.

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

Ok, will modify.

> > Source/WebCore/html/HTMLPropertiesCollection.cpp:119
> > +
> 
> Extra blank line.

Will correct.

> > Source/WebCore/html/HTMLPropertiesCollection.cpp:123
> > +unsigned HTMLPropertiesCollection::calcLength() const
> 
> calcLength => please use complete works in function names.

Will do.

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

Will do.

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

Will modify.

> > Source/WebCore/html/HTMLPropertiesCollection.h:49
> > +private:
> 
> Blank line before visibility specifiers.
> 
> > Source/WebCore/html/HTMLPropertiesCollection.idl:42
> > +        // TODO: override inherited namedItem()
> 
> TODO => FIXME
Will change.

Thanks for the review comments.

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