[Webkit-unassigned] [Bug 48821] Let HTMLObjectElement be a form associated element

[bugzilla-daemon at webkit.org]

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


Kent Tamura <tkent at chromium.org> changed:

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




--- Comment #21 from Kent Tamura <tkent at chromium.org>  2010-12-01 01:43:16 PST ---
(From update of attachment 75250)
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?

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