[webkit-reviews] review granted: [Bug 197958] Implement <form>.requestSubmit() : [Attachment 427667] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Rob Buis <rbuis at igalia.com>'s request
for review:
Bug 197958: Implement <form>.requestSubmit()
https://bugs.webkit.org/show_bug.cgi?id=197958

Attachment 427667: Patch

https://bugs.webkit.org/attachment.cgi?id=427667&action=review




--- Comment #44 from Darin Adler <darin at apple.com> ---
Comment on attachment 427667
  --> https://bugs.webkit.org/attachment.cgi?id=427667
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=427667&action=review

> Source/WebCore/html/HTMLFormElement.cpp:330
> +    // Update layout before processing form actions in case the style
changes
> +    // the Form or button relationships.
> +    document().updateLayoutIgnorePendingStylesheets();

Given our comment it seems wrong to do this *after* checking the control’s
membership of the form. That sure sounds like a form and button relationship!

Our comment leaves it unclear why we need this here, but do not need it for
other callers of the prepareForSubmission function. Let’s explain our thinking.

I suggest lowercasing "Form" here.

> Source/WebCore/html/HTMLFormElement.cpp:388
> +	   for (auto& associatedElement : m_associatedElements) {
> +	       if (!is<HTMLFormControlElement>(*associatedElement))
> +		   continue;
> +	       if (needButtonActivation) {
> +		   HTMLFormControlElement& control =
downcast<HTMLFormControlElement>(*associatedElement);
> +		   if (control.isActivatedSubmit())
> +		       needButtonActivation = false;
> +		   else if (!firstSuccessfulSubmitButton &&
control.isSuccessfulSubmitButton())
> +		       firstSuccessfulSubmitButton = &control;
> +	       }
>	   }

This loop seems like something that should be factored out into a named
function. Could even pass the submitter and include the other side of the if
statement too. The return value would be a struct or pair with both the button
and the boolean.

> Source/WebCore/html/HTMLFormElement.h:80
> +    void prepareForSubmission(Event*, HTMLFormControlElement* = nullptr,
FormSubmissionTrigger = NotSubmittedByJavaScript); // FIXME: This function
doesn't only prepare, it sometimes calls submit() itself.

This kind of FIXME seems unnecessary. It’s implying we should rename the
function. So let us do that! We can rename a function!

> Source/WebCore/loader/FormSubmission.cpp:149
> +    RefPtr<HTMLFormControlElement> submitter = makeRefPtr(overrideSubmitter
? overrideSubmitter : form.findSubmitter(event));

auto