[Webkit-unassigned] [Bug 34930] Implement interactive validation for forms

[bugzilla-daemon at webkit.org]

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





--- Comment #23 from TAMURA, Kent <tkent at chromium.org>  2010-04-09 10:36:14 PST ---
Thank you for really helpful review!

(In reply to comment #20)
> > +description('Test if the form is submitted when an "invalid" evnet for a control is canceled.');
> Typo: "evnet"

Fixed. Fixed another "evnet" too.

> > +    Document* originalDocument = document();
> 
> This should be a RefPtr. I don't know what guarantees the document will still
> be around after dispatching the event in pathological cases. There there may be
> some subtle guarantee I can't spot, but I think it would be best to not make
> the code depend on something that subtle when using a RefPtr would eliminate
> the risk.

I see.  Changed to RefPtr<Document>.

> Also, the document == originalDocument check does not replace the inDocument
> check. I'm not sure why you removed the inDocument check. That covers elements
> that are no longer in the document tree. I think you should have a test case
> that covers that and restore the inDocument check.

Fixed.

> > +    HTMLFormControlElement* submitElement = 0;
> > +    Node* targetNode = event->target()->toNode();
> > +    if (targetNode && targetNode->isElementNode()) {
> > +        Element* targetElement = static_cast<Element*>(targetNode);
> > +        if (targetElement->isFormControlElement())
> > +            submitElement = static_cast<HTMLFormControlElement*>(targetElement);
> > +    }
> 
> Breaking this out into a helper function would make the code easier to read.
> Just a suggestion -- the function this code is in is getting a bit long.

Made a function.

> > +            Document* originalDocument = unhandled->document();
> May need to be a RefPtr for the same reason as above. Same inDocument issue as
> above.

Fixed.

> > +    Frame* frame = document()->frame();
> > +    for (unsigned i = 0; frame && i < unhandledInvalidControls.size(); ++i) {
> 
> Doesn't make sense to have "frame" in the loop condition. The value of "frame"
> does not change as we iterate through the loop.

Fixed.

> > +    RefPtr<HTMLFormElement> protector(this);
> > +    for (unsigned i = 0; i < formElements.size(); ++i)
> > +        formElements[i]->checkValidity(&unhandledInvalidControls);
> >  }
> 
> The protector here doesn't seem to cover all the issues. The code iterates the
> form elements vector while it may be changing. This means that we could end up
> iterating the same element more than once or skipping an element if an element
> is removed or added earlier in the vector than where we are in the iteration.
> We should figure out how we want to handle cases where the event dispatch adds
> or removes elements from the form. One approach would be to copy the elements
> into a Vector<RefPtr<...>> before we begin and iterate that.

Fixed.

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