[Webkit-unassigned] [Bug 111380] Handle CRLF in MIME types of Blobs appended to multipart FormData

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=111380


Alexey Proskuryakov <ap at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #197025|review?                     |review-
               Flag|                            |




--- Comment #55 from Alexey Proskuryakov <ap at webkit.org>  2013-04-09 10:41:59 PST ---
(From update of attachment 197025)
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.html: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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.