Bug 101950 – [JS bindings] Treat itemValue attribute in HTMLElement as a special case

• Status: RESOLVED
• Resolution: WONTFIX
• Component: New Bugs

[Zan Dobersek]

[JS bindings] Treat itemValue attribute in HTMLElement as a special case

[Zan Dobersek]

Created attachment 173671 [details]
Patch

[Adam Barth]

Comment on attachment 173671 [details]
Patch

Why does these need to be a special case?

[Zan Dobersek]

(In reply to comment #2)
> (From update of attachment 173671 [details])
> Why does these need to be a special case?

To generate getter and setter methods that are functionally equal and mostly identical to the custom methods that are being removed in bug #101882.

The methods that are currently generated and cause compilation failure (jsHTMLElementItemValue and setJSHTMLElementItemValue) can be visible here:
https://bugs.webkit.org/attachment.cgi?id=173666

[Zan Dobersek]

With the patch, the generated methods subsequently look like this:

JSValue jsHTMLElementItemValue(ExecState* exec, JSValue slotBase, PropertyName)
{
    JSHTMLElement* castedThis = jsCast<JSHTMLElement*>(asObject(slotBase));
    UNUSED_PARAM(exec);
    HTMLElement* impl = static_cast<HTMLElement*>(castedThis->impl());
    JSValue result = toJS(exec, castedThis->globalObject(), WTF::getPtr(impl->itemValue()));
    return result;
}

void setJSHTMLElementItemValue(ExecState* exec, JSObject* thisObject, JSValue value)
{
    UNUSED_PARAM(exec);
    JSHTMLElement* castedThis = jsCast<JSHTMLElement*>(thisObject);
    HTMLElement* impl = static_cast<HTMLElement*>(castedThis->impl());
    ExceptionCode ec = 0;
    impl->setItemValue(exec->globalData(), value, ec);
    setDOMException(exec, ec);
}

[Adam Barth]

It's not clear to me that adding a special case to the code generator is better than having custom bindings code.  Why should "DOMObject" and "any" behave differently for HTMLElement?  Perhaps we should change the IDL somehow?

[Zan Dobersek]

(In reply to comment #5)
> It's not clear to me that adding a special case to the code generator is better than having custom bindings code.  Why should "DOMObject" and "any" behave differently for HTMLElement?  Perhaps we should change the IDL somehow?

The 'DOMObject' and 'any' types are translated to the native ScriptValue type. This is obviously causing problems here because the 'DOMObject' type represents MicroDataItemValue.

This seems to me more of a limit imposed by the JSC bindings generator. From that point of view the itemValue attribute can be assigned the JSCustom IDL attribute along with the current getter and setter custom implementations being preserved.

[Adam Barth]

Perhaps the IDL should be changed to MicroDataItemValue?

[Kentaro Hara]

First of all, thank you very much for rolling out my patch and uploading the fix.

Another question is that in the current custom setter, JSC does the null check (i.e. valueToStringWithNullCheck(exec, value)) but V8 doesn't do the null check (toWebCoreString(value)). Which is an expected behavior?

[Zan Dobersek]

Can someone confirm whether or not the MicroData feature is enabled on Chromium? As far as I've grepped it doesn't seem to be.
I'm asking because I'm inspecting how the V8-generated bindings for this attribute behave when the Custom IDL attribute is removed and the generated code appears incorrect - it should cause compilation failure on Chromium as well if the feature would be enabled.

(In reply to comment #7)
> Perhaps the IDL should be changed to MicroDataItemValue?

I think that would be best, but I'm not sure whether it would be doable without adding the JSCustomToNativeObject IDL attribute to the MicroDataItemValue interface and supplying the custom implementation. OTOH, V8 bindings don't support such attribute at the moment.

(In reply to comment #8)
> Another question is that in the current custom setter, JSC does the null check (i.e. valueToStringWithNullCheck(exec, value)) but V8 doesn't do the null check (toWebCoreString(value)). Which is an expected behavior?

From the spec:
http://www.whatwg.org/specs/web-apps/current-work/multipage/microdata.html#dom-itemvalue
"When the itemValue IDL attribute is reflecting a content attribute or acting like the element's textContent attribute, the user agent must, on setting, convert the new value to the IDL DOMString value before using it according to the mappings described above."

I understand this as if null should be converted to an empty string, so using valueToStringWithNullCheck is correct.

[Adam Barth]

> Can someone confirm whether or not the MicroData feature is enabled on Chromium?

I don't believe it is.

[Zan Dobersek]

(In reply to comment #10)
> > Can someone confirm whether or not the MicroData feature is enabled on Chromium?
> 
> I don't believe it is.

Can now confirm that it isn't.

I've tried to build the Chromium port with the patch from bug #101882 applied and the build fails - the generated bindings for itemValue are incorrect.

I'd recommend keeping the custom implementations and marking both bug #101882 and this bug as wontfix.

[Zan Dobersek]

Support for the Microdata specification was removed from the source, so this can be resolved as WONTFIX.