Bug 48821 - Let HTMLObjectElement be a form associated element
Summary: Let HTMLObjectElement be a form associated element
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Normal
Assignee: Nobody
URL: http://www.w3.org/TR/html5/forms.html...
Keywords:
: 50179 (view as bug list)
Depends on:
Blocks: 13061 50662 50663
  Show dependency treegraph
 
Reported: 2010-11-01 23:53 PDT by Kenichi Ishibashi
Modified: 2010-12-08 15:28 PST (History)
10 users (show)

See Also:


Attachments
A minimum diff (1.95 KB, text/plain)
2010-11-17 01:06 PST, Kenichi Ishibashi
no flags Details
Patch V0 (108.80 KB, patch)
2010-11-30 14:32 PST, Kenichi Ishibashi
tkent: review-
Details | Formatted Diff | Diff
Patch V1 (155.01 KB, patch)
2010-11-30 22:12 PST, Kenichi Ishibashi
tkent: review-
Details | Formatted Diff | Diff
Patch V2 (178.17 KB, patch)
2010-12-01 11:05 PST, Kenichi Ishibashi
no flags Details | Formatted Diff | Diff
Patch V3 (167.83 KB, patch)
2010-12-03 20:23 PST, Kenichi Ishibashi
tkent: review-
Details | Formatted Diff | Diff
Patch V4 (146.28 KB, patch)
2010-12-05 22:56 PST, Kenichi Ishibashi
tkent: review-
Details | Formatted Diff | Diff
Patch V5 (154.26 KB, patch)
2010-12-06 03:26 PST, Kenichi Ishibashi
no flags Details | Formatted Diff | Diff
Patch V6 (154.63 KB, patch)
2010-12-06 17:05 PST, Kenichi Ishibashi
tkent: review+
commit-queue: commit-queue-
Details | Formatted Diff | Diff
Patch V7 (154.65 KB, patch)
2010-12-06 19:25 PST, Kenichi Ishibashi
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kenichi Ishibashi 2010-11-01 23:53:01 PDT
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.
Comment 1 Kent Tamura 2010-11-02 00:31:32 PDT
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.
Comment 2 Kenichi Ishibashi 2010-11-03 20:41:34 PDT
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,
Comment 3 Kent Tamura 2010-11-03 21:14:23 PDT
(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>.
Comment 4 Kenichi Ishibashi 2010-11-03 21:35:31 PDT
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!
Comment 5 Kenichi Ishibashi 2010-11-16 16:52:44 PST
(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?
Comment 6 Kent Tamura 2010-11-16 18:46:38 PST
(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?
Comment 7 Kenichi Ishibashi 2010-11-16 23:02:21 PST
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.
Comment 8 Kent Tamura 2010-11-16 23:19:49 PST
(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().
Comment 9 Kenichi Ishibashi 2010-11-17 01:06:47 PST
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]
Comment 10 Kent Tamura 2010-11-17 01:27:04 PST
(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(); }
Comment 11 Kenichi Ishibashi 2010-11-17 08:43:27 PST
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(); }
Comment 12 Yael 2010-11-30 12:21:40 PST
*** Bug 50179 has been marked as a duplicate of this bug. ***
Comment 13 Kenichi Ishibashi 2010-11-30 14:32:31 PST
Created attachment 75205 [details]
Patch V0
Comment 14 Kenichi Ishibashi 2010-11-30 14:37:59 PST
(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.
Comment 15 Darin Adler 2010-11-30 15:03:43 PST
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.
Comment 16 Kent Tamura 2010-11-30 18:11:13 PST
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.
Comment 17 Kent Tamura 2010-11-30 18:16:56 PST
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.
Comment 18 Kenichi Ishibashi 2010-11-30 18:50:09 PST
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,
Comment 19 Kenichi Ishibashi 2010-11-30 18:54:11 PST
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.
Comment 20 Kenichi Ishibashi 2010-11-30 22:12:15 PST
Created attachment 75250 [details]
Patch V1

Used svn copy to generate FormAssociatedElement.{cpp,h} and followed kent-san's comments.
Comment 21 Kent Tamura 2010-12-01 01:43:16 PST
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?
Comment 22 Kenichi Ishibashi 2010-12-01 11:03:15 PST
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.
Comment 23 Kenichi Ishibashi 2010-12-01 11:05:42 PST
Created attachment 75293 [details]
Patch V2
Comment 24 Kent Tamura 2010-12-02 02:20:37 PST
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.
Comment 25 Darin Adler 2010-12-02 11:58:21 PST
(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.
Comment 26 Kenichi Ishibashi 2010-12-03 19:39:38 PST
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.
Comment 27 Kenichi Ishibashi 2010-12-03 20:23:24 PST
Created attachment 75593 [details]
Patch V3

Tried to keep the FormAssociatedElement minimal.
Comment 28 Kent Tamura 2010-12-05 21:37:25 PST
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.
Comment 29 Kenichi Ishibashi 2010-12-05 22:54:38 PST
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.
Comment 30 Kenichi Ishibashi 2010-12-05 22:56:05 PST
Created attachment 75648 [details]
Patch V4
Comment 31 WebKit Review Bot 2010-12-05 23:27:55 PST
Attachment 75648 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/6739053
Comment 32 Build Bot 2010-12-05 23:54:22 PST
Attachment 75648 [details] did not build on win:
Build output: http://queues.webkit.org/results/6855041
Comment 33 Kent Tamura 2010-12-06 01:36:07 PST
Comment on attachment 75648 [details]
Patch V4

Please update the ChangeLog entry, and fix build errors.
I have no other comments.
Comment 34 Kenichi Ishibashi 2010-12-06 03:26:22 PST
Created attachment 75663 [details]
Patch V5

Updated ChangeLogs and fixed build errors.
Comment 35 Kent Tamura 2010-12-06 15:59:32 PST
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)
Comment 36 Kenichi Ishibashi 2010-12-06 17:05:02 PST
Created attachment 75756 [details]
Patch V6

Fixed style issue.
Comment 37 WebKit Review Bot 2010-12-06 17:10:58 PST
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.
Comment 38 Kent Tamura 2010-12-06 17:12:10 PST
Comment on attachment 75756 [details]
Patch V6

Looks good.  Thank you!
Comment 39 WebKit Commit Bot 2010-12-06 18:09:54 PST
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
Comment 40 Kenichi Ishibashi 2010-12-06 19:25:27 PST
Created attachment 75772 [details]
Patch V7
Comment 41 Kent Tamura 2010-12-07 01:40:08 PST
Comment on attachment 75772 [details]
Patch V7

I'll commit manually.
Comment 42 Kent Tamura 2010-12-07 01:41:33 PST
Landed: http://trac.webkit.org/changeset/73430
Comment 43 Kenichi Ishibashi 2010-12-07 01:47:08 PST
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