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

[bugzilla-daemon at webkit.org]

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 74830: Patch
https://bugs.webkit.org/attachment.cgi?id=74830&action=review

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

quick review.

> JavaScriptCore/ChangeLog:7
> +	   when empty, clicking "down" on outer-spin-button returns "max value"

> +	   https://bugs.webkit.org/show_bug.cgi?id=45491
> +

Write why we need this wtf/ change please.

> JavaScriptCore/wtf/DateMath.h:89
> +// Returns the offsets for UTC and DST.
> +int32_t calculateUTCOffset();
> +double calculateDSTOffset(double ms, double utcOffset);

The return values are unclear.	Milliseconds, seconds, minutes, or hours?

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

> +    if (typeof optionalStepCount != "undefined")
> +	   if (optionalStepCount < 0)
> +	       for (var i = 0; i < -optionalStepCount; i++)
> +		   sendKey('Down');
> +	   else
> +	       for (var i = 0; i < optionalStepCount; i++)
> +		   sendKey('Up');
> +    else

If you follow WebKit C++ style in JavaScript code, we should add {}  for
multiple-line if-clause / else-clause.

> WebCore/ChangeLog:7
> +	   when empty, clicking "down" on outer-spin-button returns "max value"

> +	   https://bugs.webkit.org/show_bug.cgi?id=45491
> +

Please write what behavior change we have.

> WebCore/html/HTMLInputElement.h:270
> +    virtual bool isRangeControl() const { return deprecatedInputType() ==
RANGE; }

Do not use deprecatedInputType() in new code.

> WebCore/html/InputType.h:92
> +    virtual double defaultValue() const;

The name "defaultValue" is not good.  The functions of InputType are usually
synchronized with HTMLInputElement, and HTMLInputElement already has
defaultValue().

> WebCore/html/RangeInputType.cpp:165
> -	   String lastStringValue = element()->value();
> -	   element()->stepUp(stepMagnification, ec);
> -	   if (lastStringValue != element()->value())
> -	       element()->dispatchFormControlChangeEvent();
> +	   // Using stepUpFromRenderer() since it is a kind of stepUp()
operation
> +	   element()->stepUpFromRenderer(stepMagnification);

Is this change related to this bug?