| Summary: | Require <form> to be connected | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Anne van Kesteren <annevk> | ||||||
| Component: | DOM | Assignee: | Rob Buis <rbuis> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | cdumez, changseok, darin, esprehn+autocc, ews-watchlist, gyuyoung.kim, mifenton, rbuis, sam, webkit-bug-importer | ||||||
| Priority: | P2 | Keywords: | InRadar | ||||||
| Version: | WebKit Nightly Build | ||||||||
| Hardware: | Unspecified | ||||||||
| OS: | Unspecified | ||||||||
| Attachments: |
|
||||||||
|
Description
Anne van Kesteren
2017-09-22 05:14:19 PDT
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]. |