Bug 177356 – Require <form> to be connected

• Component: DOM
• Status: RESOLVED FIXED

[Anne van Kesteren]

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

[Rob Buis]

Created attachment 402755 [details]
Patch

[Darin Adler]

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.

[Rob Buis]

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.

[Darin Adler]

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.

[Rob Buis]

Created attachment 402955 [details]
Patch

[EWS]

Committed r263624: <https://trac.webkit.org/changeset/263624>

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

[Radar WebKit Bug Importer]

<rdar://problem/64844813>