[webkit-reviews] review denied: [Bug 29363] [HTML5][Forms] Support for <output> element : [Attachment 72521] Patch V1

[bugzilla-daemon at webkit.org]

Kent Tamura <tkent at chromium.org> has denied Kenichi Ishibashi
<bashi at google.com>'s request for review:
Bug 29363: [HTML5][Forms] Support for <output> element
https://bugs.webkit.org/show_bug.cgi?id=29363

Attachment 72521: Patch V1
https://bugs.webkit.org/attachment.cgi?id=72521&action=review

------- Additional Comments from Kent Tamura <tkent at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=72521&action=review

>
LayoutTests/fast/dom/HTMLOutputElement/script-tests/dom-settable-token-list.js:
40
> +element.htmlFor.add('x');
shouldBeEqualToString('element.htmlFor.toString()', 'x');

Insert a line-break after ";"

>
LayoutTests/fast/dom/HTMLOutputElement/script-tests/dom-settable-token-list.js:
78
> +debug('Ensure that we can handle empty form attribute correctly');

Would you add debug('') before this line, or add "-" at the beginning of the
message like other tests for readability of the test result please?

>
LayoutTests/fast/dom/HTMLOutputElement/script-tests/dom-settable-token-list.js:
80
> +list = element.htmlFor;

Should prepend "var "

>
LayoutTests/fast/dom/HTMLOutputElement/script-tests/dom-settable-token-list.js:
91
> +element.htmlFor.add('x')

What's the expected result of these remove() and add()?

>
LayoutTests/fast/dom/HTMLOutputElement/script-tests/htmloutputelement-htmlfor.j
s:1
> +description('Tests for the htmlFor attribute of the output element.');

Many test cases in this file look equivalent to tests in
dom-settable-token-list.js.  Are they needed?

>
LayoutTests/fast/dom/HTMLOutputElement/script-tests/htmloutputelement-value.js:
7
> +output  = document.createElement('output');

Redundant spaces before "="

> WebCore/html/HTMLOutputElement.cpp:83
> +void HTMLOutputElement::childrenChanged(bool createdByParser, Node*
/*beforeChange*/, Node* /*afterChange*/, int /*childCountDelta*/)

You don't need to keep unused parameter names.

> WebCore/html/HTMLOutputElement.cpp:133
> +    setTextContent(value, ec);

Better to have ASSERT(!m_isSetTextContentInProgress)

> WebCore/html/HTMLOutputElement.idl:35
> +	   readonly attribute boolean willValidate;

You need a test for willValidate behavior.