[webkit-reviews] review denied: [Bug 111380] Handle CRLF in MIME types of Blobs appended to multipart FormData : [Attachment 196947] 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 196947: Patch
https://bugs.webkit.org/attachment.cgi?id=196947&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
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.htm
l: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.htm
l: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?