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

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> 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 79600: fix patch 2
https://bugs.webkit.org/attachment.cgi?id=79600&action=review

------- Additional Comments from Ryosuke Niwa <rniwa at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=79600&action=review

> Source/WebCore/ChangeLog:14
> +	   Related quotas: "On setting, if the new value is an ASCII
case-insensitive match for the 
> +	   string "inherit" then the content attribute must be removed, if the
new value is an ASCII 
> +	   case-insensitive match for the string "true" then the content
attribute must be set to the 
> +	   string "true", if the new value is an ASCII case-insensitive match
for the string "false" 
> +	   then the content attribute must be set to the string "false", and
otherwise the attribute 

You should probably replace " by ' inside your quotes.

> Source/WebCore/ChangeLog:19
> +	   (WebCore::HTMLElement::setContentEditable):
> +	   Throw exception on invalid input strings;

You should just do:
(WebCore::HTMLElement::setContentEditable): Throw exception on invalid input
strings;

> Source/WebCore/ChangeLog:22
> +	   * html/HTMLElement.h:
> +	   Add additional parameter ExceptionCode& for function
setContentEditable.

Put these two on the same line.

> Source/WebCore/ChangeLog:24
> +	   * html/HTMLElement.idl:
> +	   Add exception throwing support for contentEditable setter.

Ditto.

> LayoutTests/ChangeLog:20
> +	   * editing/editability/set-invalid-value-expected.txt:
> +	   * editing/editability/set-invalid-value.html:
> +	   1. Fixed the getAttribute expectation as "abc" was failed to set.
> +	   2. Added additional check for setting empty string.
> +	   * editing/editability/set-value-caseinsensitive-expected.txt:
> +	   * editing/editability/set-value-caseinsensitive.html:
> +	   Fixed the getAttribute expectations as all strings should be
converted to lower cases.
> +	   * fast/dom/element-attribute-js-null-expected.txt:
> +	   * fast/dom/element-attribute-js-null.html:
> +	   Fixed this existing test as the expectation for setting with null
should throw exception
> +	   instead of "false". Also added handling code when exception was
thrown.

Again, we usually start the explanation on the same line as the file name after
: with a space.

> LayoutTests/fast/dom/element-attribute-js-null.html:40
> +		   else
> +		       result = "<span class='fail'>TEST FAILED:</span> An
exception should have been thrown.";

I don't think this is right.  If an exception wasn't thrown then
exceptionThrown will be false, and we'll execute the else clause. r- because of
this. I think what you meant was
if (expected === null) {
    if (exceptionThrown)
	...
    else
	...
}