[webkit-reviews] review denied: [Bug 45491] when empty, clicking "down" on outer-spin-button returns "max value" : [Attachment 74839] Patch

Thu Nov 25 17:00:22 PST 2010

Kent Tamura <tkent at chromium.org> has denied Dai Mikurube
<dmikurube at google.com>'s request for review:
Bug 45491: when empty, clicking "down" on outer-spin-button returns "max value"
https://bugs.webkit.org/show_bug.cgi?id=45491

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

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

I stopped reviewing halfway.  The test quality is bad.	Please be careful.

> JavaScriptCore/ChangeLog:20
> +	   Another option was to provide another currentTime() to return the
current local milliseconds.
> +	   Surveying the codes, however, all of existing "milliseconds" in
double variables are
> +	   consistently in UTC. I guess we should not emit "milliseconds" in
non-UTC in the code.
> +	   Finally I locked "milliseconds" in non-UTC in calculating default
values.

This paragraph looks unnecessary.
Also, I feel it's strange to have the word "I" in ChangeLog.

> LayoutTests/fast/forms/script-tests/input-stepup-stepdown-from-renderer.js:1
> +description('Check stepUp() and stepDown() bahevior from renderer for
type=date, datetime, datetime-local, month, time, week.');

The test contains "number" and "range" types.

> LayoutTests/fast/forms/script-tests/input-stepup-stepdown-from-renderer.js:88

> +debug('Non-number arguments');
> +shouldBe('stepUp("2010-02-10", null, null, "0")', '"2010-02-10"');
> +shouldBe('stepDown("2010-02-10", null, null, "0")', '"2010-02-10"');
> +shouldBe('stepUp("2010-02-10", null, null, "foo")', '"2010-02-10"');
> +shouldBe('stepDown("2010-02-10", null, null, "foo")', '"2010-02-10"');
> +shouldBe('stepUp("2010-02-10", null, null, null)', '"2010-02-10"');
> +shouldBe('stepDown("2010-02-10", null, null, null)', '"2010-02-10"');

The 4th parameter is recognized in JavaScript code and is not passed to C++
code.
We don't need to test invalid values.

>
LayoutTests/fast/forms/script-tests/input-stepup-stepdown-from-renderer.js:101
> +shouldBe('stepUp("2010-02-10", "3.40282346e+38", null)','"1970-01-01"');
> +shouldBe('stepDown("2010-02-10", "3.40282346e+38", null)',
'"275760-09-13"');

These results look very strange.  Are they expected?

>
LayoutTests/fast/forms/script-tests/input-stepup-stepdown-from-renderer.js:114
> +debug('Non-number arguments');
> +shouldBe('stepUp("2010-02-10T20:13Z", null, null, "0")',
'"2010-02-10T20:13Z"');
> +shouldBe('stepDown("2010-02-10T20:13Z", null, null, "0")',
'"2010-02-10T20:13Z"');
> +shouldBe('stepUp("2010-02-10T20:13Z", null, null, "foo")',
'"2010-02-10T20:13Z"');
> +shouldBe('stepDown("2010-02-10T20:13Z", null, null, "foo")',
'"2010-02-10T20:13Z"');
> +shouldBe('stepUp("2010-02-10T20:13Z", null, null, null)',
'"2010-02-10T20:13Z"');
> +shouldBe('stepDown("2010-02-10T20:13Z", null, null, null)',
'"2010-02-10T20:13Z"');

ditto.

>
LayoutTests/fast/forms/script-tests/input-stepup-stepdown-from-renderer.js:140
> +debug('Non-number arguments');
> +shouldBe('stepUp("2010-02-10T20:13", null, null, "0")',
'"2010-02-10T20:13"');
> +shouldBe('stepDown("2010-02-10T20:13", null, null, "0")',
'"2010-02-10T20:13"');
> +shouldBe('stepUp("2010-02-10T20:13", null, null, "foo")',
'"2010-02-10T20:13"');
> +shouldBe('stepDown("2010-02-10T20:13", null, null, "foo")',
'"2010-02-10T20:13"');
> +shouldBe('stepUp("2010-02-10T20:13", null, null, null)',
'"2010-02-10T20:13"');
> +shouldBe('stepDown("2010-02-10T20:13", null, null, null)',
'"2010-02-10T20:13"');

ditto.

>
LayoutTests/fast/forms/script-tests/input-stepup-stepdown-from-renderer.js:166
> +debug('Non-number arguments');
> +shouldBe('stepUp("2010-02", null, null, "0")', '"2010-02"');
> +shouldBe('stepDown("2010-02", null, null, "0")', '"2010-02"');
> +shouldBe('stepUp("2010-02", null, null, "foo")', '"2010-02"');
> +shouldBe('stepDown("2010-02", null, null, "foo")', '"2010-02"');
> +shouldBe('stepUp("2010-02", null, null, null)', '"2010-02"');
> +shouldBe('stepDown("2010-02", null, null, null)', '"2010-02"');

ditto.

>
LayoutTests/fast/forms/script-tests/input-stepup-stepdown-from-renderer.js:195
> +debug('Non-number arguments');
> +shouldBe('stepUp("0", null, null, "0")', '"0"');
> +shouldBe('stepDown("0", null, null, "0")', '"0"');
> +shouldBe('stepUp("0", null, null, "foo")', '"0"');
> +shouldBe('stepDown("0", null, null, "foo")', '"0"');
> +shouldBe('stepUp("0", null, null, null)', '"0"');
> +shouldBe('stepDown("0", null, null, null)', '"0"');

ditto.

>
LayoutTests/fast/forms/script-tests/input-stepup-stepdown-from-renderer.js:205
> +debug('Extra arguments');
> +shouldBe('input.value = "0"; input.min = null; input.step = null;
input.stepUp(1, 2); input.value', '"1"');
> +shouldBe('input.value = "1"; input.stepDown(1, 3); input.value', '"0"');

These are not related to the code change.

>
LayoutTests/fast/forms/script-tests/input-stepup-stepdown-from-renderer.js:212
> +debug('Invalid step value');
> +shouldBe('stepUp("0", "foo", null)', '"1"');
> +shouldBe('stepUp("1", "0", null)', '"2"');
> +shouldBe('stepUp("2", "-1", null)', '"3"');
> +debug('Step=any');
> +shouldBe('stepUp("0", "any", null)', '"0"');
> +shouldBe('stepDown("0", "any", null)', '"0"');

We should add test cases with empty/invalid values and invalid/any step.

>
LayoutTests/fast/forms/script-tests/input-stepup-stepdown-from-renderer.js:217
> +shouldBe('input.value', '"0"');

This doesn't test anything.

>
LayoutTests/fast/forms/script-tests/input-stepup-stepdown-from-renderer.js:222
> +shouldBe('input.value', '"0"');

ditto.

>
LayoutTests/fast/forms/script-tests/input-stepup-stepdown-from-renderer.js:224
> +debug('stepDown()/stepUp() for stepMismatch values');

Other types should have similar test cases.

>
LayoutTests/fast/forms/script-tests/input-stepup-stepdown-from-renderer.js:226
> +shouldBe('input.stepDown(); input.value', '"0"');

This is not related to the code change.

>
LayoutTests/fast/forms/script-tests/input-stepup-stepdown-from-renderer.js:227
> +shouldBe('input.min = "0"; stepUp("9", "10", "", 9)', '"90"');

This looks equivalent to
shouldBe('input.min = "0"; stepUp("9", "10", "")', '"10"');
The 4th parameter is not needed.

>
LayoutTests/fast/forms/script-tests/input-stepup-stepdown-from-renderer.js:236
> +shouldBe('input.stepUp(); input.stepUp(); input.stepUp(); input.stepUp();
input.stepUp(); input.stepUp(); input.stepUp(); input.stepUp(); input.stepUp();
input.stepUp(); input.value', '"3"');

This is not related to the code change.

>
LayoutTests/fast/forms/script-tests/input-stepup-stepdown-from-renderer.js:238
> +shouldBe('for (var i = 0; i < 255; i++) { input.stepDown(); }; input.value',
'"0"');

ditto.

>
LayoutTests/fast/forms/script-tests/input-stepup-stepdown-from-renderer.js:250
> +debug('function arguments are (min, max, step, value, [stepCount])');

Nice to have explanation of stepUp() and stepDown() too.

>
LayoutTests/fast/forms/script-tests/input-stepup-stepdown-from-renderer.js:260
> +debug('Non-number arguments (stepCount)');
> +shouldBe('stepUpExplicitBounds(null, null, null, "0", "0")', '"0"');
> +shouldBe('stepDownExplicitBounds(null, null, null, "0", "0")', '"0"');
> +shouldBe('stepUpExplicitBounds(null, null, null, "0", "foo")', '"0"');
> +shouldBe('stepDownExplicitBounds(null, null, null, "0", "foo")', '"0"');
> +shouldBe('stepUpExplicitBounds(null, null, null, "0", null)', '"0"');
> +shouldBe('stepDownExplicitBounds(null, null, null, "0", null)', '"0"');

It makes no sense to test invalid 5th parameter.

>
LayoutTests/fast/forms/script-tests/input-stepup-stepdown-from-renderer.js:270
> +debug('Extra arguments');
> +shouldBe('setInputAttributes(null, null, null, "0"); input.stepUp(1,2);
input.value', '"1"');
> +shouldBe('setInputAttributes(null, null, null, "1"); input.stepDown(1,3);
input.value', '"0"');

These are not related to the code change.