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.

Sign up for GitHub

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

Jump to bottom

fix case where TTML data is an empty string #1960

Merged
merged 4 commits into from
Sep 11, 2019
Merged

fix case where TTML data is an empty string #1960

merged 4 commits into from
Sep 11, 2019

Conversation

fadomire
Copy link
Contributor

when using DOMParser parseFromString with an empty string it returns an errored XML document
the next part of the code is then executed and throw an unnecessary error

TheModMaker
TheModMaker suggested changes May 23, 2019
View reviewed changes
Copy link
Contributor

@TheModMaker TheModMaker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please run ./build/all.py to check for style issues and add a unit test in test/text/ttml_text_parser_unit.js.

Thanks.

lib/text/ttml_text_parser.js Outdated
@@ -47,6 +47,10 @@ shaka.text.TtmlTextParser = class {
const parser = new DOMParser();
let xml = null;

// dont try to parse empty string as
// DOMParser will not throw error but return an errored xml
if (str == '') return ret
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs curly brackets and needs to be on the next line.

@fadomire
Copy link
Contributor Author

ok will do

@fadomire
Copy link
Contributor Author

fadomire commented Sep 6, 2019

@TheModMaker hello, i finally took the time to write the unit test and fix the code style
Let me know if you want me to squash commits or do other changes

Thanks

TheModMaker
TheModMaker approved these changes Sep 6, 2019
View reviewed changes
Copy link
Contributor

@TheModMaker TheModMaker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Let me run it by the build bot.

@shaka-bot
Copy link
Collaborator

Test Failure:

Chrome 76.0.3809 (Linux 0.0.0)
  TtmlTextParser rejects invalid ttml FAILED
    Expected function to throw an exception.
        at errorHelper (test/text/ttml_text_parser_unit.js:871:8 <- test/text/ttml_text_parser_unit.js:441:8)
        at UserContext.<anonymous> (test/text/ttml_text_parser_unit.js:77:5 <- test/text/ttml_text_parser_unit.js:51:5)

  Transmuxer transmuxing transmux empty video from TS to MP4 FAILED
    Expected 0 to be greater than 0.
        at _callee5$ (test/media/transmuxer_integration.js:127:42 <- test/media/transmuxer_integration.js:186:50)
        at tryCatch (node_modules/babel-polyfill/dist/polyfill.js:6900:40)
        at Generator.invoke [as _invoke] (node_modules/babel-polyfill/dist/polyfill.js:7138:22)
        at Generator.prototype.<computed> [as next] (node_modules/babel-polyfill/dist/polyfill.js:6952:21)

Failed 2 tests, passed 1642, skipped 7

@TheModMaker
Copy link
Contributor

It looks like there is an explicit test to reject the empty string. You'll need to edit that. The other failure should be fixed with a rebase.

@fadomire
Copy link
Contributor Author

fadomire commented Sep 6, 2019

ok thanks, i should have seen that another test was checking empty string and expected a rebase would be needed :)
thanks for your time, i’ll fix that next week

@fadomire
fix case where TTML data is an empty string
bbc3cac 
when using DOMParser parseFromString with an empty string it returns an errored XML document
the next part of the code is then executed and throw an unnecessary error
@fadomire
coding style fix
90fd3f9
@fadomire
add unit test
19aa8e4
@fadomire
remove test : empty string return a valid xml
6f5058b
@fadomire
Copy link
Contributor Author

@TheModMaker i rebased and removed part of unit test that was conflicting

joeyparrish
joeyparrish approved these changes Sep 11, 2019
View reviewed changes
Copy link
Member

@joeyparrish joeyparrish left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. I'll have the build bot check it once more.

@shaka-bot
Copy link
Collaborator

All tests passed!

@joeyparrish joeyparrish merged commit 2c0d824 into shaka-project:master Sep 11, 2019
TheModMaker pushed a commit that referenced this pull request Nov 6, 2019
@fadomire @TheModMaker
Fix support for empty TTML data (#1960)
512f5a6 
When using DOMParser parseFromString with an empty string, it returns an errored XML document.  The next part of the code is then executed and throws an unnecessary error.

This adds a special case for an empty string.

Change-Id: Icb25d0d50f59b77f3bac0785e785163521276fe1
@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.
Reviewers

@joeyparrish joeyparrish joeyparrish approved these changes

@TheModMaker TheModMaker TheModMaker approved these changes

Assignees
No one assigned
Labels
status: archived Archived and locked; will not be updated
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@fadomire @shaka-bot @TheModMaker @joeyparrish