Bug 40084 – Introduce FormSubmission, the structure representing a form submission.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Dimitri Glazkov (Google)]

Introduce FormSubmission, the structure representing a form submission.

[Dimitri Glazkov (Google)]

Created attachment 57705 [details]
Patch

[Adam Barth]

Did you mean to mark this for review?

[Dimitri Glazkov (Google)]

(In reply to comment #2)
> Did you mean to mark this for review?

Not yet, it depends on bug 39430 to land first.

[Dimitri Glazkov (Google)]

Created attachment 58717 [details]
Patch

[Dimitri Glazkov (Google)]

Comment on attachment 57705 [details]
Patch

Whoops, webkit-patch upload for some reason didn't like the ChangeLog.

[Dimitri Glazkov (Google)]

Created attachment 58721 [details]
Patch

[Early Warning System Bot]

Attachment 58717 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/3281139

[Dimitri Glazkov (Google)]

Comment on attachment 58721 [details]
Patch

Argh. More merge untangling.

[Dimitri Glazkov (Google)]

Created attachment 58723 [details]
Patch

[WebKit Review Bot]

Attachment 58723 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3288158

[Dimitri Glazkov (Google)]

Comment on attachment 58723 [details]
Patch

:( I'll figure out what's wrong.

[Dimitri Glazkov (Google)]

Created attachment 58823 [details]
Actually working patch

[Dimitri Glazkov (Google)]

This now works and doesn't regress anything. Chromium fails are because the EWS doesn't seem to refresh the project after applying the patch. Please take a look.

[Darin Adler]

Comment on attachment 58823 [details]
Actually working patch

>      for (unsigned i = 0; i < m_associatedElements.size(); ++i) {
>          HTMLFormControlElement* control = m_associatedElements[i];
>          if (!control->disabled())
>              control->appendFormData(*domFormData, m_formDataBuilder.isMultiPartForm());
> +        if (control->hasLocalName(inputTag)) {
> +            HTMLInputElement* input = static_cast<HTMLInputElement*>(control);
> +            if (input->isTextField()) {
> +                formValues.append(pair<String, String>(input->name(), input->value()));
> +                if (input->isSearchField())
> +                    input->addSearchResult();
> +            }
> +        }
>      }

This redundancy between form data and form values is so bizarre. I can't believe we have it. And for the form data we have a nice abstraction and virtual functions, but for the form values, none at all! We need to fix that.

And the call to addSearchResult here is so random too! Anyway, not your fault, but nutty.

> +    FormSubmission::Method method = m_formDataBuilder.isPostMethod() ? FormSubmission::PostMethod : FormSubmission::GetMethod;
> +    return FormSubmission::create(method, m_url, m_target, m_formDataBuilder.encodingType(), FormState::create(this, formValues, document()->frame(), trigger), formData.release(), boundary, lockHistory, event);

Even I, famous for long lines, would be tempted to break this into multiple lines. Maybe put the FormState into a local variable.

It occurs to me that FormSubmission, FormData, and FormState are all too similar. Too bad, but might be worth thinking about. Do we need all three? Are they all easy to explain?

> +FormSubmission::FormSubmission(Method method, const String& action, const String& target, const String& contentType, PassRefPtr<FormState> state, PassRefPtr<FormData> data, const String& boundary, bool lockHistory, PassRefPtr<Event> event)

I suggest marking this inline since it's called only in one place.

> +{
> +}
> +
> +
> +PassRefPtr<FormSubmission> FormSubmission::create(Method method, const String& action, const String& target, const String& contentType, PassRefPtr<FormState> state, PassRefPtr<FormData> data, const String& boundary, bool lockHistory, PassRefPtr<Event> event)

Extra blank line here.

> +class FormSubmission : public RefCounted<FormSubmission> {

Do we really need a reference counted object for this, and a class? Why not just use a struct, and pass const FormSubmission& to the function that takes it? I'm not sure that a class adds enough value here.

> +    enum Method {
> +        GetMethod,
> +        PostMethod
> +    };

I’d put this on a single line instead of breaking it across multiple lines.

> +    String action() const { return m_action; }
> +    String target() const { return m_target; }
> +    String contentType() const { return m_contentType; }

> +    String boundary() const { return m_boundary; }

More efficient to return const String& from functions like these, and very little downside to doing so.

r=me

[Dimitri Glazkov (Google)]

(In reply to comment #14)
> (From update of attachment 58823 [details])
> >      for (unsigned i = 0; i < m_associatedElements.size(); ++i) {
> >          HTMLFormControlElement* control = m_associatedElements[i];
> >          if (!control->disabled())
> >              control->appendFormData(*domFormData, m_formDataBuilder.isMultiPartForm());
> > +        if (control->hasLocalName(inputTag)) {
> > +            HTMLInputElement* input = static_cast<HTMLInputElement*>(control);
> > +            if (input->isTextField()) {
> > +                formValues.append(pair<String, String>(input->name(), input->value()));
> > +                if (input->isSearchField())
> > +                    input->addSearchResult();
> > +            }
> > +        }
> >      }
> 
> This redundancy between form data and form values is so bizarre. I can't believe we have it. And for the form data we have a nice abstraction and virtual functions, but for the form values, none at all! We need to fix that.

I agree. That's my goal.

> And the call to addSearchResult here is so random too! Anyway, not your fault, but nutty.
> 
> > +    FormSubmission::Method method = m_formDataBuilder.isPostMethod() ? FormSubmission::PostMethod : FormSubmission::GetMethod;
> > +    return FormSubmission::create(method, m_url, m_target, m_formDataBuilder.encodingType(), FormState::create(this, formValues, document()->frame(), trigger), formData.release(), boundary, lockHistory, event);
> 
> Even I, famous for long lines, would be tempted to break this into multiple lines. Maybe put the FormState into a local variable.

Sounds good.

> 
> It occurs to me that FormSubmission, FormData, and FormState are all too similar. Too bad, but might be worth thinking about. Do we need all three? Are they all easy to explain?

Out of the three, I want FormState to go away. It already name-conflicts with a completely separate concept of restoring form state (see Document::formElementsWithState, et al.).

> 
> > +FormSubmission::FormSubmission(Method method, const String& action, const String& target, const String& contentType, PassRefPtr<FormState> state, PassRefPtr<FormData> data, const String& boundary, bool lockHistory, PassRefPtr<Event> event)
> 
> I suggest marking this inline since it's called only in one place.

ok.

> > +{
> > +}
> > +
> > +
> > +PassRefPtr<FormSubmission> FormSubmission::create(Method method, const String& action, const String& target, const String& contentType, PassRefPtr<FormState> state, PassRefPtr<FormData> data, const String& boundary, bool lockHistory, PassRefPtr<Event> event)
> 
> Extra blank line here.
> 
> > +class FormSubmission : public RefCounted<FormSubmission> {
> 
> Do we really need a reference counted object for this, and a class? Why not just use a struct, and pass const FormSubmission& to the function that takes it? I'm not sure that a class adds enough value here.

I've been back and forth on it. So far, I don't see the need for it to be ref-counted -- the ownership flows forward-only, so yes, simpler the better.

> > +    enum Method {
> > +        GetMethod,
> > +        PostMethod
> > +    };
> 
> I’d put this on a single line instead of breaking it across multiple lines.

ok.

> 
> > +    String action() const { return m_action; }
> > +    String target() const { return m_target; }
> > +    String contentType() const { return m_contentType; }
> 
> > +    String boundary() const { return m_boundary; }
> 
> More efficient to return const String& from functions like these, and very little downside to doing so.

ok.

> 
> r=me

Would it be ok for me to land this as-is and address the issues in a separate patch? I have two patches sitting on top of this and my last round of merge resolution was a b*tch :)

[Darin Adler]

(In reply to comment #15)
> Would it be ok for me to land this as-is and address the issues in a separate patch? I have two patches sitting on top of this and my last round of merge resolution was a b*tch :)

Yes, it’s OK with me. If it wasn’t OK I would have said review-.

[Dimitri Glazkov (Google)]

Committed r61511: <http://trac.webkit.org/changeset/61511>