[Webkit-unassigned] [Bug 47813] [HTML5] "form" attribute support for form control elements

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=47813


Kent Tamura <tkent at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #73705|review?                     |review-
               Flag|                            |




--- Comment #15 from Kent Tamura <tkent at chromium.org>  2010-11-12 00:32:03 PST ---
(From update of attachment 73705)
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);

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.