Bug 88392 – Remove unnecessary functions: setName() and formControlName()

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

[Kent Tamura]

Remove unnecessary functions: setName() and formControlName()

[Kent Tamura]

Created attachment 145935 [details]
Patch

[Kentaro Hara]

Comment on attachment 145935 [details]
Patch

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

Test cases look OK.

> Source/WebCore/ChangeLog:8
> +        'name' IDL attributes of form-related elements should be [Reflected],

Link to the spec please.

> Source/WebCore/ChangeLog:11
> +        just convert a null attribute to an empty string. Our binding code does it.

Nit: convert => converts

> Source/WebCore/html/FormAssociatedElement.cpp:226
> +    return name.isNull() ? emptyAtom : name;

What happens if you just return 'name' here? I guess that the generated code will convert a null string to an empty string. Maybe I am wrong though.

[Kent Tamura]

Comment on attachment 145935 [details]
Patch

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

>> Source/WebCore/ChangeLog:8
>> +        'name' IDL attributes of form-related elements should be [Reflected],
> 
> Link to the spec please.

will do.

>> Source/WebCore/ChangeLog:11
>> +        just convert a null attribute to an empty string. Our binding code does it.
> 
> Nit: convert => converts

will fix

>> Source/WebCore/html/FormAssociatedElement.cpp:226
>> +    return name.isNull() ? emptyAtom : name;
> 
> What happens if you just return 'name' here? I guess that the generated code will convert a null string to an empty string. Maybe I am wrong though.

If so, we'll see many crashes in our code. e.g. WebCore/dom/CheckedRadioButtons.cpp assumes name() never be NULL and HashMap<StringImpl> doesn't accept NULL.

[Kentaro Hara]

Comment on attachment 145935 [details]
Patch

Thanks for the clarification. The patch looks good to me.

[Kent Tamura]

Created attachment 145946 [details]
Patch for landing

[Kent Tamura]

(In reply to comment #5)
> Created an attachment (id=145946) [details]
> Patch for landing

I found this patch caused some regressions. Probably because of lifetime change of an AtomicString of HTMLInputElement::name().

[Kent Tamura]

Created attachment 146769 [details]
Patch 2

[Kent Tamura]

(In reply to comment #6)
> (In reply to comment #5)
> > Created an attachment (id=145946) [details] [details]
> > Patch for landing
> 
> I found this patch caused some regressions. Probably because of lifetime change of an AtomicString of HTMLInputElement::name().

There were two problems:
 * We still had Element::formControlName(), and some callsites of it.
 * HTMLInputElment has the following code:

    if (attribute.name() == nameAttr) {
        removeFromRadioButtonGroup();
        m_name = attribute.value();
        addToRadioButtonGroup();

HTMLInputELement::name() should return the old name value for removeFromRadioButtonGroup(), and should return the new name value for addToRadioButtonGroup(). So we can't remove m_name.

[Kentaro Hara]

Comment on attachment 146769 [details]
Patch 2

Looks OK to me.

[Kent Tamura]

Comment on attachment 146769 [details]
Patch 2

Thanks!

[WebKit Review Bot]

Comment on attachment 146769 [details]
Patch 2

Clearing flags on attachment: 146769

Committed r120049: <http://trac.webkit.org/changeset/120049>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.