Bug 138556 - Minor tweaks to HTMLCollection
Summary: Minor tweaks to HTMLCollection
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Darin Adler
URL:
Keywords:
Depends on: 138620
Blocks:
  Show dependency treegraph
 
Reported: 2014-11-09 22:07 PST by Darin Adler
Modified: 2014-11-11 12:13 PST (History)
2 users (show)

See Also:


Attachments
Patch (53.05 KB, patch)
2014-11-09 22:28 PST, Darin Adler
cdumez: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Darin Adler 2014-11-09 22:07:57 PST
Minor tweaks to HTMLCollection
Comment 1 Darin Adler 2014-11-09 22:28:48 PST
Created attachment 241278 [details]
Patch
Comment 2 Chris Dumez 2014-11-09 23:18:19 PST
Comment on attachment 241278 [details]
Patch

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

nice, r=me

> Source/WebCore/html/HTMLCollection.cpp:451
>      for (unsigned i = 0; i < size; i++) {

nit: ++i

> Source/WebCore/html/HTMLFormControlsCollection.cpp:124
> +Element* HTMLFormControlsCollection::namedItem(const AtomicString& name) const

This could probably return an HTMLElement* even.

> Source/WebCore/html/HTMLFormControlsCollection.h:44
> +    virtual Element* namedItem(const AtomicString& name) const override;

Could likely return an HTMLElement* even.

> Source/WebCore/html/HTMLSelectElement.h:90
> +    Element* namedItem(const AtomicString& name);

Could likely return a HTMLOptionElement*

> Source/WebCore/html/HTMLSelectElement.h:91
> +    Element* item(unsigned index);

Ditto.

> Source/WebCore/page/scrolling/AxisScrollSnapOffsets.cpp:42
> +    for (Element* child = parent.firstElementChild(); child; child = child->nextElementSibling()) {

for (auto& child : childrenOfType<Element>(parent)) { ?
Comment 3 Darin Adler 2014-11-11 09:35:28 PST
Committed r175947: <http://trac.webkit.org/changeset/175947>
Comment 4 Chris Dumez 2014-11-11 12:00:47 PST
Seems to have caused assertions in debug builds:
https://build.webkit.org/results/Apple%20Mavericks%20Debug%20WK1%20(Tests)/r175947%20(8835)/results.html

I am looking now.
Comment 5 Chris Dumez 2014-11-11 12:04:58 PST
Comment on attachment 241278 [details]
Patch

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

> Source/WebCore/html/HTMLCollection.cpp:335
> +        for (traversedCount = 0; element && traversedCount < count; ++traversedCount)

I think the issue is that traversedCount gets incremented 1 time less than it used to be, in the case where it hits the end of the list (element is null).
Comment 6 Chris Dumez 2014-11-11 12:11:17 PST
Comment on attachment 241278 [details]
Patch

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

>> Source/WebCore/html/HTMLCollection.cpp:335
>> +        for (traversedCount = 0; element && traversedCount < count; ++traversedCount)
> 
> I think the issue is that traversedCount gets incremented 1 time less than it used to be, in the case where it hits the end of the list (element is null).

Sorry, I meant it gets incremented 1 time more (not less) than before when element is null.