Bug 73156 - [Microdata] Implement HTMLPropertiesCollection collection.namedItem()
: [Microdata] Implement HTMLPropertiesCollection collection.namedItem()
Status: RESOLVED FIXED
: WebKit
HTML DOM
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
:
:
:
: 68609
  Show dependency treegraph
 
Reported: 2011-11-25 22:37 PST by
Modified: 2012-03-07 23:22 PST (History)


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


Note

You need to log in before you can comment on or make changes to this bug.


Description From 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 From 2011-11-25 22:46:50 PST -------
Created an attachment (id=116650) [details]
Patch
------- Comment #2 From 2011-11-25 22:52:51 PST -------
Why do we need custom bindings?  All custom bindings are very buggy.
------- Comment #3 From 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 From 2011-11-26 21:16:15 PST -------
(From update of attachment 116650 [details])
r-ing for removing Call support.
------- Comment #5 From 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 From 2011-11-28 02:35:45 PST -------
(In reply to comment #4)
> (From update of attachment 116650 [details] [details])
> r-ing for removing Call support.

Ok. I will remove the CustomCall support for HTMLProprtiesCollection.
------- Comment #7 From 2011-11-28 03:03:22 PST -------
Created an attachment (id=116721) [details]
Updated patch
------- Comment #8 From 2011-11-28 10:05:36 PST -------
(From update of attachment 116721 [details])
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 From 2011-11-29 07:56:05 PST -------
Created an attachment (id=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 From 2012-01-24 22:13:56 PST -------
Hi Adam Barth, can you please review the patch? Thanks.
------- Comment #11 From 2012-01-24 22:25:24 PST -------
(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?

> 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 From 2012-01-30 03:33:40 PST -------
(In reply to comment #11)
My comments are inline.

> (From update of attachment 116969 [details] [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 From 2012-01-30 04:08:07 PST -------
Created an attachment (id=124521) [details]
Updated patch

Resolving merge conflicts. Adding JSHTMLPropertiesCollectionCustom.cpp file in xcode project.
------- Comment #14 From 2012-02-13 05:54:04 PST -------
Created an attachment (id=126761) [details]
Updated patch

Modified code generator to generate code for HTMLPropertiesCollection NamedGetter property.
------- Comment #15 From 2012-02-26 23:52:45 PST -------
(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.

> 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 From 2012-02-27 01:43:21 PST -------
(In reply to comment #15)
> (From update of attachment 126761 [details] [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 From 2012-02-27 03:29:53 PST -------
(From update of attachment 126761 [details])
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 From 2012-02-28 00:22:03 PST -------
(In reply to comment #17)
> (From update of attachment 126761 [details] [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 From 2012-02-28 00:23:09 PST -------
Created an attachment (id=129205) [details]
Updated patch
------- Comment #20 From 2012-02-28 00:45:09 PST -------
(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);

> 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 From 2012-02-28 00:56:38 PST -------
Thanks for the review.

(In reply to comment #20)
> (From update of attachment 129205 [details] [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 From 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 From 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 From 2012-02-28 06:34:42 PST -------
Created an attachment (id=129238) [details]
Updated patch

Incorporating review comments.
------- Comment #25 From 2012-02-28 07:45:14 PST -------
(From update of attachment 129238 [details])
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 From 2012-02-28 22:21:31 PST -------
(In reply to comment #25)
> (From update of attachment 129238 [details] [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 From 2012-02-28 22:26:29 PST -------
Created an attachment (id=129388) [details]
Updated patch
------- Comment #28 From 2012-02-28 22:30:10 PST -------
(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.
------- Comment #29 From 2012-02-28 22:46:57 PST -------
(In reply to comment #28)
> (From update of attachment 129388 [details] [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 From 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 From 2012-02-28 23:26:09 PST -------
(From update of attachment 129388 [details])
Clearing flags on attachment: 129388

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