| Summary: | [Microdata] Add itemprop, itemref, itemvalue attributes | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Arko Saha <arko> | ||||||||||||||
| Component: | DOM | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||
| Severity: | Normal | CC: | abarth, darin, dglazkov, donggwan.kim, japhet, ojan, rniwa, sam, webkit.review.bot | ||||||||||||||
| Priority: | P2 | ||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||
| Hardware: | All | ||||||||||||||||
| OS: | All | ||||||||||||||||
| Bug Depends on: | 68610 | ||||||||||||||||
| Bug Blocks: | 68609 | ||||||||||||||||
| Attachments: |
|
||||||||||||||||
|
Description
Arko Saha
2011-10-11 07:28:07 PDT
Created attachment 110513 [details]
Initial patch
Created attachment 111245 [details]
Updated patch
Added test-cases for Microdata itemprop, itemref, itemvalue attributes.
Comment on attachment 111245 [details] Updated patch Attachment 111245 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/10058014 Comment on attachment 111245 [details] Updated patch Attachment 111245 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/10078920 Comment on attachment 111245 [details] Updated patch Attachment 111245 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10075970 Created attachment 111248 [details]
Updated patch
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. (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! Created attachment 111460 [details]
Updated patch
Incorporating review comments.
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.
(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. Created attachment 111854 [details]
Updated patch
Comment on attachment 111854 [details]
Updated patch
V8 bindings look fine.
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 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 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 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. Created attachment 112173 [details]
Updated patch
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 on attachment 112173 [details]
Updated patch
Will land the patch after making xcodeproj change.
Committed r98300: <http://trac.webkit.org/changeset/98300> |