Bug 71050

Created attachment 112731 [details] Work in progress

Created attachment 114902 [details] Patch

Comment on attachment 114902 [details] Patch 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

Those are all really minor comments.

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

> 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. IMHO, we should move them all to RareData. Almost no HTMLElements will need these fields and the pointers are just taking up memory on all HTMLElements.

(In reply to comment #6) > > 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. > > IMHO, we should move them all to RareData. Almost no HTMLElements will need these fields and the pointers are just taking up memory on all HTMLElements. Ok. I will move them all to RareData and upload the patch. Thanks.

Created attachment 115149 [details] Updated patch Incorporating review comments.

Created attachment 115575 [details] Updated patch Updated patch with some minor changes.

Comment on attachment 115575 [details] Updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=115575&action=review Below are a few minor nits. It would be nice to include more tests, but it's always nicer to include more tests. :) > Source/WebCore/dom/Node.h:86 > +class DOMSettableTokenList; This shouldn't be guarded by this macro because it's defined outside of ENABLE(MICRODATA) > Source/WebCore/dom/NodeRareData.h:29 > +#include "DOMSettableTokenList.h" Same here. > Source/WebCore/html/CollectionType.h:58 > + ItemProperties, // Microdata item properties in the document Bad indent > Source/WebCore/html/HTMLPropertiesCollection.cpp:33 > +#if ENABLE(MICRODATA) We usually have a blank line below this line. > Source/WebCore/html/HTMLPropertiesCollection.cpp:69 > + Vector<Node*> memory, pending; We usually don't use compound declarations. > Source/WebCore/html/HTMLPropertiesCollection.h:38 > +namespace WebCore { We usually have a blank line below this line.

(In reply to comment #10) > (From update of attachment 115575 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=115575&action=review > > Below are a few minor nits. It would be nice to include more tests, but it's always nicer to include more tests. :) Added more test cases. > > Source/WebCore/dom/Node.h:86 > > +class DOMSettableTokenList; > > This shouldn't be guarded by this macro because it's defined outside of ENABLE(MICRODATA) > > > Source/WebCore/dom/NodeRareData.h:29 > > +#include "DOMSettableTokenList.h" > > Same here. Done. > > Source/WebCore/html/CollectionType.h:58 > > + ItemProperties, // Microdata item properties in the document > > Bad indent Fixed. > > Source/WebCore/html/HTMLPropertiesCollection.cpp:33 > > +#if ENABLE(MICRODATA) > > We usually have a blank line below this line. Done. > > Source/WebCore/html/HTMLPropertiesCollection.cpp:69 > > + Vector<Node*> memory, pending; > > We usually don't use compound declarations. Ok. Modified accordingly. > > Source/WebCore/html/HTMLPropertiesCollection.h:38 > > +namespace WebCore { > > We usually have a blank line below this line. Done.

Created attachment 116353 [details] Updated patch

Comment on attachment 116353 [details] Updated patch Thanks.

Comment on attachment 116353 [details] Updated patch Rejecting attachment 116353 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 Last 500 characters of output: fast/dom/MicroData/properties-collection-must-see-the-properties-added-in-itemref-expected.txt patching file LayoutTests/fast/dom/MicroData/properties-collection-must-see-the-properties-added-in-itemref.html patching file LayoutTests/fast/dom/MicroData/properties-collection-test-expected.txt patching file LayoutTests/fast/dom/MicroData/properties-collection-test.html Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'Adam Barth', u'--force']" exit_code: 1 Full output: http://queues.webkit.org/results/10647024

Created attachment 116480 [details] Updated patch

Hi Adam Barth, Can you please r+ & cq+ this updated patch? Thanks.

Comment on attachment 116480 [details] Updated patch Clearing flags on attachment: 116480 Committed r101144: <http://trac.webkit.org/changeset/101144>

All reviewed patches have been landed. Closing bug.

HTMLPropertiesCollection is implemented very poorly :(

> HTMLPropertiesCollection is implemented very poorly :( Is there something specific we should change? Should we revert this patch and try again?

(In reply to comment #20) > > HTMLPropertiesCollection is implemented very poorly :( > > Is there something specific we should change? Should we revert this patch and try again? length, item, etc... are not using the cache at all. It's re-computing it every time.

(In reply to comment #21) > (In reply to comment #20) > > > HTMLPropertiesCollection is implemented very poorly :( > > > > Is there something specific we should change? Should we revert this patch and try again? > > length, item, etc... are not using the cache at all. It's re-computing it every time. Currently item, length does not use cache, its recomputing item, length every time. I have a thought of optimize the HTMLPropertiesCollection in a new patch. I will log a new bug and optimizing the HTMLPropertiesCollection. so that item, length will cache. Any other stuff you think needs improvement?

(In reply to comment #22) > Currently item, length does not use cache, its recomputing item, length every time. I have a thought of optimize the HTMLPropertiesCollection in a new patch. I will log a new bug and optimizing the HTMLPropertiesCollection. so that item, length will cache. Any other stuff you think needs improvement? Logged a new bug to implement caching mechanism for HTMLPropertiesCollection : https://bugs.webkit.org/show_bug.cgi?id=80490