Bug 83858

Summary: The cuechange event on track element is fired synchronously rather than queued
Product: WebKit Reporter: Anna Cavender <annacc>
Component: MediaAssignee: Anna Cavender <annacc>
Status: RESOLVED FIXED    
Severity: Normal CC: eric.carlson, feature-media-reviews, pnormand, vcarbune, webkit-bug-importer, webkit.review.bot
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
assert track element and remove fireCueChangeEvent() eric.carlson: review+, annacc: commit-queue-

Description Anna Cavender 2012-04-12 21:25:46 PDT
4.8.10.8 Playing the media resource: http://www.whatwg.org/specs/web-apps/current-work/#playing-the-media-resource "When the current playback position of a media element changes..." specifies how events are to be sorted and filtered before dispatch.

Currently, the cuechange event on track elements is fired synchronously rather than queued along with the other events in the queueing algorithm.
Comment 1 Anna Cavender 2012-04-12 21:57:03 PDT
Created attachment 137037 [details]
Patch
Comment 2 Eric Carlson 2012-04-13 11:20:13 PDT
Comment on attachment 137037 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=137037&action=review

Nice fix, thanks!

> Source/WebCore/html/HTMLMediaElement.cpp:-1197
> -            affectedTracks[i]->fireCueChangeEvent();

It looks like fireCueChangeEvent will not be used after this change. If so, it should be removed.

> Source/WebCore/html/HTMLMediaElement.cpp:1197
> +        if (affectedTracks[i]->trackType() == TextTrack::TrackElement) {
> +            RefPtr<Event> event = Event::create(eventNames().cuechangeEvent, false, false);
> +            event->setTarget(static_cast<LoadableTextTrack*>(affectedTracks[i])->trackElement());

I think it would be safer to put the HTMLTrackElement in a local so you can ASSERT it value before setting the event target.
Comment 3 Radar WebKit Bug Importer 2012-04-13 11:20:52 PDT
<rdar://problem/11246028>
Comment 4 Anna Cavender 2012-04-13 13:41:14 PDT
Comment on attachment 137037 [details]
Patch

Thanks Eric!

View in context: https://bugs.webkit.org/attachment.cgi?id=137037&action=review

>> Source/WebCore/html/HTMLMediaElement.cpp:-1197
>> -            affectedTracks[i]->fireCueChangeEvent();
> 
> It looks like fireCueChangeEvent will not be used after this change. If so, it should be removed.

Good call.

>> Source/WebCore/html/HTMLMediaElement.cpp:1197
>> +            event->setTarget(static_cast<LoadableTextTrack*>(affectedTracks[i])->trackElement());
> 
> I think it would be safer to put the HTMLTrackElement in a local so you can ASSERT it value before setting the event target.

Another good call.  Thanks.
Comment 5 Anna Cavender 2012-04-13 13:42:03 PDT
Created attachment 137139 [details]
assert track element and remove fireCueChangeEvent()
Comment 6 Anna Cavender 2012-04-13 14:48:05 PDT
Committed as http://trac.webkit.org/changeset/114171
Comment 7 Alexey Proskuryakov 2012-04-13 16:05:03 PDT
*** Bug 83878 has been marked as a duplicate of this bug. ***