[webkit-reviews] review denied: [Bug 70304] width/height attributes of input element should be supported when the type of the input element is image. : [Attachment 113447] Patch

[bugzilla-daemon at webkit.org]

Kent Tamura <tkent at chromium.org> has denied Dongwoo Joshua Im
<dw.im at samsung.com>'s request for review:
Bug 70304: width/height attributes of input element should be supported when
the type of the input element is image.
https://bugs.webkit.org/show_bug.cgi?id=70304

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

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


> LayoutTests/ChangeLog:14
> +	   * fast/forms/resources/samsung.gif: Added.

Are you sure that this image can be distributed in WebKit's license?

> LayoutTests/fast/forms/input-width-height-attributes.html:4
> +    <link rel="stylesheet" href="../../fast/js/resources/js-test-style.css">


This line is not needed.

> LayoutTests/fast/forms/input-width-height-attributes.html:42
> +    <div id="console"></div>

This line is not needed. js-test-pre.js automatically creates it.

> LayoutTests/fast/forms/input-width-height-attributes.html:45
> +    <script>
> +	   description('width and height attributes of HTMLInputElement.<br>');


nit: Indenting all of the content is meaningless.

> LayoutTests/fast/forms/input-width-height-attributes.html:50
> +	   div1.innerHTML = "<b>case #1.</b> Image type. --- HTML inline
setting as \"160\", \"80px\","
> +			     + " result is " + image1.width + ", " +
image1.height + "<br>" + div1.innerHTML;

We had better show "PASS" or "FAIL" in order to tell what is expected to other
developers.

> Source/WebCore/html/HTMLInputElement.cpp:1017
> +int HTMLInputElement::height() const

Why is this "int"?

> Source/WebCore/html/HTMLInputElement.cpp:1022
> +    if (m_inputType->shouldRespectHeightAndWidthAttributes())

We prefer early exit. So, this function should be:

{
    if (!m_inputType->shouldRespectHeightAndWidthAttributes())
	return 0;
    :
    :

> Source/WebCore/html/HTMLInputElement.cpp:1023
> +	   height = fastGetAttribute(heightAttr).toInt(&ok);

We had better use parseHTMLNonNegativeInteger().

Also, this implementation is incorrect.  The specification says height/width
returns the intrinsic size of the image.  Please refer to the implementation of
HTMLImageElement::width() and height().