Bug 148849 - [iOS] Need a test for bug #145539: Uploading an animated GIF from the photo library uploads a JPEG
Summary: [iOS] Need a test for bug #145539: Uploading an animated GIF from the photo l...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: Safari 9
Hardware: Unspecified iOS 9.0
: P2 Normal
Assignee: Jon Honeycutt
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-09-04 16:37 PDT by Jon Honeycutt
Modified: 2015-09-04 17:59 PDT (History)
3 users (show)

See Also:


Attachments
Patch (3.04 KB, patch)
2015-09-04 16:52 PDT, Jon Honeycutt
dbates: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jon Honeycutt 2015-09-04 16:37:20 PDT
Need a test for bug #145539: Uploading an animated GIF from the photo library uploads a JPEG.
Comment 1 Jon Honeycutt 2015-09-04 16:52:20 PDT
Created attachment 260651 [details]
Patch
Comment 2 Daniel Bates 2015-09-04 17:35:34 PDT
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.
Comment 3 Jon Honeycutt 2015-09-04 17:58:45 PDT
Committed r189410: <http://trac.webkit.org/changeset/189410>
Comment 4 Jon Honeycutt 2015-09-04 17:59:46 PDT
(In reply to comment #2)
Thanks for the review! I made your suggested changes before landing.