[webkit-reviews] review denied: [Bug 34930] Implement interactive validation for forms : [Attachment 52900] Proposed patch (rev.7)

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied TAMURA, Kent <tkent at chromium.org>'s
request for review:
Bug 34930: Implement interactive validation for forms
https://bugs.webkit.org/show_bug.cgi?id=34930

Attachment 52900: Proposed patch (rev.7)
https://bugs.webkit.org/attachment.cgi?id=52900&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
Basics look great here, but the edge cases are still not quite right.

> +description('Test if the form is submitted when an "invalid" evnet for a
control is canceled.');

Typo: "evnet"

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

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.

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

> +	       Document* originalDocument = unhandled->document();

May need to be a RefPtr for the same reason as above. Same inDocument issue as
above.

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

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