[webkit-reviews] review granted: [Bug 40084] Introduce FormSubmission, the structure representing a form submission. : [Attachment 58823] Actually working 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 40084: Introduce FormSubmission, the structure representing a form
submission.
https://bugs.webkit.org/show_bug.cgi?id=40084

Attachment 58823: Actually working patch
https://bugs.webkit.org/attachment.cgi?id=58823&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
>      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