[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





--- Comment #22 from Kenichi Ishibashi <bashi at google.com>  2010-12-01 11:03:15 PST ---
(From update of attachment 75250)
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.

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