[webkit-reviews] review denied: [Bug 69455] Option.value should trim extra internal html spaces : [Attachment 109848] Patch fixing the space triming issue for value attribute.

[bugzilla-daemon at webkit.org]

Kent Tamura <tkent at chromium.org> has denied Sachin Puranik
<jcqt43 at motorola.com>'s request for review:
Bug 69455: Option.value should trim extra internal html spaces
https://bugs.webkit.org/show_bug.cgi?id=69455

Attachment 109848: Patch fixing the space triming issue for value attribute.
https://bugs.webkit.org/attachment.cgi?id=109848&action=review

------- Additional Comments from Kent Tamura <tkent at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=109848&action=review


> Source/WebCore/ChangeLog:13
> +	   * dom/OptionElement.cpp: code restructure.

Please write what is changed and the reason.

> Source/WebCore/ChangeLog:16
> +	   (WebCore::HTMLOptionElement::value): Fixed the issue

Please write what's changed.

> Source/WebCore/dom/OptionElement.h:44
> +    virtual String label() const = 0;

Why was this added?

> Source/WebCore/html/HTMLOptionElement.cpp:227
> -    String label = m_data.label();
> -    if (!label.isNull())
> -	   return label;
> - 
> -    label = collectOptionInnerText(this).stripWhiteSpace(isHTMLSpace);
> -    label = label.simplifyWhiteSpace(isHTMLSpace);
> +    if (!m_data.label().isNull())
> +	   return m_data.label();
>  
> -    return label;
> +    return
collectOptionInnerText(this).stripWhiteSpace(isHTMLSpace).simplifyWhiteSpace(is
HTMLSpace);

This part is not related to the bug.  Don't include this part in the patch.

> LayoutTests/fast/forms/option-value-trim-html-spaces.html:4
> +<meta http-equiv="Content-Type" content="text/html; charset=iso-8859-1">

Why iso-8859-1? This files contains only ASCII characters.