[webkit-reviews] review denied: [Bug 65542] Need support for dirname attribute : [Attachment 111237] Proposed patch

[bugzilla-daemon at webkit.org]

Kent Tamura <tkent at chromium.org> has denied Rakesh <rakesh.kn at motorola.com>'s
request for review:
Bug 65542: Need support for dirname attribute
https://bugs.webkit.org/show_bug.cgi?id=65542

Attachment 111237: Proposed patch
https://bugs.webkit.org/attachment.cgi?id=111237&action=review

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


> Source/WebCore/ChangeLog:24
> +	   * html/HTMLAttributeNames.in:
> +	   * html/HTMLInputElement.cpp:
> +	   (WebCore::HTMLInputElement::parseMappedAttribute):
> +	   (WebCore::HTMLInputElement::appendFormData):
> +	   * html/HTMLInputElement.idl:
> +	   * html/HTMLTextAreaElement.cpp:
> +	   (WebCore::HTMLTextAreaElement::appendFormData):
> +	   * html/HTMLTextAreaElement.idl:
> +	   * html/HTMLTextFormControlElement.cpp:
> +	   (WebCore::HTMLTextFormControlElement::parseMappedAttribute):
> +	   (WebCore::enclosingTextFormControl):
> +	   (WebCore::HTMLTextFormControlElement::appendFormData):
> +	   * html/HTMLTextFormControlElement.h:

Please write what you changed for each of files/functions.

> Source/WebCore/html/HTMLInputElement.cpp:825
> +    } else if (attr->name() == dirnameAttr) {
> +	   HTMLTextFormControlElement::parseMappedAttribute(attr);

This should be removed.
We call HTMLTextFormControlElement::parseMappedAttribute(attr) at the bottom of
the function.

> Source/WebCore/html/HTMLInputElement.cpp:941
> +    if (result && fastHasAttribute(dirnameAttr) && (isTextField() ||
isSearchField()))
> +	   result = HTMLTextFormControlElement::appendFormData(encoding,
multipart);
> +    return result;

* isSearchField() is redundant because isTextField() is always true if
isSearchField() is true.
* This code should be in TextFieldInputType::appendFormData().
  Having dirname processing in HTMLTextFormControlElement::appendFormData() is
very strange.
  So, I think we had better add a public helper function to
HTMLTextFormControlElement, and it is called from
TextFieldInputType::appendFormData() and HTMLTextAreaElemet::appendFormData().

> Source/WebCore/html/HTMLTextFormControlElement.cpp:434
> +    else if (attr->name() == dirnameAttr)
> +	   m_dirName = attr->value();

m_dirName is not needed.

> Source/WebCore/html/HTMLTextFormControlElement.cpp:585
> +    else if (hasAttribute(dirAttr))
> +	   dir = getAttribute(dirAttr).string();

Should use fastHasAttribute() and fastGetAttribute().

> Source/WebCore/html/HTMLTextFormControlElement.cpp:589
> +    list.appendData(m_dirName.string(), dir);

We can use fastGetAttribute(dirnameAttr)

> LayoutTests/fast/forms/form-dirname-attribute.html:18
> +var input = document.createElement('input');
> +input.setAttribute('dirName', "Hello");
> +shouldBeEqualToString('input.dirName', "Hello");
> +
> +var textArea = document.createElement('textarea');
> +textArea.setAttribute('dirName', "Hello");
> +shouldBeEqualToString('textArea.dirName', "Hello");

Need tests of form submission with dirname attribute.