[webkit-reviews] review denied: [Bug 47813] [HTML5] "form" attribute support for form control elements : [Attachment 73705] Patch V3

[bugzilla-daemon at webkit.org]

Kent Tamura <tkent at chromium.org> has denied Kenichi Ishibashi
<bashi at google.com>'s request for review:
Bug 47813: [HTML5] "form" attribute support for form control elements
https://bugs.webkit.org/show_bug.cgi?id=47813

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

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

> WebCore/html/HTMLFormElement.cpp:403
> +static int compareTreeOrder(Node* n1, int depth1, Node* n2, int depth2)

- 'n1' 'n2" are not good names.  They should be node1 and node2.
- This function should belong to Document class.
- The "int" return value is not good.  We should introduce new enum.

> WebCore/html/HTMLFormElement.cpp:415
> +    // Keeping the nearest sucessors of each nodes for later use.

typo and grammer: "successor of each node"

> WebCore/html/HTMLFormElement.cpp:420
> +    while (ancestor1 && ancestor2) {
> +	   if (ancestor1 == ancestor2)
> +	       break;

You can write "while (ancestor1 && ancestor2 && ancestor1 != ancestor2) {"

> WebCore/html/HTMLFormElement.cpp:441
> +    ASSERT(successor1 && successor2);

successor1 or successor2 can be 0 if n1 is an ancestor of n2, or vice versa. 
It never happens in the form attribute case, but we should support it because
we'd like to move this function to Document.

BTW, you shouldn't use && in ASSERT().	You should have written
ASSERT(successor1);
ASSERT(successor2);