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

[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 79484: fix patch
https://bugs.webkit.org/attachment.cgi?id=79484&action=review

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

The code change itself looks good but r- because the change log entry lacks
sufficient explanation as to why and what change is made.

> Source/WebCore/ChangeLog:10
> +	   1. Updated idl file and function signature to throw exception when
necessary
> +	   2. Threw exception on invalid strings
> +	   2. Made setting values case-insensitive

You should explain why you're making this change. Namely, refer to the part of
the spec this change is required:
http://www.whatwg.org/specs/web-apps/current-work/multipage/editing.html#attr-c
ontenteditable

Also, you seem to be also making the return value to be "true", "false", or
"inherit" and not returning values such as "TRue" and "FaLSe" as tested below. 
And we should definitely be explaining that in the change log.

> Source/WebCore/html/HTMLElement.idl:63
> +		   attribute [ConvertNullToNullString] DOMString
contentEditable
> +		       setter raises(DOMException);

I think you need to have one more space before attribute and setter to align
with other attributes.

> LayoutTests/ChangeLog:8
> +	   Updated expected results after the fix. Also corrected test cases.

Please explain what changes you have made.

> LayoutTests/ChangeLog:15
> +	   * fast/dom/element-attribute-js-null-expected.txt:
> +	   * fast/dom/element-attribute-js-null.html:

You should explain why this test had to be modified.