[webkit-reviews] review denied: [Bug 57746] HTMLOptionElement::text incorrectly normalize U+3000 to U+0020. : [Attachment 101863] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Jul 25 22:45:42 PDT 2011


Kent Tamura <tkent at chromium.org> has denied Shinya Kawanaka
<shinyak at google.com>'s request for review:
Bug 57746: HTMLOptionElement::text incorrectly normalize U+3000 to U+0020.
https://bugs.webkit.org/show_bug.cgi?id=57746

Attachment 101863: Patch
https://bugs.webkit.org/attachment.cgi?id=101863&action=review

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


> LayoutTests/fast/forms/option-strip-unicode-spaces.html:11
> +<script src="script-tests/option-strip-unicode-spaces.js"></script>

You don't need to have a separated JavaScript file.  Please embed the
JavaScript code here.

> LayoutTests/fast/forms/script-tests/option-strip-unicode-spaces.js:3
> +// see
http://www.whatwg.org/specs/web-apps/current-work/multipage/common-microsyntaxe
s.html#space-character

Start the comment with a capital letter.
// See http:...

> LayoutTests/fast/forms/script-tests/option-strip-unicode-spaces.js:4
> +var spaces = [

The name should be 'HTMLSpaces' or something.

> LayoutTests/fast/forms/script-tests/option-strip-unicode-spaces.js:15
> +var nonSpaces = [

The name should be 'nonHTMLSpaces' or something.

> LayoutTests/fast/forms/script-tests/option-strip-unicode-spaces.js:47
> +debug('insert one space before/after/between the text');

A heading should be start with a capital letter.

> LayoutTests/fast/forms/script-tests/option-strip-unicode-spaces.js:67
> +debug('insert one non space before/after/between the text');

'one non space' is confusing.  'one non-HTML space' might be better.

> Source/WebCore/dom/OptionElement.cpp:38
> +// For removing whitespaces of text attribute in option element, only these
5 characters are treated as space.
> +// See also
http://www.whatwg.org/specs/web-apps/current-work/multipage/the-button-element.
html#dom-option-text
> +static bool isOptionTextSpace(UChar c)
> +{

We already have isHTMLSpace() in WebCore/html/parserHTMLParserIdioms.h.


More information about the webkit-reviews mailing list