[webkit-reviews] review denied: [Bug 48821] Let HTMLObjectElement be a form associated element : [Attachment 75593] Patch V3

[bugzilla-daemon at webkit.org]

Kent Tamura <tkent at chromium.org> has denied Kenichi Ishibashi
<bashi at google.com>'s request for review:
Bug 48821: Let HTMLObjectElement be a form associated element
https://bugs.webkit.org/show_bug.cgi?id=48821

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

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

Looks almost ok.  r- for minor issues and patch conflict.

We should address the FIXME in HTMLObjectElement::appendFormData(), and adding
validation properties of HTMLObjectElement.idl later.

> WebCore/html/FormAssociatedElement.cpp:55
> +static HTMLFormElement* findFormOwner(HTMLElement* element)

This function is very similar to HTMLElement::findFormAncestor().  We had
better unify them, or should name this findFormAncestor() too.

> WebCore/html/HTMLFormCollection.cpp:116
> +	   if (associatedElement->isEnumeratable()
> +	       && element->getAttribute(attrName) == name) {

You don't need to wrap the line.

> WebCore/html/HTMLFormControlElement.h:58
> +    virtual void setDisabled(bool);

Do we need to add "virtual"?

> WebCore/html/HTMLFormElement.cpp:221
>      for (unsigned i = 0; i < m_associatedElements.size(); ++i)
> -	   m_associatedElements[i]->hideVisibleValidationMessage();
> +	   if (m_associatedElements[i]->isFormControlElement())
> +	      
static_cast<HTMLFormControlElement*>(m_associatedElements[i])->hideVisibleValid
ationMessage();

We should add { } to the "for" because the content has two physical lines.

> WebCore/html/HTMLFormElement.cpp:357
>      for (unsigned i = 0; i < m_associatedElements.size(); ++i)
> -	   m_associatedElements[i]->reset();
> +	   if (m_associatedElements[i]->isFormControlElement())
> +	      
static_cast<HTMLFormControlElement*>(m_associatedElements[i])->reset();

ditto.

> WebCore/html/HTMLFormElement.cpp:652
>      for (unsigned i = 0; i < m_associatedElements.size(); ++i)
> -	   m_associatedElements[i]->reset();
> +	   if (m_associatedElements[i]->isFormControlElement())
> +	      
static_cast<HTMLFormControlElement*>(m_associatedElements[i])->reset();

ditto.