Closed Bug 882131 Opened 11 years ago Closed 11 years ago

[webvtt] Add a test for invalid timestamps

Categories

(Core :: Audio/Video, defect)

Product:
Core
Component:
Audio/Video
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED INVALID

People

(Reporter: reyre, Unassigned)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 4 obsolete files)

Test that WebVTT cues with bad times are dropped. V5
4.58 KB, patch
caitlin.potter
: review+
Details | Diff | Splinter Review
We just landed a fix that allows us to not add cues for non-fatal errors while parsing. Currently the only non-fatal cue we care about is invalid timestamps. We should probably have a mochitest for this so that we don't accidentally allow this to happen again.
Blocks: webvtt
Assignee: nobody → caitlin.potter
Where do we want these, content/media/test? dom/media/test?
Content/media/test would probably be the best since that is where WebVTTLoadListener is and the relevant code.
The commit message is a bit weird, I'm open to suggestions on improving that if need be. Otherwise it's all passing here.
Just some formatting fixups
Attachment #761517 - Attachment is obsolete: true
Attachment #761517 - Flags: review?(giles)
Attachment #761522 - Flags: review?(giles)
Depends on: 882535
Like V2, but now relies on HTMLTrackElement's readyState to determine if the track is loaded and useable or not. This test does not rely on flaky arbitrary timed timeouts, and has a way of knowing when to stop retrying, so it should work consistently, hopefully.
Attachment #761522 - Attachment is obsolete: true
Attachment #761522 - Flags: review?(giles)
Attachment #761847 - Flags: review?(giles)
Attachment #761847 - Flags: feedback?(rick.eyre)
Comment on attachment 761847 [details] [diff] [review] Test that WebVTT cues with bad times are dropped. V3 Review of attachment 761847 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me with nits. Great work. ::: content/media/test/Makefile.in @@ +383,5 @@ > test_can_play_type_no_wave.html \ > $(NULL) > endif > > +# Bug 882131 I think you can move these up to the main lists. The first MOCHITEST_FILES list for the .html and the #sample files for the .vtt file. Check with Ralph though if this is needed. ::: content/media/test/test_bug882131.html @@ +13,5 @@ > + /** Test for Bug 882131 **/ > + function test_bug882131() { > + var video = document.createElement("video"); > + video.src = "seek.webm"; > + White space here. @@ +53,5 @@ > + > + </script> > +</head> > +<body> > +<a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=882131">Mozilla Bug 882131</a> The style is usually to put this section, including the content div and pre, etc, above your script.
Attachment #761847 - Flags: feedback?(rick.eyre) → feedback+
This is the structure output from gen_template.pl, so I think I'm going to leave that. Will see what Ralph thinks about the makefile
Removed trailing whitespace from test_bug882131.html
Attachment #761847 - Attachment is obsolete: true
Attachment #761847 - Flags: review?(giles)
Attachment #762003 - Flags: review?(giles)
Attachment #762003 - Flags: feedback+
Depends on: 882817
https://tbpl.mozilla.org/?tree=Try&rev=9f09df497708 Changesets for tests don't show up in the try push as I pushed them earlier today, but you can see the tests were run correctly in the log file.
I don't actually see test_881232.html in the full logs
Nevermind, ignore that, it's 6am it's allowed.
Nevermind, ignore that, it's 6am it's allowed.
Comment on attachment 762003 [details] [diff] [review] Test that WebVTT cues with bad times are dropped. V4 Review of attachment 762003 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, thanks. I agree with Rick that you might as well add the html and vtt files to the respective combined MOCHITEST_FILE lists. It makes the file slightly shorter and easier to search. Not hugely important though, so r=me either way.
Attachment #762003 - Flags: review?(giles) → review+
I did a try for this the other day and it's suffering from the same android problem: https://tbpl.mozilla.org/?tree=Try&rev=5d6c005645fc
Goddamned Android (╯°□°）╯︵ ┻━┻ fails at everything.
Added files at the first chunk of MOCHITEST_FILES in content/media/test/Makefile.in
Attachment #762003 - Attachment is obsolete: true
Attachment #764358 - Flags: review+
Depends on: 884884
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → WONTFIX
Delete your patch if you don't want it to land, but this bug is still valid.
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
Assignee: caitlin.potter → nobody
The code for this has been removed. The parser that will be landed doesn't work with error messages at all. Closing as invalid.
Status: REOPENED → RESOLVED
Closed: 11 years ago → 11 years ago
Resolution: --- → INVALID
You're an idiot, Rick. This bug had nothing to do with reporting error messages, it is about dropping cues.
What I mean is that the new parser does not return cues that we may want to discard due to an error. The new parser just ignores anything that spec tells us to ignore. Therefore, we do not need to have this code anymore.
You're an idiot, Rick. "The new parser just ignores anything that the spec tells us to ignore. Therefore, we do not need tests to ensure that we are ignoring things" That is exactly the way the C parser worked -- The test is to verify that it is working like that. But the three of you are so mentally incompetent that it's not even worth looking at, I'm only bringing this up because I'm still getting emails about this ****. Honestly, pull your heads out of your asses. Tests are implemented for a reason, to ensure that you don't **** up.
And don't think I'm talking **** to you, I'm doing you a favour. The only excuse for not having tests here, would be if the tests are already implemented somewhere else, which is being monitored regularly. And honestly that's not even a very good excuse, especially considering that these tests predate the other **** entirely. But it doesn't matter whether it's these tests, or other ones, the bottom line is that if you don't have the tests, and you aren't running and monitoring them, you're a **** idiot and should remove yourself from computers and get a job as a busboy or something, because software is not your thing. And when your rationale for closing something as invalid is as **** as "well our parser doesn't report errors", you demonstrate a fundamental misunderstanding of what the **** the bug and tests are even about, and that you don't have any **** concept of behaviour driven development, which, considering the new implementation is javascript, IS **** UNACCEPTABLE, ENTIRELY. Like, you simply cannot **** write a javascript library and NOT do that. That is **** idiocy.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: