[webkit-reviews] review denied: [Bug 52057] setContentEditable with invalid string should throw exception : [Attachment 79612] fix patch 3

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Chang Shu <Chang.Shu at nokia.com>'s
request for review:
Bug 52057: setContentEditable with invalid string should throw exception
https://bugs.webkit.org/show_bug.cgi?id=52057

Attachment 79612: fix patch 3
https://bugs.webkit.org/attachment.cgi?id=79612&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=79612&action=review

review- because the set-invalid-value.html test is incorrect

> Source/WebCore/html/HTMLElement.cpp:732
> -void HTMLElement::setContentEditable(const String &enabled)
> +void HTMLElement::setContentEditable(const String &enabled, ExceptionCode&
ec)

Since you are touching this line of code, you should change the incorrect
"String & enabled" to the correctly-styled "String& enabled".

> Source/WebCore/html/HTMLElement.cpp:737
> +	   setAttribute(contenteditableAttr, "true");
> +    else if (equalIgnoringCase(enabled, "false"))
> +	   setAttribute(contenteditableAttr, "false");

It would be slightly better to pass "ec" in these two calls to setAttribute.
Probably no real effect, but the version without the exception code argument
ignores exceptions, and I don’t think we need to ignore them.

> LayoutTests/editing/editability/set-invalid-value.html:22
>  

This entire test should be in dom/HTMLInputElement/script-tests rather than in
editing/editability. Not something we have to fix now.

> LayoutTests/editing/editability/set-invalid-value.html:34
> +try {
> +    document.getElementById("div").contentEditable = "";
> +} catch (e) {
> +    if (e.code == 12)
> +	   exceptionThrown = true;
> +}

This test is wrong. At the start of this block of code, the exceptionThrown
variable is already true, so it will pass no matter what. It also would be
better to use shouldThrow in all of these tests that are checking for an
exception instead of hand-rolling an exception testing code path. An example of
how to use shouldThrow is in
dom/HTMLElement/script-tests/insertAdjacentHTML-errors.js and there are many
other examples too.

> LayoutTests/fast/dom/element-attribute-js-null.html:38
> +		       result = "<span class='pass'>TEST SUCCEEDED:</span> The
exception was thrown as expected.";

It would be better if this checked what kind of exception was thrown.