Bug 90111 - Cleanup HTMLFormCollection
Summary: Cleanup HTMLFormCollection
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ryosuke Niwa
URL:
Keywords:
Depends on:
Blocks: 90118
  Show dependency treegraph
 
Reported: 2012-06-27 16:42 PDT by Ryosuke Niwa
Modified: 2012-06-28 16:03 PDT (History)
5 users (show)

See Also:


Attachments
Cleanup (5.26 KB, patch)
2012-06-27 16:57 PDT, Ryosuke Niwa
kling: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2012-06-27 16:42:55 PDT
As the comment at the beginning of HTMLFormCollection.h this entire class is a giant hack, and indeed the class is filled with questionable code.
Clean up some of that mess first so that we can do more refactoring later.
Comment 1 Ryosuke Niwa 2012-06-27 16:57:43 PDT
Created attachment 149824 [details]
Cleanup
Comment 2 Ryosuke Niwa 2012-06-27 17:00:06 PDT
Comment on attachment 149824 [details]
Cleanup

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

> Source/WebCore/html/HTMLFormCollection.cpp:-176
> -    m_cache.current = getNamedItem(idAttr, name);
> -    if (m_cache.current)
> -        return m_cache.current;
> -    m_cache.current = getNamedItem(nameAttr, name);
> -    return m_cache.current;

Updating m_cache.current here made no sense. In fact, I bet you can find a test case where this introduces a bug using the Objective-C binding.
e.g. collection.item(0); collection.namedItem('blah'); collection.item(0);
The second call to item() returns the element with id/name 'blah'.
Unfortunately, ES5 binding uses namedItems so we can't add a layout test for it.
Comment 3 Andreas Kling 2012-06-28 12:00:03 PDT
Comment on attachment 149824 [details]
Cleanup

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

> Source/WebCore/ChangeLog:8
> +        Got rid of getNamedItem and enamed getNamedFormItem to firstNamedItem and got rid of duplicateNumber argument since

Typo, enamed.

> Source/WebCore/html/HTMLFormCollection.cpp:126
> +        if (elementsArray[i]->isEnumeratable() && element->fastGetAttribute(attrName) == name)

Maybe someone should rename isEnumeratable() this to isEnumerable() someday.
Comment 4 Ryosuke Niwa 2012-06-28 16:01:10 PDT
Committed r121478: <http://trac.webkit.org/changeset/121478>
Comment 5 Ryosuke Niwa 2012-06-28 16:03:09 PDT
(In reply to comment #4)
> Committed r121478: <http://trac.webkit.org/changeset/121478>

I've figured out a way to write a WebKit API test so I went ahead and landed it with it. Fixed the typo in http://trac.webkit.org/changeset/121479.