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

Tue Oct 18 09:23:07 PDT 2011

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


--- Comment #8 from Arko Saha <nghq36 at motorola.com>  2011-10-18 09:23:06 PST ---
(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!

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