Fix Blob type-string

[izgzhen]

Use a final construction guard over type-string format; and other minor related cleanups

r? @Manishearth

---

• ./mach build -d does not report any errors
• ./mach test-tidy does not report any errors

• There are tests for these changes OR
• These changes do not require tests because _____

---

This change is

[highfive]

Heads up! This PR modifies the following files:

• @KiChjang: components/script/dom/file.rs, components/script/dom/blob.rs

[highfive]

Warning

• These commits modify script code, but no tests are modified. Please consider adding a test!

[Manishearth]

Can we start including tests with such PRs, now that it's testable (fine if you put the test in a later PR)

@bors-servo r+

[bors-servo]

📌 Commit f24ddc0 has been approved by Manishearth

[bors-servo]

⌛ Testing commit f24ddc0 with merge a866ab8...

[izgzhen]

👌 Sure, I think I can activate Blob URL as soon as the #12406 is landed

[bors-servo]

💔 Test failed - mac-rel-wpt

[izgzhen]

Wow ... unexpected but nice to see the failures

[izgzhen]

From spec

type , of type DOMString, readonly
The ASCII-encoded string in lower case representing the media type of the Blob. On getting, user agents must return the type of a Blob as an ASCII-encoded string in lower case, such that when it is converted to a byte sequence, it is a parsable MIME type, or the empty string – 0 bytes – if the type cannot be determined.

Note: The type t of a Blob is considered a parsable MIME type if the ASCII-encoded string representing the Blob object’s type, when converted to a byte sequence, does not return undefined for the parse a MIME type algorithm.

Thus I doubt if the failed test cases are reasonable at all

[izgzhen]

@Manishearth Comment on the failed tests?

[Manishearth]

This is sort of contradictory.

On one hand, when constructing the blob, the type has these restrictions:

If t contains any characters outside the range U+0020 to U+007E, then set t to the empty string and return from these substeps.

When getting the type, the condition that it is parseable is further introduced.

Firefox does not seem to impose either of these restrictions on it, a content type of text\x01/plain is also acceptable.

@sicking @annevk thoughts? This looks like a spec bug. The spec seems to assert that the following code:

a = new Blob([1,2,3], {type: "text/(plain"});
console.log(a.type) // prints text/(plain

shouldn't work. It does in Firefox, and the WPT tests /FileAPI/blob/Blob-constructor.html / /FileAPI/blob/Blob-slice.html rely on it

[bors-servo]

☔ The latest upstream changes (presumably #12406) made this pull request unmergeable. Please resolve the merge conflicts.

[Manishearth]

Filed w3c/FileAPI#43

Feel free to mark those tests as expected failures and file a tracking issue.

(Or do the reverse, make the tests work, and file an issue)

[izgzhen]

@Manishearth okay, thanks!

[Manishearth]

Since most browsers follow this behavior, it may make more sense to make the tests work.

[izgzhen]

@bors-servo try

[bors-servo]

⌛ Trying commit 8985f37 with merge 06c25a0...

[bors-servo]

💔 Test failed - arm64

[izgzhen]

@bors-servo retry

Looks like git problem

[bors-servo]

⌛ Trying commit 8985f37 with merge 11478f0...

[bors-servo]

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt, windows

[sicking]

Since I was pinged...

I think that in theory it would be nice to enforce that the .type is a "real" mimetype. However if browsers consistently don't do that, then it's probably not worth trying to add such a restriction.

Either way, filing a bug against the spec is likely the right thing to do.

[Manishearth]

@sicking thanks. Bug was filed at w3c/FileAPI#43. What are your thoughts on the issue of the character range in the constructor? Chrome and the spec agree, Firefox doesn't.

[sicking]

If spec and Chrome agree, then we should change firefox to match both of those. But based on comments in that bug, Chrome doesn't seem to match the spec exactly either. But lets continue diescussion over in the other bug.

[Manishearth]

@bors-servo r+

r+ for now, we can update when we decide on the spec thing.

[bors-servo]

📌 Commit 8985f37 has been approved by Manishearth

[bors-servo]

⌛ Testing commit 8985f37 with merge fa432a5...

[bors-servo]

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt, windows