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

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted 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 57594: Patch
https://bugs.webkit.org/attachment.cgi?id=57594&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
> +	       // FIXME: This may fire a DOM Mutation Event. Do we really want
this here?

I don’t see any reason to capitalize “Mutation Event”. Also, I’m not sure about
the use of the word “may” here. It either will fire the event or won‘t. Maybe
the use of “may” is related to the fact that most likely no one is listening so
we will optimize out the work.

> +	       setEnctype("application/x-www-form-urlencoded");

It’s unfortunate that this function has to have a side effect at all. We should
explore whether the form element’s enctype actually needs to be changed. I’m
guessing that could be incorrect behavior. Sure, we want this encoding type to
be used, but doing that by mutating the DOM seems wrong.

Thus the comment seems to miss a point that is even more pertinent than the the
mutation event. Do we want to mutate the form element at all in this case, or
is there a better way to handle it? Maybe the form data builder or some other
transient object could hold the encoding type.

>      return result;

This really should be returning result.release().