Bug 226172 – Implement form[method=dialog]

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Tim Nguyen (:ntim)]

https://html.spec.whatwg.org/#form-submission-attributes
https://html.spec.whatwg.org/#form-submission-algorithm

[Radar WebKit Bug Importer]

<rdar://problem/78682625>

[Tim Nguyen (:ntim)]

Created attachment 432499 [details]
Patch

[Chris Dumez]

Comment on attachment 432499 [details]
Patch

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

> Source/WebCore/html/HTMLFormElement.cpp:421
> +    HTMLDialogElement* dialog = ancestorsOfType<HTMLDialogElement>(*this).first();

Ref dialog = ancestorsOfType<HTMLDialogElement>(*this).first();

would be safer.

> Source/WebCore/html/ImageInputType.cpp:220
> +    return String::number(m_clickLocation.x()) + "," + String::number(m_clickLocation.y());

You probably want:
return makeString(m_clickLocation.x(), ',', m_clickLocation.y());

for better perf.

You'll likely need to include <wtf/text/StringConcatenateNumbers.h>

> Source/WebCore/loader/FormSubmission.cpp:266
> +        frameRequest.resourceRequest().setHTTPBody(WTFMove(m_formData));

Assuming m_formData can be used later on, you probably should use std::exchange(), not WTFMove().

Also, is it really OK to consume m_formData? Cannot populateFrameLoadRequest() be called several times?

> Source/WebCore/loader/FormSubmission.h:48
> +    enum class Method : int { Get, Post, Dialog };

int -> uint8_t

> Source/WebCore/loader/FormSubmission.h:96
> +    FormData& data() const { return *m_formData; }

This looks dangerous, why is it safe?

[Tim Nguyen (:ntim)]

(In reply to Chris Dumez from comment #3)
> Comment on attachment 432499 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=432499&action=review
> 
> > Source/WebCore/html/HTMLFormElement.cpp:421
> > +    HTMLDialogElement* dialog = ancestorsOfType<HTMLDialogElement>(*this).first();
> 
> Ref dialog = ancestorsOfType<HTMLDialogElement>(*this).first();
> 
> would be safer.

I've used RefPtr, since this can be null (when there are no ancestors), there's a check just below handling that case.

> > Source/WebCore/html/ImageInputType.cpp:220
> > +    return String::number(m_clickLocation.x()) + "," + String::number(m_clickLocation.y());
> 
> You probably want:
> return makeString(m_clickLocation.x(), ',', m_clickLocation.y());
> 
> for better perf.
> 
> You'll likely need to include <wtf/text/StringConcatenateNumbers.h>

Done, thanks.

> > Source/WebCore/loader/FormSubmission.h:48
> > +    enum class Method : int { Get, Post, Dialog };
> 
> int -> uint8_t
>

Done.

> > Source/WebCore/loader/FormSubmission.cpp:266
> > +        frameRequest.resourceRequest().setHTTPBody(WTFMove(m_formData));
> 
> Assuming m_formData can be used later on, you probably should use
> std::exchange(), not WTFMove().
> 
> Also, is it really OK to consume m_formData? Cannot
> populateFrameLoadRequest() be called several times?
>
>
> > Source/WebCore/loader/FormSubmission.h:96
> > +    FormData& data() const { return *m_formData; }
> 
> This looks dangerous, why is it safe?

I mostly changed these bits because I needed m_formData to be optional to construct, I thought of using std::optional<Ref<>>, but it's not much cleaner. Happy to take any suggestions.

[Tim Nguyen (:ntim)]

Created attachment 432506 [details]
Patch

[Chris Dumez]

Comment on attachment 432506 [details]
Patch

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

r=me with changes.

> Source/WebCore/html/HTMLFormElement.cpp:421
> +    RefPtr<HTMLDialogElement> dialog = ancestorsOfType<HTMLDialogElement>(*this).first();

RefPtr<HTMLDialogElement> -> RefPtr

> Source/WebCore/html/ImageInputType.cpp:220
> +    return String::number(m_clickLocation.x()) + "," + String::number(m_clickLocation.y());

As mentioned earlier, this is inefficient and should be using makeString(), NOT String::number().

[Tim Nguyen (:ntim)]

Created attachment 432507 [details]
Patch

[Chris Dumez]

Comment on attachment 432507 [details]
Patch

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

> Source/WebCore/html/HTMLFormElement.cpp:421
> +    RefPtr<HTMLDialogElement> dialog = ancestorsOfType<HTMLDialogElement>(*this).first();

nit: RefPtr<HTMLDialogElement> -> RefPtr (with C++17, the compiler should be able to deduce the type).

[Tim Nguyen (:ntim)]

Created attachment 432509 [details]
Patch

[Chris Dumez]

Comment on attachment 432509 [details]
Patch

Let's make sure the bots are happy before landing.

[Chris Dumez]

Looks like imported/w3c/web-platform-tests/html/dom/reflection-forms.html needs rebaselining.

[Darin Adler]

Comment on attachment 432509 [details]
Patch

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

> Source/WebCore/loader/FormSubmission.cpp:184
> +        String returnValue = submitter ? submitter->resultForDialogSubmit() : "";

emptyString() is more efficient than “” when passing a String. Null string is even more efficient if it’s acceptable.

> Source/WebCore/loader/FormSubmission.h:55
> +        static ASCIILiteral methodString(Method method)

When function body becomes multi-line like this we prefer to put the inline function definition outside the class definition, at the namespace level to preserve readability of the class definition.

[Tim Nguyen (:ntim)]

Created attachment 432521 [details]
Patch

[Chris Dumez]

Comment on attachment 432521 [details]
Patch

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

> Source/WebCore/loader/FormSubmission.cpp:184
> +        String returnValue = submitter ? submitter->resultForDialogSubmit() : "";

Darin recommended:
String returnValue = submitter ? submitter->resultForDialogSubmit() : emptyString();

As it is more efficient.

> Source/WebCore/loader/FormSubmission.h:55
> +        static ASCIILiteral methodString(Method method)

Ditto, please see Darin's feedback about moving this outside the class.

[Tim Nguyen (:ntim)]

Created attachment 432524 [details]
Patch

[Chris Dumez]

Comment on attachment 432524 [details]
Patch

Doesn't address Darin's feedback.

[Tim Nguyen (:ntim)]

Created attachment 432529 [details]
Patch

[Tim Nguyen (:ntim)]

Created attachment 432531 [details]
Patch

[Chris Dumez]

Comment on attachment 432531 [details]
Patch

Looks like the baseline for imported/w3c/web-platform-tests/html/semantics/interactive-elements/the-dialog-element/dialog-form-submission.html on iOS-wk2 is missing a blank line, causing a failure on iOS-EWS.

[Tim Nguyen (:ntim)]

Created attachment 432566 [details]
Patch

[EWS]

Committed r279401 (239266@main): <https://commits.webkit.org/239266@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 432566 [details].