Bug 80269

Created attachment 130101 [details] Work in progress.

Created attachment 130364 [details] Patch

Comment on attachment 130364 [details] Patch Can we implement this without custom bindings?

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.

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.

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

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

Created attachment 133059 [details] Updated patch

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.

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

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.

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

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

Created attachment 137769 [details] Updated patch Incorporated review comments..

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

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.

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

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

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

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

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

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

Created attachment 144807 [details] Updated patch Incorporated review comments.

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.

Created attachment 148559 [details] Updated patch

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

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

Created attachment 149279 [details] Updated patch

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.

Created attachment 150414 [details] Updated patch Incorporated review comments.

Created attachment 151939 [details] Updated TOT patch

Comment on attachment 151939 [details] Updated TOT patch Attachment 151939 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/13204723

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?

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

Created attachment 152265 [details] Updated patch Incorporated review comments.

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

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

Created attachment 152603 [details] Updated patch Incorporated review comments.

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.

Created attachment 153779 [details] Updated patch

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.

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