https://html.spec.whatwg.org/#form-submission-attributes https://html.spec.whatwg.org/#form-submission-algorithm
<rdar://problem/78682625>
Created attachment 432499 [details] Patch
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?
(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.
Created attachment 432506 [details] Patch
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().
Created attachment 432507 [details] Patch
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).
Created attachment 432509 [details] Patch
Comment on attachment 432509 [details] Patch Let's make sure the bots are happy before landing.
Looks like imported/w3c/web-platform-tests/html/dom/reflection-forms.html needs rebaselining.
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.
Created attachment 432521 [details] Patch
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.
Created attachment 432524 [details] Patch
Comment on attachment 432524 [details] Patch Doesn't address Darin's feedback.
Created attachment 432529 [details] Patch
Created attachment 432531 [details] Patch
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.
Created attachment 432566 [details] Patch
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].