Bug 89030 – el.getAttribute('type') returns 'null' when setting el.type to null

• Component: DOM
• Status: RESOLVED FIXED

[Edaena Salinas]

Created attachment 147388 [details]
test

After setting the type of a button to null using btn.type = null, btn.getAttribute('type') should return a JavaScript null and not the string 'null'.

[Edaena Salinas]

Created attachment 147413 [details]
Proposed fix and updated tests.

[Alexey Proskuryakov]

Comment on attachment 147413 [details]
Proposed fix and updated tests.

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

> Source/WebCore/ChangeLog:14
> +        * html/HTMLButtonElement.cpp:
> +        (WebCore::HTMLButtonElement::setType):
> +        * html/HTMLButtonElement.h:
> +        (HTMLButtonElement):
> +        * html/HTMLButtonElement.idl:

This is a good place to explain changes in your files. For example, the change from String to AtomicString definitely needs an explanation, because it's not even part of fixing this bug. E.g. "Address another unrelated review comment from bug 14439."

> LayoutTests/fast/dom/HTMLButtonElement/change-type.html:64
>      shouldBe("btn.getAttribute('type')", "undefined + ''");

As you are addressing unrelated feedback, you can consider changing this to "'undefined'", too.

[Alexey Proskuryakov]

> E.g. "Address another unrelated review comment from bug 14439."

Better yet, "Address another unrelated review comment from bug 14439 for slightly better performance."

[Darin Adler]

Comment on attachment 147413 [details]
Proposed fix and updated tests.

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

>> LayoutTests/fast/dom/HTMLButtonElement/change-type.html:64
>>      shouldBe("btn.getAttribute('type')", "undefined + ''");
> 
> As you are addressing unrelated feedback, you can consider changing this to "'undefined'", too.

I agree, you should do that.

[Darin Adler]

Comment on attachment 147413 [details]
Proposed fix and updated tests.

For what it’s worth, I agree with all of Alexey’s comments and suggest you post a new patch dealing with them.

[Edaena Salinas]

Created attachment 147424 [details]
Proposed fix and updated tests.

[Alexey Proskuryakov]

Comment on attachment 147424 [details]
Proposed fix and updated tests.

+        Reviewed by NOBODY (OOPS!).

For minor changes like these, you could have edited ChangeLogs to say "Reviewed by Darin Adler", and post a new patch with "commit-queue?" flag alone, no "review?" flag.

+        (HTMLButtonElement): Changed parameter to be AtomicString to address an unrelated 
+        review comment from bug 14439 for slightly better performance.

We traditionally just say "Ditto" for repeated comments.

[WebKit Review Bot]

Comment on attachment 147424 [details]
Proposed fix and updated tests.

Clearing flags on attachment: 147424

Committed r120296: <http://trac.webkit.org/changeset/120296>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.