[webkit-reviews] review granted: [Bug 66241] Crash when inserting text with a trailing newline into a textarea via JS : [Attachment 105878] fixes the bug

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Ryosuke Niwa <rniwa at webkit.org>'s
request for review:
Bug 66241: Crash when inserting text with a trailing newline into a textarea
via JS
https://bugs.webkit.org/show_bug.cgi?id=66241

Attachment 105878: fixes the bug
https://bugs.webkit.org/attachment.cgi?id=105878&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=105878&action=review


>>> Source/WebCore/html/HTMLInputElement.cpp:627
>>> +	 if (!isTextField() && !isPasswordField())
>> 
>> Type=password is also a text field.	So "if (!isTextField())" is enough.
>> 
>> nit: Such type-specific operation should be in specific InputType.
>>  1. Add an empty InputType::updateInnerTextValue()
>>  2. Add TextFieldInputType::updateInnerTextValue()
>>  3. Change the call sites of updateInnerTextValue()	to
m_inputType->updateInnerTextValue().
> 
> Oh oops, forgot to remove isPasswordField().	Will do.

Putting the type-specific operation on the specific InputType the way Kent is
telling you to is better. The isXXX functions are deprecated within the class.

> Source/WebCore/html/HTMLInputElement.cpp:1105
>      if (isTextField()) {
> +	   if (valueChanged)
> +	       updateInnerTextValue();

Since updateInnerTextValue already has the isTextField check, it might be
clearer to leave the call outside the if statement. Because later we’d probably
like to refactor the code within the isTextField block into an InputType
function.

> Source/WebCore/html/HTMLInputElement.h:148
> +    void updateInnerTextValue();

A shame this has to public just so it can be called by one of the InputType
functions.

> Source/WebCore/html/HTMLTextFormControlElement.h:107
>      String valueWithHardLineBreaks() const;
> +
>  private:

Nice to add this blank line--it’s the preferred style--but not sure this should
be in this patch.