[webkit-reviews] review denied: [Bug 23465] Refactor some more HTMLOptionElement code : [Attachment 26910] Initial patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Jan 21 16:04:45 PST 2009

Eric Seidel <eric at webkit.org> has denied Nikolas Zimmermann
<zimmermann at kde.org>'s request for review:
Bug 23465: Refactor some more HTMLOptionElement code

Attachment 26910: Initial patch

------- Additional Comments from Eric Seidel <eric at webkit.org>
Seems like this one:
 982	     if
ntedToRespectGroupLabel()).startsWith(prefix, false)) {

Doesn't want that call.  Also, that line could be broken into multiple lines to
be more readable.

What is "aggregateOptionText"?	Maybe you mean aggregate as a verb?  Still not
sure why it's used that way.

maybe m_optionData?  m_data is so bland, reminds me of the kde "d" pointers
which I was never a fan of.

I think "aggregate*" I would just call "optionText" and "optionValue".	The
fact that they have to do work under the covers is OK to hide in this case I

Otherwise this looks fine to me.  I'd like to see the final patch though.

More information about the webkit-reviews mailing list