101950 2012-11-12 10:46:26 -0800 [JS bindings] Treat itemValue attribute in HTMLElement as a special case 2013-08-08 05:19:57 -0700 1 1 1 Unclassified WebKit New Bugs 528+ (Nightly build) Unspecified Unspecified RESOLVED WONTFIX P2 Normal --- 101882 1 zan zan abarth haraken japhet webkit.review.bot oldest_to_newest 764562 0 zan 2012-11-12 10:46:26 -0800 [JS bindings] Treat itemValue attribute in HTMLElement as a special case 764566 1 173671 zan 2012-11-12 10:52:28 -0800 Created attachment 173671 Patch 764605 2 173671 abarth 2012-11-12 11:21:16 -0800 Comment on attachment 173671 Patch Why does these need to be a special case? 764619 3 zan 2012-11-12 11:33:07 -0800 (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 764623 4 zan 2012-11-12 11:34:36 -0800 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); } 764643 5 abarth 2012-11-12 12:02:08 -0800 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? 764777 6 zan 2012-11-12 13:49:08 -0800 (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. 764898 7 abarth 2012-11-12 15:44:35 -0800 Perhaps the IDL should be changed to MicroDataItemValue? 764932 8 haraken 2012-11-12 16:14:48 -0800 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? 765512 9 zan 2012-11-13 05:48:10 -0800 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. 773587 10 abarth 2012-11-21 22:45:23 -0800 > Can someone confirm whether or not the MicroData feature is enabled on Chromium? I don't believe it is. 775739 11 zan 2012-11-26 11:53:18 -0800 (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. 915387 12 zan 2013-08-08 05:18:38 -0700 Support for the Microdata specification was removed from the source, so this can be resolved as WONTFIX. 173671 2012-11-12 10:52:28 -0800 2012-11-12 11:21:16 -0800 Patch bug-101950-20121112195030.patch text/plain 5137 zan U3VidmVyc2lvbiBSZXZpc2lvbjogMTM0MjQ2CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggMGVhNDRjNzUyMWNjZDMx Nzk4ZmJiZjViMmI2MTczZmE4NDBjZGJiMS4uMThjODNhZjA2YTNhMjY1YzEyMDcxYWQ3NmJkNmQ0 YWUwY2QzM2MyYSAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDIzIEBACisyMDEyLTExLTEyICBaYW4g RG9iZXJzZWsgIDx6YW5kb2JlcnNla0BnbWFpbC5jb20+CisKKyAgICAgICAgW0pTIGJpbmRpbmdz XSBUcmVhdCBpdGVtVmFsdWUgYXR0cmlidXRlIGluIEhUTUxFbGVtZW50IGFzIGEgc3BlY2lhbCBj YXNlCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0xMDE5 NTAKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBUaGUg aXRlbVZhbHVlIGF0dHJpYnV0ZSBpbiBIVE1MRWxlbWVudCBpbnRlcmZhY2UgcmVxdWlyZXMgc3Bl Y2lhbC1jYXNpbmcsCisgICAgICAgIGNvbnZlcnRpbmcgdGhlIHBvaW50ZXIgdG8gSlNWYWx1ZSBp biB0aGUgZ2V0dGVyIG1ldGhvZCBhbmQgdHJlYXRpbmcgdGhlCisgICAgICAgIHBhc3NlZC1pbiBK U1ZhbHVlIGFzIGEgbnVsbC1jaGVja2VkIHN0cmluZyBpbiB0aGUgc2V0dGVyIG1ldGhvZC4KKwor ICAgICAgICBObyBuZXcgdGVzdHMgLSBubyBuZXcgZnVuY3Rpb25hbGl0eSwganVzdCBtZXJlIHBy ZXBhcmF0aW9uIGZvciByZW1vdmluZyB0aGUgQ3VzdG9tCisgICAgICAgIGF0dHJpYnV0ZSB0aGF0 IGN1cnJlbnRseSBkZWNvcmF0ZXMgdGhlIGF0dHJpYnV0ZSBpbiBJREwuCisKKyAgICAgICAgKiBi aW5kaW5ncy9zY3JpcHRzL0NvZGVHZW5lcmF0b3JKUy5wbToKKyAgICAgICAgKEdlbmVyYXRlSW1w bGVtZW50YXRpb24pOgorICAgICAgICAoR2VuZXJhdGVQYXJhbWV0ZXJzQ2hlY2spOgorICAgICAg ICAoSlNWYWx1ZVRvTmF0aXZlKToKKyAgICAgICAgKE5hdGl2ZVRvSlNWYWx1ZSk6CisKIDIwMTIt MTEtMTIgIENhcmxvcyBHYXJjaWEgQ2FtcG9zICA8Y2dhcmNpYUBpZ2FsaWEuY29tPgogCiAgICAg ICAgIFVucmV2aWV3ZWQuIEZpeCBtYWtlIGRpc3RjaGVjay4KZGlmZiAtLWdpdCBhL1NvdXJjZS9X ZWJDb3JlL2JpbmRpbmdzL3NjcmlwdHMvQ29kZUdlbmVyYXRvckpTLnBtIGIvU291cmNlL1dlYkNv cmUvYmluZGluZ3Mvc2NyaXB0cy9Db2RlR2VuZXJhdG9ySlMucG0KaW5kZXggNDBkNjc5ZGE2ZjE4 Y2I2OWM3ZDBjODA1OGY0NzI1NzAxYjNhZGE0OS4uOGJlZDZjYzNiYjg3OTY1ZGIyYzk4YmZjNWMy NDE0NWQ1YzZlNDFlNyAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvYmluZGluZ3Mvc2NyaXB0 cy9Db2RlR2VuZXJhdG9ySlMucG0KKysrIGIvU291cmNlL1dlYkNvcmUvYmluZGluZ3Mvc2NyaXB0 cy9Db2RlR2VuZXJhdG9ySlMucG0KQEAgLTIxMzYsNyArMjEzNiw3IEBAIHN1YiBHZW5lcmF0ZUlt cGxlbWVudGF0aW9uCiAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICB9CiAgICAg ICAgICAgICAgICAgICAgICAgICAgICAgICAgIH0KIAotICAgICAgICAgICAgICAgICAgICAgICAg ICAgICAgICBteSAkbmF0aXZlVmFsdWUgPSBKU1ZhbHVlVG9OYXRpdmUoJGF0dHJpYnV0ZS0+c2ln bmF0dXJlLCAidmFsdWUiKTsKKyAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgbXkgJG5h dGl2ZVZhbHVlID0gSlNWYWx1ZVRvTmF0aXZlKCRhdHRyaWJ1dGUtPnNpZ25hdHVyZSwgInZhbHVl IiwgJGltcGxDbGFzc05hbWUpOwogICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICBpZiAo JHN2Z1Byb3BlcnR5T3JMaXN0UHJvcGVydHlUeXBlKSB7CiAgICAgICAgICAgICAgICAgICAgICAg ICAgICAgICAgICAgICBpZiAoJHN2Z1Byb3BlcnR5VHlwZSkgewogICAgICAgICAgICAgICAgICAg ICAgICAgICAgICAgICAgICAgICAgIHB1c2goQGltcGxDb250ZW50LCAiICAgIGlmIChpbXBsLT5p c1JlYWRPbmx5KCkpIHtcbiIpOwpAQCAtMjc2Niw3ICsyNzY2LDcgQEAgc3ViIEdlbmVyYXRlUGFy YW1ldGVyc0NoZWNrCiAgICAgICAgICAgICAgICAgJHBhcmFtZXRlckRlZmF1bHRQb2xpY3kgPSAi RGVmYXVsdElzTnVsbFN0cmluZyI7CiAgICAgICAgICAgICB9CiAKLSAgICAgICAgICAgIHB1c2go QCRvdXRwdXRBcnJheSwgIiAgICAiIC4gR2V0TmF0aXZlVHlwZUZyb21TaWduYXR1cmUoJHBhcmFt ZXRlcikgLiAiICRuYW1lKCIgLiBKU1ZhbHVlVG9OYXRpdmUoJHBhcmFtZXRlciwgIk1BWUJFX01J U1NJTkdfUEFSQU1FVEVSKGV4ZWMsICRhcmdzSW5kZXgsICRwYXJhbWV0ZXJEZWZhdWx0UG9saWN5 KSIpIC4gIik7XG4iKTsKKyAgICAgICAgICAgIHB1c2goQCRvdXRwdXRBcnJheSwgIiAgICAiIC4g R2V0TmF0aXZlVHlwZUZyb21TaWduYXR1cmUoJHBhcmFtZXRlcikgLiAiICRuYW1lKCIgLiBKU1Zh bHVlVG9OYXRpdmUoJHBhcmFtZXRlciwgIk1BWUJFX01JU1NJTkdfUEFSQU1FVEVSKGV4ZWMsICRh cmdzSW5kZXgsICRwYXJhbWV0ZXJEZWZhdWx0UG9saWN5KSIsICRpbXBsQ2xhc3NOYW1lKSAuICIp O1xuIik7CiAKICAgICAgICAgICAgICMgSWYgYSBwYXJhbWV0ZXIgaXMgImFuIGluZGV4IiBhbmQg aXQncyBuZWdhdGl2ZSBpdCBzaG91bGQgdGhyb3cgYW4gSU5ERVhfU0laRV9FUlIgZXhjZXB0aW9u LgogICAgICAgICAgICAgIyBCdXQgdGhpcyBuZWVkcyB0byBiZSBkb25lIGluIHRoZSBiaW5kaW5n cywgYmVjYXVzZSB0aGUgdHlwZSBpcyB1bnNpZ25lZCBhbmQgdGhlIGZhY3QgdGhhdCBpdApAQCAt MzE0MSw2ICszMTQxLDcgQEAgc3ViIEpTVmFsdWVUb05hdGl2ZQogewogICAgIG15ICRzaWduYXR1 cmUgPSBzaGlmdDsKICAgICBteSAkdmFsdWUgPSBzaGlmdDsKKyAgICBteSAkaW1wbENsYXNzTmFt ZSA9IHNoaWZ0OwogCiAgICAgbXkgJGNvbmRpdGlvbmFsID0gJHNpZ25hdHVyZS0+ZXh0ZW5kZWRB dHRyaWJ1dGVzLT57IkNvbmRpdGlvbmFsIn07CiAgICAgbXkgJHR5cGUgPSAkY29kZUdlbmVyYXRv ci0+U3RyaXBNb2R1bGUoJHNpZ25hdHVyZS0+dHlwZSk7CkBAIC0zMTU5LDE3ICszMTYwLDIxIEBA IHN1YiBKU1ZhbHVlVG9OYXRpdmUKICAgICAgICAgIyBGSVhNRTogVGhpcyBpbXBsZW1lbnRzIFtU cmVhdE51bGxBcz1OdWxsU3RyaW5nXSBhbmQgW1RyZWF0VW5kZWZpbmVkQXM9TnVsbFN0cmluZ10s CiAgICAgICAgICMgYnV0IHRoZSBXZWIgSURMIHNwZWMgcmVxdWlyZXMgW1RyZWF0TnVsbEFzPUVt cHR5U3RyaW5nXSBhbmQgW1RyZWF0VW5kZWZpbmVkQXM9RW1wdHlTdHJpbmddLgogICAgICAgICBp ZiAoKCRzaWduYXR1cmUtPmV4dGVuZGVkQXR0cmlidXRlcy0+eyJUcmVhdE51bGxBcyJ9IGFuZCAk c2lnbmF0dXJlLT5leHRlbmRlZEF0dHJpYnV0ZXMtPnsiVHJlYXROdWxsQXMifSBlcSAiTnVsbFN0 cmluZyIpIGFuZCAoJHNpZ25hdHVyZS0+ZXh0ZW5kZWRBdHRyaWJ1dGVzLT57IlRyZWF0VW5kZWZp bmVkQXMifSBhbmQgJHNpZ25hdHVyZS0+ZXh0ZW5kZWRBdHRyaWJ1dGVzLT57IlRyZWF0VW5kZWZp bmVkQXMifSBlcSAiTnVsbFN0cmluZyIpKSB7Ci0gICAgICAgICAgICByZXR1cm4gInZhbHVlVG9T dHJpbmdXaXRoVW5kZWZpbmVkT3JOdWxsQ2hlY2soZXhlYywgJHZhbHVlKSIKKyAgICAgICAgICAg IHJldHVybiAidmFsdWVUb1N0cmluZ1dpdGhVbmRlZmluZWRPck51bGxDaGVjayhleGVjLCAkdmFs dWUpIjsKICAgICAgICAgfQogICAgICAgICBpZiAoKCRzaWduYXR1cmUtPmV4dGVuZGVkQXR0cmli dXRlcy0+eyJUcmVhdE51bGxBcyJ9IGFuZCAkc2lnbmF0dXJlLT5leHRlbmRlZEF0dHJpYnV0ZXMt PnsiVHJlYXROdWxsQXMifSBlcSAiTnVsbFN0cmluZyIpIG9yICRzaWduYXR1cmUtPmV4dGVuZGVk QXR0cmlidXRlcy0+eyJSZWZsZWN0In0pIHsKLSAgICAgICAgICAgIHJldHVybiAidmFsdWVUb1N0 cmluZ1dpdGhOdWxsQ2hlY2soZXhlYywgJHZhbHVlKSIKKyAgICAgICAgICAgIHJldHVybiAidmFs dWVUb1N0cmluZ1dpdGhOdWxsQ2hlY2soZXhlYywgJHZhbHVlKSI7CiAgICAgICAgIH0KICAgICAg ICAgIyBGSVhNRTogQWRkIHRoZSBjYXNlIGZvciAnaWYgKCRzaWduYXR1cmUtPmV4dGVuZGVkQXR0 cmlidXRlcy0+eyJUcmVhdFVuZGVmaW5lZEFzIn0gYW5kICRzaWduYXR1cmUtPmV4dGVuZGVkQXR0 cmlidXRlcy0+eyJUcmVhdFVuZGVmaW5lZEFzIn0gZXEgIk51bGxTdHJpbmciKSknLgogICAgICAg ICByZXR1cm4gIiR2YWx1ZS5pc0VtcHR5KCkgPyBTdHJpbmcoKSA6ICR2YWx1ZS50b1N0cmluZyhl eGVjKS0+dmFsdWUoZXhlYykiOwogICAgIH0KIAogICAgIGlmICgkdHlwZSBlcSAiRE9NT2JqZWN0 IiBvciAkdHlwZSBlcSAiYW55IikgewotICAgICAgICByZXR1cm4gImV4ZWMtPmdsb2JhbERhdGEo KSwgJHZhbHVlIjsKKyAgICAgICAgaWYgKCRpbXBsQ2xhc3NOYW1lIGVxICJIVE1MRWxlbWVudCIp IHsKKyAgICAgICAgICAgIHJldHVybiAidmFsdWVUb1N0cmluZ1dpdGhOdWxsQ2hlY2soZXhlYywg JHZhbHVlKSI7CisgICAgICAgIH0gZWxzZSB7CisgICAgICAgICAgICByZXR1cm4gImV4ZWMtPmds b2JhbERhdGEoKSwgJHZhbHVlIjsKKyAgICAgICAgfQogICAgIH0KIAogICAgIGlmICgkdHlwZSBl cSAiTm9kZUZpbHRlciIpIHsKQEAgLTMzMDAsNiArMzMwNSw4IEBAIHN1YiBOYXRpdmVUb0pTVmFs dWUKICAgICBpZiAoJHR5cGUgZXEgIkRPTU9iamVjdCIgb3IgJHR5cGUgZXEgImFueSIpIHsKICAg ICAgICAgaWYgKCRpbXBsQ2xhc3NOYW1lIGVxICJEb2N1bWVudCIpIHsKICAgICAgICAgICAgIEFk ZFRvSW1wbEluY2x1ZGVzKCJKU0NhbnZhc1JlbmRlcmluZ0NvbnRleHQyRC5oIiwgJGNvbmRpdGlv bmFsKTsKKyAgICAgICAgfSBlbHNpZiAoJGltcGxDbGFzc05hbWUgZXEgIkhUTUxFbGVtZW50Iikg eworICAgICAgICAgICAgQWRkVG9JbXBsSW5jbHVkZXMoIkpTTWljcm9EYXRhSXRlbVZhbHVlLmgi LCAkY29uZGl0aW9uYWwpOwogICAgICAgICB9IGVsc2UgewogICAgICAgICAgICAgcmV0dXJuICIo JHZhbHVlLmhhc05vVmFsdWUoKSA/IGpzTnVsbCgpIDogJHZhbHVlLmpzVmFsdWUoKSkiOwogICAg ICAgICB9Cg==