[Webkit-unassigned] [Bug 69839] [Microdata] Add itemprop, itemref, itemvalue attributes

Mon Oct 17 13:09:53 PDT 2011

https://bugs.webkit.org/show_bug.cgi?id=69839

Ryosuke Niwa <rniwa at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #111248|review?                     |review-
               Flag|                            |

--- Comment #7 from Ryosuke Niwa <rniwa at webkit.org>  2011-10-17 13:09:53 PST ---
(From update of attachment 111248)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.