[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





--- Comment #16 from Kenichi Ishibashi <bashi at google.com>  2010-11-12 02:55:56 PST ---
(In reply to comment #15)
Kent-san,

Thank you for your quick review.

> (From update of attachment 73705 [details])
> 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.

I see. I'll move the function to Document class. So, should we also make nodeDepth() belong to other class, say, Node class?

> > WebCore/html/HTMLFormElement.cpp:415
> > +    // Keeping the nearest sucessors of each nodes for later use.
> 
> typo and grammer: "successor of each node"

Thanks, I'll fix it.

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

I'll fix it.

> 
> > 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.

I'll remove it when I moves the function to the Document class.

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

I see. Thank you for suggestion:-)

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