Bug 80269 – [Microdata] Implement PropertyNodeList interface.

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Arko Saha]

1. Implement PropertyNodeList interface. HTMLPropertiesCollection.namedItem(name) must return PropertyNodeList object.
2. Implement propertyNodeList.getValues() method. Returns an array of various values. Its values are obtained from the "itemValue" DOM property of each of the elements represented by the object, in tree order.

Specfication : http://www.whatwg.org/specs/web-apps/current-work/multipage/common-dom-interfaces.html#propertynodelist

[Arko Saha]

Created attachment 130101 [details]
Work in progress.

[Arko Saha]

Created attachment 130364 [details]
Patch

[Adam Barth]

Comment on attachment 130364 [details]
Patch

Can we implement this without custom bindings?

[Adam Barth]

It seems like whenever someone needs to do something with arrays, they resort to custom bindings.  We should be able to improve the code generator so that it can handle these array cases.

[Arko Saha]

After r111416 <http://trac.webkit.org/changeset/111416> revision I was trying to remove the custom binding for [Custom] Array getValues() and using sequence<MicroDataItemValue> getValues(). It works for JS bindings but for Gobject bindings it is trying to include #include "sequence<MicroDataItemValue>.h" in WebKitDOMPropertyNodeList.cpp.

[Kentaro Hara]

