[webkit-reviews] review denied: [Bug 51897] Escape should clear search field : [Attachment 100667] Patch

[bugzilla-daemon at webkit.org]

Kent Tamura <tkent at chromium.org> has denied Kentaro Hara <haraken at google.com>'s
request for review:
Bug 51897: Escape should clear search field
https://bugs.webkit.org/show_bug.cgi?id=51897

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

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


r- because of possible use-after-free.


> LayoutTests/fast/forms/input-search-press-escape-key.html:28
> +    eventSender.keyDown("escape");

Do you need to use eventSender?
Doesn't the following event dispatching work?
 var event = document.createEvent('KeyboardEvents');
 event.initKeyboardEvent(...);
 input.dispatchEvent(event);

> Source/WebCore/html/SearchInputType.cpp:110
> +	   input->setValueForUser("");
> +	   input->onSearch();

setValueForUser() dispatches 'change' event and a JavaScript handler can delete
'input'.
You need to use RefPtr<HTMLInputElement>.

>> Source/WebCore/html/SearchInputType.cpp:113
>> +	    TextFieldInputType::handleKeydownEvent(event);
> 
> Does this mean it's impossible for content to handle 'esc' specially?  I
wonder if this maybe belongs on keypress rather than keydown.

Agree with Evan.