Bug 69839 - [Microdata] Add itemprop, itemref, itemvalue attributes
Summary: [Microdata] Add itemprop, itemref, itemvalue attributes
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 68610
Blocks: 68609
  Show dependency treegraph
 
Reported: 2011-10-11 07:28 PDT by Arko Saha
Modified: 2011-10-24 17:12 PDT (History)
9 users (show)

See Also:


Attachments
Initial patch (32.06 KB, patch)
2011-10-11 07:39 PDT, Arko Saha
no flags Details | Formatted Diff | Diff
Updated patch (62.71 KB, patch)
2011-10-17 04:58 PDT, Arko Saha
webkit-ews: commit-queue-
Details | Formatted Diff | Diff
Updated patch (67.40 KB, patch)
2011-10-17 05:26 PDT, Arko Saha
rniwa: review-
Details | Formatted Diff | Diff
Updated patch (103.25 KB, patch)
2011-10-18 10:09 PDT, Arko Saha
no flags Details | Formatted Diff | Diff
Updated patch (101.79 KB, patch)
2011-10-20 15:02 PDT, Arko Saha
rniwa: review+
Details | Formatted Diff | Diff
Updated patch (101.23 KB, patch)
2011-10-24 04:25 PDT, Arko Saha
rniwa: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Arko Saha 2011-10-11 07:28:07 PDT
Implement itemprop, itemtype, itemvalue Microdata attributes according to the specification http://www.whatwg.org/specs/web-apps/current-work/complete/microdata.html.
Comment 1 Arko Saha 2011-10-11 07:39:47 PDT
Created attachment 110513 [details]
Initial patch
Comment 2 Arko Saha 2011-10-17 04:58:00 PDT
Created attachment 111245 [details]
Updated patch

Added test-cases for Microdata itemprop, itemref, itemvalue attributes.
Comment 3 Early Warning System Bot 2011-10-17 05:06:20 PDT
Comment on attachment 111245 [details]
Updated patch

Attachment 111245 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10058014
Comment 4 Gyuyoung Kim 2011-10-17 05:06:33 PDT
Comment on attachment 111245 [details]
Updated patch

Attachment 111245 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/10078920
Comment 5 WebKit Review Bot 2011-10-17 05:11:28 PDT
Comment on attachment 111245 [details]
Updated patch

Attachment 111245 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10075970
Comment 6 Arko Saha 2011-10-17 05:26:29 PDT
Created attachment 111248 [details]
Updated patch
Comment 7 Ryosuke Niwa 2011-10-17 13:09:53 PDT
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.
Comment 8 Arko Saha 2011-10-18 09:23:06 PDT
(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!
Comment 9 Arko Saha 2011-10-18 10:09:14 PDT
Created attachment 111460 [details]
Updated patch

Incorporating review comments.
Comment 10 WebKit Review Bot 2011-10-18 10:12:21 PDT
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.
Comment 11 Ryosuke Niwa 2011-10-20 10:20:41 PDT
(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.
Comment 12 Arko Saha 2011-10-20 15:02:37 PDT
Created attachment 111854 [details]
Updated patch
Comment 13 Dimitri Glazkov (Google) 2011-10-21 13:17:12 PDT
Comment on attachment 111854 [details]
Updated patch

V8 bindings look fine.
Comment 14 Ryosuke Niwa 2011-10-21 14:59:32 PDT
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>?
Comment 15 Ryosuke Niwa 2011-10-21 15:08:28 PDT
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.
Comment 16 Daniel Cheng 2011-10-22 00:07:11 PDT
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.
Comment 17 Ryosuke Niwa 2011-10-22 05:37:48 PDT
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.
Comment 18 Arko Saha 2011-10-24 04:25:17 PDT
Created attachment 112173 [details]
Updated patch
Comment 19 Arko Saha 2011-10-24 04:28:33 PDT
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 :)
Comment 20 Ryosuke Niwa 2011-10-24 14:54:42 PDT
Comment on attachment 112173 [details]
Updated patch

Will land the patch after making xcodeproj change.
Comment 21 Ryosuke Niwa 2011-10-24 17:12:33 PDT
Committed r98300: <http://trac.webkit.org/changeset/98300>