Bug 40084

Created attachment 57705 [details] Patch

Did you mean to mark this for review?

(In reply to comment #2) > Did you mean to mark this for review? Not yet, it depends on bug 39430 to land first.

Created attachment 58717 [details] Patch

Comment on attachment 57705 [details] Patch Whoops, webkit-patch upload for some reason didn't like the ChangeLog.

Created attachment 58721 [details] Patch

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

Comment on attachment 58721 [details] Patch Argh. More merge untangling.

Created attachment 58723 [details] Patch

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

Comment on attachment 58723 [details] Patch :( I'll figure out what's wrong.

Created attachment 58823 [details] Actually working patch

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.

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

(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 :)

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

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