[webkit-reviews] review denied: [Bug 60673] <input type=number> doesn't ignore size="" attribute : [Attachment 102434] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Shinya Kawanaka <shinyak at google.com>'s
request for review:
Bug 60673: <input type=number> doesn't ignore size="" attribute
https://bugs.webkit.org/show_bug.cgi?id=60673

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

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=102434&action=review


> LayoutTests/fast/forms/input-number-size.html:19
> +    text.size = size;
> +    number.step = step;
> +    number.min = min;
> +    number.max = max;
> +    shouldBe('number.offsetWidth', 'text.offsetWidth');

This can be structured so the test output is much nicer! The test output should
include the values of size, step, min, and max. That can be done by making a
function that sets up the text and number and then computes the width like
this:

    shouldBe('numberWidth(' + step + ', ' + min + ', ' + max + ')',
'textWidth(' + size + ')');

Then you just write the numberWidth and textWidth functions:

    function textWidth(size)
    {
	text.size  = size;
	return text.offsetWidth;
    }

If you do that, the test output will be informative instead of just a long row
of PASS PASS PASS.

> Source/WebCore/html/HTMLInputElement.cpp:998
> +int HTMLInputElement::possibleContentSize() const

What is "possible content size"? Is it the maximum size that would be needed
for the largest possible allowed content? I understand that size means "number
of characters", and "content" means "text in the input element", but what does
the word "possible" mean here? This name is not clear.

> Source/WebCore/html/NumberInputType.cpp:61
> +    double absValue = fabs(value);

No reason to abbreviate here. How about "absoluteValue" instead of "absValue".

> Source/WebCore/html/NumberInputType.cpp:65
> +    unsigned length = static_cast<int>(log10(floor(absValue))) + 1;

Why cast to an int and then convert to an unsigned?

> Source/WebCore/html/NumberInputType.cpp:145
> +    unsigned minValueDecimalPlaces, maxValueDecimalPlaces,
stepValueDecimalPlaces;
> +    double minValueDouble, maxValueDouble, stepValueDouble;

We don’t declare multiple variables on a single line in WebKit code.

> Source/WebCore/html/NumberInputType.cpp:354
> +bool NumberInputType::shouldAvoidReformat()

"should avoid reformat" is not good grammar. And I have no idea what it means.
What is a "reformat"?

> Source/WebCore/html/NumberInputType.cpp:356
> +    // TODO:

?

> Source/WebCore/platform/text/LocalizedNumber.h:54
> +String getLocalizedDecimalPoint();

We don’t use get in function names like this in WebKit.

> Source/WebCore/rendering/RenderTextControlSingleLine.cpp:429
> +    if (RenderBox* spinRenderer = spinButton ? spinButton->renderBox() : 0)
> +	   result += spinRenderer->borderLeft() + spinRenderer->borderRight() +

> +		     spinRenderer->paddingLeft() +
spinRenderer->paddingRight();

We don’t line up subsequent lines like this. We use braces if there are
multiple lines in an if statement body.

I don’t see any coverage of this code change in the test case.