[webkit-reviews] review denied: [Bug 80269] [Microdata] Implement PropertyNodeList interface. : [Attachment 137769] Updated patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has denied Arko Saha <arko at motorola.com>'s
request for review:
Bug 80269: [Microdata] Implement PropertyNodeList interface.
https://bugs.webkit.org/show_bug.cgi?id=80269

Attachment 137769: Updated patch
https://bugs.webkit.org/attachment.cgi?id=137769&action=review

------- Additional Comments from Ryosuke Niwa <rniwa at webkit.org>
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).