[webkit-reviews] review denied: [Bug 39430] Refactor form submission code in HTMLFormElement to remove hairballs and add logical clarity. : [Attachment 57046] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Dimitri Glazkov (Google)
<dglazkov at chromium.org>'s request for review:
Bug 39430: Refactor form submission code in HTMLFormElement to remove hairballs
and add logical clarity.
https://bugs.webkit.org/show_bug.cgi?id=39430

Attachment 57046: Patch
https://bugs.webkit.org/attachment.cgi?id=57046&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
Looks like a good idea. I have a few concerns.

> +	   * html/HTMLFormElement.cpp:
> +	   (WebCore::transferMailtoPostFormDataToURL): Removed clearing out of
the FormData and
> +	       moved it to a new place (next to where it's now used).

To me, the name of this function implies that the data is moved from one place
into the other. The use of the verb "transfer" as opposed to one more commonly
used in software such as "set" is indicating that. So the name isn't quite as
good as it was before given the change in behavior of the function. I suggest
changing the name of the function and maybe the order of the arguments to make
it clearer this function now sets the URL and doesn't touch its other argument.
For the same reason, the argument can now be "const FormData&" now instead of
"FormData*" and probably should go after the URL argument.

>  PassRefPtr<FormData> HTMLFormElement::createFormData()

The old createFormData function used to create a FormData object without side
effects. But now the function has side effects:

    - It can set the enctypeAttr of the form element.
    - It can change the state of the form data builder, clearing the is
multi-part-form flag.
    - It can change the URL, putting the form data there instead of in the
FormData object.

This reduces the amount of code in HTMLFormElement::submit at the cost of
making the createFormData function a bigger one that has more diffuse
responsibilities.

Can we avoid this? Maybe add a new function called something like
prepareFormData could be the outer function with the additional code in it
instead of putting it all into createFormData.

>      result->setIdentifier(generateFormDataIdentifier());

This line of code now runs after transferMailtoPostFormDataToURL, and it used
to run before. Why doesn't that give us a different result with mailto? Is
there test coverage of this?