Bug 69839 – [Microdata] Add itemprop, itemref, itemvalue attributes

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

[Arko Saha]

Implement itemprop, itemtype, itemvalue Microdata attributes according to the specification http://www.whatwg.org/specs/web-apps/current-work/complete/microdata.html.

[Arko Saha]

Created attachment 110513 [details]
Initial patch

[Arko Saha]

Created attachment 111245 [details]
Updated patch

Added test-cases for Microdata itemprop, itemref, itemvalue attributes.

[Early Warning System Bot]

Comment on attachment 111245 [details]
Updated patch

Attachment 111245 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10058014

[Gyuyoung Kim]

Comment on attachment 111245 [details]
Updated patch

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

[WebKit Review Bot]

Comment on attachment 111245 [details]
Updated patch

Attachment 111245 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10075970

[Arko Saha]

Created attachment 111248 [details]
Updated patch

[Ryosuke Niwa]

Comment on attachment 111248 [details]
Updated patch

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.

[Arko Saha]

(In reply to comment #7)
> (From update of attachment 111248 [details])
> 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.
Done! Updated V8 binding.

> > Source/WebCore/html/HTMLAnchorElement.h:130
> > +    virtual String itemValueText() const;
> > +    virtual void setItemValueText(const String&, ExceptionCode&);
> 
> Please use OVERRIDE macro.
Done!

> > 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.
Done!

> > 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?
Done! Modified the function name accordingly.

> > Source/WebCore/html/HTMLElement.idl:76
> > +                attribute [Conditional=MICRODATA, Custom] custom itemValue
> 
> Missing one space in indentation?
Corrected 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?
HTMLVideoElement and HTMLAudioElement are derived from HTMLMediaElement, so I kept this functions in HTMLMediaElement.

> > Source/WebCore/html/MicroDataItemValue.h:57
> > +    String m_str;
> 
> Please don't abbreviate string as str.
Done!

> > +        * 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?
Done! Changed all file names accordingly.

> > 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 :.
Incorporated this comments.

> > 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.
Done!

> > 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().
Done! Using debug() to print useful information.

> > 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'");
Done!

> > 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.
Done!

> 
> > 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.
Done!

[Arko Saha]

Created attachment 111460 [details]
Updated patch

Incorporating review comments.

[WebKit Review Bot]

Attachment 111460 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1

Source/WebCore/WebCore.vcproj/WebCore.vcproj:25583:  mismatched tag  [xml/syntax] [5]
Total errors found: 1 in 80 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Ryosuke Niwa]

(In reply to comment #10)
> Attachment 111460 [details] did not pass style-queue:
> 
> Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1
> 
> Source/WebCore/WebCore.vcproj/WebCore.vcproj:25583:  mismatched tag  [xml/syntax] [5]
> Total errors found: 1 in 80 files
> 
> 
> If any of these errors are false positives, please file a bug against check-webkit-style.

Could you fix this? Your change to vcproj is probably incorrect.

[Arko Saha]

Created attachment 111854 [details]
Updated patch

[Dimitri Glazkov (Google)]

Comment on attachment 111854 [details]
Updated patch

V8 bindings look fine.

[Ryosuke Niwa]

Comment on attachment 111854 [details]
Updated patch

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

r=me. I'm very impressed by your through testing and how clean your patch is! Keep up your great work.

I can fix those myself and land it for you if you'd like.

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

Please fix indentation here (missing one space).

> LayoutTests/fast/dom/MicroData/002-expected.txt:5
> +document.getItems() without aurgument
> +TEST SUCCEEDED

Why the change? PASS looked better.

> LayoutTests/fast/dom/MicroData/itemprop-for-an-element-must-be-correct-expected.txt:15
> +PASS element.itemProp[2] == 'FOO' is true

Maybe try something like fOo?

> LayoutTests/fast/dom/MicroData/itemprop-for-an-element-must-be-correct.html:28
> +debug("<br>itemProp.length should be 0 if element has not tokens.");

Nit: has no tokens or doesn't have any tokens?

> LayoutTests/fast/dom/MicroData/itemvalue-reflects-the-src-attr.html:1
> +<html>

Nit: can we add <!DOCTYPE html>?

[Ryosuke Niwa]

Comment on attachment 111854 [details]
Updated patch

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

> LayoutTests/fast/dom/MicroData/itemvalue-throws-excpetion-onsetting-001.html:19
> +    }catch (e) {

Please put a space before catch. Also, please spell out exception instead of using one-letter variable e.

> LayoutTests/fast/dom/MicroData/itemvalue-throws-excpetion-onsetting-002.html:19
> +    }catch (e) {

Ditto.

[Daniel Cheng]

Comment on attachment 111854 [details]
Updated patch

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

Drive by... a couple nits.

> Source/WebCore/html/HTMLElement.cpp:205
> +    }  else if (attr->name() == itemrefAttr) {

Extra space

> Source/WebCore/html/MicroDataItemValue.cpp:38
> +namespace WebCore {

Newline here for consistency with the other files you added.

> Source/WebCore/html/MicroDataItemValue.h:50
> +    static PassRefPtr<MicroDataItemValue> createFromString(String);

Use a const reference.

> Source/WebCore/html/MicroDataItemValue.h:58
> +    MicroDataItemValue(String);

Use a const reference.

[Ryosuke Niwa]

Comment on attachment 111854 [details]
Updated patch

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

>> Source/WebCore/html/MicroDataItemValue.h:50
>> +    static PassRefPtr<MicroDataItemValue> createFromString(String);
> 
> Use a const reference.

That's a good catch!

>> Source/WebCore/html/MicroDataItemValue.h:58

> 
> Use a const reference.

Ditto.

[Arko Saha]

Created attachment 112173 [details]
Updated patch

[Arko Saha]

Thank you all for your review comments.
I have incorporated all review comments and uploaded the patch.

Ryosuke, please help me to land this patch. Also please help me to add newly added files i.e, MicroDataItemValue.cpp/h to xcode project.
Thanks again :)

[Ryosuke Niwa]

Comment on attachment 112173 [details]
Updated patch

Will land the patch after making xcodeproj change.

[Ryosuke Niwa]

Committed r98300: <http://trac.webkit.org/changeset/98300>