Bug 73156 - [Microdata] Implement HTMLPropertiesCollection collection.namedItem()
Summary: [Microdata] Implement HTMLPropertiesCollection collection.namedItem()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 68609
  Show dependency treegraph
 
Reported: 2011-11-25 22:37 PST by Arko Saha
Modified: 2012-03-07 23:22 PST (History)
12 users (show)

See Also:


Attachments
Patch (31.01 KB, patch)
2011-11-25 22:46 PST, Arko Saha
sam: review-
Details | Formatted Diff | Diff
Updated patch (27.52 KB, patch)
2011-11-28 03:03 PST, Arko Saha
abarth: review-
Details | Formatted Diff | Diff
Updated patch (22.95 KB, patch)
2011-11-29 07:56 PST, Arko Saha
abarth: review-
Details | Formatted Diff | Diff
Updated patch (26.09 KB, patch)
2012-01-30 04:08 PST, Arko Saha
no flags Details | Formatted Diff | Diff
Updated patch (17.78 KB, patch)
2012-02-13 05:54 PST, Arko Saha
haraken: review-
Details | Formatted Diff | Diff
Updated patch (18.17 KB, patch)
2012-02-28 00:23 PST, Arko Saha
haraken: commit-queue-
Details | Formatted Diff | Diff
Updated patch (19.52 KB, patch)
2012-02-28 06:34 PST, Arko Saha
haraken: commit-queue-
Details | Formatted Diff | Diff
Updated patch (17.74 KB, patch)
2012-02-28 22:26 PST, Arko Saha
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Arko Saha 2011-11-25 22:37:13 PST
Support for:
1. collection.namedItem()
2. collection(name)
3. collection[name]
4. collection(index)
5. collection[index]

Specification: http://www.whatwg.org/specs/web-apps/current-work/multipage/common-dom-interfaces.html#htmlpropertiescollection
Comment 1 Arko Saha 2011-11-25 22:46:50 PST
Created attachment 116650 [details]
Patch
Comment 2 Adam Barth 2011-11-25 22:52:51 PST
Why do we need custom bindings?  All custom bindings are very buggy.
Comment 3 Sam Weinig 2011-11-26 21:14:53 PST
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 4 Sam Weinig 2011-11-26 21:16:15 PST
Comment on attachment 116650 [details]
Patch

r-ing for removing Call support.
Comment 5 Arko Saha 2011-11-28 02:34:57 PST
(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.
Comment 6 Arko Saha 2011-11-28 02:35:45 PST
(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.
Comment 7 Arko Saha 2011-11-28 03:03:22 PST
Created attachment 116721 [details]
Updated patch
Comment 8 Adam Barth 2011-11-28 10:05:36 PST
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?
Comment 9 Arko Saha 2011-11-29 07:56:05 PST
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.
Comment 10 Arko Saha 2012-01-24 22:13:56 PST
Hi Adam Barth, can you please review the patch? Thanks.
Comment 11 Adam Barth 2012-01-24 22:25:24 PST
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?
Comment 12 Arko Saha 2012-01-30 03:33:40 PST
(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;
}
Comment 13 Arko Saha 2012-01-30 04:08:07 PST
Created attachment 124521 [details]
Updated patch

Resolving merge conflicts. Adding JSHTMLPropertiesCollectionCustom.cpp file in xcode project.
Comment 14 Arko Saha 2012-02-13 05:54:04 PST
Created attachment 126761 [details]
Updated patch

Modified code generator to generate code for HTMLPropertiesCollection NamedGetter property.
Comment 15 Kentaro Hara 2012-02-26 23:52:45 PST
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
Comment 16 Arko Saha 2012-02-27 01:43:21 PST
(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 17 Kentaro Hara 2012-02-27 03:29:53 PST
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].)
Comment 18 Arko Saha 2012-02-28 00:22:03 PST
(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.
Comment 19 Arko Saha 2012-02-28 00:23:09 PST
Created attachment 129205 [details]
Updated patch
Comment 20 Kentaro Hara 2012-02-28 00:45:09 PST
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.
Comment 21 Arko Saha 2012-02-28 00:56:38 PST
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.
Comment 22 Kentaro Hara 2012-02-28 00:59:44 PST
(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 ==.
Comment 23 Arko Saha 2012-02-28 01:02:44 PST
(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.
Comment 24 Arko Saha 2012-02-28 06:34:42 PST
Created attachment 129238 [details]
Updated patch

Incorporating review comments.
Comment 25 Kentaro Hara 2012-02-28 07:45:14 PST
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?
Comment 26 Arko Saha 2012-02-28 22:21:31 PST
(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.
Comment 27 Arko Saha 2012-02-28 22:26:29 PST
Created attachment 129388 [details]
Updated patch
Comment 28 Kentaro Hara 2012-02-28 22:30:10 PST
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.
Comment 29 Arko Saha 2012-02-28 22:46:57 PST
(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.
Comment 30 WebKit Review Bot 2012-02-28 23:23:07 PST
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 31 WebKit Review Bot 2012-02-28 23:26:09 PST
Comment on attachment 129388 [details]
Updated patch

Clearing flags on attachment: 129388

Committed r109200: <http://trac.webkit.org/changeset/109200>
Comment 32 WebKit Review Bot 2012-02-28 23:26:16 PST
All reviewed patches have been landed.  Closing bug.
Comment 33 gur.trio 2012-03-05 04:03:48 PST
Please add the equivalent for CodeGeneratorV8.pm
Comment 34 gur.trio 2012-03-07 23:22:02 PST
Please ignore the Comment #33. Not required