[webkit-reviews] review denied: [Bug 111380] Handle CRLF in MIME types of Blobs appended to multipart FormData : [Attachment 197025] Patch

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has denied Victor Costan
<costan at gmail.com>'s request for review:
Bug 111380: Handle CRLF in MIME types of Blobs appended to multipart FormData
https://bugs.webkit.org/show_bug.cgi?id=111380

Attachment 197025: Patch
https://bugs.webkit.org/attachment.cgi?id=197025&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=197025&action=review


I like the new behavior, but I think that another round is needed to improve
style.

> Source/WebCore/fileapi/Blob.h:48
> +// Utilities for handling a Blob's type attribute.
> +namespace BlobType {

It is unusual in WebKit code base to put helpers in a namespace. And we try to
not define two entities in one header file.

What you had in the previous iteration was perfectly fine, these functions can
be in Blob class.

> Source/WebCore/platform/network/BlobData.h:169
> +// Declared in Blob.h, which includes this header.
> +namespace BlobType {
> +bool isNormalized(const String&);
> +} // namespace WebCore::BlobType

I understand why you needed this, but it's not a great thing to do. The correct
things to do would be:

- put the functions in a separate header file,
- or just move setContentType() to BlobData.cpp. It's not hot code, and the
function does not need to be inline.

> Source/WebCore/platform/network/BlobData.h:183
> +	   // FIXME: This would be useful, but it causes a link error in
WebKit2.framework
> +	   // ASSERT(BlobType::isNormalized(contentType));

The reason why this fails to link is that the function needs to be exported
using WebCore.exp.in file.

>
LayoutTests/http/tests/local/formdata/send-form-data-mimetype-normalization.htm
l:57
> +<script src="/js-test-resources/js-test-post.js"></script>
> +<script src="../../../../fast/js/resources/js-test-post.js"></script>

I still don't understand why both are needed.

> LayoutTests/http/tests/local/formdata/resources/send-form-data-common.js:48
> +    if (sendAsAsync)
> +	   return null;
> +    else
> +	   return dumpResponse(xhr, fileSliced);

WebKit style is to not have an "else" after "return".

I'd still like to have an explanation of the changes to this file in a
ChageLog.