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

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Rick Eyre (:reyre)]

TextTrackCue needs a GetTrack() method.

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

Comment 1

[Andriy (:Andriy)]

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

Comment 2

[Rick Eyre (:reyre)]

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

[Andriy (:Andriy)]

Attachment: v1: test file content/media/test/test_texttrackcue.html was modified to check the GetTrack() function

Comment 4

[Ralph Giles (:rillian)]

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.

Comment 5

[Rick Eyre (:reyre)]

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.

Comment 6

[Rick Eyre (:reyre)]

Attachment: 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.

Comment 7

[Ralph Giles (:rillian)]

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?

Comment 8

[Rick Eyre (:reyre)]

Attachment: 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.

Comment 9

[Rick Eyre (:reyre)]

Attachment: 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.

Comment 10

[Rick Eyre (:reyre)]

Try looks good: https://tbpl.mozilla.org/?tree=Try&rev=b0ebc90da4d8

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8b59bee4fa90

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8b59bee4fa90