882703 - [webvtt] Implement TextTrackList onchange event

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Rick Eyre (:reyre)]

We don't implement this in the webidl yet.

Comment 1

[Brendan Long]

Mind if I implement this?

Comment 2

[Ralph Giles (:rillian)]

Fine with me. Thanks for taking a look!

Comment 3

[Rick Eyre (:reyre)]

Thanks Brendan!

Comment 4

[Brendan Long]

Is there any standard way of dispatching simple events in Firefox? I tried looking at HTMLMediaElement for clues, but it has its own nsAsyncEventRunner. Is every class with events expected to do that, or is there a less-code-duplication way?

Comment 5

[Rick Eyre (:reyre)]

You can look at how we dispatch other Events in TextTrackList: https://github.com/mozilla/mozilla-central/blob/master/content/media/TextTrackList.cpp#L120.

The only difference for onchange is that it's not a TrackEvent it's a simple Event. So instead of instantiating a TrackEvent you would create an Event and then dispatch it the same way. You can probably just use the basic Runnable instead of the TrackEvent runner.

You'd also need to include the onchange event in the TextTrackList webidl: https://github.com/mozilla/mozilla-central/blob/master/dom/webidl/TextTrackList.webidl.

Comment 6

[Brendan Long]

Sorry this is taking so long. Most of this was pretty easy (I've done it before in WebKit and Blink), but I can't figure out how to instantiate a simple event. From what I can tell, the way to do it is with nsIDOMDocument::CreateEvent, like how nsContentUtils::GetEventAndTarget() does it, but TextTrackList doesn't have access to an nsIDOMDocument. The mGlobal member is instantiated like this in TextTrackManager:

    mTextTracks = new TextTrackList(mMediaElement->OwnerDoc()->GetParentObject());

I'm not even sure what a document's parent object is, but it's apparently not a document.

Am I missing something obvious here? I'll upload my current (crashing) patch so you can see the approach I'm taking.

Comment 7

[Brendan Long]

Attachment: texttracklist-change-event.diff

This is what I'm working with now. It compiles, but crashes here:

    nsCOMPtr<nsIDOMDocument> domDoc = do_QueryInterface(mGlobal);
    nsCOMPtr<nsIDOMEvent> changeEvent;
    nsresult rv = domDoc->CreateEvent(NS_LITERAL_STRING("change"),
                                      getter_AddRefs(changeEvent));

I'm pretty sure the problem is that mGlobal isn't a document, but I'm not sure what a reasonable way of getting a document here is.

Comment 8

[Rick Eyre (:reyre)]

I'm fuzzy around this area as well so I've set your flag for feedback to Boris. He'll be able to help you along.

Comment 9

[Boris Zbarsky [:bzbarsky]]

mGlobal is a window, not a document.

If we wanted to associate a TextTrackList with a document instead of a window, we could do that.  We'd need to grab the current document in the webidl constructor and pass in a document when we construct manually in the line cited in comment 6.

Olli, is there a way to create/dispatch an event if there is no document on hand?

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8342089 [details] [diff] [review]
texttracklist-change-event.diff

No matter what, we don't want to be using nsIDOMDocument; nsIDocument is the way to go.  nsIDOMDocument is deprecated.  ;)

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

NS_NewDOMEvent(getter_AddRefs(event), <some event target from the context the event will be dispatched>, nullptr, nullptr);

Comment 12

[Brendan Long]

Attachment: Patch adding change event

Using NS_NewDOMEvent I was able to make this work. It passes the test in WebKit[1] (with some minor changes, since it relies on window.event). I'm guessing this should be tested with a mochitest? Or maybe a reftest? How are 

[1] https://trac.webkit.org/browser/trunk/LayoutTests/media/track/track-change-event.html

Comment 13

[Brendan Long]

Attachment: Patch adding change event

I accidentally "hg add"ed the patch to the patch last time, here's a fixed version.

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

You want to ask review from someone. Empty review? isn't too useful.

And yes, mochitest please.

Comment 15

[Brendan Long]

Attachment: Patch to add TextTrackList change event

Comment 16

[Brendan Long]

Comment on attachment 8347747 [details] [diff] [review]
Patch to add TextTrackList change event

I think this is done. I'm not exactly sure how I should pick people for review/checkin, so I picked Rick since it was originally assigned to him.

Comment 17

[Rick Eyre (:reyre)]

Comment on attachment 8347747 [details] [diff] [review]
Patch to add TextTrackList change event

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

Looks good to me overall. Passing to Ralph for additional spec review and Boris for DOM review.

::: content/media/TextTrack.cpp
@@ +88,5 @@
>  
>  void
>  TextTrack::SetMode(TextTrackMode aValue)
>  {
> +  bool modeChanged = mMode != aValue;

How about:

if (mMode !== aValue) {
  mMode = aValue;
  mMediaElement->TextTracks()->CreateAndDispatchChangeEventRunner();
}

And get ride of the mode changed bit. This is the pattern we've been following in other areas too.

Comment 18

[Brendan Long]

Attachment: texttracklist_change_event.diff

I simplified the logic in SetMode() like you suggested, and also removed a superfluous include which I added before I understood how to create the event.

Comment 19

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8348091 [details] [diff] [review]
texttracklist_change_event.diff

r=me

Comment 20

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8348091 [details] [diff] [review]
texttracklist_change_event.diff

The checkin? on this bug is showing up in my "needs landing" queries. Please put the checkin-needed bug keyword on this bug when it has all the needed reviews. Thanks!

Comment 21

[Ralph Giles (:rillian)]

Comment on attachment 8348091 [details] [diff] [review]
texttracklist_change_event.diff

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

I'm not sure the test works. Please address comments and resubmit.

Please call 'test_texttracklist_change_event.html' just 'test_texttracklist.html'. It's shorter, and for performance reasons it's better to have more tests in a single file, so testing things like onaddtrack should go in the same file.

::: content/media/TextTrack.cpp
@@ +90,5 @@
>  TextTrack::SetMode(TextTrackMode aValue)
>  {
> +  if (mMode != aValue) {
> +    mMode = aValue;
> +    mMediaElement->TextTracks()->CreateAndDispatchChangeEventRunner();

What if mMediaElement->mTextTrackManager is null? Is there a reason that can't happen?

::: content/media/TextTrackList.h
@@ +56,5 @@
>    void RemoveTextTrack(TextTrack* aTrack);
>    void DidSeek();
>  
> +  nsresult DispatchTrackEvent(nsIDOMEvent* aEvent);
> +  void CreateAndDispatchChangeEventRunner();

CreateAndDispatchChangeEvent() to match other modules?

::: content/media/test/test_texttracklist_change_event.html
@@ +9,5 @@
> +  <script type="application/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
> +  <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css"/>
> +  <script type="application/javascript">
> +
> +  function changed(event) {

Please put the event handler at the end of the script, so reading order is the same as execution order.

@@ +16,5 @@
> +    ok(!event.bubbles, "change event should not bubble.");
> +    ok(event.isTrusted, "change event should be trusted.");
> +    ok(!event.cancelable, "change event should not be cancelable.");
> +
> +    SimpleTest.finish();

Does this test actually work? The code looks ok, but when I run it the handler never fires and it eventually fails with a timeout.

Comment 22

[Ralph Giles (:rillian)]

Comment on attachment 8348091 [details] [diff] [review]
texttracklist_change_event.diff

Oops, my bad. Forgot to rebuild when testing. Please still address the comments, but you can carry forward r=rillian.

Comment 23

[Brendan Long]

Attachment: Add TextTrackList change event

I addressed the issues Ralph found. Should I mark this as review?(people) again?

In response to Ralph's question about mMediaElement->textTracks():

mMediaElement can theoretically be null, since one of the constructors doesn't set it. HTMLMediaElement::textTracks() will never be null, since HTMLMediaElement's mTextTrackManager is allocated in the constructor.

I added a check for `if (mMediaElement)` to handle this correctly.

Comment 24

[Brendan Long]

Comment on attachment 8349183 [details] [diff] [review]
Add TextTrackList change event

I'm guessing you guys need to mark this review+ again?

Comment 25

[Rick Eyre (:reyre)]

You don't have to ask for review again if you just made changes to address review comments from Ralph. Both Ralph and Boris gave you an r+, Ralph's was conditional. Usually it's enough to make the changes and then carry forward the r+ on your updated patch. However, if you made changes that weren't discussed, added new functionality, or didn't address everything then you would need to ask for review again.

The next stage would be to push this to the try server to get tested. Do you have commit access for the Brandon? If not I can do it for you.

If it's green on the try server then we can mark your patch to get checked in.

Also for the future, if you need some information you can add a 'needinfo' flag to the bug and flag someone that way your comment gets addressed quickly.

One more thing though--can you change your commit message to be 'Bug 882703 - Add TextTrackList change event r=rillian,bz'. We use the form 'Bug XXX - Commit message r=[reviewers]' to track things in the commit logs.

Comment 26

[Rick Eyre (:reyre)]

Comment on attachment 8349183 [details] [diff] [review]
Add TextTrackList change event

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

Please update the commit message. See comment 25.

Comment 27

[Brendan Long]

(In reply to Rick Eyre (:reyre) from comment #25)
> Usually it's enough to make the changes and then carry forward
> the r+ on your updated patch.

How do I make the r+ carry forward to a new patch?

> The next stage would be to push this to the try server to get tested. Do you
> have commit access for the Brandon? If not I can do it for you.

No, I don't have access.

> One more thing though--can you change your commit message to be 'Bug 882703
> - Add TextTrackList change event r=rillian,bz'. We use the form 'Bug XXX -
> Commit message r=[reviewers]' to track things in the commit logs.

Ok I'll upload a fixed patch.

Comment 28

[Brendan Long]

Attachment: Add TextTrackList change event

I fixed the commit message. Can you run this on the try server?

Comment 29

[Rick Eyre (:reyre)]

(In reply to Brendan Long from comment #27)

> How do I make the r+ carry forward to a new patch?

Just upload the patch and in the comment say that you are carrying forward your reviewers. So for example you could say:

"Addressed all of Ralph's review comments. Carrying forward r=bz,rillian."

If you'd like, or you feel it necessary for whatever reason, you can add more comments about what you've addressed, etc.

> No, I don't have access.

Okay, I'll push it to the try server for you :).

Comment 30

[Rick Eyre (:reyre)]

Try: https://tbpl.mozilla.org/?tree=Try&rev=d75ef2b8ff0a

Comment 31

[Rick Eyre (:reyre)]

Looks green so I'll mark it for check-in for you Brendan. Thanks for this!

Comment 32

[Brendan Long]

(In reply to Rick Eyre (:reyre) from comment #31)
> Looks green so I'll mark it for check-in for you Brendan. Thanks for this!

Ok, thanks!

Comment 33

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9664a697fd18

Comment 34

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/9664a697fd18