Bug 150347 – Use tighter typing for collections / node lists' item() / namedItem() methods

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Chris Dumez]

Use tighter typing for collections / node lists' item() / namedItem() methods

[Chris Dumez]

Created attachment 263540 [details]
Patch

[Darin Adler]

Comment on attachment 263540 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=263540&action=review

More elegant; seems good.

A little sad to have to remove the final from some of the item functions, just so we can narrow the type in a derived class. We don’t gain anything by calling the derived class’s override when calling the function on the base class at runtime except maybe an assertion in debug builds. But I can’t think of a simple alternative.

These function bodies are messy enough that I would prefer to see them after the classes rather than inside the class definitions.

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:1783
> +    for (unsigned i = 0; auto* current = links->item(i); ++i) {

Would be nice to eventually come up with a cleaner idiom for iterating an HTMLCollection in C++ code. We could probably make begin/end and iterators to make this work with a modern for loop.

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:1786
> +            RefPtr<AccessibilityObject> axObject = document.axObjectCache()->getOrCreate(renderer);
> +            ASSERT(axObject);

Makes me wonder why getOrCreate doesn’t return a Ref.

> Source/WebCore/html/HTMLSelectElement.cpp:458
> +        before = options()->item(index + 1);

Seems like we could call our own item() here rather than explicitly calling options()->item().

> Source/WebCore/html/HTMLTableRowElement.cpp:136
> -        RefPtr<Node> cell = children->item(index);
> +        RefPtr<Element> cell = children->item(index);

I suggest removing this local variable entirely so we don’t have to state the type.

> Source/WebCore/html/HTMLTableSectionElement.cpp:91
> -        RefPtr<Node> row = children->item(index);
> +        RefPtr<Element> row = children->item(index);

I suggest removing this local variable entirely so we don’t have to state the type.

> Source/WebCore/html/RadioNodeList.h:46
> +    HTMLElement* item(unsigned offset) const override { return downcast<HTMLElement>(CachedLiveNodeList<RadioNodeList>::item(offset)); }
> +
> +    virtual ~RadioNodeList();

I think the override should come after the destructor, not before.

[Chris Dumez]

Comment on attachment 263540 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=263540&action=review

>> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:1783
>> +    for (unsigned i = 0; auto* current = links->item(i); ++i) {
> 
> Would be nice to eventually come up with a cleaner idiom for iterating an HTMLCollection in C++ code. We could probably make begin/end and iterators to make this work with a modern for loop.

Agreed.

>> Source/WebCore/html/HTMLSelectElement.cpp:458
>> +        before = options()->item(index + 1);
> 
> Seems like we could call our own item() here rather than explicitly calling options()->item().

Ok.

>> Source/WebCore/html/HTMLTableRowElement.cpp:136
>> +        RefPtr<Element> cell = children->item(index);
> 
> I suggest removing this local variable entirely so we don’t have to state the type.

Ok

>> Source/WebCore/html/HTMLTableSectionElement.cpp:91
>> +        RefPtr<Element> row = children->item(index);
> 
> I suggest removing this local variable entirely so we don’t have to state the type.

Ok.

>> Source/WebCore/html/RadioNodeList.h:46
>> +    virtual ~RadioNodeList();
> 
> I think the override should come after the destructor, not before.

Ok.

[Chris Dumez]

Created attachment 263592 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 263592 [details]
Patch

Clearing flags on attachment: 263592

Committed r191351: <http://trac.webkit.org/changeset/191351>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Simon Fraser (smfr)]

This broke EFL:
DerivedSources/webkitdom/WebKitDOMHTMLFormElement.cpp:31:56: fatal error: WebKitDOMHTMLFormControlsCollectionPrivate.h: No such file or directory
 #include "WebKitDOMHTMLFormControlsCollectionPrivate.h"