Closed Bug 879431 Opened 8 years ago Closed 7 years ago

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


(Core :: Audio/Video, defect)

Not set





(Reporter: reyre, Assigned: reyre)





(1 file, 3 obsolete files)

Blocks: 882700
No longer blocks: webvtt
Closed: 8 years ago
Resolution: --- → WONTFIX
Resolution: WONTFIX → ---
Assignee: nobody → justandy50
The bug is already fixed (GetTrack method is already added to the TextTrackCue class) and can be marked as resolved.
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,
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-
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
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+
(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
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
Closed: 8 years ago7 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.