[webkit-reviews] review denied: [Bug 54443] Textarea maxlength doesn't account for newlines : [Attachment 85907] 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 85907: Patch
https://bugs.webkit.org/attachment.cgi?id=85907&action=review

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

> LayoutTests/ChangeLog:9
> +	   (createFocusedTextAreaWithMaxLength3): Added to make sure
consecutive insertbreaks work correctly for textarea maxlength.

Please split this into two lines.  It's way too long to fit in one line.

> Source/WebCore/editing/TypingCommand.cpp:50
> +// Helper function to check if we can insert new line feed with \n(line
feed) string.

This comment seems to repeat what code does.  In general, we'd like to make
code self-explanatory and add as little comment as possible.

> Source/WebCore/editing/TypingCommand.cpp:51
> +static bool canAppendNewLineFeed(TypingCommand& typingCommand)

It's uncommon for a function to take a reference to an EditCommand like this
because EditCommand is ref-counted.

> Source/WebCore/editing/TypingCommand.cpp:416
> +    if (!canAppendNewLineFeed(*this))
> +	   return;

I'd pass typingCommand.endingSelection() or the root editable element instead.