Summary: | charset in contentType used in Blob.prototype.slice(start, end, contentType) is lost | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Rick Waldron <rwaldron> | ||||||||||||
Component: | DOM | Assignee: | Alex Christensen <achristensen> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | achristensen, ap, cdumez, eoconnor, rwaldron, webkit-bug-importer | ||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||
Version: | Safari 14 | ||||||||||||||
Hardware: | Mac (Intel) | ||||||||||||||
OS: | macOS 10.15 | ||||||||||||||
Attachments: |
|
Description
Rick Waldron
2021-04-26 09:28:55 PDT
Created attachment 427092 [details]
Patch
Comment on attachment 427092 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=427092&action=review > LayoutTests/fast/files/blob-content-type-expected.txt:1 > +ALERT: content-type text/html2;charset=utf-8 Please write a test with actual PASS / FAIL lines. This output really does not make it obvious if the test is failing or passing. If I were to make a change breaking this, I could easily rebaseline this test by mistake and still think it is fine. Created attachment 427093 [details]
Patch
Comment on attachment 427093 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=427093&action=review > LayoutTests/fast/files/blob-content-type.html:14 > + alert(response.headers.get('content-type') == "" ? "PASS" : "FAIL") This fails in Chrome. response.headers.get('content-type') returns null. Who is correct? Comment on attachment 427093 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=427093&action=review >> LayoutTests/fast/files/blob-content-type.html:14 >> + alert(response.headers.get('content-type') == "" ? "PASS" : "FAIL") > > This fails in Chrome. response.headers.get('content-type') returns null. Who is correct? And Firefox gives "text/html" lol :) Seems browsers really don't agree on this. Comment on attachment 427093 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=427093&action=review > LayoutTests/fast/files/blob-content-type-expected.txt:2 > +ALERT: PASS > +ALERT: PASS Would you mind making the test more conventional in having some output that explains what is expected, what bug was fixed etc? There isn't always the time to do a blame to see what a test was added for. Can this use the usual harness to avoid the need for manual testRunner.dumpAsText, and to have the "test completed" line? Oh, no. I'm glad I added that test. In the simple case where a content type is provided, all browsers agree that that should be used. Where a content type is not provided, since browsers don't agree maybe we should continue our existing behavior and inherit from the sliced-from blob. I think there is always enough time to git blame, but I can indeed make the test nicer. Created attachment 427103 [details]
Patch
Comment on attachment 427103 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=427103&action=review > LayoutTests/fast/files/blob-content-type-expected.txt:3 > +TEST COMPLETE Well this is wrong! Comment on attachment 427103 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=427103&action=review >> LayoutTests/fast/files/blob-content-type-expected.txt:3 >> +TEST COMPLETE > > Well this is wrong! Indeed. > LayoutTests/fast/files/blob-content-type.html:1 > +<script src="../../resources/js-test-pre.js"></script> You can just use js-test.js > LayoutTests/fast/files/blob-content-type.html:3 > +async function runTest() { This test is missing a description("What the test does") call and `jsTestIsAsync = true;` > LayoutTests/fast/files/blob-content-type.html:26 > +<script src="../../resources/js-test-post.js"></script> Not needed. Comment on attachment 427103 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=427103&action=review > LayoutTests/fast/files/blob-content-type.html:10 > + shouldBe("response.headers.get('content-type')", "'text/html2;charset=utf-8'") You can use shouldBeEqualToString() for this. > LayoutTests/fast/files/blob-content-type.html:15 > + shouldBe("response.headers.get('content-type')", "'text/original-blob-content-type;'") this > LayoutTests/fast/files/blob-content-type.html:20 > + shouldBe("response.headers.get('content-type')", "'text/original-blob-content-type;'") and this. Comment on attachment 427103 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=427103&action=review > LayoutTests/fast/files/blob-content-type.html:22 > + asyncTestPassed(); finishJSTest(); Created attachment 427108 [details]
Patch
Comment on attachment 427108 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=427108&action=review > LayoutTests/fast/files/blob-content-type.html:5 > + const blob = new Blob([`<style></style>`], { type: 'text/original-blob-content-type;' }); Why the weird ` quotes? > LayoutTests/fast/files/blob-content-type.html:8 > + const slice = blob.slice(0, blob.size, `text/html2;charset=utf-8`); ditto. > LayoutTests/fast/files/blob-content-type.html:22 > + const slice = blob.slice(0, blob.size, ""); and then here double quotes, even though the rest of the file seems to be using single quotes. (In reply to Chris Dumez from comment #3) > Comment on attachment 427092 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=427092&action=review > > > LayoutTests/fast/files/blob-content-type-expected.txt:1 > > +ALERT: content-type text/html2;charset=utf-8 > > Please write a test with actual PASS / FAIL lines. This output really does > not make it obvious if the test is failing or passing. If I were to make a > change breaking this, I could easily rebaseline this test by mistake and > still think it is fine. I've updated the jsfiddle, but here is the test code: const blob = new Blob([`<style></style>`], { type: 'text/html' }); const normalizedBlob = blob.slice(0, blob.size, `text/html;charset=utf-8`); const url = URL.createObjectURL(normalizedBlob); const xhr = new XMLHttpRequest(); xhr.addEventListener('load', () => { console.log(xhr.getResponseHeader('content-type') === 'text/html;charset=utf-8' ? 'PASS' : 'FAIL'); }); xhr.open('GET', url); xhr.send(); Chrome and Firefox: PASS Safari: FAIL (In reply to Rick Waldron from comment #16) > (In reply to Chris Dumez from comment #3) > > Comment on attachment 427092 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=427092&action=review > > > > > LayoutTests/fast/files/blob-content-type-expected.txt:1 > > > +ALERT: content-type text/html2;charset=utf-8 > > > > Please write a test with actual PASS / FAIL lines. This output really does > > not make it obvious if the test is failing or passing. If I were to make a > > change breaking this, I could easily rebaseline this test by mistake and > > still think it is fine. > > I've updated the jsfiddle, but here is the test code: > > > const blob = new Blob([`<style></style>`], { type: 'text/html' }); > const normalizedBlob = blob.slice(0, blob.size, > `text/html;charset=utf-8`); > const url = URL.createObjectURL(normalizedBlob); > const xhr = new XMLHttpRequest(); > xhr.addEventListener('load', () => { > console.log(xhr.getResponseHeader('content-type') === > 'text/html;charset=utf-8' ? 'PASS' : 'FAIL'); > }); > xhr.open('GET', url); > xhr.send(); > > > Chrome and Firefox: PASS > Safari: FAIL Thanks Rick. I was actually talking to Alex who wrote the patch. Our code changes always need to be committed with a corresponding test. Alex did include a test in his latest iteration and the fix should get committed soon. Created attachment 427160 [details]
Patch
Committed r276677 (237094@main): <https://commits.webkit.org/237094@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 427160 [details]. |