[webkit-reviews] review denied: [Bug 30491] Pressing Enter in <isindex> doesn't submit it if there is no form : [Attachment 41814] Patch with test cases

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Daniel Bates <dbates at webkit.org>'s
request for review:
Bug 30491: Pressing Enter in <isindex> doesn't submit it if there is no form
https://bugs.webkit.org/show_bug.cgi?id=30491

Attachment 41814: Patch with test cases
https://bugs.webkit.org/attachment.cgi?id=41814&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
> +	   RefPtr<HTMLFormElement> myForm = form();

I don’t think the "my" prefix is helpful to explain this local variable's
purpose. By definition, this->form() is "my form", so the prefix simply creates
confusion. I suggest formForSubmission as the name here.

> +	   bool isIsIndexElementAndThereIsNoForm = (inputType() == ISINDEX) &&
!form();

It seems a waste to call form() again when we have a local variable with that
same value in it. Also, those parentheses are not needed, and we normally omit
them although I suppose they could confuse newcomers to C programming.

> +	   if (isIsIndexElementAndThereIsNoForm) {
> +	       // Since this <isindex> is not contained within a <form>, we
create one.
> +	       myForm = new HTMLFormElement(formTag, document());
> +	       myForm->registerFormElement(this);
> +	       myForm->setMethod("GET");
> +	       if (!document()->baseURL().isEmpty()) {
> +		   // We treat the href property of the <base> element as the
form action, as per section 7.5 
> +		   // "Queries and Indexes" of the HTML 2.0 spec.
<http://www.w3.org/MarkUp/html-spec/html-spec_7.html#SEC7.5>.
> +		   myForm->setAction(document()->baseURL().string());
> +	       }	    
> +	   }

Putting so much code in line into this function just for this element is
inelegant. I suggest creating a separate function to create the temporary form.
It could be a non-member function or if you prefer it could be a private member
function named createTemporaryForm which returns a PassRefPtr<HTMLFormElement>.
Then the code here would be just:

    RefPtr<HTMLFormElement> formForSubmission = form();

    // If there is no form and the element is an <isindex>, then create a
temporary form just to be used for submission.
    if (!formForSubmission && inputType() == ISINDEX)
	formForSubmission = createTemporaryForm(this).

    // Form may never have been present, or may have been destroyed by code
responding to the change event.
    if (formForSubmission)
	formForSubmission->submitClick(evt);

I think that's easier to read.

> +	   if (isIsIndexElementAndThereIsNoForm)
> +	       myForm.clear();

This is not needed. A RefPtr will automatically release its value when it goes
out of scope and there's no need to ensure it's dereferenced before calling
setDefaultHandled.

Since the <isindex> is one of the form's elements, isn't there a chance this
might call dispatchSimulatedClick on the <isindex>? What prevents this from
happening? If it does happen, will it do the right thing? Why won't this just
end up creating another temporary form in an infinite loop?

Do we guarantee this form won't be seen by any JavaScript code? Is there any
way an event handler might somehow see this event and get its target and so get
at the underlying form?

> +If you are running this test by hand, submit the phrase: "This is a test"
(without quotes).

Doesn't that text automatically get put into the field by the JavaScript even
when running the test by hand? I think that instruction is unneeded, but OK.

Why does this need to be an HTTP test? We can do form submissions with local
URLs. Can these same tests just be put in fast/forms instead of http/tests/misc
and run without requiring the http server? Is it in the http section because of
the base element?

This patch seems pretty good and probably ready to go, but I had enough
questions and suggestions that I'm going to say review- for now.