Bug 39430 – Refactor form submission code in HTMLFormElement to remove hairballs and add logical clarity.

• Component: New Bugs
• Status: RESOLVED FIXED

[Dimitri Glazkov (Google)]

Refactor form submission code in HTMLFormElement to remove hairballs and add logical clarity.

[Dimitri Glazkov (Google)]

Created attachment 56603 [details]
Patch

[Dimitri Glazkov (Google)]

Created attachment 57046 [details]
Patch

[Darin Adler]

Comment on attachment 57046 [details]
Patch

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?

[Dimitri Glazkov (Google)]

(In reply to comment #3)
> (From update of attachment 57046 [details])
> 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.

These are all good points. What do you think about moving this method to FormDataBuilder? It looks very builder-y.

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

Yup, good point, too. Since this is a private method with one callsite, we can just rename it to what it actually does.

> >      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?

With mailto, the identifier never gets to be used -- the result is emptied out after being appended to form's URL. The test coverage is pretty thorough in fast/forms/mailto.

I'll put up a new patch in a few.

[Dimitri Glazkov (Google)]

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

By the way, I'll investigate these separately. The setting of the attr doesn't seem good at all. For now, I am just moving the code (and adding a fixme).

[Dimitri Glazkov (Google)]

Created attachment 57594 [details]
Patch

[Dimitri Glazkov (Google)]

To give you a bit more context, this is part of a series of patches to introduce a proper form submission abstraction/flow. See bug 40084 for the next patch (not yet ready for review).

[Adam Barth]

I tried looking at your patch but it was too hard for me.  I'll try again if you don't find a reviewer who is more familiar with this code.

[Dimitri Glazkov (Google)]

(In reply to comment #8)
> I tried looking at your patch but it was too hard for me.  I'll try again if you don't find a reviewer who is more familiar with this code.

No worries, Darin will look when he has a chance. Right Darin? :)

[Darin Adler]

Comment on attachment 57594 [details]
Patch

> +            // 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().

[Dimitri Glazkov (Google)]

Thanks for the review!

(In reply to comment #10)
> (From update of attachment 57594 [details])
> > +            // 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.

Exactly -- I already investigated this and fixed just the way you're suggesting in the patch on bug 40184.

> 
> >      return result;
> 
> This really should be returning result.release().

Good point! Will fix on landing.

[Dimitri Glazkov (Google)]

Committed r61152: <http://trac.webkit.org/changeset/61152>

[Eric Seidel (no email)]

Seems to have caused a Gtk API test regression:

ERROR:../../WebKit/gtk/tests/testatkroles.c:98:get_child_and_test_role: assertion failed: (child_role == role)

[Dimitri Glazkov (Google)]

(In reply to comment #13)
> Seems to have caused a Gtk API test regression:
> 
> ERROR:../../WebKit/gtk/tests/testatkroles.c:98:get_child_and_test_role: assertion failed: (child_role == role)

Weeeeird. I wonder how that's possible. Xan, Martin -- any ideas?

[Eric Seidel (no email)]

https://bugs.webkit.org/show_bug.cgi?id=40133 is the culprit.