[webkit-reviews] review granted: [Bug 26062] Refactor HTMLSelectElement code, to be reusable for WMLSelectElement : [Attachment 30738] Updated patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Nikolas Zimmermann
<zimmermann at kde.org>'s request for review:
Bug 26062: Refactor HTMLSelectElement code, to be reusable for WMLSelectElement
https://bugs.webkit.org/show_bug.cgi?id=26062

Attachment 30738: Updated patch
https://bugs.webkit.org/attachment.cgi?id=30738&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
It's unfortunate that we're losing the Subversion history for all these
functions. Also makes the patch harder to review, since I can't see what you
did and did not change when moving the functions.

> +static const DOMTimeStamp s_typeAheadTimeout = 1000;

The "s_" prefix seems a little strange here, since this is not a static data
member. (Not that I like the s_ prefix even for static data members.) I liked
the old name without a prefix better.

> +	   cachedStateForActiveSelection.append(optionElement ?
optionElement->selected() : false);

This is probably not new code, but I would write this with && instead of ?: as
optionElement && optionElement->selected().

> +    for (unsigned int i = 0; i < items.size(); ++i) {

Should be unsigned or size_t, not unsigned int.

> +    Vector<char, 1024> characters(length);
> +    for (int i = 0; i < length; ++i) {
> +	   OptionElement* optionElement = toOptionElement(items[i]);
> +	   bool selected = optionElement ? optionElement->selected() : false;
> +	   characters[i] = selected ? 'X' : '.';
> +    }
> +
> +    value = String(characters.data(), length);

Not related to refactoring, but we should change this to use
String::createUninitialized.

> +    // ### this case should not happen. make sure that we select the first
option
> +    // in any case. otherwise we have no consistency with the DOM interface.
FIXME!
> +    // we return the first one if it was a combobox select

This old comment could be changed to use modern WebKit style, with FIXME prefix
instead of ### and sentence format.

>  #include <wtf/Vector.h>
> +#include "Event.h"

We normally put "" includes before <> includes because that's ASCII sort order.


> +    ~SelectElementData();

Does this need to be explicitly declared and defined? Can't we just let the
compiler generate it?

r=me