(In reply to comment #5)
> After r111416 <http://trac.webkit.org/changeset/111416> revision I was trying to remove the custom binding for [Custom] Array getValues() and using sequence<MicroDataItemValue> getValues(). It works for JS bindings but for Gobject bindings it is trying to include #include "sequence<MicroDataItemValue>.h" in WebKitDOMPropertyNodeList.cpp.

The issue was already fixed at r111535, thanks to Vineet. Sorry for the trouble.

[Arko Saha]

(In reply to comment #6)
> 
> The issue was already fixed at r111535, thanks to Vineet. Sorry for the trouble.

Thanks Vineet for the quick fix.

[Arko Saha]

Created attachment 133059 [details]
Updated patch

[Adam Barth]

Comment on attachment 133059 [details]
Updated patch

View in context: https://bugs.webkit.org/attachment.cgi?id=133059&action=review

> Source/WebCore/bindings/js/JSMicroDataItemValueCustom.cpp:49
> +    return jsString(exec, itemValue->getString());

Why does this return a string?  We have a MicroDataItemValue.idl in this file.  I would have expected it to return a MicroDataItemValue...

> Source/WebCore/bindings/v8/custom/V8MicroDataItemValueCustom.cpp:48
> +    return v8String(itemValue->getString());

Same question here.

> Source/WebCore/dom/PropertyNodeList.cpp:48
> +        m_propertyValue.append(element->itemValue());

Is this going to keep appending to m_propertyValue every time this function is called?  That doesn't seem right.

> Source/WebCore/dom/PropertyNodeList.cpp:66
> +Node* PropertyNodeList::itemWithName(const AtomicString& id) const

itemWithName not itemWithID ?

> Source/WebCore/dom/PropertyNodeList.h:52
> +    PropertyValueArray getValues();

Is this going to thrash the RefCounts of the MicroDataItemValue?

> Source/WebCore/dom/PropertyNodeList.h:61
> +    PropertyNodeList(Vector<RefPtr<Node> >& nodes)

Please add the explicit keyword.

[Arko Saha]

(In reply to comment #9)
> > Source/WebCore/bindings/js/JSMicroDataItemValueCustom.cpp:49
> > +    return jsString(exec, itemValue->getString());
> 
> Why does this return a string?  We have a MicroDataItemValue.idl in this file.  I would have expected it to return a MicroDataItemValue...

According to the spec http://www.whatwg.org/specs/web-apps/current-work/multipage/common-dom-interfaces.html#propertynodelist : getValues() method object must return a newly constructed array whose values are the values obtained from the itemValue DOM property. itemValue of a property depends on the element. Please check http://www.whatwg.org/specs/web-apps/current-work/multipage/microdata.html#dom-itemvalue.

For an example:

<div itemscope>
<a itemprop="foo" href="webkit.org"></a>
<p itemprop="foo" itemscope><span itemprop="quz"></p>
</div>

itemValue will return node itself if it has an itemscope attribute specified.
If the element is an <a> element then it will return src content attribute string.

<div> defines the microdata item with two properties <a> and <p>
Here itemValue of the first property is string "webkit.org" and second property is node <p> as it has itemscope attribute.
getValues() will return an array which contains a string and a node i.e, 'webkit.org' and '<p>'.

> > Source/WebCore/dom/PropertyNodeList.cpp:48
> > +        m_propertyValue.append(element->itemValue());
> 
> Is this going to keep appending to m_propertyValue every time this function is called?  That doesn't seem right.

Ok, we will cache m_propertyValue. so that it does not append to m_propertyValue every time.

> > Source/WebCore/dom/PropertyNodeList.cpp:66
> > +Node* PropertyNodeList::itemWithName(const AtomicString& id) const
> 
> itemWithName not itemWithID ?

Ok will modify.
 
> > Source/WebCore/dom/PropertyNodeList.h:52
> > +    PropertyValueArray getValues();
> 
> Is this going to thrash the RefCounts of the MicroDataItemValue?

I could not get this. Can you please explain a bit.

> > Source/WebCore/dom/PropertyNodeList.h:61
> > +    PropertyNodeList(Vector<RefPtr<Node> >& nodes)
> 
> Please add the explicit keyword.

Ok.

[Ojan Vafai]

FYI, I'm lobbying to kill the getValues method on PropertyNodeList: http://lists.whatwg.org/htdig.cgi/whatwg-whatwg.org/2012-March/035280.html. My preference would be to leave it out of this patch until that discussion is resolved (i.e. only do step 1 from comment 0), but I wouldn't be too upset if we left it in and removed it later.

[Ojan Vafai]

(In reply to comment #11)
> FYI, I'm lobbying to kill the getValues method on PropertyNodeList: http://lists.whatwg.org/htdig.cgi/whatwg-whatwg.org/2012-March/035280.html. My preference would be to leave it out of this patch until that discussion is resolved (i.e. only do step 1 from comment 0), but I wouldn't be too upset if we left it in and removed it later.

Nevermind. I was wrong.

[Arko Saha]

(In reply to comment #9)
> > Source/WebCore/dom/PropertyNodeList.cpp:48
> > +        m_propertyValue.append(element->itemValue());
> 
> Is this going to keep appending to m_propertyValue every time this function is called?  That doesn't seem right.

Done!
 
> > Source/WebCore/dom/PropertyNodeList.cpp:66
> > +Node* PropertyNodeList::itemWithName(const AtomicString& id) const
> 
> itemWithName not itemWithID ?

I checked the existing implementation of itemWithID in StaticNodeList and DynamicNodeList. Both used to return the node with the given id from the NodeList. 
 
> > Source/WebCore/dom/PropertyNodeList.h:52
> > +    PropertyValueArray getValues();
> 
> Is this going to thrash the RefCounts of the MicroDataItemValue?

I am not clear about this. WebKitMutationObserver and Console follow the same convension. So I have implemented in the same way.

> > Source/WebCore/dom/PropertyNodeList.h:61
> > +    PropertyNodeList(Vector<RefPtr<Node> >& nodes)
> 
> Please add the explicit keyword.

Done!

[Arko Saha]

Created attachment 137769 [details]
Updated patch

Incorporated review comments..

[Ryosuke Niwa]

Comment on attachment 137769 [details]
Updated patch

View in context: https://bugs.webkit.org/attachment.cgi?id=137769&action=review

> Source/WebCore/bindings/js/JSNodeListCustom.cpp:75
> +JSValue toJS(ExecState* exec, JSDOMGlobalObject* globalObject, NodeList* nodeList)
> +{
> +    if (!nodeList)
> +        return jsNull();
> +
> +#if ENABLE(MICRODATA)
> +    if (nodeList->isPropertyNodeList())
> +        return CREATE_DOM_WRAPPER(exec, globalObject, PropertyNodeList, nodeList);
> +#endif
> +
> +    return CREATE_DOM_WRAPPER(exec, globalObject, NodeList, nodeList);
> +}

I'm surprised that we need this code. Doesn't binding code automatically do this for you?

> Source/WebCore/dom/PropertyNodeList.cpp:50
> +void PropertyNodeList::invalidateCacheIfNeeded() const
> +{
> +    uint64_t docversion = m_nodes[0]->document()->domTreeVersion();
> +
> +    if (m_cache.version == docversion)
> +        return;
> +
> +    m_cache.clear();
> +    m_cache.version = docversion;
> +}

This is not a way to implement live node list. You should be using DynamicSubtreeNodeList.
If your class needs to watch outside of the subtree, then take a look at LabelsNodeList.
r- because of this.

> Source/WebCore/dom/PropertyNodeList.h:46
> +    // Adopts the contents of the nodes vector.

This comment repeats the code. Please remove.

> LayoutTests/fast/dom/MicroData/propertynodelist-getvalues-array-values-obtained-from-itemvalue-of-each-element-expected.txt:12
> +Created element of type: div
> +Created element of type: div
> +Set attribute: itemscope, value: itemscope
> +Set attribute: itemref, value: id1
> +Created element of type: meta
> +Set attribute: itemprop, value: foo
> +Set attribute: content, value: test
> +Created element of type: audio
> +Set attribute: itemprop, value: foo
> +Set attribute: src, value: test.com

These debug info is cluttering the test. Can we either interleave these with PASS or if all of these setups are needed, then could you simply hide it?

> LayoutTests/fast/dom/MicroData/propertynodelist-getvalues-array-values-obtained-from-itemvalue-of-each-element.html:29
> +var parentElement = createElement('div', {}, '<div id="id1"></div>');
> +document.body.appendChild(parentElement);
> +var testElement = createElement('div', {itemscope: 'itemscope', itemref: 'id1'});
> +parentElement.appendChild(testElement);
> +testElement.appendChild(createElement('meta', {itemprop: 'foo', content: 'test'}));
> +testElement.appendChild(createElement('audio', {itemprop: 'foo', src: 'test.com'}, 'text'));
> +testElement.appendChild(createElement('embed', {itemprop:'foo', src: 'test.com'}));
> +testElement.appendChild(createElement('iframe', {itemprop: 'foo', src: 'test.com'}, 'text'));
> +testElement.appendChild(createElement('img', {itemprop: 'foo', src: 'test.com'}));
> +testElement.appendChild(createElement('source', {itemprop: 'foo', src: 'test.com'}));
> +testElement.appendChild(createElement('track', {itemprop: 'foo', src: 'test.com'}));
> +testElement.appendChild(createElement('video', {itemprop: 'foo', src: 'test.com'}, 'text'));
> +testElement.appendChild(createElement('a', {itemprop: 'foo', href: 'test.com'}, 'text'));
> +testElement.appendChild(createElement('area', {itemprop: 'foo', href: 'test.com'}));
> +testElement.appendChild(createElement('link', {itemprop: 'foo', href: 'test.com'}));
> +testElement.appendChild(createElement('object', {itemprop: 'foo', data: 'test.com'}, 'text'));
> +parentElement.firstChild.appendChild(createElement('div', {itemprop: 'foo'}, '<span itemprop="foo" itemscope></span>'));
> +testElement.appendChild(createElement('p', {itemprop: 'foo'}, '<span itemprop="foo" itemscope></span>'));

Since this setup is done only once, you should just put it as a markup (i.e. HTML).

[Ryosuke Niwa]

Haraken, could you look at the custom binding code change in NodeList? I don't think we really need this change. If we DO need this change at the moment, then it'll be nice to fix the code generator to avoid having to write code like this.

[Kentaro Hara]

Comment on attachment 137769 [details]
Updated patch

View in context: https://bugs.webkit.org/attachment.cgi?id=137769&action=review

> Source/WebCore/bindings/js/JSNodeListCustom.cpp:72
> +#if ENABLE(MICRODATA)
> +    if (nodeList->isPropertyNodeList())
> +        return CREATE_DOM_WRAPPER(exec, globalObject, PropertyNodeList, nodeList);
> +#endif

We want to remove this custom binding, but I have two questions before posting more detailed comments:

- The reason why you want to write a custom code for this is to insert '#if ENABLE(MICRODATA)', right?
- Why did you write a custom code for JSC only? (i.e. why didn't you write a custom code for V8?)

[Arko Saha]

(In reply to comment #17)
> (From update of attachment 137769 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=137769&action=review
> 
> > Source/WebCore/bindings/js/JSNodeListCustom.cpp:72
> > +#if ENABLE(MICRODATA)
> > +    if (nodeList->isPropertyNodeList())
> > +        return CREATE_DOM_WRAPPER(exec, globalObject, PropertyNodeList, nodeList);
> > +#endif
> 
> We want to remove this custom binding, but I have two questions before posting more detailed comments:
> 
> - The reason why you want to write a custom code for this is to insert '#if ENABLE(MICRODATA)', right?
> - Why did you write a custom code for JSC only? (i.e. why didn't you write a custom code for V8?)

PropertiesCollection.namedItem() should return ProprtyNodeList object. But without above JS custom binding code namedItem() returns NodeList object. JS code generator does not automatically generate binging code to do the same.
But V8 codegenerater generates appropriate code and V8 returns PropertyNodeList object in this case.
I think we should fix the JS code generator to generate appropriate code.

[Ryosuke Niwa]

(In reply to comment #18)
> PropertiesCollection.namedItem() should return ProprtyNodeList object. But without above JS custom binding code namedItem() returns NodeList object. JS code generator does not automatically generate binging code to do the same.
> But V8 codegenerater generates appropriate code and V8 returns PropertyNodeList object in this case.
> I think we should fix the JS code generator to generate appropriate code.

That definitely sounds like a bug in JS code generator.

[Kentaro Hara]

(In reply to comment #18)
> I think we should fix the JS code generator to generate appropriate code.

Yes! Please edit CodeGeneratorJS.pm.

[Arko Saha]

(In reply to comment #20)
> (In reply to comment #18)
> > I think we should fix the JS code generator to generate appropriate code.
> 
> Yes! Please edit CodeGeneratorJS.pm.

CodeGeneratorJS.pm generates toJS() code in below case:

if ((!$hasParent or $dataNode->extendedAttributes->{"JSGenerateToJSObject"}) and !($dataNode->extendedAttributes->{"CustomToJSObject"} or $dataNode->extendedAttributes->{"JSCustomToJSObject"})) {...}

Code generator was not generating the toJS() code as PropertyNodeList is derived from NodeList.
So we need to use JSGenerateToJSObject in PropertyNodeList.idl, so that it can generate toJS() code in JSPropertyNodeList.cpp .

[Kentaro Hara]

(In reply to comment #21)
> Code generator was not generating the toJS() code as PropertyNodeList is derived from NodeList.
> So we need to use JSGenerateToJSObject in PropertyNodeList.idl, so that it can generate toJS() code in JSPropertyNodeList.cpp .

Adding [JSGenerateToJSObject] would make sense. I think that [JSGenerateToJSObject] is assuming such use cases.

[Arko Saha]

Created attachment 144807 [details]
Updated patch

Incorporated review comments.

[Ryosuke Niwa]

Comment on attachment 144807 [details]
Updated patch

View in context: https://bugs.webkit.org/attachment.cgi?id=144807&action=review

> Source/WebCore/dom/Node.h:644
> +    PassRefPtr<PropertyNodeList> propertyNodeList(const Vector<Element*>&, const String&);
> +    void removeCachedPropertyNodeList(PropertyNodeList*, const String&);

You need to update invalidateNodeListsCacheAfterAttributeChanged and invalidateCachesThatDependOnAttributes. r- because of this.
Also, please add a test case (i.e. modify element attributes and add/remove elements dynamically that demonstrates that the current patch doesn't update property node list properly).

> Source/WebCore/dom/PropertyNodeList.cpp:46
> +    : DynamicSubtreeNodeList(itemElement->fastHasAttribute(itemrefAttr) ? static_cast<Node*>(itemElement->document()) : itemElement)

Here's no point in casing document() to Node* since Document is a Node.

> Source/WebCore/dom/PropertyNodeList.cpp:63
> +bool PropertyNodeList::isValidAncestor(const Node* testElement, const Node *other) const
> +{
> +    // Return true if other is an ancestor of testElement and any ancestor of testElement
> +    // doesn't have an itemscope attribute, otherwise false

Instead of adding a comment like this, we should give a descriptive name to the function.
How about elementIsPropertyOfRefElement(const Node* testElement, const Node* refElement)?
Nit: other isn't a great variable name here.

> Source/WebCore/dom/PropertyNodeList.cpp:69
> +            // Don't return false if node is the microdata top level item. Microdata top level item
> +            // always have itemscope attribute specified.
> +            if (element->fastHasAttribute(itemscopeAttr) && node != m_itemElement)

It's better to define an inline helper function whose name reflect this comment:
bool ALWAYS_INLINE PropertyNodeList::isTopLevelItem(Node* node)
{
    return node == m_itemElement;
}
Or maybe
bool ALWAYS_INLINE PropertyNodeList::elementDefinesDifferentItem()
{
    return element->fastHasAttribute(itemscopeAttr) && node != m_itemElement;
}

> Source/WebCore/dom/PropertyNodeList.cpp:88
> +            validAncestor = true;
> +            break;

I would move the return statement here.

> Source/WebCore/dom/PropertyNodeList.cpp:92
> +    return validAncestor && testElement->itemProp()->tokens().contains(m_name);

And just return false here. You don't need validAncestor then.

> Source/WebCore/dom/PropertyNodeList.cpp:101
> +        Node* node = item(i);
> +        HTMLElement* element = toHTMLElement(node);
> +        propertyValue.append(element->itemValue());

I would combine these 3 lines into one:
propertyValue.append(toElement(item(i))->itemValue());

> Source/WebCore/dom/PropertyNodeList.h:59
> +    virtual bool nodeMatches(Element*) const;

Please use OVERRIDE macro.

> Source/WebCore/html/MicroDataItemValue.idl:36
> +    interface [
> +        Conditional=MICRODATA,
> +        CustomToJSObject
> +    ] MicroDataItemValue {
> +    };

Clever! But we should probably not expose the interface object; OmitConstructor?

> LayoutTests/fast/dom/MicroData/propertynodelist-getvalues-array-values-obtained-from-itemvalue-of-each-element.html:51
> +    shouldBe("valuesArray[i]", "propertyNodeList[i].itemValue");

You should also check that the interface name of each item in the list is not MicroDataItemValue.

[Arko Saha]

Created attachment 148559 [details]
Updated patch

[Arko Saha]

(In reply to comment #24)

> > Source/WebCore/dom/PropertyNodeList.cpp:46
> > +    : DynamicSubtreeNodeList(itemElement->fastHasAttribute(itemrefAttr) ? static_cast<Node*>(itemElement->document()) : itemElement)
> 
> Here's no point in casing document() to Node* since Document is a Node.

We would require a cast here reason being :
exp1 ? exp2 : exp3
The return type of ternary operator depends on exp2 (Document), and convertibility of exp3 (Element) into exp2 (Document) as per usual/overloaded conversion rules. Otherwise the compiler will complain of  "conditional expression between distinct pointer types 'WebCore::Document*' and 'WebCore::Element*' ". So we need to make sure both the expressions exp2 and exp3 return same type or atleast safely convertible types by using cast.

[Build Bot]

Comment on attachment 148559 [details]
Updated patch

Attachment 148559 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13001216

[Arko Saha]

Created attachment 149279 [details]
Updated patch

[Ryosuke Niwa]

Comment on attachment 149279 [details]
Updated patch

View in context: https://bugs.webkit.org/attachment.cgi?id=149279&action=review

> Source/WebCore/dom/DynamicNodeList.h:72
> -    void invalidateCache() { m_caches.reset(); }
> +    virtual void invalidateCache() { m_caches.reset(); }

We definitely shouldn't make this a virtual function. This function is hot when inserting/removing nodes. r- because of this.

> Source/WebCore/dom/Node.cpp:2765
> +Vector<Element*> Node::getItemRefElements()
> +{
> +    ASSERT(isElementNode());

Can we add this to Element or HTMLElement instead?
Also, it's awfully inefficient to return a Vector. Consider making it an out argument (i.e. Vector<Element*>&).

> Source/WebCore/dom/PropertyNodeList.cpp:117
> +    m_caches.rootedAtDocument = toElement(ownerNode())->fastHasAttribute(itemrefAttr);

We should do this in DynamicNodeList::rootNode() instead. We should probably refactor itemForwardsFromCurrent and itemBackwardsFromCurrent so that they don't take rootNode() as an argument instead of calling it themselves.

> Source/WebCore/dom/PropertyNodeList.cpp:118
> +    m_isItemRefElementsCacheValid = false;

You should add this to DynamicNodeList::Caches directly.

[Arko Saha]

Created attachment 150414 [details]
Updated patch

Incorporated review comments.

[Arko Saha]

Created attachment 151939 [details]
Updated TOT patch

[Gyuyoung Kim]

Comment on attachment 151939 [details]
Updated TOT patch

Attachment 151939 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13204723

[Ryosuke Niwa]

Comment on attachment 151939 [details]
Updated TOT patch

View in context: https://bugs.webkit.org/attachment.cgi?id=151939&action=review

> Source/WebCore/dom/DynamicNodeList.cpp:134
> +bool DynamicNodeList::ownerNodeHasItemRefAttribute() const

Please add inline keyword here.
Also, I would add this right next to rootNode if I were you.

> Source/WebCore/dom/DynamicNodeList.h:52
> +        , m_isRootedAtDocument(rootType == RootedAtDocument)

We don't need this variable anymore.

> Source/WebCore/dom/PropertyNodeList.cpp:61
> +    return element->fastHasAttribute(itemscopeAttr) && element != ownerNode();

I would have included this condition directly in elementIsPropertyOfRefElement.
I don't think it's really helping the code clarity.

> Source/WebCore/dom/PropertyNodeList.cpp:68
> +        if (node->isHTMLElement()) {
> +            if (elementDefinesDifferentItem(toHTMLElement(node)))

Can we combine these if statements?

> Source/WebCore/dom/PropertyNodeList.cpp:108
> +    for (unsigned i = 0; i < length(); ++i)
> +        propertyValue.append(toHTMLElement(item(i))->itemValue());

This is inefficient when the cache is invalid (we'll iterate over nodes twice).
It's probably better to use while and loop as long as item() is returning a non-zero value.

> Source/WebCore/dom/PropertyNodeList.h:61
> +    bool ALWAYS_INLINE elementDefinesDifferentItem(const HTMLElement*) const;

ALWAYS_INLINE here doesn't do anything.

> Source/WebCore/html/HTMLElement.cpp:1027
> +        if (!processedItemRef->tokens().contains(id) && itemRefs->tokens().contains(id)) {
> +            processedItemRef->setValue(id);
> +            if (!element->isDescendantOf(this))
> +                itemRefElements.append(element);
> +        }

I just realized that http://www.whatwg.org/specs/web-apps/current-work/multipage/microdata.html#associating-names-with-items
says that we should be adding the first element that matches the id. Here, we're adding all elements that match the id.
We should fix that (with a test). In addition, that means we can just use TreeScope::getElementById instead of traversing the entire tree scope.

> Source/WebCore/html/HTMLPropertiesCollection.h:91
>      mutable bool m_hasItemRefElements : 1;

Can we also remove this member variable now that DynamicNodeListCacheBase has isItemRefElementsCacheValid?

[Ryosuke Niwa]

Sorry, you'll need to rebaseline your patch again after http://trac.webkit.org/changeset/122498.

[Arko Saha]

Created attachment 152265 [details]
Updated patch

Incorporated review comments.

[Arko Saha]

(In reply to comment #33)
> (From update of attachment 151939 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=151939&action=review
> 
> > Source/WebCore/dom/DynamicNodeList.cpp:134
> > +bool DynamicNodeList::ownerNodeHasItemRefAttribute() const
> 
> Please add inline keyword here.
> Also, I would add this right next to rootNode if I were you.

Done.
 
> > Source/WebCore/dom/DynamicNodeList.h:52
> > +        , m_isRootedAtDocument(rootType == RootedAtDocument)
> 
> We don't need this variable anymore.

Yes you are right. Removed the variable.
 
> > Source/WebCore/dom/PropertyNodeList.cpp:61
> > +    return element->fastHasAttribute(itemscopeAttr) && element != ownerNode();
> 
> I would have included this condition directly in elementIsPropertyOfRefElement.
> I don't think it's really helping the code clarity.

Ok.

> > Source/WebCore/dom/PropertyNodeList.cpp:68
> > +        if (node->isHTMLElement()) {
> > +            if (elementDefinesDifferentItem(toHTMLElement(node)))
> 
> Can we combine these if statements?
> 
> > Source/WebCore/dom/PropertyNodeList.cpp:108
> > +    for (unsigned i = 0; i < length(); ++i)
> > +        propertyValue.append(toHTMLElement(item(i))->itemValue());
> 
> This is inefficient when the cache is invalid (we'll iterate over nodes twice).
> It's probably better to use while and loop as long as item() is returning a non-zero value.

Done.

> > Source/WebCore/dom/PropertyNodeList.h:61
> > +    bool ALWAYS_INLINE elementDefinesDifferentItem(const HTMLElement*) const;
> 
> ALWAYS_INLINE here doesn't do anything.

ok. Removed inline keyword.
 
> > Source/WebCore/html/HTMLElement.cpp:1027
> > +        if (!processedItemRef->tokens().contains(id) && itemRefs->tokens().contains(id)) {
> > +            processedItemRef->setValue(id);
> > +            if (!element->isDescendantOf(this))
> > +                itemRefElements.append(element);
> > +        }
> 
> I just realized that http://www.whatwg.org/specs/web-apps/current-work/multipage/microdata.html#associating-names-with-items
> says that we should be adding the first element that matches the id. Here, we're adding all elements that match the id.
> We should fix that (with a test). In addition, that means we can just use TreeScope::getElementById instead of traversing the entire tree scope.

Ok. Filed a bug to address this https://bugs.webkit.org/show_bug.cgi?id=91249

> > Source/WebCore/html/HTMLPropertiesCollection.h:91
> >      mutable bool m_hasItemRefElements : 1;
> 
> Can we also remove this member variable now that DynamicNodeListCacheBase has isItemRefElementsCacheValid?

Done.

[Ryosuke Niwa]

Comment on attachment 152265 [details]
Updated patch

View in context: https://bugs.webkit.org/attachment.cgi?id=152265&action=review

> Source/WebCore/dom/DynamicNodeList.h:109
> +    const NodeListRootType m_rootType;

We shouldn't do this. It'll use 32-bits. Please use unsigned with 2 bits and cast as needed. See how m_invalidationType is initialized & usd.

> Source/WebCore/html/HTMLPropertiesCollection.h:87
>      mutable bool m_hasPropertyNameCache : 1;
> -    mutable bool m_hasItemRefElements : 1;
> +    mutable bool m_hasNamedItem : 1;

Why do we need these variables? DynamicNodeListCacheBase already has hasNameCache() and setHasNameCache().

[Arko Saha]

Created attachment 152603 [details]
Updated patch

Incorporated review comments.

[Ryosuke Niwa]

Comment on attachment 152603 [details]
Updated patch

View in context: https://bugs.webkit.org/attachment.cgi?id=152603&action=review

> Source/WebCore/dom/DynamicNodeList.cpp:43
> +    if (rootType() == NodeListIsRootedAtDocumentIfOwnerHasItemrefAttr)

You can directly use m_rootType here or make rootType() private.

> Source/WebCore/dom/DynamicNodeList.h:73
> +    ALWAYS_INLINE NodeListRootType rootType() const { return static_cast<NodeListRootType>(m_rootType); }

This member function shouldn't be public.

> Source/WebCore/dom/DynamicNodeList.h:115
> +    mutable unsigned m_isItemRefElementsCacheValid : 1;

You should move this to below m_isNameCacheValid since this variable is only used in HTMLCollection.

> Source/WebCore/dom/PropertyNodeList.cpp:84
> +    updateItemRefElements();

We shouldn't be calling this function here since this function is called for every node during the iteration.
It appears to me that you should wait until my patch for the bug 91335 lands.

> Source/WebCore/html/HTMLElement.cpp:1028
> +    for (Node* current = rootNode->firstChild(); current; current = current->traverseNextNode(rootNode)) {
> +        if (!current->isHTMLElement())
> +            continue;
> +        HTMLElement* element = toHTMLElement(current);
> +
> +        if (element == this) {
> +            itemRefElements.append(element);
> +            continue;
> +        }
> +
> +        const AtomicString& id = element->getIdAttribute();
> +        if (!processedItemRef->tokens().contains(id) && itemRefs->tokens().contains(id)) {
> +            processedItemRef->setValue(id);
> +            if (!element->isDescendantOf(this))
> +                itemRefElements.append(element);
> +        }
> +    }

We should probably fix https://bugs.webkit.org/show_bug.cgi?id=91249 first.

[Arko Saha]

Created attachment 153779 [details]
Updated patch

[Ryosuke Niwa]

Comment on attachment 153779 [details]
Updated patch

View in context: https://bugs.webkit.org/attachment.cgi?id=153779&action=review

> Source/WebCore/dom/PropertyNodeList.cpp:61
> +                return false;

Nit: Wrong indentation

> Source/WebCore/dom/PropertyNodeList.cpp:99
> +    unsigned offset = 0;
> +
> +    while (Node* node = item(offset)) {

Use for loop instead so that you can declare offset in the for loop.

[Arko Saha]

Landed in: <http://trac.webkit.org/changeset/123434>