[webkit-reviews] review denied: [Bug 54443] Textarea maxlength doesn't account for newlines : [Attachment 85904] Patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has denied Naoki Takano
<takano.naoki at gmail.com>'s request for review:
Bug 54443: Textarea maxlength doesn't account for newlines
https://bugs.webkit.org/show_bug.cgi?id=54443

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

------- Additional Comments from Ryosuke Niwa <rniwa at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=85904&action=review

> LayoutTests/ChangeLog:5
> +	   Textarea maxlength doesn't account for newlines

"doesn't" should be "doesn't"

> LayoutTests/fast/forms/script-tests/textarea-maxlength.js:52
> +    return textArea;

This line isn't necessary as textAre is declared in the global scope.

> LayoutTests/fast/forms/script-tests/textarea-maxlength.js:93
> +textArea = createFocusedTextAreaWithMaxLength3();

No need to update textArea manually here.

> LayoutTests/fast/forms/script-tests/textarea-maxlength.js:95
> +document.execCommand('insertLineBreak'); // This linebreak should be ignored
because limitation up to 3 characters.

Given we already call createFocusedTextAreaWithMaxLength3, the comment is
redundant.  Please get rid of it.

> LayoutTests/fast/forms/script-tests/textarea-maxlength.js:103
> +document.execCommand('insertLineBreak'); // This linebreak should be ignored
because limitation up to 3 characters.

Ditto about the comment.

> Source/WebCore/ChangeLog:5
> +	   Textarea maxlength doesn't account for newlines

Same comment about "'"

> Source/WebCore/ChangeLog:8
> +	   When a user presses a return key, TypingCommand::insertLineBreak()
is called. So before append a new line, check if we can add the new line.

This seems to be excessively long.  Please split the line at the end of the
first sentence so that each line is ~150 columns.

> Source/WebCore/editing/TypingCommand.cpp:412
> +    // Check if we can insert new line break with \n(line feed) string.
> +    ExceptionCode ec = 0;
> +    RefPtr<BeforeTextInsertedEvent> event =
BeforeTextInsertedEvent::create(String("\n"));
> +    endingSelection().rootEditableElement()->dispatchEvent(event, ec);
> +    if (!event->text().length())
> +	   return;
> +

Now that I think about it, it seems odd that we don't do the same thing for
insertParagraphSeparator (i.e. execCommand('InsertParagraph', false, null))