Bug 126684 – Switch HTMLTableRowsCollection from Traversal<> to iterators

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

[Antti Koivisto]

This is the last remaining client of Traversal<> outside the iterator implementation.

[Antti Koivisto]

Created attachment 220705 [details]
patch

[Andreas Kling]

Comment on attachment 220705 [details]
patch

Very snug.

[Antti Koivisto]

https://trac.webkit.org/r161551

[Darin Adler]

Comment on attachment 220705 [details]
patch

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

> Source/WebCore/dom/ElementChildIterator.h:160
> +    if (!isElementOfType<const ElementType>(child))
> +        return end();

If this element is a child of m_parent, just not the correct type, I would expect this function to return the next child of the correct type if any, not always end().

A way to avoid this problem entirely would be to make the argument be ElementType& instead of const Element&.

[Darin Adler]

Comment on attachment 220705 [details]
patch

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

>> Source/WebCore/dom/ElementChildIterator.h:160
>> +        return end();
> 
> If this element is a child of m_parent, just not the correct type, I would expect this function to return the next child of the correct type if any, not always end().
> 
> A way to avoid this problem entirely would be to make the argument be ElementType& instead of const Element&.

I guess I’m wrong. We are using end() here as a sort of null value.

But I do think that we should have a find that takes the specific element type to avoid the runtime isElementOfType check.

[Darin Adler]

Comment on attachment 220705 [details]
patch

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

>>> Source/WebCore/dom/ElementChildIterator.h:160
>>> +        return end();
>> 
>> If this element is a child of m_parent, just not the correct type, I would expect this function to return the next child of the correct type if any, not always end().
>> 
>> A way to avoid this problem entirely would be to make the argument be ElementType& instead of const Element&.
> 
> I guess I’m wrong. We are using end() here as a sort of null value.
> 
> But I do think that we should have a find that takes the specific element type to avoid the runtime isElementOfType check.

I guess I really don’t like find for the name of this. It sounds like something that iterates the children, when really it’s just a sort of alternate iterator constructor.

[Antti Koivisto]

(In reply to comment #6)
> I guess I really don’t like find for the name of this. It sounds like something that iterates the children, when really it’s just a sort of alternate iterator constructor.

I mostly added it because we had find like this already in ElementDescendantIteratorAdapter. I agree that it is a silly name and should be replaced with a constructor function. I don't want to call constructors explicitly from regular code though. Maybe FooAdapter::iteratorFor(ElementType&)?