Bug 70139

Summary: Follow up the recent HTMLSelectElement improvements with a little bit more
Product: WebKit Reporter: Darin Adler <darin>
Component: FormsAssignee: Darin Adler <darin>
Status: RESOLVED FIXED    
Severity: Normal CC: luiz, ossy, rniwa, tkent
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 70173    
Bug Blocks:    
Attachments:
Description Flags
Patch rniwa: review+, rniwa: commit-queue-

Darin Adler
Reported 2011-10-14 14:02:32 PDT
Follow up the recent HTMLSelectElement improvements with a little bit more
Attachments
Patch (54.65 KB, patch)
2011-10-14 14:22 PDT, Darin Adler 		rniwa: review+
rniwa: commit-queue-
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Darin Adler
Comment 1 2011-10-14 14:22:40 PDT
Created attachment 111075 [details] Patch
Ryosuke Niwa
Comment 2 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.
Darin Adler
Comment 3 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.
Darin Adler
Comment 4 2011-10-14 17:29:40 PDT
Committed r97533: <http://trac.webkit.org/changeset/97533>
Ryosuke Niwa
Comment 5 2011-10-14 17:47:19 PDT
Windows build fix attempt committed in http://trac.webkit.org/changeset/97534.
Ryosuke Niwa
Comment 6 2011-10-14 18:01:01 PDT
Thanks for fixing the chromium windows build in http://trac.webkit.org/changeset/97535.
Ryosuke Niwa
Comment 7 2011-10-14 23:56:51 PDT
GTK build fix landed in http://trac.webkit.org/changeset/97539.
Csaba Osztrogonác
Comment 8 2011-10-16 14:39:03 PDT
Windows build is still broken ...
Csaba Osztrogonác
Comment 9 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.
Note You need to log in before you can comment on or make changes to this bug.