Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Blobs] Correctly set mimetype of blob in FetchDataLoader. #10101

Merged
merged 1 commit into from Mar 21, 2018

Conversation

Projects
None yet
5 participants
@chromium-wpt-export-bot
Copy link
Collaborator

chromium-wpt-export-bot commented Mar 19, 2018

Just creating a new BlobDataHandle doesn't actually update the mimetype
of the blob (if later the blob is requested through a blob: URL for
example), so instead actually create a new blob wrapping the existing
blob to properly change the mimetype.

Bug: none
Change-Id: I93b6d584178a02a74d68bdd6fcace1514ca90ec0
Reviewed-on: https://chromium-review.googlesource.com/967271
Reviewed-by: Hiroshige Hayashizaki hiroshige@chromium.org
Commit-Queue: Marijn Kruisselbrink mek@chromium.org
Cr-Commit-Position: refs/heads/master@{#544864}

@wpt-pr-bot
Copy link
Collaborator

wpt-pr-bot left a comment

Already reviewed downstream.

@w3c-bots

This comment has been minimized.

Copy link

w3c-bots commented Mar 19, 2018

Build PASSED

Started: 2018-03-21 22:31:41
Finished: 2018-03-21 22:36:27

View more information about this build on:

[Blobs] Correctly set mimetype of blob in FetchDataLoader.
Just creating a new BlobDataHandle doesn't actually update the mimetype
of the blob (if later the blob is requested through a blob: URL for
example), so instead actually create a new blob wrapping the existing
blob to properly change the mimetype.

Bug: none
Change-Id: I93b6d584178a02a74d68bdd6fcace1514ca90ec0
Reviewed-on: https://chromium-review.googlesource.com/967271
Reviewed-by: Hiroshige Hayashizaki <hiroshige@chromium.org>
Commit-Queue: Marijn Kruisselbrink <mek@chromium.org>
Cr-Commit-Position: refs/heads/master@{#544864}

@chromium-wpt-export-bot chromium-wpt-export-bot force-pushed the chromium-export-cl-967271 branch from 8a48e67 to 3a5695c Mar 21, 2018

@chromium-wpt-export-bot chromium-wpt-export-bot merged commit 3c0e03d into master Mar 21, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@chromium-wpt-export-bot chromium-wpt-export-bot deleted the chromium-export-cl-967271 branch Mar 21, 2018

@wanderview

This comment has been minimized.

Copy link
Contributor

wanderview commented Mar 22, 2018

@mkruisselbrink Are you sure that mimetype must include the boundary string? It seems there has been some disagreement between browsers on what to include here beyond multipart/form-data. For example, see whatwg/fetch#540. That's about charset, but its unclear to me if strictly requiring the boundary is correct here.

@mkruisselbrink

This comment has been minimized.

Copy link
Contributor

mkruisselbrink commented Mar 22, 2018

The test is merely verifying that the Content-Type header matches the type attribute of the blob. And that most definitely matches what the spec currently is saying (i.e. https://fetch.spec.whatwg.org/#scheme-fetch "Append Content-Type/blob’s type attribute value to response’s header list.").

I'm not sure where the boundary string bit comes from in these tests, but wherever it comes from that seems to have already been part of the test. But yeah, there definitely are open questions/inconsistencies around Blob.type in general. I'm not sure how that should be resolved (Blob.type is on my todo list, but I haven't gotten around to it yet unfortunately), but it seems very weird to me if the decision would be that Blob.type and the Content-Type header on fetching that blob wouldn't always be the same?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.