Bug 89923 – Get rid of firstItem and nextItem from HTMLCollection

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

[Ryosuke Niwa]

These two methods can be safely replaced by item() to reduce the interface surface.

[Ryosuke Niwa]

Created attachment 149401 [details]
Cleanup; also improve some insanely inefficient code

[Ryosuke Niwa]

Comment on attachment 149401 [details]
Cleanup; also improve some insanely inefficient code

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

> Source/WebCore/html/HTMLCollection.cpp:-202
> -     invalidateCacheIfNeeded();

Note I'm fixing 5-space indentation here.

[Adam Klein]

Comment on attachment 149401 [details]
Cleanup; also improve some insanely inefficient code

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

> Source/WebCore/html/HTMLCollection.cpp:-223
> -     return item(0);

Rather than deleting this entirely, why not just make it inline? firstItem() reads better to me than item(0) (compare to Vector::at(0) vs Vector::first())

[WebKit Review Bot]

Comment on attachment 149401 [details]
Cleanup; also improve some insanely inefficient code

Attachment 149401 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13101240

[Ryosuke Niwa]

(In reply to comment #3)
> (From update of attachment 149401 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=149401&action=review
> 
> > Source/WebCore/html/HTMLCollection.cpp:-223
> > -     return item(0);
> 
> Rather than deleting this entirely, why not just make it inline? firstItem() reads better to me than item(0) (compare to Vector::at(0) vs Vector::first())

I think it's better to have fewer methods.

[Ryosuke Niwa]

Created attachment 149446 [details]
Fixed Chromium build

[WebKit Review Bot]

Comment on attachment 149446 [details]
Fixed Chromium build

Attachment 149446 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13099278

[Ryosuke Niwa]

Created attachment 149449 [details]
Fixed more Chromium build failures

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[WebKit Review Bot]

Comment on attachment 149449 [details]
Fixed more Chromium build failures

Attachment 149449 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13090379

[Ryosuke Niwa]

Created attachment 149450 [details]
Another Chromium build fix

[WebKit Review Bot]

Comment on attachment 149450 [details]
Another Chromium build fix

Attachment 149450 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13101292

[Ryosuke Niwa]

Created attachment 149455 [details]
It must compile this time

[Andreas Kling]

Comment on attachment 149455 [details]
It must compile this time

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

r=me assuming it builds etc.

> Source/WebCore/html/HTMLCollection.h:56
> +    bool hasAnyItem() const

I'd call this hasItems().

[WebKit Review Bot]

Comment on attachment 149455 [details]
It must compile this time

Attachment 149455 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13101300

[Ryosuke Niwa]

Committed r121232: <http://trac.webkit.org/changeset/121232>

[Darin Adler]

Comment on attachment 149455 [details]
It must compile this time

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

> Source/WebCore/html/HTMLCollection.h:65
> +    bool hasExactlyOneItem() const
> +    {
> +        invalidateCacheIfNeeded();
> +        return (m_cache.hasLength && m_cache.length == 1) || (m_cache.current && !itemAfter(m_cache.current)) || (item(0) && !item(1));
> +    }

This looks like it could return true for a list with two items if hasLength is false, m_cache.current is 2, and itemAfter(2) is 0. Is there some reason that’s not a problem?

[Darin Adler]

Comment on attachment 149455 [details]
It must compile this time

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

>> Source/WebCore/html/HTMLCollection.h:65
>> +    }
> 
> This looks like it could return true for a list with two items if hasLength is false, m_cache.current is 2, and itemAfter(2) is 0. Is there some reason that’s not a problem?

Sorry, let me say that again:

This looks like it could return true for a collection with two items if m_cache.hasLength is false and m_cache.current is 1. If so, there is a bug here. If not, why can’t this happen?

[Ryosuke Niwa]

Comment on attachment 149455 [details]
It must compile this time

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

>>> Source/WebCore/html/HTMLCollection.h:65
>>> +    }
>> 
>> This looks like it could return true for a list with two items if hasLength is false, m_cache.current is 2, and itemAfter(2) is 0. Is there some reason that’s not a problem?
> 
> Sorry, let me say that again:
> 
> This looks like it could return true for a collection with two items if m_cache.hasLength is false and m_cache.current is 1. If so, there is a bug here. If not, why can’t this happen?

Oops, you're right. I should have done !item(m_cache. position + 1) instead of !itemAfter(m_cache.current)