[Webkit-unassigned] [Bug 70304] width/height attributes of input element should be supported when the type of the input element is image.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=70304


Kent Tamura <tkent at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #117162|review?                     |review-
               Flag|                            |




--- Comment #35 from Kent Tamura <tkent at chromium.org>  2011-12-01 01:01:26 PST ---
(From update of attachment 117162)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.