Bug 66321 – Handle "form" attribute updates in parseMappedAttribute() instead of attributeChanged() to better match HTMLElement practices

• Component: New Bugs
• Status: RESOLVED FIXED

[Adam Klein]

Handle "form" attribute updates in parseMappedAttribute() instead of attributeChanged() to better match HTMLElement practices

[Adam Klein]

Created attachment 104072 [details]
Patch

[Adam Klein]

Background: I was trying to understand the implementation of WebKit's form attribute, and was confused by the fact that formAttr didn't show up in parseMappedAttribute.  This change should be a no-op, but imho makes the code easier to understand.

[Darin Adler]

Comment on attachment 104072 [details]
Patch

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

Do we have test coverage for this? I suggest removing the three things done by attributeChanged one at a time and verifying that in each case an existing regression test fails. If not, then we should add regression tests. Since the benefit level of this is low (makes the code slightly more consistent and elegant) we want to be doubly sure we are not breaking anything.

I’m going to say review+ but I do not think this should go in unless we are sure we have test coverage.

> Source/WebCore/html/HTMLObjectElement.cpp:90
> +    if (attr->name() == formAttr) {
> +        formAttributeChanged();
> +    } else if (attr->name() == typeAttr) {

WebKit coding style says to not use braces around a single-line body like this one.

[Adam Klein]

Created attachment 104236 [details]
Patch

[Adam Klein]

Created attachment 104237 [details]
Patch

[Adam Klein]

(In reply to comment #3)
> (From update of attachment 104072 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=104072&action=review
> 
> Do we have test coverage for this? I suggest removing the three things done by attributeChanged one at a time and verifying that in each case an existing regression test fails. If not, then we should add regression tests. Since the benefit level of this is low (makes the code slightly more consistent and elegant) we want to be doubly sure we are not breaking anything.

Thanks for the suggestion! Though this looked at first glance to be covered, it turns out only one of the three things done by these attributeChanged was covered. I've added coverage for both of those.
 
> I’m going to say review+ but I do not think this should go in unless we are sure we have test coverage.

Thanks.  Care to look again?

> > Source/WebCore/html/HTMLObjectElement.cpp:90
> > +    if (attr->name() == formAttr) {
> > +        formAttributeChanged();
> > +    } else if (attr->name() == typeAttr) {
> 
> WebKit coding style says to not use braces around a single-line body like this one.

Fixed.

[Erik Arvidsson]

Comment on attachment 104237 [details]
Patch

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

> LayoutTests/fast/forms/script-tests/form-attribute.js:100
> +    '<object id=objectElement name=victim />';

object requires a closing tag... this works in innerHTML because it is the last element and we hit the error case.

[Adam Klein]

Created attachment 104249 [details]
Patch for landing

[Adam Klein]

(In reply to comment #7)
> (From update of attachment 104237 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=104237&action=review
> 
> > LayoutTests/fast/forms/script-tests/form-attribute.js:100
> > +    '<object id=objectElement name=victim />';
> 
> object requires a closing tag... this works in innerHTML because it is the last element and we hit the error case.

Fixed here and in above, where this already existed in the test.

[WebKit Review Bot]

Comment on attachment 104249 [details]
Patch for landing

Clearing flags on attachment: 104249

Committed r93292: <http://trac.webkit.org/changeset/93292>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.