Bug 70139 - Follow up the recent HTMLSelectElement improvements with a little bit more
Summary: Follow up the recent HTMLSelectElement improvements with a little bit more
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Darin Adler
URL:
Keywords:
Depends on: 70173
Blocks:
  Show dependency tree / graph
 
Reported: 2011-10-14 14:02 PDT by Darin Adler
Modified: 2011-10-16 14:40 PDT (History)
4 users (show)

See Also:

Attachments
Patch (54.65 KB, patch)
2011-10-14 14:22 PDT, Darin Adler 		rniwa: review+
rniwa: commit-queue-
Details | Formatted Diff | Diff
View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Darin Adler 2011-10-14 14:02:32 PDT 
Follow up the recent HTMLSelectElement improvements with a little bit more
Comment 1 Darin Adler 2011-10-14 14:22:40 PDT 
Created attachment 111075 [details]
Patch
Comment 2 Ryosuke Niwa 2011-10-14 16:19:01 PDT 
Comment on attachment 111075 [details]
Patch

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

The code looks much better after this patch. r=me provided assertions in toHTMLSelectElement are fixed.

> Source/WebCore/html/HTMLSelectElement.cpp:471
>      const Vector<HTMLElement*>& items = listItems();

This local variable seems unnecessary.

> Source/WebCore/html/HTMLSelectElement.h:168
> +    mutable Vector<HTMLElement*> m_listItems;

I'm not sure making this mutable is necessarily cleaner.

> Source/WebCore/html/HTMLSelectElement.h:181
> +    mutable bool m_shouldRecalcListItems;

Ditto.

> Source/WebCore/html/HTMLSelectElement.h:200
> +    ASSERT(!node || node->isHTMLElement());

This should be isHTMLSelectElement.

> Source/WebCore/html/HTMLSelectElement.h:206
> +    ASSERT(!node || node->isHTMLElement());

Ditto.
Comment 3 Darin Adler 2011-10-14 16:24:15 PDT 
Comment on attachment 111075 [details]
Patch

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

>> Source/WebCore/html/HTMLSelectElement.h:168
>> +    mutable Vector<HTMLElement*> m_listItems;
> 
> I'm not sure making this mutable is necessarily cleaner.

How so? This is exactly what mutable is for.
Comment 4 Darin Adler 2011-10-14 17:29:40 PDT 
Committed r97533: <http://trac.webkit.org/changeset/97533>
Comment 5 Ryosuke Niwa 2011-10-14 17:47:19 PDT 
Windows build fix attempt committed in http://trac.webkit.org/changeset/97534.
Comment 6 Ryosuke Niwa 2011-10-14 18:01:01 PDT 
Thanks for fixing the chromium windows build in http://trac.webkit.org/changeset/97535.
Comment 7 Ryosuke Niwa 2011-10-14 23:56:51 PDT 
GTK build fix landed in http://trac.webkit.org/changeset/97539.
Comment 8 Csaba Osztrogonác 2011-10-16 14:39:03 PDT 
Windows build is still broken ...
Comment 9 Csaba Osztrogonác 2011-10-16 14:40:59 PDT 
(In reply to comment #8)
> Windows build is still broken ...

Sorry for blaming you, http://trac.webkit.org/changeset/97536 is the culprit.