Test: https://github.com/w3c/web-platform-tests/pull/10865. (Discovered while making an editorial change to the HTML Standard: https://github.com/whatwg/html/pull/3658.)
Same as bug 184490? This bug has no substance other than the title and some links that sound like they would take excessive time to turn into actionable information.
It seems like this bug was about how a FormData object should represent empty <input type="file"> entries with an "application/octet-stream" content type. That is what the WPT PR is testing (although that test file has since been expanded to cover bug 221549), and see https://github.com/whatwg/html/pull/3658#discussion_r185426252 re: the conversation in the editorial change to HTML.
Created attachment 444216 [details] Patch
Created attachment 444343 [details] Patch
Committed r285861 (244287@main): <https://commits.webkit.org/244287@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 444343 [details].
<rdar://problem/85457351>
Comment on attachment 444343 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=444343&action=review > Source/WebCore/html/FileInputType.cpp:189 > + auto file = File::create( > + document, > + Blob::create(document, { }, "application/octet-stream"), > + emptyString()); This vertical formatting is not normally WebKit coding style, although it is technically allowed. I would prefer not to switch to it. Writing "application/octet-stream" here isn’t as good as the WebCore::defaultMIMEType() function, which is more efficient than creating a string from a C literal each time, and it's also good to not have to worry about typos. We should use that function here. As a follow-up, someone should also use that function in at least: WebCore::FileReaderLoader::convertToDataURL, WebCore::FormData::appendMultiPartFileValue, WebCore::XMLHttpRequest::overrideMimeType, and WebKit::LegacyDownloadClient::legacyDidStart. And we should also consider making a function for comparing the string for use in suggestedFilenameWithMIMEType and WebKit::NetworkResourceLoader::didReceiveResponse.
Comment on attachment 444343 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=444343&action=review > Source/WebCore/html/FileInputType.cpp:182 > + // application/octet-stream content type. Null would be more logical, but I don’t think this comment change is an improvement; mentioning exactly what the code does is a non-goal for comments. The comment needs to cover "why", but no reason for it to say exactly "what".
Comment on attachment 444343 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=444343&action=review >> Source/WebCore/html/FileInputType.cpp:182 >> + // application/octet-stream content type. Null would be more logical, but > > I don’t think this comment change is an improvement; mentioning exactly what the code does is a non-goal for comments. The comment needs to cover "why", but no reason for it to say exactly "what". I read "empty" as meaning "with the default settings", which would include an empty content-type. But you're right, the code makes that clear anyway. I'll revert this change. >> Source/WebCore/html/FileInputType.cpp:189 >> + emptyString()); > > This vertical formatting is not normally WebKit coding style, although it is technically allowed. I would prefer not to switch to it. > > Writing "application/octet-stream" here isn’t as good as the WebCore::defaultMIMEType() function, which is more efficient than creating a string from a C literal each time, and it's also good to not have to worry about typos. We should use that function here. > > As a follow-up, someone should also use that function in at least: WebCore::FileReaderLoader::convertToDataURL, WebCore::FormData::appendMultiPartFileValue, WebCore::XMLHttpRequest::overrideMimeType, and WebKit::LegacyDownloadClient::legacyDidStart. And we should also consider making a function for comparing the string for use in suggestedFilenameWithMIMEType and WebKit::NetworkResourceLoader::didReceiveResponse. I think this code was reformatted by my IDE, using clang-format I believe, and since webkit-patch did not complain, I assumed that was fine. My bad for not actually checking that it followed the coding style. I suppose the right way would be having all the parameters in one line? I did not know that WebCore::defaultMIMEType() was a thing. Should I open a new bug for these changes?
(In reply to Andreu Botella from comment #9) > I think this code was reformatted by my IDE, using clang-format I believe, > and since webkit-patch did not complain, I assumed that was fine. My bad for > not actually checking that it followed the coding style. I suppose the right > way would be having all the parameters in one line? I think so. > Should I open a new bug for these changes? Sure, why not?
Fixing the code review suggestions in https://bugs.webkit.org/show_bug.cgi?id=233229