[webvtt] TextTrackCue's TextTrack property is not being set correctly

RESOLVED FIXED in mozilla31

Status

()

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

People

(Reporter: reyre, Assigned: reyre)

Tracking

(Blocks: 1 bug)

Trunk
mozilla31
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

4 years ago
TextTrackCue needs a GetTrack() method.

http://www.whatwg.org/specs/web-apps/current-work/multipage/the-video-element.html#texttrackcue
(Assignee)

Updated

4 years ago
Blocks: 629350
(Assignee)

Updated

4 years ago
Blocks: 882700
No longer blocks: 629350
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → WONTFIX
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
(Assignee)

Updated

4 years ago
Assignee: nobody → justandy50

Comment 1

4 years ago
The bug is already fixed (GetTrack method is already added to the TextTrackCue class) and can be marked as resolved.
(Assignee)

Comment 2

4 years ago
Okay cool. I don't know why I thought it wasn't at the time :S heh.

Can you please add some code to the test content/media/test/test_texttrackcue.html to verify that this is working. You'll probably want to check that GetTrack() works on Cues that were created by a TrackElement and also by Cues that were created and then added to a TextTrack. You'll also want to check that it returns null, or whatever the spec says, when a Cue is created, but not attached to a TextTrack.

You can read up here on Mochitests, which is what this test uses, https://developer.mozilla.org/en/docs/Mochitest.

Comment 3

4 years ago
Created attachment 819882 [details] [diff] [review]
v1:  test file content/media/test/test_texttrackcue.html was modified to check the GetTrack() function
Attachment #819882 - Flags: review?(giles)
Comment on attachment 819882 [details] [diff] [review]
v1:  test file content/media/test/test_texttrackcue.html was modified to check the GetTrack() function

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

Thanks for the patch! The new tests look good, but I had some comments on the C++ changes. Please address them and submit an updated patch for review.

Your commit message should also be more clear. The most important part of this patch is that you're setting mTrack from AddCue, so say that in the first line. Then leave a blank line before describing why the change was necessary and that tests were added. It's also helpful if you put 'r=rillian' at the end of the first line in expectation of it landing with my r+.

::: content/media/TextTrack.cpp
@@ -113,4 @@
>  {
>    TextTrackRegion* region = mRegionList->GetRegionById(aRegion.Id());
>    if (!region) {
> -    aRegion.SetTextTrack(this);

Why is this no longer needed? I don't see how calling SetTextTrack from AddCue means you don't need to call it from AddRegion.

::: content/media/TextTrackCue.h
@@ +60,4 @@
>      return mTrack;
>    }
>  
> +  void SetTextTrack(TextTrack* aTextTrack);

The getter is defined in the header; might as well put the setter here too. However please move it until after the "helper function for implementation" comment, so it's clear it's not the implementation for the IDL setter.

I would also call it 'SetTrack' since it's still the corresponding C++ setter for that member.

::: content/media/test/test_texttrackcue.html
@@ +72,4 @@
>  
>        // Check that we can create and add new VTTCues
>        var vttCue = new VTTCue(3.999, 4, "foo");
> +      is(vttCue.track, undefined, "Cue's track should be undefined.");

Since you added a C++ level setter, please also verify it can't be called from javascript. That is, assigning to vttCue.track should have no effect.
Attachment #819882 - Flags: review?(giles) → review-
(Assignee)

Comment 5

3 years ago
I'm going to take over finish this patch up since Andriy doesn't have time to finish it off right now and we're going to need this implemented for when the pref gets turned on, which means landing it in time for FF31.
Assignee: justandy50 → rick.eyre
(Assignee)

Comment 6

3 years ago
Created attachment 8400745 [details] [diff] [review]
v2: Set VTTCue's TextTrack from TextTrack::AddCue r=rillian

I've kept the author as Andriy. I've only rebased and fixed the issues that Ralph pointed out.
Attachment #819882 - Attachment is obsolete: true
Attachment #8400745 - Flags: review?(giles)
Comment on attachment 8400745 [details] [diff] [review]
v2: Set VTTCue's TextTrack from TextTrack::AddCue r=rillian

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

r=me with nit.

::: content/media/TextTrackCue.cpp
@@ +7,5 @@
>  #include "mozilla/dom/TextTrackCue.h"
>  #include "mozilla/dom/TextTrackRegion.h"
>  #include "nsComponentManagerUtils.h"
>  #include "mozilla/ClearOnShutdown.h"
> +#include "mozilla/dom/TextTrack.h"

Why is this needed if it wasn't before?
Attachment #8400745 - Flags: review?(giles) → review+
(Assignee)

Comment 8

3 years ago
Created attachment 8400812 [details] [diff] [review]
v2: Set VTTCue's TextTrack from TextTrack::AddCue r=rillian

(In reply to Ralph Giles (:rillian) from comment #7)

> Why is this needed if it wasn't before?

Woops, I missed taking that out when moving the SetTrack function from the .cpp file to the .h file.

Carrying forward r=rillian.
Attachment #8400745 - Attachment is obsolete: true
(Assignee)

Comment 9

3 years ago
Created attachment 8401026 [details] [diff] [review]
v3: Set VTTCue's TextTrack from TextTrack::AddCue r=rillian

Had to include TextTrack.h, but in the TextTrackCue header, not cpp, which Ralph questioned earlier. We need it in the TextTrack header because we're assigning into a TextTrack smart pointer. If we don't add it complains about access into an incomplete type.

Carrying forward r=rillian.
Attachment #8400812 - Attachment is obsolete: true
(Assignee)

Comment 10

3 years ago
Try looks good: https://tbpl.mozilla.org/?tree=Try&rev=b0ebc90da4d8
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/8b59bee4fa90
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/8b59bee4fa90
Status: REOPENED → RESOLVED
Last Resolved: 4 years ago3 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in before you can comment on or make changes to this bug.