Bug 185416 – Empty <input type=file> is represented incorrectly in FormData

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Anne van Kesteren]

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.)

[Alexey Proskuryakov]

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.

[Andreu Botella]

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.

[Andreu Botella]

Created attachment 444216 [details]
Patch

[Andreu Botella]

Created attachment 444343 [details]
Patch

[EWS]

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].

[Radar WebKit Bug Importer]

<rdar://problem/85457351>

[Darin Adler]

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.

[Darin Adler]

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".

[Andreu Botella]

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?

[Darin Adler]

(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?

[Andreu Botella]

Fixing the code review suggestions in https://bugs.webkit.org/show_bug.cgi?id=233229