Closed Bug 917945 Opened 11 years ago Closed 11 years ago

[webvtt] Enable regions to be handled by Gecko correctly upon parsing

Categories

(Core :: Audio/Video, defect)

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

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla27

People

(Reporter: reyre, Assigned: reyre)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 5 obsolete files)

Part 1 v2: Expose TextTrackRegion* as VTTRegion*
(deleted), patch
Details | Diff | Splinter Review
Part 2 v1: Add RegionId attribute to VTTCue
(deleted), patch
Details | Diff | Splinter Review
Part 3 v2: Add VTTRegions to its TextTrack upon receiving them
(deleted), patch
Details | Diff | Splinter Review
We need to update the WebVTTListener class and rename TextTrackRegion* to VTTRegion* as this is how content JS is supposed to see it.
Assignee: nobody → rick.eyre
Blocks: webvtt
Depends on: 895091
Attachment #806778 - Flags: review?(giles)
Attachment #806778 - Flags: review?(bzbarsky)
Attached patch Part 2 v2: Add RegionId attribute to VTTCue (obsolete) (deleted) — — Splinter Review
Attachment #806779 - Flags: review?(giles)
Attachment #806780 - Flags: review?(giles)
Attachment #806780 - Flags: review?(bzbarsky)
Depends on bug 895091 which *hasn't* landed yet, but is checked-in so hopefully soon.
Comment on attachment 806778 [details] [diff] [review] Part 1 v1: Expose TextTrackRegion* as VTTRegion* r=me
Attachment #806778 - Flags: review?(bzbarsky) → review+
Comment on attachment 806780 [details] [diff] [review] Part 3 v1: Add VTTRegions to its TextTrack upon receiving them >+ nsresult rv = UNWRAP_OBJECT(VTTRegion, aCx, &aRegion.toObject(), What if it fails? r=me with that fixed.
Attachment #806780 - Flags: review?(bzbarsky) → review+
Thanks for review Boris. Carrying forward r=bz.
Attachment #806780 - Attachment is obsolete: true
Attachment #806780 - Flags: review?(giles)
Attachment #806911 - Flags: review?(giles)
Comment on attachment 806911 [details] [diff] [review] Part 3 v2: Add VTTRegions to its TextTrack upon receiving them Review of attachment 806911 [details] [diff] [review]: ----------------------------------------------------------------- r=me with nit. ::: content/media/test/test_texttrackregion.html @@ +36,5 @@ > + } > + is(trackElement.readyState, 2, "Track::ReadyState should be set to LOADED."); > + > + var cues = trackElement.track.cues, > + regions = trackElement.track.regions; no 'var'?
Attachment #806911 - Flags: review?(giles) → review+
Attachment #806779 - Flags: review?(giles) → review+
Comment on attachment 806778 [details] [diff] [review] Part 1 v1: Expose TextTrackRegion* as VTTRegion* Review of attachment 806778 [details] [diff] [review]: ----------------------------------------------------------------- VTTRegion.webidl has some DOS line endings in my checkout. You might want to fix those while you're moving the file.
Attachment #806778 - Flags: review?(giles) → review+
(In reply to Ralph Giles (:rillian) from comment #8) > Comment on attachment 806911 [details] [diff] [review] > Part 3 v2: Add VTTRegions to its TextTrack upon receiving them > > Review of attachment 806911 [details] [diff] [review]: > ----------------------------------------------------------------- > > r=me with nit. > > ::: content/media/test/test_texttrackregion.html > @@ +36,5 @@ > > + } > > + is(trackElement.readyState, 2, "Track::ReadyState should be set to LOADED."); > > + > > + var cues = trackElement.track.cues, > > + regions = trackElement.track.regions; > > no 'var'? Multiline declaration. I can change it to have 'var' per line though if you'd like?
Oh, sorry. I missed the comment. I find var-per-line easier to read (obviously) but don't feel strongly. Code is correct.
I missed the *comma*.
Thanks for review Ralph. Replaced the line endings with Unix line endings. Carrying forward r=rillian,bz
Attachment #806778 - Attachment is obsolete: true
Had to generate the patch in HG in order to get the line endings to apply cleanly from a patch file. Carrying forward r=rillian,bz
Attachment #807731 - Attachment is obsolete: true
Just adding "Part 2" to commit message. Carrying forward r=rillian.
Attachment #806779 - Attachment is obsolete: true
Just adding "Part 3" to the commit message. Carrying forward r=rillian,bz
Attachment #806911 - Attachment is obsolete: true
Try looks green.
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: