Closed Bug 462570 Opened 11 years ago Closed 11 years ago

Network state and Ready state names in HTMLMediaElement are not as defined in spec

Categories

(Core :: Audio/Video, defect)

defect
Not set

Tracking

()

VERIFIED FIXED

People

(Reporter: cajbir, Assigned: cpearce)

References

()

Details

(Keywords: verified1.9.1)

Attachments

(1 file, 4 obsolete files)

Section 4.8.10 of the spec outlines the ready state and network states in HTMLMediaElement. These have recently changed and the current implementation we have in nsIDOMHTMLMediaElement is now incorrect.
Attached patch Patch v1 (obsolete) — Splinter Review
Renames network state and ready state. I updated unit tests to pass. I recreated content/media/video/test/test_constants.html based on http://simon.html5.org/test/html/dom/interfaces/HTMLElement/HTMLMediaElement/const-unsigned-short/001.htm and I kept the HTMLMediaElement.prototype.NETWORK_* tests, but left them as todo, since they don't pass. I think HTMLMediaElement.prototype.NETWORK_* should work, but currently it doesn't.
Assignee: nobody → chris
Attachment #346559 - Flags: superreview?(roc)
Attachment #346559 - Flags: review?(chris.double)
Comment on attachment 346559 [details] [diff] [review]
Patch v1

nice
Attachment #346559 - Flags: superreview?(roc) → superreview+
Comment on attachment 346559 [details] [diff] [review]
Patch v1

Patch is bitrotten.
Attachment #346559 - Attachment is obsolete: true
Attachment #346559 - Flags: review?(chris.double)
Attached patch Patch v2 (obsolete) — Splinter Review
Updated to account for bitrot and changes in the spec. I removed some events which are no longer in the spec as well, and added the loadeddata event.
Attachment #352660 - Flags: superreview?(roc)
Attachment #352660 - Flags: review?(chris.double)
Attachment #352660 - Flags: review?(chris.double) → review+
Comment on attachment 352660 [details] [diff] [review]
Patch v2

> +GK_ATOM(loadedfirstframe, "loadeddata")

Copy/Paste error? r+ with that fixed
As Patch v2 with review comment addressed. r+ chris.double.
Attachment #352660 - Attachment is obsolete: true
Attachment #352663 - Flags: superreview?(roc)
Attachment #352663 - Flags: review+
Attachment #352660 - Flags: superreview?(roc)
Attachment #352663 - Flags: superreview?(roc) → superreview+
Keywords: checkin-needed
Whiteboard: [needs landing]
Keywords: checkin-needed
Whiteboard: [needs landing]
Attached patch Patch v2.2 - unbitrotten (obsolete) — Splinter Review
As v2.1 but unbitrotten. r+ chris.double sr+ roc.
Attachment #352663 - Attachment is obsolete: true
Attachment #352954 - Flags: superreview+
Attachment #352954 - Flags: review+
Keywords: checkin-needed
Whiteboard: [needs landing]
Now with even less bitrot!
Attachment #352954 - Attachment is obsolete: true
Attachment #352965 - Flags: superreview+
Attachment #352965 - Flags: review+
Pushed to mozilla-central:
http://hg.mozilla.org/mozilla-central/rev/3a33323d7270
Status: NEW → RESOLVED
Closed: 11 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [needs landing] → [needs landing][baking for 1.9.1]
Keywords: checkin-needed
When this lands on 1.9.1, the mochitest for bug 461680 needs to be changed to listen for 'loadeddata' event, rather than 'loadedmetadata'.
Comment on attachment 352965 [details] [diff] [review]
Patch v2.3 unbitrotten++
[Checkin: Comment 9]

(In reply to comment #10)
> When this lands on 1.9.1, the mochitest for bug 461680 needs to be changed to

This patch does not apply (cleanly) to 1.9.1 branch.

> listen for 'loadeddata' event, rather than 'loadedmetadata'.

This has not been done (yet) on trunk...
Attachment #352965 - Attachment description: Patch v2.3 unbitrotten++ → Patch v2.3 unbitrotten++ [Checkin: Comment 9]
Flags: in-testsuite+
Keywords: checkin-needed
Comment on attachment 352965 [details] [diff] [review]
Patch v2.3 unbitrotten++
[Checkin: Comment 9]

(In reply to comment #11)
> > listen for 'loadeddata' event, rather than 'loadedmetadata'.
> 
> This has not been done (yet) on trunk...

My bad: it has :->
Could you include this change in the 1.9.1 patch ?
(In reply to comment #10)
> When this lands on 1.9.1, the mochitest for bug 461680 needs to be changed to
> listen for 'loadeddata' event, rather than 'loadedmetadata'.

It should be fine without it, it can be changed separately or just ignored.
Depends on: 467972
*sigh*... This requires bug 467972 to land on 1.9.1 before this can land on 1.9.1. I'll unbitrot the patch once bug 467972 has landed...
We need to land the patch for bug 469598 to land on 1.9.1 before the patch for this bug can land on 1.9.1.
Depends on: 469598
We need to land the patch for bug 466432 to land on 1.9.1 before the patch for
this bug can land on 1.9.1.
Depends on: 466432
Pushed bc83afa92f67 to 1.9.1
Keywords: fixed1.9.1
Whiteboard: [needs landing][baking for 1.9.1]
Seeing as there hasn't been any discussions about this bug for 3 months and it's been in mochitest, I'm assuming there aren't any residual issues. I'm moving this to verified as a result. If anyone has any qualms, feel free to bring them up.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.