[webkit-reviews] review granted: [Bug 237525] [selection] Set correct selection range for TEXTAREA when updating default value : [Attachment 458582] Patch

[bugzilla-daemon at webkit.org]

Chris Dumez <cdumez at apple.com> has granted zsun at igalia.com's request for
review:
Bug 237525: [selection] Set correct selection range for TEXTAREA when updating
default value
https://bugs.webkit.org/show_bug.cgi?id=237525

Attachment 458582: Patch

https://bugs.webkit.org/attachment.cgi?id=458582&action=review




--- Comment #6 from Chris Dumez <cdumez at apple.com> ---
Comment on attachment 458582
  --> https://bugs.webkit.org/attachment.cgi?id=458582
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=458582&action=review

r=me with comments. (also agree with all of Tim's)

> Source/WebCore/html/HTMLTextAreaElement.cpp:139
> +	   setNonDirtyValue(defaultValue(), 
TextControlSetValueSelection::Clamp);

Too many spaces after the comma.

> Source/WebCore/html/HTMLTextAreaElement.cpp:406
> +    bool isClamp = selection == TextControlSetValueSelection::Clamp;

I think this indicates whether we want to clamp or not. If so, I think
"shouldClamp" would be a better variable name.

>> Source/WebCore/html/HTMLTextFormControlElement.h:86
>> +	WEBCORE_EXPORT unsigned computeSelectionEnd() const;
> 
> This doesn't need WEBCORE_EXPORT, you're not using outside of the webcore
directory.

True but also, why did you make them public. Seems like they could be
protected?