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'.
Created attachment 147413 [details] Proposed fix and updated tests.
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.
> E.g. "Address another unrelated review comment from bug 14439." Better yet, "Address another unrelated review comment from bug 14439 for slightly better performance."
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.
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.
Created attachment 147424 [details] Proposed fix and updated tests.
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.
Comment on attachment 147424 [details] Proposed fix and updated tests. Clearing flags on attachment: 147424 Committed r120296: <http://trac.webkit.org/changeset/120296>
All reviewed patches have been landed. Closing bug.