[webkit-reviews] review denied: [Bug 69839] [Microdata] Add itemprop, itemref, itemvalue attributes : [Attachment 111248] Updated patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has denied Arko Saha <nghq36 at motorola.com>'s
request for review:
Bug 69839: [Microdata] Add itemprop, itemref, itemvalue attributes
https://bugs.webkit.org/show_bug.cgi?id=69839

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

------- Additional Comments from Ryosuke Niwa <rniwa at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=111248&action=review


> Source/WebCore/bindings/js/JSHTMLElementCustom.cpp:41
> +#if ENABLE(MICRODATA)
> +static JSValue toJS(ExecState* exec, JSDOMGlobalObject* globalObject,
MicroDataItemValue* itemValue)

You need to update V8 binding as well.

> Source/WebCore/html/HTMLAnchorElement.h:130
> +    virtual String itemValueText() const;
> +    virtual void setItemValueText(const String&, ExceptionCode&);

Please use OVERRIDE macro.

> Source/WebCore/html/HTMLAreaElement.h:64
> +    virtual String itemValueText() const;
> +    virtual void setItemValueText(const String&, ExceptionCode&);

Ditto.

> Source/WebCore/html/HTMLElement.cpp:46
> +#if ENABLE(MICRODATA)
> +#include "MicroDataItemValue.h"
> +#endif

This should be added below all other headers with a blank line to separate it
from the rest.

> Source/WebCore/html/HTMLElement.cpp:994
> +void HTMLElement::itemPropSetToken(const String& value)

We don't prefix function names like this. Please rename. To begin with, why
does it need to be token? Can't it just be setItemProp?

> Source/WebCore/html/HTMLElement.cpp:1010
> +void HTMLElement::itemRefSetToken(const String& value)

Ditto about the function name.

> Source/WebCore/html/HTMLElement.idl:76
> +		   attribute [Conditional=MICRODATA, Custom] custom itemValue

Missing one space in indentation?

> Source/WebCore/html/HTMLMediaElement.cpp:3037
> +String HTMLMediaElement::itemValueText() const
> +{
> +    return getURLAttribute(srcAttr);
> +}
> +
> +void HTMLMediaElement::setItemValueText(const String& value, ExceptionCode&
ec)
> +{
> +    setAttribute(srcAttr, value, ec);
> +}

This element isn't listed in
http://dev.w3.org/html5/md/Overview.html#dom-itemvalue. Are you sure this is
the correct behavior?

> Source/WebCore/html/MicroDataItemValue.h:57
> +    String m_str;

Please don't abbreviate string as str.

> LayoutTests/ChangeLog:41
> +	   * fast/dom/MicroData/010-expected.txt: Added.
> +	   * fast/dom/MicroData/010.html: Added.
> +	   * fast/dom/MicroData/011-expected.txt: Added.
> +	   * fast/dom/MicroData/011.html: Added.
> +	   * fast/dom/MicroData/012-expected.txt: Added.
> +	   * fast/dom/MicroData/012.html: Added.
> +	   * fast/dom/MicroData/013-expected.txt: Added.
> +	   * fast/dom/MicroData/013.html: Added.
> +	   * fast/dom/MicroData/014-expected.txt: Added.
> +	   * fast/dom/MicroData/014.html: Added.
> +	   * fast/dom/MicroData/015-expected.txt: Added.
> +	   * fast/dom/MicroData/015.html: Added.
> +	   * fast/dom/MicroData/016-expected.txt: Added.
> +	   * fast/dom/MicroData/016.html: Added.
> +	   * fast/dom/MicroData/017-expected.txt: Added.
> +	   * fast/dom/MicroData/017.html: Added.
> +	   * fast/dom/MicroData/018-expected.txt: Added.
> +	   * fast/dom/MicroData/018.html: Added.
> +	   * fast/dom/MicroData/019-expected.txt: Added.
> +	   * fast/dom/MicroData/019.html: Added.
> +	   * fast/dom/MicroData/020-expected.txt: Added.
> +	   * fast/dom/MicroData/020.html: Added.
> +	   * fast/dom/MicroData/021-expected.txt: Added.
> +	   * fast/dom/MicroData/021.html: Added.
> +	   * fast/dom/MicroData/022-expected.txt: Added.
> +	   * fast/dom/MicroData/022.html: Added.
> +	   * fast/dom/MicroData/023-expected.txt: Added.
> +	   * fast/dom/MicroData/023.html: Added.
> +	   * fast/dom/MicroData/024-expected.txt: Added.
> +	   * fast/dom/MicroData/024.html: Added.
> +	   * fast/dom/MicroData/025-expected.txt: Added.
> +	   * fast/dom/MicroData/025.html: Added.

It's not great that all these tests are numbered. Can we give more descriptive
names?

> LayoutTests/fast/dom/MicroData/010.html:1
> +<html>

Missing DOCTYPE.

> LayoutTests/fast/dom/MicroData/010.html:11
> +var element = createElem('div',{itemprop:'http://example.com/foo'});

I must have missed it in my previous review, but we shouldn't be abbreviating
element as Elem. This is clearly stated in our style guide:
http://www.webkit.org/coding/coding-style.html Please fix.

Also you need a space after, and :.

> LayoutTests/fast/dom/MicroData/011.html:11
> +var element = createElem('div',{itemprop:'foo bar FOO'});

It'll be helpful to show this setup in the result so that we can make sense of
the results below.

> LayoutTests/fast/dom/MicroData/011.html:21
> +// itemProp should return case-sensitive strings

It'll be helpful if this was output in the result using debug().

> LayoutTests/fast/dom/MicroData/011.html:25
> +// itemProp should not contain an undefined token

Ditto.

> LayoutTests/fast/dom/MicroData/011.html:28
> +// itemProp.length should be 0 if element has not tokens

Ditto.

> LayoutTests/fast/dom/MicroData/011.html:34
> +

Why we do have a blank line here?

> LayoutTests/fast/dom/MicroData/012.html:13
> +// itemProp.add must reflect correctly

We probably want to test that itemprop content attribute is properly updated as
well.

> LayoutTests/fast/dom/MicroData/013.html:13
> +element.itemProp = 'test';
> +shouldBeTrue("element.itemProp.toString() == 'foo'");

It'll be helpful do:
shouldBeTrue("element.itemProp = 'test'; element.itemProp.toString() ==
'foo'");

> LayoutTests/fast/dom/MicroData/013.html:17
> +element.itemProp.length = 0;
> +shouldBeTrue("element.itemProp.length == 1");

Ditto.

> LayoutTests/fast/dom/MicroData/019.html:13
> +    try {

You need to initialize exceptionCode before try or otherwise exceptionCode will
never be updated when no exception was thrown.
That would mean that if we throw an exception for meta, then all subsequent
tests will pass even if they never threw an exception. r- because of this bug.

> LayoutTests/fast/dom/MicroData/020.html:8
> +<p>This test ensures that writing to itemValue must throw an
INVALID_ACCESS_ERR error if the element does not have an itemprop
attribute.</p>

This test description is incorrect.

> LayoutTests/fast/dom/MicroData/022-expected.txt:4
> +PASS testElement.itemValue is 'http://example.org/'
> +PASS testElement.itemValue is 'http://example.net/'

It'll be helpful if the result told us what kind of element testElement is.

> LayoutTests/fast/dom/MicroData/024-expected.txt:4
> +PASS testElement.itemValue is 'http://example.org/'
> +PASS testElement.itemValue is 'http://example.net/'

Ditto about the tag name of testElement not shown here.