web-platform-tests: http://w3c-test.org/html/semantics/forms/form-submission-0/submission-checks.window.html HTML Standard PR: https://github.com/whatwg/html/pull/2613
Created attachment 402755 [details] Patch
Comment on attachment 402755 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=402755&action=review > Source/WebCore/html/HTMLFormElement.cpp:337 > + // FIXME: duplicated in case prepareForSubmission was called. I don’t understand what "duplicated" means here. Could we be use a few more words and be explicit. I don’t understand what "in case prepareForSubmission was called. This doesn’t match our "sentence-case" comment formatting. Maybe this is the comment we want? // The prepareForSubmission function also does this check, but need to do it here too, since there are some code paths that bypass that function. But honestly, I don’t know if that’s what we’re trying to say.
Comment on attachment 402755 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=402755&action=review >> Source/WebCore/html/HTMLFormElement.cpp:337 >> + // FIXME: duplicated in case prepareForSubmission was called. > > I don’t understand what "duplicated" means here. Could we be use a few more words and be explicit. > > I don’t understand what "in case prepareForSubmission was called. > > This doesn’t match our "sentence-case" comment formatting. > > Maybe this is the comment we want? > > // The prepareForSubmission function also does this check, but need to do it here too, since there are some code paths that bypass that function. > > But honestly, I don’t know if that’s what we’re trying to say. Your statement is better. Basically I do not like to check for isConnected in two places, my wording was probably off (I guess I wrote it so that I would understand what to do, but maybe not others :)). How about "FIXME: ideally we have only one place to check isConnected in the submit process"? Or I can leave the comment off if we do not think it is a big deal.
Comment on attachment 402755 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=402755&action=review >>> Source/WebCore/html/HTMLFormElement.cpp:337 >>> + // FIXME: duplicated in case prepareForSubmission was called. >> >> I don’t understand what "duplicated" means here. Could we be use a few more words and be explicit. >> >> I don’t understand what "in case prepareForSubmission was called. >> >> This doesn’t match our "sentence-case" comment formatting. >> >> Maybe this is the comment we want? >> >> // The prepareForSubmission function also does this check, but need to do it here too, since there are some code paths that bypass that function. >> >> But honestly, I don’t know if that’s what we’re trying to say. > > Your statement is better. Basically I do not like to check for isConnected in two places, my wording was probably off (I guess I wrote it so that I would understand what to do, but maybe not others :)). How about "FIXME: ideally we have only one place to check isConnected in the submit process"? Or I can leave the comment off if we do not think it is a big deal. I would not comment that "it would be better to only check once". The best purpose for a comment is to help people the reason behind something non-obvious. So a comment here would answer "why is this check also here since it’s already checked elsewhere" or be omitted.
Created attachment 402955 [details] Patch
Committed r263624: <https://trac.webkit.org/changeset/263624> All reviewed patches have been landed. Closing bug and clearing flags on attachment 402955 [details].
<rdar://problem/64844813>