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

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

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

> WebCore/WebCore.xcodeproj/project.pbxproj:14746
> +				4A0DA2FC129B241900AB61E1 /*
FormAssociatedElement.cpp */,
> +				4A0DA2FD129B241900AB61E1 /*
FormAssociatedElement.h */,
>				49484FAE102CF01E00187DD3 /* canvas */,
>				97C1F5511228558800EDE616 /* parser */,
>				B0149E7911A4B21500196A7B /*
AsyncImageResizer.cpp */,

Should be sorted.

> WebCore/WebCore.xcodeproj/project.pbxproj:21442
> +				4A0DA2FF129B241900AB61E1 /*
FormAssociatedElement.h in Headers */,

ditto.
Appending new entry at the last causes patch-conflict easily.

> WebCore/WebCore.xcodeproj/project.pbxproj:24022
> +				4A0DA2FE129B241900AB61E1 /*
FormAssociatedElement.cpp in Sources */,

ditto.

> WebCore/html/FormAssociatedElement.h:62
> +    virtual bool disabled() const { return m_disabled; }
> +    virtual void setDisabled(bool value) { m_disabled = value; }
> +    bool required() const { return m_required; }
> +    void setRequired(bool value) { m_required = value; }
> +    bool readOnly() const { return m_readOnly; }
> +    void setReadOnly(bool value) { m_readOnly = value; }

Do we need them in FormAssociatedElement?
<object> doesn't have such properties.

> WebCore/html/HTMLFormControlElement.h:42
> +// The HTML5 specification calls these control elements and object element
as
> +// form-associated elements. So we introduce FormAssoiatedElement class as a

> +// superclass of those elements to handle these in much the same manner.
> +class HTMLFormControlElement : public HTMLElement, public
FormAssociatedElement {

This comment doesn't explain difference between FormAssociatedElement and
HTMLFormControlElement.
I think what we should write is "HTMLFormControlElement is the default
implementation of FormAssociatedElement, and form-associated element
implmeentations should use HTMLFormControlElement unless there is a special
reason." or something like that.

> WebCore/html/HTMLFormElement.cpp:478
> +    // FIXME: Removes later
> +    for (unsigned i = 0; i < m_associatedElements.size(); ++i)
> +	   ASSERT(e != m_associatedElements[i]);

Do you still need this?

> WebCore/html/HTMLObjectElement.cpp:496
> +    const AtomicString& name(m_name);
> +    return name.isNull() ? emptyAtom : name;

The variable "name" looks redundant.

> WebCore/html/HTMLObjectElement.cpp:502
> +    const AtomicString& serviceType(m_serviceType);
> +    return serviceType.isNull() ? emptyAtom : serviceType;

This function returns an address of a temporal object on the stack.
Anyway, is returning m_serviceType correct?  m_serviceType is not compatible
with other form control types, It's a MIME type. Do we need formControlType()
in FormAssociatedElement?