[webvtt] HTMLTrackElement.readyState is not being set

RESOLVED FIXED in mozilla24

Status

()

Core
Audio/Video
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: caitp, Assigned: caitp)

Tracking

(Blocks: 1 bug)

Trunk
mozilla24
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

4 years ago
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.

Updated

4 years ago
Blocks: 833386
(Assignee)

Comment 1

4 years ago
Created attachment 761844 [details] [diff] [review]
Set HTMLTrackElement Readiness State during WebVTT Parsing.

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 2

4 years ago
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-
(Assignee)

Comment 4

4 years ago
> 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"
(Assignee)

Comment 5

4 years ago
Created attachment 762756 [details] [diff] [review]
Set HTMLTrackElement Readiness State during WebVTT Parsing. V2

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.
(Assignee)

Comment 7

4 years ago
Created attachment 762791 [details] [diff] [review]
Set HTMLTrackElement Readiness State during WebVTT Parsing. V3

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+
https://tbpl.mozilla.org/?tree=Try&rev=03efefa4f74b
(Assignee)

Comment 10

4 years ago
Try looks pretty green
(Assignee)

Comment 11

4 years ago
With the exception of the TestSTSParser stuff which is unrelated to this patch and has been happening for a while :>
https://hg.mozilla.org/integration/mozilla-inbound/rev/de0018e03e39
https://hg.mozilla.org/mozilla-central/rev/de0018e03e39
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.