Closed Bug 893309 Opened 11 years ago Closed 11 years ago

[webvtt] Change events dispatched by TextTrack to be TrackEvents

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: drexler)

References

(
URL
)

Details

(Keywords: dev-doc-needed)

Attachments

(2 files, 2 obsolete files)

Impl v3
(deleted), patch
cpearce
: review+
Details | Diff | Splinter Review
Part B: Expose TrackEvent as a property on the window
(deleted), patch
smaug
: review+
Details | Diff | Splinter Review
We've implemented generic trusted events on the TextTrack. We need to instead create a TextTrackEvent object and dispatch those.
With this dependent on AudioTrack and VideoTrack implementations, will a partial implementation supporting just TextTrack suffice in the interim, Rick? That is: [Constructor(DOMString type, optional TrackEventInit eventInitDict)] interface TrackEvent : Event { readonly attribute TextTrack track; }; dictionary TrackEventInit : EventInit { TextTrack track; }; This is just to help close out work on the TextTrack a bit faster. Thereafter, we can update the webidl to handle AudioTrack & VideoTrack when those are completed. Any thoughts?
Depends on: 744896
I think that might be best for now. Chris, do you have any thoughts on this as the MediaElement will need to interface with this in the future?
Flags: needinfo?(cpearce)
AudioTrack and VideoTrack aren't implemented yet, and we haven't discussed implementing them, or the MediaController API that uses them (I think only Safari is interested in it? I'm not sure...) so just implementing the TrackEvent for a TextTrack is fine for now.
Flags: needinfo?(cpearce)
Assignee: nobody → andrew.quartey
Attached patch TrackEvent.patch (obsolete) (deleted) — — Splinter Review
According to the spec:" The track attribute must return the value it was initialized to. When the object is created, this attribute must be initialized to null. It represents the context information for the event". Since the track attribute be null, i modified idl to: [Constructor(DOMString type, optional TrackEventInit eventInitDict)] interface TrackEvent : Event { readonly attribute TextTrack? track; }; dictionary TrackEventInit : EventInit { TextTrack? track; };
Attachment #806439 - Flags: review?(cpearce)
Comment on attachment 806439 [details] [diff] [review] TrackEvent.patch Review of attachment 806439 [details] [diff] [review]: ----------------------------------------------------------------- Overall looks good. I would like to see the const issue fixed, and the shallow copy explained (or the operator= removed) before I r+ this. ::: content/media/TextTrack.cpp @@ +58,5 @@ > + if (this != &aTrack) { > + mParent = aTrack.mParent; > + mKind = aTrack.mKind; > + mMode = aTrack.mMode; > + mCueList = aTrack.mCueList; This doesn't cause the cuelist etc to be copied, they're references. i.e. the original and the assignee will point to the same (non-const) lists here. Is that OK? What calls this code? I can't see any use of this code, all the assignments I can find of TextTracks are for TextTrack*, not TextTrack&. Can we delete this? ::: content/media/TextTrackList.cpp @@ +76,5 @@ > void > TextTrackList::RemoveTextTrack(const TextTrack& aTrack) > { > + if (mTextTracks.RemoveElement(&aTrack)) { > + DispatchTrackEvent(const_cast<TextTrack*>(&aTrack), *Do not* break the contract of const. If something should not be const, make it not const, don't have the contract (the method declaration) lie about it. People rely on const meaning const. The track attribute of the TrackEvent is readonly, can you make DispatchTrackEvent take a const TextTrack* instead of casting away the const here? ::: content/media/test/test_trackevent.html @@ +18,5 @@ > + function() { > + var label, language, kind; > + > + var video = document.createElement("video"); > + video.src = "seek.webm"; Do you need a video here for the tracks to be added? Remove it if not. If you need a video here, include manifest.js in the <head>, and call getPlayableVideo(gSmallTests), so that this test works if webm is disabled. @@ +35,5 @@ > + ok(!event.cancelable, "Event shouldn't be cancelable!"); > + textTrack = event.track; > + isnot(textTrack, null, "TextTrack should not be null"); > + is(textTrack.label, label, "Label should be set to "+ label); > + is(textTrack.language, language, "Language should be" + language); nit: indentation here, and trailing spaces in the file.
Attachment #806439 - Flags: review?(cpearce) → review-
(In reply to Chris Pearce (:cpearce) from comment #7) > Do you need a video here for the tracks to be added? Remove it if not. All of the text track tests right now are using a video and they don't really need one ASFAIK. We should probably open a bug to remove these if you think we should.
(In reply to Rick Eyre (:reyre) from comment #8) > (In reply to Chris Pearce (:cpearce) from comment #7) > > > Do you need a video here for the tracks to be added? Remove it if not. > > All of the text track tests right now are using a video and they don't > really need > one ASFAIK. We should probably open a bug to remove these if you think we > should. Please do.
(In reply to Chris Pearce (:cpearce) from comment #9) > Please do. Filed as bug 918289.
> Overall looks good. I would like to see the const issue fixed, and the > shallow copy explained (or the operator= removed) before I r+ this. > > ::: content/media/TextTrack.cpp > @@ +58,5 @@ > > + if (this != &aTrack) { > > + mParent = aTrack.mParent; > > + mKind = aTrack.mKind; > > + mMode = aTrack.mMode; > > + mCueList = aTrack.mCueList; Basically dead code on something i was trying with the earlier idl definition before i changed it. It shouldn't have been included. > ::: content/media/TextTrackList.cpp > @@ +76,5 @@ > > void > > TextTrackList::RemoveTextTrack(const TextTrack& aTrack) > > { > > + if (mTextTracks.RemoveElement(&aTrack)) { > > + DispatchTrackEvent(const_cast<TextTrack*>(&aTrack), > > *Do not* break the contract of const. If something should not be const, make > it not const, don't have the contract (the method declaration) lie about it. > People rely on const meaning const. > > The track attribute of the TrackEvent is readonly, can you make > DispatchTrackEvent take a const TextTrack* instead of casting away the const > here? > The problem here is that TrackEventInitInitializer's |mTrack| is an nsRefPtr<TrackEvent> and nsRefPtr<T> only has operator=(T*). Thus, the cast will still be necessary in order to initialize |mTrack| despite making DispatchTrackEvent to take const TextTrack*.
Comment on attachment 806439 [details] [diff] [review] TrackEvent.patch Review of attachment 806439 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/test/test_trackevent.html @@ +34,5 @@ > + ok(!event.bubbles, "Event shouldn't bubble!"); > + ok(!event.cancelable, "Event shouldn't be cancelable!"); > + textTrack = event.track; > + isnot(textTrack, null, "TextTrack should not be null"); > + is(textTrack.label, label, "Label should be set to "+ label); Instead of checking if label, language, and kind are equal should we be checking that textTrack === event.track? This was we ensure that we are testing that the event was fired at the exact track instead of one that just looks like it.
> Instead of checking if label, language, and kind are equal should we be > checking that textTrack === event.track? This was we ensure that we are > testing that the event was fired at the exact track instead of one that just > looks like it. Good spot. Thanks. Just added it.
Attached patch Impl v2 (obsolete) (deleted) — — Splinter Review
In addition to my comments in comment 11 on why making DispatchTrackEvent to take const TextTrack* wont work, i removed the const on |TextTrackList::RemoveTextTrack| since |RemoveElement| makes the arg const anyway, so there is no danger of it being modified.
Attachment #806439 - Attachment is obsolete: true
Attachment #808671 - Flags: review?(cpearce)
Comment on attachment 808671 [details] [diff] [review] Impl v2 Review of attachment 808671 [details] [diff] [review]: ----------------------------------------------------------------- r=cpearce, assuming you fix the test. ::: content/media/TextTrackList.cpp @@ +89,5 @@ > + eventInit.mBubbles = false; > + eventInit.mCancelable = false; > + eventInit.mTrack = aTrack; > + nsRefPtr<TrackEvent> trackEvent = > + TrackEvent::Constructor(this, aEventName, eventInit); More of a question than a review comment: I don't see the C++ definition for the TrackEvent functions, does the WebIDL bindings generator make them for you? ::: content/media/test/test_trackevent.html @@ +48,5 @@ > + } > + > + //TODO: Tests for removetrack event to be added along with bug 882677 > + > + SimpleTest.finish(); Won't the finish() call run before the TrackEvents arrive (since I'd expect DispatchDOMEvent() to be async, and put an event to run the DOM TrackEvent in JS into the Gecko event queue, and that Gecko event won't run until the JS context that called video.addTextTrack() exits), and so you'll be calling ok() after the test has finished? I think you should call finish() in the addtrack handler, once you're sure all TrackEvents have been received (you can probably just count the number of times onaddtrack is called).
Attachment #808671 - Flags: review?(cpearce) → review+
(In reply to Chris Pearce (:cpearce) from comment #15) > Comment on attachment 808671 [details] [diff] [review] > Impl v2 > > Review of attachment 808671 [details] [diff] [review]: > ----------------------------------------------------------------- > > r=cpearce, assuming you fix the test. > > ::: content/media/TextTrackList.cpp > @@ +89,5 @@ > More of a question than a review comment: I don't see the C++ definition for > the TrackEvent functions, does the WebIDL bindings generator make them for > you? > Thanks for the review...and yes, WebIDL bindings generator does take care of the definitions. See bug 900904. I'll amend the tests accordingly.
(In reply to Chris Pearce (:cpearce) from comment #15) > Comment on attachment 808671 [details] [diff] [review] > Impl v2 > > Review of attachment 808671 [details] [diff] [review]: > ----------------------------------------------------------------- > > r=cpearce, assuming you fix the test. > > ::: content/media/TextTrackList.cpp > @@ +89,5 @@ > More of a question than a review comment: I don't see the C++ definition for > the TrackEvent functions, does the WebIDL bindings generator make them for > you? Thanks for the review...and yes, WebIDL bindings generator does take care of the definitions. See bug 900904. I'll amend the tests accordingly.
> Won't the finish() call run before the TrackEvents arrive (since I'd expect > DispatchDOMEvent() to be async, and put an event to run the DOM TrackEvent > in JS into the Gecko event queue, and that Gecko event won't run until the > JS context that called video.addTextTrack() exits. On second thoughts, no. The finish() will run after the loop ends. nsDOMEventTargetHelper::DispatchDOMEvent is synchronous as far as i can tell, otherwise then encapsulating TrackEvent in an AsyncDOMEvent would be the way to go. Why do you expect it to be async?
Flags: needinfo?(cpearce)
The spec says that we should "queue a task to fire the TrackEvent": http://www.whatwg.org/specs/web-apps/current-work/multipage/the-video-element.html#dom-media-addtexttrack The task will be added to the event loops task queue which executes asynchrnously: http://www.whatwg.org/specs/web-apps/current-work/multipage/webappapis.html#task-queue However, I'm not aware of how DispatchDOMEvent() works, whether it's asynchronous or not.
> The task will be added to the event loops task queue which executes > asynchrnously: > http://www.whatwg.org/specs/web-apps/current-work/multipage/webappapis. > html#task-queue > Not all task queues execute asynchronously. "For example, a user agent could have one task queue for mouse and key events (the user interaction task source), and another for everything else" http://www.whatwg.org/specs/web-apps/current-work/multipage/webappapis.html#dfnReturnLink-3 ...but we have mouse events which are synchronous eg: http://www.w3.org/TR/DOM-Level-3-Events/#event-type-click. It would really help if the spec had clarified whether TrackEvent was sync or async... or it was left implementation dependent(?), in which case, this is sync.
(In reply to Andrew Quartey [:drexler] from comment #20) > Not all task queues execute asynchronously. > > "For example, a user agent could have one task queue for mouse and key > events (the user interaction task source), and another for everything else" > http://www.whatwg.org/specs/web-apps/current-work/multipage/webappapis. > html#dfnReturnLink-3 > > ...but we have mouse events which are synchronous eg: > http://www.w3.org/TR/DOM-Level-3-Events/#event-type-click. This part here makes me think that they are async: http://www.whatwg.org/specs/web-apps/current-work/multipage/webappapis.html#task-queue " Asynchronously dispatching an Event object at a particular EventTarget object is often done by a dedicated task. Note: Not all events are dispatched using the task queue, many are dispatched synchronously during other tasks." That makes me think that by definition if it is in a task queue the event will be async. > It would really help if the spec had clarified whether TrackEvent was sync > or async... or it was left implementation dependent(?), in which case, this > is sync. You're right and it's not very clear. I think for now we can go with what ever DispatchDOMEvent gives us. Do you know if DispatchDOMEvent gives a synchronous Event Andrew?
(In reply to Andrew Quartey [:drexler] from comment #18) > > Won't the finish() call run before the TrackEvents arrive (since I'd expect > > DispatchDOMEvent() to be async, and put an event to run the DOM TrackEvent > > in JS into the Gecko event queue, and that Gecko event won't run until the > > JS context that called video.addTextTrack() exits. > > On second thoughts, no. The finish() will run after the loop ends. > nsDOMEventTargetHelper::DispatchDOMEvent is synchronous as far as i can > tell Yeah, looks like it. > Why do you expect it to be async? Because most of the other media events are dispatched async. If a function you call dispatched events synchronously, event listeners can run inside a call to an web API function, and the listeners could make changes making the calling code inconsistent; limiting sync events makes it easier to reason about code for web devs (and us!). (In reply to Rick Eyre (:reyre) from comment #19) > The spec says that we should "queue a task to fire the TrackEvent" This means that it should be run asynchronously. i.e. the current JS context (the code calling the loop in this case) should finish executing before the code to dispatch the event runs. You can assume we have one task queue. (In reply to Andrew Quartey [:drexler] from comment #18) > otherwise then encapsulating TrackEvent in an AsyncDOMEvent would be the way to go Please make this change and re-request review.
Flags: needinfo?(cpearce)
Comment on attachment 808671 [details] [diff] [review] Impl v2 Review of attachment 808671 [details] [diff] [review]: ----------------------------------------------------------------- The track events need to be dispatched async, and the test needs to be changed to reflect that. Sorry I missed that the first time! ::: content/media/TextTrackList.cpp @@ +89,5 @@ > + eventInit.mBubbles = false; > + eventInit.mCancelable = false; > + eventInit.mTrack = aTrack; > + nsRefPtr<TrackEvent> trackEvent = > + TrackEvent::Constructor(this, aEventName, eventInit); More of a question than a review comment: I don't see the C++ definition for the TrackEvent functions, does the WebIDL bindings generator make them for you? ::: content/media/test/test_trackevent.html @@ +48,5 @@ > + } > + > + //TODO: Tests for removetrack event to be added along with bug 882677 > + > + SimpleTest.finish(); Won't the finish() call run before the TrackEvents arrive (since I'd expect DispatchDOMEvent() to be async, and put an event to run the DOM TrackEvent in JS into the Gecko event queue, and that Gecko event won't run until the JS context that called video.addTextTrack() exits), and so you'll be calling ok() after the test has finished? I think you should call finish() in the addtrack handler, once you're sure all TrackEvents have been received (you can probably just count the number of times onaddtrack is called).
Attachment #808671 - Flags: review+ → review-
Attached patch Impl v3 (deleted) — — Splinter Review
Using an nsAsyncDOMEvent didn't pan out as i thought - TextTrackList is not an nsINode - so created a runnable object, TrackEventRunner, which dispatches the TrackEvent when run on the main thread. In addition, amended tests to reflect that TrackEvent is now async.
Attachment #808671 - Attachment is obsolete: true
Attachment #811412 - Flags: review?(cpearce)
Attachment #811412 - Flags: review?(cpearce) → review+
Requesting a DOM peer review to address test failures raised in comment 27.
Attachment #813508 - Flags: review?(bugs)
(In reply to Ed Morley [:edmorley UTC+1] from comment #26) > Backed out for compilation failures: > https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=9ee3b91945ce > > https://hg.mozilla.org/integration/mozilla-inbound/rev/4d1985390599 Issue was the order of the initialization list for the runnable i added. i'll fix and repush pending Olli's review for the other follow-ups.
Attachment #813508 - Flags: review?(bugs) → review+
https://hg.mozilla.org/mozilla-central/rev/a3d6065cb0c5 https://hg.mozilla.org/mozilla-central/rev/6cfba9b1f6d4
Status: NEW → RESOLVED
Closed: 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: