[webkit-reviews] review denied: [Bug 26246] Implement WML specific features for <select> : [Attachment 31802] Updated patch

[bugzilla-daemon at webkit.org]

Adam Treat <treat at kde.org> has denied Nikolas Zimmermann <zimmermann at kde.org>'s
request for review:
Bug 26246: Implement WML specific features for <select>
https://bugs.webkit.org/show_bug.cgi?id=26246

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

------- Additional Comments from Adam Treat <treat at kde.org>
> +	   * dom/SelectElement.cpp:
> +	   (WebCore::SelectElement::optionCount):
> +	   (WebCore::SelectElementData::name):
> +	   * dom/SelectElement.h:
> +	   (WebCore::SelectElementData::setName):
> +	   * html/HTMLSelectElement.cpp:
> +	   (WebCore::HTMLSelectElement::formControlName):
> +	   (WebCore::HTMLSelectElement::length):
> +	   (WebCore::HTMLSelectElement::parseMappedAttribute):
> +	   * html/HTMLSelectElement.h:
> +	   * wml/WMLCardElement.cpp:
> +	   (WebCore::WMLCardElement::handleIntrinsicEventIfNeeded):
> +	   * wml/WMLSelectElement.cpp:
> +	   (WebCore::WMLSelectElement::title):
> +	   (WebCore::WMLSelectElement::formControlName):
> +	   (WebCore::WMLSelectElement::parseMappedAttribute):
> +	   (WebCore::WMLSelectElement::defaultEventHandler):
> +	   (WebCore::WMLSelectElement::selectInitialOptions):
> +	   (WebCore::WMLSelectElement::calculateDefaultOptionIndices):
> +	   (WebCore::WMLSelectElement::selectDefaultOptions):
> +	   (WebCore::WMLSelectElement::initializeVariables):
> +	   (WebCore::WMLSelectElement::updateVariables):
> +	   (WebCore::WMLSelectElement::parseIndexValueString):
> +	   (WebCore::WMLSelectElement::valueStringToOptionIndices):
> +	   (WebCore::WMLSelectElement::optionIndicesToValueString):
> +	   (WebCore::WMLSelectElement::optionIndicesToString):
> +	   * wml/WMLSelectElement.h:

This is a big change to review.  I'd appreciate it if you could go through the
ChangeLog's and document specifically what you change to each method and why. 
And also why you add methods in a couple of cases.  From looking, it seems that
you add 'optionCount' to SelectElement so that both HTMLSelectElement and
WMLSelectElement can use it whereas before it was implemented in
HTMLSelectElement.  You've also changed it so that it uses 'isOptionElement' as
the test rather than 'hasLocalName(optionTag).'  In general, it'd be great to
have these changes and the reasoning behind them documented in the ChangeLog so
I know specifically what you were thinking without guessing.

This is best practices anyway...