Bug 148849 – [iOS] Need a test for bug #145539: Uploading an animated GIF from the photo library uploads a JPEG

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Jon Honeycutt]

Need a test for bug #145539: Uploading an animated GIF from the photo library uploads a JPEG.

[Jon Honeycutt]

Created attachment 260651 [details]
Patch

[Daniel Bates]

Comment on attachment 260651 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=260651&action=review

> ManualTests/ios/image-file-uploads-as-original-type.html:9
> +                    if (!arrayBuffer || arrayBuffer.byteLength < 6) {

Instead of hardcoding the length of the GIF header (6), I suggest that we use the expression '"GIF89a".length' so as to make it clear to a reader how we came to require that the file contain at least 6 bytes:

if (!arrayBuffer || arrayBuffer.byteLength < "GIF89a".length) {

> ManualTests/ios/image-file-uploads-as-original-type.html:15
> +                    var view = new Uint8Array(arrayBuffer, 0, 3);
> +                    var signature = String.fromCharCode(view[0], view[1], view[2]);
> +                    if (signature !== "GIF") {

This is OK as-is. Since we are requiring that the file contain at least six bytes we may want to consider ensuring that signature is equal to the string "GIF89a". If you choose to compare to "GIF89a" then I suggest we define a constant say GIFHeader := "GIF89a" and then write line 9, line 13, and line 15 in terms of it.

> ManualTests/ios/image-file-uploads-as-original-type.html:16
> +                        document.getElementById("console").innerHTML = "TEST FAILED. File was not of correct type.";

This is OK as-is. It is sufficient to use either textContent or innerText here since there is no markup in the string.

> ManualTests/ios/image-file-uploads-as-original-type.html:19
> +                    document.getElementById("console").innerHTML = "TEST PASSED";

Ditto.

> ManualTests/ios/image-file-uploads-as-original-type.html:41
> +            <input type=file onchange="runTest(this)">

The attribute value quoting style for attribute type (no quotes around "file") differs from the quoting style used for other HTML attribute values in this file. I suggest that we add quotes around the attribute value for attribute type. Regardless, we should pick an attribute quote style and stick with it throughout this file for consistency.

[Jon Honeycutt]

Committed r189410: <http://trac.webkit.org/changeset/189410>

[Jon Honeycutt]

(In reply to comment #2)
Thanks for the review! I made your suggested changes before landing.