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

Fix when using Data URIs for the manifest #550

Merged
merged 3 commits into from
Oct 17, 2016

Conversation

unkiwii
Copy link
Contributor

@unkiwii unkiwii commented Oct 13, 2016

When using Data URIs for the manifest an UNABLE_TO_GUESS_MANIFEST_TYPE is thrown. The mimeType is used to look up the manifesst parser but mimeType is undefined because shaka.net.DataUriPlugin does not return the correct 'content-type' in the header.

With this change shaka.media.ManifestParser (lib/media/manifest_parser.js, line167) can select the manifest parser correctly.

Data URIs used:

data:application/dash+xml,[encoded xml]

and

data:application/dash+xml;base64,[base64 encoded xml]

@joeyparrish joeyparrish self-assigned this Oct 15, 2016
@joeyparrish joeyparrish added this to the v2.1.0 milestone Oct 15, 2016
@joeyparrish
Copy link
Member

Looks good to me. Thanks for the patch!

Could you please update the tests in test/net/data_uri_plugin_unit.js to reflect this? There's already one that involves a MIME type, so the testSucceeds() helper just needs an update to check the headers. You can run tests locally with python build/test.py

@googlebot
Copy link

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.

@googlebot
Copy link

CLAs look good, thanks!

@unkiwii
Copy link
Contributor Author

unkiwii commented Oct 17, 2016

Updated the tests so it checks for the response header.

When no content type is present in the uri, an empty string is returned as the content-type, maybe it should return an text/plain as default. What do you think?

@joeyparrish
Copy link
Member

I think an empty string or null is appropriate. I wouldn't fill in defaults for content-type if the underlying transport doesn't have a MIME type concept. It might lead to confusion later.

@joeyparrish
Copy link
Member

Everything looks good to me. I'll run it through the build bot.

@shaka-bot
Copy link
Collaborator

Testing in progress...

@shaka-bot
Copy link
Collaborator

All tests passed!

@joeyparrish joeyparrish merged commit 3db71cf into shaka-project:v2.0.x Oct 17, 2016
@joeyparrish
Copy link
Member

Thank you for your contribution!

joeyparrish pushed a commit that referenced this pull request Oct 19, 2016
* Fix when using Data URIs for the manifest
* update test/net/data_uri_plugin_unit.js to test Data URIs content-type header response
joeyparrish pushed a commit that referenced this pull request Oct 19, 2016
* Fix when using Data URIs for the manifest
* update test/net/data_uri_plugin_unit.js to test Data URIs content-type header response
@github-actions github-actions bot added the status: archived Archived and locked; will not be updated label Jul 25, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
status: archived Archived and locked; will not be updated
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants