Bug 71050 - [Microdata] Support for properties attribute.
Summary: [Microdata] Support for properties attribute.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 68609
  Show dependency treegraph
 
Reported: 2011-10-27 12:18 PDT by Arko Saha
Modified: 2012-03-06 22:16 PST (History)
7 users (show)

See Also:


Attachments
Work in progress (14.96 KB, patch)
2011-10-27 12:32 PDT, Arko Saha
no flags Details | Formatted Diff | Diff
Patch (48.46 KB, patch)
2011-11-14 01:53 PST, Arko Saha
abarth: review-
Details | Formatted Diff | Diff
Updated patch (55.52 KB, patch)
2011-11-15 05:34 PST, Arko Saha
no flags Details | Formatted Diff | Diff
Updated patch (53.37 KB, patch)
2011-11-17 06:10 PST, Arko Saha
abarth: review+
abarth: commit-queue-
Details | Formatted Diff | Diff
Updated patch (68.69 KB, patch)
2011-11-23 07:13 PST, Arko Saha
abarth: review+
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Updated patch (68.71 KB, patch)
2011-11-23 22:48 PST, Arko Saha
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Arko Saha 2011-10-27 12:18:13 PDT
1. Implement HTMLPropertiesCollection : A collection of elements that add name-value pairs to a particular item in the microdata model.
2. Support for HMTL5 Microdata properties attribute. The properties attribute returns an HTMLPropertiesCollection object with all the element's properties. Otherwise, an empty HTMLPropertiesCollection object.

Here is the spec: http://www.whatwg.org/specs/web-apps/current-work/multipage/microdata.html#dom-properties
Comment 1 Arko Saha 2011-10-27 12:32:02 PDT
Created attachment 112731 [details]
Work in progress
Comment 2 Arko Saha 2011-11-14 01:53:32 PST
Created attachment 114902 [details]
Patch
Comment 3 Adam Barth 2011-11-14 08:39:20 PST
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
Comment 4 Adam Barth 2011-11-14 08:39:40 PST
Those are all really minor comments.
Comment 5 Arko Saha 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.
Comment 6 Adam Barth 2011-11-14 22:59:28 PST
> 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.
Comment 7 Arko Saha 2011-11-14 23:14:33 PST
(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.
Comment 8 Arko Saha 2011-11-15 05:34:06 PST
Created attachment 115149 [details]
Updated patch

Incorporating review comments.
Comment 9 Arko Saha 2011-11-17 06:10:23 PST
Created attachment 115575 [details]
Updated patch

Updated patch with some minor changes.
Comment 10 Adam Barth 2011-11-23 01:03:32 PST
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.
Comment 11 Arko Saha 2011-11-23 07:12:24 PST
(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.
Comment 12 Arko Saha 2011-11-23 07:13:35 PST
Created attachment 116353 [details]
Updated patch
Comment 13 Adam Barth 2011-11-23 14:34:23 PST
Comment on attachment 116353 [details]
Updated patch

Thanks.
Comment 14 WebKit Review Bot 2011-11-23 14:51:11 PST
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
Comment 15 Arko Saha 2011-11-23 22:48:46 PST
Created attachment 116480 [details]
Updated patch
Comment 16 Arko Saha 2011-11-24 05:04:20 PST
Hi Adam Barth, Can you please r+ & cq+ this updated patch? Thanks.
Comment 17 WebKit Review Bot 2011-11-24 12:03:17 PST
Comment on attachment 116480 [details]
Updated patch

Clearing flags on attachment: 116480

Committed r101144: <http://trac.webkit.org/changeset/101144>
Comment 18 WebKit Review Bot 2011-11-24 12:03:23 PST
All reviewed patches have been landed.  Closing bug.
Comment 19 Ryosuke Niwa 2012-03-06 12:32:17 PST
HTMLPropertiesCollection is implemented very poorly :(
Comment 20 Adam Barth 2012-03-06 13:36:25 PST
> HTMLPropertiesCollection is implemented very poorly :(

Is there something specific we should change?  Should we revert this patch and try again?
Comment 21 Ryosuke Niwa 2012-03-06 13:37:51 PST
(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.
Comment 22 Arko Saha 2012-03-06 22:08:56 PST
(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?
Comment 23 Arko Saha 2012-03-06 22:16:20 PST
(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