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

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


> LayoutTests/fast/forms/input-width-height-attributes.html:11
> +<style>
> +    div {
> +	   border-style:solid;
> +	   border-width:1px;
> +	   width: 600px;
> +    }
> +</style>

Indenting whole the content makes no sense.  Please write it like:
<style>
div {
    border-stylid: solid;
    border-width: 1px;
    width: 600px;
}
</style>

BTW, this style looks not needed.

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

Indenting whole the content makes no sense.

<br> is not needed.

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

You should print it by debug().

> LayoutTests/fast/forms/input-width-height-attributes.html:70
> +</script>

Test coverage is not good.  We should test height/image properties with
non-image types.

> Source/WebCore/html/HTMLInputElement.cpp:1022
> +    if (!m_inputType->isImageButton())
> +	   return 0;
> +

This type check is not needed. We can just call InputType::height().

> Source/WebCore/html/HTMLInputElement.h:138
> +    unsigned height(bool ignorePendingStylesheets = false) const;
> +    unsigned width(bool ignorePendingStylesheets = false) const;

There are no call sites of ignorePendingStylesheets=true.  We should remove the
ignorePendingStylesheets parameter.

The ignorePendingStylesheets parameter of HTMLImageElement::width() and
height() was introduced by http://trac.webkit.org/changeset/7625. But it seems
the parameter is not used now.

> Source/WebCore/html/ImageInputType.cpp:187
> +	   // check the attribute first for an explicit pixel valuea

A comment should be like a normal English sentences. It should start with a
capital letter, and should end with '.'

> Source/WebCore/html/ImageInputType.h:69
> +    virtual unsigned height(bool) const;
> +    virtual unsigned width(bool) const;
> +    virtual void setHeight(unsigned);
> +    virtual void setWidth(unsigned);

We had better append OVERRIDE keyword to them.

> Source/WebCore/html/InputType.cpp:747
> +void InputType::setHeight(unsigned height)
> +{
> +}
> +
> +void InputType::setWidth(unsigned width)
> +{
> +}

The specification says:
> On setting, they must act as if they reflected the respective content
attributes of the same name.

and doesn't say setting depends on input type.	I think we should set the
corresponding attribute value here.