[webkit-reviews] review denied: [Bug 54152] typing enter in the input element should not fire textInput : [Attachment 83600] with layout test

Wed Feb 23 21:01:19 PST 2011

Ryosuke Niwa <rniwa at webkit.org> has denied Alice Boxhall
<aboxhall at chromium.org>'s request for review:
Bug 54152: typing enter in the input element should not fire textInput
https://bugs.webkit.org/show_bug.cgi?id=54152

Attachment 83600: with layout test
https://bugs.webkit.org/attachment.cgi?id=83600&action=review

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

The code change looks good but you need to explain what you're doing.  Also,
the layout test can be simplified further.

> LayoutTests/ChangeLog:7
> +

You should explain what kind of a test you're adding.

> LayoutTests/fast/forms/textinput-not-fired-on-enter-in-input.html:1
> +<html>

Missing <!DOCTYPE html>

> LayoutTests/fast/forms/textinput-not-fired-on-enter-in-input.html:6
> +    <script>
> +    function log(msg) {
> +	  
document.getElementById('console').appendChild(document.createTextNode(msg +
'\n'));
> +    }

We don't normally indent tag like this.

> LayoutTests/fast/forms/textinput-not-fired-on-enter-in-input.html:19
> +	   } catch (e) {}  // Ignore ReferenceError if eventSender is undefined

You should exit early when eventSender is not defined.

> LayoutTests/fast/forms/textinput-not-fired-on-enter-in-input.html:27
> +	       var resultDiv = document.getElementById('result');
> +	       resultDiv.firstChild.innerHTML = 'FAIL';

You should call log instead.

> LayoutTests/fast/forms/textinput-not-fired-on-enter-in-input.html:33
> +	   if (lastKeyPressed == 13)

What does 13 mean?

> LayoutTests/fast/forms/textinput-not-fired-on-enter-in-input.html:34
> +	       document.getElementById('result').style.setProperty('display',
'block');

I don't think it's great that you hide/show SUCCESS like this.	You should call
log on console instead.

> LayoutTests/fast/forms/textinput-not-fired-on-enter-in-input.html:50
> +    function loaded() {
> +	   var el = document.getElementById('el');
> +	   el.addEventListener('keypress', setLastKeyPressed);
> +	   el.addEventListener('keypress', showResults);
> +	   el.addEventListener('input', fail);
> +	   el.addEventListener('textInput', fail);
> +
> +	   runTest();
> +    }

I don't think it's necessary to wait until page loads.	You should just place
this entire script element below pre and just run the test there while parsing
is done.

> LayoutTests/fast/forms/textinput-not-fired-on-enter-in-input.html:57
> +<br><br>

Do we need these BRs?

> Source/WebCore/ChangeLog:7
> +

You should explain what caused the bug and how you fixed it.