Bug 73156

Created attachment 116650 [details] Patch

Why do we need custom bindings? All custom bindings are very buggy.

Please don't implement the CustomCall aspect of HTMLProprtiesCollection. We have much of our other call support on collections and don't want to add it here.

Comment on attachment 116650 [details] Patch r-ing for removing Call support.

(In reply to comment #2) > Why do we need custom bindings? All custom bindings are very buggy. 1. HTMLPropertiesCollection is derived from HTMLCollection. namedItem() method is virtual and returns Node* in HTMLCollection. But as per the specification HTMLPropertiesCollection namedItem() should return PropertyNodeList, which is a different hierarchy i.e, its a simple standalone structure. So I implemented namedItem() method as custom to return NodeList for now. 2. The patch also adds NameGetter support for HTMLPropertiesCollection. So we need to implement this as custom bindings. Please let me know your thoughts on this.

(In reply to comment #4) > (From update of attachment 116650 [details]) > r-ing for removing Call support. Ok. I will remove the CustomCall support for HTMLProprtiesCollection.

Created attachment 116721 [details] Updated patch

Comment on attachment 116721 [details] Updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=116721&action=review We should be able to do this without custom bindings code. > Source/WebCore/bindings/v8/custom/V8HTMLPropertiesCollectionCustom.cpp:64 > + if (!info.Holder()->GetRealNamedPropertyInPrototypeChain(name).IsEmpty()) > + return notHandledByInterceptor(); > + if (info.Holder()->HasRealNamedCallbackProperty(name)) > + return notHandledByInterceptor(); This pattern is very subtle. Are you sure you've got it right?

Created attachment 116969 [details] Updated patch 1. Removed custom bindings code for namedItem() method. 2. Removed V8 custom binding code for NameGetter as V8binding takes care of it.

Hi Adam Barth, can you please review the patch? Thanks.

Comment on attachment 116969 [details] Updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=116969&action=review This patch confused me. It looks like there's a bunch of extra code in here that we don't need anymore. Am I confused? > Source/WebCore/html/HTMLPropertiesCollection.cpp:202 > + // FIXME: HTML5 specifies that this should return PropertyNodeList. > + return !namedItems.isEmpty() ? StaticNodeList::adopt(namedItems) : 0; So, the nodelist this returns isn't live? It's just static? > Source/WebCore/html/HTMLPropertiesCollection.h:54 > + bool canGetItemsForName(const String& name) const; I'm confused. Who calls this function? > Source/WebCore/html/HTMLPropertiesCollection.idl:44 > + NodeList namedItem(in DOMString name); This isn't marked Custom anymore, but you still have the custom bindings code in your patch. Can we remove that code?

(In reply to comment #11) My comments are inline. > (From update of attachment 116969 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=116969&action=review > > This patch confused me. It looks like there's a bunch of extra code in here that we don't need anymore. Am I confused? > > > Source/WebCore/html/HTMLPropertiesCollection.cpp:202 > > + // FIXME: HTML5 specifies that this should return PropertyNodeList. > > + return !namedItems.isEmpty() ? StaticNodeList::adopt(namedItems) : 0; > > So, the nodelist this returns isn't live? It's just static? Yes. currently its static, similar as HTMLCollection::namedItem() implementation. HTML5 specifies that this should return PropertyNodeList which is yet to be implemented and will be covered in the upcoming patch. > > Source/WebCore/html/HTMLPropertiesCollection.idl:44 > > + NodeList namedItem(in DOMString name); > > This isn't marked Custom anymore, but you still have the custom bindings code in your patch. Can we remove that code? namedItem() method is not custom here and I have removed the related code. The JS custom bindings code is needed for HasNameGetter support for HTMLPropertiesCollection. example: element.properties['foo'] > > Source/WebCore/html/HTMLPropertiesCollection.h:54 > > + bool canGetItemsForName(const String& name) const; > > I'm confused. Who calls this function? This function is getting called from JS custom bindings. While retrieving collection[name] (element.properties['foo']) in JavaScript -> JSHTMLPropertiesCollection::getOwnPropertySlot() calls canGetItemsForName()[JSHTMLPropertiesCollectionCustom.cpp] to check if the items can be retrieved or not based on the name argument passed. if (canGetItemsForName(exec, static_cast<HTMLPropertiesCollection*>(thisObject->impl()), propertyName)) { slot.setCustom(thisObject, thisObject->nameGetter); return true; }

Created attachment 124521 [details] Updated patch Resolving merge conflicts. Adding JSHTMLPropertiesCollectionCustom.cpp file in xcode project.

Created attachment 126761 [details] Updated patch Modified code generator to generate code for HTMLPropertiesCollection NamedGetter property.

Comment on attachment 126761 [details] Updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=126761&action=review > Source/WebCore/ChangeLog:7 > + Please describe what the patch is doing. > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2235 > + if ($interfaceName eq "HTMLPropertiesCollection") { Maybe you can remove the hard-coding by using [CustomNamedGetter] (or [JSCustomNamedGetter]). https://trac.webkit.org/wiki/WebKitIDL#CustomNamedGetter

(In reply to comment #15) > (From update of attachment 126761 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=126761&action=review > > > Source/WebCore/ChangeLog:7 > > + > > Please describe what the patch is doing. Ok, I will update the Changelog. > > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2235 > > + if ($interfaceName eq "HTMLPropertiesCollection") { > > Maybe you can remove the hard-coding by using [CustomNamedGetter] (or [JSCustomNamedGetter]). https://trac.webkit.org/wiki/WebKitIDL#CustomNamedGetter To use [CustomNamedGetter] or [NamedGetter] we need to define JSPropertiesCollection::canGetItemsForName() and JSPropertiesCollection::nameGetter() in JSPropertiesCollectionCustom.cpp Please check my previous patch https://bugs.webkit.org/attachment.cgi?id=124521&action=prettypatch. Here I have used [NamedGetter] and defined above functions in JSPropertiesCollection.cpp. Is this approach is correct?

Comment on attachment 126761 [details] Updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=126761&action=review >>> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2235 >>> + if ($interfaceName eq "HTMLPropertiesCollection") { >> >> Maybe you can remove the hard-coding by using [CustomNamedGetter] (or [JSCustomNamedGetter]). https://trac.webkit.org/wiki/WebKitIDL#CustomNamedGetter > > To use [CustomNamedGetter] or [NamedGetter] we need to define JSPropertiesCollection::canGetItemsForName() and JSPropertiesCollection::nameGetter() in JSPropertiesCollectionCustom.cpp > > Please check my previous patch https://bugs.webkit.org/attachment.cgi?id=124521&action=prettypatch. Here I have used [NamedGetter] and defined above functions in JSPropertiesCollection.cpp. Is this approach is correct? Sorry, I was confused. You are doing the right thing. The core problem would be that CodeGeneratorJS.pm does not yet implement non-custom named getters. Currently, no matter if it is [NamedGetter] or [CustomNamedGetter], we need to write custom bindings to WebCore/bindings/js/JS*Custom.cpp. And your patch is trying to avoid the custom bindings (only) for HTMLPropertiesCollection... right? In that case, hard-coding "HTMLPropertiesCollection" would make sense as a first step. (Eventually we should fix CodeGeneratorJS.pm so that it generates the bindings code automatically for [NamedGetter].)

(In reply to comment #17) > (From update of attachment 126761 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=126761&action=review > The core problem would be that CodeGeneratorJS.pm does not yet implement non-custom named getters. Currently, no matter if it is [NamedGetter] or [CustomNamedGetter], we need to write custom bindings to WebCore/bindings/js/JS*Custom.cpp. And your patch is trying to avoid the custom bindings (only) for HTMLPropertiesCollection... right? In that case, hard-coding "HTMLPropertiesCollection" would make sense as a first step. (Eventually we should fix CodeGeneratorJS.pm so that it generates the bindings code automatically for [NamedGetter].) Yes, currently CodeGeneratorJS.pm does not generate JS bindings code for [NamedGetter]. In my current patch I have modified the CodeGeneratorJS.pm to avoid JS custom bindings code only for HTMLPropertiesCollection. I will log a separate bug and will fix CodeGeneratorJS.pm. So that bindings code for [NamedGetter] should get generated automatically for all the interfaces.

Created attachment 129205 [details] Updated patch

Comment on attachment 129205 [details] Updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=129205&action=review Almost looks good to me. Thanks for the patch. > Source/WebCore/html/HTMLPropertiesCollection.cpp:202 > + return !namedItems.isEmpty() ? StaticNodeList::adopt(namedItems) : 0; Nit: return namedItem.isEmpty() ? 0 : StaticNodeList::adopt(namedItems); > Source/WebCore/html/HTMLPropertiesCollection.cpp:208 > + getNamedItems(namedItems, name); Just an out-of-curious question: Don't you have any concern about the performance of hasNamedItem()? It is a bit wasteful to completely get all named items just to know whether there is at least one item. If you have any concern, maybe you can optimize it (e.g. std::sort would not be necessary). If you do not have the concern, the current patch looks good to me. > Source/WebCore/html/HTMLPropertiesCollection.cpp:210 > + return !namedItems.isEmpty() ? true : false; Nit: return namedItems.isEmpty()? false : true; > Source/WebCore/html/HTMLPropertiesCollection.h:54 > + bool hasNamedItem(const AtomicString& name) const; Nit: 'name' is not necessary. > LayoutTests/fast/dom/MicroData/nameditem-must-be-case-sensitive.html:13 > +shouldBeTrue("element.properties.namedItem('foo').length == '2'"); shouldBe("element.properties.namedItem('foo').length", '2') might be better. It's up to you, but if you want to use shouldBeTrue(...), please use === instead of ==. The semantics of JavaScript == is really chaos. The same comment for other test cases.

Thanks for the review. (In reply to comment #20) > (From update of attachment 129205 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=129205&action=review > > Almost looks good to me. Thanks for the patch. > > > Source/WebCore/html/HTMLPropertiesCollection.cpp:202 > > + return !namedItems.isEmpty() ? StaticNodeList::adopt(namedItems) : 0; > > Nit: return namedItem.isEmpty() ? 0 : StaticNodeList::adopt(namedItems); Ok. > > Source/WebCore/html/HTMLPropertiesCollection.cpp:208 > > + getNamedItems(namedItems, name); > > Just an out-of-curious question: Don't you have any concern about the performance of hasNamedItem()? It is a bit wasteful to completely get all named items just to know whether there is at least one item. If you have any concern, maybe you can optimize it (e.g. std::sort would not be necessary). If you do not have the concern, the current patch looks good to me. > Good point. I will optimize hasNamedItem() as you suggested. > > Source/WebCore/html/HTMLPropertiesCollection.cpp:210 > > + return !namedItems.isEmpty() ? true : false; > > Nit: return namedItems.isEmpty()? false : true; Ok, will modify. > > Source/WebCore/html/HTMLPropertiesCollection.h:54 > > + bool hasNamedItem(const AtomicString& name) const; > > Nit: 'name' is not necessary. Ok. > > LayoutTests/fast/dom/MicroData/nameditem-must-be-case-sensitive.html:13 > > +shouldBeTrue("element.properties.namedItem('foo').length == '2'"); > > shouldBe("element.properties.namedItem('foo').length", '2') might be better. It's up to you, but if you want to use shouldBeTrue(...), please use === instead of ==. The semantics of JavaScript == is really chaos. > > The same comment for other test cases. Ok. I will use shouldBeTrue(...) in the test cases.

(In reply to comment #21) > > shouldBe("element.properties.namedItem('foo').length", '2') might be better. It's up to you, but if you want to use shouldBeTrue(...), please use === instead of ==. The semantics of JavaScript == is really chaos. > > > > The same comment for other test cases. > > Ok. I will use shouldBeTrue(...) in the test cases. (Maybe just your typo but for clarification...) I meant shouldBe(...) is better than shouldBeTrue(...) because shouldBe(...) uses === internally. If you want to use shouldBeTrue(...) for some reason, please use === instead of ==.

(In reply to comment #22) > (In reply to comment #21) > (Maybe just your typo but for clarification...) I meant shouldBe(...) is better than shouldBeTrue(...) because shouldBe(...) uses === internally. If you want to use shouldBeTrue(...) for some reason, please use === instead of ==. Sorry for the typo, I will use shouldBe(...) in the test cases as per your suggestion.

Created attachment 129238 [details] Updated patch Incorporating review comments.

Comment on attachment 129238 [details] Updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=129238&action=review > Source/WebCore/html/HTMLPropertiesCollection.cpp:210 > +bool HTMLPropertiesCollection::hasNamedItem(const AtomicString& name) const Thanks for the optimization! But I am a bit afraid that the optimization might be too complicated and error-prone. Now hasNamedItem() looks like a copy & paste of findPropertiesOfAnItem(). How about the following, more "modest" optimization? (I know this is less optimized than your patch though.) bool HTMLPropertiesCollection::hasNamedItem(const AtomicString& name) const { if (!base()->isHTMLElement() || !toHTMLElement(base())->fastHasAttribute(itemscopeAttr)) return false; m_properties.clear(); findPropetiesOfAnItem(base()); for (size_t i = 0; i < m_properties.size(); ++i) { DOMSettableTokenList* itemProperty = m_properties[i]->itemProp(); if (itemProperty->tokens().contains(name)) return true; } return false } Anyway, it might be a good idea to split the patch for implementing the named getter and the patch for optimizing it. How about committing this patch with the "modest" optimization for now? If you found that the hasNamedItem() performance is _really_ critical, you can try to optimize hasNamedItem() in another patch. WDYT?

(In reply to comment #25) > (From update of attachment 129238 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=129238&action=review > Anyway, it might be a good idea to split the patch for implementing the named getter and the patch for optimizing it. How about committing this patch with the "modest" optimization for now? If you found that the hasNamedItem() performance is _really_ critical, you can try to optimize hasNamedItem() in another patch. WDYT? Yes, its more modest optimization. For now we can commit the patch with this change. We will optimize hasNamedItem() in another patch as you suggested. Thanks for the guidance.

Created attachment 129388 [details] Updated patch

Comment on attachment 129388 [details] Updated patch Thank you very much for the iterative improvement! I am not sure if we really need further optimization. If you want to try it, I would recommend you to measure the performance using DROMAEO or SunSpider and see if hasNamedItem() has impact on the practical performance, before you start the optimization.

(In reply to comment #28) > (From update of attachment 129388 [details]) > Thank you very much for the iterative improvement! > > I am not sure if we really need further optimization. If you want to try it, I would recommend you to measure the performance using DROMAEO or SunSpider and see if hasNamedItem() has impact on the practical performance, before you start the optimization. Sure. I will start the optimization only if I found some performance improvement.

The commit-queue encountered the following flaky tests while processing attachment 129388 [details]: css3/filters/effect-invert-hw.html bug 79639 (author: cmarrin@apple.com) The commit-queue is continuing to process your patch.

Comment on attachment 129388 [details] Updated patch Clearing flags on attachment: 129388 Committed r109200: <http://trac.webkit.org/changeset/109200>

All reviewed patches have been landed. Closing bug.

Please add the equivalent for CodeGeneratorV8.pm

Please ignore the Comment #33. Not required