[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 #196947|review?                     |review-
               Flag|                            |




--- Comment #51 from Alexey Proskuryakov <ap at webkit.org>  2013-04-08 16:59:40 PST ---
(From update of attachment 196947)
View in context: https://bugs.webkit.org/attachment.cgi?id=196947&action=review

The spec says that the type should be normalized when Blob is created (with constructor or slice()). But in this patch, normalization is not done in the constructor. Is this intentional?

This can be observed through (new Blob([], {type:'<invalid string>'})).type value. Please make this match the spec, and add a new test case (in a separate file) for this.

> Source/WTF/ChangeLog:7
> +        * wtf/text/WTFString.h: usage comments for characters{,8,16}() and is8Bit()
> +        (WTF::String::is8Bit): now ASSERTs that the String is not null, so failures are more expressive

Please split this into a separate patch. This is not really related to the change you are making.

> Source/WebCore/ChangeLog:10
> +        (WebCore):

It's best to remove useless parts of generated ChangeLog. The part generated by prepare-ChangeLog is meant to be helpful to humans, not prescriptive.

> Source/WebCore/ChangeLog:14
> +        (Blob):

Ditto.

> Source/WebCore/fileapi/Blob.h:77
> +    // The checks described in the File API spec for Blob.slice().

I think that it's helpful to have this comment  - there are many definitions of "valid" indeed.

But it's not just about Blob.slice() - Blob constructor algorithm in the spec says the same. So, perhaps the comment should say something less specific, like maybe "// As defined by File API specification."

> Source/WebCore/fileapi/Blob.h:79
> +    static bool isValidContentType(const String& type);
> +    static String normalizedContentType(const String& contentType);

The argument names don't seem helpful, please remove them.

> Source/WebCore/platform/network/FormData.cpp:266
> +                String normalizedContentType = Blob::normalizedContentType(value.blob()->type());
>                  String contentType;
> -                if (value.blob()->type().isEmpty())
> +                if (normalizedContentType.isEmpty())
>                      contentType = "application/octet-stream";
>                  else
> -                    contentType = value.blob()->type();
> +                    contentType = normalizedContentType;

This can be written in a simpler way:

String contentType = Blob::normalizedContentType(value.blob()->type());
if (contentType.isEmpty())
    contentType = "application/octet-stream";

> LayoutTests/http/tests/local/formdata/send-form-data-filename-escaping.html:5
> +<script src="/js-test-resources/js-test-pre.js"></script>
> +<script src="../../../../fast/js/resources/js-test-pre.js"></script>

Why do you need both?

> LayoutTests/http/tests/local/formdata/send-form-data-filename-escaping.html:68
> +<script src="/js-test-resources/js-test-post.js"></script>
> +<script src="../../../../fast/js/resources/js-test-post.js"></script>

Ditto.

> LayoutTests/http/tests/local/formdata/send-form-data-mimetype-normalization.html:5
> +<script src="/js-test-resources/js-test-pre.js"></script>
> +<script src="../../../../fast/js/resources/js-test-pre.js"></script>

Ditto.

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

Ditto.

> LayoutTests/http/tests/local/formdata/resources/send-form-data-common.js:26
>  function dumpResponse(xhr, fileSliced)
>  {
>      debug(xhr.responseText);
> +    return xhr.responseText;
>  }

I don't quite understand the changes in this file. Could you please explain them in ChangeLog?

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