Closed Bug 882535 Opened 7 years ago Closed 7 years ago

[webvtt] HTMLTrackElement.readyState is not being set

Categories

(Core :: Audio/Video, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla24

People

(Reporter: caitlin.potter, Assigned: caitlin.potter)

References

(Blocks 1 open bug, )

Details

Attachments

(1 file, 2 obsolete files)

While readyState is no longer being assigned to random values (bug871188), we need something like readyState to determine if a track is LOADING or not, for use by DOM tests (to avoid flakiness by waiting arbitrarily long).

The readyState attribute needs to be set during the processing of the TrackElement's source file (eg WebVTTLoadListener) so that this information is available.
Blocks: 833386
Sets readiness state to "LOADING" when WebVTTLoadListener::LoadResource is called, "ERROR" when a non-recoverable error occurs (in WebVTTLoadListener::OnReportError), and "LOADED" when WebVTTLoadListener::OnStopRequest is called, unless the state is already set to ERROR.

Perhaps more work must be done to make this more robust, but WFM atm.
Attachment #761844 - Flags: review?(giles)
Attachment #761844 - Flags: feedback?(rick.eyre)
Comment on attachment 761844 [details] [diff] [review]
Set HTMLTrackElement Readiness State during WebVTT Parsing.

Review of attachment 761844 [details] [diff] [review]:
-----------------------------------------------------------------

We'll want to set it to error as well if we get an error in WebVTTLoadListener::ParseChunk. If we get an error here we're going to abort the parsing for sure.

Nice work. Great solution for the tests.
Attachment #761844 - Flags: feedback?(rick.eyre) → feedback+
Comment on attachment 761844 [details] [diff] [review]
Set HTMLTrackElement Readiness State during WebVTT Parsing.

Review of attachment 761844 [details] [diff] [review]:
-----------------------------------------------------------------

R- for not understanding the logic. See below.

::: content/media/WebVTTLoadListener.cpp
@@ +89,5 @@
>                                    nsISupports* aContext,
>                                    nsresult aStatus)
>  {
>    webvtt_finish_parsing(mParser);
> +  if(mElement.get()->mReadyState != HTMLTrackElement::ERROR) {

Why do you clear the error when the load stops? It's not clear to me from the spec section you link to why this is correct.

@@ +231,5 @@
>  #endif
>  
>    switch(aError) {
> +    // Fatal errors require us to abort parsing, and set readyState
> +    // to ERROR

How about just "Fatal errors with require us to abort parsing:"

Parallels the comment on the next case, and it's not necessary to state that readyState is being set since that happens immediately below.
Attachment #761844 - Flags: review?(giles) → review-
> Why do you clear the error when the load stops? It's not clear to me from the spec section you link to why this is correct.

I'm not clearing the error here. You are misreading the code:

>  if(mElement.get()->mReadyState != HTMLTrackElement::ERROR) {
>    mElement.get()->mReadyState = HTMLTrackElement::LOADED;
>  }

This clearly shows that I'm explicitly AVOIDING clearing the error. "If mReadyState is NOT ERROR, THEN set it to LOADED"
Updated comment as suggested
Attachment #761844 - Attachment is obsolete: true
Attachment #762756 - Flags: review?(giles)
(In reply to Caitlin Potter (:caitp) from comment #4)

> This clearly shows that I'm explicitly AVOIDING clearing the error. "If
> mReadyState is NOT ERROR, THEN set it to LOADED"

Sorry, you're right. Thanks for clarifying.

Can you try it with mElement->mReadyState instead of mElement.get()->mReadyState? The class should take care of the dereference for you.
Removed calls to `get()` from smart pointer usage.
Attachment #762756 - Attachment is obsolete: true
Attachment #762756 - Flags: review?(giles)
Attachment #762791 - Flags: review?(giles)
Comment on attachment 762791 [details] [diff] [review]
Set HTMLTrackElement Readiness State during WebVTT Parsing. V3

Review of attachment 762791 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, thanks.
Attachment #762791 - Flags: review?(giles) → review+
Try looks pretty green
With the exception of the TestSTSParser stuff which is unrelated to this patch and has been happening for a while :>
https://hg.mozilla.org/mozilla-central/rev/de0018e03e39
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.