Bug 48821 – Let HTMLObjectElement be a form associated element

• Status: RESOLVED
• Resolution: FIXED
• Component: Forms

[Kenichi Ishibashi]

This issue comes from https://bugs.webkit.org/show_bug.cgi?id=29363#c17.

The draft of HTML5 spec <http://www.w3.org/TR/html5/forms.html#category-listed> says following form associated elements are listed in the form.elements:  <button>, <fieldset>, <input>, <keygen>, <object>, <output>, <select>, and <textarea>. But in WebKit, <fieldset>, <keygen> and <object> are not appeared in the form.elements property. We might need to discuss what we should do on this difference.

[Kent Tamura]

I checked behaviors of IE, Firefox, and Opera.

IE8:
 fieldset and object were listed.  keygen is not supported.

Firefox 3.6:
 fieldset and object were listed.  keygen is not supported.

Opera 10.36:
 fieldset, object, and keygen were listed.

So, the specification looks reasonable and we should fix WebKit behavior.

[Kenichi Ishibashi]

Kent-san,

(In reply to comment #1)
> So, the specification looks reasonable and we should fix WebKit behavior.

Thank you for the investigation. I'd like to write a patch for this. I looked briefly at the code and found that HTMLObjectElement class is not a subclass of HTMLFormControlElement so far, that means we need to change its class inheritance to list HTMLObjectElement in form.elements. I'm not sure whether it is worth to put an effort to follow the spec so I'd like to ask your opinion.

Thanks,

[Kent Tamura]

(In reply to comment #2)
> I looked briefly at the code and found that HTMLObjectElement class is not a subclass of HTMLFormControlElement so far, that means we need to change its class inheritance to list HTMLObjectElement in form.elements. I'm not sure whether it is worth to put an effort to follow the spec so I'd like to ask your opinion.

We need to add validation properties to HTMLObjectElement.  So making HTMObjectElement a subclass of HTMLFormControlElement is needed.
Could you try it please?  If the change was very complex and large, we would give it up, leave FIXME comments, and fix only for <fieldset> and <keygen>.

[Kenichi Ishibashi]

Kent-san,

(In reply to comment #3)
> We need to add validation properties to HTMLObjectElement.  So making HTMObjectElement a subclass of HTMLFormControlElement is needed.
> Could you try it please?  If the change was very complex and large, we would give it up, leave FIXME comments, and fix only for <fieldset> and <keygen>.

I would be happy to try it. Thank you for your advice!

[Kenichi Ishibashi]

(In reply to comment #4)
> I would be happy to try it.

I've tried to make HTMLObjectElement to be a subclass of HTMLFormControlElement but gave it up so far.

One of the major difficulties is its class hierarchy. The easiest way to do that is inserting HTMLFormControlElement class in the class hierarchy. But HTMLFrameOwnerElement, which is the class to be a subclass of HTMLFormControlElement in that way, is the super class of other non-form associated elements. So it wouldn't be good idea. The second idea came up with me is using multiple inheritance. It might not be good idea as well because it will make diamond class inheritance, which is never used in WebKit AFAIK and I guess it is not allowed to use. Another possible workaround is introducing an interface class which represents a form associated element but is not a subclass of HTMLElement. This approach also doesn't work. checkValidity() of HTMLFormControlElement takes a vector of RefPtr of form associated elements as an argument. RefPtr requires its object to have ref() and deref(), so we need to define these function to the interface but they are also defined in HTMLElement and it will also bring the diamond inheritance problem. I couldn't figure out other convenient way to make HTMLObjectElement be a subclass of HTMLFormControlElement.

Do you have any suggestion?

[Kent Tamura]

(In reply to comment #5)
> Another possible workaround is introducing an interface class which represents a form associated element but is not a subclass of HTMLElement. This approach also doesn't work. checkValidity() of HTMLFormControlElement takes a vector of RefPtr of form associated elements as an argument. RefPtr requires its object to have ref() and deref(), so we need to define these function to the interface but they are also defined in HTMLElement and it will also bring the diamond inheritance problem.

I think it's doable.  Did you try it and have problems?

[Kenichi Ishibashi]

Kent-san,

(In reply to comment #6)
> I think it's doable.  Did you try it and have problems?

I tried introducing the interface class, say FormAssociatedElementInterface, and changed the argument of checkValidity from Vector<RefPtr<HTMLFormControlElement> to Vector<RefPtr<FormAssociatedElementInterface>. Since RefPtr requires ref() and deref(), I needed to define these functions, but they are also defined in EventTarget class, which is a superclass of HTMLElement subclasses. I think, in this case, virtual inheritance is needed to make both HTMLFormControlElement and HTMLObjectElement to be a subclass of not only HTMLElement but also FormAssociatedElementInterface. I can't come up with a workaround to avoid it.

[Kent Tamura]

(In reply to comment #7)
> Kent-san,
> 
> (In reply to comment #6)
> > I think it's doable.  Did you try it and have problems?
> 
> I tried introducing the interface class, say FormAssociatedElementInterface, and changed the argument of checkValidity from Vector<RefPtr<HTMLFormControlElement> to Vector<RefPtr<FormAssociatedElementInterface>. Since RefPtr requires ref() and deref(), I needed to define these functions, but they are also defined in EventTarget class, which is a superclass of HTMLElement subclasses. I think, in this case, virtual inheritance is needed to make both HTMLFormControlElement and HTMLObjectElement to be a subclass of not only HTMLElement but also FormAssociatedElementInterface. I can't come up with a workaround to avoid it.

Could you show me the patch please?
WebCore::Node inherits EventTarget, TreeShared, and ScriptWrappable.  Both of EventTarget and TreeShared have ref() and deref().

[Kenichi Ishibashi]

Created attachment 74088 [details]
A minimum diff

Thank you for your help. With attached a minimum patch, I got following errors:

In file included from /Users/bashi/webkit/work/WebCore/html/HTMLFormControlElement.cpp:49:
/Users/bashi/webkit/work/WebCore/html/ValidityState.h: In member function ‘void WebCore::ValidityState::ref()’:
/Users/bashi/webkit/work/WebCore/html/ValidityState.h:39: error: request for member ‘ref’ is ambiguous
/Users/bashi/webkit/work/WebCore/html/HTMLFormControlElement.h:41: error: candidates are: virtual void WebCore::FormAssociatedElementInterface::ref()
/Users/bashi/webkit/work/WebCore/platform/TreeShared.h:58: error:                 void WebCore::TreeShared<T>::ref() [with T = WebCore::ContainerNode]
/Users/bashi/webkit/work/WebCore/html/ValidityState.h: In member function ‘void WebCore::ValidityState::deref()’:
/Users/bashi/webkit/work/WebCore/html/ValidityState.h:40: error: request for member ‘deref’ is ambiguous
/Users/bashi/webkit/work/WebCore/html/HTMLFormControlElement.h:42: error: candidates are: virtual void WebCore::FormAssociatedElementInterface::deref()
/Users/bashi/webkit/work/WebCore/platform/TreeShared.h:67: error:                 void WebCore::TreeShared<T>::deref() [with T = WebCore::ContainerNode]
/Users/bashi/webkit/work/WebKitBuild/Debug/JavaScriptCore.framework/PrivateHeaders/PassRefPtr.h: In function ‘void WTF::refIfNotNull(T*) [with T = WebCore::HTMLFormControlElement]’:
/Users/bashi/webkit/work/WebKitBuild/Debug/JavaScriptCore.framework/PrivateHeaders/RefPtr.h:42:   instantiated from ‘WTF::RefPtr<T>::RefPtr(T*) [with T = WebCore::HTMLFormControlElement]’
/Users/bashi/webkit/work/WebCore/html/HTMLFormControlElement.cpp:374:   instantiated from here
/Users/bashi/webkit/work/WebKitBuild/Debug/JavaScriptCore.framework/PrivateHeaders/PassRefPtr.h:53: error: request for member ‘ref’ is ambiguous
/Users/bashi/webkit/work/WebCore/html/HTMLFormControlElement.h:41: error: candidates are: virtual void WebCore::FormAssociatedElementInterface::ref()
/Users/bashi/webkit/work/WebCore/platform/TreeShared.h:58: error:                 void WebCore::TreeShared<T>::ref() [with T = WebCore::ContainerNode]
/Users/bashi/webkit/work/WebKitBuild/Debug/JavaScriptCore.framework/PrivateHeaders/PassRefPtr.h: In function ‘void WTF::derefIfNotNull(T*) [with T = WebCore::HTMLFormControlElement]’:
/Users/bashi/webkit/work/WebKitBuild/Debug/JavaScriptCore.framework/PrivateHeaders/RefPtr.h:57:   instantiated from ‘WTF::RefPtr<T>::~RefPtr() [with T = WebCore::HTMLFormControlElement]’
/Users/bashi/webkit/work/WebCore/html/HTMLFormControlElement.cpp:374:   instantiated from here/Users/bashi/webkit/work/WebKitBuild/Debug/JavaScriptCore.framework/PrivateHeaders/PassRefPtr.h:59: error: request for member ‘deref’ is ambiguous
/Users/bashi/webkit/work/WebCore/html/HTMLFormControlElement.h:42: error: candidates are: virtual void WebCore::FormAssociatedElementInterface::deref()/Users/bashi/webkit/work/WebCore/platform/TreeShared.h:67: error:                 void WebCore::TreeShared<T>::deref() [with T = WebCore::ContainerNode]

[Kent Tamura]

(In reply to comment #9)
> Created an attachment (id=74088) [details]
> A minimum diff
> 
> Thank you for your help. With attached a minimum patch, I got following errors:
> 
> In file included from /Users/bashi/webkit/work/WebCore/html/HTMLFormControlElement.cpp:49:
> /Users/bashi/webkit/work/WebCore/html/ValidityState.h: In member function ‘void WebCore::ValidityState::ref()’:
> /Users/bashi/webkit/work/WebCore/html/ValidityState.h:39: error: request for member ‘ref’ is ambiguous

This ambiguity can be avoided by adding

    using TreeShared<ContainerNode>::ref;
    using TreeShared<ContainerNode>::deref;

to HTMLFormControlElement declaration.  See Node.h.

Having non-virtual ref()/deref() and virtual def()/deref() is confusing.  So FormAssociatedElementInterface (I don't like to add "Interface" to the name) definition should be similar to EventTarget.

class FormAssociatedElement {
    void ref() { refFormAssociatedElement(); }
    void deref() { derefFormAssociatedElement(); }
    virtual void refFormAssociatedElement() = 0;
    virtual void derefFormAssociatedElement() = 0;
};

HTMLFormControlElement should have:
void HTMLFormControlElement::refFormAssociatedElement() { ref(); }
void HTMLFormControlElement::derefFormAssociatedElement() { deref(); }

[Kenichi Ishibashi]

Kent-san,

Thank you for the solution! This is what I'm looking for. I'm going to try it again.

Regards,

(In reply to comment #10)
> (In reply to comment #9)
> > Created an attachment (id=74088) [details] [details]
> > A minimum diff
> > 
> > Thank you for your help. With attached a minimum patch, I got following errors:
> > 
> > In file included from /Users/bashi/webkit/work/WebCore/html/HTMLFormControlElement.cpp:49:
> > /Users/bashi/webkit/work/WebCore/html/ValidityState.h: In member function ‘void WebCore::ValidityState::ref()’:
> > /Users/bashi/webkit/work/WebCore/html/ValidityState.h:39: error: request for member ‘ref’ is ambiguous
> 
> This ambiguity can be avoided by adding
> 
>     using TreeShared<ContainerNode>::ref;
>     using TreeShared<ContainerNode>::deref;
> 
> to HTMLFormControlElement declaration.  See Node.h.
> 
> Having non-virtual ref()/deref() and virtual def()/deref() is confusing.  So FormAssociatedElementInterface (I don't like to add "Interface" to the name) definition should be similar to EventTarget.
> 
> class FormAssociatedElement {
>     void ref() { refFormAssociatedElement(); }
>     void deref() { derefFormAssociatedElement(); }
>     virtual void refFormAssociatedElement() = 0;
>     virtual void derefFormAssociatedElement() = 0;
> };
> 
> HTMLFormControlElement should have:
> void HTMLFormControlElement::refFormAssociatedElement() { ref(); }
> void HTMLFormControlElement::derefFormAssociatedElement() { deref(); }

[Yael]

*** Bug 50179 has been marked as a duplicate of this bug. ***

[Kenichi Ishibashi]

Created attachment 75205 [details]
Patch V0

[Kenichi Ishibashi]

(In reply to comment #13)
> Created an attachment (id=75205) [details]
> Patch V0

Changeset http://trac.webkit.org/changeset/72835 made both <fieldset> and <keygen> to be enumerable so this patch trying to make HTMLObjectElement to be a form associated element.

[Darin Adler]

Comment on attachment 75205 [details]
Patch V0

We don’t want to lose all the history for all the functions in HTMLFormControlElement that you moved to FormAssociatedElement. So what you want to do is use svn copy to make copies of the source files and then delete what you are not keeping.

You can do this after the fact by moving the files aside, doing the svn copy, then overwriting with your new files. This way we retain the past history of the functions in that file, and can see the changes you made instead of the whole file looking like new code.

[Kent Tamura]

Comment on attachment 75205 [details]
Patch V0

View in context: https://bugs.webkit.org/attachment.cgi?id=75205&action=review

> LayoutTests/fast/forms/script-tests/form-collection-elements-order.js:14
> +    if (owner.elements.length != victims.length)
> +      return 'length mismatch, elements.length = ' +
> +             owner.elements.length + ', expected length = ' + victims.length;
> +    for (var i = 0; i < victims.length; ++i)
> +      if (owner.elements[i] != victims[i])
> +        return 'element mismatch at index ' + i;
> +    return 'OK';

Indentation is inconsistent.

> WebCore/ChangeLog:6
> +        "elements" for form and fieldsset don't conform to the HTML5 spec
> +        https://bugs.webkit.org/show_bug.cgi?id=48821

We had better change the summary (and the bug title) because we address only <object>.

> WebCore/WebCore.xcodeproj/project.pbxproj:-21495
> -			developmentRegion = English;

Do not remove this line.  You need the latest Xcode.

> WebCore/html/FormAssociatedElement.cpp:2
> + * Copyright (c) 2010 Google Inc. All rights reserved.

This file contains a lot of code witten by non-Google. Please respect the original copyright header.

> WebCore/html/FormAssociatedElement.h:2
> + * Copyright (c) 2010 Google Inc. All rights reserved.

ditto.

[Kent Tamura]

Comment on attachment 75205 [details]
Patch V0

View in context: https://bugs.webkit.org/attachment.cgi?id=75205&action=review

> WebCore/html/HTMLFormControlElement.h:41
>  // FIXME: The HTML5 specification calls these form-associated elements.
>  // So consider renaming this to HTMLFormAssociatedElement.
> -class HTMLFormControlElement : public HTMLElement {
> +class HTMLFormControlElement : public HTMLElement, public FormAssociatedElement {

Need to update the comment.  You had better explain what's the new role of HTMLFormControlElement.

[Kenichi Ishibashi]

Hi Darin,

(In reply to comment #15)
> (From update of attachment 75205 [details])
> We don’t want to lose all the history for all the functions in HTMLFormControlElement that you moved to FormAssociatedElement. So what you want to do is use svn copy to make copies of the source files and then delete what you are not keeping.

Thank you for suggestions. I didn't aware of the history and you are absolutely right. I'll do this over.

Regards,

[Kenichi Ishibashi]

Kent-san,

Thank you for your prompt review. I'll apply all your comment in the next patch.

(In reply to comment #17)
> (From update of attachment 75205 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=75205&action=review
> 
> > WebCore/html/HTMLFormControlElement.h:41
> >  // FIXME: The HTML5 specification calls these form-associated elements.
> >  // So consider renaming this to HTMLFormAssociatedElement.
> > -class HTMLFormControlElement : public HTMLElement {
> > +class HTMLFormControlElement : public HTMLElement, public FormAssociatedElement {
> 
> Need to update the comment.  You had better explain what's the new role of HTMLFormControlElement.

[Kenichi Ishibashi]

Created attachment 75250 [details]
Patch V1

Used svn copy to generate FormAssociatedElement.{cpp,h} and followed kent-san's comments.

[Kent Tamura]

Comment on attachment 75250 [details]
Patch V1

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?

[Kenichi Ishibashi]

Comment on attachment 75250 [details]
Patch V1

View in context: https://bugs.webkit.org/attachment.cgi?id=75250&action=review

Kent-san,

Thank you for review. I'll post revised patch soon.

Two comments are not yet applied to ask your opinion:
- Flag variables and their setter/getter still remain in FormAssociatedElement.
- The return value of HTMLObjectElement::formControlType() is not changed.

I'll revise the patch again when I get your feedback.

Thanks,

>> WebCore/WebCore.xcodeproj/project.pbxproj:14746
>>  				B0149E7911A4B21500196A7B /* AsyncImageResizer.cpp */,
> 
> Should be sorted.

Done.

>> WebCore/WebCore.xcodeproj/project.pbxproj:21442
>> +				4A0DA2FF129B241900AB61E1 /* FormAssociatedElement.h in Headers */,
> 
> ditto.
> Appending new entry at the last causes patch-conflict easily.

Done.

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

Done.

>> WebCore/html/FormAssociatedElement.h:62
>> +    void setReadOnly(bool value) { m_readOnly = value; }
> 
> Do we need them in FormAssociatedElement?
> <object> doesn't have such properties.

Actually they are not required for <object>, but these flags are bit fields and would be stored into the same place(byte) of validation related bit fields so I thought it might be better putting these flags in FormAssociatedElement for memory consuming. Would it be better keeping these in HTMLFormControlElement?

>> WebCore/html/HTMLFormControlElement.h:42
>> +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.

Thank you for suggestion. Revised as you suggested.

>> WebCore/html/HTMLFormElement.cpp:478
>> +        ASSERT(e != m_associatedElements[i]);
> 
> Do you still need this?

Sorry, I forgot to remove it. Removed.

>> WebCore/html/HTMLObjectElement.cpp:496
>> +    return name.isNull() ? emptyAtom : name;
> 
> The variable "name" looks redundant.

Removed the temporal variable and just returning m_name.

>> WebCore/html/HTMLObjectElement.cpp:502
>> +    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?

Thank you for warning. Removed temporal object and added a member variables for it. As for the return value, HTML5 spec says the type attribute must be a valid MIME type (http://dev.w3.org/html5/spec/the-iframe-element.html#attr-object-type). That's why I tried to return m_serviceType here.

[Kenichi Ishibashi]

Created attachment 75293 [details]
Patch V2

[Kent Tamura]

Comment on attachment 75250 [details]
Patch V1

View in context: https://bugs.webkit.org/attachment.cgi?id=75250&action=review

>>> WebCore/html/FormAssociatedElement.h:62
>>> +    void setReadOnly(bool value) { m_readOnly = value; }
>> 
>> Do we need them in FormAssociatedElement?
>> <object> doesn't have such properties.
> 
> Actually they are not required for <object>, but these flags are bit fields and would be stored into the same place(byte) of validation related bit fields so I thought it might be better putting these flags in FormAssociatedElement for memory consuming. Would it be better keeping these in HTMLFormControlElement?

IMO, it's ok to split these flags into two classes.
BTW, do we really need to put validation members to FormAssociatedElement? <object> needs to have a ValidityState object, but other validation flags should be constant in <object>.

>>> WebCore/html/HTMLObjectElement.cpp:502
>>> +    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?
> 
> Thank you for warning. Removed temporal object and added a member variables for it. As for the return value, HTML5 spec says the type attribute must be a valid MIME type (http://dev.w3.org/html5/spec/the-iframe-element.html#attr-object-type). That's why I tried to return m_serviceType here.

Do we need formControlType() in FormAssociatedElement?  I think we can remove type() and formControlType() from FormAssociatedElement.  We had better minimize the number of methods of FormAssociatedElement.

[Darin Adler]

(In reply to comment #24)
> IMO, it's ok to split these flags into two classes.

I agree, but also, if we do decide to put all the flags in one class to try to save memory, we should use the pattern from Node/Element and not expose the public access to the flags until the class they belong on.

> BTW, do we really need to put validation members to FormAssociatedElement? <object> needs to have a ValidityState object, but other validation flags should be constant in <object>.

It’s useful to keep the FormAssociatedElement class minimal.

> HTML5 spec says the type attribute must be a valid MIME type (http://dev.w3.org/html5/spec/the-iframe-element.html#attr-object-type). That's why I tried to return m_serviceType here.

Sure, that’s the type attribute of HTMLObjectElement. I’m not sure FormAssociatedElement needs a type attribute at all.

> Do we need formControlType() in FormAssociatedElement?  I think we can remove type() and formControlType() from FormAssociatedElement.  We had better minimize the number of methods of FormAssociatedElement.

I agree.

[Kenichi Ishibashi]

Kent-san, Darin,

Thank you for your suggestions and comments. I'll update the patch following your comments.

(In reply to comment #25)
> (In reply to comment #24)
> > IMO, it's ok to split these flags into two classes.
> 
> I agree, but also, if we do decide to put all the flags in one class to try to save memory, we should use the pattern from Node/Element and not expose the public access to the flags until the class they belong on.
> 
> > BTW, do we really need to put validation members to FormAssociatedElement? <object> needs to have a ValidityState object, but other validation flags should be constant in <object>.
> 
> It’s useful to keep the FormAssociatedElement class minimal.
> 
> > HTML5 spec says the type attribute must be a valid MIME type (http://dev.w3.org/html5/spec/the-iframe-element.html#attr-object-type). That's why I tried to return m_serviceType here.
> 
> Sure, that’s the type attribute of HTMLObjectElement. I’m not sure FormAssociatedElement needs a type attribute at all.
> 
> > Do we need formControlType() in FormAssociatedElement?  I think we can remove type() and formControlType() from FormAssociatedElement.  We had better minimize the number of methods of FormAssociatedElement.
> 
> I agree.

[Kenichi Ishibashi]

Created attachment 75593 [details]
Patch V3

Tried to keep the FormAssociatedElement minimal.

[Kent Tamura]

Comment on attachment 75593 [details]
Patch V3

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])->hideVisibleValidationMessage();

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.

[Kenichi Ishibashi]

Comment on attachment 75593 [details]
Patch V3

View in context: https://bugs.webkit.org/attachment.cgi?id=75593&action=review

Kent-san,

Thank you for review. I'll post revised patch in a minute.

>> 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.

To unify them, I removed findFormOwner() and changed the access level of HTMLElement::findFormAncestor() from protected to public. Exposing findFormAncestor() might be controversial so please r- if you have objections.

>> WebCore/html/HTMLFormCollection.cpp:116
>> +            && element->getAttribute(attrName) == name) {
> 
> You don't need to wrap the line.

Done.

>> WebCore/html/HTMLFormControlElement.h:58
>> +    virtual void setDisabled(bool);
> 
> Do we need to add "virtual"?

Removed.

>> WebCore/html/HTMLFormElement.cpp:221
>> +            static_cast<HTMLFormControlElement*>(m_associatedElements[i])->hideVisibleValidationMessage();
> 
> We should add { } to the "for" because the content has two physical lines.

Done.

>> WebCore/html/HTMLFormElement.cpp:357
>> +            static_cast<HTMLFormControlElement*>(m_associatedElements[i])->reset();
> 
> ditto.

Done.

>> WebCore/html/HTMLFormElement.cpp:652
>> +            static_cast<HTMLFormControlElement*>(m_associatedElements[i])->reset();
> 
> ditto.

Done.

[Kenichi Ishibashi]

Created attachment 75648 [details]
Patch V4

[WebKit Review Bot]

Attachment 75648 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/6739053

[Build Bot]

Attachment 75648 [details] did not build on win:
Build output: http://queues.webkit.org/results/6855041

[Kent Tamura]

Comment on attachment 75648 [details]
Patch V4

Please update the ChangeLog entry, and fix build errors.
I have no other comments.

[Kenichi Ishibashi]

Created attachment 75663 [details]
Patch V5

Updated ChangeLogs and fixed build errors.

[Kent Tamura]

Comment on attachment 75663 [details]
Patch V5

View in context: https://bugs.webkit.org/attachment.cgi?id=75663&action=review

> WebKit/win/WebFrame.cpp:1144
> +            HTMLFormControlElement *elt = static_cast<HTMLFormControlElement*>(elements[i]);

It should be "HTMLFormControlElement* elt" (no space, *, space)

[Kenichi Ishibashi]

Created attachment 75756 [details]
Patch V6

Fixed style issue.

[WebKit Review Bot]

Attachment 75756 [details] did not pass style-queue:

Failed to run "[u'git', u'reset', u'--hard', u'HEAD']" exit_code: 128
error: Could not write new index file.
fatal: Could not reset index file to revision 'HEAD'.


If any of these errors are false positives, please file a bug against check-webkit-style.

[Kent Tamura]

Comment on attachment 75756 [details]
Patch V6

Looks good.  Thank you!

[WebKit Commit Bot]

Comment on attachment 75756 [details]
Patch V6

Rejecting patch 75756 from commit-queue.

Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=eseidel-cq-sl', 'apply-attachment', '--force-clean', '--non-interactive', 75756]" exit_code: 2
Last 500 characters of output:
LayoutTests/fast/forms/form-collection-elements-order-expected.txt
patching file LayoutTests/fast/forms/form-collection-elements-order.html
patching file LayoutTests/fast/forms/script-tests/form-attribute.js
patching file LayoutTests/fast/forms/script-tests/form-collection-elements-order.js
patching file LayoutTests/fast/forms/script-tests/form-collection-elements.js

Failed to run "[u'/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', u'--reviewer', u'Kent Tamura', u'--force']" exit_code: 1

Full output: http://queues.webkit.org/results/6748089

[Kenichi Ishibashi]

Created attachment 75772 [details]
Patch V7

[Kent Tamura]

Comment on attachment 75772 [details]
Patch V7

I'll commit manually.

[Kent Tamura]

Landed: http://trac.webkit.org/changeset/73430

[Kenichi Ishibashi]

Kent-san,

Thank you very much for taking a lot of time for this patch. I'm grateful to you.

(In reply to comment #42)
> Landed: http://trac.webkit.org/changeset/73430